fix(x/tx): sort with oneof field name in amino-json #21782
Conversation
📝 WalkthroughWalkthroughThe recent update introduces a fix for the JSON attribute sorting mechanism specifically for messages that utilize Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
maharifu
requested review from
kocubinski,
testinginprod,
aaronc and
a team
as code owners
|
@maharifu Thanks for your PR. I am trying to reproduce a failing test case on |
Thank you @kocubinski. I ran into this issue by running a StakeAuthorization transaction with a deny-list. This was the resulting JSON:
Does this help? |
After some testing this message seems OK, but I see other ones failing. Looking deeper into this I think our amino JSON test suite needs a rewrite after recent refactoring as we should have caught this. In the meantime, can I ask you to apply this patch to your PR and see if the (now failing) tests pass? c2f94a3 I have the following behavior on my branch: |
@kocubinski The tests would still fail because of some custom encoders not having the expected behaviour. I pushed your commit, as well as this one and now all tests pass. Please double-check it. |
The Marshal operation in amino-json needs to take the oneof field name into consideration when ordering the fields, otherwise the resulting JSON will not be properly ordered.
Custom encoders module_account and threshold_string need to output fields in lexicographical order.
maharifu
force-pushed
the
fix/amino_json_field_order
branch
from
dc99ced
to
b16697e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Commits
Files that changed from the base of the PR and between e17c406ef9232be3102ec609e785076de48013ab and b16697e.
Files selected for processing (4)
- tests/integration/tx/aminojson/aminojson_test.go (6 hunks)
- x/tx/CHANGELOG.md (1 hunks)
- x/tx/signing/aminojson/encoder.go (2 hunks)
- x/tx/signing/aminojson/json_marshal.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/tx/CHANGELOG.md
- x/tx/signing/aminojson/json_marshal.go
Additional context used
Path-based instructions (2)
x/tx/signing/aminojson/encoder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/integration/tx/aminojson/aminojson_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (5)
x/tx/signing/aminojson/encoder.go (2)
129-133: LGTM!The reordering of fields in the
moduleAccountPrettystruct does not introduce any functional changes. While it may affect the order of fields in the serialized JSON, this change aligns with the rest of the codebase and does not violate any style guidelines.
173-197: Looks good!The modifications to the
thresholdStringEncoderfunction improve the JSON encoding process by:
- Adjusting the output format to write the
pubkeysarray before thethresholdvalue.- Streamlining the handling of empty
pk.PublicKeys.- Correctly appending the
thresholdvalue after thepubkeysarray.These changes enhance the clarity and correctness of the generated JSON while maintaining a valid structure. The code adheres to the Uber Go Style Guide.
tests/integration/tx/aminojson/aminojson_test.go (3)
Line range hint
1-217: LGTM!The
TestAminoJSON_Equivalencefunction is a comprehensive test that ensures the equivalence of the new x/tx/Encoder encoding and the legacy Encoder encoding. The workflow for testing legacy gogo types is well-defined and handles the necessary conversions.
218-218: LGTM!The change to the
ajvariable declaration is correct and removes theDoNotSortFieldsoption, which aligns with the goal of producing a standardized JSON output.
397-401: LGTM!The changes to the
StakeAuthorizationtest case are correct and ensure consistency between the gogo and pulsar implementations by including theMaxTokensandAuthorizationTypefields.Also applies to: 404-408
| gogoBytes, err = sortJson(gogoBytes) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
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
|
|
||
| // 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 | ||
| } |
There was a problem hiding this comment.
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
}
|
@maharifu thanks again for your PR! diff --git a/tests/integration/rapidgen/rapidgen.go b/tests/integration/rapidgen/rapidgen.go
index 0060ca1976..2d2f5ab486 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 cf9695b4b4..e7d5e5b172 100644
--- a/tests/integration/tx/aminojson/aminojson_test.go
+++ b/tests/integration/tx/aminojson/aminojson_test.go
@@ -311,9 +311,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{}}, |
|
@kocubinski I resolved the conflicts and added your patch. Let me know if something is still missing. |
julienrbrt
changed the title
julienrbrt
added
the
backport/v0.50.x
|
Could you merge main? |
Co-authored-by: Matt Kocubinski <[email protected]> (cherry picked from commit 60cbe2d) # Conflicts: # tests/integration/tx/aminojson/aminojson_test.go # x/tx/CHANGELOG.md
…#22228) Co-authored-by: Luis Carvalho <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
The Marshal operation in amino-json needs to take the oneof field name into consideration when ordering the fields, otherwise the resulting JSON will not be properly ordered.
This PR adds checks to the oneof field name when ordering fields.
This issue prevents ledger devices from signing
authz grant delegatetransactions with adeny-list, since theValidatorsfield was not properly ordered.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Bug Fixes
oneoffields, ensuring consistent and predictable output.Improvements
oneoffields, ensuring accurate field names and structure in the output.