fix(x/auth/tx): JSON TX encoding/decoding #20803
Conversation
|
Warning Rate limit exceeded@kocubinski has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 15 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughWalkthroughThe recent updates introduce key enhancements to the Cosmos SDK, focusing on the addition of new fields and adjustments in protobuf messaging ( Changes
Sequence DiagramsThe provided changes do not necessitate a sequence diagram as they mainly involve struct enhancements, protocol buffer changes, and minor testing tweaks without altering the control flow or introducing significant new features. 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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
kocubinski
changed the title
|
@kocubinski your pull request is missing a changelog! |
kocubinski
changed the title
…o kocu/json-encoding
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
x/auth/tx/decoder.go (1)
Line range hint
18-43: Improve error handling consistency.Ensure that all errors are wrapped consistently using
errorsmod.Wrap.- if err != nil { - return nil, err - } + if err != nil { + return nil, errorsmod.Wrap(sdkerrors.ErrTxDecode, err.Error()) + }
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (2)
testutil/testdata/tx.pb.gois excluded by!**/*.pb.gox/staking/types/query.pb.gois excluded by!**/*.pb.go
Files selected for processing (7)
- api/cosmos/staking/v1beta1/query.pulsar.go (1 hunks)
- testutil/testdata/testpb/tx.proto (2 hunks)
- testutil/testdata/testpb/tx.pulsar.go (15 hunks)
- x/auth/migrations/legacytx/stdtx_test.go (1 hunks)
- x/auth/tx/decoder.go (1 hunks)
- x/auth/tx/encoder.go (2 hunks)
- x/auth/tx/testutil/suite.go (5 hunks)
Additional context used
Path-based instructions (6)
x/auth/tx/encoder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/tx/decoder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/migrations/legacytx/stdtx_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/auth/tx/testutil/suite.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.testutil/testdata/testpb/tx.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.api/cosmos/staking/v1beta1/query.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (13)
testutil/testdata/testpb/tx.proto (1)
40-45: Ensure consistency in field options.The
decFieldhas multiple options applied. Ensure that these options are consistent with the rest of the proto file and do not conflict with each other.Verification successful
Field options are consistent.
The
gogoproto.customtypefield option is consistently used across the proto files without any conflicts.
cosmossdk.io/math.LegacyDeccosmossdk.io/math.IntScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistency of field options in proto files. # Test: Search for other fields with similar options. Expect: No conflicts or inconsistencies. rg --type proto $'gogoproto.customtype'Length of output: 68
Script:
#!/bin/bash # Locate all .proto files and search for the gogoproto.customtype field option within them fd -e proto -x rg 'gogoproto.customtype' {}Length of output: 3247
x/auth/migrations/legacytx/stdtx_test.go (1)
47-47: Ensure test coverage for all scenarios.The string
msgStrincludes a new fielddecField. Ensure that tests cover various values for this field, including edge cases.x/auth/tx/testutil/suite.go (5)
72-73: EnsureDecFieldis correctly set.The
DecFieldis being set tomath.LegacyZeroDec(). Verify that this is the intended value for the tests.
81-82: Ensure all messages are correctly set.The
unbuiltMsgsshould be equal tomsgs. Verify that all messages are correctly set in the transaction builder.
250-251: EnsureDecFieldis correctly set to the value of pi.The
DecFieldis being set tomath.LegacyMustNewDecFromStr(pi). Verify that this is the intended value for the tests.
296-298: Check JSON representation for human-readable decimal value.Ensure that the JSON representation contains the human-readable decimal value of
pi.
315-316: VerifyDecFieldafter JSON decoding.Ensure that the
DecFieldretains the correct value ofpiafter JSON decoding.testutil/testdata/testpb/tx.pulsar.go (5)
8-8: Approved: New import statement forcosmos-proto.The addition of the import statement for
cosmos-protois necessary for the new functionality.
985-987: Approved: New field descriptor forDecField.The addition of the field descriptor
fd_TestMsg_decFieldis necessary for handling the newDecField.
994-994: Approved: Initialization of new field descriptor ininitfunction.The initialization of the new field descriptor
fd_TestMsg_decFieldin theinitfunction is correct.
1068-1073: Approved: Logic for iterating overDecFieldinRangemethod.The logic for iterating over the new
DecFieldin theRangemethod is correct.
1091-1092: Approved: Logic for handlingDecFieldin various methods.The logic for handling the new
DecFieldin theHas,Clear,Get,Set,Mutable,NewField,ProtoMethods, andUnmarshalmethods is correct.Also applies to: 1111-1112, 1135-1137, 1162-1163, 1190-1191, 1208-1209, 1285-1288, 1415-1446
api/cosmos/staking/v1beta1/query.pulsar.go (1)
14424-14428: Ensure fieldValidatorInfois correctly populated.The
ValidatorInfofield is a new addition that provides additional details for each validator. Ensure that this field is correctly populated and maintained in sync with theValidatorsfield in all relevant parts of the codebase.Comments are clear and helpful.
The comments explaining the usage of the
ValidatorInfofield are clear and provide valuable context for developers. This is a good practice and should be continued.Consider adding validation logic.
Consider adding validation logic to ensure the
ValidatorInfolist always corresponds in order and length to theValidatorslist, to prevent potential mismatches.
x/auth/tx/encoder.go
Outdated
| bz, err := proto.Marshal(gogoWrapper.Tx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| v1Tx := &sdktx.Tx{} | ||
| err = gogoproto.Unmarshal(bz, v1Tx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return cdc.MarshalJSON(v1Tx) |
There was a problem hiding this comment.
Consider simplifying the encoding process.
The function uses both proto.Marshal and gogoproto.Unmarshal. This might lead to unnecessary complexity and potential issues. If possible, use a single protobuf library for encoding.
- bz, err := proto.Marshal(gogoWrapper.Tx)
+ bz, err := gogoproto.Marshal(gogoWrapper.Tx)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bz, err := proto.Marshal(gogoWrapper.Tx) | |
| if err != nil { | |
| return nil, err | |
| } | |
| v1Tx := &sdktx.Tx{} | |
| err = gogoproto.Unmarshal(bz, v1Tx) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return cdc.MarshalJSON(v1Tx) | |
| bz, err := gogoproto.Marshal(gogoWrapper.Tx) | |
| if err != nil { | |
| return nil, err | |
| } | |
| v1Tx := &sdktx.Tx{} | |
| err = gogoproto.Unmarshal(bz, v1Tx) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return cdc.MarshalJSON(v1Tx) |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- testutil/testdata/tx.go (2 hunks)
- types/tx_msg_test.go (2 hunks)
- x/auth/tx/testutil/suite.go (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/auth/tx/testutil/suite.go
Additional context used
Path-based instructions (2)
types/tx_msg_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"testutil/testdata/tx.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
golangci-lint
types/tx_msg_test.go
6-6: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
5-5: File is not
gofumpt-ed with-extra(gofumpt)
Additional comments not posted (1)
testutil/testdata/tx.go (1)
85-86: LGTM!The code changes are approved. The initialization of
DecFieldwithmath.LegacyZeroDec()ensures proper handling of the new field.
|
Unfortunately the correct proto JSON rendering for LegacyDec is "3141590000000000000" because the proto JSON spec says it should match the (bad) binary encoding. Yes it is not human readable because of how the binary encoding was setup years ago. Trying to make it human readable with the correct decimal point is simply incorrect because it breaks the proto JSON spec. We may think this is a small issue but whenever you break a spec you're going to be causing some bugs somewhere else in the system. If it is proto JSON and the type is string, the rendering should be the exact same string that is encoded in proto binary. Two alternatives are 1) create another json encoding where we control the rules (we actually already have one called amino) or 2) use a different Dec type where the binary encoding includes a decimal point. We could accomplish 2 without even moving over to a GDA Dec but simply by creating a v2 of this LegacyDec. Trying to make things sort of work by hacking into how gogo proto works is not a good idea IMHO. The fact that gogo supports custom JSON marshaling was never a good idea and created subtle bugs for us years ago. Probably the panic is because of these custom gogo JSON methods. The problem though is that no other protobuf clients will be able to match our encoding if we use gogo to break the spec. Consider what would happen if a user who has a correct protobuf client takes some legacy dec value from a proto binary endpoint and then tries to pass it to a "gogo proto json" endpoints. They will be off by an order of magnitude. Or if someone is using proto v2 as a client in go and uses both proto json and binary (which is the case with autocli). Now I personally think proto json is extremely limiting and we should move on from it. But breaking specs is almost always bad and that's one of the big footguns of gogo - lots of customization introduces lots of ways to mess up. |
Unfortunately the SDK spec has this hack as canonical (SDK correct?) up to v0.50, so changing it now is breaking for any client which reads or writes JSON, including a JSON formatted genesis file. |
kocubinski
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- .vscode/launch.json (1 hunks)
Additional comments not posted (1)
.vscode/launch.json (1)
24-31: LGTM! Configuration forsimapp/v1is correctly added.The new launch configuration for
simapp/v1follows the same structure as the existing configurations and appears to be correct.
|
@Mergifyio backport release/v0.52.x |
✅ Backports have been created
|
(cherry picked from commit d80afaa)
Description
Fixes a regression in JSON transaction encoding/decoding introduced somewhere in recent x/tx refactors.
Previously decimal fields in proto specs were represented in JSON as human readable strings, e.g.
3.14159 -> "3.141590000000000000"On main (today) they are represented by their backing word in the math package, e.g
3.14159 -> "3141590000000000000"The latter is consistent with the at rest state representation, and proto wire format, but different from how JSON RPC represents decimals (the former) in the Amino ProtoJSON spec. See: #10863 (comment). As long as we're committing to AminoJSON at the JSON RPC boundary, we ought to represent JSON Txs with the same spec, Amino JSON.
The added test now fails without the patch to
x/auth/txin this PR like: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
Summary by CodeRabbit
New Features
ValidatorInfofield to provide additional details for each validator.DecFieldinTestMsgfor better message handling.Bug Fixes
Tests
DecFieldfor comprehensive validation.Chores