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)
2022-03-08 17:32:54 +01:00 · 2022-03-08 17:32:54 +01:00 · 68f6c8b74e
parent ccaf94391e
commit 68f6c8b74e
4 changed files with 135 additions and 43 deletions
--- 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)
--- 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
+}
--- 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")
 	}
--- 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",
 			},