fix(x/tx): fix amino json drift from legacy spec #21825
Conversation
WalkthroughWalkthroughThe pull request includes changes across multiple files, focusing on refining type handling, serialization, and testing frameworks within the Cosmos SDK. Key modifications involve removing certain type declarations, enhancing error handling in encoding functions, and introducing a new testing fixture to streamline test setups. Additionally, the changes aim to improve the organization and maintainability of the codebase by modularizing components and optimizing existing logic. Changes
Possibly related PRs
Suggested labels
Recent review detailsConfiguration used: .coderabbit.yml Files selected for processing (2)
Files skipped from review as they are similar to previous changes (2)
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
|
| @@ -51,7 +53,12 @@ func cosmosDecEncoder(_ *Encoder, v protoreflect.Value, w io.Writer) error { | |||
| if val == "" { | |||
| return jsonMarshal(w, "0") | |||
| } | |||
| return jsonMarshal(w, val) | |||
| var dec math.LegacyDec | |||
| err := dec.Unmarshal([]byte(val)) | |||
There was a problem hiding this comment.
Dynamic messages represent the packed Decimal value without a radix, so math marshaling (String() implementation) is needed here.
x/tx/signing/aminojson/encoder.go
Outdated
| @@ -27,6 +27,8 @@ func cosmosIntEncoder(_ *Encoder, v protoreflect.Value, w io.Writer) error { | |||
| if val == "" { | |||
| return jsonMarshal(w, "0") | |||
| } | |||
| // TODO | |||
| // I think this needs a similar path as cosmosDecEncoder | |||
There was a problem hiding this comment.
I'm not sure on this, is the packed encoding equivalent?
kocubinski
requested review from
testinginprod,
aaronc and
a team
as code owners
|
@kocubinski your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (2)
tests/integration/tx/internal/util.go (1)
210-210: Typographical Correction in CommentIn the comment on line 210, there is a missing word. It should read: "It is used here to ensure the x/tx JSON encoding..."
Apply this diff to fix the typo:
// amino JSON encoding x/auth/migrations/legacytx.StdSignBytes. It is used here ensure the x/tx +// amino JSON encoding x/auth/migrations/legacytx.StdSignBytes. It is used here to ensure the x/txtests/integration/tx/aminojson/aminojson_test.go (1)
145-146: Maintain consistent formatting for module listings in fixture initialization.In
TestAminoJSON_LegacyParity, the modules are listed in a compact format, whereas inTestAminoJSON_Equivalence, each module is listed on a separate line. For consistency and improved readability, consider formatting the module list in the same way in both functions.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (5)
- tests/integration/rapidgen/rapidgen.go (1 hunks)
- tests/integration/tx/aminojson/aminojson_test.go (4 hunks)
- tests/integration/tx/context_test.go (2 hunks)
- tests/integration/tx/internal/util.go (1 hunks)
- x/tx/signing/aminojson/encoder.go (4 hunks)
Additional context used
Path-based instructions (5)
tests/integration/rapidgen/rapidgen.go (2)
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"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"tests/integration/tx/context_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"tests/integration/tx/internal/util.go (2)
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"x/tx/signing/aminojson/encoder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (15)
tests/integration/tx/context_test.go (3)
10-10: LGTM!Moving the custom signer logic to a separate internal package is a good refactoring decision. It improves code organization and maintainability.
14-14: LGTM!Importing the
requirepackage fromtestifyis a standard practice for writing assertions in Go tests. It will help in validating the expected behavior of the code under test.
29-29: LGTM!Providing the
internal.ProvideCustomGetSignerfunction to the dependency injection container is the correct way to make the custom signer logic available for the tests after the refactoring. It ensures that the tests can access the custom signer functionality.tests/integration/rapidgen/rapidgen.go (2)
262-264: Skipping review of the comment.The comment provides useful context about a pending fix in a separate pull request. The TODO comment is a good practice to track pending tasks. No changes are required for this comment in this pull request.
262-264: Skipping review of the commented out code.The code segment is intentionally commented out due to a pending fix in a separate pull request. Commenting out the code is a good practice to avoid breaking changes until the fix is merged. No changes are required for this code segment in this pull request.
x/tx/signing/aminojson/encoder.go (9)
56-61: Correctly handle decimal unmarshalling incosmosDecEncoder.The code properly unmarshals the decimal value using
dec.Unmarshal([]byte(val))and marshals it back to a string representation. This ensures that decimal values are accurately processed and enhances the robustness of the encoder.
136-140: Fields added tomoduleAccountPrettyare consistent with JSON output.The inclusion of
AddressandPubKeyfields with appropriate JSON tags ensures that all necessary account information is included in the serialized JSON output. This aligns with the expected structure for module accounts.
146-152: Proper type assertion inmoduleAccountEncoder.The code correctly asserts that the provided
msgis of typeauthapi.ModuleAccountby comparing the full names of the descriptors. This improves type safety and prevents potential runtime errors due to incorrect message types.
154-156: InitializePubKeyand populateNameinmoduleAccountPretty.The
PubKeyfield is initialized as an empty string, and theNamefield is correctly retrieved from the message fields. This ensures that the JSON output includes these fields, even if the public key is not set.
162-168: Extract base account information if available.The code correctly checks for the presence of
base_accountand extractsAddress,AccountNumber, andSequencefields. This enhances flexibility and prevents potential nil pointer dereferences when the base account is not set.
189-191: Initialize variables inthresholdStringEncoder.The necessary variables
pk,msgDesc, andfieldsare properly initialized to process the multisig public key message. This sets the foundation for accurate serialization of multisig keys.
199-201: Write opening of JSON object and handle errors properly.The code correctly writes the beginning of the JSON object with
{"pubkeys":and checks for potential I/O errors. This ensures that any write failures are promptly handled.
203-213: Ensure correct JSON serialization ofpubkeys.The function properly handles the case where
pubkeysis empty by writing an empty array[]. Whenpubkeysare present, it serializes the list usingenc.marshalList, ensuring accurate JSON representation.
215-216: Serializethresholdfield as a string in JSON output.The code appends the
thresholdvalue as a string to the JSON output usingfmt.Fprintf. Serializing numeric values as strings maintains compatibility with legacy amino JSON encoding requirements.tests/integration/tx/aminojson/aminojson_test.go (1)
99-99: Uset.Logfinstead offmt.Printffor test logging.In test code, it's recommended to use
t.Logffor logging instead offmt.Printf. This ensures that log messages are properly associated with the test and can be filtered or formatted by the testing framework.Apply this diff to make the change:
- fmt.Printf("testing %s\n", tt.Pulsar.ProtoReflect().Descriptor().FullName()) + t.Logf("testing %s", tt.Pulsar.ProtoReflect().Descriptor().FullName())Likely invalid or redundant comment.
| testMsg := msg.(*testpb.TestRepeatedFields) | ||
| // arbitrary logic | ||
| signer := testMsg.NullableDontOmitempty[1].Value | ||
| return [][]byte{[]byte(signer)}, nil |
There was a problem hiding this comment.
Prevent Potential Index Out of Range Panic in ProvideCustomGetSigner
In the Fn function (lines 35-40), accessing testMsg.NullableDontOmitempty[1] at line 38 without checking the slice length may cause an index out of range panic if the slice has fewer than two elements. To ensure robustness, add a check to confirm that testMsg.NullableDontOmitempty has at least two elements before accessing.
Apply this diff to add the safety check:
func (msg proto.Message) ([][]byte, error) {
testMsg := msg.(*testpb.TestRepeatedFields)
+ if len(testMsg.NullableDontOmitempty) <= 1 {
+ return nil, fmt.Errorf("NullableDontOmitempty must have at least two elements")
+ }
// arbitrary logic
signer := testMsg.NullableDontOmitempty[1].Value
return [][]byte{[]byte(signer)}, nil
}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.
| testMsg := msg.(*testpb.TestRepeatedFields) | |
| // arbitrary logic | |
| signer := testMsg.NullableDontOmitempty[1].Value | |
| return [][]byte{[]byte(signer)}, nil | |
| testMsg := msg.(*testpb.TestRepeatedFields) | |
| if len(testMsg.NullableDontOmitempty) <= 1 { | |
| return nil, fmt.Errorf("NullableDontOmitempty must have at least two elements") | |
| } | |
| // arbitrary logic | |
| signer := testMsg.NullableDontOmitempty[1].Value | |
| return [][]byte{[]byte(signer)}, nil |
x/tx/signing/aminojson/encoder.go
Outdated
| // TODO | ||
| // I think this needs a similar path as cosmosDecEncoder |
There was a problem hiding this comment.
Address the TODO and implement handling in cosmosIntEncoder.
The cosmosIntEncoder function contains a TODO comment indicating that further implementation is needed. Additionally, there's a note suggesting that it may need a similar approach to cosmosDecEncoder. Please complete the implementation to ensure proper encoding of cosmos.Int types and remove the TODO comment once addressed.
| permissions := msg.Get(fields.ByName("permissions")).List() | ||
| for i := 0; i < permissions.Len(); i++ { | ||
| pretty.Permissions = append(pretty.Permissions, permissions.Get(i).String()) | ||
| } |
There was a problem hiding this comment.
Preallocate Permissions slice to improve performance.
Since the length of permissions is known, consider preallocating the Permissions slice to avoid multiple allocations during the append operations.
Apply this diff to preallocate the slice:
permissions := msg.Get(fields.ByName("permissions")).List()
+pretty.Permissions = make([]string, 0, permissions.Len())
for i := 0; i < permissions.Len(); i++ {
pretty.Permissions = append(pretty.Permissions, permissions.Get(i).String())
}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.
| permissions := msg.Get(fields.ByName("permissions")).List() | |
| for i := 0; i < permissions.Len(); i++ { | |
| pretty.Permissions = append(pretty.Permissions, permissions.Get(i).String()) | |
| } | |
| permissions := msg.Get(fields.ByName("permissions")).List() | |
| pretty.Permissions = make([]string, 0, permissions.Len()) | |
| for i := 0; i < permissions.Len(); i++ { | |
| pretty.Permissions = append(pretty.Permissions, permissions.Get(i).String()) | |
| } |
…into kocu/amino-json-tests
Description
Related to: #21782
Recent changes had caused all amino JSON legacy equivalence rapid tests to early out before running. As a consequence changes to the x/tx Amino JSON encoder were not properly tested, and several issues were not caught.
They are:
The tests and specs for amino JSON encoding have been tightened up. The tests now closely mimic the same code paths followed in the updated TxDecoder in x/tx.
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
New Features
Bug Fixes
Refactor
Chores