Skip to content
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

Voting Contract: vote and close uses current status #60

Merged
merged 7 commits into from
Feb 3, 2022
18 changes: 11 additions & 7 deletions packages/voting-contract/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,10 @@ where
{
// ensure proposal exists and can be voted on
let mut prop = proposals().load(deps.storage, proposal_id)?;
if prop.status != Status::Open {

if prop.current_status(&env.block) != Status::Open {
return Err(ContractError::NotOpen {});
}
if prop.expires.is_expired(&env.block) {
return Err(ContractError::Expired {});
}
ueco-jb marked this conversation as resolved.
Show resolved Hide resolved

// use a snapshot of "start of proposal"
// Must be a member of voting group and have voting power >= 1
Expand Down Expand Up @@ -197,8 +195,15 @@ 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)?;

if prop.status == Status::Rejected {
return Err(ContractError::NotOpen {});
}

prop.update_status(&env.block);
Copy link
Contributor

@maurolacy maurolacy Feb 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the key thing to do. Wouldn't this have to be done in the other cases where current_status() is being called as well? (i.e. our previous "execute proposal" fix).
That means, using current_status() only for queries, and update_status() for execution everywhere. And, "never" reading prop.status directly. More consistent and robust, I think.

We can use an update_status() impl that returns the updated status, as mentioned above (just an idea).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea with this stuff is that queries use current_status() and executions use update_status() and then everything checks out.

Yeah, you're right. I just realized, that despite status in memory not being correct, in the end it is correct for end user who uses query. We "simply" must be carefull to make sure we use that helper everywhere.

Just wrote a variant of the same above.

Copy link
Contributor

@maurolacy maurolacy Feb 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the code errors below, this status update will be discarded. And if it does not error, the status will be updated and saved later anyway. So, there's really no need to call update_status instead of current_status.

The bug is: not calling any of those, and reading the status directly.


if [Status::Executed, Status::Passed]
.iter()
.any(|x| *x == prop.status)
{
Expand All @@ -208,7 +213,6 @@ where
return Err(ContractError::NotExpired {});
}

// set it to failed
prop.status = Status::Rejected;
proposals::<P>().save(deps.storage, proposal_id, &prop)?;

Expand Down
5 changes: 4 additions & 1 deletion packages/voting-contract/src/multitest/closing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
ueco-jb marked this conversation as resolved.
Show resolved Hide resolved
.build();

let mut suite = SuiteBuilder::new()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand All @@ -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::NotOpen {}, err.downcast().unwrap());
}
4 changes: 3 additions & 1 deletion packages/voting-contract/src/multitest/voting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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::NotOpen {}, err.downcast().unwrap());
}
106 changes: 53 additions & 53 deletions packages/voting-contract/src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,58 @@ impl<P> From<Proposal<P>> for ProposalInfo {
}
}

impl<P> Proposal<P> {
ueco-jb marked this conversation as resolved.
Show resolved Hide resolved
/// 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
Expand Down Expand Up @@ -121,7 +173,7 @@ impl RulesBuilder {
pub fn new() -> Self {
Self {
voting_period: 14,
quorum: Decimal::percent(1),
quorum: Decimal::percent(20),
threshold: Decimal::percent(50),
allow_end_early: true,
}
Expand Down Expand Up @@ -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 {
Expand Down