fix(x/auth/tx): avoid panic from intoAnyV2 when v1.PublicKey is optional #23148
Conversation
mmsqe
requested review from
facundomedica,
testinginprod,
aaronc and
a team
as code owners
📝 WalkthroughWalkthroughThis pull request introduces a bug fix for the Cosmos SDK's Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
![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 (3)
CHANGELOG.md (3)
Line range hint
1-1: Add missing title to changelog fileThe changelog file should start with a clear title like "# Changelog" to follow standard markdown practices.
+ # Changelog
53-53: Fix inconsistent version link formattingThe version link format is inconsistent with other version links in the document. It should follow the same pattern as other version links.
- ## v0.47.15 - 2024-12-16 + ## [v0.47.15](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.15) - 2024-12-16Also applies to: 54-54
Line range hint
2892-2892: Remove redundant empty code blockThe empty code block at the end of the file serves no purpose and should be removed.
- ```
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)x/auth/tx/builder.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
x/auth/tx/builder.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 (2)
x/auth/tx/builder.go (1)
296-300: Ensure robust handling of nilPublicKeyThese lines correctly handle the scenario when
v1.PublicKeyisnilby checking before callingintoAnyV2. This aligns with the PR’s goal of preventing possible panics. Please also confirm that test coverage includes cases wherePublicKeyis intentionally left unset to validate this logic works as expected.CHANGELOG.md (1)
Line range hint
1-2892: LGTM on overall changelog structure and contentThe changelog follows good practices by:
- Organizing changes by version with clear release dates
- Categorizing changes into features, improvements, bug fixes etc.
- Including PR references
- Providing detailed descriptions of breaking changes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
x/auth/tx/builder_test.go (2)
6-6: Consider renaming the import alias for clarity.
Currently, the aliasanymight be confused with Go’sanytype. Consider using a more descriptive alias, e.g.,protoAny.-import ( - any "github.com/cosmos/gogoproto/types/any" - "github.com/stretchr/testify/require" - ... -) +import ( + protoAny "github.com/cosmos/gogoproto/types/any" + "github.com/stretchr/testify/require" + ... +)
12-15: Expand test assertions for better coverage.
Currently, you’re only checking that the result is non-nil. Consider asserting other properties (e.g., length, content) to ensureintoV2SignerInfobehaves correctly under different permutations (like providing actual data vs. empty arrays).
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/auth/tx/builder_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/auth/tx/builder_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"
|
@Mergifyio backport release/v0.52.x |
✅ Backports have been created
|
…nal (backport #23148) (#23187) Co-authored-by: mmsqe <[email protected]> Co-authored-by: Julien Robert <[email protected]>
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
x/auth/txmodule when handling optionalPublicKeyvalues.intoV2SignerInfofunction to prevent nil dereference issues.intoV2SignerInfofunction to ensure expected behavior with various inputs.