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
📝 WalkthroughWalkthroughThis pull request focuses on enhancing the Cosmos SDK's upgrade module by addressing a registration issue for Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
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
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
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
Bug Fixes
SoftwareUpgradeProposalwas missing a concrete type registrationModule Changes
x/params,x/crisis,x/distributionx/protocolpool,x/validateDocumentation
UPGRADING.mdwith comprehensive migration instructions for developersGovernance
These changes enhance the Cosmos SDK's modularity, error handling, and upgrade processes.