Skip to content

fix(client/v2): improve message parsing by replacing proto.Merge with cloneMessage #24722

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
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
4 changes: 4 additions & 0 deletions client/v2/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ Ref: https://keepachangelog.com/en/1.0.0/

# Changelog

### Bug Fixes

* [#24722](https://github.com/cosmos/cosmos-sdk/pull/24722) Fix msg parsing in when no pulsar file is present.

## [v2.0.0-beta.9](https://github.com/cosmos/cosmos-sdk/tree/client/v2.0.0-beta.9) - 2025-04-24

### Features
Expand Down
6 changes: 5 additions & 1 deletion client/v2/Makefile
Original file line number Diff line number Diff line change
@@ -1,2 +1,6 @@
codegen:
@(cd internal; buf generate)
@(cd internal; buf generate --template testpbpulsar/buf.gen.yaml)
@(cd internal; buf generate --template testpbgogo/buf.gen.yaml)
@(cd internal/testpbgogo; rm *_grpc.pb.go; rm *.pulsar.go)
@(cd internal; mv github.com/cosmos/cosmos-sdk/client/v2/internal/testpbgogo/* testpbgogo)
@(cd internal; rm -r github.com)
13 changes: 11 additions & 2 deletions client/v2/autocli/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"net"
"testing"

"github.com/cosmos/gogoproto/proto"
"github.com/spf13/cobra"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
Expand All @@ -15,14 +16,16 @@ import (
autocliv1 "cosmossdk.io/api/cosmos/autocli/v1"
reflectionv2alpha1 "cosmossdk.io/api/cosmos/base/reflection/v2alpha1"
"cosmossdk.io/client/v2/autocli/flag"
"cosmossdk.io/client/v2/internal/testpb"
"cosmossdk.io/client/v2/internal/testpbgogo"
testpb "cosmossdk.io/client/v2/internal/testpbpulsar"

"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/client/flags"
addresscodec "github.com/cosmos/cosmos-sdk/codec/address"
sdkkeyring "github.com/cosmos/cosmos-sdk/crypto/keyring"
sdk "github.com/cosmos/cosmos-sdk/types"
moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil"
"github.com/cosmos/cosmos-sdk/types/msgservice"
"github.com/cosmos/cosmos-sdk/x/bank"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
)
Expand Down Expand Up @@ -57,6 +60,7 @@ func initFixture(t *testing.T) *fixture {

interfaceRegistry := encodingConfig.Codec.InterfaceRegistry()
banktypes.RegisterInterfaces(interfaceRegistry)
msgservice.RegisterMsgServiceDesc(interfaceRegistry, &testpbgogo.MsgGogoOnly_serviceDesc)

clientCtx := client.Context{}.
WithKeyring(kr).
Expand All @@ -69,10 +73,15 @@ func initFixture(t *testing.T) *fixture {
WithChainID("autocli-test")

conn := &testClientConn{ClientConn: clientConn}

// using merged registry to get pulsar + gogo files
mergedFiles, err := proto.MergedRegistry()
assert.NilError(t, err)

b := &Builder{
Builder: flag.Builder{
TypeResolver: protoregistry.GlobalTypes,
FileResolver: protoregistry.GlobalFiles,
FileResolver: mergedFiles,
AddressCodec: addresscodec.NewBech32Codec("cosmos"),
ValidatorAddressCodec: addresscodec.NewBech32Codec("cosmosvaloper"),
ConsensusAddressCodec: addresscodec.NewBech32Codec("cosmosvalcons"),
Expand Down
61 changes: 58 additions & 3 deletions client/v2/autocli/msg.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (

gogoproto "github.com/cosmos/gogoproto/proto"
"github.com/spf13/cobra"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"
"google.golang.org/protobuf/types/dynamicpb"

Expand Down Expand Up @@ -161,7 +160,7 @@ func (b *Builder) BuildMsgMethodCommand(descriptor protoreflect.MethodDescriptor
// Here we use dynamicpb, to create a proto v1 compatible message.
// The SDK codec will handle protov2 -> protov1 (marshal)
msg := dynamicpb.NewMessage(input.Descriptor())
proto.Merge(msg, input.Interface())
cloneMessage(msg, input)

return clienttx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
}
Expand Down Expand Up @@ -217,11 +216,67 @@ func (b *Builder) handleGovProposal(
// Here we use dynamicpb, to create a proto v1 compatible message.
// The SDK codec will handle protov2 -> protov1 (marshal)
msg := dynamicpb.NewMessage(input.Descriptor())
proto.Merge(msg, input.Interface())
cloneMessage(msg, input)

if err := proposal.SetMsgs([]gogoproto.Message{msg}); err != nil {
return fmt.Errorf("failed to set msg in proposal %w", err)
}

return clienttx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), proposal)
}

// cloneMessage safely copies fields from src message to dst message.
// this avoids the proto.Merge issue with field descriptors from different repositories.
func cloneMessage(dst, src protoreflect.Message) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to test this method

Copy link
Member Author

Choose a reason for hiding this comment

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

i'll add a test 👍🏾

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a test: 6bdfb65.
It fails before with that error, passes after the fix.

// iterate through all populated fields in the source message
src.Range(func(fd protoreflect.FieldDescriptor, v protoreflect.Value) bool {
// get corresponding field in destination message
dstFd := dst.Descriptor().Fields().ByName(fd.Name())
if dstFd == nil {
panic(fmt.Sprintf("field %s not found in destination message", fd.Name()))
}

if fd.IsList() {
// handle repeated fields
srcList := v.List()
dstList := dst.Mutable(dstFd).List()
for i := 0; i < srcList.Len(); i++ {
listVal := srcList.Get(i)
if fd.Kind() == protoreflect.MessageKind {
// recursively clone message values
newMsg := dynamicpb.NewMessage(dstFd.Message())
cloneMessage(newMsg, listVal.Message())
dstList.Append(protoreflect.ValueOfMessage(newMsg))
} else {
// directly copy primitive values
dstList.Append(listVal)
}
}
} else if fd.IsMap() {
// handle map fields
srcMap := v.Map()
dstMap := dst.Mutable(dstFd).Map()
srcMap.Range(func(k protoreflect.MapKey, v protoreflect.Value) bool {
if fd.MapValue().Kind() == protoreflect.MessageKind {
// recursively clone message values
newMsg := dynamicpb.NewMessage(dstFd.MapValue().Message())
cloneMessage(newMsg, v.Message())
dstMap.Set(k, protoreflect.ValueOfMessage(newMsg))
} else {
// directly copy primitive values
dstMap.Set(k, v)
}
return true
})
} else if fd.Kind() == protoreflect.MessageKind {
// recursively clone nested messages
newMsg := dynamicpb.NewMessage(dstFd.Message())
cloneMessage(newMsg, v.Message())
dst.Set(dstFd, protoreflect.ValueOfMessage(newMsg))
} else {
// directly copy primitive values
dst.Set(dstFd, v)
}
return true
})
}
28 changes: 27 additions & 1 deletion client/v2/autocli/msg_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ import (

autocliv1 "cosmossdk.io/api/cosmos/autocli/v1"
bankv1beta1 "cosmossdk.io/api/cosmos/bank/v1beta1"
"cosmossdk.io/client/v2/internal/testpb"
"cosmossdk.io/client/v2/internal/testpbgogo"
testpb "cosmossdk.io/client/v2/internal/testpbpulsar"

"github.com/cosmos/cosmos-sdk/client"
)
Expand Down Expand Up @@ -124,6 +125,31 @@ func TestMsg(t *testing.T) {
assertNormalizedJSONEqual(t, out.Bytes(), goldenLoad(t, "msg-output.golden"))
}

// TestMsgGogo set same as previous, but the message are only gogoproto generated.
// There are no protov2 files registered in the global registry for those types.
func TestMsgGogo(t *testing.T) {
fixture := initFixture(t)
out, err := runCmd(fixture, buildCustomModuleMsgCommand(&autocliv1.ServiceCommandDescriptor{
Service: testpbgogo.MsgGogoOnly_serviceDesc.ServiceName, // using gogoproto only test msg
RpcCommandOptions: []*autocliv1.RpcCommandOptions{
{
RpcMethod: "Send",
Use: "send [a_coin] [str] [flags]",
Short: "Send coins from one account to another",
PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "a_coin"}, {ProtoField: "str"}},
// an_address should be automatically added
},
},
}), "send",
"1foo", "henlo",
"--from", "cosmos1y74p8wyy4enfhfn342njve6cjmj5c8dtl6emdk",
"--generate-only",
"--output", "json",
)
assert.NilError(t, err)
golden.Assert(t, out.String(), "msg-gogo-output.golden")
}

func TestMsgWithFlattenFields(t *testing.T) {
fixture := initFixture(t)

Expand Down
2 changes: 1 addition & 1 deletion client/v2/autocli/query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (
autocliv1 "cosmossdk.io/api/cosmos/autocli/v1"
queryv1beta1 "cosmossdk.io/api/cosmos/base/query/v1beta1"
basev1beta1 "cosmossdk.io/api/cosmos/base/v1beta1"
"cosmossdk.io/client/v2/internal/testpb"
testpb "cosmossdk.io/client/v2/internal/testpbpulsar"

"github.com/cosmos/cosmos-sdk/client"
)
Expand Down
4 changes: 2 additions & 2 deletions client/v2/autocli/testdata/help-deprecated.golden
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ Flags:
--a-bool
--a-coin cosmos.base.v1beta1.Coin
--a-consensus-address account address or key name
--a-message testpb.AMessage (json)
--a-message testpbpulsar.AMessage (json)
--a-validator-address account address or key name
--an-address account address or key name
--an-enum Enum (unspecified | one | two | five | neg-three) (default unspecified)
Expand Down Expand Up @@ -41,7 +41,7 @@ Flags:
--positional2 string
--positional3-varargs cosmos.base.v1beta1.Coin (repeated)
--shorthand-deprecated-field string
--some-messages testpb.AMessage (json) (repeated)
--some-messages testpbpulsar.AMessage (json) (repeated)
--str string
--strings strings
--timestamp timestamp (RFC 3339)
Expand Down
4 changes: 2 additions & 2 deletions client/v2/autocli/testdata/help-echo.golden
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ Flags:
--a-bool
--a-coin cosmos.base.v1beta1.Coin some random coin
--a-consensus-address account address or key name
--a-message testpb.AMessage (json)
--a-message testpbpulsar.AMessage (json)
--a-validator-address account address or key name
--an-address account address or key name
--an-enum Enum (unspecified | one | two | five | neg-three) (default unspecified)
Expand Down Expand Up @@ -42,7 +42,7 @@ Flags:
--page-offset uint
--page-reverse
-s, --shorthand-deprecated-field string (DEPRECATED: bad idea)
--some-messages testpb.AMessage (json) (repeated)
--some-messages testpbpulsar.AMessage (json) (repeated)
--str string
--strings strings
--timestamp timestamp (RFC 3339)
Expand Down
2 changes: 1 addition & 1 deletion client/v2/autocli/testdata/help-skip.golden
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
Querying commands for the testpb.Query service
Querying commands for the testpbpulsar.Query service

Usage:
test skipecho [flags]
Expand Down
4 changes: 2 additions & 2 deletions client/v2/autocli/testdata/help-toplevel.golden
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ Usage:

Available Commands:
completion Generate the autocompletion script for the specified shell
deprecatedecho Querying commands for the testpb.Query service
deprecatedecho Querying commands for the testpbpulsar.Query service
echo echo echos the value provided by the user
help Help about any command
skipecho Querying commands for the testpb.Query service
skipecho Querying commands for the testpbpulsar.Query service

Flags:
-h, --help help for test
Expand Down
1 change: 1 addition & 0 deletions client/v2/autocli/testdata/msg-gogo-output.golden
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"body":{"messages":[{"@type":"/testpbgogo.MsgRequestGogoOnly","str":"henlo","a_coin":{"denom":"foo","amount":"1"},"an_address":"cosmos1y74p8wyy4enfhfn342njve6cjmj5c8dtl6emdk","a_validator_address":""}],"memo":"","timeout_height":"0","unordered":false,"timeout_timestamp":null,"extension_options":[],"non_critical_extension_options":[]},"auth_info":{"signer_infos":[],"fee":{"amount":[],"gas_limit":"200000","payer":"","granter":""},"tip":null},"signatures":[]}
17 changes: 6 additions & 11 deletions client/v2/internal/buf.lock
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ deps:
- remote: buf.build
owner: cosmos
repository: cosmos-sdk
commit: 05419252bcc241ea8023acf1ed4cadc5
digest: shake256:1e54a48c19a8b59d35e0a7efa76402939f515f2d8005df099856f24c37c20a52800308f025abb8cffcd014d437b49707388aaca4865d9d063d8f25d5d4eb77d5
commit: 34ac2e8322d44db08830e553ad21b93c
digest: shake256:b0dd0de5ef770147002e290a9e9f8de8cc8727cbf932628892fbb9027d5ebec5c2a63c0e340e4ef7af72369ecdf51221dcc385ce2137afaa72532228f6999d74
- remote: buf.build
owner: cosmos
repository: gogo-proto
Expand All @@ -19,15 +19,10 @@ deps:
- remote: buf.build
owner: googleapis
repository: googleapis
commit: 7e6f6e774e29406da95bd61cdcdbc8bc
digest: shake256:fe43dd2265ea0c07d76bd925eeba612667cf4c948d2ce53d6e367e1b4b3cb5fa69a51e6acb1a6a50d32f894f054a35e6c0406f6808a483f2752e10c866ffbf73
commit: 751cbe31638d43a9bfb6162cd2352e67
digest: shake256:87f55470d9d124e2d1dedfe0231221f4ed7efbc55bc5268917c678e2d9b9c41573a7f9a557f6d8539044524d9fc5ca8fbb7db05eb81379d168285d76b57eb8a4
- remote: buf.build
owner: protocolbuffers
repository: wellknowntypes
commit: 657250e6a39648cbb169d079a60bd9ba
digest: shake256:00de25001b8dd2e29d85fc4bcc3ede7aed886d76d67f5e0f7a9b320b90f871d3eb73507d50818d823a0512f3f8db77a11c043685528403e31ff3fef18323a9fb
- remote: buf.build
owner: tendermint
repository: tendermint
commit: 33ed361a90514289beabf3189e1d7665
digest: shake256:038267e06294714fd883610626554b04a127b576b4e253befb4206cb72d5d3c1eeccacd4b9ec8e3fb891f7c14e1cb0f770c077d2989638995b0a61c85afedb1d
commit: 7727a3b7399540398e5736b9916f0faa
digest: shake256:74c046f009811965d227acd11bb2c6259bf387759d4230b95405e402a2ed4212d6f4babab690407d07bc81095c3917c4ae9144195ddfb081525c9aec58e51173
2 changes: 2 additions & 0 deletions client/v2/internal/buf.yaml
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
version: v1
deps:
- buf.build/cosmos/gogo-proto
- buf.build/cosmos/cosmos-sdk
- buf.build/cosmos/cosmos-proto
- buf.build/protocolbuffers/wellknowntypes
lint:
use:
- STANDARD
Expand Down
14 changes: 14 additions & 0 deletions client/v2/internal/testpbgogo/buf.gen.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
version: v1
managed:
enabled: true
go_package_prefix:
default: github.com/cosmos/cosmos-sdk/client/v2/internal
except:
- buf.build/cosmos/cosmos-proto
- buf.build/cosmos/cosmos-sdk
override:
buf.build/cosmos/gogo-proto: github.com/cosmos/gogoproto
plugins:
- name: gocosmos
out: .
opt: plugins=grpc,Mgoogle/protobuf/any.proto=github.com/cosmos/gogoproto/types/any
Loading
Loading