fix: query signal after successful try upgrade #4045
Conversation
rootulp
requested review from
staheri14 and
evan-forbes
and removed request for
a team
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request enhance the integration and unit tests for the upgrade signaling mechanism in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 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
🧹 Outside diff range and nitpick comments (2)
x/signal/keeper.go (1)
126-128: Consider extracting duplicate upgrade key checkThe upgrade key check is duplicated in both
VersionTallyandTallyVotingPower. Consider extracting this into a helper method to improve maintainability and ensure consistent behavior.Example implementation:
+// isUpgradeKey returns true if the given key is the upgrade key +func (k Keeper) isUpgradeKey(key []byte) bool { + return bytes.Equal(key, types.UpgradeKey) +} func (k Keeper) VersionTally(...) { ... for ; iterator.Valid(); iterator.Next() { - if bytes.Equal(iterator.Key(), types.UpgradeKey) { + if k.isUpgradeKey(iterator.Key()) { continue } ... } func (k Keeper) TallyVotingPower(...) { ... for ; iterator.Valid(); iterator.Next() { - if bytes.Equal(iterator.Key(), types.UpgradeKey) { + if k.isUpgradeKey(iterator.Key()) { continue } ... }Also applies to: 165-167
x/signal/keeper_test.go (1)
433-468: Test successfully addresses the panic issue mentioned in #4007.The test case effectively verifies that querying version tally after a successful upgrade doesn't panic. However, there are a few suggestions to make the test more robust:
- The test could verify
VotingPoweris 0 after the upgrade- Consider adding assertions for version 3 tally as well, since that's the version being signaled
res, err := upgradeKeeper.VersionTally(goCtx, &types.QueryVersionTallyRequest{ Version: 2, }) require.NoError(t, err) +require.EqualValues(t, 0, res.VotingPower) require.EqualValues(t, 100, res.ThresholdPower) require.EqualValues(t, 120, res.TotalVotingPower) + +// Also verify version 3 tally +res, err = upgradeKeeper.VersionTally(goCtx, &types.QueryVersionTallyRequest{ + Version: 3, +}) +require.NoError(t, err) +require.EqualValues(t, 100, res.VotingPower) +require.EqualValues(t, 100, res.ThresholdPower) +require.EqualValues(t, 120, res.TotalVotingPower)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
x/signal/integration_test.go(1 hunks)x/signal/keeper.go(3 hunks)x/signal/keeper_test.go(1 hunks)
🔇 Additional comments (5)
x/signal/integration_test.go (1)
63-71: LGTM! Good test coverage for the empty tally query issue.
This test addition directly addresses issue #4007 by verifying that querying the version tally remains functional after a successful upgrade. The assertions ensure consistent behavior and prevent potential panics.
Let's verify there are no other places where version tally queries might need similar protection:
✅ Verification successful
Based on the search results, I can now generate the final response:
✓ The version tally query implementation is properly protected against empty data
The codebase search reveals that the VersionTally implementation in x/signal/keeper.go properly handles empty data cases by:
- Initializing voting power with
sdk.NewInt(0) - Using proper error handling throughout the query flow
- Returning consistent response structure even when no votes are present
The test additions in the integration test complement the existing test coverage in keeper_test.go, which already includes test cases for:
- Empty tally queries
- Queries after reset
- Queries with varying voting power states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other version tally query calls that might need similar protection
ast-grep --pattern 'VersionTally($$$)'
Length of output: 40
Script:
#!/bin/bash
# Let's try a more comprehensive search for version tally related code
rg -i "versiontally" -A 3 -B 3
Length of output: 31038
Script:
#!/bin/bash
# Search for the query handler implementation
ast-grep --pattern 'func $_ VersionTally($$$) {
$$$
}'
Length of output: 58
Script:
#!/bin/bash
# Look for the protobuf service definition
rg "VersionTally" --type proto
Length of output: 60
x/signal/keeper.go (3)
4-4: LGTM: Import addition is appropriate
The addition of the "bytes" package import is necessary for the new byte comparison operations.
165-167: LGTM: Consistent handling of upgrade key
The change maintains consistency with the VersionTally fix by properly skipping the upgrade key during power tallying.
126-128: Fix confirmed: Properly handles upgrade key during version tally
This change correctly addresses issue #4007 by skipping the upgrade key during iteration, preventing the index panic when querying an empty signal tally.
Let's verify that this is the only place where we might encounter similar issues with the upgrade key:
✅ Verification successful
Upgrade key handling is correctly implemented
The search results confirm that the upgrade key check is only needed in the signal module's version tally operations, where it's already properly implemented. Other store iterations in the codebase either:
- Use specific prefixes that exclude the upgrade key
- Have different purposes where upgrade key handling is not relevant
- Intentionally iterate over all keys (like in ResetTally)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other store iterations that might need similar upgrade key handling
ast-grep --pattern 'store.Iterator($$$)'
Length of output: 42
Script:
#!/bin/bash
# Search for store iterations using ripgrep for a broader search
rg -i "iterator|store.iter" --type go -A 3 -B 3
Length of output: 5226
x/signal/keeper_test.go (1)
458-460: Well-documented test case with clear reference to the issue.
The comment effectively explains the bug being addressed and includes a link to the issue.
Closes #4007
Testing
Using the updated single-node.sh script (see other PR), I can query the version tally even after a successful try upgrade.