Repository: DA0-DA0/dao-contracts
Pull request #221 (2022-03-25 00:55:08)

v1 contracts - Properly determine if a proposal is rejected.
This updates the rejection logic so that proposals are considered
rejected if the number of no votes > 100% - threshold.

This also adds a number of tests that check our behavior in the event
that a proposal results in a tie or a proposal has only abstain
votes. The current behavior, and the behavior of the old contracts is
to pass the proposal in both cases. We should do some deep thinking
about if this is the desired behavior.



Patch:
diff --git a/contracts/cw-govmod-single/src/proposal.rs b/contracts/cw-govmod-single/src/proposal.rs
index 214bd14b..5443cdc2 100644
--- a/contracts/cw-govmod-single/src/proposal.rs
+++ b/contracts/cw-govmod-single/src/proposal.rs
@@ -1,4 +1,4 @@
-use cosmwasm_std::{Addr, BlockInfo, CosmosMsg, Empty, StdResult, Storage, Uint128};
+use cosmwasm_std::{Addr, BlockInfo, CosmosMsg, Decimal, Empty, StdResult, Storage, Uint128};
 use cw_utils::Expiration;
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
@@ -83,32 +83,16 @@ impl Proposal {
 
     /// Sets a proposals status to its current status.
     pub fn update_status(&mut self, block: &BlockInfo) {
-        self.status = self.current_status(block)
+        self.status = self.current_status(block);
     }
 
     /// Returns true iff this proposal is sure to pass (even before
     /// expiration if no future sequence of possible votes can cause
     /// it to fail)
     pub fn is_passed(&self, block: &BlockInfo) -> bool {
-        self.does_vote_count_reach_threshold(self.votes.yes, block)
-    }
-
-    /// As above for the passed check, used to check if a proposal is
-    /// already rejected.
-    pub fn is_rejected(&self, block: &BlockInfo) -> bool {
-        self.does_vote_count_reach_threshold(self.votes.no, block)
-    }
-
-    /// Helper function to determine if a vote count has reached the
-    /// proposal's threshold. Useful for determining if a particular
-    /// outcome has been reached, for example
-    /// `does_vote_count_meet_threshold(self.votes.yes, block)` will
-    /// return true if there are a sufficent number of yes votes to
-    /// pass the proposal.
-    fn does_vote_count_reach_threshold(&self, vote_count: Uint128, block: &BlockInfo) -> bool {
         match self.threshold {
             Threshold::AbsolutePercentage { percentage } => votes_meet_threshold(
-                vote_count,
+                self.votes.yes,
                 self.total_power - self.votes.abstain,
                 percentage,
             ),
@@ -130,10 +114,115 @@ impl Proposal {
                     // `token_price * quorum` as opposed to the
                     // 'desired' `token_price * threshold`.
                     let options = self.votes.total() - self.votes.abstain;
-                    votes_meet_threshold(vote_count, options, threshold)
+                    votes_meet_threshold(self.votes.yes, options, threshold)
                 } else {
                     let options = self.total_power - self.votes.abstain;
-                    votes_meet_threshold(vote_count, options, threshold)
+                    votes_meet_threshold(self.votes.yes, options, threshold)
+                }
+            }
+        }
+    }
+
+    /// As above for the passed check, used to check if a proposal is
+    /// already rejected.
+    pub fn is_rejected(&self, block: &BlockInfo) -> bool {
+        match self.threshold {
+            Threshold::AbsolutePercentage {
+                percentage: percentage_needed,
+            } => {
+                let options = self.total_power - self.votes.abstain;
+
+                // If there is a 100% passing threshold..
+                if percentage_needed == Decimal::percent(100) {
+                    if options == Uint128::zero() {
+                        // and there are no possible votes (zero
+                        // voting power or all abstain), then this
+                        // proposal has been rejected.
+                        return true;
+                    } else {
+                        // and there are possible votes, then this is
+                        // rejected if there is a single no vote.
+                        //
+                        // We need this check becuase otherwise when
+                        // we invert the threshold (`Decimal::one() -
+                        // threshold`) we get a 0% requirement for no
+                        // votes. Zero no votes do indeed meet a 0%
+                        // threshold.
+                        return self.votes.no >= Uint128::new(1);
+                    }
+                }
+
+                // We need to invert the threshold, when nos exceed
+                // this inverted threshold we know the yes can never
+                // reach the threshold.
+                votes_meet_threshold(self.votes.no, options, Decimal::one() - percentage_needed)
+            }
+            Threshold::ThresholdQuorum { threshold, quorum } => {
+                match (
+                    votes_meet_threshold(self.votes.total(), self.total_power, quorum),
+                    self.expiration.is_expired(block),
+                ) {
+                    // Has met quorum and is expired.
+                    (true, true) => {
+                        // => consider only votes cast and see if no
+                        //    votes meet threshold.
+                        let options = self.votes.total() - self.votes.abstain;
+
+                        // If there is a 100% passing threshold..
+                        if threshold == Decimal::percent(100) {
+                            if options == Uint128::zero() {
+                                // and there are no possible votes (zero
+                                // voting power or all abstain), then this
+                                // proposal has been rejected.
+                                return true;
+                            } else {
+                                // and there are possible votes, then this is
+                                // rejected if there is a single no vote.
+                                //
+                                // We need this check becuase
+                                // otherwise when we invert the
+                                // threshold (`Decimal::one() -
+                                // threshold`) we get a 0% requirement
+                                // for no votes. Zero no votes do
+                                // indeed meet a 0% threshold.
+                                return self.votes.no >= Uint128::new(1);
+                            }
+                        }
+
+                        votes_meet_threshold(self.votes.no, options, Decimal::one() - threshold)
+                    }
+                    // Has met quorum and is not expired.
+                    // | Hasn't met quorum and is not expired.
+                    (true, false) | (false, false) => {
+                        // => consider all possible votes and see if
+                        //    no votes meet threshold.
+                        let options = self.total_power - self.votes.abstain;
+
+                        // If there is a 100% passing threshold..
+                        if threshold == Decimal::percent(100) {
+                            if options == Uint128::zero() {
+                                // and there are no possible votes (zero
+                                // voting power or all abstain), then this
+                                // proposal has been rejected.
+                                return true;
+                            } else {
+                                // and there are possible votes, then this is
+                                // rejected if there is a single no vote.
+                                //
+                                // We need this check becuase otherwise
+                                // when we invert the threshold
+                                // (`Decimal::one() - threshold`) we
+                                // get a 0% requirement for no
+                                // votes. Zero no votes do indeed meet
+                                // a 0% threshold.
+                                return self.votes.no >= Uint128::new(1);
+                            }
+                        }
+
+                        votes_meet_threshold(self.votes.no, options, Decimal::one() - threshold)
+                    }
+                    // Hasn't met quorum requirement and voting has closed => rejected.
+                    (false, true) => true,
                 }
             }
         }
@@ -411,8 +500,11 @@ mod test {
             Uint128::new(30),
             true
         ));
