[CORE-538] Add 4.0.0 upgrade handler for Cosmos 0.47 -> 0.50 upgrade handler. #918
Conversation
|
Warning Rate Limit Exceeded@lcwik has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 54 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe changes involve a protocol application upgrade from version 3.0.0 to 4.0.0. This upgrade includes adding new imports for IBC client and connection types, cleaning up code, updating upgrade handlers and constants, and adjusting testing scripts and containers to accommodate the new version. There is also a focus on migrating messages related to client updates and upgrades, as well as registering key tables for legacy parameter subspaces. 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's AI:
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 (
|
lcwik
changed the title
lcwik
changed the title
lcwik
changed the title
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- protocol/go.mod
- protocol/testing/containertest/preupgrade_genesis.json
Files selected for processing (9)
- protocol/app/app.go (3 hunks)
- protocol/app/msgs/normal_msgs.go (1 hunks)
- protocol/app/upgrades.go (1 hunks)
- protocol/app/upgrades/v4.0.0/constants.go (1 hunks)
- protocol/app/upgrades/v4.0.0/upgrade.go (1 hunks)
- protocol/testing/containertest/Dockerfile (1 hunks)
- protocol/testing/containertest/containertest.sh (3 hunks)
- protocol/testing/containertest/testnet.go (1 hunks)
- protocol/testing/containertest/testnet_test.go (1 hunks)
Files skipped from review due to trivial changes (3)
- protocol/app/msgs/normal_msgs.go
- protocol/testing/containertest/Dockerfile
- protocol/testing/containertest/testnet_test.go
Additional comments: 9
protocol/app/upgrades/v4.0.0/upgrade.go (1)
- 12-22: The
CreateUpgradeHandlerfunction correctly sets up an upgrade handler that logs the upgrade name and runs migrations. Ensure that the logging format and migration process are consistent with the project's standards.protocol/app/upgrades/v4.0.0/constants.go (1)
- 11-28: The
Upgradevariable is correctly defined with the newUpgradeNameandStoreUpgrades. Verify that the added module names inStoreUpgradesmatch the expected modules for the 0.47 to 0.50 upgrade.protocol/app/upgrades.go (1)
- 9-34: The
Upgradesslice andsetupUpgradeHandlersfunction have been correctly updated to include and handle thev4_0_0upgrade. Ensure that there are no duplicate handlers for the same upgrade name.protocol/testing/containertest/testnet.go (1)
- 28-28: The
UpgradeToVersionconstant has been correctly updated to "v4.0.0". Confirm that this version is used consistently in the testnet setup and matches the upgrade version being tested.protocol/testing/containertest/containertest.sh (2)
156-171: The
copy_preupgrade_binary_archfunction has been updated to copy the preupgrade binary based on the system architecture. Verify that the correct binaries are being copied for each architecture.190-190: The script now calls
copy_preupgrade_binary_archto handle the preupgrade binary. Ensure that this change is reflected in the CI/CD pipeline and that the correct binary is used during testing.protocol/app/app.go (3)
- 182-183: The imports for
ibcclientandibcconnectiontypeshave been added. Ensure that these new dependencies are used appropriately within the application and that their introduction does not introduce any compatibility issues with existing code.Verification successful
The executed scripts aimed to verify the usage of the newly added imports
ibcclientandibcconnectiontypesin theapp.gofile. However, the scripts did not produce any output, which suggests that these specific imports might not be used in the codebase, or there might be an issue with the script execution.To further investigate, I will generate a new script to search for the usage of the types or functions from these imported packages throughout the entire codebase, not just in the
app.gofile.
The new script output confirms that the imports
ibcclientandibcconnectiontypesare indeed used within the codebase. Theibcclientpackage is referenced in various files, such asinternal_msg.go,normal_msgs.go,internal_msgs.go, and withinapp.goitself. Similarly, theibcconnectiontypespackage is used inapp.goto register a parameter set.This indicates that the new dependencies are utilized appropriately and are not just added without being referenced, which addresses the initial concern of the review comment.
* 1624-1631: The `initParamsKeeper` function has been adjusted to register key tables for legacy param subspaces, specifically for IBC-related modules. Confirm that these changes are in line with the intended upgrade path and that the key tables are correctly configured.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the newly added imports are used in the codebase. ast-grep --lang go --pattern $'import $$$; ibcclient "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"' ast-grep --lang go --pattern $'import $$$; ibcconnectiontypes "github.com/cosmos/ibc-go/v8/modules/core/03-connection/types"'Length of output: 240
Script:
#!/bin/bash # Search for usage of types or functions from the ibcclient package rg "ibcclient\." # Search for usage of types or functions from the ibcconnectiontypes package rg "ibcconnectiontypes\."Length of output: 1119
Verification successful
The full contents of the
initParamsKeeperfunction have been successfully retrieved, showing the registration of key tables for legacy param subspaces, particularly for IBC-related modules. This confirms that the changes are in line with the PR objectives and the review comment is consistent with the codebase.* 1626-1631: The registration of IBC-related subspaces within the `initParamsKeeper` function includes the creation of a key table and the registration of parameter sets. Ensure that the parameter sets are complete and that the key table is properly utilized within the application.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the key tables for legacy param subspaces are correctly registered in the codebase. ast-grep --lang go --pattern $'func initParamsKeeper($$$) $$$ { $$$ keyTable := ibcclient.ParamKeyTable(); $$$ }'Length of output: 114
Script:
#!/bin/bash # Search for the initParamsKeeper function to verify the registration of key tables for legacy param subspaces. ast-grep --lang go --pattern $'func initParamsKeeper($$$) { $$$ }'Length of output: 67
Script:
#!/bin/bash # Search for the initParamsKeeper function across the codebase to verify its existence and contents. rg "func initParamsKeeper"Length of output: 71
Script:
#!/bin/bash # Display the contents of the initParamsKeeper function from the protocol/app/app.go file. cat protocol/app/app.go | sed -n -e '/func initParamsKeeper/,/}/p'Length of output: 818
Script:
#!/bin/bash # Display the full contents of the initParamsKeeper function from the protocol/app/app.go file to verify the registration of key tables for legacy param subspaces. cat protocol/app/app.go | sed -n -e '/func initParamsKeeper/,/}/p'Length of output: 818
Script:
#!/bin/bash # Display the full contents of the initParamsKeeper function from the protocol/app/app.go file to verify the registration of key tables for legacy param subspaces. # Using a range pattern that extends to the end of the function block. cat protocol/app/app.go | sed -n '/func initParamsKeeper(/,/^}/p'Length of output: 1210
Verification successful
The search results confirm the registration of IBC-related subspaces and their key tables within the
initParamsKeeperfunction in theapp.gofile. The key table is created usingibcclient.ParamKeyTable()and is used to register the parameter set foribcconnectiontypes.Params. Subsequently, theparamsKeeper.Subspacemethod is called with the appropriate module names and their respective key tables.Based on the provided output, the parameter sets appear to be complete, and the key table is properly utilized within the application. Therefore, the review comment has been verified and no issues were found with the registration of these subspaces and their key tables.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the parameter sets are complete and that the key table is properly utilized. ast-grep --lang go --pattern $'func initParamsKeeper($$$) $$$ { $$$ keyTable.RegisterParamSet(&ibcconnectiontypes.Params{}); $$$ }'Length of output: 132
Script:
#!/bin/bash # Verify the registration of the IBC-related subspaces and their key tables in the `initParamsKeeper` function. ast-grep --lang go --pattern $'func initParamsKeeper($$$) { $$$ keyTable := ibcclient.ParamKeyTable() keyTable.RegisterParamSet(&$_) paramsKeeper.Subspace($_).WithKeyTable(keyTable) $$$ }'Length of output: 199
Script:
#!/bin/bash # Adjust the pattern to be less specific and try to match any usage of `keyTable.RegisterParamSet` and `paramsKeeper.Subspace` within the `initParamsKeeper` function. ast-grep --lang go --pattern $'func initParamsKeeper($$$) { $$$ $_.RegisterParamSet(&$_) paramsKeeper.Subspace($_).WithKeyTable($_) $$$ }'Length of output: 147
Script:
#!/bin/bash # Use ripgrep to search for the relevant lines in the `app.go` file directly. rg "keyTable := ibcclient.ParamKeyTable\(\)" -A 5 protocol/app/app.goLength of output: 536
…handler. This adds the 3.0.0-dev0 binaries statically and fixes the IBC parameter space to map the old parameters to the new ones allowing for the IBC upgrade as per the upgrade guide (https://github.com/cosmos/ibc-go/blob/e3e8f343d05d3f7dfd958877562bca3aff89b758/docs/docs/05-migrations/11-v7-to-v8.md#params-migration) and this sample (https://github.com/cosmos/ibc-go/blob/v8.0.0/testing/simapp/app.go#L1007-L1012). It turns out that the upgrade handlers are already routable and the existing logic could be removed safely without requiring a replacement. Decided to allow for using the existing upgrade messages to allow the maximum flexibility when doing the release and punted deprecation till once users are on Cosmos 0.50 for some time. Finally, delete the 3.0.0 upgrade handler.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- protocol/go.mod
- protocol/testing/containertest/preupgrade_genesis.json
Files selected for processing (9)
- protocol/app/app.go (3 hunks)
- protocol/app/msgs/normal_msgs.go (1 hunks)
- protocol/app/upgrades.go (1 hunks)
- protocol/app/upgrades/v4.0.0/constants.go (1 hunks)
- protocol/app/upgrades/v4.0.0/upgrade.go (1 hunks)
- protocol/testing/containertest/Dockerfile (1 hunks)
- protocol/testing/containertest/containertest.sh (3 hunks)
- protocol/testing/containertest/testnet.go (1 hunks)
- protocol/testing/containertest/testnet_test.go (1 hunks)
Files skipped from review due to trivial changes (2)
- protocol/app/msgs/normal_msgs.go
- protocol/testing/containertest/Dockerfile
Additional comments: 8
protocol/app/upgrades/v4.0.0/upgrade.go (1)
- 12-22: The
CreateUpgradeHandlerfunction correctly creates an upgrade handler that logs the upgrade name and runs migrations. The use offmt.Sprintfwithin the logger is appropriate for dynamic messages.protocol/app/upgrades/v4.0.0/constants.go (1)
- 11-28: The
UpgradeNameconstant andUpgradevariable are correctly defined. The added module names in theStoreUpgradesreflect the necessary changes for the Cosmos SDK upgrade from 0.47 to 0.50.protocol/app/upgrades.go (2)
13-17: The
Upgradesslice has been correctly updated to include thev4_0_0.Upgrade, which is necessary for the upgrade process.21-34: The
setupUpgradeHandlersfunction properly checks for duplicate handlers and sets up the new upgrade handler forv4_0_0. The panic on duplicate handlers is a good safeguard.protocol/testing/containertest/testnet.go (1)
- 28-28: The
UpgradeToVersionconstant has been correctly updated to "v4.0.0" to reflect the new upgrade version.protocol/testing/containertest/containertest.sh (2)
156-171: The
copy_preupgrade_binary_archfunction has been introduced to handle the copying of the preupgrade binary based on the system architecture. This change simplifies the process and removes the need to download the binary.190-190: The
copy_preupgrade_binary_archfunction is correctly called within the script to ensure the preupgrade binary is available for the correct architecture.protocol/app/app.go (1)
- 1621-1631: The changes in the
initParamsKeeperfunction include the registration of key tables for legacy param subspaces. Ensure that the subspace names and key tables are correctly associated with their respective modules. This is crucial for the correct functioning of the parameter store.
| @@ -179,6 +179,8 @@ import ( | |||
| ibctransferkeeper "github.com/cosmos/ibc-go/v8/modules/apps/transfer/keeper" | |||
| ibctransfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" | |||
| ibc "github.com/cosmos/ibc-go/v8/modules/core" | |||
| ibcclient "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" // nolint:staticcheck | |||
There was a problem hiding this comment.
The import of ibcclient is marked with nolint:staticcheck. Please ensure that ignoring lint warnings is justified and documented as to why it's necessary in this case. If the import is unused, consider removing it to keep the code clean.
| govRouter := govv1beta1.NewRouter() | ||
| govRouter.AddRoute(govtypes.RouterKey, govv1beta1.ProposalHandler). | ||
| AddRoute(paramproposal.RouterKey, params.NewParamChangeProposalHandler(app.ParamsKeeper)) | ||
| /** TODO(CORE-538): Migrate software upgrade type to Cosmos 0.50. | ||
| .AddRoute(upgradetypes.RouterKey, upgrade.NewSoftwareUpgradeProposalHandler(app.UpgradeKeeper)) */ | ||
| govConfig := govtypes.DefaultConfig() | ||
| /* | ||
| Example of setting gov params: |
There was a problem hiding this comment.
The commented code in lines 513-518 appears to be an example configuration for governance parameters. If this is intended as documentation, it should be clearly marked as such or moved to official documentation. If it's not needed, it should be removed to avoid confusion.
| func TestUpgrade(t *testing.T) { | ||
| t.Skip("TODO(CORE-538): Update upgrade handler for Cosmos 0.50 upgrade") | ||
|
|
||
| // TODO(CORE-538): Swap to using official 3.0.0 binaries, if they are the same as 3.0.0-dev0 then remove this | ||
| // line as that is what is currently being used. | ||
| testnet, err := NewTestnetWithPreupgradeGenesis() | ||
| require.NoError(t, err, "failed to create testnet - is docker daemon running?") | ||
| err = testnet.Start() |
There was a problem hiding this comment.
The TestUpgrade function has been updated with a TODO comment regarding the use of official binaries. This comment should be addressed to ensure that the correct binaries are used for testing the upgrade process.
| sdkCtx.Logger().Info("Running %s Upgrade...", UpgradeName) | ||
| InitializeModuleAccs(sdkCtx, ak) | ||
|
|
||
| // TODO(CORE-824): Initialize ratelimit module params to desired state. |
There was a problem hiding this comment.
Can you move this TODO to v4.0.0 handler? ratelimit module is not implemented fully yet, but it will have genesis params and those need to be initialized properly.
| @@ -154,27 +153,22 @@ setup_cosmovisor() { | |||
| done | |||
| } | |||
|
|
|||
| download_preupgrade_binary() { | |||
| copy_preupgrade_binary_arch() { | |||
There was a problem hiding this comment.
Can start using v3.0.0-rc0 binaries (building right now and will be published soon for testnet).
There was a problem hiding this comment.
Can we just swap them for the 3.0.0 binaries once they are qualified. I don't suspect that this swap will cause many issues and then we will only need to do it the one time.
| @@ -1,169 +0,0 @@ | |||
| package v_3_0_0 | |||
There was a problem hiding this comment.
Can we keep this old upgrade handler around? If I understand correctly it doesn't cost anything.
If we want to cherry-pick something that needs to modify the upgrade handler, it will be nice to have still.
There was a problem hiding this comment.
I can keep but I suspect the merge pain of developing here and then porting it to 0.47 would be annoying when cherry-picking on to 3.0.0 release branch.
There was a problem hiding this comment.
Oh good point. Then I don't care either way.
Changelist
[CORE-538] Add 4.0.0 upgrade handler for Cosmos 0.47 -> 0.50 upgrade handler.
This adds the 3.0.0-dev0 binaries statically and fixes the IBC parameter space to map the old parameters to the new ones allowing for the IBC upgrade as per the upgrade guide and this sample.
It turns out that the upgrade handlers are already routable and the existing logic could be removed safely without requiring a replacement.
Decided to allow for using the existing upgrade messages to allow the maximum flexibility when doing the release and punted deprecation till once users are on Cosmos 0.50 for some time.
Finally, delete the 3.0.0 upgrade handler.
Test Plan
Re-enabled
TestUpgradeAuthor/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.