v1.7.4 upgrade #1758
v1.7.4 upgrade #1758
Conversation
WalkthroughThe changes in this pull request introduce a new upgrade constant and handler for version Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 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
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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/interchainstaking/keeper/abci.go (1)
Line range hint
28-28: Document the rationale for commented out HandleMaturedUnbondings.The comment
// k.HandleMaturedUnbondings(ctx)lacks explanation about what issues it was causing. Given that the PR objectives mention "requeuing failed cosmos unbonding processes", it would be valuable to document:
- What issues this task was causing
- The plan/timeline for revisiting this functionality
- Any potential impact on the unbonding process
go.mod (1)
Line range hint
486-486: Document the safety bug in the retraction commentWhile the retraction of v1.7.0 is a good safety measure, consider adding more context about the safety bug in the comment to help users understand the impact and necessity of avoiding this version.
-retract v1.7.0 // safety bug +retract v1.7.0 // safety bug: <brief description of the issue and potential impact>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
app/upgrades/types.go(1 hunks)app/upgrades/upgrades.go(1 hunks)app/upgrades/v1_7.go(2 hunks)go.mod(1 hunks)x/interchainstaking/keeper/abci.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: devskim
app/upgrades/v1_7.go
[failure] 93-93: A token or key was found in source code. If this represents a secret, it should be moved somewhere else.
Do not store tokens or keys in source code.
[failure] 105-105: A token or key was found in source code. If this represents a secret, it should be moved somewhere else.
Do not store tokens or keys in source code.
[failure] 134-134: A token or key was found in source code. If this represents a secret, it should be moved somewhere else.
Do not store tokens or keys in source code.
[failure] 135-135: A token or key was found in source code. If this represents a secret, it should be moved somewhere else.
Do not store tokens or keys in source code.
🔇 Additional comments (8)
app/upgrades/upgrades.go (1)
29-29: LGTM! Verify handler implementation.
The new upgrade entry follows the established naming convention and version sequence. However, let's verify that the handler implements all the objectives mentioned in the PR description.
✅ Verification successful
Handler implementation verified and matches PR objectives
The handler implementation in app/upgrades/v1_7.go correctly addresses all the objectives:
- Removes old withdrawal records by deleting specific hashes for
stargaze-1 - Requeues failed cosmos unbonding by resetting a specific withdrawal record for
cosmoshub-4 - Eliminates outdated inter-chain queries by deleting two specific query hashes
- Version sequence is correct: v1.7.0 -> v1.7.2 -> v1.7.4
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify handler implementation and version sequence
# Check if handler exists and implements required functionality
echo "Checking handler implementation..."
ast-grep --pattern 'func V010704UpgradeHandler($$$) upgradetypes.UpgradeHandler {
$$$
}'
# Search for withdrawal cleanup
echo "Checking withdrawal cleanup implementation..."
rg -A 5 "withdrawal" "app/upgrades/v1_7.go"
# Search for cosmos unbonding requeue
echo "Checking unbonding requeue implementation..."
rg -A 5 "unbonding" "app/upgrades/v1_7.go"
# Search for inter-chain query cleanup
echo "Checking inter-chain query cleanup..."
rg -A 5 "inter.*chain.*quer" "app/upgrades/v1_7.go"
# Verify version sequence
echo "Checking version sequence..."
rg "V0107.*UpgradeName.*=.*" "app/upgrades/types.go"
Length of output: 5026
app/upgrades/types.go (1)
54-54: LGTM! The upgrade constant follows established patterns.
The new constant V010704UpgradeName is correctly defined and follows the existing naming convention for mainnet upgrades.
Let's verify that this constant is properly used in the upgrade handler:
✅ Verification successful
Let me gather more information about the upgrade handler implementation:
✅ Constant is properly integrated with its upgrade handler
The verification shows that:
- The constant
V010704UpgradeNameis correctly registered in the upgrade configuration with its corresponding handlerV010704UpgradeHandler - The upgrade handler is properly implemented in
app/upgrades/v1_7.gowith specific state migrations for mainnet/testnet:- Cleanup of withdrawal records
- Redemption rate adjustments
- Removal of outdated ICQ queries
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the upgrade constant usage in the upgrade handler
# Expected: The constant should be used in the upgrades configuration
# Search for usage of the new upgrade constant
rg "V010704UpgradeName" -A 5
# Search for the upgrade handler registration pattern
ast-grep --pattern 'CreateUpgradeHandler: V010704UpgradeHandler'
Length of output: 821
Script:
#!/bin/bash
# Search for the V010704UpgradeHandler function definition
ast-grep --pattern 'func V010704UpgradeHandler($$$) $$$'
# Search for any references to this handler
rg "V010704UpgradeHandler" -A 5
Length of output: 4678
x/interchainstaking/keeper/abci.go (1)
72-72: LGTM! Critical fix for zone state persistence.
The addition of k.SetZone(ctx, zone) is essential as it persists the zone state after incrementing the balanceWaitgroup, preventing potential state inconsistencies that could lead to the balanceWaitgroup going below zero.
Let's verify if there are other similar patterns in the codebase that might need the same fix:
✅ Verification successful
Let me analyze the results from the shell scripts to verify if there are other places where zone state might need to be persisted after balanceWaitgroup modifications.
Let me try a different search pattern to find balanceWaitgroup modifications.
Based on the search results, I can now provide a comprehensive verification of the balanceWaitgroup state persistence. Let me analyze the patterns:
- The original fix adds
k.SetZone(ctx, zone)afterzone.DelegationAddress.IncrementBalanceWaitgroup()in abci.go - Looking at all balanceWaitgroup modifications:
- Increments occur in abci.go and zones.go
- Decrements occur in zones.go
- Direct assignments occur in callbacks.go and keeper.go
I can see that all other locations that modify balanceWaitgroup already have corresponding SetZone calls:
- In zones.go, both increment operations are followed by
k.SetZone(ctx, &zone) - In callbacks.go, all balance waitgroup zeroing operations are followed by
k.SetZone(ctx, zone) - In keeper.go, the performance address waitgroup assignment is followed by
k.SetZone(ctx, zone)
LGTM! The fix correctly addresses the only missing state persistence after balanceWaitgroup modification.
The addition of k.SetZone(ctx, zone) in abci.go was necessary as it was the only location where the balanceWaitgroup was modified without persisting the state. All other code paths correctly persist zone state after modifying the balanceWaitgroup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for patterns where zone.DelegationAddress is modified without subsequent k.SetZone call
# This could help identify other potential state persistence issues
# Search for DelegationAddress modifications
ast-grep --pattern 'zone.DelegationAddress.$$$'
# Search for balanceWaitgroup modifications
rg -A 5 'balanceWaitgroup'
# Search for SetZone calls to analyze existing patterns
rg -A 5 'SetZone\('
Length of output: 109641
Script:
#!/bin/bash
# Search for patterns where balanceWaitgroup is modified without subsequent SetZone call
# This will help identify if there are any other places where we might be missing state persistence
# Look for IncrementBalanceWaitgroup calls and their context
ast-grep --pattern 'IncrementBalanceWaitgroup()' -A 5
# Look for DecrementBalanceWaitgroup calls and their context
ast-grep --pattern 'DecrementBalanceWaitgroup()' -A 5
# Look for direct assignments to balanceWaitgroup
ast-grep --pattern 'BalanceWaitgroup = $_'
Length of output: 156
Script:
#!/bin/bash
# Search for patterns where balanceWaitgroup is modified
# Using ripgrep for text-based search since ast-grep patterns didn't match
# Look for increment/decrement operations
rg -B 3 -A 3 'IncrementBalanceWaitgroup|DecrementBalanceWaitgroup'
# Look for direct assignments to balanceWaitgroup
rg -B 3 -A 3 'BalanceWaitgroup\s*='
# Look for all references to balanceWaitgroup to ensure we don't miss any patterns
rg -B 3 -A 3 'balanceWaitgroup' -i
Length of output: 41468
go.mod (1)
12-12: Verify the stability of cometbft-db v0.12.0
The downgrade from v1.0.1 to v0.12.0 is intentional to resolve ICA functionality issues as mentioned in the PR objectives. However, we should verify that this version doesn't introduce other compatibility issues.
✅ Verification successful
Let me gather more information about the dependency usage and potential impacts.
✅ The cometbft-db v0.12.0 downgrade appears safe and well-justified
The verification shows:
- No security vulnerabilities reported for cometbft-db v0.12.0
- Limited direct usage of cometbft-db in the codebase (only in forceprune.go and store_helper.go)
- Multiple ICA-related files that would benefit from this fix
- Clean dependency resolution in go.sum
The downgrade to v0.12.0 is a targeted fix for ICA functionality and appears to be a stable version with no known security concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any known issues or breaking changes between v0.12.0 and v1.0.1
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/cometbft/cometbft-db") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
# Check for any references to this version in issues or discussions
gh search issues "cometbft-db v0.12.0" --repo quicksilver-zone/quicksilver
Length of output: 439
Script:
#!/bin/bash
# Check for any direct usage of cometbft-db in the codebase
rg -l "cometbft-db" --type go
# Check for any specific version constraints or related changes in go.sum
grep "cometbft-db" go.sum
# Look for any related test failures or issues in recent commits
git log --oneline -n 10 | grep -i "cometbft\|ica"
# Check for any ICA-related code that might be affected
rg -l "InterchainAccount" --type go
Length of output: 771
app/upgrades/v1_7.go (4)
4-5: Correct Import of 'time' Package
The addition of the time package is necessary for handling time.Time{} in the upgrade handler.
85-86: Appropriate Environment Check Before Upgrade Logic
The condition if isMainnet(ctx) || isTest(ctx) ensures that the upgrade logic executes only in the intended environments. This prevents unintended changes in other environments.
130-131: Verify Direct Modification of LastRedemptionRate
Directly setting zone.LastRedemptionRate may bypass validation or trigger unintended side effects. Ensure that this direct assignment is safe and consistent with the application's state management practices.
To confirm that no unintended consequences arise from directly setting LastRedemptionRate, consider using an existing method that handles validation and events, or ensure that all necessary procedures are followed after the assignment.
110-112:
Avoid Using panic in Upgrade Handler
Using panic when a withdrawal record is not found can cause the upgrade process to halt unexpectedly, leading to potential downtime. Instead, handle the error gracefully to allow the upgrade to continue smoothly.
Suggested change:
if !found {
- panic("record not found")
+ appKeepers.Logger(ctx).Error("Withdrawal record not found", "zone", hashRecord.Zone, "hash", hashRecord.Hash)
+ // Decide whether to skip processing this record or handle it accordingly
+ continue
}⛔ Skipped due to learnings
Learnt from: joe-bowman
PR: quicksilver-zone/quicksilver#1099
File: app/upgrades/upgrades.go:0-0
Timestamp: 2024-11-12T07:06:41.714Z
Learning: Using `panic` in the context of upgrade handlers is acceptable to ensure the upgrade process is halted in case of critical issues.
Learnt from: faddat
PR: quicksilver-zone/quicksilver#1099
File: app/upgrades/upgrades.go:50-114
Timestamp: 2024-11-12T07:06:35.954Z
Learning: Using `panic` in the context of upgrade handlers is considered acceptable in the Quicksilver project to ensure the upgrade process halts immediately if an error is encountered.
Learnt from: joe-bowman
PR: quicksilver-zone/quicksilver#1747
File: app/upgrades/v1_7.go:44-49
Timestamp: 2024-11-19T14:54:21.046Z
Learning: In the Quicksilver codebase, panicking in an upgrade handler (e.g., using `panic(err)` within `V010702UpgradeHandler` in `app/upgrades/v1_7.go`) is acceptable to prevent the state from being committed when an error occurs during an upgrade.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1758 +/- ##
==========================================
- Coverage 61.57% 61.40% -0.17%
==========================================
Files 196 196
Lines 17026 17075 +49
==========================================
+ Hits 10483 10485 +2
- Misses 5696 5742 +46
- Partials 847 848 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
1. Summary
Summary by CodeRabbit
Release Notes
New Features
v1.7.4with its corresponding handler, enhancing the upgrade management system.Bug Fixes
Dependency Updates
github.com/cometbft/cometbft-dbfor enhanced stability and security.v1.7.0due to a safety bug.