feat(x/gov): multiple choice proposals #18762
Conversation
|
Adding tests later. Concept R4R for review. |
This comment has been minimized.
This comment has been minimized.
WalkthroughThe Cosmos governance module ( Changes
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 (
|
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
x/gov/keeper/grpc_query_test.go
Outdated
| func (suite *KeeperTestSuite) TestProposalVoteOptions() { | ||
| suite.reset() | ||
|
|
||
| } |
There was a problem hiding this comment.
The function TestProposalVoteOptions is empty and does not contain any test code. Please implement the test or add a TODO comment if it's a work in progress.
| type _MsgSubmitMultipleChoiceProposal_1_list struct { | ||
| list *[]*v1beta1.Coin | ||
| } | ||
|
|
||
| func (x *_MsgSubmitMultipleChoiceProposal_1_list) Len() int { | ||
| if x.list == nil { | ||
| return 0 | ||
| } | ||
| return len(*x.list) | ||
| } | ||
|
|
||
| func (x *_MsgSubmitMultipleChoiceProposal_1_list) Get(i int) protoreflect.Value { | ||
| return protoreflect.ValueOfMessage((*x.list)[i].ProtoReflect()) | ||
| } | ||
|
|
||
| func (x *_MsgSubmitMultipleChoiceProposal_1_list) Set(i int, value protoreflect.Value) { | ||
| valueUnwrapped := value.Message() | ||
| concreteValue := valueUnwrapped.Interface().(*v1beta1.Coin) | ||
| (*x.list)[i] = concreteValue | ||
| } | ||
|
|
||
| func (x *_MsgSubmitMultipleChoiceProposal_1_list) Append(value protoreflect.Value) { | ||
| valueUnwrapped := value.Message() | ||
| concreteValue := valueUnwrapped.Interface().(*v1beta1.Coin) | ||
| *x.list = append(*x.list, concreteValue) | ||
| } | ||
|
|
||
| func (x *_MsgSubmitMultipleChoiceProposal_1_list) AppendMutable() protoreflect.Value { | ||
| v := new(v1beta1.Coin) | ||
| *x.list = append(*x.list, v) | ||
| return protoreflect.ValueOfMessage(v.ProtoReflect()) | ||
| } | ||
|
|
||
| func (x *_MsgSubmitMultipleChoiceProposal_1_list) Truncate(n int) { | ||
| for i := n; i < len(*x.list); i++ { | ||
| (*x.list)[i] = nil | ||
| } | ||
| *x.list = (*x.list)[:n] | ||
| } | ||
|
|
||
| func (x *_MsgSubmitMultipleChoiceProposal_1_list) NewElement() protoreflect.Value { | ||
| v := new(v1beta1.Coin) | ||
| return protoreflect.ValueOfMessage(v.ProtoReflect()) | ||
| } | ||
|
|
||
| func (x *_MsgSubmitMultipleChoiceProposal_1_list) IsValid() bool { | ||
| return x.list != nil | ||
| } |
There was a problem hiding this comment.
The _MsgSubmitMultipleChoiceProposal_1_list type and its methods seem to implement the protoreflect.List interface correctly. However, ensure that the Set and Append methods handle the case where the passed protoreflect.Value is not of the expected type, which could lead to a panic if not properly checked.
There was a problem hiding this comment.
don't look at proto generated files!
x/gov/keeper/tally_test.go
Outdated
| func TestTally_Multiple_Choice(t *testing.T) { | ||
|
|
||
| } |
There was a problem hiding this comment.
The function TestTally_Multiple_Choice is empty. Please implement the test logic or add a TODO comment if it's intended to be completed later.
x/gov/keeper/msg_server_test.go
Outdated
| func (suite *KeeperTestSuite) TestSubmitMultipleChoiceProposal() { | ||
|
|
||
| } |
There was a problem hiding this comment.
The TestSubmitMultipleChoiceProposal function is currently empty. It should be implemented to test the new functionality or marked with a TODO if it's a work in progress.
julienrbrt
force-pushed
the
julien/mc
branch
from
8e3a271
to
a8da787
Compare
| // ProposalVoteOptions defines the stringified vote options for proposals. | ||
| // This allows to support multiple choice options for a given proposal. | ||
| // | ||
| // Since: x/gov v1.0.0 | ||
| message ProposalVoteOptions { | ||
| // option_one is the first option of the proposal | ||
| string option_one = 1; | ||
|
|
||
| // option_two is the second option of the proposal | ||
| string option_two = 2; | ||
|
|
||
| // option_three is the third option of the proposal | ||
| string option_three = 3; | ||
|
|
||
| // option_four is the fourth option of the proposal | ||
| string option_four = 4; | ||
|
|
||
| // option_spam is always present for all proposals. | ||
| string option_spam = 5; | ||
| } |
There was a problem hiding this comment.
The introduction of the ProposalVoteOptions message with fields for multiple options and a spam option is consistent with the PR's objective to support multiple choice proposals. However, the naming of the options as option_one, option_two, etc., and option_spam may not be descriptive enough for future maintainers or users of the SDK. Consider using more descriptive names that convey the meaning or context of each option.
x/gov/keeper/tally_test.go
Outdated
| func TestTally_Multiple_Choice(t *testing.T) { | ||
| } |
There was a problem hiding this comment.
The function TestTally_Multiple_Choice is currently empty. If the implementation is pending, consider adding a TODO comment to indicate that tests need to be written. Otherwise, please implement the test cases for multiple choice tallying.
func TestTally_Multiple_Choice(t *testing.T) {
+ // TODO: Implement test cases for multiple choice tallying
}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.
| func TestTally_Multiple_Choice(t *testing.T) { | |
| } | |
| func TestTally_Multiple_Choice(t *testing.T) { | |
| // TODO: Implement test cases for multiple choice tallying | |
| } |
| @@ -939,3 +939,329 @@ func TestTally_Optimistic(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| // TODO(@julienrbrt): refactor tally result to fit all proposal types | |||
There was a problem hiding this comment.
The TODO comment on line 943 suggests that there is an intention to refactor the tally result to fit all proposal types. This should be tracked in the project's issue tracker to ensure it is not forgotten.
| expectedTally: v1.TallyResult{ | ||
| YesCount: "2000000", | ||
| AbstainCount: "0", | ||
| NoCount: "2000000", | ||
| NoWithVetoCount: "0", | ||
| SpamCount: "0", | ||
| }, | ||
| }, | ||
| { | ||
| name: "quorum reached with spam > all other votes: prop fails/burn deposit", | ||
| setup: func(s tallyFixture) { | ||
| setTotalBonded(s, 10000000) | ||
| validatorVote(s, s.valAddrs[0], v1.VoteOption_VOTE_OPTION_ONE) | ||
| // spam votes | ||
| validatorVote(s, s.valAddrs[1], v1.VoteOption_VOTE_OPTION_SPAM) | ||
| validatorVote(s, s.valAddrs[2], v1.VoteOption_VOTE_OPTION_SPAM) | ||
| validatorVote(s, s.valAddrs[3], v1.VoteOption_VOTE_OPTION_SPAM) | ||
| validatorVote(s, s.valAddrs[4], v1.VoteOption_VOTE_OPTION_SPAM) | ||
| validatorVote(s, s.valAddrs[5], v1.VoteOption_VOTE_OPTION_SPAM) | ||
| validatorVote(s, s.valAddrs[6], v1.VoteOption_VOTE_OPTION_SPAM) | ||
| }, | ||
| expectedPass: false, | ||
| expectedBurn: true, | ||
| expectedTally: v1.TallyResult{ | ||
| YesCount: "1000000", | ||
| AbstainCount: "0", | ||
| NoCount: "0", | ||
| NoWithVetoCount: "0", | ||
| SpamCount: "6000000", | ||
| }, | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| govKeeper, mocks, _, ctx := setupGovKeeper(t, mockAccountKeeperExpectations) | ||
| params := v1.DefaultParams() | ||
| // Ensure params value are different than false | ||
| params.BurnVoteQuorum = true | ||
| params.BurnVoteVeto = true | ||
| err := govKeeper.Params.Set(ctx, params) | ||
| require.NoError(t, err) | ||
| var ( | ||
| numVals = 10 | ||
| numDelegators = 5 | ||
| addrs = simtestutil.CreateRandomAccounts(numVals + numDelegators) | ||
| valAddrs = simtestutil.ConvertAddrsToValAddrs(addrs[:numVals]) | ||
| delAddrs = addrs[numVals:] | ||
| ) | ||
| // Mocks a bunch of validators | ||
| mocks.stakingKeeper.EXPECT(). | ||
| IterateBondedValidatorsByPower(ctx, gomock.Any()). | ||
| DoAndReturn( | ||
| func(ctx context.Context, fn func(index int64, validator sdk.ValidatorI) bool) error { | ||
| for i := int64(0); i < int64(numVals); i++ { | ||
| fn(i, stakingtypes.Validator{ | ||
| OperatorAddress: valAddrs[i].String(), | ||
| Status: stakingtypes.Bonded, | ||
| Tokens: sdkmath.NewInt(1000000), | ||
| DelegatorShares: sdkmath.LegacyNewDec(1000000), | ||
| }) | ||
| } | ||
| return nil | ||
| }) | ||
|
|
||
| // Submit and activate a proposal | ||
| proposal, err := govKeeper.SubmitProposal(ctx, nil, "", "title", "summary", delAddrs[0], v1.ProposalType_PROPOSAL_TYPE_MULTIPLE_CHOICE) | ||
| require.NoError(t, err) | ||
| err = govKeeper.ProposalVoteOptions.Set(ctx, proposal.Id, v1.ProposalVoteOptions{ | ||
| OptionOne: "Vote Option 1", | ||
| OptionTwo: "Vote Option 2", | ||
| OptionThree: "Vote Option 3", | ||
| OptionFour: "Vote Option 4", | ||
| }) | ||
| require.NoError(t, err) | ||
| err = govKeeper.ActivateVotingPeriod(ctx, proposal) | ||
| require.NoError(t, err) | ||
| suite := tallyFixture{ | ||
| t: t, | ||
| proposal: proposal, | ||
| valAddrs: valAddrs, | ||
| delAddrs: delAddrs, | ||
| ctx: ctx, | ||
| keeper: govKeeper, | ||
| mocks: mocks, | ||
| } | ||
| tt.setup(suite) | ||
|
|
||
| pass, burn, tally, err := govKeeper.Tally(ctx, proposal) | ||
|
|
||
| require.NoError(t, err) | ||
| assert.Equal(t, tt.expectedPass, pass, "wrong pass") | ||
| assert.Equal(t, tt.expectedBurn, burn, "wrong burn") | ||
| assert.Equal(t, tt.expectedTally, tally) | ||
| // Assert votes removal after tally | ||
| rng := collections.NewPrefixedPairRange[uint64, sdk.AccAddress](proposal.Id) | ||
| _, err = suite.keeper.Votes.Iterate(suite.ctx, rng) | ||
| assert.NoError(t, err) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The test cases within TestTally_MultipleChoice appear to be comprehensive and should effectively validate the tallying logic for multiple-choice proposals. However, it is important to ensure that the test setup does not perform unnecessary computations that could slow down the test suite. For example, if the setup for each test case is similar, consider using a shared setup function to reduce redundancy and improve test execution time.
| @@ -2,6 +2,7 @@ package keeper | |||
|
|
|||
| import ( | |||
| "context" | |||
| stderrors "errors" | |||
There was a problem hiding this comment.
The import alias stderrors for the standard errors package is unconventional. Consider using a more descriptive alias if necessary to avoid confusion with the cosmossdk.io/errors package.
| func TestVotes_MultipleChoiceProposal(t *testing.T) { | ||
| govKeeper, mocks, _, ctx := setupGovKeeper(t) | ||
| authKeeper, bankKeeper, stakingKeeper := mocks.acctKeeper, mocks.bankKeeper, mocks.stakingKeeper | ||
| addrs := simtestutil.AddTestAddrsIncremental(bankKeeper, stakingKeeper, ctx, 2, sdkmath.NewInt(10000000)) | ||
| authKeeper.EXPECT().AddressCodec().Return(address.NewBech32Codec("cosmos")).AnyTimes() | ||
|
|
||
| proposal, err := govKeeper.SubmitProposal(ctx, nil, "", "title", "description", sdk.AccAddress("cosmos1ghekyjucln7y67ntx7cf27m9dpuxxemn4c8g4r"), v1.ProposalType_PROPOSAL_TYPE_MULTIPLE_CHOICE) | ||
| require.NoError(t, err) | ||
| err = govKeeper.ProposalVoteOptions.Set(ctx, proposal.Id, v1.ProposalVoteOptions{ | ||
| OptionOne: "Vote for @tac0turle", | ||
| OptionTwo: "Vote for @facudomedica", | ||
| OptionThree: "Vote for @alexanderbez", | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| proposal.Status = v1.StatusVotingPeriod | ||
| require.NoError(t, govKeeper.SetProposal(ctx, proposal)) | ||
|
|
||
| proposalID := proposal.Id | ||
|
|
||
| // invalid options | ||
| require.Error(t, govKeeper.AddVote(ctx, proposalID, addrs[0], v1.NewNonSplitVoteOption(invalidOption), ""), "invalid option") | ||
| require.Error(t, govKeeper.AddVote(ctx, proposalID, addrs[0], v1.NewNonSplitVoteOption(v1.OptionFour), ""), "invalid option") // option four is not defined. | ||
|
|
||
| // valid options | ||
| require.NoError(t, govKeeper.AddVote(ctx, proposalID, addrs[0], v1.NewNonSplitVoteOption(v1.OptionOne), "")) | ||
| require.NoError(t, govKeeper.AddVote(ctx, proposalID, addrs[1], v1.NewNonSplitVoteOption(v1.OptionTwo), "")) | ||
| require.NoError(t, govKeeper.AddVote(ctx, proposalID, addrs[0], v1.NewNonSplitVoteOption(v1.OptionThree), "")) | ||
| } |
There was a problem hiding this comment.
The new test function TestVotes_MultipleChoiceProposal seems to cover the basic scenarios for multiple-choice proposals, including submitting a proposal, setting proposal vote options, and adding votes with both valid and invalid options. However, it does not appear to test the actual vote tallying or the behavior when a vote is cast for an option that is not part of the proposal. It would be beneficial to add such test cases to ensure the tallying logic handles these scenarios correctly.
There was a problem hiding this comment.
because it then tests voting instead of tallying, which isn't the purpose of this test. this is properly tested in vote testes
Description
ref: #18498
Implements multiple choice proposals
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...