refactor(gov): migrate gov module to v8 #735
Conversation
WalkthroughThe pull request introduces several enhancements to the testing and governance functionalities within the application. Key modifications include the addition of new functions for checking governance parameters and simplifying key deletion logic in the upgrade tests. Additionally, new functions are implemented in the upgrade handler to manage governance parameters during upgrades, while deprecated key management is introduced in the migrations. The changes also involve the removal of outdated variables from the governance types. Changes
Possibly related PRs
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: 3
🧹 Outside diff range and nitpick comments (6)
x/staking/migrations/v8/migrate.go (3)
Line range hint
8-14: Consider removing deprecated variables and updatingGetRemovedStoreKeysThe variables
ValidatorOperatorKey,ConsensusPubKey, andConsensusProcessKeyare marked as deprecated and scheduled for removal in v8. However, they are still being used in theGetRemovedStoreKeysfunction.Consider removing these variables entirely and updating the
GetRemovedStoreKeysfunction to return the byte slices directly. This would look like:func GetRemovedStoreKeys() [][]byte { return [][]byte{ {0x91}, // ValidatorOperatorKey {0x92}, // ConsensusPubKey {0x93}, // ConsensusProcessKey } }This change would complete the deprecation process and remove the potential for these variables to be used elsewhere in the codebase.
17-20: Approve function renaming and suggest documentation improvementThe renaming of
GetRemovedValidatorStoreKeystoGetRemovedStoreKeysis appropriate, especially if these keys are not specific to validators.Consider adding a brief comment to explain the purpose of this function and the nature of the keys it returns. For example:
// GetRemovedStoreKeys returns a list of deprecated store keys that should be removed during migration. func GetRemovedStoreKeys() [][]byte { // ... (existing implementation) }This would improve the function's documentation and make its purpose clearer to other developers.
Line range hint
22-33: Approve function update and suggest error handling improvementThe update to use the renamed
GetRemovedStoreKeysfunction is correct, and the overall logic for deleting keys is sound and efficient.Consider adding error handling for the iterator creation and closing. Here's a suggested improvement:
func DeleteMigrationValidatorStore( ctx sdk.Context, storeKey storetypes.StoreKey, ) error { store := ctx.KVStore(storeKey) removeKeys := GetRemovedStoreKeys() for _, key := range removeKeys { iterator := storetypes.KVStorePrefixIterator(store, key) defer iterator.Close() for ; iterator.Valid(); iterator.Next() { store.Delete(iterator.Key()) } if err := iterator.Error(); err != nil { return fmt.Errorf("error iterating over keys: %w", err) } } return nil }This change adds proper cleanup with
defer iterator.Close()and checks for any errors that might have occurred during iteration.app/upgrade_test.go (2)
102-109: LGTM: Comprehensive governance parameter verification added.The
checkGovCustomParamsfunction effectively verifies the custom parameters for theMsgCommunityPoolSpendmessage type and checks for the deletion of removed store keys. This addition ensures that governance-related changes are correctly applied during the upgrade.Consider adding a comment explaining the purpose of the
EGFCustomParamDepositRatioand other default values for better code readability.
Line range hint
115-123: Approve with suggestion: Generic key deletion check function added.The
checkKeysIsDeletefunction is a valuable addition that improves code reusability by providing a generic way to check for key deletions. However, there's a potential resource management issue:The
defer iterator.Close()statement is inside the loop, which may lead to resource leaks if multiple iterations occur. Consider moving the iterator creation and closure outside the loop:func checkKeysIsDelete(t *testing.T, kvStore storetypes.KVStore, keys [][]byte) { require.Greater(t, len(keys), 0) for _, removeKey := range keys { iterator := storetypes.KVStorePrefixIterator(kvStore, removeKey) hasKey := false for ; iterator.Valid(); iterator.Next() { hasKey = true break } iterator.Close() require.False(t, hasKey, "key is not deleted, prefix:%x", removeKey) } }This change ensures that each iterator is properly closed after its use, preventing potential resource leaks.
app/upgrades/v8/upgrade.go (1)
42-46: Consistent Comment FormattingFor improved readability, consider adding a space after the numbering in the comments.
Apply this diff to adjust the comments:
// 1. delete fxParams key +// 1. Delete fxParams key // 2. init custom params +// 2. Init custom params
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- app/upgrade_test.go (3 hunks)
- app/upgrades/v8/upgrade.go (2 hunks)
- x/gov/migrations/v8/migrate.go (1 hunks)
- x/gov/types/params.go (0 hunks)
- x/staking/migrations/v8/migrate.go (2 hunks)
💤 Files with no reviewable changes (1)
- x/gov/types/params.go
🧰 Additional context used
🔇 Additional comments (7)
x/gov/migrations/v8/migrate.go (2)
1-7: LGTM: Package declaration and imports are appropriate.The package name
v8aligns with the migration version, and the imports are relevant to the functionality provided in this file.
15-17: LGTM: GetRemovedStoreKeys function is correct and concise.The function correctly returns a slice containing the deprecated key prefixes. Its simplicity makes it easy to understand and maintain.
app/upgrade_test.go (3)
17-18: LGTM: New imports added for enhanced functionality.The new import statements for distribution, governance, and custom types are appropriate for the changes made in this file. They provide the necessary types and functions for the new governance and upgrade checks.
Also applies to: 27-28
98-100: LGTM: Enhanced upgrade verification with governance parameter check.The addition of
checkGovCustomParamsto thecheckAppUpgradefunction improves the upgrade verification process by including checks for governance custom parameters. This ensures that the governance module is correctly configured after the upgrade.
112-113: LGTM: Improved code reusability in staking migration check.The refactoring of
checkStakingMigrationDeleteto use the newcheckKeysIsDeletefunction improves code reusability and maintainability. This change simplifies the staking migration check while keeping the functionality intact.app/upgrades/v8/upgrade.go (2)
6-15: Imports are Correct and NecessaryThe added imports for
storetypes,govtypes, and the keeper packages are appropriate and necessary for the new functionality introduced in the code.
31-34: Proper Placement ofmigrationGovCustomParamCallIntegrating
migrationGovCustomParamafter the validator migrations and before committing the context ensures that governance parameters are correctly handled during the upgrade process.
| var ( | ||
| // Deprecated: do not use, remove in v8 | ||
| FxBaseParamsKeyPrefix = []byte("0x90") | ||
| // Deprecated: do not use, remove in v8 | ||
| FxEGFParamsKey = []byte("0x91") | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor deprecated key declarations and clarify deprecation timeline.
- Consider using
constinstead ofvarfor these values, as they appear to be constants. - The deprecation comments state "remove in v8", but this file is already in the v8 package. Please clarify the deprecation timeline.
- Add comments explaining the significance of the byte values
0x90and0x91.
Here's a suggested refactoring:
const (
// FxBaseParamsKeyPrefix is the deprecated key prefix for base parameters.
// It will be removed in the next major version.
// The value 0x90 represents [explain significance].
FxBaseParamsKeyPrefix = []byte{0x90}
// FxEGFParamsKey is the deprecated key for EGF parameters.
// It will be removed in the next major version.
// The value 0x91 represents [explain significance].
FxEGFParamsKey = []byte{0x91}
)| func DeleteOldParamsStore( | ||
| ctx sdk.Context, | ||
| storeKey storetypes.StoreKey, | ||
| ) { | ||
| store := ctx.KVStore(storeKey) | ||
| removeKeys := GetRemovedStoreKeys() | ||
| for _, key := range removeKeys { | ||
| iterator := storetypes.KVStorePrefixIterator(store, key) | ||
| for ; iterator.Valid(); iterator.Next() { | ||
| store.Delete(iterator.Key()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Close iterator and consider minor optimization in DeleteOldParamsStore.
The function correctly deletes deprecated entries, but there are two points to address:
- The iterator is not being closed, which could lead to resource leaks.
- A minor optimization can be made by moving the store retrieval inside the loop.
Here's a suggested refactoring:
func DeleteOldParamsStore(
ctx sdk.Context,
storeKey storetypes.StoreKey,
) {
removeKeys := GetRemovedStoreKeys()
for _, key := range removeKeys {
store := ctx.KVStore(storeKey)
iterator := storetypes.KVStorePrefixIterator(store, key)
defer iterator.Close()
for ; iterator.Valid(); iterator.Next() {
store.Delete(iterator.Key())
}
}
}This change ensures that the iterator is properly closed and slightly optimizes the function by moving the store retrieval inside the loop, ensuring we always work with the most up-to-date store state.
| func migrationGovCustomParam(ctx sdk.Context, keeper *keeper.Keeper, storeKey *storetypes.KVStoreKey) error { | ||
| // 1. delete fxParams key | ||
| fxgovv8.DeleteOldParamsStore(ctx, storeKey) | ||
|
|
||
| // 2. init custom params | ||
| return keeper.InitCustomParams(ctx) | ||
| } |
There was a problem hiding this comment.
Handle Potential Errors from DeleteOldParamsStore Function
The fxgovv8.DeleteOldParamsStore(ctx, storeKey) function might return an error that is currently not being handled. To enhance robustness, consider checking and handling any potential errors from this function.
Apply this diff to handle the error:
func migrationGovCustomParam(ctx sdk.Context, keeper *keeper.Keeper, storeKey *storetypes.KVStoreKey) error {
// 1. delete fxParams key
- fxgovv8.DeleteOldParamsStore(ctx, storeKey)
+ if err := fxgovv8.DeleteOldParamsStore(ctx, storeKey); err != nil {
+ return err
+ }
// 2. init custom params
return keeper.InitCustomParams(ctx)
}📝 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 migrationGovCustomParam(ctx sdk.Context, keeper *keeper.Keeper, storeKey *storetypes.KVStoreKey) error { | |
| // 1. delete fxParams key | |
| fxgovv8.DeleteOldParamsStore(ctx, storeKey) | |
| // 2. init custom params | |
| return keeper.InitCustomParams(ctx) | |
| } | |
| func migrationGovCustomParam(ctx sdk.Context, keeper *keeper.Keeper, storeKey *storetypes.KVStoreKey) error { | |
| // 1. delete fxParams key | |
| if err := fxgovv8.DeleteOldParamsStore(ctx, storeKey); err != nil { | |
| return err | |
| } | |
| // 2. init custom params | |
| return keeper.InitCustomParams(ctx) | |
| } |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores