Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: sort with oneof field name in amino-json #21782

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+84 −31
Open

fix: sort with oneof field name in amino-json #21782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1beb756
fix: sort with oneof field name in amino-json
maharifu Sep 17, 2024
61ed948
chore: add unreleased entry to changelog
maharifu Sep 17, 2024
16dc491
sort JSON after marshalling
kocubinski Sep 17, 2024
b16697e
fix: amino-json custom encoders
maharifu Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion tests/integration/tx/aminojson/aminojson_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package aminojson

import (
"context"
"encoding/json"
"fmt"
"reflect"
"strings"
Expand Down Expand Up @@ -214,7 +215,7 @@ func TestAminoJSON_LegacyParity(t *testing.T) {
vesting.AppModule{}, gov.AppModule{})
legacytx.RegressionTestingAminoCodec = encCfg.Amino

aj := aminojson.NewEncoder(aminojson.EncoderOptions{DoNotSortFields: true})
aj := aminojson.NewEncoder(aminojson.EncoderOptions{})
addr1 := types.AccAddress("addr1")
now := time.Now()

Expand Down Expand Up @@ -393,14 +394,18 @@ func TestAminoJSON_LegacyParity(t *testing.T) {
},
"staking/stake_authorization_allow": {
gogo: &stakingtypes.StakeAuthorization{
MaxTokens: &types.Coin{Denom: "foo", Amount: math.NewInt(123)},
Validators: &stakingtypes.StakeAuthorization_AllowList{
AllowList: &stakingtypes.StakeAuthorization_Validators{Address: []string{"foo"}},
},
AuthorizationType: stakingtypes.AuthorizationType_AUTHORIZATION_TYPE_DELEGATE,
},
pulsar: &stakingapi.StakeAuthorization{
MaxTokens: &v1beta1.Coin{Denom: "foo", Amount: "123"},
Validators: &stakingapi.StakeAuthorization_AllowList{
AllowList: &stakingapi.StakeAuthorization_Validators{Address: []string{"foo"}},
},
AuthorizationType: stakingapi.AuthorizationType_AUTHORIZATION_TYPE_DELEGATE,
},
},
"vesting/base_account_empty": {
Expand Down Expand Up @@ -432,6 +437,8 @@ func TestAminoJSON_LegacyParity(t *testing.T) {
t.Run(name, func(t *testing.T) {
gogoBytes, err := encCfg.Amino.MarshalJSON(tc.gogo)
require.NoError(t, err)
gogoBytes, err = sortJson(gogoBytes)
require.NoError(t, err)
Comment on lines +440 to +441
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a third-party library for sorting JSON.

Instead of using the custom sortJson function, consider using a well-established library like github.com/tidwall/gjson for sorting JSON. This library provides a more robust and efficient way to sort JSON fields.

import "github.com/tidwall/gjson"

// ...

gogoBytes, err = gjson.ParseBytes(gogoBytes).Sort().Bytes()
require.NoError(t, err)

// ...

newGogoBytes, err = gjson.ParseBytes(newGogoBytes).Sort().Bytes()
require.NoError(t, err)

Also applies to: 469-470


pulsarBytes, err := aj.Marshal(tc.pulsar)
if tc.pulsarMarshalFails {
Expand Down Expand Up @@ -459,6 +466,8 @@ func TestAminoJSON_LegacyParity(t *testing.T) {

newGogoBytes, err := encCfg.Amino.MarshalJSON(newGogo)
require.NoError(t, err)
newGogoBytes, err = sortJson(newGogoBytes)
require.NoError(t, err)
if tc.roundTripUnequal {
require.NotEqual(t, string(gogoBytes), string(newGogoBytes))
return
Expand Down Expand Up @@ -598,3 +607,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
}
Comment on lines +610 to +626
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the sortJson function to a separate utility package.

Consider moving the sortJson function to a separate utility package to improve code organization and reusability. This function is a general-purpose utility and can be used in other parts of the codebase.

// utils/json.go
package utils

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
}

2 changes: 2 additions & 0 deletions x/tx/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ 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.

### Improvements

* [#21073](https://github.com/cosmos/cosmos-sdk/pull/21073) In Context use sync.Map `getSignersFuncs` map from concurrent writes, we also need to call Validate when using the legacy app.
Expand Down
34 changes: 19 additions & 15 deletions x/tx/signing/aminojson/encoder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,12 @@ 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
Expand Down Expand Up @@ -170,25 +170,29 @@ func thresholdStringEncoder(enc *Encoder, msg protoreflect.Message, w io.Writer)
if !ok {
return errors.New("thresholdStringEncoder: msg not a multisig.LegacyAminoPubKey")
}
_, err := fmt.Fprintf(w, `{"threshold":"%d","pubkeys":`, pk.Threshold)

_, err := io.WriteString(w, `{"pubkeys":`)
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 = io.WriteString(w, `[]`)
if err != nil {
return err
}
} else {
fields := msg.Descriptor().Fields()
pubkeysField := fields.ByName("public_keys")
pubkeys := msg.Get(pubkeysField).List()

err = enc.marshalList(pubkeys, pubkeysField, w)
if err != nil {
return err
err = enc.marshalList(pubkeys, pubkeysField, w)
if err != nil {
return err
}
}
_, err = io.WriteString(w, `}`)

_, err = fmt.Fprintf(w, `,"threshold":"%d"}`, pk.Threshold)
return err
}

Expand Down
51 changes: 36 additions & 15 deletions x/tx/signing/aminojson/json_marshal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,8 +254,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 {
Expand Down Expand Up @@ -286,14 +289,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
})
}

Expand All @@ -302,22 +328,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 &&
Expand All @@ -335,7 +356,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
}
Expand Down
Loading