fix(group): Update TresholdDecisionPolicy to handle threshold>totalWeight
#11325
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,14 +74,23 @@ func (p ThresholdDecisionPolicy) Allow(tallyResult TallyResult, totalPower strin | |
| if err != nil { | ||
| return DecisionPolicyResult{}, err | ||
| } | ||
|
|
||
| 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) | ||
|
There was a problem hiding this comment. @AmauryM here can we just check if everyone voted yes instead of checking against realThreshold? There was a problem hiding this comment. I see. There are 2 scenarios:
In the 2nd cases, we don't check everyone voted yes. The There was a problem hiding this comment. 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? |
||
|
|
||
| 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 | ||
| } | ||
| // maxYesCount is the max number of yes count, i.e the current yes count | ||
amaury1093 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // plus all undecided count (supposing they all vote yes). | ||
| maxYesCount, err := yesCount.Add(undecided) | ||
| if err != nil { | ||
| return DecisionPolicyResult{}, err | ||
| } | ||
|
|
||
| if maxYesCount.Cmp(realThreshold) < 0 { | ||
| return DecisionPolicyResult{Allow: false, Final: true}, nil | ||
| } | ||
| return DecisionPolicyResult{Allow: false, Final: false}, nil | ||
| } | ||
|
|
||
| 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 { | ||
| _, err := math.NewPositiveDecFromString(p.Threshold) | ||
| if err != nil { | ||
| return sdkerrors.Wrap(err, "threshold") | ||
| } | ||
| _, err = math.NewNonNegativeDecFromString(g.TotalWeight) | ||
| if err != nil { | ||
| return sdkerrors.Wrap(err, "group total weight") | ||
| } | ||
|
|
||
| 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") | ||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:anywhere else you think we need to add validation?