Voting Contract: Slightly refactor implementation, so that during vot…

…ing and closing current status is used and saved

packages/voting-contract/src/error.rs

@@ -24,6 +24,9 @@ pub enum ContractError {
     #[error("Proposal voting period has expired")]
     Expired {},
 
+    #[error("Proposal is already closed - rejected, because it was open and expired")]
+    Rejected {},
+
     #[error("Proposal must expire before you can close it")]
     NotExpired {},
 

packages/voting-contract/src/lib.rs

@@ -116,12 +116,14 @@ where
 {
     // ensure proposal exists and can be voted on
     let mut prop = proposals().load(deps.storage, proposal_id)?;
-    if prop.status != Status::Open {
+    prop.update_status(&env.block);
+    proposals::<P>().save(deps.storage, proposal_id, &prop)?;
+
+    if prop.status == Status::Rejected {
+        return Err(ContractError::Rejected {});
+    } else if prop.status != Status::Open {
         return Err(ContractError::NotOpen {});
     }
-    if prop.expires.is_expired(&env.block) {
-        return Err(ContractError::Expired {});
-    }
 
     // use a snapshot of "start of proposal"
     // Must be a member of voting group and have voting power >= 1
@@ -197,20 +199,34 @@ where
 {
     // anyone can trigger this if the vote passed
 
-    let mut prop = proposals::<P>().load(deps.storage, proposal_id)?;
-    if [Status::Executed, Status::Rejected, Status::Passed]
+    let mut prop = proposals().load(deps.storage, proposal_id)?;
+    let previous_status = prop.status;
+    prop.update_status(&env.block);
+    proposals::<P>().save(deps.storage, proposal_id, &prop)?;
+
+    if [Status::Executed, Status::Passed]
         .iter()
         .any(|x| *x == prop.status)
     {
         return Err(ContractError::WrongCloseStatus {});
     }
-    if !prop.expires.is_expired(&env.block) {
+    if prop.status == Status::Open {
         return Err(ContractError::NotExpired {});
     }
 
-    // set it to failed
-    prop.status = Status::Rejected;
-    proposals::<P>().save(deps.storage, proposal_id, &prop)?;
+    // Two scenarios:
+    // 1) any status -> rejected (update_status changed it)
+    // 2) proposal was already rejected (double close)
+    // To differentiate those, we need to compare current status with previous one
+    // If that condition is false, then it is first time closing this proposal
+    if prop.status == Status::Rejected && previous_status == Status::Rejected {
+        return Err(ContractError::Rejected {});
+    } else if prop.status != Status::Rejected {
+        // update_status could already set status to Rejected
+        // set it only if it is not
+        prop.status = Status::Rejected;
+        proposals::<P>().save(deps.storage, proposal_id, &prop)?;
+    };
 
     Ok(Response::new()
         .add_attribute("action", "close")

packages/voting-contract/src/multitest/closing.rs

@@ -8,6 +8,7 @@ use crate::ContractError;
 fn expired_proposals_can_be_closed() {
     let rules = RulesBuilder::new()
         .with_threshold(Decimal::percent(51))
+        .with_quorum(Decimal::percent(35))
         .build();
 
     let mut suite = SuiteBuilder::new()
@@ -39,6 +40,7 @@ fn expired_proposals_can_be_closed() {
 fn active_proposals_cannot_be_closed() {
     let rules = RulesBuilder::new()
         .with_threshold(Decimal::percent(51))
+        .with_quorum(Decimal::percent(35))
         .build();
 
     let mut suite = SuiteBuilder::new()
@@ -81,6 +83,7 @@ fn passed_proposals_cannot_be_closed() {
 fn expired_proposals_cannot_be_closed_twice() {
     let rules = RulesBuilder::new()
         .with_threshold(Decimal::percent(51))
+        .with_quorum(Decimal::percent(60))
         .build();
 
     let mut suite = SuiteBuilder::new()
@@ -100,5 +103,5 @@ fn expired_proposals_cannot_be_closed_twice() {
     suite.close("anybody", proposal_id).unwrap();
     // ...but a closed one can't be closed again
     let err = suite.close("anybody", proposal_id).unwrap_err();
-    assert_eq!(ContractError::WrongCloseStatus {}, err.downcast().unwrap());
+    assert_eq!(ContractError::Rejected {}, err.downcast().unwrap());
 }

packages/voting-contract/src/multitest/early_end.rs

@@ -51,13 +51,6 @@ fn passed_on_expiration_can_be_executed() {
     let prop = suite.query_proposal(proposal_id).unwrap();
     assert_eq!(prop.status, Status::Open);
 
-    // Proposal cannot be executed yet
-    let err = suite.execute_proposal("anybody", proposal_id).unwrap_err();
-    assert_eq!(
-        ContractError::WrongExecuteStatus {},
-        err.downcast().unwrap()
-    );
-
     // Proposal can be executed once expired
     suite.app.advance_seconds(rules.voting_period_secs());
     suite.execute_proposal("anybody", proposal_id).unwrap();

packages/voting-contract/src/multitest/voting.rs

@@ -349,6 +349,7 @@ fn passing_a_proposal_after_voting_period_works() {
 fn expired_proposals_cannot_be_voted_on() {
     let rules = RulesBuilder::new()
         .with_threshold(Decimal::percent(51))
+        .with_quorum(Decimal::percent(35))
         .build();
 
     let mut suite = SuiteBuilder::new()
@@ -366,5 +367,6 @@ fn expired_proposals_cannot_be_voted_on() {
 
     // Bob can't vote on the expired proposal
     let err = suite.vote("bob", proposal_id, Vote::Yes).unwrap_err();
-    assert_eq!(ContractError::Expired {}, err.downcast().unwrap());
+    // proposal that is open and expired is rejected
+    assert_eq!(ContractError::Rejected {}, err.downcast().unwrap());
 }

packages/voting-contract/src/state.rs

@@ -47,6 +47,58 @@ impl<P> From<Proposal<P>> for ProposalInfo {
     }
 }
 
+impl<P> Proposal<P> {
+    /// current_status is non-mutable and returns what the status should be.
+    /// (designed for queries)
+    pub fn current_status(&self, block: &BlockInfo) -> Status {
+        let mut status = self.status;
+
+        // if open, check if voting is passed or timed out
+        if status == Status::Open && self.is_passed(block) {
+            status = Status::Passed;
+        }
+        if status == Status::Open && self.expires.is_expired(block) {
+            status = Status::Rejected;
+        }
+
+        status
+    }
+
+    /// update_status sets the status of the proposal to current_status.
+    /// (designed for handler logic)
+    pub fn update_status(&mut self, block: &BlockInfo) {
+        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 {
+        let VotingRules {
+            quorum,
+            threshold,
+            allow_end_early,
+            ..
+        } = self.rules;
+
+        // we always require the quorum
+        if self.votes.total() < votes_needed(self.total_weight, quorum) {
+            return false;
+        }
+        if self.expires.is_expired(block) {
+            // If expired, we compare Yes votes against the total number of votes (minus abstain).
+            let opinions = self.votes.total() - self.votes.abstain;
+            self.votes.yes >= votes_needed(opinions, threshold)
+        } else if allow_end_early {
+            // If not expired, we must assume all non-votes will be cast as No.
+            // We compare threshold against the total weight (minus abstain).
+            let possible_opinions = self.total_weight - self.votes.abstain;
+            self.votes.yes >= votes_needed(possible_opinions, threshold)
+        } else {
+            false
+        }
+    }
+}
+
 /// Note, if you are storing custom messages in the proposal,
 /// the querier needs to know what possible custom message types
 /// those are in order to parse the response
@@ -193,58 +245,6 @@ impl Votes {
     }
 }
 
-impl<P> Proposal<P> {
-    /// current_status is non-mutable and returns what the status should be.
-    /// (designed for queries)
-    pub fn current_status(&self, block: &BlockInfo) -> Status {
-        let mut status = self.status;
-
-        // if open, check if voting is passed or timed out
-        if status == Status::Open && self.is_passed(block) {
-            status = Status::Passed;
-        }
-        if status == Status::Open && self.expires.is_expired(block) {
-            status = Status::Rejected;
-        }
-
-        status
-    }
-
-    /// update_status sets the status of the proposal to current_status.
-    /// (designed for handler logic)
-    pub fn update_status(&mut self, block: &BlockInfo) {
-        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 {
-        let VotingRules {
-            quorum,
-            threshold,
-            allow_end_early,
-            ..
-        } = self.rules;
-
-        // we always require the quorum
-        if self.votes.total() < votes_needed(self.total_weight, quorum) {
-            return false;
-        }
-        if self.expires.is_expired(block) {
-            // If expired, we compare Yes votes against the total number of votes (minus abstain).
-            let opinions = self.votes.total() - self.votes.abstain;
-            self.votes.yes >= votes_needed(opinions, threshold)
-        } else if allow_end_early {
-            // If not expired, we must assume all non-votes will be cast as No.
-            // We compare threshold against the total weight (minus abstain).
-            let possible_opinions = self.total_weight - self.votes.abstain;
-            self.votes.yes >= votes_needed(possible_opinions, threshold)
-        } else {
-            false
-        }
-    }
-}
-
 // this is a helper function so Decimal works with u64 rather than Uint128
 // also, we must *round up* here, as we need 8, not 7 votes to reach 50% of 15 total
 fn votes_needed(weight: u64, percentage: Decimal) -> u64 {