refactor(staking): migrate staking module #934
Conversation
WalkthroughThis pull request introduces enhancements to the upgrade process for a blockchain application, focusing on staking module migration and coin swapping functionality. The changes include adding a new migration function for the staking module, implementing a coin swapping mechanism, and extending upgrade testing capabilities. The modifications ensure proper handling of staking parameters, validator data, and provide additional testing coverage for coin transformation operations. Changes
Sequence DiagramsequenceDiagram
participant Upgrade as Upgrade Process
participant StakingKeeper as Staking Keeper
participant Validator as Validators
Upgrade->>StakingKeeper: Migrate Staking Module
StakingKeeper->>StakingKeeper: Update Bond Denomination
StakingKeeper->>Validator: Update Validator Tokens
StakingKeeper->>Validator: Update Redelegations
StakingKeeper->>Validator: Update Unbonding Delegations
Upgrade->>StakingKeeper: Validate Migration
Possibly related PRs
Poem
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
app/upgrades/v8/staking.go (1)
41-55: Refactor duplicated code in iterations over delegations.The logic for iterating and updating redelegations and unbonding delegations is similar. Consider refactoring this code into a reusable function to reduce duplication and improve maintainability.
For example, create a generic function to update delegation entries:
func updateDelegationEntries[T any](ctx sdk.Context, keeper *stakingkeeper.Keeper, iterateFunc func(sdk.Context, func(int64, T) bool) error, setFunc func(sdk.Context, T) error) error { var err error iterErr := iterateFunc(ctx, func(_ int64, delegation T) (stop bool) { // Update delegation entries... if setErr := setFunc(ctx, delegation); setErr != nil { err = setErr return true } return false }) if iterErr != nil { return iterErr } if err != nil { return err } return nil }Then use this function for both redelegations and unbonding delegations.
Also applies to: 57-72
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/upgrade_test.go(1 hunks)app/upgrades/v8/staking.go(1 hunks)app/upgrades/v8/upgrade.go(1 hunks)types/constant.go(1 hunks)types/constant_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (4)
types/constant.go (1)
113-119: Verify impact on staking module migration.Since this PR is about migrating the staking module, please verify:
- The coin swap ratio (division by 100) aligns with staking module requirements
- All staking-related coin denominations are handled correctly
- Impact on existing staked amounts and validator power
app/upgrades/v8/staking.go (1)
76-99: LGTM: Staking module validation function is correctly implemented.The
CheckStakingModulefunction effectively validates staking parameters and ensures the integrity of delegations after migration. The implementation follows best practices and enhances the robustness of the upgrade process.app/upgrade_test.go (1)
657-657: Add staking module check to module data validation.The addition of
nextversion.CheckStakingModuleensures that the staking module is validated during the upgrade tests. This step is crucial for verifying that staking data is correctly migrated and aligns with the expected state.app/upgrades/v8/upgrade.go (1)
213-215: Include staking module migration in the upgrade process.By calling
migrateStakingModulewithinmigrateModulesData, the upgrade now properly migrates staking module data. This addition ensures that all staking-related data is updated accordingly, maintaining consistency and data integrity during the upgrade.
| func TestSwapCoins(t *testing.T) { | ||
| coins := sdk.NewCoins(sdk.NewCoin(FXDenom, sdkmath.NewInt(100))) | ||
| swapCoins := SwapCoins(coins) | ||
| assert.NotEqual(t, swapCoins.String(), coins.String()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for SwapCoins function.
The current test is limited to a single coin and only verifies string representation. Consider adding:
- Test cases with multiple coins (mix of FX and non-FX denominations)
- Explicit verification of individual coin amounts
- Edge cases (empty coins, nil coins)
func TestSwapCoins(t *testing.T) {
- coins := sdk.NewCoins(sdk.NewCoin(FXDenom, sdkmath.NewInt(100)))
- swapCoins := SwapCoins(coins)
- assert.NotEqual(t, swapCoins.String(), coins.String())
+ testCases := []struct {
+ name string
+ coins sdk.Coins
+ expected sdk.Coins
+ }{
+ {
+ name: "mixed denominations",
+ coins: sdk.NewCoins(
+ sdk.NewCoin(FXDenom, sdkmath.NewInt(100)),
+ sdk.NewCoin("other", sdkmath.NewInt(100)),
+ ),
+ expected: sdk.NewCoins(
+ sdk.NewCoin(FXDenom, sdkmath.NewInt(1)),
+ sdk.NewCoin("other", sdkmath.NewInt(100)),
+ ),
+ },
+ {
+ name: "empty coins",
+ coins: sdk.Coins{},
+ expected: sdk.Coins{},
+ },
+ {
+ name: "nil coins",
+ coins: nil,
+ expected: sdk.Coins{},
+ },
+ }
+
+ for _, tc := range testCases {
+ t.Run(tc.name, func(t *testing.T) {
+ result := SwapCoins(tc.coins)
+ assert.Equal(t, tc.expected, result)
+ })
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestSwapCoins(t *testing.T) { | |
| coins := sdk.NewCoins(sdk.NewCoin(FXDenom, sdkmath.NewInt(100))) | |
| swapCoins := SwapCoins(coins) | |
| assert.NotEqual(t, swapCoins.String(), coins.String()) | |
| } | |
| func TestSwapCoins(t *testing.T) { | |
| testCases := []struct { | |
| name string | |
| coins sdk.Coins | |
| expected sdk.Coins | |
| }{ | |
| { | |
| name: "mixed denominations", | |
| coins: sdk.NewCoins( | |
| sdk.NewCoin(FXDenom, sdkmath.NewInt(100)), | |
| sdk.NewCoin("other", sdkmath.NewInt(100)), | |
| ), | |
| expected: sdk.NewCoins( | |
| sdk.NewCoin(FXDenom, sdkmath.NewInt(1)), | |
| sdk.NewCoin("other", sdkmath.NewInt(100)), | |
| ), | |
| }, | |
| { | |
| name: "empty coins", | |
| coins: sdk.Coins{}, | |
| expected: sdk.Coins{}, | |
| }, | |
| { | |
| name: "nil coins", | |
| coins: nil, | |
| expected: sdk.Coins{}, | |
| }, | |
| } | |
| for _, tc := range testCases { | |
| t.Run(tc.name, func(t *testing.T) { | |
| result := SwapCoins(tc.coins) | |
| assert.Equal(t, tc.expected, result) | |
| }) | |
| } | |
| } |
| func TestSwapCoin(t *testing.T) { | ||
| coin := sdk.NewCoin(FXDenom, sdkmath.NewInt(100)) | ||
| swapCoin := SwapCoin(coin) | ||
| assert.NotEqual(t, swapCoin.String(), coin.String()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for SwapCoin function.
The current test only verifies that the string representation changes. Consider adding:
- Explicit verification that amount is divided by 100 for FXDenom
- Test case for non-FXDenom coins (should remain unchanged)
- Edge cases (zero amount, max int amount)
func TestSwapCoin(t *testing.T) {
- coin := sdk.NewCoin(FXDenom, sdkmath.NewInt(100))
- swapCoin := SwapCoin(coin)
- assert.NotEqual(t, swapCoin.String(), coin.String())
+ testCases := []struct {
+ name string
+ coin sdk.Coin
+ expected sdk.Coin
+ }{
+ {
+ name: "FXDenom coin",
+ coin: sdk.NewCoin(FXDenom, sdkmath.NewInt(100)),
+ expected: sdk.NewCoin(FXDenom, sdkmath.NewInt(1)),
+ },
+ {
+ name: "non-FXDenom coin",
+ coin: sdk.NewCoin("other", sdkmath.NewInt(100)),
+ expected: sdk.NewCoin("other", sdkmath.NewInt(100)),
+ },
+ {
+ name: "zero amount",
+ coin: sdk.NewCoin(FXDenom, sdkmath.ZeroInt()),
+ expected: sdk.NewCoin(FXDenom, sdkmath.ZeroInt()),
+ },
+ }
+
+ for _, tc := range testCases {
+ t.Run(tc.name, func(t *testing.T) {
+ result := SwapCoin(tc.coin)
+ assert.Equal(t, tc.expected, result)
+ })
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestSwapCoin(t *testing.T) { | |
| coin := sdk.NewCoin(FXDenom, sdkmath.NewInt(100)) | |
| swapCoin := SwapCoin(coin) | |
| assert.NotEqual(t, swapCoin.String(), coin.String()) | |
| } | |
| func TestSwapCoin(t *testing.T) { | |
| testCases := []struct { | |
| name string | |
| coin sdk.Coin | |
| expected sdk.Coin | |
| }{ | |
| { | |
| name: "FXDenom coin", | |
| coin: sdk.NewCoin(FXDenom, sdkmath.NewInt(100)), | |
| expected: sdk.NewCoin(FXDenom, sdkmath.NewInt(1)), | |
| }, | |
| { | |
| name: "non-FXDenom coin", | |
| coin: sdk.NewCoin("other", sdkmath.NewInt(100)), | |
| expected: sdk.NewCoin("other", sdkmath.NewInt(100)), | |
| }, | |
| { | |
| name: "zero amount", | |
| coin: sdk.NewCoin(FXDenom, sdkmath.ZeroInt()), | |
| expected: sdk.NewCoin(FXDenom, sdkmath.ZeroInt()), | |
| }, | |
| } | |
| for _, tc := range testCases { | |
| t.Run(tc.name, func(t *testing.T) { | |
| result := SwapCoin(tc.coin) | |
| assert.Equal(t, tc.expected, result) | |
| }) | |
| } | |
| } |
| func SwapCoins(coins sdk.Coins) sdk.Coins { | ||
| swapCoins := sdk.Coins{} | ||
| for _, coin := range coins { | ||
| swapCoins.Add(SwapCoin(coin)) | ||
| } | ||
| return swapCoins | ||
| } |
There was a problem hiding this comment.
Fix potential issues in SwapCoins implementation.
The current implementation has several issues:
- Using
Addmethod modifies the receiver in-place - Missing nil input handling
- No pre-allocation for better performance
- Missing validation for empty coins
func SwapCoins(coins sdk.Coins) sdk.Coins {
+ if coins == nil {
+ return sdk.Coins{}
+ }
+ if len(coins) == 0 {
+ return sdk.Coins{}
+ }
- swapCoins := sdk.Coins{}
+ swapCoins := make(sdk.Coins, 0, len(coins))
for _, coin := range coins {
- swapCoins.Add(SwapCoin(coin))
+ swapCoins = append(swapCoins, SwapCoin(coin))
}
return swapCoins
}Additionally, consider adding documentation to explain the purpose and behavior of this function.
+// SwapCoins applies the SwapCoin transformation to each coin in the input collection.
+// For coins with FXDenom, the amount is divided by 100, while other denominations remain unchanged.
+// Returns a new Coins collection containing the transformed coins.
func SwapCoins(coins sdk.Coins) sdk.Coins {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func SwapCoins(coins sdk.Coins) sdk.Coins { | |
| swapCoins := sdk.Coins{} | |
| for _, coin := range coins { | |
| swapCoins.Add(SwapCoin(coin)) | |
| } | |
| return swapCoins | |
| } | |
| // SwapCoins applies the SwapCoin transformation to each coin in the input collection. | |
| // For coins with FXDenom, the amount is divided by 100, while other denominations remain unchanged. | |
| // Returns a new Coins collection containing the transformed coins. | |
| func SwapCoins(coins sdk.Coins) sdk.Coins { | |
| if coins == nil { | |
| return sdk.Coins{} | |
| } | |
| if len(coins) == 0 { | |
| return sdk.Coins{} | |
| } | |
| swapCoins := make(sdk.Coins, 0, len(coins)) | |
| for _, coin := range coins { | |
| swapCoins = append(swapCoins, SwapCoin(coin)) | |
| } | |
| return swapCoins | |
| } |
| iterErr := keeper.IterateRedelegations(ctx, func(_ int64, red stakingtypes.Redelegation) (stop bool) { | ||
| for i := 0; i < len(red.Entries); i++ { | ||
| red.Entries[i].InitialBalance = fxtypes.SwapAmount(red.Entries[i].InitialBalance) | ||
| } | ||
| if err = keeper.SetRedelegation(ctx, red); err != nil { | ||
| return true | ||
| } | ||
| return false | ||
| }) | ||
| if iterErr != nil { | ||
| return iterErr | ||
| } | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Avoid variable shadowing and ensure correct error handling.
In the closures for IterateRedelegations and IterateUnbondingDelegations, the variable err is assigned within the closure, which may shadow the outer err variable and lead to unintended behavior. To prevent this, declare a new variable inside the closure to capture errors, and handle them appropriately outside.
Apply this diff to fix the error handling in IterateRedelegations:
for _, validator := range validators {
// ...
}
-iterErr := keeper.IterateRedelegations(ctx, func(_ int64, red stakingtypes.Redelegation) (stop bool) {
+iterErr := keeper.IterateRedelegations(ctx, func(_ int64, red stakingtypes.Redelegation) (stop bool) {
+ var setErr error
for i := 0; i < len(red.Entries); i++ {
red.Entries[i].InitialBalance = fxtypes.SwapAmount(red.Entries[i].InitialBalance)
}
- if err = keeper.SetRedelegation(ctx, red); err != nil {
+ if setErr = keeper.SetRedelegation(ctx, red); setErr != nil {
+ err = setErr
return true
}
return false
})
if iterErr != nil {
return iterErr
}
-if err != nil {
+if err != nil {
return err
}Similarly, fix the error handling in IterateUnbondingDelegations:
iterErr = keeper.IterateUnbondingDelegations(ctx, func(index int64, ubd stakingtypes.UnbondingDelegation) (stop bool) {
+ var setErr error
for i := 0; i < len(ubd.Entries); i++ {
ubd.Entries[i].Balance = fxtypes.SwapAmount(ubd.Entries[i].Balance)
ubd.Entries[i].InitialBalance = fxtypes.SwapAmount(ubd.Entries[i].InitialBalance)
}
- if err = keeper.SetUnbondingDelegation(ctx, ubd); err != nil {
+ if setErr = keeper.SetUnbondingDelegation(ctx, ubd); setErr != nil {
+ err = setErr
return true
}
return false
})
if iterErr != nil {
return iterErr
}
-if err != nil {
+if err != nil {
return err
}Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
Tests
Improvements