fix(x/upgrade): register missing implementation for SoftwareUpgradeProposal #23179
Conversation
…oposal to avoid no concrete type registered for type URL /cosmos.upgrade.v1beta1.SoftwareUpgradeProposal against interface *v1beta1.Content error
📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the Cosmos SDK, focusing on modularization, improved transaction handling, and enhanced governance features. Key changes include the removal of several modules like Changes
Sequence DiagramsequenceDiagram
participant Client
participant UpgradeModule
participant GovModule
participant AccountsModule
Client->>UpgradeModule: Submit SoftwareUpgradeProposal
UpgradeModule->>GovModule: Register Proposal
GovModule->>Client: Proposal Registered
Client->>GovModule: Vote on Proposal
GovModule->>AccountsModule: Prepare Migration
AccountsModule->>Client: Migration Ready
Client->>UpgradeModule: Execute Upgrade
UpgradeModule->>Client: Upgrade Complete
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
|
with messages in gov v1, there shouldnt be a need for this. |
not sure but I need register this when using tx upgrade |
Very weird, tx upgrade uses gov v1: cosmos-sdk/x/upgrade/client/cli/tx.go Line 108 in 787a713 Are you submitting a legacy upgrade proposal and then querying it on the updated binary? |
Upgrade is from sdk50 binary to sdk52 using |
mmsqe
requested review from
facundomedica,
cool-develope,
akhilkumarpilli,
lucaslopezf and
a team
as code owners
| registrar.RegisterImplementations( | ||
| (*v1beta1.Content)(nil), | ||
| &SoftwareUpgradeProposal{}, | ||
| ) |
There was a problem hiding this comment.
| ) | |
| ) | |
| registrar.RegisterImplementations( | |
| (*v1beta1.Content)(nil), | |
| &CancelSoftwareUpgradeProposal{}, | |
| ) |
![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
🧹 Nitpick comments (5)
UPGRADING.md (1)
472-480: Consider adding error handling for the migrationThe upgrade handler correctly calls
MigrateAccountNumberUnsafebut it would be helpful to add some logging or telemetry to track the migration progress, especially for chains with many accounts.app.UpgradeKeeper.SetUpgradeHandler(planName, func(ctx context.Context, _ upgradetypes.Plan, fromVM appmodule.VersionMap) (appmodule.VersionMap, error) { + logger := app.Logger() + logger.Info("starting account number migration") if err := authkeeper.MigrateAccountNumberUnsafe(ctx, &app.AuthKeeper); err != nil { + logger.Error("failed to migrate account numbers", "error", err) return nil, err } + logger.Info("completed account number migration") return app.ModuleManager.RunMigrations(ctx, app.configurator, fromVM) }, )x/upgrade/types/codec_test.go (1)
17-33: Consider adding error test casesWhile the happy path is tested, consider adding test cases for:
- Invalid TypeURL in Any
- Malformed Value in Any
- Missing interface registration
func TestInterfaceRegistrationOfContent(t *testing.T) { testCases := []struct { name string typeURL string value []byte expErr bool }{ {"valid case", "/cosmos.upgrade.v1beta1.SoftwareUpgradeProposal", []byte{}, false}, {"invalid type url", "/invalid.Type.URL", []byte{}, true}, {"malformed value", "/cosmos.upgrade.v1beta1.SoftwareUpgradeProposal", []byte{0x1}, true}, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { // ... test implementation }) } }CHANGELOG.md (3)
Line range hint
1-1: Add missing title to the CHANGELOGThe CHANGELOG file should start with a # title like "# Changelog" for better documentation structure.
+ # Changelog
Line range hint
14-25: Consider adding a table of contentsFor better navigation in such a large CHANGELOG, consider adding a table of contents at the top listing the major versions.
Line range hint
2968-2970: Add link to current CHANGELOGThe reference to "CHANGELOG of previous versions" should include a link to the current CHANGELOG file as well for completeness.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)UPGRADING.md(1 hunks)x/upgrade/types/codec.go(2 hunks)x/upgrade/types/codec_test.go(1 hunks)x/upgrade/types/proposal.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
x/upgrade/types/codec_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/types/codec.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/upgrade/types/proposal.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
UPGRADING.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: tests (03)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: test-integration
- GitHub Check: build (arm64)
- GitHub Check: build (amd64)
- GitHub Check: Analyze
- GitHub Check: golangci-lint
- GitHub Check: Summary
🔇 Additional comments (6)
UPGRADING.md (1)
482-488: LGTM: Store upgrade configuration is correctThe store upgrade correctly adds the accounts module store key to the list of added stores during the upgrade.
x/upgrade/types/proposal.go (3)
8-15: LGTM: Clean implementation of proposal interface methodsThe GetTitle, GetDescription, and ProposalRoute methods are implemented correctly and well documented.
20-21: LGTM: ValidateBasic implementationThe ValidateBasic method correctly delegates to v1beta1.ValidateAbstract for title and description validation.
17-18: Verify ProposalType return valueThe method returns
v1beta1.ProposalTypeTextwhich seems incorrect for a software upgrade proposal. Consider if this should return a more specific proposal type.✅ Verification successful
ProposalType implementation is correct
Returning
ProposalTypeTextis the expected behavior. The gov module provides base proposal types, and modules register their proposal types through theRegisterProposalTypefunction. The recent CHANGELOG entry confirms theSoftwareUpgradeProposalimplementation was properly registered.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any other upgrade proposals using ProposalTypeText rg -A 1 'func.*ProposalType\(\).*string.*{.*return.*ProposalTypeText'Length of output: 246
Script:
#!/bin/bash # Search for ProposalType implementations rg "func.*ProposalType.*string" # Search for proposal type constants rg "ProposalType[A-Za-z]+ += +" # Check the v1beta1 package for proposal types fd "v1beta1" --type f --exec rg -A 2 "type.*Proposal.*interface|const.*ProposalType"Length of output: 1170
Script:
#!/bin/bash # Search for upgrade proposal related constants and types rg -A 3 "const.*ProposalType|type.*Upgrade.*Proposal" # Check the content registry for upgrade proposals rg -A 5 "RegisterProposalType.*upgrade"Length of output: 4029
Script:
#!/bin/bash # Check v1beta1 package for proposal type constants fd "v1beta1" --type f --exec rg "const.*ProposalType.*=|var.*ProposalType.*=" # Check the specific file where ProposalTypeText is used cat x/gov/types/v1beta1/proposal.goLength of output: 7256
x/upgrade/types/codec.go (1)
27-30: LGTM: Interface registration is correctThe implementation correctly registers SoftwareUpgradeProposal as a Content interface implementation.
The previous suggestion about adding CancelSoftwareUpgradeProposal has been addressed in the code.
CHANGELOG.md (1)
Line range hint
1-3000: LGTM on overall CHANGELOG structure and contentThe CHANGELOG follows good practices:
- Proper categorization of changes (Features, Bug Fixes, etc.)
- Clear descriptions with PR/issue links
- Follows semantic versioning
- Includes migration notes and breaking changes
|
Can you run |
CHANGELOG.md
Outdated
| @@ -56,6 +56,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i | |||
| * (x/auth/tx) [#23170](https://github.com/cosmos/cosmos-sdk/pull/23170) Avoid panic from newWrapperFromDecodedTx when AuthInfo.Fee is optional in decodedTx. | |||
| * (x/auth/tx) [23144](https://github.com/cosmos/cosmos-sdk/pull/23144) Add missing CacheWithValue for ExtensionOptions. | |||
| * (x/auth/tx) [#23148](https://github.com/cosmos/cosmos-sdk/pull/23148) Avoid panic from intoAnyV2 when v1.PublicKey is optional. | |||
| * (x/upgrade) [#23179](https://github.com/cosmos/cosmos-sdk/pull/23179) Register missing implementation for SoftwareUpgradeProposal to avoid no concrete type registered for type URL /cosmos.upgrade.v1beta1.SoftwareUpgradeProposal against interface *v1beta1.Content error. | |||
There was a problem hiding this comment.
Should be under x/upgrade/changelog.md
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
x/upgrade/CHANGELOG.md (1)
28-30: Add blank lines for better readability.To maintain consistent formatting throughout the changelog:
## [Unreleased] +### Bug Fixes * (x/upgrade) [#23179](https://github.com/cosmos/cosmos-sdk/pull/23179) Register missing implementation for SoftwareUpgradeProposal to avoid no concrete type registered for type URL /cosmos.upgrade.v1beta1.SoftwareUpgradeProposal against interface *v1beta1.Content error.🧰 Tools
🪛 Markdownlint (0.37.0)
28-28: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
29-29: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
x/upgrade/go.mod (1)
14-14: Consider using a more flexible version constraintThe dependency is strictly pinned to v1.0.0. Consider using a more flexible version constraint (e.g.,
v1.x.x) to allow for minor and patch updates while maintaining compatibility.- cosmossdk.io/x/tx v1.0.0 + cosmossdk.io/x/tx v1.0.0 // indirect
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
x/upgrade/CHANGELOG.md(1 hunks)x/upgrade/go.mod(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/upgrade/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🪛 Markdownlint (0.37.0)
x/upgrade/CHANGELOG.md
28-28: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
(MD022, blanks-around-headings)
29-29: null
Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: test-integration
- GitHub Check: Analyze
- GitHub Check: build (amd64)
- GitHub Check: Summary
🔇 Additional comments (3)
x/upgrade/CHANGELOG.md (2)
28-30: LGTM! The changelog entry is well-structured and informative.The entry follows the changelog guidelines, correctly categorizes the change as a bug fix, and provides clear context about the issue being resolved.
🧰 Tools
🪛 Markdownlint (0.37.0)
28-28: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
29-29: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
Line range hint
1-27: LGTM! The changelog structure follows best practices.The file maintains a clear structure with well-defined sections, follows semantic versioning, and includes proper documentation of the guiding principles.
🧰 Tools
🪛 Markdownlint (0.37.0)
28-28: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
29-29: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
x/upgrade/go.mod (1)
14-14: Verify the necessity of the tx dependencyGiven the discussion about gov v1 handling upgrade proposals, please verify if this direct dependency on
cosmossdk.io/x/txis necessary for the SoftwareUpgradeProposal implementation.Run the following script to analyze the usage of tx package in the upgrade module:
✅ Verification successful
Dependency on
cosmossdk.io/x/txis necessaryThe dependency is required for the core type system infrastructure, specifically for interface registration and codec configuration of upgrade proposals. The package provides essential functionality for address coding and validator address handling used in the module's type registration system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the tx package is actually used for SoftwareUpgradeProposal implementation # Search for imports of the tx package echo "Checking for tx package imports:" rg --type go "cosmossdk.io/x/tx" # Search for SoftwareUpgradeProposal implementation echo -e "\nChecking SoftwareUpgradeProposal implementation:" ast-grep --pattern 'type SoftwareUpgradeProposal struct { $$$ }'Length of output: 11409
Script:
#!/bin/bash # Check the specific usage of tx package in codec_test.go and any connections to SoftwareUpgradeProposal echo "Checking codec_test.go content:" cat x/upgrade/types/codec_test.go echo -e "\nChecking for any connections between tx and SoftwareUpgradeProposal:" rg -A 5 -B 5 "SoftwareUpgradeProposal" x/upgrade/types/codec_test.goLength of output: 1529
to avoid
no concrete type registered for type URL /cosmos.upgrade.v1beta1.SoftwareUpgradeProposal against interface *v1beta1.ContenterrorDescription
Closes: #XXXX
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
Release Notes
Documentation
UPGRADING.mdwith significant Cosmos SDK changesNew Features
x/protocolpoolandx/validatemodulesModule Changes
x/params,x/crisis, andx/distributionmodulesx/evidence,x/nft, andx/feegrantas standalone modulesBug Fixes
SoftwareUpgradeProposalRefactor