fix(x/tx): fix amino json drift from legacy spec (backport #21825) (#…

…22088)

go.mod

@@ -11,7 +11,7 @@ require (
 	cosmossdk.io/log v1.4.1
 	cosmossdk.io/math v1.4.0
 	cosmossdk.io/store v1.1.1
-	cosmossdk.io/x/tx v0.13.5
+	cosmossdk.io/x/tx v0.13.6-0.20241003112805-ff8789a02871
 	github.com/99designs/keyring v1.2.1
 	github.com/bgentry/speakeasy v0.1.1-0.20220910012023-760eaf8b6816
 	github.com/bits-and-blooms/bitset v1.8.0

go.sum

@@ -16,8 +16,8 @@ cosmossdk.io/math v1.4.0 h1:XbgExXFnXmF/CccPPEto40gOO7FpWu9yWNAZPN3nkNQ=
 cosmossdk.io/math v1.4.0/go.mod h1:O5PkD4apz2jZs4zqFdTr16e1dcaQCc5z6lkEnrrppuk=
 cosmossdk.io/store v1.1.1 h1:NA3PioJtWDVU7cHHeyvdva5J/ggyLDkyH0hGHl2804Y=
 cosmossdk.io/store v1.1.1/go.mod h1:8DwVTz83/2PSI366FERGbWSH7hL6sB7HbYp8bqksNwM=
-cosmossdk.io/x/tx v0.13.5 h1:FdnU+MdmFWn1pTsbfU0OCf2u6mJ8cqc1H4OMG418MLw=
-cosmossdk.io/x/tx v0.13.5/go.mod h1:V6DImnwJMTq5qFjeGWpXNiT/fjgE4HtmclRmTqRVM3w=
+cosmossdk.io/x/tx v0.13.6-0.20241003112805-ff8789a02871 h1:+lRwWQRVvB3jgRgdqrgeFUJ45BoXZh/UeeAV5f/m2Gk=
+cosmossdk.io/x/tx v0.13.6-0.20241003112805-ff8789a02871/go.mod h1:V6DImnwJMTq5qFjeGWpXNiT/fjgE4HtmclRmTqRVM3w=
 dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU=
 filippo.io/edwards25519 v1.0.0 h1:0wAIcmJUqRdI8IJ/3eGi5/HwXZWPujYXXlkrQogz0Ek=
 filippo.io/edwards25519 v1.0.0/go.mod h1:N1IkdkCkiLB6tki+MYJoSx2JTY9NUlxZE7eHn5EwJns=

simapp/go.mod

@@ -16,7 +16,7 @@ require (
 	cosmossdk.io/x/evidence v0.1.1
 	cosmossdk.io/x/feegrant v0.1.1
 	cosmossdk.io/x/nft v0.1.1
-	cosmossdk.io/x/tx v0.13.5
+	cosmossdk.io/x/tx v0.13.6-0.20241003112805-ff8789a02871
 	cosmossdk.io/x/upgrade v0.1.4
 	github.com/cometbft/cometbft v0.38.12
 	github.com/cosmos/cosmos-db v1.1.0

simapp/go.sum

@@ -210,8 +210,8 @@ cosmossdk.io/x/feegrant v0.1.1 h1:EKFWOeo/pup0yF0svDisWWKAA9Zags6Zd0P3nRvVvw8=
 cosmossdk.io/x/feegrant v0.1.1/go.mod h1:2GjVVxX6G2fta8LWj7pC/ytHjryA6MHAJroBWHFNiEQ=
 cosmossdk.io/x/nft v0.1.1 h1:pslAVS8P5NkW080+LWOamInjDcq+v2GSCo+BjN9sxZ8=
 cosmossdk.io/x/nft v0.1.1/go.mod h1:Kac6F6y2gsKvoxU+fy8uvxRTi4BIhLOor2zgCNQwVgY=
-cosmossdk.io/x/tx v0.13.5 h1:FdnU+MdmFWn1pTsbfU0OCf2u6mJ8cqc1H4OMG418MLw=
-cosmossdk.io/x/tx v0.13.5/go.mod h1:V6DImnwJMTq5qFjeGWpXNiT/fjgE4HtmclRmTqRVM3w=
+cosmossdk.io/x/tx v0.13.6-0.20241003112805-ff8789a02871 h1:+lRwWQRVvB3jgRgdqrgeFUJ45BoXZh/UeeAV5f/m2Gk=
+cosmossdk.io/x/tx v0.13.6-0.20241003112805-ff8789a02871/go.mod h1:V6DImnwJMTq5qFjeGWpXNiT/fjgE4HtmclRmTqRVM3w=
 cosmossdk.io/x/upgrade v0.1.4 h1:/BWJim24QHoXde8Bc64/2BSEB6W4eTydq0X/2f8+g38=
 cosmossdk.io/x/upgrade v0.1.4/go.mod h1:9v0Aj+fs97O+Ztw+tG3/tp5JSlrmT7IcFhAebQHmOPo=
 dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU=

tests/go.mod

@@ -14,7 +14,7 @@ require (
 	cosmossdk.io/x/evidence v0.1.1
 	cosmossdk.io/x/feegrant v0.1.1
 	cosmossdk.io/x/nft v0.1.1 // indirect
-	cosmossdk.io/x/tx v0.13.5
+	cosmossdk.io/x/tx v0.13.6-0.20241003112805-ff8789a02871
 	cosmossdk.io/x/upgrade v0.1.4
 	github.com/cometbft/cometbft v0.38.12
 	github.com/cosmos/cosmos-db v1.1.0
@@ -202,6 +202,7 @@ require (
 // replace (
 // 	<temporary replace>
 // )
+replace cosmossdk.io/x/tx => ../x/tx
 
 // Below are the long-lived replace for tests.
 replace (

tests/go.sum

@@ -208,8 +208,6 @@ cosmossdk.io/x/feegrant v0.1.1 h1:EKFWOeo/pup0yF0svDisWWKAA9Zags6Zd0P3nRvVvw8=
 cosmossdk.io/x/feegrant v0.1.1/go.mod h1:2GjVVxX6G2fta8LWj7pC/ytHjryA6MHAJroBWHFNiEQ=
 cosmossdk.io/x/nft v0.1.1 h1:pslAVS8P5NkW080+LWOamInjDcq+v2GSCo+BjN9sxZ8=
 cosmossdk.io/x/nft v0.1.1/go.mod h1:Kac6F6y2gsKvoxU+fy8uvxRTi4BIhLOor2zgCNQwVgY=
-cosmossdk.io/x/tx v0.13.5 h1:FdnU+MdmFWn1pTsbfU0OCf2u6mJ8cqc1H4OMG418MLw=
-cosmossdk.io/x/tx v0.13.5/go.mod h1:V6DImnwJMTq5qFjeGWpXNiT/fjgE4HtmclRmTqRVM3w=
 cosmossdk.io/x/upgrade v0.1.4 h1:/BWJim24QHoXde8Bc64/2BSEB6W4eTydq0X/2f8+g38=
 cosmossdk.io/x/upgrade v0.1.4/go.mod h1:9v0Aj+fs97O+Ztw+tG3/tp5JSlrmT7IcFhAebQHmOPo=
 dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU=

tests/integration/rapidgen/rapidgen.go

@@ -231,7 +231,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),

tests/integration/tx/aminojson/aminojson_test.go

@@ -2,6 +2,7 @@ package aminojson
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"reflect"
 	"testing"
@@ -97,7 +98,7 @@ func TestAminoJSON_Equivalence(t *testing.T) {
 		gov.AppModuleBasic{}, groupmodule.AppModuleBasic{}, mint.AppModuleBasic{}, params.AppModuleBasic{},
 		slashing.AppModuleBasic{}, staking.AppModuleBasic{}, upgrade.AppModuleBasic{}, vesting.AppModuleBasic{})
 	legacytx.RegressionTestingAminoCodec = encCfg.Amino
-	aj := aminojson.NewEncoder(aminojson.EncoderOptions{DoNotSortFields: true})
+	aj := aminojson.NewEncoder(aminojson.EncoderOptions{})
 
 	for _, tt := range rapidgen.DefaultGeneratedTypes {
 		desc := tt.Pulsar.ProtoReflect().Descriptor()
@@ -131,6 +132,7 @@ func TestAminoJSON_Equivalence(t *testing.T) {
 
 				legacyAminoJSON, err := encCfg.Amino.MarshalJSON(gogo)
 				require.NoError(t, err)
+				legacyAminoJSON = sortJSON(t, legacyAminoJSON)
 				aminoJSON, err := aj.Marshal(msg)
 				require.NoError(t, err)
 				require.Equal(t, string(legacyAminoJSON), string(aminoJSON))
@@ -219,19 +221,21 @@ func TestAminoJSON_LegacyParity(t *testing.T) {
 		// 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
+		// sort JSON bytes before comparison.  for certain types (like ModuleAccount) x/tx is not able to provide an
+		// unsorted version.  note that the legacy amino signer always sorted JSON bytes by round tripping them to/from
+		// JSON before signing over them.
+		sortJSON bool
 	}{
 		"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),
 			},
 			pulsar: &authapi.ModuleAccount{
-				BaseAccount: &authapi.BaseAccount{Address: addr1.String()}, Permissions: []string{},
+				BaseAccount: &authapi.BaseAccount{Address: addr1.String()},
 			},
 			roundTripUnequal: true,
+			sortJSON:         true,
 		},
 		"auth/base_account": {
 			gogo:   &authtypes.BaseAccount{Address: addr1.String(), PubKey: pubkeyAny},
@@ -258,9 +262,8 @@ func TestAminoJSON_LegacyParity(t *testing.T) {
 			pulsar: &distapi.DelegatorStartingInfo{},
 		},
 		"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)},
+			pulsar: &distapi.DelegatorStartingInfo{Stake: string(dec10bz)},
 		},
 		"distribution/delegation_delegator_reward": {
 			gogo:   &disttypes.DelegationDelegatorReward{},
@@ -282,13 +285,17 @@ func TestAminoJSON_LegacyParity(t *testing.T) {
 			gogo:   &secp256k1types.PubKey{Key: []byte("key")},
 			pulsar: &secp256k1.PubKey{Key: []byte("key")},
 		},
-		"crypto/legacy_amino_pubkey": {
-			gogo:   &multisig.LegacyAminoPubKey{PubKeys: []*codectypes.Any{pubkeyAny}},
-			pulsar: &multisigapi.LegacyAminoPubKey{PublicKeys: []*anypb.Any{pubkeyAnyPulsar}},
+		"crypto/legacy_amino_pubkey/filled": {
+			gogo:             &multisig.LegacyAminoPubKey{PubKeys: []*codectypes.Any{pubkeyAny}},
+			pulsar:           &multisigapi.LegacyAminoPubKey{PublicKeys: []*anypb.Any{pubkeyAnyPulsar}},
+			sortJSON:         true,
+			roundTripUnequal: true,
 		},
 		"crypto/legacy_amino_pubkey/empty": {
-			gogo:   &multisig.LegacyAminoPubKey{},
-			pulsar: &multisigapi.LegacyAminoPubKey{},
+			gogo:             &multisig.LegacyAminoPubKey{},
+			pulsar:           &multisigapi.LegacyAminoPubKey{},
+			sortJSON:         true,
+			roundTripUnequal: true,
 		},
 		"consensus/evidence_params/duration": {
 			gogo:   &gov_v1beta1_types.VotingParams{VotingPeriod: 1e9 + 7},
@@ -339,6 +346,14 @@ func TestAminoJSON_LegacyParity(t *testing.T) {
 		// 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()
+		"gov/v1_params": {
+			gogo: &gov_v1_types.Params{
+				Quorum: math.LegacyMustNewDecFromStr("0.33").String(),
+			},
+			pulsar: &gov_v1_api.Params{
+				Quorum: math.LegacyMustNewDecFromStr("0.33").String(),
+			},
+		},
 		"slashing/params/dec": {
 			gogo: &slashingtypes.Params{
 				DowntimeJailDuration: 1e9 + 7,
@@ -407,6 +422,9 @@ func TestAminoJSON_LegacyParity(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			gogoBytes, err := encCfg.Amino.MarshalJSON(tc.gogo)
 			require.NoError(t, err)
+			if tc.sortJSON {
+				gogoBytes = sortJSON(t, gogoBytes)
+			}
 
 			pulsarBytes, err := aj.Marshal(tc.pulsar)
 			if tc.pulsarMarshalFails {
@@ -426,10 +444,6 @@ func TestAminoJSON_LegacyParity(t *testing.T) {
 			newGogo := reflect.New(gogoType).Interface().(gogoproto.Message)
 
 			err = encCfg.Codec.Unmarshal(pulsarProtoBytes, newGogo)
-			if tc.protoUnmarshalFails {
-				require.Error(t, err)
-				return
-			}
 			require.NoError(t, err)
 
 			newGogoBytes, err := encCfg.Amino.MarshalJSON(newGogo)
@@ -573,3 +587,12 @@ func postFixPulsarMessage(msg proto.Message) {
 		}
 	}
 }
+
+func sortJSON(t require.TestingT, bz []byte) []byte {
+	var c interface{}
+	err := json.Unmarshal(bz, &c)
+	require.NoError(t, err)
+	bz, err = json.Marshal(c)
+	require.NoError(t, err)
+	return bz
+}

x/tx/CHANGELOG.md

@@ -33,8 +33,12 @@ Since v0.13.0, x/tx follows Cosmos SDK semver: https://github.com/cosmos/cosmos-
 
 ## [Unreleased]
 
+## [v0.13.6](https://github.com/cosmos/cosmos-sdk/releases/tag/x/tx/v0.13.6) - 2024-10-XX
+
 ### Bug Fixes
 
+* [#22161](https://github.com/cosmos/cosmos-sdk/pull/22161) Add special case for string represented decimals.
+* [#21825](https://github.com/cosmos/cosmos-sdk/pull/21825) Fix decimal encoding and field ordering in Amino JSON encoder.
 * [#21782](https://github.com/cosmos/cosmos-sdk/pull/21782) Fix JSON attribute sort order on messages with oneof fields.
 
 ## [v0.13.5](https://github.com/cosmos/cosmos-sdk/releases/tag/x/tx/v0.13.5) - 2024-09-18

x/tx/signing/aminojson/encoder.go

@@ -43,15 +43,20 @@ func cosmosIntEncoder(_ *Encoder, v protoreflect.Value, w io.Writer) error {
 	}
 }
 
-// cosmosDecEncoder provides legacy compatible encoding for cosmos.Dec and cosmos.Int types. These are sometimes
+// cosmosDecEncoder provides legacy compatible encoding for cosmos.Dec types. These are sometimes
 // represented as strings in pulsar messages and sometimes as bytes.  This encoder handles both cases.
 func cosmosDecEncoder(_ *Encoder, v protoreflect.Value, w io.Writer) error {
 	switch val := v.Interface().(type) {
 	case string:
 		if val == "" {
 			return jsonMarshal(w, "0")
 		}
-		return jsonMarshal(w, val)
+		var dec math.LegacyDec
+		err := dec.Unmarshal([]byte(val))
+		if err != nil {
+			return fmt.Errorf("failed to unmarshal for Amino JSON encoding; string %q into Dec: %w", val, err)
+		}
+		return jsonMarshal(w, dec.String())
 	case []byte:
 		if len(val) == 0 {
 			return jsonMarshal(w, "0")
@@ -125,27 +130,40 @@ 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,
-	}
-	if ma.BaseAccount != nil {
-		pretty.Address = ma.BaseAccount.Address
-		pretty.AccountNumber = ma.BaseAccount.AccountNumber
-		pretty.Sequence = ma.BaseAccount.Sequence
+		PubKey: "",
+		Name:   msg.Get(fields.ByName("name")).String(),
+	}
+	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 +184,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
 }
 

x/tx/signing/aminojson/json_marshal.go

@@ -16,6 +16,8 @@ import (
 	"cosmossdk.io/x/tx/signing"
 )
 
+const cosmosDecType = "cosmos.Dec"
+
 // MessageEncoder is a function that can encode a protobuf protoreflect.Message to JSON.
 type MessageEncoder func(*Encoder, protoreflect.Message, io.Writer) error
 
@@ -68,8 +70,8 @@ func NewEncoder(options EncoderOptions) Encoder {
 	}
 	enc := Encoder{
 		cosmosProtoScalarEncoders: map[string]FieldEncoder{
-			"cosmos.Dec": cosmosDecEncoder,
-			"cosmos.Int": cosmosIntEncoder,
+			cosmosDecType: cosmosDecEncoder,
+			"cosmos.Int":  cosmosIntEncoder,
 		},
 		aminoMessageEncoders: map[string]MessageEncoder{
 			"key_field":        keyFieldEncoder,
@@ -387,7 +389,7 @@ func (enc Encoder) marshalMessage(msg protoreflect.Message, writer io.Writer) er
 		}
 
 		// encode value
-		if encoder := enc.getFieldEncoding(f); encoder != nil {
+		if encoder := enc.getFieldEncoder(f); encoder != nil {
 			err = encoder(&enc, v, writer)
 			if err != nil {
 				return err