feat(client/v2): implement version filtering using annotation #20083
Conversation
julienrbrt
added
the
backport/v0.50.x
julienrbrt
changed the title
WalkthroughWalkthroughThe recent updates focus on enhancing client functionalities by integrating a new version of cosmos-proto, refining version filtering, and improving command-line tools. Changes include serialization adjustments, simplified conditions for version checking, and new RPC methods. Additionally, there's an update in the handling of protobuf extensions and version parsing, alongside improved documentation on protobuf annotations and command functionalities. Changes
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (2)
Files skipped from review as they are similar to previous changes (1)
Additional Context UsedLanguageTool (40)
Path-based Instructions (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| func init() { | ||
| file_testpb_msg_proto_init() | ||
| md_MsgClawbackRequest = File_testpb_msg_proto.Messages().ByName("MsgClawbackRequest") | ||
| } |
There was a problem hiding this comment.
Initialization of MsgClawbackRequest descriptor should be thread-safe.
To ensure thread safety during the initialization of MsgClawbackRequest, consider using sync.Once or similar synchronization primitives to avoid potential race conditions in concurrent environments.
| // Range iterates over every populated field in an undefined order, | ||
| // calling f for each field descriptor and value encountered. | ||
| // Range returns immediately if f returns false. | ||
| // While iterating, mutating operations may only be performed | ||
| // on the current field descriptor. | ||
| func (x *fastReflection_MsgClawbackRequest) Range(f func(protoreflect.FieldDescriptor, protoreflect.Value) bool) { | ||
| } |
There was a problem hiding this comment.
Range method implementation is missing.
The Range method in fastReflection_MsgClawbackRequest is expected to iterate over fields of the message, but its implementation is missing. This method should be implemented to comply with the protoreflect.Message interface requirements.
| func (x *fastReflection_MsgClawbackRequest) Get(descriptor protoreflect.FieldDescriptor) protoreflect.Value { | ||
| switch descriptor.FullName() { | ||
| default: | ||
| if descriptor.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: testpb.MsgClawbackRequest")) | ||
| } | ||
| panic(fmt.Errorf("message testpb.MsgClawbackRequest does not contain field %s", descriptor.FullName())) | ||
| } |
There was a problem hiding this comment.
Handle extension fields appropriately.
The method Get in fastReflection_MsgClawbackRequest panics when encountering extension fields. If extensions are expected, modify the method to handle them correctly; otherwise, ensure that the panic condition is documented and justified.
| @@ -49,7 +44,7 @@ func (b *Builder) buildMethodCommandCommon(descriptor protoreflect.MethodDescrip | |||
| cmd := &cobra.Command{ | |||
| SilenceUsage: false, | |||
| Use: use, | |||
| Long: long, | |||
| Long: options.Long, | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The search results indicate that the direct use of options.Long in command setups is unique to client/v2/autocli/common.go and does not appear to be a common pattern throughout the rest of the project. This suggests that the change might not be consistent with other similar setups within the codebase.
client/v2/autocli/common.go: Unique instance of usingoptions.Longdirectly.
Analysis chain
Direct use of options.Long simplifies the command setup. Ensure this change is consistent with other similar setups.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistency of direct use of options fields in command setups across the project.
rg --type go 'Long: options.'
Length of output: 97
There was a problem hiding this comment.
Actionable comments posted: 1
Actionable comments outside the diff hunks (2)
client/v2/CHANGELOG.md (2)
Line range hint
53-53: Add a space after the period for better readability.- TxConfig should always be set in the `client.Context` in `root.go` of an app. + TxConfig should always be set in the `client.Context` in `root.go` of an app.
Line range hint
64-64: Add a comma after 'Instead' to improve the grammatical structure.- Instead client/v2 uses the address codecs present in the context + Instead, client/v2 uses the address codecs present in the context
There was a problem hiding this comment.
Actionable comments posted: 1
Actionable comments outside the diff hunks (8)
client/v2/README.md (8)
Line range hint
5-5: Correct the spelling mistake.- sidebar_position + sidebar-position
Line range hint
21-21: Ensure the sentence starts with an uppercase letter.- `autocli` generates CLI commands and flags for each method defined in your gRPC service. + `Autocli` generates CLI commands and flags for each method defined in your gRPC service.
Line range hint
39-41: Add spaces between sentences for better readability.- Ensure your app's modules implements the `appmodule.AppModule` interface. 2. (optional) Configure how... + Ensure your app's modules implements the `appmodule.AppModule` interface. 2. (optional) Configure how...
Line range hint
78-78: Start the sentence with an uppercase letter.- `autocli` uses a keyring for key name resolving and signing transactions. + `Autocli` uses a keyring for key name resolving and signing transactions.
Line range hint
81-81: Revise the sentence for grammatical correctness.- This provides a better UX as it allows to resolve key names directly from the keyring in all transactions and commands. + This provides a better UX as it allows resolving key names directly from the keyring in all transactions and commands.
Line range hint
126-126: Revise the sentence for grammatical correctness.- The `AutoCLIOptions()` method on your module allows to specify custom commands, sub-commands or flags for each service. + The `AutoCLIOptions()` method on your module allows specifying custom commands, sub-commands or flags for each service.
Line range hint
142-142: Consider adding commas for clarity.- Users can however use the `--no-proposal` flag to disable the proposal creation. + Users can, however, use the `--no-proposal` flag to disable the proposal creation.
Line range hint
173-173: Address the inconsistency in spelling variants (American vs British English).- ### Customising Flag Names + ### Customizing Flag Names
| * `sign-file` for signing a file. | ||
| * `verify-file` for verifying a previously signed file. |
There was a problem hiding this comment.
Ensure sentences start with an uppercase letter.
- * `sign-file` for signing a file.
- * `verify-file` for verifying a previously signed file.
+ * `Sign-file` for signing a file.
+ * `Verify-file` for verifying a previously signed file.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| * `sign-file` for signing a file. | |
| * `verify-file` for verifying a previously signed file. | |
| * `Sign-file` for signing a file. | |
| * `Verify-file` for verifying a previously signed file. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Actionable comments outside the diff hunks (2)
docs/build/building-modules/05-protobuf-annotations.md (2)
Line range hint
117-117: Correct the spelling of "Dont_OmitEmpty" to "Don't_OmitEmpty" to adhere to proper contraction usage.- ### Dont_OmitEmpty + ### Don't_OmitEmpty
Line range hint
158-158: Add a comma after "Thus" to improve the readability of the sentence.- Thus a JSON validity check needs to be in place at some point of the process. + Thus, a JSON validity check needs to be in place at some point of the process.
| @@ -58,7 +58,7 @@ Example of Int scalar: | |||
| https://github.com/cosmos/cosmos-sdk/blob/e8f28bf5db18b8d6b7e0d94b542ce4cf48fed9d6/proto/cosmos/gov/v1/gov.proto#L137 | |||
| ``` | |||
|
|
|||
| There are a few options for what can be provided as a scalar: cosmos.AddressString, cosmos.ValidatorAddressString, cosmos.ConsensusAddressString, cosmos.Int, cosmos.Dec. | |||
| There are a few options for what can be provided as a scalar: `cosmos.AddressString`, `cosmos.ValidatorAddressString`, `cosmos.ConsensusAddressString`, `cosmos.Int`, `cosmos.Dec`. | |||
There was a problem hiding this comment.
Add a space after each comma in the list of scalars to improve readability.
- There are a few options for what can be provided as a scalar: `cosmos.AddressString`, `cosmos.ValidatorAddressString`, `cosmos.ConsensusAddressString`, `cosmos.Int`, `cosmos.Dec`.
+ There are a few options for what can be provided as a scalar: `cosmos.AddressString`, `cosmos.ValidatorAddressString`, `cosmos.ConsensusAddressString`, `cosmos.Int`, `cosmos.Dec`.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| There are a few options for what can be provided as a scalar: `cosmos.AddressString`, `cosmos.ValidatorAddressString`, `cosmos.ConsensusAddressString`, `cosmos.Int`, `cosmos.Dec`. | |
| There are a few options for what can be provided as a scalar: `cosmos.AddressString`, `cosmos.ValidatorAddressString`, `cosmos.ConsensusAddressString`, `cosmos.Int`, `cosmos.Dec`. |
| @@ -68,9 +68,21 @@ | |||
| option (cosmos_proto.implements_interface) = "cosmos.auth.v1beta1.AccountI"; | |||
| ``` | |||
|
|
|||
| ## Method,Field,Message Added In | |||
There was a problem hiding this comment.
Insert a space after each comma in the section title for consistency and readability.
- ## Method,Field,Message Added In
+ ## Method, Field, Message Added InCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| ## Method,Field,Message Added In | |
| ## Method, Field, Message Added In |
|
|
||
| `method_added_in`, `field_added_in` and `message_added_in` are annotations to denotate to clients that a field has been supported in a later version. This is useful when new methods or fields are added in later versions and that the client needs to be aware of what it can call. | ||
|
|
||
| The annotation should be worded as follow: |
There was a problem hiding this comment.
Correct the phrase to "as follows" for grammatical accuracy.
- The annotation should be worded as follow:
+ The annotation should be worded as follows:Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| The annotation should be worded as follow: | |
| The annotation should be worded as follows: |
| ## Amino | ||
|
|
||
| The amino codec was removed in 0.50.0, this means there is not a need register `legacyAminoCodec`. To replace the amino codec, Amino protobuf annotations are used to provide information to the amino codec on how to encode and decode protobuf messages. | ||
| The amino codec was removed in `v0.50+`, this means there is not a need register `legacyAminoCodec`. To replace the amino codec, Amino protobuf annotations are used to provide information to the amino codec on how to encode and decode protobuf messages. |
There was a problem hiding this comment.
Add the missing preposition "to" to complete the phrase correctly.
- The amino codec was removed in `v0.50+`, this means there is not a need register `legacyAminoCodec`.
+ The amino codec was removed in `v0.50+`, this means there is not a need to register `legacyAminoCodec`.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| The amino codec was removed in `v0.50+`, this means there is not a need register `legacyAminoCodec`. To replace the amino codec, Amino protobuf annotations are used to provide information to the amino codec on how to encode and decode protobuf messages. | |
| The amino codec was removed in `v0.50+`, this means there is not a need to register `legacyAminoCodec`. To replace the amino codec, Amino protobuf annotations are used to provide information to the amino codec on how to encode and decode protobuf messages. |
| ## Amino | ||
|
|
||
| The amino codec was removed in 0.50.0, this means there is not a need register `legacyAminoCodec`. To replace the amino codec, Amino protobuf annotations are used to provide information to the amino codec on how to encode and decode protobuf messages. | ||
| The amino codec was removed in `v0.50+`, this means there is not a need register `legacyAminoCodec`. To replace the amino codec, Amino protobuf annotations are used to provide information to the amino codec on how to encode and decode protobuf messages. | ||
|
|
||
| :::note | ||
| Amino annotations are only used for backwards compatibility with amino. New modules are not required use amino annotations. |
There was a problem hiding this comment.
Insert the missing preposition "to" for grammatical correctness.
- New modules are not required use amino annotations.
+ New modules are not required to use amino annotations.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| Amino annotations are only used for backwards compatibility with amino. New modules are not required use amino annotations. | |
| Amino annotations are only used for backwards compatibility with amino. New modules are not required to use amino annotations. |
(cherry picked from commit b491335) # Conflicts: # client/v2/CHANGELOG.md # client/v2/README.md # client/v2/go.mod # client/v2/go.sum # client/v2/internal/offchain/msgSignArbitraryData.pulsar.go # tests/starship/tests/go.mod # tests/starship/tests/go.sum
Description
ref: cosmos/cosmos-proto#133 cosmos/cosmos-proto#133 (comment)
follow-up of #19377
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
ClawbackRPC method in theMsgservice.Enhancements
cosmos-proto.Refactor
Documentation