fix: state export for gov module constitution #8777
Conversation
WalkthroughThe pull request introduces updates to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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: 2
🧹 Outside diff range and nitpick comments (8)
app/upgrades/v27/upgrades_test.go (3)
21-23: Consider using a more realistic upgrade height.The
v27UpgradeHeightconstant is set to 10, which seems quite low for a real-world scenario. Consider using a more realistic value or adding a comment explaining why this specific value was chosen for the test.
50-59: Consider making the time increment configurable.The
dummyUpgradefunction correctly simulates the upgrade process. However, the time increment of 1 second is arbitrary. Consider making this value configurable or add a comment explaining why 1 second was chosen. This would make the test more flexible and self-documenting.For example:
const upgradeTimeIncrement = time.Second // Arbitrary time increment for upgrade simulation // ... s.Ctx = s.Ctx.WithHeaderInfo(header.Info{ Height: v27UpgradeHeight, Time: s.Ctx.BlockTime().Add(upgradeTimeIncrement), }).WithBlockHeight(v27UpgradeHeight)
69-75: Consider using a constant for the expected constitution value.The
ExecuteGovModuleConstitutionTestmethod correctly verifies the state of the constitution after the upgrade. However, the expected constitution value is hardcoded. Consider defining this as a constant at the package level for better maintainability and to make it easier to update if needed.For example:
const expectedConstitution = "This chain has no constitution." // ... func (s *UpgradeTestSuite) ExecuteGovModuleConstitutionTest() { // ... s.Require().Equal(expectedConstitution, post) }CHANGELOG.md (5)
Line range hint
1-49: Well-organized unreleased section with room for improvementThe unreleased section of the changelog is well-structured, clearly separating state breaking changes, config changes, and state compatible changes. It provides a good overview of the upcoming features and fixes. However, some entries could benefit from more detailed explanations or links to relevant issues/PRs for better context.
Consider adding more context to some entries, such as brief explanations of the impact of certain changes or links to related issues/PRs for readers who want to dive deeper into specific changes.
Line range hint
51-55: Concise and clear bug fix entriesThe v26.0.1 section effectively communicates the minor CLI bug fixes addressed in this release. The fixes for the vesting by duration command and pagination in x/incentives module queries are clearly stated.
Consider adding the PR numbers or links for these fixes to make it easier for developers to reference the specific changes if needed.
Line range hint
57-190: Comprehensive changelog with room for consistency improvementsThe v26.0.0 section provides a detailed and extensive overview of the major release changes, covering breaking changes, new features, bug fixes, and improvements across multiple modules. The inclusion of SDK updates and config changes is particularly helpful for users and developers.
- Improve consistency by adding PR numbers to all entries, not just some.
- Consider providing brief explanations for more technical changes to help users understand their impact.
- Group related changes together (e.g., all SDK-related changes in one subsection) to improve readability.
- For entries like "Remove x/Bech32IBC", consider adding a brief explanation of why this module was removed and its impact on users.
Line range hint
192-1160: Valuable historical changelog with opportunities for improvementThe inclusion of changelogs for older releases, dating back to v1.0.0, provides a valuable historical record of the project's evolution. This information is crucial for users and developers who need to understand the changes between different versions.
- Strive for consistent levels of detail across all release entries. Some releases have very detailed changelogs, while others are quite brief.
- For major releases or significant changes, consider adding more context or explanations, especially in older entries where the reasons for changes might not be immediately apparent to new users.
- Use consistent formatting and structure across all release entries to improve readability.
- Consider adding links to relevant discussions, proposals, or design documents for major feature additions or breaking changes, particularly in older releases.
Line range hint
1-1160: Comprehensive changelog with room for consistency and clarity improvementsThis CHANGELOG.md file provides a detailed and valuable history of the project's evolution from v1.0.0 to the latest unreleased changes. It covers a wide range of updates including features, bug fixes, improvements, and breaking changes across multiple releases.
To further improve this already informative changelog:
- Standardize the format and level of detail across all releases for better consistency.
- Always include PR/issue numbers for easy reference.
- Provide more context for major changes, especially in older releases.
- Group related changes together within each release (e.g., by module or type of change).
- Consider adding a brief summary at the top of each major release highlighting the most significant changes.
- Ensure all entries clearly indicate whether they are features, bug fixes, or breaking changes.
- For breaking changes, always include upgrade instructions or links to upgrade guides.
These improvements would make the changelog even more useful for both users and developers, providing a clearer picture of the project's development over time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- app/upgrades/v27/upgrades.go (2 hunks)
- app/upgrades/v27/upgrades_test.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
app/upgrades/v27/upgrades_test.go (4)
3-19: LGTM: Import statements are well-organized and relevant.The import statements are properly grouped and include all necessary packages for the test suite. There are no unused imports.
25-28: LGTM: UpgradeTestSuite struct is well-defined.The
UpgradeTestSuitestruct is properly structured for a test suite. It embedsapptesting.KeeperTestHelperfor common testing utilities and includes apreModulefield to test the pre-block logic of the upgrade module.
30-32: LGTM: TestUpgradeTestSuite function is correctly implemented.The
TestUpgradeTestSuitefunction properly usessuite.Runto execute the test suite, following the standard pattern for testify suites.
61-67: LGTM: PrepareGovModuleConstitutionTest method is well-implemented.The
PrepareGovModuleConstitutionTestmethod correctly verifies the initial state of the constitution before the upgrade. It uses proper error handling and assertions to ensure the constitution is initially empty.app/upgrades/v27/upgrades.go (2)
7-9: Appropriate addition of necessary importsThe added imports for
sdkandgovkeeperare required for the new functionality and are correctly referenced.
40-44: Initialization function correctly sets the default constitutionThe
InitializeConstitutionCollectionfunction accurately sets the default constitution in the governance keeper, aligning with the cosmos-sdk v5 governance migration guidelines.
app/upgrades/v27/upgrades_test.go
Outdated
| func (s *UpgradeTestSuite) TestUpgrade() { | ||
| s.Setup() | ||
| s.preModule = upgrade.NewAppModule(s.App.UpgradeKeeper, addresscodec.NewBech32Codec("osmo")) | ||
|
|
||
| s.PrepareGovModuleConstitutionTest() | ||
|
|
||
| // Run the upgrade | ||
| dummyUpgrade(s) | ||
| s.Require().NotPanics(func() { | ||
| _, err := s.preModule.PreBlock(s.Ctx) | ||
| s.Require().NoError(err) | ||
| }) | ||
|
|
||
| s.ExecuteGovModuleConstitutionTest() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding more specific error checks for pre-block execution.
The TestUpgrade method provides a good coverage of the upgrade scenario. However, the error handling for the pre-block execution could be more specific. Instead of just checking for panics, consider adding assertions for expected behavior or specific error types.
For example:
preBlockResp, err := s.preModule.PreBlock(s.Ctx)
s.Require().NoError(err)
// Add more specific checks on preBlockResp if applicableThis would provide more detailed validation of the upgrade process.
| sdkCtx := sdk.UnwrapSDKContext(ctx) | ||
|
|
||
| err = InitializeConstitutionCollection(sdkCtx, *keepers.GovKeeper) | ||
| if err != nil { | ||
| sdkCtx.Logger().Error("Error initializing Constitution Collection:", "message", err.Error()) | ||
| } | ||
|
|
There was a problem hiding this comment.
Handle errors properly after initializing the constitution collection
Currently, if InitializeConstitutionCollection returns an error, it is logged but not propagated. To prevent the application from running in a faulty state, consider returning the error to halt the upgrade process if initialization fails.
Apply this diff to handle the error appropriately:
err = InitializeConstitutionCollection(sdkCtx, *keepers.GovKeeper)
if err != nil {
- sdkCtx.Logger().Error("Error initializing Constitution Collection:", "message", err.Error())
+ return nil, err
}This change ensures that the upgrade handler returns the error, stopping the upgrade if the constitution collection fails to initialize.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sdkCtx := sdk.UnwrapSDKContext(ctx) | |
| err = InitializeConstitutionCollection(sdkCtx, *keepers.GovKeeper) | |
| if err != nil { | |
| sdkCtx.Logger().Error("Error initializing Constitution Collection:", "message", err.Error()) | |
| } | |
| sdkCtx := sdk.UnwrapSDKContext(ctx) | |
| err = InitializeConstitutionCollection(sdkCtx, *keepers.GovKeeper) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // setting the default constitution for the chain | ||
| // this is in line with cosmos-sdk v5 gov migration: https://github.com/cosmos/cosmos-sdk/blob/v0.50.10/x/gov/migrations/v5/store.go#L57 | ||
| func InitializeConstitutionCollection(ctx sdk.Context, govKeeper govkeeper.Keeper) error { | ||
| return govKeeper.Constitution.Set(ctx, "This chain has no constitution.") | ||
| } |
There was a problem hiding this comment.
Does this imply none of the v5 gov migrations were run? Do we need to do the others as well?
There was a problem hiding this comment.
The migration on v0.50 didn't function as expected eg
50: // setting the default constitution for the chain
51: // this is in line with cosmos-sdk v5 gov migration: https://github.com/cosmos/cosmos-sdk/blob/v0.50.10/x/gov/migrations/v5/store.go#L57
52: func InitializeConstitutionCollection(ctx sdk.Context, govKeeper govkeeper.Keeper) error {
53: ok, err := govKeeper.Constitution.Has(ctx)
=> 54: if !ok || err != nil {
55: if err := govKeeper.Constitution.Set(ctx, "This chain has no constitution."); err != nil {
56: return err
57: }
58: }
59:
(dlv) p ok
true
(dlv) p err
error nil
(dlv) s
> github.com/osmosis-labs/osmosis/v26/app/upgrades/v27.InitializeConstitutionCollection() ./upgrades.go:60 (PC: 0x49c4319)
55: if err := govKeeper.Constitution.Set(ctx, "This chain has no constitution."); err != nil {
56: return err
57: }
58: }
59:
=> 60: return nil
https://github.com/cosmos/cosmos-sdk/blob/main/x/gov/migrations/v5/store.go#L56-L60
The rest ran, you can check by:
osmosisd q gov params --node https://rpc.osmosis.zone:443 --output json | jq
What is the purpose of the change
Currently the export of Osmosis to genesis state isn't working as expected because of the export of gov is missing a gov collection key.
see: https://github.com/cosmos/cosmos-sdk/issues/21887
Testing and Verifying
Unit test:
cd ./app/upgrades/v27go test ./...in-place-testnet:
osmosisd export --modules-to-export "gov"Query:
osmosisd q gov constitutionBUG:
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)