Governance `Abstain` vote #2128

grarco · 2023-11-08T10:28:24Z

Describe your changes

Closes #2090.

Implements the Abstain governance vote. Also reviews the tally process for the different proposal types.

Indicate on which release or other PRs this topic is based on

Commit 3a0ecd8 on base (v0.25.0)

Checklist before merging to `draft`

I have added a changelog
Git history is in acceptable state

cwgoes · 2023-11-08T12:56:19Z

core/src/ledger/governance/utils.rs

+        let at_least_two_thirds_voted = self.total_yay_power
+            + self.total_nay_power
+            + self.total_abstain_power
+            >= self.total_voting_power / 3 * 2;


Suggested change

>= self.total_voting_power / 3 * 2;

>= self.total_voting_power * 2 / 3;

in general, we should practice multiply-then-divide

I think the operators precedence in Rust already evaluates multiplications before divisions, but I can change it. Do you think at this point it's better to split the fraction among the two sides of the inequality an be (yay + nay + abstain) * 3 >= total * 2?

how you have it is fine, I just prefer * 3 / 2. admittedly this is somewhat of an aesthetic choice

cwgoes · 2023-11-08T12:56:36Z

core/src/ledger/governance/utils.rs

+            >= self.total_voting_power / 3 * 2;
+
+        let at_least_two_thirds_nay = self.total_nay_power
+            >= (self.total_nay_power + self.total_yay_power) / 3 * 2;


Suggested change

>= (self.total_nay_power + self.total_yay_power) / 3 * 2;

>= (self.total_nay_power + self.total_yay_power) * 2 / 3;

cwgoes · 2023-11-08T12:57:56Z

core/src/ledger/governance/utils.rs

+            (TallyVote::Offline(vote), TallyVote::Offline(other_vote)) => {
+                vote.is_same_side(other_vote)
+            }
+            _ => false,


when would this case happen? shouldn't this be impossible?

Never, it's just the the method is implemented on the TallyVote enum which contains two variants, one for the on-chain votes and the other one for off-chain votes and we still need the pattern matching to be exhaustive, so we need to cover also the case in which the two instances are actually two different variants. We can return an error instead of false to make it more clear

wherever a case isn't expected to happen, we should return an error

cwgoes · 2023-11-08T12:58:52Z

core/src/ledger/governance/utils.rs

                nay_voting_power += vote_power;
