feat(x/gov): add yes_quorum parameter
#19167
Conversation
This comment has been minimized.
This comment has been minimized.
I haven't added any tests here yet, waiting for #19101 to be merged first for avoiding merge conflicts. |
Co-authored-by: Robert Zaremba <[email protected]>
|
Warning Rate Limit Exceeded@julienrbrt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 2 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 Cosmos governance module has undergone significant updates, introducing new parameters like Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Co-authored-by: Facundo Medica <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
x/gov/migrations/v6/store_test.go
Outdated
| func TestMigrateStore(t *testing.T) { | ||
| cdc := moduletestutil.MakeTestEncodingConfig(gov.AppModuleBasic{}).Codec | ||
| govKey := storetypes.NewKVStoreKey("gov") | ||
| ctx := testutil.DefaultContext(govKey, storetypes.NewTransientStoreKey("transient_test")) | ||
| storeService := runtime.NewKVStoreService(govKey) | ||
| sb := collections.NewSchemaBuilder(storeService) | ||
| paramsCollection := collections.NewItem(sb, types.ParamsKey, "params", codec.CollValue[v1.Params](cdc)) | ||
| proposalCollection := collections.NewMap(sb, types.ProposalsKeyPrefix, "proposals", collections.Uint64Key, codec.CollValue[v1.Proposal](cdc)) | ||
|
|
||
| // set defaults without newly added fields | ||
| previousParams := v1.DefaultParams() | ||
| previousParams.YesQuorum = "" | ||
| previousParams.ProposalCancelMaxPeriod = "" | ||
| previousParams.OptimisticAuthorizedAddresses = nil | ||
| previousParams.OptimisticRejectedThreshold = "" | ||
| paramsCollection.Set(ctx, previousParams) | ||
|
|
||
| // Run migrations. | ||
| err := v6.MigrateStore(ctx, paramsCollection, proposalCollection) | ||
| require.NoError(t, err) | ||
|
|
||
| // Check params | ||
| newParams, err := paramsCollection.Get(ctx) | ||
| require.NoError(t, err) | ||
| require.Equal(t, v1.DefaultParams().YesQuorum, newParams.YesQuorum) | ||
| require.Equal(t, v1.DefaultParams().ProposalCancelMaxPeriod, newParams.ProposalCancelMaxPeriod) | ||
| require.Equal(t, v1.DefaultParams().OptimisticAuthorizedAddresses, newParams.OptimisticAuthorizedAddresses) | ||
| require.Equal(t, v1.DefaultParams().OptimisticRejectedThreshold, newParams.OptimisticRejectedThreshold) | ||
| } |
There was a problem hiding this comment.
The test function TestMigrateStore is well-structured and follows best practices for testing migrations. However, it's important to ensure that the test covers all relevant scenarios, including edge cases and failure modes. Consider adding tests that:
- Verify behavior when the store is empty or contains unexpected data formats.
- Check for proper handling of invalid or borderline parameter values.
- Ensure that the migration is idempotent, meaning running it multiple times does not alter the outcome after the first successful migration.
Adding these tests will enhance the robustness of the migration process and ensure that edge cases are properly handled.
Would you like assistance in implementing these additional test scenarios?
| SpamCount: "6000000", | ||
| }, | ||
| }, | ||
| { | ||
| name: "quorum reached, yes quorum not reached: prop fails/burn deposit", | ||
| setup: func(s tallyFixture) { | ||
| params, _ := s.keeper.Params.Get(s.ctx) | ||
| params.YesQuorum = "0.7" | ||
| _ = s.keeper.Params.Set(s.ctx, params) | ||
|
|
||
| setTotalBonded(s, 10000000) | ||
| validatorVote(s, s.valAddrs[0], v1.VoteOption_VOTE_OPTION_ONE) | ||
| validatorVote(s, s.valAddrs[1], v1.VoteOption_VOTE_OPTION_THREE) | ||
| validatorVote(s, s.valAddrs[2], v1.VoteOption_VOTE_OPTION_TWO) | ||
| validatorVote(s, s.valAddrs[4], v1.VoteOption_VOTE_OPTION_ONE) | ||
| validatorVote(s, s.valAddrs[5], v1.VoteOption_VOTE_OPTION_ONE) | ||
| validatorVote(s, s.valAddrs[6], v1.VoteOption_VOTE_OPTION_TWO) | ||
| }, | ||
| expectedPass: false, | ||
| expectedBurn: false, | ||
| expectedTally: v1.TallyResult{ | ||
| YesCount: "3000000", | ||
| AbstainCount: "2000000", | ||
| NoCount: "1000000", | ||
| NoWithVetoCount: "0", | ||
| SpamCount: "0", | ||
| }, | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-766]
Overall, the test file modifications introduce comprehensive scenarios to validate the new yes_quorum parameter's behavior across different proposal types. It's essential to ensure that these tests cover all edge cases and accurately reflect the intended governance logic, especially regarding the handling of abstain votes and the calculation of quorums. Additionally, consider adding tests that specifically target edge cases around the yes_quorum threshold, such as exactly meeting the threshold and just falling short, to ensure robust coverage.
Description
Closes: #11461
Based on #19101 as it is a good parameter to add to msg based params.
#19101 must be merged first.
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.
I have...