fix: verify delegation and reward data after upgrade #943
Conversation
WalkthroughThis pull request introduces comprehensive changes to the upgrade testing and migration processes across multiple files. The modifications primarily focus on enhancing the upgrade test framework in Changes
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: 1
🧹 Nitpick comments (6)
app/upgrades/v8/distribution.go (2)
17-17: Update Function Documentation to Reflect New ParameterThe
migrateDistributionfunction now includes an additional parameterstakingKeeper *fxstakingkeeper.Keeper. Please update the function's documentation to reflect this change, providing a clear explanation of the purpose and usage of the new parameter.
71-71: Add a Check for Zero Stake ValuesAfter calculating
info.Stakewithvalidator.TokensFromSharesTruncated, consider adding a check to handle cases where the stake might be zero. This can prevent potential issues in downstream processes that expect a positive stake value.Optionally, you could add:
if info.Stake.IsZero() { // Handle zero stake scenario if necessary }types/constant.go (1)
122-138: Add Documentation for theSwapDecCoinsFunctionConsider adding a comment to the newly introduced
SwapDecCoinsfunction to explain its purpose and usage. This improves code readability and helps other developers understand how and when to use this function.Example:
// SwapDecCoins swaps the denomination of all DecCoins from LegacyFXDenom to DefaultDenom // and adjusts their amounts using SwapDecAmount. It returns the resulting DecCoins. func SwapDecCoins(decCoins sdk.DecCoins) sdk.DecCoins { // Function implementation... }app/upgrade_test.go (3)
25-28: Organize Imports for ClarityFor better readability, consider grouping standard library imports, third-party imports, and project-specific imports separately. This makes the import section cleaner and easier to navigate.
180-207: Handle Errors Appropriately indelegatesAndRewardsFunctionIn the
delegatesAndRewardsfunction, errors within the iteration are being checked usingrequire.NoError(t, err), which will fail the test immediately on error but won't stop the iteration. Consider modifying the error handling to capture the error and stop the iteration to prevent unnecessary continued processing after a failure.Example:
err := myApp.StakingKeeper.IterateAllDelegations(ctx, func(del stakingtypes.Delegation) (stop bool) { // ... if err != nil { return true // Stop iteration on error } // ... return false }) if err != nil { require.NoError(t, err) }
253-253: Clarify Why Assertion Is Commented OutAt line 253~, the assertion comparing rewards before and after the upgrade is commented out:
// assert.EqualValues(t, beforeRewards.String(), afterRewards.String(), delegateKey)Please either uncomment this assertion if it is necessary for the test, or add a comment explaining why it is being skipped. This ensures that the test code remains clear and maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/upgrade_test.go(6 hunks)app/upgrades/v8/distribution.go(6 hunks)app/upgrades/v8/upgrade.go(1 hunks)go.mod(1 hunks)types/constant.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (6)
app/upgrades/v8/distribution.go (2)
27-27: Verify Consistent Use of Staking Keeper in Function CallsThe
migrateDistributionfunction now passesstakingKeepertomigrateDelegatorStartingInfos. Ensure that all calls tomigrateDistributionand related functions consistently use the correctstakingKeeperinstance to prevent potential issues.
39-46: Correctly Update Denominations in Historical RewardsThe changes within
migrateValidatorHistoricalRewardsappropriately update the denominations fromLegacyFXDenomtoDefaultDenom. The logic ensures that all rewards are correctly mapped to the new denomination.app/upgrade_test.go (2)
58-58: UpdatebeforeUpgradeFunction SignatureThe
beforeUpgradefunction now acceptst *testing.Tas a parameter, which allows for the use of testing utilities within the function. This is a good practice for helper functions in tests.
161-167: Include Delegation Data inBeforeUpgradeDataStructAdding
DelegatesandDelegateRewardsto theBeforeUpgradeDatastruct is beneficial for capturing and comparing delegation state before and after the upgrade. This enhances the comprehensiveness of the upgrade tests.app/upgrades/v8/upgrade.go (1)
231-231: LGTM! Adding StakingKeeper enhances delegation data verification.The addition of
StakingKeepertomigrateDistributionaligns with the PR's objective of fixing delegation and reward data verification during upgrades.Run the following script to verify the migration logic:
✅ Verification successful
Verification confirms proper keeper usage in migration
The implementation in
app/upgrades/v8/distribution.goshows correct usage of both keepers, withStakingKeeperspecifically utilized for delegator data migration viamigrateDelegatorStartingInfos, whileDistrKeeperhandles all reward-related migrations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of migrateDistribution function. # Test: Search for the function implementation to ensure it properly uses both keepers. ast-grep --pattern $'func migrateDistribution(ctx sdk.Context, stakingKeeper *stakingkeeper.Keeper, distrKeeper *distributionkeeper.Keeper) error { $$$ }'Length of output: 157
Script:
#!/bin/bash # Search with a more generic pattern ast-grep --pattern 'func migrateDistribution($$$) error' # Backup with ripgrep echo "=== Function definition ===" rg "func migrateDistribution" -A 10 echo "=== Function usage ===" rg "migrateDistribution\(" -A 2Length of output: 2882
go.mod (1)
76-76: LGTM! Zerolog is now a direct dependency.The removal of the
// indirectcomment correctly reflects thatzerologis now directly used in the codebase, supporting enhanced logging during upgrade testing.
| func migrateDelegatorStartingInfos(ctx sdk.Context, stakingKeeper *fxstakingkeeper.Keeper, distrKeeper distributionkeeper.Keeper) error { | ||
| var err error | ||
| validatorMap := make(map[string]stakingtypes.Validator) | ||
| distrKeeper.IterateDelegatorStartingInfos(ctx, func(valAddr sdk.ValAddress, addr sdk.AccAddress, info types.DelegatorStartingInfo) bool { | ||
| info.Stake = fxtypes.SwapDecAmount(info.Stake) | ||
| var delegation stakingtypes.Delegation | ||
| delegation, err = stakingKeeper.GetDelegation(ctx, addr, valAddr) | ||
| if err != nil { | ||
| return true | ||
| } | ||
| validator, ok := validatorMap[delegation.ValidatorAddress] | ||
| if !ok { | ||
| validator, err = stakingKeeper.GetValidator(ctx, valAddr) | ||
| if err != nil { | ||
| return true | ||
| } | ||
| validatorMap[delegation.ValidatorAddress] = validator | ||
| } | ||
| info.Stake = validator.TokensFromSharesTruncated(delegation.Shares) |
There was a problem hiding this comment.
Ensure Proper Error Handling and Propagation
In the migrateDelegatorStartingInfos function, errors within the iterator callback are not being captured in the err variable. Specifically:
- At line 60~, if
stakingKeeper.GetDelegationreturns an error, the error is not assigned toerr. - At line 66~, if
stakingKeeper.GetValidatorreturns an error, the error is not assigned toerr.
As a result, the function may return nil even when an error has occurred during iteration.
Apply this diff to capture and propagate errors correctly:
delegation, err = stakingKeeper.GetDelegation(ctx, addr, valAddr)
if err != nil {
+ err = fmt.Errorf("failed to get delegation for delegator %s and validator %s: %w", addr.String(), valAddr.String(), err)
return true
} validator, err = stakingKeeper.GetValidator(ctx, valAddr)
if err != nil {
+ err = fmt.Errorf("failed to get validator %s: %w", valAddr.String(), err)
return true
}This ensures that any errors encountered are properly returned and can be handled upstream.
📝 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 migrateDelegatorStartingInfos(ctx sdk.Context, stakingKeeper *fxstakingkeeper.Keeper, distrKeeper distributionkeeper.Keeper) error { | |
| var err error | |
| validatorMap := make(map[string]stakingtypes.Validator) | |
| distrKeeper.IterateDelegatorStartingInfos(ctx, func(valAddr sdk.ValAddress, addr sdk.AccAddress, info types.DelegatorStartingInfo) bool { | |
| info.Stake = fxtypes.SwapDecAmount(info.Stake) | |
| var delegation stakingtypes.Delegation | |
| delegation, err = stakingKeeper.GetDelegation(ctx, addr, valAddr) | |
| if err != nil { | |
| return true | |
| } | |
| validator, ok := validatorMap[delegation.ValidatorAddress] | |
| if !ok { | |
| validator, err = stakingKeeper.GetValidator(ctx, valAddr) | |
| if err != nil { | |
| return true | |
| } | |
| validatorMap[delegation.ValidatorAddress] = validator | |
| } | |
| info.Stake = validator.TokensFromSharesTruncated(delegation.Shares) | |
| func migrateDelegatorStartingInfos(ctx sdk.Context, stakingKeeper *fxstakingkeeper.Keeper, distrKeeper distributionkeeper.Keeper) error { | |
| var err error | |
| validatorMap := make(map[string]stakingtypes.Validator) | |
| distrKeeper.IterateDelegatorStartingInfos(ctx, func(valAddr sdk.ValAddress, addr sdk.AccAddress, info types.DelegatorStartingInfo) bool { | |
| var delegation stakingtypes.Delegation | |
| delegation, err = stakingKeeper.GetDelegation(ctx, addr, valAddr) | |
| if err != nil { | |
| err = fmt.Errorf("failed to get delegation for delegator %s and validator %s: %w", addr.String(), valAddr.String(), err) | |
| return true | |
| } | |
| validator, ok := validatorMap[delegation.ValidatorAddress] | |
| if !ok { | |
| validator, err = stakingKeeper.GetValidator(ctx, valAddr) | |
| if err != nil { | |
| err = fmt.Errorf("failed to get validator %s: %w", valAddr.String(), err) | |
| return true | |
| } | |
| validatorMap[delegation.ValidatorAddress] = validator | |
| } | |
| info.Stake = validator.TokensFromSharesTruncated(delegation.Shares) |
Summary by CodeRabbit
Release Notes
New Features
Improvements
Dependency Updates
These changes improve the system's upgrade process, testing capabilities, and dependency management.