diff --git a/tests/integration/aminojson/aminojson_test.go b/tests/integration/aminojson/aminojson_test.go index fbe2777e9c..22cd7b3b8d 100644 --- a/tests/integration/aminojson/aminojson_test.go +++ b/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() diff --git a/tests/integration/aminojson/repeated_test.go b/tests/integration/aminojson/repeated_test.go index 25a9cced33..63842e58c2 100644 --- a/tests/integration/aminojson/repeated_test.go +++ b/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 diff --git a/x/tx/CHANGELOG.md b/x/tx/CHANGELOG.md index 72ca17b084..9bffe20092 100644 --- a/x/tx/CHANGELOG.md +++ b/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 diff --git a/x/tx/signing/aminojson/bench_test.go b/x/tx/signing/aminojson/bench_test.go new file mode 100644 index 0000000000..9ccc11ddcf --- /dev/null +++ b/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 +} diff --git a/x/tx/signing/aminojson/json_marshal.go b/x/tx/signing/aminojson/json_marshal.go index 642bf78c16..06d8641c59 100644 --- a/x/tx/signing/aminojson/json_marshal.go +++ b/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) diff --git a/x/tx/signing/aminojson/json_marshal_test.go b/x/tx/signing/aminojson/json_marshal_test.go index 9b59c0f966..1d8ec8cf61 100644 --- a/x/tx/signing/aminojson/json_marshal_test.go +++ b/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) })