fix(client/v2): fix marshalling of queries with any #18309
Conversation
julienrbrt
added
the
backport/v0.50.x
|
Warning Rate Limit Exceeded@julienrbrt has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 18 minutes and 2 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per repository. WalkthroughThe changes primarily revolve around the update and improvement of the encoder in the project. The encoder now defaults amino types to the message type URL. The modifications also include changes in the handling of Changes
Poem
TipsChat with CodeRabbit Bot (
|
github-prbot
requested review from
a team,
kocubinski and
atheeshp
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- client/v2/go.mod
- x/tx/go.mod
Files selected for processing (5)
- client/v2/autocli/app.go (2 hunks)
- x/tx/CHANGELOG.md (1 hunks)
- x/tx/signing/aminojson/any.go (2 hunks)
- x/tx/signing/aminojson/json_marshal.go (2 hunks)
- x/tx/signing/aminojson/options.go (2 hunks)
Files skipped from review due to trivial changes (2)
- x/tx/CHANGELOG.md
- x/tx/signing/aminojson/any.go
Additional comments: 7
client/v2/autocli/app.go (2)
1-6: The import of
github.com/cosmos/gogoproto/protopackage has been removed. Ensure that this package is not used elsewhere in the file.65-71: The
FileResolverfield of theBuilderstruct has been changed fromproto.HybridResolvertoappOptions.ClientCtx.InterfaceRegistry. This implies a change in the type ofappOptions.ClientCtx.InterfaceRegistry. Ensure that this change is compatible with the rest of the codebase.x/tx/signing/aminojson/options.go (3)
2-8: The new import statement for the package "github.com/cosmos/gogoproto/proto" is introduced. Ensure that this package is included in your dependencies.
16-24: The function
getMessageAminoNamenow accepts aprotoreflect.Messageparameter instead ofproto.Message. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.29-46: The function
getMessageAminoNameAnynow accepts aprotoreflect.Messageparameter instead ofproto.Messageand returns a string instead of a string and a boolean. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.x/tx/signing/aminojson/json_marshal.go (2)
- 156-162: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [156-171]
The
Marshalfunction has been updated to include afalseargument in thebeginMarshalfunction call. This change is likely due to the addition of theisAnyparameter in thebeginMarshalfunction. Ensure that this change does not affect other parts of the codebase that use theMarshalfunction.
- 173-187: The
beginMarshalfunction now accepts an additionalisAnyparameter. This parameter is used to determine how thenameandnamedvariables are set. IfisAnyistrue, thegetMessageAminoNameAnyfunction is used to setnameandnamedis set totrue. Otherwise, thegetMessageAminoNamefunction is used to setnameandnamed. This change allows for different handling of the message based on whether it is anAnymessage or not. Ensure that this change does not affect other parts of the codebase that use thebeginMarshalfunction.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- client/v2/go.mod
- x/tx/go.mod
Files selected for processing (5)
- client/v2/autocli/app.go (2 hunks)
- x/tx/CHANGELOG.md (1 hunks)
- x/tx/signing/aminojson/any.go (2 hunks)
- x/tx/signing/aminojson/json_marshal.go (2 hunks)
- x/tx/signing/aminojson/options.go (2 hunks)
Files skipped from review due to trivial changes (2)
- x/tx/CHANGELOG.md
- x/tx/signing/aminojson/any.go
Additional comments: 7
client/v2/autocli/app.go (2)
1-6: The removal of the
github.com/cosmos/gogoproto/protopackage import is fine as long as there are no references to it in the rest of the file.65-71: The change of
FileResolverfromproto.HybridResolvertoappOptions.ClientCtx.InterfaceRegistrychanges the type ofFileResolver. Ensure that all usages ofFileResolverare compatible with the new type.- FileResolver: proto.HybridResolver, + FileResolver: appOptions.ClientCtx.InterfaceRegistry,x/tx/signing/aminojson/json_marshal.go (2)
156-160: The
Marshalfunction has been updated to include a newisAnyparameter in thebeginMarshalfunction call. This allows for different handling of the message based on whether it is anAnymessage or not. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.173-187: The
beginMarshalfunction has been updated to handle the newisAnyparameter and determine thenameandnamedvalues accordingly. This change allows for different handling of the message based on whether it is anAnymessage or not. Thenamedvariable and associated error check have been removed, and theenc.beginMarshalfunction is now called with an additional boolean argument.x/tx/signing/aminojson/options.go (3)
2-8: The new import statement for the package "github.com/cosmos/gogoproto/proto" is introduced. Ensure that this package is included in your dependencies.
14-24: The function
getMessageAminoNamehas been updated to accept aprotoreflect.Messageparameter instead ofproto.Message. This change should be reflected wherever this function is called.- func getMessageAminoName(msg proto.Message) (string, bool) { + func getMessageAminoName(msg protoreflect.Message) (string, bool) {
- 26-46: A new function
getMessageAminoNameAnyhas been added. This function returns the amino name of a message if it has been set by theamino.nameoption, or the message URL if it cannot get the URL. This function seems to be a more flexible version ofgetMessageAminoNameand could be used in cases where the amino name might not be set.func getMessageAminoNameAny(msg protoreflect.Message) string { messageOptions := msg.Descriptor().Options() if proto.HasExtension(messageOptions, amino.E_Name) { name := proto.GetExtension(messageOptions, amino.E_Name) return name.(string) } msgURL := "/" + string(msg.Descriptor().FullName()) if msgURL != "/" { return msgURL } if m, ok := msg.(gogoproto.Message); ok { return "/" + gogoproto.MessageName(m) } return "" }
| @@ -66,7 +65,7 @@ func (appOptions AppOptions) EnhanceRootCommand(rootCmd *cobra.Command) error { | |||
| builder := &Builder{ | |||
| Builder: flag.Builder{ | |||
| TypeResolver: protoregistry.GlobalTypes, | |||
| FileResolver: proto.HybridResolver, | |||
| FileResolver: appOptions.ClientCtx.InterfaceRegistry, | |||
There was a problem hiding this comment.
Why favouring interface registry over the proto.HybridResolver?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- client/v2/go.mod
Files selected for processing (1)
- x/tx/signing/aminojson/options_test.go (1 hunks)
Additional comments: 1
x/tx/signing/aminojson/options_test.go (1)
- 1-30: The tests are well written and cover the main functionality of the functions they are testing. They also handle both positive and negative cases. Good job on that.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- client/debug/main.go (1 hunks)
Additional comments: 1
client/debug/main.go (1)
- 86-92: The
cobra.ExactArgs(1)ensures that the command requires exactly one argument. This is a good practice for command-line tools as it provides a clear interface for the user.
(cherry picked from commit 2caf00d) # Conflicts: # client/debug/main.go # client/v2/autocli/app.go # client/v2/go.mod # x/tx/CHANGELOG.md # x/tx/signing/aminojson/json_marshal.go
…#18354) Co-authored-by: Julien Robert <[email protected]>
Description
ref: #18256
While investigating #18256 I have noticed that doing the following, after having submitted a proposal without amino annotations, would lead to the following:
$ simd q gov proposal 1 cannot marshal response proposal:{id:1 messages:{[/cosmos.protocolpool.v1.MsgCommunityPoolSpend]:{authority:"cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn" recipient:"cosmos13fmxs08hvt6k2hez7vlk6qsgllykcumvp5u65z"}} status:PROPOSAL_STATUS_DEPOSIT_PERIOD final_tally_result:{yes_count:"0" abstain_count:"0" no_count:"0" no_with_veto_count:"0"} submit_time:{seconds:1698686061 nanos:741926444} deposit_end_time:{seconds:1698858861 nanos:741926444} total_deposit:{denom:"stake" amount:"10000"} metadata:"ipfs://CID" title:"autocli test" summary:"testing" proposer:"cosmos1acmdqwzyglh0fyh4u8sdsgp8ej50hfeafsy40d"}: message /cosmos.protocolpool.v1.MsgCommunityPoolSpend is packed into an any field, so requires an amino.name annotationIn the backported PR, x/tx changes will be reverted as x/tx is tagged from main. x/tx v0.12 will be tagged once this PR is merged.
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers 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...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
Refactor:
FileResolverfield in theBuilderstruct to useappOptions.ClientCtx.InterfaceRegistryinstead ofproto.HybridResolver, enhancing the type ofFileResolverfield.marshalAnyandmarshalDynamicfunctions by removing thenamedvariable and error check, and introducing theenc.beginMarshalfunction with an additional boolean argument.New Features:
getMessageAminoNameAnythat returns the amino name of a message or the message URL if the URL is not available.Style:
Marshalfunction in theEncoderstruct.Chores:
github.com/cosmos/gogoproto/protopackage inclient/v2/autocli/app.go.x/tx/signing/aminojson/options.go.