feat: x/tx/signing/aminojson: Marshal sort fields (#16254)

tests/integration/aminojson/aminojson_test.go

@@ -99,7 +99,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{})
+	aj := aminojson.NewEncoder(aminojson.EncoderOptions{DoNotSortFields: true})
 
 	for _, tt := range rapidgen.DefaultGeneratedTypes {
 		desc := tt.Pulsar.ProtoReflect().Descriptor()
@@ -209,7 +209,7 @@ func TestAminoJSON_LegacyParity(t *testing.T) {
 		vesting.AppModuleBasic{}, gov.AppModuleBasic{})
 	legacytx.RegressionTestingAminoCodec = encCfg.Amino
 
-	aj := aminojson.NewEncoder(aminojson.EncoderOptions{})
+	aj := aminojson.NewEncoder(aminojson.EncoderOptions{DoNotSortFields: true})
 	addr1 := types.AccAddress("addr1")
 	now := time.Now()
 

tests/integration/aminojson/repeated_test.go

@@ -17,7 +17,7 @@ import (
 
 func TestRepeatedFields(t *testing.T) {
 	cdc := codec.NewLegacyAmino()
-	aj := aminojson.NewEncoder(aminojson.EncoderOptions{})
+	aj := aminojson.NewEncoder(aminojson.EncoderOptions{DoNotSortFields: true})
 
 	cases := map[string]struct {
 		gogo    gogoproto.Message

x/tx/CHANGELOG.md

@@ -37,6 +37,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 * [#16044](https://github.com/cosmos/cosmos-sdk/pull/16044): rename aminojson.NewAminoJSON -> aminojson.NewEncoder
 * [#16047](https://github.com/cosmos/cosmos-sdk/pull/16047): aminojson.NewEncoder now takes EncoderOptions as an argument.
+* [#16254](https://github.com/cosmos/cosmos-sdk/pull/16254): aminojson.Encoder.Marshal now sorts all fields like encoding/json.Marshal does, hence no more need for sdk.\*SortJSON.
 
 ## v0.6.2
 

x/tx/signing/aminojson/bench_test.go

@@ -0,0 +1,66 @@
+package aminojson_test
+
+import (
+	"testing"
+
+	"cosmossdk.io/x/tx/signing/aminojson"
+	"cosmossdk.io/x/tx/signing/aminojson/internal/testpb"
+)
+
+var sink any
+
+var msg = &testpb.ABitOfEverything{
+	Message: &testpb.NestedMessage{
+		Foo: "test",
+		Bar: 0, // this is the default value and should be omitted from output
+	},
+	Enum:     testpb.AnEnum_ONE,
+	Repeated: []int32{3, -7, 2, 6, 4},
+	Str:      `abcxyz"foo"def`,
+	Bool:     true,
+	Bytes:    []byte{0, 1, 2, 3},
+	I32:      -15,
+	F32:      1001,
+	U32:      1200,
+	Si32:     -376,
+	Sf32:     -1000,
+	I64:      14578294827584932,
+	F64:      9572348124213523654,
+	U64:      4759492485,
+	Si64:     -59268425823934,
+	Sf64:     -659101379604211154,
+}
+
+func BenchmarkAminoJSONNaiveSort(b *testing.B) {
+	benchmarkAminoJSON(b, true)
+}
+
+func BenchmarkAminoJSONDefaultSort(b *testing.B) {
+	benchmarkAminoJSON(b, false)
+}
+
+func benchmarkAminoJSON(b *testing.B, addNaiveSort bool) {
+	enc := aminojson.NewEncoder(aminojson.EncoderOptions{DoNotSortFields: addNaiveSort})
+	b.ReportAllocs()
+	b.ResetTimer()
+
+	for i := 0; i < b.N; i++ {
+		sink = runAminoJSON(b, enc, addNaiveSort)
+	}
+	if sink == nil {
+		b.Fatal("Benchmark was not run")
+	}
+	sink = nil
+}
+
+func runAminoJSON(b *testing.B, enc aminojson.Encoder, addNaiveSort bool) []byte {
+	bz, err := enc.Marshal(msg)
+	if err != nil {
+		b.Fatal(err)
+	}
+
+	if addNaiveSort {
+		return naiveSortedJSON(b, bz)
+	}
+	return bz
+}

x/tx/signing/aminojson/json_marshal.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
+	"sort"
 
 	"github.com/pkg/errors"
 	"google.golang.org/protobuf/proto"
@@ -22,6 +23,8 @@ type FieldEncoder func(*Encoder, protoreflect.Value, io.Writer) error
 
 // EncoderOptions are options for creating a new Encoder.
 type EncoderOptions struct {
+	// DonotSortFields when set turns off sorting of field names.
+	DoNotSortFields bool
 	// TypeResolver is used to resolve protobuf message types by TypeURL when marshaling any packed messages.
 	TypeResolver protoregistry.MessageTypeResolver
 	// FileResolver is used to resolve protobuf file descriptors TypeURL when TypeResolver fails.
@@ -36,6 +39,7 @@ type Encoder struct {
 	fieldEncoders   map[string]FieldEncoder
 	fileResolver    signing.ProtoFileResolver
 	typeResolver    protoregistry.MessageTypeResolver
+	doNotSortFields bool
 }
 
 // NewEncoder returns a new Encoder capable of serializing protobuf messages to JSON using the Amino JSON encoding
@@ -61,8 +65,9 @@ func NewEncoder(options EncoderOptions) Encoder {
 			"legacy_coins":     nullSliceAsEmptyEncoder,
 			"cosmos_dec_bytes": cosmosDecEncoder,
 		},
-		fileResolver: options.FileResolver,
-		typeResolver: options.TypeResolver,
+		fileResolver:    options.FileResolver,
+		typeResolver:    options.TypeResolver,
+		doNotSortFields: options.DoNotSortFields,
 	}
 	return enc
 }
@@ -164,6 +169,11 @@ func (enc Encoder) marshal(value protoreflect.Value, writer io.Writer) error {
 	}
 }
 
+type nameAndIndex struct {
+	i    int
+	name string
+}
+
 func (enc Encoder) marshalMessage(msg protoreflect.Message, writer io.Writer) error {
 	if msg == nil {
 		return errors.New("nil message")
@@ -192,10 +202,27 @@ func (enc Encoder) marshalMessage(msg protoreflect.Message, writer io.Writer) er
 	fields := msg.Descriptor().Fields()
 	first := true
 	emptyOneOfWritten := map[string]bool{}
+
+	// 1. If permitted, ensure the names are sorted.
+	indices := make([]*nameAndIndex, 0, fields.Len())
 	for i := 0; i < fields.Len(); i++ {
 		f := fields.Get(i)
-		v := msg.Get(f)
 		name := getAminoFieldName(f)
+		indices = append(indices, &nameAndIndex{i: i, name: name})
+	}
+
+	if shouldSortFields := !enc.doNotSortFields; shouldSortFields {
+		sort.Slice(indices, func(i, j int) bool {
+			ni, nj := indices[i], indices[j]
+			return ni.name < nj.name
+		})
+	}
+
+	for _, ni := range indices {
+		i := ni.i
+		name := ni.name
+		f := fields.Get(i)
+		v := msg.Get(f)
 		oneof := f.ContainingOneof()
 		isOneOf := oneof != nil
 		oneofFieldName, oneofTypeName, err := getOneOfNames(f)

x/tx/signing/aminojson/json_marshal_test.go

@@ -1,6 +1,7 @@
 package aminojson_test
 
 import (
+	"encoding/json"
 	"fmt"
 	"reflect"
 	"testing"
@@ -96,18 +97,42 @@ func TestAminoJSON(t *testing.T) {
 		Si64:     -59268425823934,
 		Sf64:     -659101379604211154,
 	}
-	bz, err := aminojson.NewEncoder(aminojson.EncoderOptions{}).Marshal(msg)
+
+	unsortedBz, err := aminojson.NewEncoder(aminojson.EncoderOptions{DoNotSortFields: true}).Marshal(msg)
 	assert.NilError(t, err)
 	legacyBz, err := cdc.MarshalJSON(msg)
 	assert.NilError(t, err)
-	require.Equal(t, string(legacyBz), string(bz))
+	require.Equal(t, string(legacyBz), string(unsortedBz))
+
+	// Now ensure that the default encoder behavior sorts fields and that they match
+	// as we'd have them from encoding/json.Marshal.
+	// Please see https://github.com/cosmos/cosmos-sdk/issues/2350
+	encodedDefaultBz, err := aminojson.NewEncoder(aminojson.EncoderOptions{}).Marshal(msg)
+	assert.NilError(t, err)
+
+	// Ensure that it is NOT equal to the legacy JSON but that it is equal to the sorted JSON.
+	require.NotEqual(t, string(legacyBz), string(encodedDefaultBz))
+
+	// Now ensure that the legacy's sortedJSON is as the aminojson.Encoder would produce.
+	// This proves that we can eliminate the use of sdk.*SortJSON(encoderBz)
+	sortedBz := naiveSortedJSON(t, unsortedBz)
+	require.Equal(t, string(sortedBz), string(encodedDefaultBz))
+}
+
+func naiveSortedJSON(t testing.TB, jsonToSort []byte) []byte {
+	var c interface{}
+	err := json.Unmarshal(jsonToSort, &c)
+	assert.NilError(t, err)
+	sortedBz, err := json.Marshal(c)
+	assert.NilError(t, err)
+	return sortedBz
 }
 
 func TestRapid(t *testing.T) {
 	gen := rapidproto.MessageGenerator(&testpb.ABitOfEverything{}, rapidproto.GeneratorOptions{})
 	rapid.Check(t, func(t *rapid.T) {
 		msg := gen.Draw(t, "msg")
-		bz, err := aminojson.NewEncoder(aminojson.EncoderOptions{}).Marshal(msg)
+		bz, err := aminojson.NewEncoder(aminojson.EncoderOptions{DoNotSortFields: true}).Marshal(msg)
 		assert.NilError(t, err)
 		checkInvariants(t, msg, bz)
 	})