-        // Under quorum means it cannot be rejected
-        assert!(!check_is_rejected(
+        // Total power of 33. 13 total votes. 8 no votes, 3 yes, 2
+        // abstain. 39.3% turnout. Expired. As it is expired we see if
+        // the 8 no votes excede the 50% failing threshold, which they
+        // do.
+        assert!(check_is_rejected(
             quorum.clone(),
             rejecting.clone(),
             Uint128::new(33),
diff --git a/packages/voting/src/lib.rs b/packages/voting/src/lib.rs
index 281ff21a..f22c2557 100644
--- a/packages/voting/src/lib.rs
+++ b/packages/voting/src/lib.rs
@@ -42,17 +42,7 @@ pub enum VotesNeeded {
 /// Note that it is possible, though unlikely, that no number of votes
 /// will ever meet the threshold. This happens if the total power is
 /// zero. For example, this may happen if all votes are abstain.
-pub fn votes_needed(total_power: Uint128, passing_percentage: Decimal) -> VotesNeeded {
-    // If there are no votes possible then no threshold will ever be
-    // reachable except for the case where there is a zero percent
-    // passing percentage in which case all vote counts should pass.
-    //
-    // A zero percent passing threshold is not actually allowed in the
-    // `validate_percentage` function so we should really never hit
-    // that case.
-    if total_power.is_zero() && !passing_percentage.is_zero() {
-        return VotesNeeded::Unreachable;
-    }
+pub fn votes_needed(total_power: Uint128, passing_percentage: Decimal) -> Uint128 {
     // Voting power is counted with a Uint128. In order to avoid an
     // overflow while multiplying by the percision factor we need to
     // do a full mul which results in a Uint256.
@@ -96,8 +86,7 @@ pub fn votes_needed(total_power: Uint128, passing_percentage: Decimal) -> VotesN
     // This number is 2^128 which will fit into a 128 bit
     // integer. Note: if we didn't floor here this would not be the
     // case, happily unsigned integer division does indeed do that.
-    let truncated: Uint128 = rounded.try_into().unwrap();
-    VotesNeeded::Finite(truncated)
+    rounded.try_into().unwrap()
 }
 
 /// Determines if a number of votes meets the provided threshold given
@@ -107,10 +96,7 @@ pub fn votes_meet_threshold(
     total_power: Uint128,
     passing_percentage: Decimal,
 ) -> bool {
-    match votes_needed(total_power, passing_percentage) {
-        VotesNeeded::Finite(needed) => votes >= needed,
-        VotesNeeded::Unreachable => false,
-    }
+    votes >= votes_needed(total_power, passing_percentage)
 }
 
 impl Votes {
@@ -186,48 +172,53 @@ mod test {
     fn votes_needed_rounds_properly() {
         // round up right below 1
         assert_eq!(
-            VotesNeeded::Finite(Uint128::new(1)),
+            Uint128::new(1),
             votes_needed(Uint128::new(3), Decimal::permille(333))
         );
         // round up right over 1
         assert_eq!(
-            VotesNeeded::Finite(Uint128::new(2)),
+            Uint128::new(2),
             votes_needed(Uint128::new(3), Decimal::permille(334))
         );
         assert_eq!(
-            VotesNeeded::Finite(Uint128::new(11)),
+            Uint128::new(11),
             votes_needed(Uint128::new(30), Decimal::permille(334))
         );
 
         // exact matches don't round
         assert_eq!(
-            VotesNeeded::Finite(Uint128::new(17)),
+            Uint128::new(17),
             votes_needed(Uint128::new(34), Decimal::percent(50))
         );
         assert_eq!(
-            VotesNeeded::Finite(Uint128::new(12)),
+            Uint128::new(12),
             votes_needed(Uint128::new(48), Decimal::percent(25))
         );
 
         assert_eq!(
-            VotesNeeded::Finite(Uint128::new(7)),
+            Uint128::new(7),
             votes_needed(Uint128::new(13), Decimal::percent(50))
         );
 
         assert_eq!(
-            VotesNeeded::Unreachable,
+            Uint128::zero(),
             votes_needed(Uint128::zero(), Decimal::percent(50))
         );
 
         assert_eq!(
-            VotesNeeded::Finite(Uint128::new(1)),
+            Uint128::new(1),
             votes_needed(Uint128::new(1), Decimal::percent(1))
         );
 
         assert_eq!(
-            VotesNeeded::Finite(Uint128::new(0)),
+            Uint128::new(0),
             votes_needed(Uint128::new(1), Decimal::percent(0))
         );
+
+        assert_eq!(
+            Uint128::zero(),
+            votes_needed(Uint128::zero(), Decimal::percent(99))
+        );
     }
 
     #[test]
@@ -240,7 +231,8 @@ mod test {
                 threshold
             ))
         }
-        assert!(!votes_meet_threshold(
+        // Zero votes out of zero total power meet any threshold.
+        assert!(votes_meet_threshold(
             Uint128::new(0),
             Uint128::new(0),
             threshold


Test patch:
diff --git a/contracts/cw-govmod-single/src/tests.rs b/contracts/cw-govmod-single/src/tests.rs
index a97bd6b9..e22bce37 100644
--- a/contracts/cw-govmod-single/src/tests.rs
+++ b/contracts/cw-govmod-single/src/tests.rs
@@ -454,7 +454,82 @@ fn test_vote_abstain_only() {
         Threshold::AbsolutePercentage {
             percentage: Decimal::percent(100),
         },
-        Status::Open,
+        Status::Passed,
+        None,
+        None,
+    );
+
+    // The quorum shouldn't matter here in determining if the vote is
+    // rejected.
+    for i in 0..101 {
+        do_test_votes(
+            vec![TestVote {
+                voter: "ekez".to_string(),
+                position: Vote::Abstain,
+                weight: Uint128::new(u128::max_value()),
+                should_execute: ShouldExecute::Yes,
+            }],
+            Threshold::ThresholdQuorum {
+                threshold: Decimal::percent(100),
+                quorum: Decimal::percent(i),
+            },
+            Status::Passed,
+            None,
+            None,
+        );
+    }
+}
+
+#[test]
+fn test_single_no() {
+    do_test_votes(
+        vec![TestVote {
+            voter: "ekez".to_string(),
+            position: Vote::No,
+            weight: Uint128::new(1),
+            should_execute: ShouldExecute::Yes,
+        }],
+        Threshold::AbsolutePercentage {
+            percentage: Decimal::percent(100),
+        },
+        Status::Rejected,
+        Some(Uint128::from(u128::max_value())),
+        None,
+    );
+}
+
+#[test]
+fn test_tricky_rounding() {
+    // This tests the smallest possible round up for passing
+    // thresholds we can have. Specifically, a 1% passing threshold
+    // and 1 total vote. This should round up and only pass if there
+    // are more than 1 yes votes.
+    do_test_votes(
+        vec![TestVote {
+            voter: "ekez".to_string(),
+            position: Vote::Yes,
+            weight: Uint128::new(1),
+            should_execute: ShouldExecute::Yes,
+        }],
+        Threshold::AbsolutePercentage {
+            percentage: Decimal::percent(1),
+        },
+        Status::Passed,
+        None,
+        None,
+    );
+
+    do_test_votes(
+        vec![TestVote {
+            voter: "ekez".to_string(),
+            position: Vote::Abstain,
+            weight: Uint128::new(1),
+            should_execute: ShouldExecute::Yes,
+        }],
+        Threshold::AbsolutePercentage {
+            percentage: Decimal::percent(1),
+        },
+        Status::Passed,
         None,
         None,
     );
@@ -480,6 +555,13 @@ fn test_no_double_votes() {
         Threshold::AbsolutePercentage {
             percentage: Decimal::percent(100),
         },
+        // NOTE: Updating our cw20-base version will cause this to
+        // fail. In versions of cw20-base before Feb 15 2022 (the one
+        // we use at the time of writing) it was allowed to have an
+        // initial balance that repeats for a given address but it
+        // would cause miscalculation of the total supply. In this
+        // case the total supply is miscumputed to be 4 so this is
+        // assumed to have 2 abstain votes out of 4 possible votes.
         Status::Open,
         None,
         None,
@@ -623,7 +705,9 @@ fn test_pass_threshold_not_quorum() {
             threshold: Decimal::percent(50),
             quorum: Decimal::percent(60),
         },
-        Status::Open,
+        // As the threshold is 50% and 59% of voters have voted no
+        // this is unable to pass.
+        Status::Rejected,
         Some(Uint128::new(100)),
         None,
     );