-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(group): Update TresholdDecisionPolicy to handle threshold>totalWeight
#11325
Conversation
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm adding DecisionPolicy.Validate()
calls in a few places in the msg server:
- MsgCreateGroupPolicy and MsgCreateGroupWithPolicy
- MsgUpdateDecisionPolicy
- MsgLeaveGroup
- MsgUpdateGroupMembers
anywhere else you think we need to add validation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Just a thought, can we use if allYesVotes
instead of min(threshold,total_weight)
, that sounds safe for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
@anilcse Could you explain your change in more details? e.g. which line? |
// 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AmauryM here can we just check if everyone voted yes instead of checking against realThreshold?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. There are 2 scenarios:
- if threshold>totalPower, then we should check that everyone votes yes
- if threshold<totalPower, we don't need everyone to vote yes
In the 2nd cases, we don't check everyone voted yes. The realThreshold
is used to share logic to avoid having an if then else
case, but I can introduce that if it's clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes. I don't have a strong preference now. Let's go with this unless anyone are strongly in favor of checking all-yes-votes for the special case. @blushi @robert-zaremba wdyt?
Description
Closes: #10945
In
ThresholdDecisionPolicy
, allow the threshold to be arbitrarily big (e.g. bigger than the group total weight). In that case, inAllow()
, the tally result is compared againstmin(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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
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...
!
in the type prefix if API or client breaking change