fix(x/auth/tx): avoid panic from newWrapperFromDecodedTx #23170
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a bug fix for the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (5)
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
|
mmsqe
changed the title
mmsqe
requested review from
facundomedica,
testinginprod,
aaronc and
a team
as code owners
when AuthInfo.Fee is optional in decodedTx lint Apply suggestions from code review
tac0turtle
added
the
backport/v0.52.x
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
x/auth/tx/gogotx_test.go (1)
17-26: Consider validating final wrapper objects as well as the error.While the test verifies that no error is returned, it might also be beneficial to confirm expected fields in the resulting wrapper (e.g., fees, payer, etc.) for stronger coverage. Using
require.NoError(t, err)is typically more explicit thanrequire.Nil.CHANGELOG.md (4)
Line range hint
1-1: Add title to changelog fileThe file should start with a clear title like "# Changelog" to follow standard changelog conventions.
+ # Changelog
Line range hint
13-20: Improve changelog guiding principles formattingThe guiding principles section should be formatted as a proper markdown list for better readability.
- Changelogs are for humans, not machines. - There should be an entry for every single version. - The same types of changes should be grouped. - Versions and sections should be linkable. - The latest version comes first. - The release date of each version is displayed. - Mention whether you follow Semantic Versioning. + * Changelogs are for humans, not machines + * There should be an entry for every single version + * The same types of changes should be grouped + * Versions and sections should be linkable + * The latest version comes first + * The release date of each version is displayed + * Mention whether you follow Semantic Versioning
Line range hint
22-32: Improve usage instructions formattingThe usage instructions section should use consistent markdown formatting and clearer structure.
- Change log entries are to be added to the Unreleased section under the - appropriate stanza (see below). Each entry is required to include a tag and - the Github issue reference in the following format: + # Usage + + ## Adding Entries + Add change log entries to the "Unreleased" section under the appropriate stanza. + Each entry must include: + * A tag indicating where the change was made (e.g. `(x/staking)`) + * The GitHub issue/PR reference + * A description of the change + + Format: `* (<tag>) \#<issue-number> <description>`
Line range hint
34-41: Standardize change type descriptionsThe "Types of changes" section should use consistent formatting and provide clearer descriptions.
- "Features" for new features. - "Improvements" for changes in existing functionality. - "Deprecated" for soon-to-be removed features. - "Bug Fixes" for any bug fixes. + ## Types of Changes + + * `Features` - New functionality added + * `Improvements` - Enhancements to existing functionality + * `Deprecated` - Features that will be removed in future versions + * `Bug Fixes` - Fixes for broken functionality + * `Breaking Changes` - Changes that break backward compatibility: + * `Client Breaking` - Changes affecting client applications + * `CLI Breaking` - Changes affecting command line interface + * `API Breaking` - Changes affecting programmatic interfaces + * `State Machine Breaking` - Changes affecting blockchain state
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)x/auth/tx/gogotx.go(1 hunks)x/auth/tx/gogotx_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
x/auth/tx/gogotx_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/gogotx.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🔇 Additional comments (5)
x/auth/tx/gogotx_test.go (2)
1-2: File-level overview and test naming clarity.The new file is appropriately placed under the same package, and the test function name clearly indicates the target being tested. This setup follows Golang naming best practices for test files (
*_test.go).
3-7: Imports usage.Using
testify/requireis a good practice for concise test assertions. No issues found here.x/auth/tx/gogotx.go (2)
35-37: Graceful handling of optional fee.Great addition of a default to an empty coin set in case of a nil
AuthInfo.Fee. This prevents panics and aligns with the design goal of optional fees.
39-74: Conditional logic for fee processing.This
ifcheck accurately ensures no fee logic is processed whenFeeis nil. The follow-on validations and conversions are handled cleanly, reducing the risk of a nil-pointer dereference.CHANGELOG.md (1)
Line range hint
1-5000: Fix broken links in changelogSeveral PR/issue links appear to be broken or malformed. Please verify all links are correct and properly formatted.
Co-authored-by: Alex | Skip <alex@djinntek.world>
julienrbrt
requested review from
alpe
and removed request for
facundomedica and
testinginprod
Co-authored-by: Alex | Skip <alex@djinntek.world> Co-authored-by: Alex | Interchain Labs <alex@skip.money> (cherry picked from commit 517839b)
when AuthInfo.Fee is optional in decodedTx
Description
Closes: #XXXX
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
AuthInfo.Feeis optional.Tests
AuthInfo.