Handle group member weight updates that break threshold decision policy

[blushi]

Summary

Let's discuss the different proposals below for dealing with group member updates that might break the ThresholdDecisionPolicy.

Problem Definition

As discussed in these threads #9657 and #10887, as soon as a group member weight is udpated (whether the member is removed from or leaves the group, or his associated weight is lowered down), the threshold of the ThresholdDecisionPolicy of associated group policy/ies (previously called group account) could become greater than the group total weight.

In this case, the current version of the group module just blocks the creation of any new proposal (because they would be no way for them to pass):

cosmos-sdk/x/group/keeper/msg_server.go

Line 416 in aaa61e3

err = policy.Validate(g)

cosmos-sdk/x/group/types.go

Lines 115 to 117 in aaa61e3

	if threshold.Cmp(totalWeight) > 0 {
	return sdkerrors.Wrap(errors.ErrInvalid, "policy threshold should not be greater than the total group weight")
	}

But this is not ideal from a user standpoint.

Proposal

Here are a few options to improve that:

1. On group member updates (through an event handler), update the threshold of any ThresholdDecisionPolicy associated to the group to the minimum value between group total weight and current threshold. One drawback here is that automatic threshold updates might not be wanted (e.g. multisig scenario).
2. Return error if a group member update would cause any threshold to be greater than the group total weight. This is a bit more restrictive than the current version and means that some member could potentially not leave a group by itself until the threshold is updated by group policy/ies admin.

A percentage based decision policy could also avoid this type of issue (#10946).

cc/ @cmwaters @aaronc @AmauryM @atheeshp @likhita-809 @aleem1314

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[amaury1093]

Between 1 and 2, I think I prefer 2.

Whether to update the threshold or not should IMO be configurable by the group itself. A 3rd proposal could be:

type DecisionPolicy interface {
  // snip
+ OnWeightUpdate(oldTotalWeight, newTotalWeight uint64, memberUpdates []Member) (DecisionPolicy, error)
}

On MsgLeave or MsgUpdateGroupMember, we would loop through all policies of a group, and for each policy, call its OnWeightUpdate. It returns a new updated DecisionPolicy, that we pack into an Any and store in the group account.

• For percentage decision policy, OnWeightUpdate would do nothing (return itself).
• For threshold decision policy, OnWeightUpdate would be:

func (p ThresholdDecisionPolicy) OnWeightUpdate(oldTotalWeight, newTotalWeight uint64, _ []Member) (DecisionPolicy, error) {
  p.Threshold += newTotalWeight
  p.Threshold -= oldTotalWeight
  // some additional checks?
  return p, nil
}

or a separate threshold decision policy could decide to not update the threshold.

[amaury1093]

I also want to note that this issue hadn't been triaged (unfortunately), and maybe not worth delaying the release.

Marie's solutions 1 and 2 are simpler to implement (I think). The 3rd one above is more involved. If no solution seems perfect, we could also only ship the percentage decision policy for 0.46?

[cmwaters]

I'm not a fan of changing the interface, especially if it only affects a single implementation of the interface.

Couldn't we just change the Allow method of ThresholdDecisionPolicy that states that if every member in a group votes for a proposal, regardless of the threshold, the proposal should pass. That way we're never locked in a scenario where there isn't enough weighting to not allow a proposal to pass.

I would personally also be for punting on ThresholdDecisionPolicy and just shipping with PercentageDecisionPolicy. Correct me if I'm wrong but would it not be possible to add ThresholdDecisionPolicy in a patch release

[amaury1093]

Couldn't we just change the Allow method of ThresholdDecisionPolicy that states that if every member in a group votes for a proposal, regardless of the threshold, the proposal should pass.

Yeah, I like this solution, it would be a variant of Marie's 1, but instead of updating state, we update Allow() and Validate(). It should be fairly easy too.

Any objections to this solution? (e.g. from @blushi)