+            } else {


vote.is_abstain() would be more defensive, in case we add another voting option in the future

cwgoes · 2023-11-08T12:59:07Z

core/src/ledger/governance/utils.rs

-                    } else {
+                        if validator_vote.is_nay() {
+                            nay_voting_power -= voting_power;
+                        } else {


cwgoes · 2023-11-08T12:59:12Z

core/src/ledger/governance/utils.rs

-                        yay_voting_power -= voting_power;
+                        if validator_vote.is_yay() {
+                            yay_voting_power -= voting_power;
+                        } else {


cwgoes · 2023-11-08T12:59:18Z

core/src/ledger/governance/utils.rs

+                        abstain_voting_power += voting_power;
+                        if validator_vote.is_yay() {
+                            yay_voting_power -= voting_power;
+                        } else {


cwgoes · 2023-11-08T12:59:22Z

core/src/ledger/governance/utils.rs

                nay_voting_power += voting_power;
+            } else {


cwgoes · 2023-11-08T12:59:39Z

tests/src/e2e/ledger_tests.rs

@@ -2095,7 +2095,7 @@ fn proposal_submission() -> Result<()> {
    let mut client = run!(test, Bin::Client, query_proposal, Some(15))?;
    client.exp_string("Proposal Id: 0")?;
    client.exp_string(
-        "passed with 120900.000000 yay votes and 0.000000 nay votes (0.%)",
+        "passed with 120000.000000 yay votes and 900.000000 nay votes (0.%)",


why did this change? 🤔

In test we submit two votes, a yay with the validator (120000 voting power) and a nay with a delegator (900 voting power) and previously we were not seeing the nay votes before because of a bug in the is_same_side function. I've tagged you at the wrong line

cwgoes · 2023-11-08T13:00:02Z

core/src/ledger/governance/cli/onchain.rs

-    /// Rappresent an invalid proposal vote
+    /// Represent an abstain proposal vote
+    Abstain,
+    /// Represent an invalid proposal vote
    Invalid,


when does this case happen?

Actually nowhere, I'm removing it thanks!

grarco · 2023-11-08T14:45:19Z

core/src/ledger/governance/utils.rs

-        let both_yay = self.is_yay() && other.is_yay();
-        let both_nay = !self.is_yay() && !other.is_yay();
-
-        both_yay || !both_nay


@cwgoes here's the bug for the wrong e2e tally, it should have been both_yay || both_nay

cwgoes

lgtm

Introduces governance Abstain vote and adjusts tally

6afb474

grarco changed the title ~~Introduces governance Abstain vote and adjusts tally~~ Governance Abstain vote Nov 8, 2023

Updates wrong expected value in governance e2e test

3197da0

grarco added a commit that referenced this pull request Nov 8, 2023

Changelog #2128

4bd7b2d

grarco marked this pull request as ready for review November 8, 2023 12:33

grarco requested review from Fraccaman and cwgoes November 8, 2023 12:33

cwgoes requested changes Nov 8, 2023

View reviewed changes

grarco commented Nov 8, 2023

View reviewed changes

grarco mentioned this pull request Nov 8, 2023

Draft #2122

Closed

grarco added 6 commits November 8, 2023 17:26

Fixes typo

0debbfc

Removes unused Invalid variant for governance vote

1279b74

Introduces is_abstain method for governance votes

41276f5

Inverts order of operands in governance tally

84eb5a5

Returns error in is_same_side

b106249

Changelog #2128

20dc3b3

grarco force-pushed the grarco/governance-abstain-vote branch from c79675f to 20dc3b3 Compare November 8, 2023 16:26

grarco requested a review from cwgoes November 8, 2023 16:58

cwgoes approved these changes Nov 8, 2023

View reviewed changes

grarco added the ready for draft label Nov 9, 2023

grarco added a commit that referenced this pull request Nov 9, 2023

Merge branch 'grarco/governance-abstain-vote' (#2128)

fe63db3

grarco added the merged to draft label Nov 9, 2023

grarco merged commit 0de7638 into main Nov 11, 2023
13 of 14 checks passed

grarco deleted the grarco/governance-abstain-vote branch November 11, 2023 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Governance `Abstain` vote #2128

Governance `Abstain` vote #2128

grarco commented Nov 8, 2023 •

edited

Loading

cwgoes Nov 8, 2023

grarco Nov 8, 2023

cwgoes Nov 8, 2023

cwgoes Nov 8, 2023

cwgoes Nov 8, 2023

grarco Nov 8, 2023

cwgoes Nov 8, 2023

cwgoes Nov 8, 2023

cwgoes Nov 8, 2023

cwgoes Nov 8, 2023

cwgoes Nov 8, 2023

cwgoes Nov 8, 2023

cwgoes Nov 8, 2023

grarco Nov 8, 2023 •

edited

Loading

cwgoes Nov 8, 2023

cwgoes Nov 8, 2023

grarco Nov 8, 2023

grarco Nov 8, 2023

cwgoes left a comment

	>= self.total_voting_power / 3 * 2;
	>= self.total_voting_power * 2 / 3;

	>= (self.total_nay_power + self.total_yay_power) / 3 * 2;
	>= (self.total_nay_power + self.total_yay_power) * 2 / 3;

Governance Abstain vote #2128

Governance Abstain vote #2128

Conversation

grarco commented Nov 8, 2023 • edited Loading

Describe your changes

Indicate on which release or other PRs this topic is based on

Checklist before merging to draft

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grarco Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Governance `Abstain` vote #2128

Governance `Abstain` vote #2128

grarco commented Nov 8, 2023 •

edited

Loading

Checklist before merging to `draft`

grarco Nov 8, 2023 •

edited

Loading