refactor: migrate feegrant module #936
Conversation
WalkthroughThis pull request introduces a comprehensive migration mechanism for fee grant allowances in a Cosmos SDK application. The changes include a new Changes
Sequence DiagramsequenceDiagram
participant Upgrade as Upgrade Handler
participant Keeper as Fee Grant Keeper
participant Store as KV Store
Upgrade->>Keeper: Retrieve existing fee allowances
loop For each allowance
Keeper->>Keeper: Swap allowance type
Keeper->>Store: Update allowance
end
Upgrade-->>Keeper: Return migration result
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: 3
🧹 Nitpick comments (3)
app/upgrades/v8/feegrant.go (2)
54-54: Improve error message inswapAllowancedefault caseThe error message in the default case of
swapAllowancedoes not specify the type of allowance that failed to swap. Including the allowance type in the error message would aid in debugging.Apply this diff to enhance the error message:
-return nil, sdkerrors.ErrPackAny.Wrap("failed to swap allowance") +return nil, sdkerrors.ErrPackAny.Wrapf("failed to swap allowance for type %T", allowanceAny.GetCachedValue())
30-34: Simplify final return statement inMigrateFeegrantAfter checking for errors, the final
return errin theMigrateFeegrantfunction always returnsnil, since any non-nilerrwould have been returned earlier. Simplify the code by returningnildirectly.Apply this diff to simplify the return statement:
if iterErr != nil { return iterErr } if err != nil { return err } - return err + return nil }app/upgrades/v8/feegrant_test.go (1)
75-79: Enhance test assertions to verify correct migrationCurrently, the test asserts that the migrated allowances are not equal to the original allowances using
require.NotEqual. However, this does not confirm that the migration has correctly transformed the allowances as intended. Consider enhancing the test by comparing the migrated allowances to the expected values after migration to ensure the migration logic works correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/upgrades/v8/feegrant.go(1 hunks)app/upgrades/v8/feegrant_test.go(1 hunks)app/upgrades/v8/upgrade.go(8 hunks)testutil/helpers/app.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
app/upgrades/v8/feegrant.go
[warning] 36-36: redefines-builtin-id: redefinition of the built-in type any
(revive)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (4)
app/upgrades/v8/upgrade.go (2)
Line range hint
52-70: Integration ofcodecparameter into upgrade handler functionsThe addition of the
codecparameter to theCreateUpgradeHandlerand its correct propagation toupgradeTestnet,upgradeMainnet, andmigrateModulesDatafunctions facilitates proper handling of codec operations during the upgrade process.
Line range hint
212-227: Proper integration of fee grant migrationThe inclusion of the
MigrateFeegrantfunction call withinmigrateModulesData, utilizing thecodec,storeService, andapp.AccountKeeper, is correctly implemented. This ensures fee grant allowances are appropriately migrated during the upgrade.testutil/helpers/app.go (2)
4-8: LGTM! Imports are properly organized.The new imports are necessary for the function implementation and follow standard Go conventions.
41-49: Verify the helper function dependencies.The function relies on
generateGenesisValidatorandsetupWithGenesisValSetwhich are not visible in the provided code. Let's verify their existence and implementation:Consider adding documentation for test setup.
Add a doc comment explaining:
- The purpose of this test helper
- The expected range for
valNumber- Whether the returned context is deterministic
- Example usage in tests
+// NewAppWithValNumber creates a new app instance with the specified number of validators +// for testing purposes. It returns the app instance and a context with the proposer set. +// This is a test helper function and should not be used in production code. +// Parameters: +// - t: testing context +// - valNumber: number of validators to generate (must be > 0) +// Returns: +// - *app.App: initialized application instance +// - sdk.Context: context with proposer set from validator set func NewAppWithValNumber(t *testing.T, valNumber int) (*app.App, sdk.Context) {✅ Verification successful
Helper functions are properly implemented and located
The helper functionsgenerateGenesisValidatorandsetupWithGenesisValSetare correctly implemented intestutil/helpers/helpers.go. They follow test helper best practices by usingt.Helper()and are appropriately located in the test utilities package. The original documentation suggestion remains valid but is not a critical concern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the helper function implementations rg -A 5 "func (generateGenesisValidator|setupWithGenesisValSet)" # Verify these functions are only used in test files rg -l "(generateGenesisValidator|setupWithGenesisValSet)" | grep "_test.go$"Length of output: 1072
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/upgrades/v8/feegrant.go (1)
75-75:⚠️ Potential issueFix incorrect return statement.
The
kvStore.Setmethod does not return an error, so returning it directly is incorrect.Apply this diff to fix the issue:
- return kvStore.Set(key, bz) + kvStore.Set(key, bz) + return nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/upgrades/v8/feegrant.go(1 hunks)app/upgrades/v8/feegrant_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/upgrades/v8/feegrant_test.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (2)
app/upgrades/v8/feegrant.go (2)
1-14: LGTM!The imports are well-organized and all appear to be necessary for the implementation.
36-56: LGTM!The function implementation is solid:
- Properly handles all allowance types
- Has appropriate error handling
- Parameter naming issue has been fixed
Summary by CodeRabbit
New Features
Tests
Refactor