fix(client/v2): *big.Int unmarshal #21853
Conversation
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request updates the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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
|
julienrbrt
changed the title
julienrbrt
changed the title
julienrbrt
changed the title
julienrbrt
requested review from
kocubinski,
testinginprod,
aaronc,
a team,
facundomedica and
JulianToledano
as code owners
julienrbrt
added
the
backport/v0.52.x
This comment has been minimized.
This comment has been minimized.
|
doing a fix for the marshalling as well. |
![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 (8)
client/v2/autocli/flag/legacy_dec.go (5)
11-19: LGTM: decType struct and methods are well-implemented.The
decTypestruct and its methods follow Go naming conventions and are concise. TheNewValuemethod creates a newdecValue, andDefaultValuereturns a default string representation of zero.Consider adding a comment to explain the purpose of the
decTypestruct, especially since it's empty. This would improve code readability and maintainability.
21-23: LGTM: decValue struct is correctly declared.The
decValuestruct follows Go naming conventions, and the field name is clear and concise.Consider adding a comment to explain the purpose of the
decValuestruct and itsvaluefield. This would improve code readability and help other developers understand the intended use of this type.
25-31: LGTM: Get and String methods are implemented correctly.Both methods follow Go naming conventions and are concise. The
Stringmethod appropriately returns the string representation of the value.The
Getmethod ignores its input parameter of typeprotoreflect.Value. Consider adding a comment to explain why this parameter is ignored or remove it if it's not needed. This would improve code clarity.
46-48: LGTM: Type method is correctly implemented.The
Typemethod follows Go naming conventions and returns the expected type string.Consider defining the return value "cosmos.Dec" as a constant at the package level. This would improve maintainability and reduce the risk of typos if the string is used elsewhere in the package.
1-48: Overall, the implementation is solid with minor suggestions for improvement.The file introduces well-structured types and methods for handling decimal values, following Go naming conventions and best practices. The code is focused and implements the required functionality effectively.
To further enhance the code:
- Consider adding package-level documentation to explain the purpose and usage of this package.
- Implement unit tests to ensure the correctness of the decimal handling, especially the conversion process in the
Setmethod.- If this package is part of a larger module, ensure that it integrates well with other components and follows the overall architecture of the system.
client/v2/CHANGELOG.md (1)
56-59: Consider expanding the bug fix description.The bug fix entry is correctly formatted and linked. However, the description could be more informative about the nature of the fix. Consider expanding it to briefly explain the impact of the bug and how it was resolved.
For example:
* [#21853](https://github.com/cosmos/cosmos-sdk/pull/21853) Fix `*big.Int` unmarshalling in transactions to prevent panics when handling certain numeric inputs.This provides more context about the bug and its resolution.
x/tx/decode/decode.go (1)
139-139: Enhanced error message for Any unmarshal failureThe change improves error handling by providing a more specific error message for Any unmarshal failures. This enhances debugging capabilities.
Consider using
errors.Newinstead offmt.Sprintffor slightly better performance:return nil, errorsmod.Wrap(ErrTxDecode, errors.New("cannot unmarshal Any message").Error())client/v2/autocli/flag/builder.go (1)
71-71: LGTM: Scalar flag type map updated correctlyThe
initmethod is properly updated to include the newDecScalarTypein thescalarFlagTypesmap. This change enables theBuilderto handle the new decimal scalar type consistently with other scalar types.Consider adding a comment explaining the purpose of
decType{}or providing a link to its definition for better code readability and maintainability.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (5)
- client/v2/CHANGELOG.md (1 hunks)
- client/v2/autocli/flag/builder.go (2 hunks)
- client/v2/autocli/flag/legacy_dec.go (1 hunks)
- x/auth/tx/builder.go (3 hunks)
- x/tx/decode/decode.go (3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
client/v2/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"client/v2/autocli/flag/builder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/v2/autocli/flag/legacy_dec.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/tx/builder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/tx/decode/decode.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (15)
client/v2/autocli/flag/legacy_dec.go (1)
1-9: LGTM: Package declaration and imports are well-organized.The package name is concise and appropriate. Imports are correctly organized with standard library imports separated from third-party imports by a blank line. No unused imports are present.
client/v2/CHANGELOG.md (3)
56-59: LGTM: New features are well-documented.The new feature entries in the "Unreleased" section are properly formatted, clear, and correctly linked to their respective issues/PRs. This provides good traceability and information for users and developers.
56-59: LGTM: Improvement is well-documented.The new improvement entry in the "Unreleased" section is properly formatted, clear, and correctly linked to its PR. The description provides useful information about the enhancement to error messages.
Line range hint
1-59: Overall, the changelog updates are well-structured and informative.The new entries in the "Unreleased" section of the changelog are properly formatted, clearly written, and correctly linked to their respective issues/PRs. The updates effectively communicate the recent changes to the
client/v2module, covering new features, improvements, and bug fixes. The adherence to the changelog guidelines ensures that the document remains readable and useful for both users and developers.🧰 Tools
🪛 LanguageTool
[uncategorized] ~54-~54: A comma may be missing after the conjunctive/linking adverb ‘Instead’.
Context: ...autocli.AppOptionsandflag.Builder`. Instead client/v2 uses the address codecs prese...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
x/tx/decode/decode.go (3)
87-87: Improved error handlingThe change enhances error context by wrapping the error with
ErrTxDecode. This follows Go best practices and improves error traceability.
151-151: Consistent error wrappingThe change maintains consistency in error handling by wrapping the error with
ErrTxDecode. This aligns with the error handling pattern used throughout the function.
Line range hint
87-151: Overall improvement in error handlingThe changes in this file consistently enhance error handling by wrapping errors with
ErrTxDecodeusingerrorsmod.Wrap. This approach provides better context for debugging and aligns with Go best practices. The modifications improve the maintainability and debuggability of theDecodemethod.x/auth/tx/builder.go (6)
115-115: Improved error handling with contextThe error wrapping here is a good improvement. It provides more context about the operation that failed, making debugging easier. This change aligns well with the Uber Golang style guide recommendations for error handling.
139-139: Consistent error handling improvementThis change follows the same pattern of improved error handling as seen earlier. It provides clear context about the marshaling operation that failed, which is beneficial for debugging and error tracing.
144-144: Consistent error handling patternThis change maintains the consistent pattern of improved error handling seen throughout the file. It provides clear context about the auth info marshaling operation, which is crucial for effective debugging and error tracing.
153-153: Continued improvement in error handlingThis change further demonstrates the consistent approach to improving error handling throughout the file. The added context about the tx raw marshaling operation enhances the ability to trace and debug issues effectively.
158-158: Comprehensive error handling improvementThis final change completes the set of improvements to error handling in the
getTxmethod. The consistent approach to providing detailed context in error messages significantly enhances the method's maintainability and debuggability.
Line range hint
115-158: Overall improvement in error handlingThe changes in this file demonstrate a systematic approach to enhancing error handling in the
getTxmethod. By consistently wrapping errors with additional context, the code now provides more informative error messages. This improvement aligns well with the Uber Golang style guide and significantly enhances the code's maintainability, debuggability, and overall quality.Some key benefits of these changes include:
- Easier troubleshooting and debugging
- Improved error tracing capabilities
- Better adherence to Go best practices
Great job on implementing these improvements consistently throughout the method!
client/v2/autocli/flag/builder.go (2)
29-29: LGTM: New scalar type constant added correctlyThe new
DecScalarTypeconstant is well-defined and follows the existing pattern for scalar type constants in this file. Its placement and naming convention are appropriate.
29-29: Summary: New decimal scalar type support addedThe changes introduce support for a new decimal scalar type in the autocli flag system. This includes:
- A new
DecScalarTypeconstant.- An update to the
Builder.init()method to handle the new type.These modifications are consistent with the existing code structure and conventions.
To ensure completeness of the implementation, please verify the existence and correctness of the
decTypedefinition and its associated methods. Run the following command to check for thedecTypedefinition:Also applies to: 71-71
✅ Verification successful
Verification: decType Implemented Correctly
The
decTypeis properly defined inclient/v2/autocli/flag/legacy_dec.goas an empty struct and is correctly utilized withinbuilder.goby assigningdecType{}tob.scalarFlagTypes[DecScalarType]. The absence of associated methods is consistent with its intended usage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for decType definition in Go files rg --type go 'type\s+decType\s+struct'Length of output: 99
Script:
#!/bin/bash # Find all methods associated with decType rg --type go 'func\s+\(d\s+decType\)\s+'Length of output: 42
Script:
#!/bin/bash # Check usage of decType within Builder.init() and related functions rg --type go 'decType' client/v2/autocli/flag/builder.goLength of output: 103
julienrbrt
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
client/v2/autocli/query.go (1)
239-241: Improve comment formatting for better readabilityThe comment is split mid-sentence across multiple lines, which may hinder readability. Consider reformatting it for clarity.
Apply this diff to adjust the comment:
- // If the decimal doesn't contain a point, we assume it's a value formatted using the legacy - // `math.Dec`. So we try to parse it as an integer and then convert it to a - // decimal. + // If the decimal doesn't contain a point, we assume it's a value formatted using the legacy `math.Dec`. + // So we try to parse it as an integer and then convert it to a decimal.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- client/v2/CHANGELOG.md (1 hunks)
- client/v2/autocli/flag/legacy_dec.go (1 hunks)
- client/v2/autocli/query.go (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/v2/CHANGELOG.md
- client/v2/autocli/flag/legacy_dec.go
🧰 Additional context used
📓 Path-based instructions (1)
client/v2/autocli/query.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
client/v2/autocli/query.go (2)
8-8: Import "math/big" is necessary for big integer operationsThe addition of
"math/big"is appropriate as it is required for handling big integers in the custom scalar encoding for"cosmos.Dec".
235-259: Custom scalar encoding for "cosmos.Dec" is well-implementedThe custom scalar encoding correctly handles the conversion of legacy-formatted decimals and includes proper error handling for invalid inputs. The implementation aligns with best practices and conforms to the Uber Go style guide.
julienrbrt
changed the title
Reverted the fix for marshalling at e6e4583, as it was indeed not autocli related. |
(cherry picked from commit 43c41be) # Conflicts: # client/v2/CHANGELOG.md # x/tx/decode/decode.go
Description
Closes: #20935
I am mostly curious about what has changed between 0.50 to 0.52/main x/tx, which makes us need those extra encoders. For instance marshalling was working fine on 0.50, but regressed in 0.52.Fixed by #22161Author 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
Bug Fixes
*big.Intin transactions.Documentation