fix(group): Update TresholdDecisionPolicy to handle `threshold>totalW…

…eight` (#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

@@ -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)

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
+}

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")
 	}

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",
 			},