fix: query balances with --resolve-denom panics on invalid metadata.Display #23149
Conversation
arlai-mk
requested review from
julienrbrt,
sontrinh16 and
a team
as code owners
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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
|
![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 (1)
x/bank/keeper/grpc_query.go (1)
68-70: Consider adding explicit handling or logging for the fallback scenario.When
metadata.Displayis invalid, the code defaults to usingkey.K2(). While this behavior is correct, it would be helpful to add a comment or log message to clarify that the fallback is triggered due tometadata.Displaybeing invalid. This will improve the code's maintainability and debuggability.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/bank/keeper/grpc_query.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/bank/keeper/grpc_query.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
|
@arlai-mk looks good! can you add a test for this case? |
|
+1 for the test case @arlai-mk |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/bank/keeper/keeper_test.go (1)
118-131: Consider adding validation for required fields.The helper function creates metadata without validating required fields. Consider adding validation to ensure test data integrity.
func addIncompleteMetadata(ctx context.Context, k keeper.BaseKeeper) { + if incompleteBaseDenom == "" || incompletePath == "" { + panic("incomplete metadata test requires valid base denom and path") + } metadata := banktypes.Metadata{ Description: "Incomplete metadata without display field", DenomUnits: []*banktypes.DenomUnit{
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
x/bank/keeper/grpc_query_test.go(3 hunks)x/bank/keeper/keeper_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
x/bank/keeper/grpc_query_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/bank/keeper/keeper_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"
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-integration
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: golangci-lint
- GitHub Check: Summary
🔇 Additional comments (7)
x/bank/keeper/grpc_query_test.go (5)
123-124: LGTM! Good test data setup.The test properly initializes an incomplete metadata coin for testing the panic fix.
127-128: LGTM! Good ordering of test data.The coins are correctly ordered alphabetically to ensure consistent test behavior.
138-141: LGTM! Clear test assertions.The assertions are well-structured and verify both the length and content of the response.
Line range hint
159-196: LGTM! Comprehensive pagination testing.The pagination tests thoroughly verify the behavior with both regular and IBC denoms.
142-156: Test case properly verifies panic prevention.The test case effectively verifies that the query doesn't panic with incomplete metadata and correctly falls back to the original denomination.
Let's verify that this is the only test covering the panic prevention:
✅ Verification successful
Test coverage for panic prevention is complete and well-designed.
The test case is the only one needed as it comprehensively verifies both the panic prevention and correct fallback behavior in the context of incomplete metadata resolution. The test is well-integrated into a larger test suite that covers related functionality, ensuring proper coverage of edge cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other test cases that might be testing the same panic scenario rg -A 2 "NotPanics.*AllBalances.*resolve.*denom.*metadata"Length of output: 318
Script:
#!/bin/bash # Search more broadly for NotPanics tests related to AllBalances rg -B 2 -A 2 "NotPanics.*AllBalances" # Also get the full test file content to understand the complete test coverage cat x/bank/keeper/grpc_query_test.goLength of output: 27473
x/bank/keeper/keeper_test.go (2)
44-45: LGTM! Clear constant naming.Constants are well-named and follow the existing naming pattern in the file.
88-90: LGTM! Consistent helper function implementation.The helper function follows the same pattern as other coin creation helpers in the file.
|
Hi @julienrbrt @aljo242, For information:
Obviously, I checked that the new test passes fine, but I also made sure that it is relevant for this case, as it would not pass with the old code (without the fix included in this PR). If you have any question, please let me know. Thank you. |
julienrbrt
added
the
backport/v0.52.x
…isplay (#23149) Co-authored-by: Alex | Skip <[email protected]> (cherry picked from commit f77a274)
…isplay (backport #23149) (#23232) Co-authored-by: arlai <[email protected]>
Description
Closes: #23147
This PR checks that the
metadata.Displayis a valid denom when querying bank balances with--resolve-denomflag set, so that the query does not panic anymore if theDisplayfield is not a valid denom. It should fall back to the original denom.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