Fix decoding of gov props with a ParameterChangeProposal in them. #2198
Conversation
…ChangeProposal in it because that type isn't being registered anymore.
…ov props with a ParameterChangeProposal in them.
WalkthroughThis pull request introduces a registration mechanism for parameter types with codecs, enabling the reading of legacy governance properties. Key modifications include the integration of a new Changes
Possibly related PRs
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
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: 1
🧹 Outside diff range and nitpick comments (1)
app/app_test.go (1)
472-475: Consider using constants for time durations.The time durations (1 minute, 3500 milliseconds, 48 hours) could be defined as constants at the package level for better maintainability and reusability.
+const ( + depositPeriod = 1 * time.Minute + votingStartDelay = 3500 * time.Millisecond + votingPeriod = 48 * time.Hour +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .changelog/unreleased/bug-fixes/2198-fix-gov-props.md (1 hunks)
- app/app.go (2 hunks)
- app/app_test.go (3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changelog/unreleased/bug-fixes/2198-fix-gov-props.md
🔇 Additional comments (4)
app/app_test.go (2)
8-8: LGTM! Required imports added for parameter change proposal testing.The new imports are necessary for handling time-based operations, codec types, governance types, and parameter change proposals.
Also applies to: 20-20, 28-29
448-528: LGTM! Well-structured test for parameter change proposal decoding.The test comprehensively validates the encoding/decoding of parameter change proposals, directly addressing the issue described in the PR objectives.
app/app.go (2)
101-101: LGTM: Import required for legacy parameter types.The import of
parampropspackage is necessary to handle legacy governance proposals containing parameter changes, even though the params module has been removed.
790-793: LGTM: Codec registration enables reading legacy parameter change proposals.The registration of legacy amino codec and interfaces for parameter change proposals is necessary to decode existing governance proposals, even though the params module has been removed. The comment clearly explains the rationale.
Let's verify that these are the only parameter-related types being registered:
✅ Verification successful
The search results confirm that these are the only parameter-related codec registrations in the codebase. Let me verify the specific paramprops package usage to ensure it's properly scoped.
LGTM: Parameter proposal codec registration is correctly implemented
The verification confirms that:
- The paramprops package is only used for codec registration in app.go and for testing parameter change proposals in app_test.go
- The import is properly marked with a nolint comment explaining its necessity for legacy type registration
- The usage is minimal and focused only on handling existing governance proposals
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other parameter-related type registrations # Test: Search for param-related codec registrations. Expect: Only the two we added. rg -i 'param.*register.*(?:amino|interface)' --type goLength of output: 178
Script:
#!/bin/bash # Check the imports and usage of paramprops package rg -g '*.go' 'paramprops' -A 2Length of output: 965
…tion events), #2196 (timeout_commit), #2197 (recordspec cmd), #2198 (ParameterChangeProposal) #2199 (wasm build-address cmd). (#2200) * Suppress scope value owner migration events. (#2195) * Create a no-op event manager and use that during the metadata module migration. * Do not suppress the events for a testnet upgrade since they were emitted when the migration ran on testnet. * Add changelog entry. * Update all the spec proto links to reference v1.20.0 (#2192) * Update all the spec proto links to reference v1.20.0 (instead of 1.19.0). * Add changelog entry. * When prepping a release, combine the dependency bump changelog entries. (#2181) * Add a note to get-dep-changes to alert folks that changing those formats might break other things. * Create an awk script that will combine dependency changelog entries. Update prep-release to use it. Also apply a couple fixes that are alread in the release branch (and will be in main shortly). Also tweak the step 4 and 5 names to provide more context, and fix the verbose output header when recombining the sections. * Add changelog entry. * Clarify the new comment in get-dep-changes.sh. * Update stuff that uses or talks about RELEASE_NOTES.md because it should actually be RELEASE_CHANGELOG.md. The SDK uses _NOTES but only puts a blurb in there, so it's not a changelog. But we include a changelog, so it makes sense to keep it named that way. * Fix the `query metadata recordspec` command when given a rec-spec-id. (#2197) * [2148]: Fix the query metadata recordspec command to correctly use the RecordSpecification query (instead of RecordSpecificationsForContractSpecification) when provided a record specification id. * [2148]: Add changelog entry. * Fix decoding of gov props with a ParameterChangeProposal in them. (#2198) * Write a unit test that fails to parse a gov proposal with a ParameterChangeProposal in it because that type isn't being registered anymore. * Register the params module stuff with the codecs since there's some gov props with a ParameterChangeProposal in them. * Add changelog entry. * Hard-code consensus.timeout_commit to 3.5s for mainnet. (#2196) * [2121]: Change the default consensus timeout value to 3.5 seconds. * [2121]: Hard-code the consensus.timeout_commit value. * [2121]: Fix TestIsTestnetFlagSet to not be affected by existing env vars. * [2121]: Fix a couple unit tests that broke when I changed the default commit timout. * [2121]: Only hard-code the timeout commit on non-testnets. * [2121]: Change the default back to 1.5s for faster default testnets. * [2121]: Fix the TestPreUpgradeCmd that broke because of the hard-coded timeout commit. * [2121]: Add some unit tests that make sure the consensus timeout commit value is behaving as expected. * [2121]: Add changelog entry. * [2121]: When forcing the timeout_commit to be 3.5 seconds, also force the skip flag to be false. * [2121]: Update warnAboutSettings: Evaluate the timeout commit and skip-timeout-commit fields separately. Issue a warning if skip-timeout-commit is true. Issue a warning if the timeout commit is not exactly what we want it to be. * Fix: Add node flag to WASM queries (build-address) (#2199)
Description
Currently on testnet (running
v1.20.0-rc2or-rc3):provenanced query gov proposalsyou'll getError: rpc error: code = Unknown desc = runtime error: invalid memory address or nil pointer dereference: panic.provenanced query gov proposal 133, you getError: rpc error: code = Unknown desc = rpc error: code = Internal desc = collections: encoding error: value decode: no concrete type registered for type URL /cosmos.params.v1beta1.ParameterChangeProposal against interface *v1beta1.Content: unknown request.This is because several gov props (
4,14,18,23,47,49,50,86,94,103,104,133) have aParameterChangeProposalin them. Inv1.20, we completely removed the params module. With that, we stopped registering its types with the various codecs. But those gov props are stored in state, so we still need to be able to do stuff with them.This PR registers the params module stuff with the codecs again, so that those old gov props can be read again.
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments..changelog/unreleased(see Adding Changes).Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.Summary by CodeRabbit
New Features
Bug Fixes
Tests