Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(client/v2): fix marshalling of queries with any #18309

Merged
merged 10 commits into from
Nov 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions client/debug/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Expand Down
3 changes: 1 addition & 2 deletions client/v2/autocli/app.go
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why favouring interface registry over the proto.HybridResolver?

Keyring: appOptions.Keyring,
AddressCodec: appOptions.ClientCtx.AddressCodec,
ValidatorAddressCodec: appOptions.ClientCtx.ValidatorAddressCodec,
Expand Down
2 changes: 1 addition & 1 deletion client/v2/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
18 changes: 12 additions & 6 deletions x/tx/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion x/tx/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
16 changes: 2 additions & 14 deletions x/tx/signing/aminojson/any.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -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)
}
16 changes: 13 additions & 3 deletions x/tx/signing/aminojson/json_marshal.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand All @@ -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 {
Expand Down
27 changes: 26 additions & 1 deletion x/tx/signing/aminojson/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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 {
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
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()
Expand Down
30 changes: 30 additions & 0 deletions x/tx/signing/aminojson/options_test.go
Original file line number Diff line number Diff line change
@@ -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)
}