From 68f6c8b74e1a6a1a6d9795b8b37b0829a1070d99 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Tue, 8 Mar 2022 17:32:54 +0100 Subject: [PATCH] fix(group): Update TresholdDecisionPolicy to handle `threshold>totalWeight` (#11325) ## Description Closes: #10945 In `ThresholdDecisionPolicy`, allow the threshold to be arbitrarily big (e.g. bigger than the group total weight). In that case, in `Allow()`, the tally result is compared against `min(threshold,total_weight)`. --- ### 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... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers 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... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) --- x/group/keeper/keeper_test.go | 28 ++++++++++++------ x/group/keeper/msg_server.go | 54 +++++++++++++++++++++++++++++++++- x/group/types.go | 41 +++++++++++++++++++------- x/group/types_test.go | 55 +++++++++++++++++++++-------------- 4 files changed, 135 insertions(+), 43 deletions(-) diff --git a/x/group/keeper/keeper_test.go b/x/group/keeper/keeper_test.go index 4cc95f6d1422..1ce1cdaea3bc 100644 --- a/x/group/keeper/keeper_test.go +++ b/x/group/keeper/keeper_test.go @@ -1422,8 +1422,9 @@ func (s *TestSuite) TestSubmitProposal() { bigThresholdAddr := bigThresholdRes.Address defaultProposal := group.Proposal{ - Status: group.PROPOSAL_STATUS_SUBMITTED, - Result: group.PROPOSAL_RESULT_UNFINALIZED, + Address: accountAddr.String(), + Status: group.PROPOSAL_STATUS_SUBMITTED, + Result: group.PROPOSAL_RESULT_UNFINALIZED, FinalTallyResult: group.TallyResult{ YesCount: "0", NoCount: "0", @@ -1484,12 +1485,19 @@ func (s *TestSuite) TestSubmitProposal() { expErr: true, postRun: func(sdkCtx sdk.Context) {}, }, - "impossible case: decision policy threshold > total group weight": { + "decision policy threshold > total group weight": { req: &group.MsgSubmitProposal{ Address: bigThresholdAddr, Proposers: []string{addr2.String()}, }, - expErr: true, + expErr: false, + expProposal: group.Proposal{ + Address: bigThresholdAddr, + Status: group.PROPOSAL_STATUS_SUBMITTED, + Result: group.PROPOSAL_RESULT_UNFINALIZED, + FinalTallyResult: group.DefaultTallyResult(), + ExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_NOT_RUN, + }, postRun: func(sdkCtx sdk.Context) {}, }, "only group members can create a proposal": { @@ -1533,8 +1541,9 @@ func (s *TestSuite) TestSubmitProposal() { }, msgs: []sdk.Msg{msgSend}, expProposal: group.Proposal{ - Status: group.PROPOSAL_STATUS_CLOSED, - Result: group.PROPOSAL_RESULT_ACCEPTED, + Address: accountAddr.String(), + Status: group.PROPOSAL_STATUS_CLOSED, + Result: group.PROPOSAL_RESULT_ACCEPTED, FinalTallyResult: group.TallyResult{ YesCount: "2", NoCount: "0", @@ -1558,8 +1567,9 @@ func (s *TestSuite) TestSubmitProposal() { }, msgs: []sdk.Msg{msgSend}, expProposal: group.Proposal{ - Status: group.PROPOSAL_STATUS_SUBMITTED, - Result: group.PROPOSAL_RESULT_UNFINALIZED, + Address: accountAddr.String(), + Status: group.PROPOSAL_STATUS_SUBMITTED, + Result: group.PROPOSAL_RESULT_UNFINALIZED, FinalTallyResult: group.TallyResult{ YesCount: "0", // Since tally doesn't pass Allow(), we consider the proposal not final NoCount: "0", @@ -1590,7 +1600,7 @@ func (s *TestSuite) TestSubmitProposal() { s.Require().NoError(err) proposal := proposalRes.Proposal - s.Assert().Equal(accountAddr.String(), proposal.Address) + s.Assert().Equal(spec.expProposal.Address, proposal.Address) s.Assert().Equal(spec.req.Metadata, proposal.Metadata) s.Assert().Equal(spec.req.Proposers, proposal.Proposers) s.Assert().Equal(s.blockTime, proposal.SubmitTime) diff --git a/x/group/keeper/msg_server.go b/x/group/keeper/msg_server.go index c55b4773ec20..d615a105ddda 100644 --- a/x/group/keeper/msg_server.go +++ b/x/group/keeper/msg_server.go @@ -185,6 +185,11 @@ func (k Keeper) UpdateGroupMembers(goCtx context.Context, req *group.MsgUpdateGr // Update group in the groupTable. g.TotalWeight = totalWeight.String() g.Version++ + + if err := k.validateDecisionPolicies(ctx, *g); err != nil { + return err + } + return k.groupTable.Update(ctx.KVStore(k.key), g.Id, g) } @@ -314,6 +319,11 @@ func (k Keeper) CreateGroupPolicy(goCtx context.Context, req *group.MsgCreateGro return nil, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "not group admin") } + err = policy.Validate(g, k.config) + if err != nil { + return nil, err + } + // Generate account address of group policy. var accountAddr sdk.AccAddress // loop here in the rare case of a collision @@ -386,7 +396,17 @@ func (k Keeper) UpdateGroupPolicyDecisionPolicy(goCtx context.Context, req *grou policy := req.GetDecisionPolicy() action := func(groupPolicy *group.GroupPolicyInfo) error { - err := groupPolicy.SetDecisionPolicy(policy) + g, err := k.getGroupInfo(ctx, groupPolicy.GroupId) + if err != nil { + return err + } + + err = policy.Validate(g, k.config) + if err != nil { + return err + } + + err = groupPolicy.SetDecisionPolicy(policy) if err != nil { return err } @@ -840,6 +860,10 @@ func (k Keeper) LeaveGroup(goCtx context.Context, req *group.MsgLeaveGroup) (*gr return nil, err } + if err := k.validateDecisionPolicies(ctx, groupInfo); err != nil { + return nil, err + } + ctx.EventManager().EmitTypedEvent(&group.EventLeaveGroup{ GroupId: req.GroupId, Address: req.Address, @@ -949,3 +973,31 @@ func (k Keeper) assertMetadataLength(metadata string, description string) error } return nil } + +// validateDecisionPolicies loops through all decision policies from the group, +// and calls each of their Validate() method. +func (k Keeper) validateDecisionPolicies(ctx sdk.Context, g group.GroupInfo) error { + it, err := k.groupPolicyByGroupIndex.Get(ctx.KVStore(k.key), g.Id) + if err != nil { + return err + } + defer it.Close() + + var groupPolicy group.GroupPolicyInfo + for { + _, err = it.LoadNext(&groupPolicy) + if errors.ErrORMIteratorDone.Is(err) { + break + } + if err != nil { + return err + } + + err = groupPolicy.DecisionPolicy.GetCachedValue().(group.DecisionPolicy).Validate(g, k.config) + if err != nil { + return err + } + } + + return nil +} diff --git a/x/group/types.go b/x/group/types.go index fb9672a5a6bc..6dcab3c33848 100644 --- a/x/group/types.go +++ b/x/group/types.go @@ -74,14 +74,23 @@ func (p ThresholdDecisionPolicy) Allow(tallyResult TallyResult, totalPower strin if err != nil { return DecisionPolicyResult{}, err } - if yesCount.Cmp(threshold) >= 0 { - return DecisionPolicyResult{Allow: true, Final: true}, nil - } totalPowerDec, err := math.NewNonNegativeDecFromString(totalPower) if err != nil { return DecisionPolicyResult{}, err } + + // the real threshold of the policy is `min(threshold,total_weight)`. If + // the group member weights changes (member leaving, member weight update) + // and the threshold doesn't, we can end up with threshold > total_weight. + // In this case, as long as everyone votes yes (in which case + // `yesCount`==`realThreshold`), then the proposal still passes. + realThreshold := min(threshold, totalPowerDec) + + if yesCount.Cmp(realThreshold) >= 0 { + return DecisionPolicyResult{Allow: true, Final: true}, nil + } + totalCounts, err := tallyResult.TotalCounts() if err != nil { return DecisionPolicyResult{}, err @@ -90,29 +99,39 @@ func (p ThresholdDecisionPolicy) Allow(tallyResult TallyResult, totalPower strin if err != nil { return DecisionPolicyResult{}, err } - sum, err := yesCount.Add(undecided) + // maxYesCount is the max potential number of yes count, i.e the current yes count + // plus all undecided count (supposing they all vote yes). + maxYesCount, err := yesCount.Add(undecided) if err != nil { return DecisionPolicyResult{}, err } - if sum.Cmp(threshold) < 0 { + + if maxYesCount.Cmp(realThreshold) < 0 { return DecisionPolicyResult{Allow: false, Final: true}, nil } return DecisionPolicyResult{Allow: false, Final: false}, nil } -// Validate returns an error if policy threshold is greater than the total group weight +func min(a, b math.Dec) math.Dec { + if a.Cmp(b) < 0 { + return a + } + return b +} + +// Validate validates the policy against the group. Note that the threshold +// can actually be greater than the group's total weight: in the Allow method +// we check the tally weight against `min(threshold,total_weight)`. func (p *ThresholdDecisionPolicy) Validate(g GroupInfo, config Config) error { - threshold, err := math.NewPositiveDecFromString(p.Threshold) + _, err := math.NewPositiveDecFromString(p.Threshold) if err != nil { return sdkerrors.Wrap(err, "threshold") } - totalWeight, err := math.NewNonNegativeDecFromString(g.TotalWeight) + _, err = math.NewNonNegativeDecFromString(g.TotalWeight) if err != nil { return sdkerrors.Wrap(err, "group total weight") } - if threshold.Cmp(totalWeight) > 0 { - return sdkerrors.Wrapf(errors.ErrInvalid, "policy threshold %s should not be greater than the total group weight %s", p.Threshold, g.TotalWeight) - } + if p.Windows.MinExecutionPeriod > p.Windows.VotingPeriod+config.MaxExecutionPeriod { return sdkerrors.Wrap(errors.ErrInvalid, "min_execution_period should be smaller than voting_period + max_execution_period") } diff --git a/x/group/types_test.go b/x/group/types_test.go index d1cebc4db4cc..db40b238433f 100644 --- a/x/group/types_test.go +++ b/x/group/types_test.go @@ -19,17 +19,6 @@ func TestThresholdDecisionPolicyValidate(t *testing.T) { policy group.ThresholdDecisionPolicy expErr bool }{ - - { - "threshold bigger than total weight", - group.ThresholdDecisionPolicy{ - Threshold: "12", - Windows: &group.DecisionPolicyWindows{ - VotingPeriod: time.Second, - }, - }, - true, - }, { "min exec period too big", group.ThresholdDecisionPolicy{ @@ -297,13 +286,13 @@ func TestThresholdDecisionPolicyAllow(t *testing.T) { { "YesCount >= threshold decision policy", &group.ThresholdDecisionPolicy{ - Threshold: "3", + Threshold: "2", Windows: &group.DecisionPolicyWindows{ VotingPeriod: time.Second * 100, }, }, &group.TallyResult{ - YesCount: "3", + YesCount: "2", NoCount: "0", AbstainCount: "0", NoWithVetoCount: "0", @@ -319,7 +308,7 @@ func TestThresholdDecisionPolicyAllow(t *testing.T) { { "YesCount < threshold decision policy", &group.ThresholdDecisionPolicy{ - Threshold: "3", + Threshold: "2", Windows: &group.DecisionPolicyWindows{ VotingPeriod: time.Second * 100, }, @@ -339,20 +328,42 @@ func TestThresholdDecisionPolicyAllow(t *testing.T) { false, }, { - "sum votes < threshold decision policy", + "YesCount == group total weight < threshold", &group.ThresholdDecisionPolicy{ - Threshold: "3", + Threshold: "20", Windows: &group.DecisionPolicyWindows{ VotingPeriod: time.Second * 100, }, }, &group.TallyResult{ - YesCount: "1", + YesCount: "3", NoCount: "0", AbstainCount: "0", NoWithVetoCount: "0", }, - "2", + "3", + time.Duration(time.Second * 50), + group.DecisionPolicyResult{ + Allow: true, + Final: true, + }, + false, + }, + { + "maxYesCount < threshold decision policy", + &group.ThresholdDecisionPolicy{ + Threshold: "2", + Windows: &group.DecisionPolicyWindows{ + VotingPeriod: time.Second * 100, + }, + }, + &group.TallyResult{ + YesCount: "0", + NoCount: "1", + AbstainCount: "1", + NoWithVetoCount: "0", + }, + "3", time.Duration(time.Second * 50), group.DecisionPolicyResult{ Allow: false, @@ -361,16 +372,16 @@ func TestThresholdDecisionPolicyAllow(t *testing.T) { false, }, { - "sum votes >= threshold decision policy", + "maxYesCount >= threshold decision policy", &group.ThresholdDecisionPolicy{ - Threshold: "3", + Threshold: "2", Windows: &group.DecisionPolicyWindows{ VotingPeriod: time.Second * 100, }, }, &group.TallyResult{ - YesCount: "1", - NoCount: "0", + YesCount: "0", + NoCount: "1", AbstainCount: "0", NoWithVetoCount: "0", },