From 60cbe2db29c4cef12c7a743f3a87060e7f781978 Mon Sep 17 00:00:00 2001 From: Luis Carvalho Date: Fri, 11 Oct 2024 09:17:06 +0100 Subject: [PATCH] fix(x/tx): sort with oneof field name in amino-json (#21782) Co-authored-by: Matt Kocubinski --- tests/integration/rapidgen/rapidgen.go | 4 +- .../tx/aminojson/aminojson_test.go | 21 ++++++-- x/tx/CHANGELOG.md | 1 + x/tx/signing/aminojson/json_marshal.go | 51 +++++++++++++------ 4 files changed, 56 insertions(+), 21 deletions(-) diff --git a/tests/integration/rapidgen/rapidgen.go b/tests/integration/rapidgen/rapidgen.go index 0060ca1976ce..2d2f5ab48630 100644 --- a/tests/integration/rapidgen/rapidgen.go +++ b/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 diff --git a/tests/integration/tx/aminojson/aminojson_test.go b/tests/integration/tx/aminojson/aminojson_test.go index 69ab768ff4c3..c68f24e97865 100644 --- a/tests/integration/tx/aminojson/aminojson_test.go +++ b/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 +} diff --git a/x/tx/CHANGELOG.md b/x/tx/CHANGELOG.md index 9fc07fac5acf..182c6dbfefb5 100644 --- a/x/tx/CHANGELOG.md +++ b/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. diff --git a/x/tx/signing/aminojson/json_marshal.go b/x/tx/signing/aminojson/json_marshal.go index 6629fb350506..1e5d1b59af81 100644 --- a/x/tx/signing/aminojson/json_marshal.go +++ b/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 }