From 2caf00deb0c764cd16492da18857e1667d6711cd Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Fri, 3 Nov 2023 11:19:16 +0100 Subject: [PATCH] fix(client/v2): fix marshalling of queries with any (#18309) --- client/debug/main.go | 1 + client/v2/autocli/app.go | 3 +-- client/v2/go.mod | 2 +- x/tx/CHANGELOG.md | 18 ++++++++++------ x/tx/go.mod | 2 +- x/tx/signing/aminojson/any.go | 16 ++------------ x/tx/signing/aminojson/json_marshal.go | 16 +++++++++++--- x/tx/signing/aminojson/options.go | 27 ++++++++++++++++++++++- x/tx/signing/aminojson/options_test.go | 30 ++++++++++++++++++++++++++ 9 files changed, 87 insertions(+), 28 deletions(-) create mode 100644 x/tx/signing/aminojson/options_test.go diff --git a/client/debug/main.go b/client/debug/main.go index c328558dd569..508ab43e3bbb 100644 --- a/client/debug/main.go +++ b/client/debug/main.go @@ -86,6 +86,7 @@ func getCodecInterfaceImpls() *cobra.Command { Short: "List the registered type URLs for the provided interface", Long: "List the registered type URLs that can be used for the provided interface name using the application codec", Example: fmt.Sprintf("%s debug codec list-implementations cosmos.crypto.PubKey", version.AppName), + Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { clientCtx := client.GetClientContextFromCmd(cmd) impls := clientCtx.Codec.InterfaceRegistry().ListImplementations(args[0]) diff --git a/client/v2/autocli/app.go b/client/v2/autocli/app.go index 052b660e1573..2daa2e4ae151 100644 --- a/client/v2/autocli/app.go +++ b/client/v2/autocli/app.go @@ -1,7 +1,6 @@ package autocli import ( - "github.com/cosmos/gogoproto/proto" "github.com/spf13/cobra" "google.golang.org/grpc" "google.golang.org/protobuf/reflect/protoregistry" @@ -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, Keyring: appOptions.Keyring, AddressCodec: appOptions.ClientCtx.AddressCodec, ValidatorAddressCodec: appOptions.ClientCtx.ValidatorAddressCodec, diff --git a/client/v2/go.mod b/client/v2/go.mod index 089f964c3737..037238811a01 100644 --- a/client/v2/go.mod +++ b/client/v2/go.mod @@ -11,7 +11,6 @@ require ( github.com/cockroachdb/errors v1.11.1 github.com/cosmos/cosmos-proto v1.0.0-beta.3 github.com/cosmos/cosmos-sdk v0.51.0 - github.com/cosmos/gogoproto v1.4.11 github.com/spf13/cobra v1.7.0 github.com/spf13/pflag v1.0.5 google.golang.org/grpc v1.59.0 @@ -47,6 +46,7 @@ require ( github.com/cosmos/cosmos-db v1.0.0 // indirect github.com/cosmos/go-bip39 v1.0.0 // indirect github.com/cosmos/gogogateway v1.2.0 // indirect + github.com/cosmos/gogoproto v1.4.11 // indirect github.com/cosmos/iavl v1.0.0 // indirect github.com/cosmos/ics23/go v0.10.0 // indirect github.com/cosmos/ledger-cosmos-go v0.13.3 // indirect diff --git a/x/tx/CHANGELOG.md b/x/tx/CHANGELOG.md index 44157179e329..719c15cddb37 100644 --- a/x/tx/CHANGELOG.md +++ b/x/tx/CHANGELOG.md @@ -31,6 +31,12 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## [Unreleased] +## v0.12.0 + +### Improvements + +* [#18309](https://github.com/cosmos/cosmos-sdk/pull/18309) Update encoder so that amino types default to msg type url. + ## v0.11.0 ### Improvements @@ -87,18 +93,18 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements * [#15871](https://github.com/cosmos/cosmos-sdk/pull/15871) - * `HandlerMap` now has a `DefaultMode()` getter method - * Textual types use `signing.ProtoFileResolver` instead of `protoregistry.Files` + * `HandlerMap` now has a `DefaultMode()` getter method + * Textual types use `signing.ProtoFileResolver` instead of `protoregistry.Files` ## v0.6.0 ### API Breaking * [#15709](https://github.com/cosmos/cosmos-sdk/pull/15709): - * `GetSignersContext` has been renamed to `signing.Context` - * `GetSigners` now returns `[][]byte` instead of `[]string` - * `GetSignersOptions` has been renamed to `signing.Options` and requires `address.Codec`s for account and validator addresses - * `GetSignersOptions.ProtoFiles` has been renamed to `signing.Options.FileResolver` + * `GetSignersContext` has been renamed to `signing.Context` + * `GetSigners` now returns `[][]byte` instead of `[]string` + * `GetSignersOptions` has been renamed to `signing.Options` and requires `address.Codec`s for account and validator addresses + * `GetSignersOptions.ProtoFiles` has been renamed to `signing.Options.FileResolver` ### Bug Fixes diff --git a/x/tx/go.mod b/x/tx/go.mod index db308f3ccf16..e1b0240908cd 100644 --- a/x/tx/go.mod +++ b/x/tx/go.mod @@ -8,6 +8,7 @@ require ( cosmossdk.io/errors v1.0.0 cosmossdk.io/math v1.1.3-rc.1 github.com/cosmos/cosmos-proto v1.0.0-beta.3 + github.com/cosmos/gogoproto v1.4.11 github.com/google/go-cmp v0.6.0 github.com/google/gofuzz v1.2.0 github.com/iancoleman/strcase v0.3.0 @@ -20,7 +21,6 @@ require ( ) require ( - github.com/cosmos/gogoproto v1.4.11 // indirect github.com/davecgh/go-spew v1.1.1 // indirect github.com/golang/protobuf v1.5.3 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect diff --git a/x/tx/signing/aminojson/any.go b/x/tx/signing/aminojson/any.go index 4e0e46954bcb..c7e304a46977 100644 --- a/x/tx/signing/aminojson/any.go +++ b/x/tx/signing/aminojson/any.go @@ -50,13 +50,7 @@ func marshalAny(enc *Encoder, message protoreflect.Message, writer io.Writer) er protoMessage = valueMsg.ProtoReflect() } - _, named := getMessageAminoName(protoMessage.Descriptor().Options()) - if !named { - return fmt.Errorf("message %s is packed into an any field, so requires an amino.name annotation", - anyMsg.TypeUrl) - } - - return enc.beginMarshal(protoMessage, writer) + return enc.beginMarshal(protoMessage, writer, true) } const ( @@ -73,17 +67,11 @@ func marshalDynamic(enc *Encoder, message protoreflect.Message, writer io.Writer return errors.Wrapf(err, "can't resolve type URL %s", msgName) } - _, named := getMessageAminoName(desc.Options()) - if !named { - return fmt.Errorf("message %s is packed into an any field, so requires an amino.name annotation", - msgName) - } - valueMsg := dynamicpb.NewMessageType(desc.(protoreflect.MessageDescriptor)).New().Interface() err = proto.Unmarshal(msgBytes, valueMsg) if err != nil { return err } - return enc.beginMarshal(valueMsg.ProtoReflect(), writer) + return enc.beginMarshal(valueMsg.ProtoReflect(), writer, true) } diff --git a/x/tx/signing/aminojson/json_marshal.go b/x/tx/signing/aminojson/json_marshal.go index d3ca5822b54d..aa8aa76b945e 100644 --- a/x/tx/signing/aminojson/json_marshal.go +++ b/x/tx/signing/aminojson/json_marshal.go @@ -156,7 +156,7 @@ func (enc Encoder) DefineTypeEncoding(typeURL string, encoder MessageEncoder) En // Marshal serializes a protobuf message to JSON. func (enc Encoder) Marshal(message proto.Message) ([]byte, error) { buf := &bytes.Buffer{} - err := enc.beginMarshal(message.ProtoReflect(), buf) + err := enc.beginMarshal(message.ProtoReflect(), buf, false) if enc.indent != "" { indentBuf := &bytes.Buffer{} @@ -170,8 +170,18 @@ func (enc Encoder) Marshal(message proto.Message) ([]byte, error) { return buf.Bytes(), err } -func (enc Encoder) beginMarshal(msg protoreflect.Message, writer io.Writer) error { - name, named := getMessageAminoName(msg.Descriptor().Options()) +func (enc Encoder) beginMarshal(msg protoreflect.Message, writer io.Writer, isAny bool) error { + var ( + name string + named bool + ) + + if isAny { + name, named = getMessageAminoNameAny(msg), true + } else { + name, named = getMessageAminoName(msg) + } + if named { _, err := fmt.Fprintf(writer, `{"type":"%s","value":`, name) if err != nil { diff --git a/x/tx/signing/aminojson/options.go b/x/tx/signing/aminojson/options.go index 4e17060c78f3..0eecedb88731 100644 --- a/x/tx/signing/aminojson/options.go +++ b/x/tx/signing/aminojson/options.go @@ -2,6 +2,7 @@ package aminojson import ( cosmos_proto "github.com/cosmos/cosmos-proto" + gogoproto "github.com/cosmos/gogoproto/proto" "github.com/iancoleman/strcase" "github.com/pkg/errors" "google.golang.org/protobuf/proto" @@ -12,14 +13,38 @@ import ( // getMessageAminoName returns the amino name of a message if it has been set by the `amino.name` option. // If the message does not have an amino name, then the function returns false. -func getMessageAminoName(messageOptions proto.Message) (string, bool) { +func getMessageAminoName(msg protoreflect.Message) (string, bool) { + messageOptions := msg.Descriptor().Options() if proto.HasExtension(messageOptions, amino.E_Name) { name := proto.GetExtension(messageOptions, amino.E_Name) return name.(string), true } + return "", false } +// getMessageAminoName returns the amino name of a message if it has been set by the `amino.name` option. +// If the message does not have an amino name, then it returns the msg url. +// If it cannot get the msg url, then it returns false. +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 "" +} + // omitEmpty returns true if the field should be omitted if empty. Empty field omission is the default behavior. func omitEmpty(field protoreflect.FieldDescriptor) bool { opts := field.Options() diff --git a/x/tx/signing/aminojson/options_test.go b/x/tx/signing/aminojson/options_test.go new file mode 100644 index 000000000000..8038827f0f7f --- /dev/null +++ b/x/tx/signing/aminojson/options_test.go @@ -0,0 +1,30 @@ +package aminojson + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "cosmossdk.io/x/tx/signing/aminojson/internal/testpb" +) + +func Test_getMessageAminoName(t *testing.T) { + msg := &testpb.ABitOfEverything{} + name, ok := getMessageAminoName(msg.ProtoReflect()) + require.True(t, ok) + require.Equal(t, "ABitOfEverything", name) + + secondMsg := &testpb.Duration{} + _, ok = getMessageAminoName(secondMsg.ProtoReflect()) + require.False(t, ok) +} + +func Test_getMessageAminoNameAny(t *testing.T) { + msg := &testpb.ABitOfEverything{} + name := getMessageAminoNameAny(msg.ProtoReflect()) + require.Equal(t, "ABitOfEverything", name) + + secondMsg := &testpb.Duration{} + name = getMessageAminoNameAny(secondMsg.ProtoReflect()) + require.Equal(t, "/testpb.Duration", name) +}