move prod upgrade handler to v1.7.2; remove additional stargaze txs #1747
Conversation
WalkthroughThe changes in this pull request involve the addition of a new upgrade constant and associated upgrade handlers in the application. Specifically, a new constant Changes
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
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/upgrades/types.go(1 hunks)app/upgrades/upgrades.go(1 hunks)app/upgrades/v1_7.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: devskim
app/upgrades/v1_7.go
[failure] 35-35: 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] 36-36: 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 (6)
app/upgrades/upgrades.go (2)
28-28: Verify the upgrade handler implementation and sequence
The addition of v1.7.2 upgrade handler looks correct in sequence after v1.7.0. However, let's verify the implementation details.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Upgrade handler implementation verified and sequence is correct
The v1.7.2 upgrade handler is properly implemented in app/upgrades/v1_7.go with specific state migrations:
- Deletes duplicate withdrawal records for specific zones
- Mints and transfers 50.699994 uqatom to escrow
- Burns 16463.524950 qstars from escrow
- Updates redemption rates for all zones
No v1.7.1 upgrade was planned or skipped in the sequence, as there are no relevant references in the codebase. The upgrade from v1.7.0 to v1.7.2 is a valid progression.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the upgrade handler implementation and related constants
# Check the handler implementation
echo "Checking V010702UpgradeHandler implementation..."
ast-grep --pattern 'func V010702UpgradeHandler($$$) upgradetypes.UpgradeHandler {
$$$
}'
# Check the upgrade name constant
echo "Checking V010702UpgradeName constant..."
rg -A 1 "V010702UpgradeName.*=.*\"v1.7.2\""
# Look for any v1.7.1 references to ensure we're not missing anything
echo "Checking for any v1.7.1 references..."
rg "v1.7.1"
Length of output: 11147
Line range hint 1-41: Verify no critical upgrades are being skipped
The upgrade sequence jumps from v1.7.0 to v1.7.2. While this is technically valid, we should verify that no critical state migrations are being skipped.
✅ Verification successful
Let me gather more information about the v1.7.0 to v1.7.2 upgrade changes to verify if any critical migrations were skipped.
Based on the gathered information, I can now provide a final response:
No critical migrations are being skipped between v1.7.0 and v1.7.2
The v1.7.2 upgrade handler implements specific state migrations for mainnet and testnet:
- Deletes duplicate withdrawal records for specific zones (cosmoshub-4, stargaze-1)
- Performs token adjustments (minting uqatom and burning uqstars)
- Updates redemption rates for all zones
These changes appear to be targeted fixes rather than part of a sequential upgrade path, and there's no evidence of a v1.7.1 release or any skipped critical migrations. The version jump is likely intentional to align with specific fixes needed in production.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any evidence of v1.7.1 or other intermediate versions
# Check commit history for any mentions of v1.7.1
echo "Checking git history for v1.7.1 references..."
git log -p | rg -i "v1\.7\.1"
# Check issues and PRs for v1.7.1 discussions
echo "Checking GitHub issues and PRs..."
gh search issues "v1.7.1" --repo quicksilver-zone/quicksilver
# Look for migration files that might indicate skipped upgrades
echo "Checking for migration files..."
fd -e go -e json ".*1\.7\.[0-2].*"
Length of output: 1417
Script:
#!/bin/bash
# Check the changes and migrations between v1.7.0 and v1.7.2
# Look for upgrade handlers and their implementations
echo "Checking upgrade handlers implementation..."
ast-grep --pattern 'func V010702UpgradeHandler($$$) upgradetypes.UpgradeHandler { $$$ }'
# Check for any migration-related files or changes
echo "Checking for migration files and changes..."
rg -A 5 "V010702"
# Check release notes or changelog for v1.7.2
echo "Checking changelog..."
fd -g "CHANGELOG*" -X cat {}
Length of output: 6099
app/upgrades/v1_7.go (4)
16-20: Function V010700UpgradeHandler appears correct
The upgrade handler efficiently delegates to RunMigrations without additional logic, which is appropriate for this version upgrade.
22-25: Function V010702UpgradeHandler is properly defined
The function signature aligns with the expected UpgradeHandler format, ensuring compatibility with the upgrade framework.
35-36: Confirm that hardcoded hashes are non-sensitive
The hashes for the "stargaze-1" zone are hardcoded:
- Line 35:
"10af0ee10a97f01467039a69cbfb8df05dc3111c975d955ca51adda201f36555" - Line 36:
"0000000000000000000000000000000000000000000000000000000000000577"
Please verify that these hashes are public identifiers and do not contain sensitive information. Including sensitive data in the source code can pose security risks.
🧰 Tools
🪛 GitHub Check: devskim
[failure] 35-35: 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] 36-36: 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.
Line range hint 66-70: Review the impact of overriding redemption rates without a cap
The code overrides the redemption rate without a cap for all zones:
appKeepers.InterchainstakingKeeper.IterateZones(ctx, func(index int64, zone *icstypes.Zone) (stop bool) {
appKeepers.InterchainstakingKeeper.OverrideRedemptionRateNoCap(ctx, zone)
return false
})This action can have significant financial implications for stakeholders. Please verify that this is the intended behavior and that all potential impacts have been considered.
If needed, run the following script to identify zones affected by this change:
✅ Verification successful
Based on the search results, I can now provide a comprehensive response about the redemption rate override:
Uncapped redemption rate override is an intentional upgrade feature with controlled impact
The implementation shows this is a deliberate upgrade feature with the following key points:
- The normal
UpdateRedemptionRatefunction has built-in caps (max 2% increase, max 5% decrease) to prevent extreme rate changes - The
OverrideRedemptionRateNoCapfunction is specifically designed to bypass these caps for a one-time correction - Test coverage confirms the behavior:
- Regular updates are capped at 2% increase/5% decrease
- The uncapped override allows the rate to adjust to the true ratio between delegated assets and qAsset supply
- The change is part of a planned upgrade (v1.7) documented in the changelog
The removal of caps is intentional and necessary for this one-time correction during the upgrade. The redemption rate will return to using capped updates after the upgrade is complete.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: List all zones that will have their redemption rates overridden without a cap.
# Expected Result: Outputs all zones affected by the redemption rate override.
# Retrieve all zones
ast-grep --pattern 'func (k *InterchainstakingKeeper) IterateZones(ctx sdk.Context, cb func(index int64, zone *icstypes.Zone) (stop bool))'
Length of output: 141
Script:
#!/bin/bash
# Let's try to understand the implementation and impact of OverrideRedemptionRateNoCap
# Search for the implementation of OverrideRedemptionRateNoCap
ast-grep --pattern 'func (k *InterchainstakingKeeper) OverrideRedemptionRateNoCap'
# Search for any tests or documentation related to this function
rg -l "OverrideRedemptionRateNoCap"
# Look for any other redemption rate related functions for context
rg "RedemptionRate" -A 3
Length of output: 31331
🧰 Tools
🪛 GitHub Check: devskim
[failure] 35-35: 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] 36-36: 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.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1747 +/- ##
==========================================
- Coverage 61.60% 61.57% -0.04%
==========================================
Files 196 196
Lines 17013 17026 +13
==========================================
+ Hits 10481 10483 +2
- Misses 5686 5696 +10
- Partials 846 847 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
1. Summary
Summary by CodeRabbit
New Features
v1.7.2with its corresponding upgrade handler.Bug Fixes
Documentation