refactor(x/upgrade): migrate to appmodule.VersionMap #20485
Conversation
WalkthroughWalkthroughThe updates involve replacing references to Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (8)
- simapp/app_test.go (2 hunks)
- simapp/upgrades.go (2 hunks)
- types/module/module.go (3 hunks)
- x/upgrade/keeper/abci_test.go (7 hunks)
- x/upgrade/keeper/grpc_query_test.go (4 hunks)
- x/upgrade/keeper/keeper.go (6 hunks)
- x/upgrade/keeper/keeper_test.go (9 hunks)
- x/upgrade/types/handler.go (2 hunks)
Additional Context Used
Path-based Instructions (8)
x/upgrade/types/handler.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/upgrades.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/upgrade/keeper/grpc_query_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"simapp/app_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/upgrade/keeper/keeper_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/upgrade/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/upgrade/keeper/abci_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"types/module/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (23)
x/upgrade/types/handler.go (2)
6-6: The import change frommodule.VersionMaptoappmodule.VersionMapaligns with the PR's objective to refactor thex/upgrademodule for better compatibility and reduced dependencies.
27-27: The function signature change forUpgradeHandlerto useappmodule.VersionMapis consistent with the overall refactoring strategy. Ensure that all usages of this handler across the project are updated to pass the correct type.Verification successful
The search results indicate that the new signature for
UpgradeHandleris being used in all instances found. Here are the specific lines whereUpgradeHandleris used:
x/upgrade/types/handler.go: Line 27x/upgrade/keeper/keeper.go: Lines 27, 28, 30x/upgrade/keeper/keeper_test.go: Lines 27, 28, 29, 30, 31, 32, 33x/upgrade/keeper/grpc_query_test.go: Line 27x/upgrade/keeper/abci_test.go: Lines 27, 28, 29, 30, 31, 32, 33types/module/module.go: Lines 27, 28, 29simapp/upgrades.go: Line 27simapp/app_di.go: Lines 27, 28simapp/app.go: Lines 27, 28baseapp/params_legacy.go: Line 27All these instances appear to use the new signature with
appmodule.VersionMap.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `UpgradeHandler` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go $'UpgradeHandler'Length of output: 4010
simapp/upgrades.go (2)
6-6: The import ofappmoduleis necessary for the updatedRegisterUpgradeHandlersfunction to useappmodule.VersionMap. This change is well-aligned with the PR's goals.
27-27: The lambda function passed toSetUpgradeHandlernow correctly usesappmodule.VersionMap. This is a crucial update ensuring that the upgrade logic is compatible with the new module structure.x/upgrade/keeper/grpc_query_test.go (3)
10-10: The addition of theappmoduleimport is necessary for the tests to utilize the updatedVersionMap. This change is consistent with the modifications in the main codebase.
51-51: The test setup now correctly initializesModuleVersionMapwithappmodule.VersionMap. This ensures that the test environment reflects the changes made in the main application logic.
139-139: The test forSetUpgradeHandlerwithinTestAppliedCurrentPlannow usesappmodule.VersionMap, aligning with the new type requirements. This is a critical update for maintaining the integrity of the test suite.simapp/app_test.go (1)
199-199: The test functionsTestRunMigrationsandTestInitGenesisOnMigrationnow correctly useappmodule.VersionMapinstead ofmodule.VersionMap. This change is necessary to ensure that the tests are aligned with the new module structure and are testing the correct behavior.Also applies to: 250-250
x/upgrade/keeper/keeper_test.go (2)
12-12: The import ofappmodulein the test file is correctly updated to reflect the changes in the main codebase. This ensures that the test suite uses the correctVersionMaptype.
170-170: The test functions inkeeper_test.gothat involveSetUpgradeHandlerandApplyUpgradenow correctly useappmodule.VersionMap. This is crucial for ensuring that the tests accurately reflect the behavior of the upgraded system and are compatible with the new module structure.Also applies to: 292-294, 306-312, 339-339, 356-356, 381-381, 397-397
x/upgrade/keeper/keeper.go (5)
41-41: TheinitVersionMapfield has been updated to useappmodule.VersionMap. This change aligns with the PR's objective to refactor thex/upgrademodule for better compatibility and reduced dependencies.
77-77: The methodSetInitVersionMapnow correctly usesappmodule.VersionMapas per the refactoring goal. This ensures that the initial version map is set with the correct type.
83-83: The methodGetInitVersionMaphas been updated to returnappmodule.VersionMap. This change is consistent with the overall refactoring effort and ensures that the getter returns the updated type.
95-95: The methodSetModuleVersionMapnow accepts and processesappmodule.VersionMap. This is a key part of the refactoring, ensuring that module version maps are handled using the new type. The implementation here is robust, with sorting of module names for deterministic iteration, which is good practice.
123-123: The methodGetModuleVersionMaphas been updated to returnappmodule.VersionMap. The method correctly handles the iteration over the store and constructs the version map using the new type. This is crucial for maintaining consistency in how version maps are retrieved and used across the system.Also applies to: 132-132
x/upgrade/keeper/abci_test.go (6)
54-54: Change toappmodule.VersionMapinSetUpgradeHandleraligns with the PR's objectives.
72-72: Consistent use ofappmodule.VersionMapinSetUpgradeHandler.
177-177: UpdatedSetUpgradeHandlerto useappmodule.VersionMapcorrectly.
423-423: Proper update toSetUpgradeHandlerusingappmodule.VersionMap.
474-474: Correct implementation ofappmodule.VersionMapinSetUpgradeHandler.
489-489: Consistent update toSetUpgradeHandlerusingappmodule.VersionMap.types/module/module.go (2)
660-660: UpdatedRunMigrationsto useappmodule.VersionMapaligns with the refactor objectives.Also applies to: 671-671
827-828:GetVersionMapcorrectly updated to returnappmodule.VersionMap.
| @@ -24,7 +24,7 @@ const UpgradeName = "v050-to-v051" | |||
| func (app SimApp) RegisterUpgradeHandlers() { | |||
| app.UpgradeKeeper.SetUpgradeHandler( | |||
| UpgradeName, | |||
| func(ctx context.Context, _ upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) { | |||
| func(ctx context.Context, _ upgradetypes.Plan, fromVM appmodule.VersionMap) (appmodule.VersionMap, error) { | |||
There was a problem hiding this comment.
Can we add a changelog under simapp/CHANGELOD.md, as this wiring changes needs to happen for users as well
There was a problem hiding this comment.
Should I also make a CHANGELOG entry in x/upgrade? It's not breaking except for usages of the function delegate UpgradeHandler. In other words module.VersionMap is interchangeable with appmodule.VersionMap but function type with each of those as parameters is not.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
simapp/CHANGELOG.md (2)
36-36: Clarify the changelog entry for better understanding.Consider rephrasing the changelog entry to enhance clarity and precision. Here's a suggested revision:
- [#20485](https://github.com/cosmos/cosmos-sdk/pull/20485) Updated the `x/upgrade/types.UpgradeHandler` signature to accept `appmodule.VersionMap` instead of `module.VersionMap`. Adjust your usage of `UpgradeKeeper.SetUpgradeHandler` accordingly.
Line range hint
52-52: Remove extra blank line to improve formatting.<!-- TODO: move changelog.md elements to here --> - ## v0.47 to v0.50
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- simapp/CHANGELOG.md (1 hunks)
Additional context used
Path-based instructions (1)
simapp/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
simapp/CHANGELOG.md
[uncategorized] ~28-~28: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...application. This changelog is aimed to help developers understand the wiring change...
Markdownlint
simapp/CHANGELOG.md
52-52: Expected: 1; Actual: 2
Multiple consecutive blank lines
Additional comments not posted (1)
simapp/CHANGELOG.md (1)
Line range hint
28-28: The verb form is correct in this context.LanguageTool flagged this as a potential issue, but the usage of "aimed" is appropriate here, indicating the purpose of the changelog.
* main: feat(server/v2): introduce cometbft v2 (#20483) refactor(x/upgrade): migrate to appmodule.VersionMap (#20485) docs: code guidelines changes (#20482) feat(cosmovisor): load cosmovisor configuration from toml file (#19995) perf(math): Significantly speedup Dec quo truncate and quo Roundup (#20034) fix: Bump CometBFT versions (#20481)
* main: (120 commits) chore: update protoc-gen-swagger to protoc-gen-openapiv2 (#20448) ci: Add GitHub Action for go mod tidy and mocks (#20501) chore: Address linter issues (#20486) fix: wrap errors in auto CLI service registration (#20493) chore: fix comment (#20498) chore: fix the note box syntax error (#20499) feat(server/v2): introduce cometbft v2 (#20483) refactor(x/upgrade): migrate to appmodule.VersionMap (#20485) docs: code guidelines changes (#20482) feat(cosmovisor): load cosmovisor configuration from toml file (#19995) perf(math): Significantly speedup Dec quo truncate and quo Roundup (#20034) fix: Bump CometBFT versions (#20481) refactor(core): remove redundant ExecMode (#20322) feat(store/v2): pruning manager (#20430) chore: force reload sonar cloud (#20480) refactor(x/accounts): reuse calculated sum in `func safeAdd` (#20458) refactor: remove `defer` in loop (#20223) ci: remove livness test (#20474) build(deps): Bump bufbuild/buf-setup-action from 1.32.1 to 1.32.2 (#20477) chore: migrate a few diagrams to mermaid (#20473) ...
Description
Part of: #20412
Refactor x/upgrade to use
core/appmoduletypes from the aliased SDK type. Required for side-by-side compatibility with runtime/v2 providers and simapp/v2. Also removes a dependency on the SDK from x/upgrade.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Refactor
appmodule.VersionMapinstead ofmodule.VersionMapfor improved module structuring and clarity.Documentation
CHANGELOG.mdto reflect changes in function signatures and type usage related toappmodule.VersionMap.