From 2985fb35d699d82e3684cd76448459efde79590c Mon Sep 17 00:00:00 2001 From: Matt Kocubinski Date: Tue, 17 Sep 2024 13:47:21 -0500 Subject: [PATCH 01/10] reworking aminojson tests --- .../tx/aminojson/aminojson_test.go | 140 +++++++++++++++++- tests/integration/tx/context_test.go | 21 +-- tests/integration/tx/internal/util.go | 21 +++ 3 files changed, 157 insertions(+), 25 deletions(-) create mode 100644 tests/integration/tx/internal/util.go diff --git a/tests/integration/tx/aminojson/aminojson_test.go b/tests/integration/tx/aminojson/aminojson_test.go index ec218ca47da3..01691fbb5ffa 100644 --- a/tests/integration/tx/aminojson/aminojson_test.go +++ b/tests/integration/tx/aminojson/aminojson_test.go @@ -2,7 +2,12 @@ package aminojson import ( "context" + "cosmossdk.io/core/address" + "encoding/json" "fmt" + "github.com/cosmos/cosmos-proto/anyutil" + "github.com/cosmos/cosmos-sdk/codec" + "github.com/cosmos/cosmos-sdk/tests/integration/tx/internal" "reflect" "strings" "testing" @@ -51,6 +56,7 @@ import ( slashingtypes "cosmossdk.io/x/slashing/types" "cosmossdk.io/x/staking" stakingtypes "cosmossdk.io/x/staking/types" + txsigning "cosmossdk.io/x/tx/signing" "cosmossdk.io/x/tx/signing/aminojson" signing_testutil "cosmossdk.io/x/tx/signing/testutil" "cosmossdk.io/x/upgrade" @@ -98,7 +104,8 @@ func TestAminoJSON_Equivalence(t *testing.T) { gov.AppModule{}, groupmodule.AppModule{}, mint.AppModule{}, slashing.AppModule{}, staking.AppModule{}, upgrade.AppModule{}, vesting.AppModule{}) legacytx.RegressionTestingAminoCodec = encCfg.Amino - aj := aminojson.NewEncoder(aminojson.EncoderOptions{DoNotSortFields: true}) + aj := aminojson.NewEncoder(aminojson.EncoderOptions{}) + // aj := aminojson.NewEncoder(aminojson.EncoderOptions{DoNotSortFields: true}) for _, tt := range rapidgen.DefaultGeneratedTypes { desc := tt.Pulsar.ProtoReflect().Descriptor() @@ -212,9 +219,7 @@ func TestAminoJSON_LegacyParity(t *testing.T) { encCfg := testutil.MakeTestEncodingConfig(codectestutil.CodecOptions{}, auth.AppModule{}, authzmodule.AppModule{}, bank.AppModule{}, distribution.AppModule{}, slashing.AppModule{}, staking.AppModule{}, vesting.AppModule{}, gov.AppModule{}) - legacytx.RegressionTestingAminoCodec = encCfg.Amino - - aj := aminojson.NewEncoder(aminojson.EncoderOptions{DoNotSortFields: true}) + aj := aminojson.NewEncoder(aminojson.EncoderOptions{}) addr1 := types.AccAddress("addr1") now := time.Now() @@ -254,9 +259,11 @@ func TestAminoJSON_LegacyParity(t *testing.T) { }, "authz/msg_grant": { gogo: &authztypes.MsgGrant{ + Granter: addr1.String(), Grantee: addr1.String(), Grant: authztypes.Grant{Expiration: &now, Authorization: genericAuth}, }, pulsar: &authzapi.MsgGrant{ + Granter: addr1.String(), Grantee: addr1.String(), Grant: &authzapi.Grant{Expiration: timestamppb.New(now), Authorization: genericAuthPulsar}, }, }, @@ -393,14 +400,18 @@ func TestAminoJSON_LegacyParity(t *testing.T) { }, "staking/stake_authorization_allow": { gogo: &stakingtypes.StakeAuthorization{ + MaxTokens: &types.Coin{Denom: "foo", Amount: math.NewInt(123)}, Validators: &stakingtypes.StakeAuthorization_AllowList{ AllowList: &stakingtypes.StakeAuthorization_Validators{Address: []string{"foo"}}, }, + AuthorizationType: stakingtypes.AuthorizationType_AUTHORIZATION_TYPE_DELEGATE, }, pulsar: &stakingapi.StakeAuthorization{ + MaxTokens: &v1beta1.Coin{Denom: "foo", Amount: "123"}, Validators: &stakingapi.StakeAuthorization_AllowList{ AllowList: &stakingapi.StakeAuthorization_Validators{Address: []string{"foo"}}, }, + AuthorizationType: stakingapi.AuthorizationType_AUTHORIZATION_TYPE_DELEGATE, }, }, "vesting/base_account_empty": { @@ -432,6 +443,8 @@ func TestAminoJSON_LegacyParity(t *testing.T) { t.Run(name, func(t *testing.T) { gogoBytes, err := encCfg.Amino.MarshalJSON(tc.gogo) require.NoError(t, err) + gogoBytes, err = sortJson(gogoBytes) + require.NoError(t, err) pulsarBytes, err := aj.Marshal(tc.pulsar) if tc.pulsarMarshalFails { @@ -459,6 +472,8 @@ func TestAminoJSON_LegacyParity(t *testing.T) { newGogoBytes, err := encCfg.Amino.MarshalJSON(newGogo) require.NoError(t, err) + newGogoBytes, err = sortJson(newGogoBytes) + require.NoError(t, err) if tc.roundTripUnequal { require.NotEqual(t, string(gogoBytes), string(newGogoBytes)) return @@ -466,12 +481,18 @@ func TestAminoJSON_LegacyParity(t *testing.T) { require.Equal(t, string(gogoBytes), string(newGogoBytes)) // test amino json signer handler equivalence - msg, ok := tc.gogo.(legacytx.LegacyMsg) - if !ok { + if !proto.HasExtension(tc.pulsar.ProtoReflect().Descriptor().Options(), msgv1.E_Signer) { // not signable return } + // test amino json signer handler equivalence + // msg, ok := tc.gogo.(legacytx.LegacyMsg) + // if !ok { + // // not signable + // return + // } + handlerOptions := signing_testutil.HandlerArgumentOptions{ ChainID: "test-chain", Memo: "sometestmemo", @@ -493,7 +514,7 @@ func TestAminoJSON_LegacyParity(t *testing.T) { legacyHandler := tx.NewSignModeLegacyAminoJSONHandler() txBuilder := encCfg.TxConfig.NewTxBuilder() - require.NoError(t, txBuilder.SetMsgs([]types.Msg{msg}...)) + require.NoError(t, txBuilder.SetMsgs([]types.Msg{tc.gogo}...)) txBuilder.SetMemo(handlerOptions.Memo) txBuilder.SetFeeAmount(types.Coins{types.NewInt64Coin("uatom", 1000)}) theTx := txBuilder.GetTx() @@ -507,6 +528,8 @@ func TestAminoJSON_LegacyParity(t *testing.T) { legacySignBz, err := legacyHandler.GetSignBytes(signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, legacySigningData, theTx) require.NoError(t, err) + fmt.Printf("legacy: %s\n", string(legacySignBz)) + fmt.Printf(" sign: %s\n", string(signBz)) require.Equal(t, string(legacySignBz), string(signBz)) }) } @@ -598,3 +621,106 @@ func postFixPulsarMessage(msg proto.Message) { } } } + +// sortJson sorts the JSON bytes by way of the side effect of unmarshalling and remarshalling the JSON +// using encoding/json. This hacky way of sorting JSON fields was used by the legacy amino JSON encoding +// x/auth/migrations/legacytx.StdSignBytes. It is used here ensure the x/tx JSON encoding is equivalent to +// the legacy amino JSON encoding. +func sortJson(bz []byte) ([]byte, error) { + var c any + err := json.Unmarshal(bz, &c) + if err != nil { + return nil, err + } + js, err := json.Marshal(c) + if err != nil { + return nil, err + } + return js, nil +} + +type noOpAddressCodec struct{} + +func (a noOpAddressCodec) StringToBytes(text string) ([]byte, error) { + return []byte(text), nil +} + +func (a noOpAddressCodec) BytesToString(bz []byte) (string, error) { + return string(bz), nil +} + +func TestJSONSorting(t *testing.T) { + // set up transaction and signing infra + addressCodec, valAddressCodec, _ := codec.ProvideAddressCodec(codec.AddressCodecInputs{ + AddressCodecFactory: func() address.Codec { return noOpAddressCodec{} }, + ValidatorAddressCodecFactory: func() address.ValidatorAddressCodec { + return noOpAddressCodec{} + }, + ConsensusAddressCodecFactory: func() address.ConsensusAddressCodec { + return noOpAddressCodec{} + }, + }) + customGetSigners := []txsigning.CustomGetSigner{internal.ProvideCustomGetSigner()} + interfaceRegistry, _, err := codec.ProvideInterfaceRegistry( + addressCodec, + valAddressCodec, + customGetSigners, + ) + require.NoError(t, err) + protoCodec := codec.ProvideProtoCodec(interfaceRegistry) + signingOptions := &txsigning.Options{ + FileResolver: interfaceRegistry, + AddressCodec: addressCodec, + ValidatorAddressCodec: valAddressCodec, + } + for _, customGetSigner := range customGetSigners { + signingOptions.DefineCustomGetSigners(customGetSigner.MsgType, customGetSigner.Fn) + } + txConfig, err := tx.NewTxConfigWithOptions( + protoCodec, + tx.ConfigOptions{ + EnabledSignModes: []signingtypes.SignMode{signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON}, + SigningOptions: signingOptions, + }) + require.NoError(t, err) + + // define message + gogo := &stakingtypes.StakeAuthorization{ + MaxTokens: &types.Coin{Denom: "foo", Amount: math.NewInt(123)}, + Validators: &stakingtypes.StakeAuthorization_AllowList{ + AllowList: &stakingtypes.StakeAuthorization_Validators{Address: []string{"foo"}}, + }, + AuthorizationType: stakingtypes.AuthorizationType_AUTHORIZATION_TYPE_DELEGATE, + } + + // create tx envelope + txBuilder := txConfig.NewTxBuilder() + err = txBuilder.SetMsgs([]types.Msg{gogo}...) + require.NoError(t, err) + builtTx := txBuilder.GetTx() + + // round trip it to simulate application usage + txBz, err := txConfig.TxEncoder()(builtTx) + require.NoError(t, err) + theTx, err := txConfig.TxDecoder()(txBz) + require.NoError(t, err) + + pk := &secp256k1.PubKey{ + Key: make([]byte, 256), + } + anyPk, err := anyutil.New(pk) + + signerData := txsigning.SignerData{ + Address: "sender-address", + ChainID: "test-chain", + AccountNumber: 0, + Sequence: 0, + PubKey: anyPk, + } + adaptableTx, ok := theTx.(signing.V2AdaptableTx) + require.True(t, ok) + + handler := aminojson.NewSignModeHandler(aminojson.SignModeHandlerOptions{}) + _, err = handler.GetSignBytes(context.Background(), signerData, adaptableTx.GetSigningTxData()) + require.NoError(t, err) +} diff --git a/tests/integration/tx/context_test.go b/tests/integration/tx/context_test.go index ffe21c2053cb..4dd7d77e9e45 100644 --- a/tests/integration/tx/context_test.go +++ b/tests/integration/tx/context_test.go @@ -3,32 +3,17 @@ package tx import ( "testing" - "github.com/stretchr/testify/require" - "google.golang.org/protobuf/proto" - "cosmossdk.io/depinject" "cosmossdk.io/log" _ "cosmossdk.io/x/accounts" - "cosmossdk.io/x/tx/signing" - codectypes "github.com/cosmos/cosmos-sdk/codec/types" + "github.com/cosmos/cosmos-sdk/tests/integration/tx/internal" "github.com/cosmos/cosmos-sdk/tests/integration/tx/internal/pulsar/testpb" "github.com/cosmos/cosmos-sdk/testutil/configurator" simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims" + "github.com/stretchr/testify/require" ) -func ProvideCustomGetSigners() signing.CustomGetSigner { - return signing.CustomGetSigner{ - MsgType: proto.MessageName(&testpb.TestRepeatedFields{}), - Fn: func(msg proto.Message) ([][]byte, error) { - testMsg := msg.(*testpb.TestRepeatedFields) - // arbitrary logic - signer := testMsg.NullableDontOmitempty[1].Value - return [][]byte{[]byte(signer)}, nil - }, - } -} - func TestDefineCustomGetSigners(t *testing.T) { var interfaceRegistry codectypes.InterfaceRegistry _, err := simtestutil.SetupAtGenesis( @@ -41,7 +26,7 @@ func TestDefineCustomGetSigners(t *testing.T) { configurator.ConsensusModule(), ), depinject.Supply(log.NewNopLogger()), - depinject.Provide(ProvideCustomGetSigners), + depinject.Provide(internal.ProvideCustomGetSigner), ), &interfaceRegistry, ) diff --git a/tests/integration/tx/internal/util.go b/tests/integration/tx/internal/util.go new file mode 100644 index 000000000000..76ab0e61da2f --- /dev/null +++ b/tests/integration/tx/internal/util.go @@ -0,0 +1,21 @@ +package internal + +import ( + "google.golang.org/protobuf/proto" + + "cosmossdk.io/x/tx/signing" + + "github.com/cosmos/cosmos-sdk/tests/integration/tx/internal/pulsar/testpb" +) + +func ProvideCustomGetSigner() signing.CustomGetSigner { + return signing.CustomGetSigner{ + MsgType: proto.MessageName(&testpb.TestRepeatedFields{}), + Fn: func(msg proto.Message) ([][]byte, error) { + testMsg := msg.(*testpb.TestRepeatedFields) + // arbitrary logic + signer := testMsg.NullableDontOmitempty[1].Value + return [][]byte{[]byte(signer)}, nil + }, + } +} From 5fc26b9073614f05735e6b165813063846d081d1 Mon Sep 17 00:00:00 2001 From: Matt Kocubinski Date: Wed, 18 Sep 2024 08:49:09 -0500 Subject: [PATCH 02/10] wip --- .../tx/aminojson/aminojson_test.go | 278 +++++++----------- tests/integration/tx/internal/util.go | 171 +++++++++++ 2 files changed, 270 insertions(+), 179 deletions(-) diff --git a/tests/integration/tx/aminojson/aminojson_test.go b/tests/integration/tx/aminojson/aminojson_test.go index 01691fbb5ffa..227f54e2d1f2 100644 --- a/tests/integration/tx/aminojson/aminojson_test.go +++ b/tests/integration/tx/aminojson/aminojson_test.go @@ -2,17 +2,14 @@ package aminojson import ( "context" - "cosmossdk.io/core/address" - "encoding/json" "fmt" - "github.com/cosmos/cosmos-proto/anyutil" - "github.com/cosmos/cosmos-sdk/codec" - "github.com/cosmos/cosmos-sdk/tests/integration/tx/internal" "reflect" "strings" "testing" "time" + "github.com/cosmos/cosmos-sdk/tests/integration/tx/internal" + "github.com/cosmos/cosmos-proto/rapidproto" gogoproto "github.com/cosmos/gogoproto/proto" "github.com/stretchr/testify/require" @@ -56,7 +53,6 @@ import ( slashingtypes "cosmossdk.io/x/slashing/types" "cosmossdk.io/x/staking" stakingtypes "cosmossdk.io/x/staking/types" - txsigning "cosmossdk.io/x/tx/signing" "cosmossdk.io/x/tx/signing/aminojson" signing_testutil "cosmossdk.io/x/tx/signing/testutil" "cosmossdk.io/x/upgrade" @@ -99,10 +95,22 @@ import ( // by the mutation of genOpts passed to the generator. func TestAminoJSON_Equivalence(t *testing.T) { encCfg := testutil.MakeTestEncodingConfig( - codectestutil.CodecOptions{}, auth.AppModule{}, authzmodule.AppModule{}, bank.AppModule{}, - consensus.AppModule{}, distribution.AppModule{}, evidence.AppModule{}, feegrantmodule.AppModule{}, - gov.AppModule{}, groupmodule.AppModule{}, mint.AppModule{}, - slashing.AppModule{}, staking.AppModule{}, upgrade.AppModule{}, vesting.AppModule{}) + codectestutil.CodecOptions{}, + auth.AppModule{}, + authzmodule.AppModule{}, + bank.AppModule{}, + consensus.AppModule{}, + distribution.AppModule{}, + evidence.AppModule{}, + feegrantmodule.AppModule{}, + gov.AppModule{}, + groupmodule.AppModule{}, + mint.AppModule{}, + slashing.AppModule{}, + staking.AppModule{}, + upgrade.AppModule{}, + vesting.AppModule{}, + ) legacytx.RegressionTestingAminoCodec = encCfg.Amino aj := aminojson.NewEncoder(aminojson.EncoderOptions{}) // aj := aminojson.NewEncoder(aminojson.EncoderOptions{DoNotSortFields: true}) @@ -194,8 +202,11 @@ func TestAminoJSON_Equivalence(t *testing.T) { AccountNumber: handlerOptions.AccNum, Sequence: handlerOptions.AccSeq, } - legacySignBz, err := legacyHandler.GetSignBytes(signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, - legacySigningData, theTx) + legacySignBz, err := legacyHandler.GetSignBytes( + signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, + legacySigningData, + theTx, + ) require.NoError(t, err) require.Equal(t, string(legacySignBz), string(signBz)) }) @@ -216,13 +227,14 @@ func newAny(t *testing.T, msg proto.Message) *anypb.Any { // TestAminoJSON_LegacyParity tests that the Encoder encoder produces the same output as the Encoder encoder. func TestAminoJSON_LegacyParity(t *testing.T) { - encCfg := testutil.MakeTestEncodingConfig(codectestutil.CodecOptions{}, auth.AppModule{}, authzmodule.AppModule{}, + fixture := internal.NewSigningFixture(t, internal.SigningFixtureOptions{}, + auth.AppModule{}, authzmodule.AppModule{}, bank.AppModule{}, distribution.AppModule{}, slashing.AppModule{}, staking.AppModule{}, vesting.AppModule{}, gov.AppModule{}) aj := aminojson.NewEncoder(aminojson.EncoderOptions{}) + addr1 := types.AccAddress("addr1") now := time.Now() - genericAuth, _ := codectypes.NewAnyWithValue(&authztypes.GenericAuthorization{Msg: "foo"}) genericAuthPulsar := newAny(t, &authzapi.GenericAuthorization{Msg: "foo"}) pubkeyAny, _ := codectypes.NewAnyWithValue(&secp256k1types.PubKey{Key: []byte("foo")}) @@ -235,21 +247,30 @@ func TestAminoJSON_LegacyParity(t *testing.T) { pulsar proto.Message pulsarMarshalFails bool - // this will fail in cases where a lossy encoding of an empty array to protobuf occurs. the unmarshalled bytes - // represent the array as nil, and a subsequent marshal to JSON represent the array as null instead of empty. + // this will fail in cases where a lossy encoding of an empty array to protobuf occurs. + // the unmarshalled bytes represent the array as nil, and a subsequent marshal to JSON + // represent the array as null instead of empty. roundTripUnequal bool - // pulsar does not support marshaling a math.Dec as anything except a string. Therefore, we cannot unmarshal - // a pulsar encoded Math.dec (the string representation of a Decimal) into a gogo Math.dec (expecting an int64). + // pulsar does not support marshaling a math.Dec as anything except a string. + // Therefore, we cannot unmarshal a pulsar encoded Math.dec (the string representation + // of a Decimal) into a gogo Math.dec (expecting an int64). protoUnmarshalFails bool }{ - "auth/params": {gogo: &authtypes.Params{TxSigLimit: 10}, pulsar: &authapi.Params{TxSigLimit: 10}}, + "auth/params": { + gogo: &authtypes.Params{TxSigLimit: 10}, + pulsar: &authapi.Params{TxSigLimit: 10}, + }, "auth/module_account": { gogo: &authtypes.ModuleAccount{ - BaseAccount: authtypes.NewBaseAccountWithAddress(addr1), Permissions: []string{}, + BaseAccount: authtypes.NewBaseAccountWithAddress( + addr1, + ), Permissions: []string{}, }, pulsar: &authapi.ModuleAccount{ - BaseAccount: &authapi.BaseAccount{Address: addr1.String()}, Permissions: []string{}, + BaseAccount: &authapi.BaseAccount{ + Address: addr1.String(), + }, Permissions: []string{}, }, roundTripUnequal: true, }, @@ -264,7 +285,10 @@ func TestAminoJSON_LegacyParity(t *testing.T) { }, pulsar: &authzapi.MsgGrant{ Granter: addr1.String(), Grantee: addr1.String(), - Grant: &authzapi.Grant{Expiration: timestamppb.New(now), Authorization: genericAuthPulsar}, + Grant: &authzapi.Grant{ + Expiration: timestamppb.New(now), + Authorization: genericAuthPulsar, + }, }, }, "authz/msg_update_params": { @@ -309,17 +333,23 @@ func TestAminoJSON_LegacyParity(t *testing.T) { pulsar: &multisigapi.LegacyAminoPubKey{}, }, "consensus/evidence_params/duration": { - gogo: &gov_v1beta1_types.VotingParams{VotingPeriod: 1e9 + 7}, - pulsar: &gov_v1beta1_api.VotingParams{VotingPeriod: &durationpb.Duration{Seconds: 1, Nanos: 7}}, + gogo: &gov_v1beta1_types.VotingParams{VotingPeriod: 1e9 + 7}, + pulsar: &gov_v1beta1_api.VotingParams{ + VotingPeriod: &durationpb.Duration{Seconds: 1, Nanos: 7}, + }, }, "consensus/evidence_params/big_duration": { - gogo: &gov_v1beta1_types.VotingParams{VotingPeriod: time.Duration(rapidproto.MaxDurationSeconds*1e9) + 999999999}, + gogo: &gov_v1beta1_types.VotingParams{ + VotingPeriod: time.Duration(rapidproto.MaxDurationSeconds*1e9) + 999999999, + }, pulsar: &gov_v1beta1_api.VotingParams{VotingPeriod: &durationpb.Duration{ Seconds: rapidproto.MaxDurationSeconds, Nanos: 999999999, }}, }, "consensus/evidence_params/too_big_duration": { - gogo: &gov_v1beta1_types.VotingParams{VotingPeriod: time.Duration(rapidproto.MaxDurationSeconds*1e9) + 999999999}, + gogo: &gov_v1beta1_types.VotingParams{ + VotingPeriod: time.Duration(rapidproto.MaxDurationSeconds*1e9) + 999999999, + }, pulsar: &gov_v1beta1_api.VotingParams{VotingPeriod: &durationpb.Duration{ Seconds: rapidproto.MaxDurationSeconds + 1, Nanos: 999999999, }}, @@ -351,8 +381,10 @@ func TestAminoJSON_LegacyParity(t *testing.T) { pulsar: &gov_v1_api.MsgSubmitProposal{}, }, "slashing/params/empty_dec": { - gogo: &slashingtypes.Params{DowntimeJailDuration: 1e9 + 7}, - pulsar: &slashingapi.Params{DowntimeJailDuration: &durationpb.Duration{Seconds: 1, Nanos: 7}}, + gogo: &slashingtypes.Params{DowntimeJailDuration: 1e9 + 7}, + pulsar: &slashingapi.Params{ + DowntimeJailDuration: &durationpb.Duration{Seconds: 1, Nanos: 7}, + }, }, // This test cases demonstrates the expected contract and proper way to set a cosmos.Dec field represented // as bytes in protobuf message, namely: @@ -402,14 +434,18 @@ func TestAminoJSON_LegacyParity(t *testing.T) { gogo: &stakingtypes.StakeAuthorization{ MaxTokens: &types.Coin{Denom: "foo", Amount: math.NewInt(123)}, Validators: &stakingtypes.StakeAuthorization_AllowList{ - AllowList: &stakingtypes.StakeAuthorization_Validators{Address: []string{"foo"}}, + AllowList: &stakingtypes.StakeAuthorization_Validators{ + Address: []string{"foo"}, + }, }, AuthorizationType: stakingtypes.AuthorizationType_AUTHORIZATION_TYPE_DELEGATE, }, pulsar: &stakingapi.StakeAuthorization{ MaxTokens: &v1beta1.Coin{Denom: "foo", Amount: "123"}, Validators: &stakingapi.StakeAuthorization_AllowList{ - AllowList: &stakingapi.StakeAuthorization_Validators{Address: []string{"foo"}}, + AllowList: &stakingapi.StakeAuthorization_Validators{ + Address: []string{"foo"}, + }, }, AuthorizationType: stakingapi.AuthorizationType_AUTHORIZATION_TYPE_DELEGATE, }, @@ -419,8 +455,12 @@ func TestAminoJSON_LegacyParity(t *testing.T) { pulsar: &vestingapi.BaseVestingAccount{BaseAccount: &authapi.BaseAccount{}}, }, "vesting/base_account_pubkey": { - gogo: &vestingtypes.BaseVestingAccount{BaseAccount: &authtypes.BaseAccount{PubKey: pubkeyAny}}, - pulsar: &vestingapi.BaseVestingAccount{BaseAccount: &authapi.BaseAccount{PubKey: pubkeyAnyPulsar}}, + gogo: &vestingtypes.BaseVestingAccount{ + BaseAccount: &authtypes.BaseAccount{PubKey: pubkeyAny}, + }, + pulsar: &vestingapi.BaseVestingAccount{ + BaseAccount: &authapi.BaseAccount{PubKey: pubkeyAnyPulsar}, + }, }, "math/int_as_string": { gogo: &gogo_testpb.IntAsString{IntAsString: math.NewInt(123)}, @@ -441,11 +481,7 @@ func TestAminoJSON_LegacyParity(t *testing.T) { } for name, tc := range cases { t.Run(name, func(t *testing.T) { - gogoBytes, err := encCfg.Amino.MarshalJSON(tc.gogo) - require.NoError(t, err) - gogoBytes, err = sortJson(gogoBytes) - require.NoError(t, err) - + gogoBytes := fixture.MarshalLegacyAminoJSON(t, tc.gogo) pulsarBytes, err := aj.Marshal(tc.pulsar) if tc.pulsarMarshalFails { require.Error(t, err) @@ -463,17 +499,14 @@ func TestAminoJSON_LegacyParity(t *testing.T) { gogoType := reflect.TypeOf(tc.gogo).Elem() newGogo := reflect.New(gogoType).Interface().(gogoproto.Message) - err = encCfg.Codec.Unmarshal(pulsarProtoBytes, newGogo) + err = fixture.UnmarshalGogoProto(pulsarProtoBytes, newGogo) if tc.protoUnmarshalFails { require.Error(t, err) return } require.NoError(t, err) - newGogoBytes, err := encCfg.Amino.MarshalJSON(newGogo) - require.NoError(t, err) - newGogoBytes, err = sortJson(newGogoBytes) - require.NoError(t, err) + newGogoBytes := fixture.MarshalLegacyAminoJSON(t, newGogo) if tc.roundTripUnequal { require.NotEqual(t, string(gogoBytes), string(newGogoBytes)) return @@ -481,63 +514,26 @@ func TestAminoJSON_LegacyParity(t *testing.T) { require.Equal(t, string(gogoBytes), string(newGogoBytes)) // test amino json signer handler equivalence - if !proto.HasExtension(tc.pulsar.ProtoReflect().Descriptor().Options(), msgv1.E_Signer) { + if !proto.HasExtension( + tc.pulsar.ProtoReflect().Descriptor().Options(), + msgv1.E_Signer, + ) { // not signable return } - - // test amino json signer handler equivalence - // msg, ok := tc.gogo.(legacytx.LegacyMsg) - // if !ok { - // // not signable - // return - // } - - handlerOptions := signing_testutil.HandlerArgumentOptions{ - ChainID: "test-chain", - Memo: "sometestmemo", - Msg: tc.pulsar, - AccNum: 1, - AccSeq: 2, - SignerAddress: "signerAddress", - Fee: &txv1beta1.Fee{ - Amount: []*v1beta1.Coin{{Denom: "uatom", Amount: "1000"}}, - }, - } - - signerData, txData, err := signing_testutil.MakeHandlerArguments(handlerOptions) - require.NoError(t, err) - - handler := aminojson.NewSignModeHandler(aminojson.SignModeHandlerOptions{}) - signBz, err := handler.GetSignBytes(context.Background(), signerData, txData) - require.NoError(t, err) - - legacyHandler := tx.NewSignModeLegacyAminoJSONHandler() - txBuilder := encCfg.TxConfig.NewTxBuilder() - require.NoError(t, txBuilder.SetMsgs([]types.Msg{tc.gogo}...)) - txBuilder.SetMemo(handlerOptions.Memo) - txBuilder.SetFeeAmount(types.Coins{types.NewInt64Coin("uatom", 1000)}) - theTx := txBuilder.GetTx() - - legacySigningData := signing.SignerData{ - ChainID: handlerOptions.ChainID, - Address: handlerOptions.SignerAddress, - AccountNumber: handlerOptions.AccNum, - Sequence: handlerOptions.AccSeq, - } - legacySignBz, err := legacyHandler.GetSignBytes(signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, - legacySigningData, theTx) - require.NoError(t, err) - fmt.Printf("legacy: %s\n", string(legacySignBz)) - fmt.Printf(" sign: %s\n", string(signBz)) - require.Equal(t, string(legacySignBz), string(signBz)) + fixture.RequireLegacyAminoEquivalent(t, tc.gogo) }) } } func TestSendAuthorization(t *testing.T) { - encCfg := testutil.MakeTestEncodingConfig(codectestutil.CodecOptions{}, auth.AppModule{}, authzmodule.AppModule{}, - distribution.AppModule{}, bank.AppModule{}) + encCfg := testutil.MakeTestEncodingConfig( + codectestutil.CodecOptions{}, + auth.AppModule{}, + authzmodule.AppModule{}, + distribution.AppModule{}, + bank.AppModule{}, + ) aj := aminojson.NewEncoder(aminojson.EncoderOptions{}) @@ -622,105 +618,29 @@ func postFixPulsarMessage(msg proto.Message) { } } -// sortJson sorts the JSON bytes by way of the side effect of unmarshalling and remarshalling the JSON -// using encoding/json. This hacky way of sorting JSON fields was used by the legacy amino JSON encoding -// x/auth/migrations/legacytx.StdSignBytes. It is used here ensure the x/tx JSON encoding is equivalent to -// the legacy amino JSON encoding. -func sortJson(bz []byte) ([]byte, error) { - var c any - err := json.Unmarshal(bz, &c) - if err != nil { - return nil, err - } - js, err := json.Marshal(c) - if err != nil { - return nil, err - } - return js, nil -} - -type noOpAddressCodec struct{} - -func (a noOpAddressCodec) StringToBytes(text string) ([]byte, error) { - return []byte(text), nil -} - -func (a noOpAddressCodec) BytesToString(bz []byte) (string, error) { - return string(bz), nil -} - func TestJSONSorting(t *testing.T) { - // set up transaction and signing infra - addressCodec, valAddressCodec, _ := codec.ProvideAddressCodec(codec.AddressCodecInputs{ - AddressCodecFactory: func() address.Codec { return noOpAddressCodec{} }, - ValidatorAddressCodecFactory: func() address.ValidatorAddressCodec { - return noOpAddressCodec{} - }, - ConsensusAddressCodecFactory: func() address.ConsensusAddressCodec { - return noOpAddressCodec{} - }, - }) - customGetSigners := []txsigning.CustomGetSigner{internal.ProvideCustomGetSigner()} - interfaceRegistry, _, err := codec.ProvideInterfaceRegistry( - addressCodec, - valAddressCodec, - customGetSigners, + fixture := internal.NewSigningFixture( + t, + internal.SigningFixtureOptions{}, + authzmodule.AppModule{}, + staking.AppModule{}, ) - require.NoError(t, err) - protoCodec := codec.ProvideProtoCodec(interfaceRegistry) - signingOptions := &txsigning.Options{ - FileResolver: interfaceRegistry, - AddressCodec: addressCodec, - ValidatorAddressCodec: valAddressCodec, - } - for _, customGetSigner := range customGetSigners { - signingOptions.DefineCustomGetSigners(customGetSigner.MsgType, customGetSigner.Fn) - } - txConfig, err := tx.NewTxConfigWithOptions( - protoCodec, - tx.ConfigOptions{ - EnabledSignModes: []signingtypes.SignMode{signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON}, - SigningOptions: signingOptions, - }) - require.NoError(t, err) // define message - gogo := &stakingtypes.StakeAuthorization{ + authorization := &stakingtypes.StakeAuthorization{ MaxTokens: &types.Coin{Denom: "foo", Amount: math.NewInt(123)}, Validators: &stakingtypes.StakeAuthorization_AllowList{ AllowList: &stakingtypes.StakeAuthorization_Validators{Address: []string{"foo"}}, }, AuthorizationType: stakingtypes.AuthorizationType_AUTHORIZATION_TYPE_DELEGATE, } - - // create tx envelope - txBuilder := txConfig.NewTxBuilder() - err = txBuilder.SetMsgs([]types.Msg{gogo}...) - require.NoError(t, err) - builtTx := txBuilder.GetTx() - - // round trip it to simulate application usage - txBz, err := txConfig.TxEncoder()(builtTx) - require.NoError(t, err) - theTx, err := txConfig.TxDecoder()(txBz) + authorizationAny, err := codectypes.NewAnyWithValue(authorization) require.NoError(t, err) - - pk := &secp256k1.PubKey{ - Key: make([]byte, 256), - } - anyPk, err := anyutil.New(pk) - - signerData := txsigning.SignerData{ - Address: "sender-address", - ChainID: "test-chain", - AccountNumber: 0, - Sequence: 0, - PubKey: anyPk, + msgGrant := &authztypes.MsgGrant{ + Granter: "granter", + Grantee: "grantee", + Grant: authztypes.Grant{Authorization: authorizationAny}, } - adaptableTx, ok := theTx.(signing.V2AdaptableTx) - require.True(t, ok) - handler := aminojson.NewSignModeHandler(aminojson.SignModeHandlerOptions{}) - _, err = handler.GetSignBytes(context.Background(), signerData, adaptableTx.GetSigningTxData()) - require.NoError(t, err) + fixture.RequireLegacyAminoEquivalent(t, msgGrant) } diff --git a/tests/integration/tx/internal/util.go b/tests/integration/tx/internal/util.go index 76ab0e61da2f..43ff0606b47e 100644 --- a/tests/integration/tx/internal/util.go +++ b/tests/integration/tx/internal/util.go @@ -1,11 +1,28 @@ package internal import ( + "bytes" + "context" + "encoding/json" + "testing" + + "github.com/stretchr/testify/require" "google.golang.org/protobuf/proto" + "cosmossdk.io/core/transaction" "cosmossdk.io/x/tx/signing" + "cosmossdk.io/x/tx/signing/aminojson" + "github.com/cosmos/cosmos-sdk/client" + "github.com/cosmos/cosmos-sdk/codec" + "github.com/cosmos/cosmos-sdk/std" "github.com/cosmos/cosmos-sdk/tests/integration/tx/internal/pulsar/testpb" + "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/types/module" + signingtypes "github.com/cosmos/cosmos-sdk/types/tx/signing" + "github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx" + authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing" + "github.com/cosmos/cosmos-sdk/x/auth/tx" ) func ProvideCustomGetSigner() signing.CustomGetSigner { @@ -19,3 +36,157 @@ func ProvideCustomGetSigner() signing.CustomGetSigner { }, } } + +type noOpAddressCodec struct{} + +func (a noOpAddressCodec) StringToBytes(text string) ([]byte, error) { + return []byte(text), nil +} + +func (a noOpAddressCodec) BytesToString(bz []byte) (string, error) { + return string(bz), nil +} + +type SigningFixture struct { + txConfig client.TxConfig + legacy *codec.LegacyAmino + protoCodec *codec.ProtoCodec + options SigningFixtureOptions +} + +type SigningFixtureOptions struct { + DoNotSortFields bool +} + +func NewSigningFixture( + t *testing.T, + options SigningFixtureOptions, + modules ...module.AppModule, +) *SigningFixture { + t.Helper() + // set up transaction and signing infra + addressCodec, valAddressCodec := noOpAddressCodec{}, noOpAddressCodec{} + customGetSigners := []signing.CustomGetSigner{ProvideCustomGetSigner()} + interfaceRegistry, _, err := codec.ProvideInterfaceRegistry( + addressCodec, + valAddressCodec, + customGetSigners, + ) + require.NoError(t, err) + protoCodec := codec.ProvideProtoCodec(interfaceRegistry) + signingOptions := &signing.Options{ + FileResolver: interfaceRegistry, + AddressCodec: addressCodec, + ValidatorAddressCodec: valAddressCodec, + } + for _, customGetSigner := range customGetSigners { + signingOptions.DefineCustomGetSigners(customGetSigner.MsgType, customGetSigner.Fn) + } + txConfig, err := tx.NewTxConfigWithOptions( + protoCodec, + tx.ConfigOptions{ + EnabledSignModes: []signingtypes.SignMode{ + signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, + }, + SigningOptions: signingOptions, + }) + require.NoError(t, err) + + legacyAminoCodec := codec.NewLegacyAmino() + mb := module.NewManager(modules...) + std.RegisterLegacyAminoCodec(legacyAminoCodec) + std.RegisterInterfaces(interfaceRegistry) + mb.RegisterLegacyAminoCodec(legacyAminoCodec) + mb.RegisterInterfaces(interfaceRegistry) + + return &SigningFixture{ + txConfig: txConfig, + legacy: legacyAminoCodec, + options: options, + protoCodec: protoCodec, + } +} + +func (s *SigningFixture) RequireLegacyAminoEquivalent(t *testing.T, msg transaction.Msg) { + t.Helper() + // create tx envelope + txBuilder := s.txConfig.NewTxBuilder() + err := txBuilder.SetMsgs([]types.Msg{msg}...) + require.NoError(t, err) + builtTx := txBuilder.GetTx() + + // round trip it to simulate application usage + txBz, err := s.txConfig.TxEncoder()(builtTx) + require.NoError(t, err) + theTx, err := s.txConfig.TxDecoder()(txBz) + require.NoError(t, err) + + // create signing envelope + signerData := signing.SignerData{ + Address: "sender-address", + ChainID: "test-chain", + AccountNumber: 0, + Sequence: 0, + } + adaptableTx, ok := theTx.(authsigning.V2AdaptableTx) + require.True(t, ok) + handler := aminojson.NewSignModeHandler(aminojson.SignModeHandlerOptions{}) + signBz, err := handler.GetSignBytes( + context.Background(), + signerData, + adaptableTx.GetSigningTxData(), + ) + require.NoError(t, err) + + legacytx.RegressionTestingAminoCodec = s.legacy + defer func() { + legacytx.RegressionTestingAminoCodec = nil + }() + legacyAminoSignHandler := tx.NewSignModeLegacyAminoJSONHandler() + legacyBz, err := legacyAminoSignHandler.GetSignBytes( + signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, + authsigning.SignerData{ + ChainID: signerData.ChainID, + Address: signerData.Address, + AccountNumber: signerData.AccountNumber, + Sequence: signerData.Sequence, + }, + theTx) + require.NoError(t, err) + require.Truef(t, + bytes.Equal(legacyBz, signBz), + "legacy: %s\n x/tx: %s", string(legacyBz), string(signBz)) +} + +func (s *SigningFixture) MarshalLegacyAminoJSON(t *testing.T, o any) []byte { + t.Helper() + bz, err := s.legacy.MarshalJSON(o) + require.NoError(t, err) + if s.options.DoNotSortFields { + return bz + } + sortedBz, err := sortJson(bz) + require.NoError(t, err) + return sortedBz +} + +func (s *SigningFixture) UnmarshalGogoProto(bz []byte, ptr transaction.Msg) error { + return s.protoCodec.Unmarshal(bz, ptr) +} + +// sortJson sorts the JSON bytes by way of the side effect of unmarshalling and remarshalling +// the JSON using encoding/json. This hacky way of sorting JSON fields was used by the legacy +// amino JSON encoding x/auth/migrations/legacytx.StdSignBytes. It is used here ensure the x/tx +// JSON encoding is equivalent to the legacy amino JSON encoding. +func sortJson(bz []byte) ([]byte, error) { + var c any + err := json.Unmarshal(bz, &c) + if err != nil { + return nil, err + } + js, err := json.Marshal(c) + if err != nil { + return nil, err + } + return js, nil +} From f7a559faaeee4c52736b6c191b7d92f664db7395 Mon Sep 17 00:00:00 2001 From: Matt Kocubinski Date: Wed, 18 Sep 2024 16:11:34 -0500 Subject: [PATCH 03/10] TestAminoJSON_LegacyParity now passing again --- math/dec_test.go | 33 +++ .../tx/aminojson/aminojson_test.go | 231 ++++-------------- tests/integration/tx/aminojson/notes.txt | 7 + tests/integration/tx/internal/util.go | 85 ++++++- x/tx/decode/decode.go | 41 ++++ x/tx/signing/aminojson/encoder.go | 76 ++++-- 6 files changed, 255 insertions(+), 218 deletions(-) create mode 100644 tests/integration/tx/aminojson/notes.txt diff --git a/math/dec_test.go b/math/dec_test.go index 775ddd3c23cd..e8336a97a24c 100644 --- a/math/dec_test.go +++ b/math/dec_test.go @@ -1025,3 +1025,36 @@ func TestQuoMut(t *testing.T) { }) } } + +func Test_DocumentLegacyAsymmetry(t *testing.T) { + zeroDec := math.LegacyZeroDec() + emptyDec := math.LegacyDec{} + + zeroDecBz, err := zeroDec.Marshal() + require.NoError(t, err) + zeroDecJSON := zeroDec.String() + + emptyDecBz, err := emptyDec.Marshal() + require.NoError(t, err) + emptyDecJSON := emptyDec.String() + + // makes sense, zero and empty are semantically different and render differently + require.NotEqual(t, zeroDecJSON, emptyDecJSON) + // but on the proto wire they encode to the same bytes + require.Equal(t, zeroDecBz, emptyDecBz) + + // zero values are symmetrical + zeroDecRoundTrip := math.LegacyDec{} + err = zeroDecRoundTrip.Unmarshal(zeroDecBz) + require.NoError(t, err) + require.Equal(t, zeroDec.String(), zeroDecRoundTrip.String()) + require.Equal(t, zeroDec, zeroDecRoundTrip) + + // empty values are not + emptyDecRoundTrip := math.LegacyDec{} + err = emptyDecRoundTrip.Unmarshal(emptyDecBz) + require.NoError(t, err) + // !!! this is the key point, they are not equal, it looks like a bug + require.NotEqual(t, emptyDec.String(), emptyDecRoundTrip.String()) + require.NotEqual(t, emptyDec, emptyDecRoundTrip) +} diff --git a/tests/integration/tx/aminojson/aminojson_test.go b/tests/integration/tx/aminojson/aminojson_test.go index 227f54e2d1f2..696106f50159 100644 --- a/tests/integration/tx/aminojson/aminojson_test.go +++ b/tests/integration/tx/aminojson/aminojson_test.go @@ -3,7 +3,6 @@ package aminojson import ( "context" "fmt" - "reflect" "strings" "testing" "time" @@ -14,26 +13,13 @@ import ( gogoproto "github.com/cosmos/gogoproto/proto" "github.com/stretchr/testify/require" "google.golang.org/protobuf/proto" - "google.golang.org/protobuf/types/known/anypb" - "google.golang.org/protobuf/types/known/durationpb" - "google.golang.org/protobuf/types/known/timestamppb" "pgregory.net/rapid" authapi "cosmossdk.io/api/cosmos/auth/v1beta1" - authzapi "cosmossdk.io/api/cosmos/authz/v1beta1" bankapi "cosmossdk.io/api/cosmos/bank/v1beta1" v1beta1 "cosmossdk.io/api/cosmos/base/v1beta1" - "cosmossdk.io/api/cosmos/crypto/ed25519" - multisigapi "cosmossdk.io/api/cosmos/crypto/multisig" - "cosmossdk.io/api/cosmos/crypto/secp256k1" - distapi "cosmossdk.io/api/cosmos/distribution/v1beta1" - gov_v1_api "cosmossdk.io/api/cosmos/gov/v1" - gov_v1beta1_api "cosmossdk.io/api/cosmos/gov/v1beta1" msgv1 "cosmossdk.io/api/cosmos/msg/v1" - slashingapi "cosmossdk.io/api/cosmos/slashing/v1beta1" - stakingapi "cosmossdk.io/api/cosmos/staking/v1beta1" txv1beta1 "cosmossdk.io/api/cosmos/tx/v1beta1" - vestingapi "cosmossdk.io/api/cosmos/vesting/v1beta1" "cosmossdk.io/math" authztypes "cosmossdk.io/x/authz" authzmodule "cosmossdk.io/x/authz/module" @@ -64,7 +50,6 @@ import ( secp256k1types "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" "github.com/cosmos/cosmos-sdk/tests/integration/rapidgen" gogo_testpb "github.com/cosmos/cosmos-sdk/tests/integration/tx/internal/gogo/testpb" - pulsar_testpb "github.com/cosmos/cosmos-sdk/tests/integration/tx/internal/pulsar/testpb" "github.com/cosmos/cosmos-sdk/testutil/testdata" "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/bech32" @@ -214,17 +199,6 @@ func TestAminoJSON_Equivalence(t *testing.T) { } } -func newAny(t *testing.T, msg proto.Message) *anypb.Any { - t.Helper() - bz, err := proto.Marshal(msg) - require.NoError(t, err) - typeName := fmt.Sprintf("/%s", msg.ProtoReflect().Descriptor().FullName()) - return &anypb.Any{ - TypeUrl: typeName, - Value: bz, - } -} - // TestAminoJSON_LegacyParity tests that the Encoder encoder produces the same output as the Encoder encoder. func TestAminoJSON_LegacyParity(t *testing.T) { fixture := internal.NewSigningFixture(t, internal.SigningFixtureOptions{}, @@ -236,30 +210,14 @@ func TestAminoJSON_LegacyParity(t *testing.T) { addr1 := types.AccAddress("addr1") now := time.Now() genericAuth, _ := codectypes.NewAnyWithValue(&authztypes.GenericAuthorization{Msg: "foo"}) - genericAuthPulsar := newAny(t, &authzapi.GenericAuthorization{Msg: "foo"}) pubkeyAny, _ := codectypes.NewAnyWithValue(&secp256k1types.PubKey{Key: []byte("foo")}) - pubkeyAnyPulsar := newAny(t, &secp256k1.PubKey{Key: []byte("foo")}) - dec10bz, _ := math.LegacyNewDec(10).Marshal() - int123bz, _ := math.NewInt(123).Marshal() + dec5point4 := math.LegacyMustNewDecFromStr("5.4") cases := map[string]struct { - gogo gogoproto.Message - pulsar proto.Message - pulsarMarshalFails bool - - // this will fail in cases where a lossy encoding of an empty array to protobuf occurs. - // the unmarshalled bytes represent the array as nil, and a subsequent marshal to JSON - // represent the array as null instead of empty. - roundTripUnequal bool - - // pulsar does not support marshaling a math.Dec as anything except a string. - // Therefore, we cannot unmarshal a pulsar encoded Math.dec (the string representation - // of a Decimal) into a gogo Math.dec (expecting an int64). - protoUnmarshalFails bool + gogo gogoproto.Message }{ "auth/params": { - gogo: &authtypes.Params{TxSigLimit: 10}, - pulsar: &authapi.Params{TxSigLimit: 10}, + gogo: &authtypes.Params{TxSigLimit: 10}, }, "auth/module_account": { gogo: &authtypes.ModuleAccount{ @@ -267,168 +225,113 @@ func TestAminoJSON_LegacyParity(t *testing.T) { addr1, ), Permissions: []string{}, }, - pulsar: &authapi.ModuleAccount{ - BaseAccount: &authapi.BaseAccount{ - Address: addr1.String(), - }, Permissions: []string{}, - }, - roundTripUnequal: true, }, "auth/base_account": { - gogo: &authtypes.BaseAccount{Address: addr1.String(), PubKey: pubkeyAny}, - pulsar: &authapi.BaseAccount{Address: addr1.String(), PubKey: pubkeyAnyPulsar}, + gogo: &authtypes.BaseAccount{Address: addr1.String(), PubKey: pubkeyAny}, }, "authz/msg_grant": { gogo: &authztypes.MsgGrant{ Granter: addr1.String(), Grantee: addr1.String(), Grant: authztypes.Grant{Expiration: &now, Authorization: genericAuth}, }, - pulsar: &authzapi.MsgGrant{ - Granter: addr1.String(), Grantee: addr1.String(), - Grant: &authzapi.Grant{ - Expiration: timestamppb.New(now), - Authorization: genericAuthPulsar, - }, - }, }, "authz/msg_update_params": { - gogo: &authtypes.MsgUpdateParams{Params: authtypes.Params{TxSigLimit: 10}}, - pulsar: &authapi.MsgUpdateParams{Params: &authapi.Params{TxSigLimit: 10}}, + gogo: &authtypes.MsgUpdateParams{Params: authtypes.Params{TxSigLimit: 10}}, }, "authz/msg_exec/empty_msgs": { - gogo: &authztypes.MsgExec{Msgs: []*codectypes.Any{}}, - pulsar: &authzapi.MsgExec{Msgs: []*anypb.Any{}}, + gogo: &authztypes.MsgExec{Msgs: []*codectypes.Any{}}, }, "distribution/delegator_starting_info": { - gogo: &disttypes.DelegatorStartingInfo{}, - pulsar: &distapi.DelegatorStartingInfo{}, + gogo: &disttypes.DelegatorStartingInfo{Stake: math.LegacyNewDec(10)}, }, "distribution/delegator_starting_info/non_zero_dec": { - gogo: &disttypes.DelegatorStartingInfo{Stake: math.LegacyNewDec(10)}, - pulsar: &distapi.DelegatorStartingInfo{Stake: "10.000000000000000000"}, - protoUnmarshalFails: true, + gogo: &disttypes.DelegatorStartingInfo{Stake: math.LegacyNewDec(10)}, }, "distribution/delegation_delegator_reward": { - gogo: &disttypes.DelegationDelegatorReward{}, - pulsar: &distapi.DelegationDelegatorReward{}, + gogo: &disttypes.DelegationDelegatorReward{}, }, "distribution/msg_withdraw_delegator_reward": { - gogo: &disttypes.MsgWithdrawDelegatorReward{DelegatorAddress: "foo"}, - pulsar: &distapi.MsgWithdrawDelegatorReward{DelegatorAddress: "foo"}, + gogo: &disttypes.MsgWithdrawDelegatorReward{DelegatorAddress: "foo"}, }, "crypto/ed25519": { - gogo: &ed25519types.PubKey{Key: []byte("key")}, - pulsar: &ed25519.PubKey{Key: []byte("key")}, + gogo: &ed25519types.PubKey{Key: []byte("key")}, }, "crypto/secp256k1": { - gogo: &secp256k1types.PubKey{Key: []byte("key")}, - pulsar: &secp256k1.PubKey{Key: []byte("key")}, + gogo: &secp256k1types.PubKey{Key: []byte("key")}, }, "crypto/legacy_amino_pubkey": { - gogo: &multisig.LegacyAminoPubKey{PubKeys: []*codectypes.Any{pubkeyAny}}, - pulsar: &multisigapi.LegacyAminoPubKey{PublicKeys: []*anypb.Any{pubkeyAnyPulsar}}, + gogo: &multisig.LegacyAminoPubKey{PubKeys: []*codectypes.Any{pubkeyAny}}, }, "crypto/legacy_amino_pubkey/empty": { - gogo: &multisig.LegacyAminoPubKey{}, - pulsar: &multisigapi.LegacyAminoPubKey{}, + gogo: &multisig.LegacyAminoPubKey{}, }, "consensus/evidence_params/duration": { gogo: &gov_v1beta1_types.VotingParams{VotingPeriod: 1e9 + 7}, - pulsar: &gov_v1beta1_api.VotingParams{ - VotingPeriod: &durationpb.Duration{Seconds: 1, Nanos: 7}, - }, }, "consensus/evidence_params/big_duration": { gogo: &gov_v1beta1_types.VotingParams{ VotingPeriod: time.Duration(rapidproto.MaxDurationSeconds*1e9) + 999999999, }, - pulsar: &gov_v1beta1_api.VotingParams{VotingPeriod: &durationpb.Duration{ - Seconds: rapidproto.MaxDurationSeconds, Nanos: 999999999, - }}, }, "consensus/evidence_params/too_big_duration": { gogo: &gov_v1beta1_types.VotingParams{ VotingPeriod: time.Duration(rapidproto.MaxDurationSeconds*1e9) + 999999999, }, - pulsar: &gov_v1beta1_api.VotingParams{VotingPeriod: &durationpb.Duration{ - Seconds: rapidproto.MaxDurationSeconds + 1, Nanos: 999999999, - }}, - pulsarMarshalFails: true, }, // amino.dont_omitempty + empty/nil lists produce some surprising results "bank/send_authorization/empty_coins": { - gogo: &banktypes.SendAuthorization{SpendLimit: []types.Coin{}}, - pulsar: &bankapi.SendAuthorization{SpendLimit: []*v1beta1.Coin{}}, + gogo: &banktypes.SendAuthorization{SpendLimit: []types.Coin{}}, }, "bank/send_authorization/nil_coins": { - gogo: &banktypes.SendAuthorization{SpendLimit: nil}, - pulsar: &bankapi.SendAuthorization{SpendLimit: nil}, + gogo: &banktypes.SendAuthorization{SpendLimit: nil}, }, "bank/send_authorization/empty_list": { - gogo: &banktypes.SendAuthorization{AllowList: []string{}}, - pulsar: &bankapi.SendAuthorization{AllowList: []string{}}, + gogo: &banktypes.SendAuthorization{AllowList: []string{}}, }, "bank/send_authorization/nil_list": { - gogo: &banktypes.SendAuthorization{AllowList: nil}, - pulsar: &bankapi.SendAuthorization{AllowList: nil}, + gogo: &banktypes.SendAuthorization{AllowList: nil}, }, "bank/msg_multi_send/nil_everything": { - gogo: &banktypes.MsgMultiSend{}, - pulsar: &bankapi.MsgMultiSend{}, + gogo: &banktypes.MsgMultiSend{}, }, "gov/v1_msg_submit_proposal": { - gogo: &gov_v1_types.MsgSubmitProposal{}, - pulsar: &gov_v1_api.MsgSubmitProposal{}, - }, - "slashing/params/empty_dec": { - gogo: &slashingtypes.Params{DowntimeJailDuration: 1e9 + 7}, - pulsar: &slashingapi.Params{ - DowntimeJailDuration: &durationpb.Duration{Seconds: 1, Nanos: 7}, - }, + gogo: &gov_v1_types.MsgSubmitProposal{}, }, // This test cases demonstrates the expected contract and proper way to set a cosmos.Dec field represented // as bytes in protobuf message, namely: // dec10bz, _ := types.NewDec(10).Marshal() "slashing/params/dec": { gogo: &slashingtypes.Params{ - DowntimeJailDuration: 1e9 + 7, - MinSignedPerWindow: math.LegacyNewDec(10), - }, - pulsar: &slashingapi.Params{ - DowntimeJailDuration: &durationpb.Duration{Seconds: 1, Nanos: 7}, - MinSignedPerWindow: dec10bz, + DowntimeJailDuration: 1e9 + 7, + MinSignedPerWindow: math.LegacyNewDec(10), + SlashFractionDoubleSign: math.LegacyZeroDec(), + SlashFractionDowntime: math.LegacyZeroDec(), }, }, "staking/msg_update_params": { gogo: &stakingtypes.MsgUpdateParams{ Params: stakingtypes.Params{ - UnbondingTime: 0, - KeyRotationFee: types.Coin{}, - }, - }, - pulsar: &stakingapi.MsgUpdateParams{ - Params: &stakingapi.Params{ - UnbondingTime: &durationpb.Duration{Seconds: 0}, - KeyRotationFee: &v1beta1.Coin{}, + UnbondingTime: 0, + KeyRotationFee: types.Coin{}, + MinCommissionRate: math.LegacyZeroDec(), }, }, }, "staking/create_validator": { - gogo: &stakingtypes.MsgCreateValidator{Pubkey: pubkeyAny}, - pulsar: &stakingapi.MsgCreateValidator{ - Pubkey: pubkeyAnyPulsar, - Description: &stakingapi.Description{}, - Commission: &stakingapi.CommissionRates{}, - Value: &v1beta1.Coin{}, + gogo: &stakingtypes.MsgCreateValidator{ + Pubkey: pubkeyAny, + Commission: stakingtypes.CommissionRates{ + Rate: dec5point4, + MaxRate: math.LegacyZeroDec(), + MaxChangeRate: math.LegacyZeroDec(), + }, }, }, "staking/msg_cancel_unbonding_delegation_response": { - gogo: &stakingtypes.MsgCancelUnbondingDelegationResponse{}, - pulsar: &stakingapi.MsgCancelUnbondingDelegationResponse{}, + gogo: &stakingtypes.MsgCancelUnbondingDelegationResponse{}, }, "staking/stake_authorization_empty": { - gogo: &stakingtypes.StakeAuthorization{}, - pulsar: &stakingapi.StakeAuthorization{}, + gogo: &stakingtypes.StakeAuthorization{}, }, "staking/stake_authorization_allow": { gogo: &stakingtypes.StakeAuthorization{ @@ -440,84 +343,40 @@ func TestAminoJSON_LegacyParity(t *testing.T) { }, AuthorizationType: stakingtypes.AuthorizationType_AUTHORIZATION_TYPE_DELEGATE, }, - pulsar: &stakingapi.StakeAuthorization{ - MaxTokens: &v1beta1.Coin{Denom: "foo", Amount: "123"}, - Validators: &stakingapi.StakeAuthorization_AllowList{ - AllowList: &stakingapi.StakeAuthorization_Validators{ - Address: []string{"foo"}, - }, - }, - AuthorizationType: stakingapi.AuthorizationType_AUTHORIZATION_TYPE_DELEGATE, - }, }, "vesting/base_account_empty": { - gogo: &vestingtypes.BaseVestingAccount{BaseAccount: &authtypes.BaseAccount{}}, - pulsar: &vestingapi.BaseVestingAccount{BaseAccount: &authapi.BaseAccount{}}, + gogo: &vestingtypes.BaseVestingAccount{BaseAccount: &authtypes.BaseAccount{}}, }, "vesting/base_account_pubkey": { gogo: &vestingtypes.BaseVestingAccount{ BaseAccount: &authtypes.BaseAccount{PubKey: pubkeyAny}, }, - pulsar: &vestingapi.BaseVestingAccount{ - BaseAccount: &authapi.BaseAccount{PubKey: pubkeyAnyPulsar}, - }, }, "math/int_as_string": { - gogo: &gogo_testpb.IntAsString{IntAsString: math.NewInt(123)}, - pulsar: &pulsar_testpb.IntAsString{IntAsString: "123"}, + gogo: &gogo_testpb.IntAsString{IntAsString: math.NewInt(123)}, }, "math/int_as_string/empty": { - gogo: &gogo_testpb.IntAsString{}, - pulsar: &pulsar_testpb.IntAsString{}, + gogo: &gogo_testpb.IntAsString{}, }, "math/int_as_bytes": { - gogo: &gogo_testpb.IntAsBytes{IntAsBytes: math.NewInt(123)}, - pulsar: &pulsar_testpb.IntAsBytes{IntAsBytes: int123bz}, + gogo: &gogo_testpb.IntAsBytes{IntAsBytes: math.NewInt(123)}, }, "math/int_as_bytes/empty": { - gogo: &gogo_testpb.IntAsBytes{}, - pulsar: &pulsar_testpb.IntAsBytes{}, + gogo: &gogo_testpb.IntAsBytes{}, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { gogoBytes := fixture.MarshalLegacyAminoJSON(t, tc.gogo) - pulsarBytes, err := aj.Marshal(tc.pulsar) - if tc.pulsarMarshalFails { - require.Error(t, err) - return - } + dynamicBytes, err := aj.Marshal(fixture.DynamicMessage(t, tc.gogo)) require.NoError(t, err) - fmt.Printf("pulsar: %s\n", string(pulsarBytes)) + fmt.Printf("pulsar: %s\n", string(dynamicBytes)) fmt.Printf(" gogo: %s\n", string(gogoBytes)) - require.Equal(t, string(gogoBytes), string(pulsarBytes)) - - pulsarProtoBytes, err := proto.Marshal(tc.pulsar) - require.NoError(t, err) - - gogoType := reflect.TypeOf(tc.gogo).Elem() - newGogo := reflect.New(gogoType).Interface().(gogoproto.Message) - - err = fixture.UnmarshalGogoProto(pulsarProtoBytes, newGogo) - if tc.protoUnmarshalFails { - require.Error(t, err) - return - } - require.NoError(t, err) - - newGogoBytes := fixture.MarshalLegacyAminoJSON(t, newGogo) - if tc.roundTripUnequal { - require.NotEqual(t, string(gogoBytes), string(newGogoBytes)) - return - } - require.Equal(t, string(gogoBytes), string(newGogoBytes)) + require.Equal(t, string(gogoBytes), string(dynamicBytes)) // test amino json signer handler equivalence - if !proto.HasExtension( - tc.pulsar.ProtoReflect().Descriptor().Options(), - msgv1.E_Signer, - ) { + if !proto.HasExtension(fixture.MessageDescriptor(t, tc.gogo).Options(), msgv1.E_Signer) { // not signable return } diff --git a/tests/integration/tx/aminojson/notes.txt b/tests/integration/tx/aminojson/notes.txt new file mode 100644 index 000000000000..83400af18d92 --- /dev/null +++ b/tests/integration/tx/aminojson/notes.txt @@ -0,0 +1,7 @@ +x/auth/tx.builder.GetTx(): + - given a v1 message, marshals into any via gogoproto.Marshal + - given a v2 message, marshals into any via protov2.Marshal + +LegacyDec.Marshal on an uninitialized (nil) value returns a "0" +LegacyDec.Unmarshal("0") -> LegacyDec(0) +LegacyDec.Marshal(0) -> "0.000000000000000000" \ No newline at end of file diff --git a/tests/integration/tx/internal/util.go b/tests/integration/tx/internal/util.go index 43ff0606b47e..f726400b8a7a 100644 --- a/tests/integration/tx/internal/util.go +++ b/tests/integration/tx/internal/util.go @@ -4,17 +4,23 @@ import ( "bytes" "context" "encoding/json" + "fmt" "testing" + cosmos_proto "github.com/cosmos/cosmos-proto" "github.com/stretchr/testify/require" "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/reflect/protoreflect" + "google.golang.org/protobuf/types/dynamicpb" "cosmossdk.io/core/transaction" + "cosmossdk.io/math" "cosmossdk.io/x/tx/signing" "cosmossdk.io/x/tx/signing/aminojson" "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/codec" + codectypes "github.com/cosmos/cosmos-sdk/codec/types" "github.com/cosmos/cosmos-sdk/std" "github.com/cosmos/cosmos-sdk/tests/integration/tx/internal/pulsar/testpb" "github.com/cosmos/cosmos-sdk/types" @@ -23,6 +29,7 @@ import ( "github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx" authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing" "github.com/cosmos/cosmos-sdk/x/auth/tx" + gogoproto "github.com/cosmos/gogoproto/proto" ) func ProvideCustomGetSigner() signing.CustomGetSigner { @@ -52,6 +59,7 @@ type SigningFixture struct { legacy *codec.LegacyAmino protoCodec *codec.ProtoCodec options SigningFixtureOptions + registry codectypes.InterfaceRegistry } type SigningFixtureOptions struct { @@ -104,6 +112,7 @@ func NewSigningFixture( legacy: legacyAminoCodec, options: options, protoCodec: protoCodec, + registry: interfaceRegistry, } } @@ -130,13 +139,6 @@ func (s *SigningFixture) RequireLegacyAminoEquivalent(t *testing.T, msg transact } adaptableTx, ok := theTx.(authsigning.V2AdaptableTx) require.True(t, ok) - handler := aminojson.NewSignModeHandler(aminojson.SignModeHandlerOptions{}) - signBz, err := handler.GetSignBytes( - context.Background(), - signerData, - adaptableTx.GetSigningTxData(), - ) - require.NoError(t, err) legacytx.RegressionTestingAminoCodec = s.legacy defer func() { @@ -153,6 +155,15 @@ func (s *SigningFixture) RequireLegacyAminoEquivalent(t *testing.T, msg transact }, theTx) require.NoError(t, err) + + handler := aminojson.NewSignModeHandler(aminojson.SignModeHandlerOptions{}) + signBz, err := handler.GetSignBytes( + context.Background(), + signerData, + adaptableTx.GetSigningTxData(), + ) + require.NoError(t, err) + require.Truef(t, bytes.Equal(legacyBz, signBz), "legacy: %s\n x/tx: %s", string(legacyBz), string(signBz)) @@ -174,6 +185,66 @@ func (s *SigningFixture) UnmarshalGogoProto(bz []byte, ptr transaction.Msg) erro return s.protoCodec.Unmarshal(bz, ptr) } +func (s *SigningFixture) MessageDescriptor(t *testing.T, msg transaction.Msg) protoreflect.MessageDescriptor { + t.Helper() + typeName := gogoproto.MessageName(msg) + msgDesc, err := s.registry.FindDescriptorByName(protoreflect.FullName(typeName)) + require.NoError(t, err) + return msgDesc.(protoreflect.MessageDescriptor) +} + +// DynamicMessage is identical to the Decoder implementation in +// https://github.com/cosmos/cosmos-sdk/blob/6d2f6ff068c81c5783e01319beaa51c7dbb43edd/x/tx/decode/decode.go#L136 +// It is duplicated here to test dynamic message implementations specifically. +// The code path linked above is also covered in this package. +func (s *SigningFixture) DynamicMessage(t *testing.T, msg transaction.Msg) proto.Message { + t.Helper() + msgDesc := s.MessageDescriptor(t, msg) + protoBz, err := gogoproto.Marshal(msg) + require.NoError(t, err) + dynamicMsg := dynamicpb.NewMessageType(msgDesc).New().Interface() + err = proto.Unmarshal(protoBz, dynamicMsg) + require.NoError(t, err) + // err = postfixDynamicMessage(dynamicMsg.ProtoReflect()) + // require.NoError(t, err) + return dynamicMsg +} + +func postfixDynamicMessage(msg protoreflect.Message) error { + fields := msg.Descriptor().Fields() + for i := 0; i < fields.Len(); i++ { + field := fields.Get(i) + v := msg.Get(field) + + switch val := v.Interface().(type) { + case protoreflect.Message: + err := postfixDynamicMessage(val) + if err != nil { + return err + } + case string: + fieldOpts := field.Options() + if proto.HasExtension(fieldOpts, cosmos_proto.E_Scalar) { + scalar := proto.GetExtension(fieldOpts, cosmos_proto.E_Scalar).(string) + switch scalar { + case "cosmos.Dec": + dec := &math.LegacyDec{} + err := dec.Unmarshal([]byte(val)) + if err != nil { + return err + } + decStr := dec.String() + fmt.Printf("set %s=%s at %d\n", field.Name(), decStr, field.Number()) + //valStr := protoreflect.ValueOfString("5.400000000000000000") + valStr := protoreflect.ValueOfString(decStr) + msg.Set(field, valStr) + } + } + } + } + return nil +} + // sortJson sorts the JSON bytes by way of the side effect of unmarshalling and remarshalling // the JSON using encoding/json. This hacky way of sorting JSON fields was used by the legacy // amino JSON encoding x/auth/migrations/legacytx.StdSignBytes. It is used here ensure the x/tx diff --git a/x/tx/decode/decode.go b/x/tx/decode/decode.go index 994d54cf488b..bd312c948c4a 100644 --- a/x/tx/decode/decode.go +++ b/x/tx/decode/decode.go @@ -7,6 +7,9 @@ import ( "reflect" "strings" + "cosmossdk.io/math" + cosmos_proto "github.com/cosmos/cosmos-proto" + gogoproto "github.com/cosmos/gogoproto/proto" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/reflect/protoreflect" @@ -138,6 +141,10 @@ func (d *Decoder) Decode(txBytes []byte) (*DecodedTx, error) { if err != nil { return nil, err } + // err = postfixDynamicMessage(dynamicMsg.ProtoReflect()) + // if err != nil { + // return nil, err + // } dynamicMsgs = append(dynamicMsgs, dynamicMsg) // unmarshal into gogoproto message @@ -177,6 +184,40 @@ func (d *Decoder) Decode(txBytes []byte) (*DecodedTx, error) { }, nil } +func postfixDynamicMessage(msg protoreflect.Message) error { + fields := msg.Descriptor().Fields() + for i := 0; i < fields.Len(); i++ { + field := fields.Get(i) + v := msg.Get(field) + + switch val := v.Interface().(type) { + case protoreflect.Message: + err := postfixDynamicMessage(val) + if err != nil { + return err + } + case string: + fieldOpts := field.Options() + if proto.HasExtension(fieldOpts, cosmos_proto.E_Scalar) { + scalar := proto.GetExtension(fieldOpts, cosmos_proto.E_Scalar).(string) + switch scalar { + case "cosmos.Dec": + dec := &math.LegacyDec{} + err := dec.Unmarshal([]byte(val)) + if err != nil { + return err + } + decStr := dec.String() + fmt.Printf("set %s=%s at %d\n", field.Name(), decStr, field.Number()) + valStr := protoreflect.ValueOfString(decStr) + msg.Set(field, valStr) + } + } + } + } + return nil +} + // Hash implements the interface for the Tx interface. func (dtx *DecodedTx) Hash() [32]byte { if !dtx.cachedHashed { diff --git a/x/tx/signing/aminojson/encoder.go b/x/tx/signing/aminojson/encoder.go index 9fe589c0e544..dce9386b9ca8 100644 --- a/x/tx/signing/aminojson/encoder.go +++ b/x/tx/signing/aminojson/encoder.go @@ -27,6 +27,8 @@ func cosmosIntEncoder(_ *Encoder, v protoreflect.Value, w io.Writer) error { if val == "" { return jsonMarshal(w, "0") } + // TODO + // I think this needs a similar path as cosmosDecEncoder return jsonMarshal(w, val) case []byte: if len(val) == 0 { @@ -51,7 +53,12 @@ func cosmosDecEncoder(_ *Encoder, v protoreflect.Value, w io.Writer) error { if val == "" { return jsonMarshal(w, "0") } - return jsonMarshal(w, val) + var dec math.LegacyDec + err := dec.Unmarshal([]byte(val)) + if err != nil { + return err + } + return jsonMarshal(w, dec.String()) case []byte: if len(val) == 0 { return jsonMarshal(w, "0") @@ -125,27 +132,41 @@ func keyFieldEncoder(_ *Encoder, msg protoreflect.Message, w io.Writer) error { } type moduleAccountPretty struct { - Address string `json:"address"` - PubKey string `json:"public_key"` AccountNumber uint64 `json:"account_number"` - Sequence uint64 `json:"sequence"` + Address string `json:"address"` Name string `json:"name"` Permissions []string `json:"permissions"` + PubKey string `json:"public_key"` + Sequence uint64 `json:"sequence"` } // moduleAccountEncoder replicates the behavior in // https://github.com/cosmos/cosmos-sdk/blob/41a3dfeced2953beba3a7d11ec798d17ee19f506/x/auth/types/account.go#L230-L254 func moduleAccountEncoder(_ *Encoder, msg protoreflect.Message, w io.Writer) error { - ma := msg.Interface().(*authapi.ModuleAccount) + ma := &authapi.ModuleAccount{} + msgDesc := msg.Descriptor() + if msgDesc.FullName() != ma.ProtoReflect().Descriptor().FullName() { + return errors.New("moduleAccountEncoder: msg not a auth.ModuleAccount") + } + fields := msgDesc.Fields() + pretty := moduleAccountPretty{ PubKey: "", - Name: ma.Name, - Permissions: ma.Permissions, + Name: msg.Get(fields.ByName("name")).String(), + Permissions: make([]string, 0), } - if ma.BaseAccount != nil { - pretty.Address = ma.BaseAccount.Address - pretty.AccountNumber = ma.BaseAccount.AccountNumber - pretty.Sequence = ma.BaseAccount.Sequence + permissions := msg.Get(fields.ByName("permissions")).List() + for i := 0; i < permissions.Len(); i++ { + pretty.Permissions = append(pretty.Permissions, permissions.Get(i).String()) + } + + if msg.Has(fields.ByName("base_account")) { + baseAccount := msg.Get(fields.ByName("base_account")) + baMsg := baseAccount.Message() + bamdFields := baMsg.Descriptor().Fields() + pretty.Address = baMsg.Get(bamdFields.ByName("address")).String() + pretty.AccountNumber = baMsg.Get(bamdFields.ByName("account_number")).Uint() + pretty.Sequence = baMsg.Get(bamdFields.ByName("sequence")).Uint() } else { pretty.Address = "" pretty.AccountNumber = 0 @@ -166,29 +187,34 @@ func moduleAccountEncoder(_ *Encoder, msg protoreflect.Message, w io.Writer) err // also see: // https://github.com/cosmos/cosmos-sdk/blob/b49f948b36bc991db5be431607b475633aed697e/proto/cosmos/crypto/multisig/keys.proto#L15/ func thresholdStringEncoder(enc *Encoder, msg protoreflect.Message, w io.Writer) error { - pk, ok := msg.Interface().(*multisig.LegacyAminoPubKey) - if !ok { + pk := &multisig.LegacyAminoPubKey{} + msgDesc := msg.Descriptor() + fields := msgDesc.Fields() + if msgDesc.FullName() != pk.ProtoReflect().Descriptor().FullName() { return errors.New("thresholdStringEncoder: msg not a multisig.LegacyAminoPubKey") } - _, err := fmt.Fprintf(w, `{"threshold":"%d","pubkeys":`, pk.Threshold) - if err != nil { - return err - } - - if len(pk.PublicKeys) == 0 { - _, err = io.WriteString(w, `[]}`) - return err - } - fields := msg.Descriptor().Fields() pubkeysField := fields.ByName("public_keys") pubkeys := msg.Get(pubkeysField).List() - err = enc.marshalList(pubkeys, pubkeysField, w) + _, err := io.WriteString(w, `{"pubkeys":`) if err != nil { return err } - _, err = io.WriteString(w, `}`) + if pubkeys.Len() == 0 { + _, err := io.WriteString(w, `[]`) + if err != nil { + return err + } + } else { + err := enc.marshalList(pubkeys, pubkeysField, w) + if err != nil { + return err + } + } + + threshold := fields.ByName("threshold") + _, err = fmt.Fprintf(w, `,"threshold":"%d"}`, msg.Get(threshold).Uint()) return err } From 8988b88936923bed49d6a11b4b93fa66a055b019 Mon Sep 17 00:00:00 2001 From: Matt Kocubinski Date: Thu, 19 Sep 2024 16:22:12 -0500 Subject: [PATCH 04/10] more fixes --- tests/integration/rapidgen/rapidgen.go | 1 - .../tx/aminojson/aminojson_test.go | 171 +++++++++--------- x/tx/signing/aminojson/encoder.go | 5 +- 3 files changed, 89 insertions(+), 88 deletions(-) diff --git a/tests/integration/rapidgen/rapidgen.go b/tests/integration/rapidgen/rapidgen.go index e33e794b00f2..2d2f5ab48630 100644 --- a/tests/integration/rapidgen/rapidgen.go +++ b/tests/integration/rapidgen/rapidgen.go @@ -222,7 +222,6 @@ var ( NonsignableTypes = []GeneratedType{ GenType(&authtypes.Params{}, &authapi.Params{}, GenOpts), GenType(&authtypes.BaseAccount{}, &authapi.BaseAccount{}, GenOpts.WithAnyTypes(&ed25519.PubKey{})), - GenType(&authtypes.ModuleAccount{}, &authapi.ModuleAccount{}, GenOpts.WithAnyTypes(&ed25519.PubKey{})), GenType(&authtypes.ModuleCredential{}, &authapi.ModuleCredential{}, GenOpts), GenType(&authztypes.GenericAuthorization{}, &authzapi.GenericAuthorization{}, GenOpts), diff --git a/tests/integration/tx/aminojson/aminojson_test.go b/tests/integration/tx/aminojson/aminojson_test.go index 696106f50159..4e4e30c27873 100644 --- a/tests/integration/tx/aminojson/aminojson_test.go +++ b/tests/integration/tx/aminojson/aminojson_test.go @@ -1,9 +1,10 @@ package aminojson import ( - "context" + "bytes" + "encoding/json" "fmt" - "strings" + stdmath "math" "testing" "time" @@ -19,7 +20,6 @@ import ( bankapi "cosmossdk.io/api/cosmos/bank/v1beta1" v1beta1 "cosmossdk.io/api/cosmos/base/v1beta1" msgv1 "cosmossdk.io/api/cosmos/msg/v1" - txv1beta1 "cosmossdk.io/api/cosmos/tx/v1beta1" "cosmossdk.io/math" authztypes "cosmossdk.io/x/authz" authzmodule "cosmossdk.io/x/authz/module" @@ -40,7 +40,6 @@ import ( "cosmossdk.io/x/staking" stakingtypes "cosmossdk.io/x/staking/types" "cosmossdk.io/x/tx/signing/aminojson" - signing_testutil "cosmossdk.io/x/tx/signing/testutil" "cosmossdk.io/x/upgrade" codectestutil "github.com/cosmos/cosmos-sdk/codec/testutil" @@ -54,11 +53,7 @@ import ( "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/bech32" "github.com/cosmos/cosmos-sdk/types/module/testutil" - signingtypes "github.com/cosmos/cosmos-sdk/types/tx/signing" "github.com/cosmos/cosmos-sdk/x/auth" - "github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx" - "github.com/cosmos/cosmos-sdk/x/auth/signing" - "github.com/cosmos/cosmos-sdk/x/auth/tx" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" "github.com/cosmos/cosmos-sdk/x/auth/vesting" vestingtypes "github.com/cosmos/cosmos-sdk/x/auth/vesting/types" @@ -79,8 +74,7 @@ import ( // In order for step 3 to work certain restrictions on the data generated in step 1 must be enforced and are described // by the mutation of genOpts passed to the generator. func TestAminoJSON_Equivalence(t *testing.T) { - encCfg := testutil.MakeTestEncodingConfig( - codectestutil.CodecOptions{}, + fixture := internal.NewSigningFixture(t, internal.SigningFixtureOptions{}, auth.AppModule{}, authzmodule.AppModule{}, bank.AppModule{}, @@ -96,9 +90,7 @@ func TestAminoJSON_Equivalence(t *testing.T) { upgrade.AppModule{}, vesting.AppModule{}, ) - legacytx.RegressionTestingAminoCodec = encCfg.Amino aj := aminojson.NewEncoder(aminojson.EncoderOptions{}) - // aj := aminojson.NewEncoder(aminojson.EncoderOptions{DoNotSortFields: true}) for _, tt := range rapidgen.DefaultGeneratedTypes { desc := tt.Pulsar.ProtoReflect().Descriptor() @@ -106,7 +98,7 @@ func TestAminoJSON_Equivalence(t *testing.T) { t.Run(name, func(t *testing.T) { gen := rapidproto.MessageGenerator(tt.Pulsar, tt.Opts) fmt.Printf("testing %s\n", tt.Pulsar.ProtoReflect().Descriptor().FullName()) - rapid.Check(t, func(t *rapid.T) { + rapid.Check(t, func(r *rapid.T) { // uncomment to debug; catch a panic and inspect application state // defer func() { // if r := recover(); r != nil { @@ -115,39 +107,44 @@ func TestAminoJSON_Equivalence(t *testing.T) { // } // }() - msg := gen.Draw(t, "msg") + msg := gen.Draw(r, "msg") postFixPulsarMessage(msg) // txBuilder.GetTx will fail if the msg has no signers // so it does not make sense to run these cases, apparently. - signers, err := encCfg.TxConfig.SigningContext().GetSigners(msg) - if len(signers) == 0 { - // skip - return - } - if err != nil { - if strings.Contains(err.Error(), "empty address string is not allowed") { - return - } - require.NoError(t, err) - } + // signers, err := encCfg.TxConfig.SigningContext().GetSigners(msg) + // if len(signers) == 0 { + // // skip + // return + // } + // if err != nil { + // if strings.Contains(err.Error(), "empty address string is not allowed") { + // return + // } + // require.NoError(t, err) + // } gogo := tt.Gogo sanity := tt.Pulsar protoBz, err := proto.Marshal(msg) - require.NoError(t, err) + require.NoError(r, err) err = proto.Unmarshal(protoBz, sanity) - require.NoError(t, err) + require.NoError(r, err) - err = encCfg.Codec.Unmarshal(protoBz, gogo) - require.NoError(t, err) + err = fixture.UnmarshalGogoProto(protoBz, gogo) + require.NoError(r, err) - legacyAminoJSON, err := encCfg.Amino.MarshalJSON(gogo) - require.NoError(t, err) + legacyAminoJSON := fixture.MarshalLegacyAminoJSON(t, gogo) aminoJSON, err := aj.Marshal(msg) - require.NoError(t, err) - require.Equal(t, string(legacyAminoJSON), string(aminoJSON)) + require.NoError(r, err) + if !bytes.Equal(legacyAminoJSON, aminoJSON) { + // play that back, wtf + againBz := fixture.MarshalLegacyAminoJSON(t, gogo) + require.Failf(r, "JSON mismatch", "legacy: %s\n x/tx: %s\n", + string(againBz), string(aminoJSON)) + } + require.Equal(r, string(legacyAminoJSON), string(aminoJSON)) // test amino json signer handler equivalence if !proto.HasExtension(desc.Options(), msgv1.E_Signer) { @@ -155,45 +152,7 @@ func TestAminoJSON_Equivalence(t *testing.T) { return } - handlerOptions := signing_testutil.HandlerArgumentOptions{ - ChainID: "test-chain", - Memo: "sometestmemo", - Msg: tt.Pulsar, - AccNum: 1, - AccSeq: 2, - SignerAddress: "signerAddress", - Fee: &txv1beta1.Fee{ - Amount: []*v1beta1.Coin{{Denom: "uatom", Amount: "1000"}}, - }, - } - - signerData, txData, err := signing_testutil.MakeHandlerArguments(handlerOptions) - require.NoError(t, err) - - handler := aminojson.NewSignModeHandler(aminojson.SignModeHandlerOptions{}) - signBz, err := handler.GetSignBytes(context.Background(), signerData, txData) - require.NoError(t, err) - - legacyHandler := tx.NewSignModeLegacyAminoJSONHandler() - txBuilder := encCfg.TxConfig.NewTxBuilder() - require.NoError(t, txBuilder.SetMsgs([]types.Msg{tt.Gogo}...)) - txBuilder.SetMemo(handlerOptions.Memo) - txBuilder.SetFeeAmount(types.Coins{types.NewInt64Coin("uatom", 1000)}) - theTx := txBuilder.GetTx() - - legacySigningData := signing.SignerData{ - ChainID: handlerOptions.ChainID, - Address: handlerOptions.SignerAddress, - AccountNumber: handlerOptions.AccNum, - Sequence: handlerOptions.AccSeq, - } - legacySignBz, err := legacyHandler.GetSignBytes( - signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, - legacySigningData, - theTx, - ) - require.NoError(t, err) - require.Equal(t, string(legacySignBz), string(signBz)) + fixture.RequireLegacyAminoEquivalent(t, gogo) }) }) } @@ -212,22 +171,42 @@ func TestAminoJSON_LegacyParity(t *testing.T) { genericAuth, _ := codectypes.NewAnyWithValue(&authztypes.GenericAuthorization{Msg: "foo"}) pubkeyAny, _ := codectypes.NewAnyWithValue(&secp256k1types.PubKey{Key: []byte("foo")}) dec5point4 := math.LegacyMustNewDecFromStr("5.4") + failingBaseAccount := authtypes.NewBaseAccountWithAddress(addr1) + failingBaseAccount.AccountNumber = stdmath.MaxUint64 cases := map[string]struct { - gogo gogoproto.Message + gogo gogoproto.Message + fails bool }{ "auth/params": { gogo: &authtypes.Params{TxSigLimit: 10}, }, - "auth/module_account": { + "auth/module_account_nil_permissions": { + gogo: &authtypes.ModuleAccount{ + BaseAccount: authtypes.NewBaseAccountWithAddress( + addr1, + ), + }, + }, + "auth/module_account/max_uint64": { + gogo: &authtypes.ModuleAccount{ + BaseAccount: failingBaseAccount, + }, + fails: true, + }, + "auth/module_account_empty_permissions": { gogo: &authtypes.ModuleAccount{ BaseAccount: authtypes.NewBaseAccountWithAddress( addr1, - ), Permissions: []string{}, + ), + // empty set and nil are indistinguishable from the protoreflect API since they both + // marshal to zero proto bytes, there empty set is not supported. + Permissions: []string{}, }, + fails: true, }, "auth/base_account": { - gogo: &authtypes.BaseAccount{Address: addr1.String(), PubKey: pubkeyAny}, + gogo: &authtypes.BaseAccount{Address: addr1.String(), PubKey: pubkeyAny, AccountNumber: 1, Sequence: 2}, }, "authz/msg_grant": { gogo: &authztypes.MsgGrant{ @@ -262,7 +241,7 @@ func TestAminoJSON_LegacyParity(t *testing.T) { "crypto/legacy_amino_pubkey": { gogo: &multisig.LegacyAminoPubKey{PubKeys: []*codectypes.Any{pubkeyAny}}, }, - "crypto/legacy_amino_pubkey/empty": { + "crypto/legacy_amino_pubkey_empty": { gogo: &multisig.LegacyAminoPubKey{}, }, "consensus/evidence_params/duration": { @@ -297,9 +276,6 @@ func TestAminoJSON_LegacyParity(t *testing.T) { "gov/v1_msg_submit_proposal": { gogo: &gov_v1_types.MsgSubmitProposal{}, }, - // This test cases demonstrates the expected contract and proper way to set a cosmos.Dec field represented - // as bytes in protobuf message, namely: - // dec10bz, _ := types.NewDec(10).Marshal() "slashing/params/dec": { gogo: &slashingtypes.Params{ DowntimeJailDuration: 1e9 + 7, @@ -344,6 +320,15 @@ func TestAminoJSON_LegacyParity(t *testing.T) { AuthorizationType: stakingtypes.AuthorizationType_AUTHORIZATION_TYPE_DELEGATE, }, }, + "staking/stake_authorization_deny": { + gogo: &stakingtypes.StakeAuthorization{ + MaxTokens: &types.Coin{Denom: "foo", Amount: math.NewInt(123)}, + Validators: &stakingtypes.StakeAuthorization_DenyList{ + DenyList: &stakingtypes.StakeAuthorization_Validators{}, + }, + AuthorizationType: stakingtypes.AuthorizationType_AUTHORIZATION_TYPE_DELEGATE, + }, + }, "vesting/base_account_empty": { gogo: &vestingtypes.BaseVestingAccount{BaseAccount: &authtypes.BaseAccount{}}, }, @@ -367,13 +352,17 @@ func TestAminoJSON_LegacyParity(t *testing.T) { } for name, tc := range cases { t.Run(name, func(t *testing.T) { - gogoBytes := fixture.MarshalLegacyAminoJSON(t, tc.gogo) + legacyBytes := fixture.MarshalLegacyAminoJSON(t, tc.gogo) dynamicBytes, err := aj.Marshal(fixture.DynamicMessage(t, tc.gogo)) require.NoError(t, err) - fmt.Printf("pulsar: %s\n", string(dynamicBytes)) - fmt.Printf(" gogo: %s\n", string(gogoBytes)) - require.Equal(t, string(gogoBytes), string(dynamicBytes)) + fmt.Printf("legacy: %s\n", string(legacyBytes)) + fmt.Printf(" sut: %s\n", string(dynamicBytes)) + if tc.fails { + require.NotEqual(t, string(legacyBytes), string(dynamicBytes)) + return + } + require.Equal(t, string(legacyBytes), string(dynamicBytes)) // test amino json signer handler equivalence if !proto.HasExtension(fixture.MessageDescriptor(t, tc.gogo).Options(), msgv1.E_Signer) { @@ -502,4 +491,18 @@ func TestJSONSorting(t *testing.T) { } fixture.RequireLegacyAminoEquivalent(t, msgGrant) + + addr1 := types.AccAddress("addr1") + ba := authtypes.NewBaseAccountWithAddress(addr1) + ma := &authtypes.ModuleAccount{BaseAccount: ba} + bz, err := json.Marshal(ma) + require.NoError(t, err) + fmt.Printf("ma: %+v -> %s\n", ma, string(bz)) + protoBz, _ := gogoproto.Marshal(ma) + ma2 := &authtypes.ModuleAccount{} + err = gogoproto.Unmarshal(protoBz, ma2) + bz, err = json.Marshal(ma2) + require.NoError(t, err) + fmt.Printf("ma: %+v -> %s\n", ma2, string(bz)) + } diff --git a/x/tx/signing/aminojson/encoder.go b/x/tx/signing/aminojson/encoder.go index dce9386b9ca8..ab66d9b03da7 100644 --- a/x/tx/signing/aminojson/encoder.go +++ b/x/tx/signing/aminojson/encoder.go @@ -151,9 +151,8 @@ func moduleAccountEncoder(_ *Encoder, msg protoreflect.Message, w io.Writer) err fields := msgDesc.Fields() pretty := moduleAccountPretty{ - PubKey: "", - Name: msg.Get(fields.ByName("name")).String(), - Permissions: make([]string, 0), + PubKey: "", + Name: msg.Get(fields.ByName("name")).String(), } permissions := msg.Get(fields.ByName("permissions")).List() for i := 0; i < permissions.Len(); i++ { From 651b8edff507abdebd7f521089d3ef503c5c55df Mon Sep 17 00:00:00 2001 From: Matt Kocubinski Date: Thu, 19 Sep 2024 16:38:57 -0500 Subject: [PATCH 05/10] all green, cleaning up --- tests/integration/rapidgen/rapidgen.go | 4 +- .../tx/aminojson/aminojson_test.go | 66 ++----------------- 2 files changed, 8 insertions(+), 62 deletions(-) diff --git a/tests/integration/rapidgen/rapidgen.go b/tests/integration/rapidgen/rapidgen.go index 2d2f5ab48630..0060ca1976ce 100644 --- a/tests/integration/rapidgen/rapidgen.go +++ b/tests/integration/rapidgen/rapidgen.go @@ -259,7 +259,9 @@ var ( GenType(&slashingtypes.Params{}, &slashingapi.Params{}, GenOpts.WithDisallowNil()), - GenType(&stakingtypes.StakeAuthorization{}, &stakingapi.StakeAuthorization{}, GenOpts), + // JSON ordering of one of fields to be fixed in https://github.com/cosmos/cosmos-sdk/pull/21782 + // TODO uncomment once merged + // GenType(&stakingtypes.StakeAuthorization{}, &stakingapi.StakeAuthorization{}, GenOpts), GenType(&upgradetypes.CancelSoftwareUpgradeProposal{}, &upgradeapi.CancelSoftwareUpgradeProposal{}, GenOpts), //nolint:staticcheck // testing legacy code path GenType(&upgradetypes.SoftwareUpgradeProposal{}, &upgradeapi.SoftwareUpgradeProposal{}, GenOpts.WithDisallowNil()), //nolint:staticcheck // testing legacy code path diff --git a/tests/integration/tx/aminojson/aminojson_test.go b/tests/integration/tx/aminojson/aminojson_test.go index 4e4e30c27873..3afdbaa6b203 100644 --- a/tests/integration/tx/aminojson/aminojson_test.go +++ b/tests/integration/tx/aminojson/aminojson_test.go @@ -2,14 +2,11 @@ package aminojson import ( "bytes" - "encoding/json" "fmt" stdmath "math" "testing" "time" - "github.com/cosmos/cosmos-sdk/tests/integration/tx/internal" - "github.com/cosmos/cosmos-proto/rapidproto" gogoproto "github.com/cosmos/gogoproto/proto" "github.com/stretchr/testify/require" @@ -48,6 +45,7 @@ import ( "github.com/cosmos/cosmos-sdk/crypto/keys/multisig" secp256k1types "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" "github.com/cosmos/cosmos-sdk/tests/integration/rapidgen" + "github.com/cosmos/cosmos-sdk/tests/integration/tx/internal" gogo_testpb "github.com/cosmos/cosmos-sdk/tests/integration/tx/internal/gogo/testpb" "github.com/cosmos/cosmos-sdk/testutil/testdata" "github.com/cosmos/cosmos-sdk/types" @@ -109,19 +107,6 @@ func TestAminoJSON_Equivalence(t *testing.T) { msg := gen.Draw(r, "msg") postFixPulsarMessage(msg) - // txBuilder.GetTx will fail if the msg has no signers - // so it does not make sense to run these cases, apparently. - // signers, err := encCfg.TxConfig.SigningContext().GetSigners(msg) - // if len(signers) == 0 { - // // skip - // return - // } - // if err != nil { - // if strings.Contains(err.Error(), "empty address string is not allowed") { - // return - // } - // require.NoError(t, err) - // } gogo := tt.Gogo sanity := tt.Pulsar @@ -139,12 +124,9 @@ func TestAminoJSON_Equivalence(t *testing.T) { aminoJSON, err := aj.Marshal(msg) require.NoError(r, err) if !bytes.Equal(legacyAminoJSON, aminoJSON) { - // play that back, wtf - againBz := fixture.MarshalLegacyAminoJSON(t, gogo) require.Failf(r, "JSON mismatch", "legacy: %s\n x/tx: %s\n", - string(againBz), string(aminoJSON)) + string(legacyAminoJSON), string(aminoJSON)) } - require.Equal(r, string(legacyAminoJSON), string(aminoJSON)) // test amino json signer handler equivalence if !proto.HasExtension(desc.Options(), msgv1.E_Signer) { @@ -328,6 +310,9 @@ func TestAminoJSON_LegacyParity(t *testing.T) { }, AuthorizationType: stakingtypes.AuthorizationType_AUTHORIZATION_TYPE_DELEGATE, }, + // to be fixed in https://github.com/cosmos/cosmos-sdk/pull/21782 + // TODO remove once merged + fails: true, }, "vesting/base_account_empty": { gogo: &vestingtypes.BaseVestingAccount{BaseAccount: &authtypes.BaseAccount{}}, @@ -465,44 +450,3 @@ func postFixPulsarMessage(msg proto.Message) { } } } - -func TestJSONSorting(t *testing.T) { - fixture := internal.NewSigningFixture( - t, - internal.SigningFixtureOptions{}, - authzmodule.AppModule{}, - staking.AppModule{}, - ) - - // define message - authorization := &stakingtypes.StakeAuthorization{ - MaxTokens: &types.Coin{Denom: "foo", Amount: math.NewInt(123)}, - Validators: &stakingtypes.StakeAuthorization_AllowList{ - AllowList: &stakingtypes.StakeAuthorization_Validators{Address: []string{"foo"}}, - }, - AuthorizationType: stakingtypes.AuthorizationType_AUTHORIZATION_TYPE_DELEGATE, - } - authorizationAny, err := codectypes.NewAnyWithValue(authorization) - require.NoError(t, err) - msgGrant := &authztypes.MsgGrant{ - Granter: "granter", - Grantee: "grantee", - Grant: authztypes.Grant{Authorization: authorizationAny}, - } - - fixture.RequireLegacyAminoEquivalent(t, msgGrant) - - addr1 := types.AccAddress("addr1") - ba := authtypes.NewBaseAccountWithAddress(addr1) - ma := &authtypes.ModuleAccount{BaseAccount: ba} - bz, err := json.Marshal(ma) - require.NoError(t, err) - fmt.Printf("ma: %+v -> %s\n", ma, string(bz)) - protoBz, _ := gogoproto.Marshal(ma) - ma2 := &authtypes.ModuleAccount{} - err = gogoproto.Unmarshal(protoBz, ma2) - bz, err = json.Marshal(ma2) - require.NoError(t, err) - fmt.Printf("ma: %+v -> %s\n", ma2, string(bz)) - -} From 9e56d38227b3b51324d7de8fafde285954ab0985 Mon Sep 17 00:00:00 2001 From: Matt Kocubinski Date: Thu, 19 Sep 2024 16:41:18 -0500 Subject: [PATCH 06/10] rm --- tests/integration/tx/aminojson/notes.txt | 7 ------- 1 file changed, 7 deletions(-) delete mode 100644 tests/integration/tx/aminojson/notes.txt diff --git a/tests/integration/tx/aminojson/notes.txt b/tests/integration/tx/aminojson/notes.txt deleted file mode 100644 index 83400af18d92..000000000000 --- a/tests/integration/tx/aminojson/notes.txt +++ /dev/null @@ -1,7 +0,0 @@ -x/auth/tx.builder.GetTx(): - - given a v1 message, marshals into any via gogoproto.Marshal - - given a v2 message, marshals into any via protov2.Marshal - -LegacyDec.Marshal on an uninitialized (nil) value returns a "0" -LegacyDec.Unmarshal("0") -> LegacyDec(0) -LegacyDec.Marshal(0) -> "0.000000000000000000" \ No newline at end of file From e5ac23d01ad327949e4318e8965330a6c7389426 Mon Sep 17 00:00:00 2001 From: Matt Kocubinski Date: Thu, 19 Sep 2024 16:44:03 -0500 Subject: [PATCH 07/10] rm more --- tests/integration/tx/internal/util.go | 40 -------------------------- x/tx/decode/decode.go | 41 --------------------------- 2 files changed, 81 deletions(-) diff --git a/tests/integration/tx/internal/util.go b/tests/integration/tx/internal/util.go index f726400b8a7a..a10159f648c4 100644 --- a/tests/integration/tx/internal/util.go +++ b/tests/integration/tx/internal/util.go @@ -4,17 +4,14 @@ import ( "bytes" "context" "encoding/json" - "fmt" "testing" - cosmos_proto "github.com/cosmos/cosmos-proto" "github.com/stretchr/testify/require" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/reflect/protoreflect" "google.golang.org/protobuf/types/dynamicpb" "cosmossdk.io/core/transaction" - "cosmossdk.io/math" "cosmossdk.io/x/tx/signing" "cosmossdk.io/x/tx/signing/aminojson" @@ -205,46 +202,9 @@ func (s *SigningFixture) DynamicMessage(t *testing.T, msg transaction.Msg) proto dynamicMsg := dynamicpb.NewMessageType(msgDesc).New().Interface() err = proto.Unmarshal(protoBz, dynamicMsg) require.NoError(t, err) - // err = postfixDynamicMessage(dynamicMsg.ProtoReflect()) - // require.NoError(t, err) return dynamicMsg } -func postfixDynamicMessage(msg protoreflect.Message) error { - fields := msg.Descriptor().Fields() - for i := 0; i < fields.Len(); i++ { - field := fields.Get(i) - v := msg.Get(field) - - switch val := v.Interface().(type) { - case protoreflect.Message: - err := postfixDynamicMessage(val) - if err != nil { - return err - } - case string: - fieldOpts := field.Options() - if proto.HasExtension(fieldOpts, cosmos_proto.E_Scalar) { - scalar := proto.GetExtension(fieldOpts, cosmos_proto.E_Scalar).(string) - switch scalar { - case "cosmos.Dec": - dec := &math.LegacyDec{} - err := dec.Unmarshal([]byte(val)) - if err != nil { - return err - } - decStr := dec.String() - fmt.Printf("set %s=%s at %d\n", field.Name(), decStr, field.Number()) - //valStr := protoreflect.ValueOfString("5.400000000000000000") - valStr := protoreflect.ValueOfString(decStr) - msg.Set(field, valStr) - } - } - } - } - return nil -} - // sortJson sorts the JSON bytes by way of the side effect of unmarshalling and remarshalling // the JSON using encoding/json. This hacky way of sorting JSON fields was used by the legacy // amino JSON encoding x/auth/migrations/legacytx.StdSignBytes. It is used here ensure the x/tx diff --git a/x/tx/decode/decode.go b/x/tx/decode/decode.go index bd312c948c4a..994d54cf488b 100644 --- a/x/tx/decode/decode.go +++ b/x/tx/decode/decode.go @@ -7,9 +7,6 @@ import ( "reflect" "strings" - "cosmossdk.io/math" - cosmos_proto "github.com/cosmos/cosmos-proto" - gogoproto "github.com/cosmos/gogoproto/proto" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/reflect/protoreflect" @@ -141,10 +138,6 @@ func (d *Decoder) Decode(txBytes []byte) (*DecodedTx, error) { if err != nil { return nil, err } - // err = postfixDynamicMessage(dynamicMsg.ProtoReflect()) - // if err != nil { - // return nil, err - // } dynamicMsgs = append(dynamicMsgs, dynamicMsg) // unmarshal into gogoproto message @@ -184,40 +177,6 @@ func (d *Decoder) Decode(txBytes []byte) (*DecodedTx, error) { }, nil } -func postfixDynamicMessage(msg protoreflect.Message) error { - fields := msg.Descriptor().Fields() - for i := 0; i < fields.Len(); i++ { - field := fields.Get(i) - v := msg.Get(field) - - switch val := v.Interface().(type) { - case protoreflect.Message: - err := postfixDynamicMessage(val) - if err != nil { - return err - } - case string: - fieldOpts := field.Options() - if proto.HasExtension(fieldOpts, cosmos_proto.E_Scalar) { - scalar := proto.GetExtension(fieldOpts, cosmos_proto.E_Scalar).(string) - switch scalar { - case "cosmos.Dec": - dec := &math.LegacyDec{} - err := dec.Unmarshal([]byte(val)) - if err != nil { - return err - } - decStr := dec.String() - fmt.Printf("set %s=%s at %d\n", field.Name(), decStr, field.Number()) - valStr := protoreflect.ValueOfString(decStr) - msg.Set(field, valStr) - } - } - } - } - return nil -} - // Hash implements the interface for the Tx interface. func (dtx *DecodedTx) Hash() [32]byte { if !dtx.cachedHashed { From 62f47d87652281cbda769dc637014b741efbb322 Mon Sep 17 00:00:00 2001 From: Matt Kocubinski Date: Fri, 20 Sep 2024 09:57:29 -0500 Subject: [PATCH 08/10] fix test spec --- .../tx/aminojson/aminojson_test.go | 4 ++-- tests/integration/tx/context_test.go | 7 ++++++- tests/integration/tx/internal/util.go | 20 +++++++++---------- x/auth/migrations/legacytx/stdtx_test.go | 2 +- 4 files changed, 18 insertions(+), 15 deletions(-) diff --git a/tests/integration/tx/aminojson/aminojson_test.go b/tests/integration/tx/aminojson/aminojson_test.go index 3afdbaa6b203..fbaea363a1d3 100644 --- a/tests/integration/tx/aminojson/aminojson_test.go +++ b/tests/integration/tx/aminojson/aminojson_test.go @@ -341,8 +341,8 @@ func TestAminoJSON_LegacyParity(t *testing.T) { dynamicBytes, err := aj.Marshal(fixture.DynamicMessage(t, tc.gogo)) require.NoError(t, err) - fmt.Printf("legacy: %s\n", string(legacyBytes)) - fmt.Printf(" sut: %s\n", string(dynamicBytes)) + t.Logf("legacy: %s\n", string(legacyBytes)) + t.Logf(" sut: %s\n", string(dynamicBytes)) if tc.fails { require.NotEqual(t, string(legacyBytes), string(dynamicBytes)) return diff --git a/tests/integration/tx/context_test.go b/tests/integration/tx/context_test.go index 4dd7d77e9e45..08124b79e3e6 100644 --- a/tests/integration/tx/context_test.go +++ b/tests/integration/tx/context_test.go @@ -6,6 +6,7 @@ import ( "cosmossdk.io/depinject" "cosmossdk.io/log" _ "cosmossdk.io/x/accounts" + "cosmossdk.io/x/tx/signing" codectypes "github.com/cosmos/cosmos-sdk/codec/types" "github.com/cosmos/cosmos-sdk/tests/integration/tx/internal" "github.com/cosmos/cosmos-sdk/tests/integration/tx/internal/pulsar/testpb" @@ -14,6 +15,10 @@ import ( "github.com/stretchr/testify/require" ) +func ProvideCustomGetSigner() signing.CustomGetSigner { + return internal.TestRepeatedFieldsSigner +} + func TestDefineCustomGetSigners(t *testing.T) { var interfaceRegistry codectypes.InterfaceRegistry _, err := simtestutil.SetupAtGenesis( @@ -26,7 +31,7 @@ func TestDefineCustomGetSigners(t *testing.T) { configurator.ConsensusModule(), ), depinject.Supply(log.NewNopLogger()), - depinject.Provide(internal.ProvideCustomGetSigner), + depinject.Provide(ProvideCustomGetSigner), ), &interfaceRegistry, ) diff --git a/tests/integration/tx/internal/util.go b/tests/integration/tx/internal/util.go index a10159f648c4..e3f6a5828997 100644 --- a/tests/integration/tx/internal/util.go +++ b/tests/integration/tx/internal/util.go @@ -29,16 +29,14 @@ import ( gogoproto "github.com/cosmos/gogoproto/proto" ) -func ProvideCustomGetSigner() signing.CustomGetSigner { - return signing.CustomGetSigner{ - MsgType: proto.MessageName(&testpb.TestRepeatedFields{}), - Fn: func(msg proto.Message) ([][]byte, error) { - testMsg := msg.(*testpb.TestRepeatedFields) - // arbitrary logic - signer := testMsg.NullableDontOmitempty[1].Value - return [][]byte{[]byte(signer)}, nil - }, - } +var TestRepeatedFieldsSigner = signing.CustomGetSigner{ + MsgType: proto.MessageName(&testpb.TestRepeatedFields{}), + Fn: func(msg proto.Message) ([][]byte, error) { + testMsg := msg.(*testpb.TestRepeatedFields) + // arbitrary logic + signer := testMsg.NullableDontOmitempty[1].Value + return [][]byte{[]byte(signer)}, nil + }, } type noOpAddressCodec struct{} @@ -71,7 +69,7 @@ func NewSigningFixture( t.Helper() // set up transaction and signing infra addressCodec, valAddressCodec := noOpAddressCodec{}, noOpAddressCodec{} - customGetSigners := []signing.CustomGetSigner{ProvideCustomGetSigner()} + customGetSigners := []signing.CustomGetSigner{TestRepeatedFieldsSigner} interfaceRegistry, _, err := codec.ProvideInterfaceRegistry( addressCodec, valAddressCodec, diff --git a/x/auth/migrations/legacytx/stdtx_test.go b/x/auth/migrations/legacytx/stdtx_test.go index 83909d45d6ec..27156448d9ba 100644 --- a/x/auth/migrations/legacytx/stdtx_test.go +++ b/x/auth/migrations/legacytx/stdtx_test.go @@ -44,7 +44,7 @@ func TestStdSignBytes(t *testing.T) { Amount: []*basev1beta1.Coin{{Denom: "atom", Amount: "150"}}, GasLimit: 100000, } - msgStr := fmt.Sprintf(`{"type":"testpb/TestMsg","value":{"decField":"0","signers":["%s"]}}`, addr) + msgStr := fmt.Sprintf(`{"type":"testpb/TestMsg","value":{"decField":"0.000000000000000000","signers":["%s"]}}`, addr) tests := []struct { name string args args From 3e417a99dc46b0814934509573135db7ccaa959f Mon Sep 17 00:00:00 2001 From: Matt Kocubinski Date: Fri, 20 Sep 2024 10:03:42 -0500 Subject: [PATCH 09/10] rm TODO --- tests/integration/tx/aminojson/aminojson_test.go | 1 + x/tx/signing/aminojson/encoder.go | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/integration/tx/aminojson/aminojson_test.go b/tests/integration/tx/aminojson/aminojson_test.go index fbaea363a1d3..cf9695b4b451 100644 --- a/tests/integration/tx/aminojson/aminojson_test.go +++ b/tests/integration/tx/aminojson/aminojson_test.go @@ -283,6 +283,7 @@ func TestAminoJSON_LegacyParity(t *testing.T) { MaxRate: math.LegacyZeroDec(), MaxChangeRate: math.LegacyZeroDec(), }, + MinSelfDelegation: math.NewIntFromUint64(10), }, }, "staking/msg_cancel_unbonding_delegation_response": { diff --git a/x/tx/signing/aminojson/encoder.go b/x/tx/signing/aminojson/encoder.go index ab66d9b03da7..d3e1e75c8bba 100644 --- a/x/tx/signing/aminojson/encoder.go +++ b/x/tx/signing/aminojson/encoder.go @@ -27,8 +27,6 @@ func cosmosIntEncoder(_ *Encoder, v protoreflect.Value, w io.Writer) error { if val == "" { return jsonMarshal(w, "0") } - // TODO - // I think this needs a similar path as cosmosDecEncoder return jsonMarshal(w, val) case []byte: if len(val) == 0 { From 0cc6478ab73c015e98b8565a56d0c4315daf6f6e Mon Sep 17 00:00:00 2001 From: Matt Kocubinski Date: Tue, 24 Sep 2024 12:24:56 -0500 Subject: [PATCH 10/10] CHANGELOG --- x/tx/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x/tx/CHANGELOG.md b/x/tx/CHANGELOG.md index 9dac9e325c67..c987e59f2e3e 100644 --- a/x/tx/CHANGELOG.md +++ b/x/tx/CHANGELOG.md @@ -33,6 +33,8 @@ Since v0.13.0, x/tx follows Cosmos SDK semver: https://github.com/cosmos/cosmos- ## [Unreleased] +* [#21825](https://github.com/cosmos/cosmos-sdk/pull/21825) Fix decimal encoding and field ordering in Amino JSON encoder. + ## [v0.13.5](https://github.com/cosmos/cosmos-sdk/releases/tag/x/tx/v0.13.5) - 2024-09-18 ### Improvements