fix(x/tx): sort with oneof field name in amino-json (#21782)

Co-authored-by: Matt Kocubinski <[email protected]>

tests/integration/rapidgen/rapidgen.go

@@ -259,9 +259,7 @@ var (
 
 		GenType(&slashingtypes.Params{}, &slashingapi.Params{}, GenOpts.WithDisallowNil()),
 
-		// 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(&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

tests/integration/tx/aminojson/aminojson_test.go

@@ -2,6 +2,7 @@ package aminojson
 
 import (
 	"bytes"
+	"encoding/json"
 	"fmt"
 	stdmath "math"
 	"testing"
@@ -316,9 +317,6 @@ 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{}},
@@ -456,3 +454,20 @@ 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 in
+// 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
+}

x/tx/CHANGELOG.md

@@ -33,6 +33,7 @@ Since v0.13.0, x/tx follows Cosmos SDK semver: https://github.com/cosmos/cosmos-
 
 ## [Unreleased]
 
+* [#21782](https://github.com/cosmos/cosmos-sdk/pull/21782) Fix JSON attribute sort order on messages with oneof fields.
 * [#21825](https://github.com/cosmos/cosmos-sdk/pull/21825) Fix decimal encoding and field ordering in Amino JSON encoder.
 * [#21850](https://github.com/cosmos/cosmos-sdk/pull/21850) Support bytes field as signer.
 

x/tx/signing/aminojson/json_marshal.go

@@ -270,8 +270,11 @@ func (enc Encoder) marshal(value protoreflect.Value, fd protoreflect.FieldDescri
 }
 
 type nameAndIndex struct {
-	i    int
-	name string
+	i              int
+	name           string
+	oneof          protoreflect.OneofDescriptor
+	oneofFieldName string
+	oneofTypeName  string
 }
 
 func (enc Encoder) marshalMessage(msg protoreflect.Message, writer io.Writer) error {
@@ -302,14 +305,37 @@ func (enc Encoder) marshalMessage(msg protoreflect.Message, writer io.Writer) er
 	indices := make([]*nameAndIndex, 0, fields.Len())
 	for i := 0; i < fields.Len(); i++ {
 		f := fields.Get(i)
-		name := getAminoFieldName(f)
-		indices = append(indices, &nameAndIndex{i: i, name: name})
+		entry := &nameAndIndex{
+			i:     i,
+			name:  getAminoFieldName(f),
+			oneof: f.ContainingOneof(),
+		}
+
+		if entry.oneof != nil {
+			var err error
+			entry.oneofFieldName, entry.oneofTypeName, err = getOneOfNames(f)
+			if err != nil {
+				return err
+			}
+		}
+
+		indices = append(indices, entry)
 	}
 
 	if shouldSortFields := !enc.doNotSortFields; shouldSortFields {
 		sort.Slice(indices, func(i, j int) bool {
 			ni, nj := indices[i], indices[j]
-			return ni.name < nj.name
+			niName, njName := ni.name, nj.name
+
+			if indices[i].oneof != nil {
+				niName = indices[i].oneofFieldName
+			}
+
+			if indices[j].oneof != nil {
+				njName = indices[j].oneofFieldName
+			}
+
+			return niName < njName
 		})
 	}
 
@@ -318,22 +344,17 @@ func (enc Encoder) marshalMessage(msg protoreflect.Message, writer io.Writer) er
 		name := ni.name
 		f := fields.Get(i)
 		v := msg.Get(f)
-		oneof := f.ContainingOneof()
-		isOneOf := oneof != nil
-		oneofFieldName, oneofTypeName, err := getOneOfNames(f)
-		if err != nil && isOneOf {
-			return err
-		}
+		isOneOf := ni.oneof != nil
 		writeNil := false
 
 		if !msg.Has(f) {
 			// msg.WhichOneof(oneof) == nil: no field of the oneof has been set
 			// !emptyOneOfWritten: we haven't written a null for this oneof yet (only write one null per empty oneof)
 			switch {
-			case isOneOf && msg.WhichOneof(oneof) == nil && !emptyOneOfWritten[oneofFieldName]:
-				name = oneofFieldName
+			case isOneOf && msg.WhichOneof(ni.oneof) == nil && !emptyOneOfWritten[ni.oneofFieldName]:
+				name = ni.oneofFieldName
 				writeNil = true
-				emptyOneOfWritten[oneofFieldName] = true
+				emptyOneOfWritten[ni.oneofFieldName] = true
 			case omitEmpty(f):
 				continue
 			case f.Kind() == protoreflect.MessageKind &&
@@ -351,7 +372,7 @@ func (enc Encoder) marshalMessage(msg protoreflect.Message, writer io.Writer) er
 		}
 
 		if isOneOf && !writeNil {
-			_, err = fmt.Fprintf(writer, `"%s":{"type":"%s","value":{`, oneofFieldName, oneofTypeName)
+			_, err = fmt.Fprintf(writer, `"%s":{"type":"%s","value":{`, ni.oneofFieldName, ni.oneofTypeName)
 			if err != nil {
 				return err
 			}