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

Backport multisig fixes to voting contracts #178

Merged
merged 5 commits into from
Sep 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions packages/voting-contract/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,15 @@ where
// ensure proposal exists and can be voted on
let mut prop = proposals().load(deps.storage, proposal_id)?;

if prop.current_status(&env.block) != Status::Open {
if ![Status::Open, Status::Passed, Status::Rejected].contains(&prop.status) {
return Err(ContractError::NotOpen {});
}

// if they are not expired
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
let cfg = CONFIG.load(deps.storage)?;
Expand Down Expand Up @@ -157,7 +162,8 @@ where
P: Serialize + DeserializeOwned,
{
let mut proposal = proposals::<P>().load(storage, proposal_id)?;

// Update Status
proposal.update_status(&env.block);
Comment on lines +165 to +166
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed. Status will be updated below, and the check is already using current_status.

Copy link
Contributor

@maurolacy maurolacy Sep 6, 2022

Choose a reason for hiding this comment

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

Funny how we have two, three or even more different implementations / variations of the same logic.

It'll be good to move this voting-contract package to cw-plus at some point, and change all multisigs (cw-plus, poe-contracts, and tgrade-contracts) to use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Created See #194 to track that effort.

// We allow execution even after the proposal "expiration" as long as all votes come in before
// that point. If it was approved on time, it can be executed any time.
if proposal.current_status(&env.block) != Status::Passed {
Expand Down
47 changes: 46 additions & 1 deletion packages/voting-contract/src/multitest/voting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -368,5 +368,50 @@ 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();
// proposal that is open and expired is rejected
assert_eq!(ContractError::NotOpen {}, err.downcast().unwrap());
assert_eq!(ContractError::Expired {}, err.downcast().unwrap());
}

#[test]
fn proposal_pass_on_expiration() {
let rules = RulesBuilder::new()
.with_threshold(Decimal::percent(51))
.with_quorum(Decimal::percent(35))
.build();

let mut suite = SuiteBuilder::new()
.with_member("alice", 1)
.with_member("bob", 2)
.with_rules(rules.clone())
.build();

// Create proposal with 1 voting power
let response = suite.propose("alice", "cool proposal", "proposal").unwrap();
let proposal_id: u64 = get_proposal_id(&response).unwrap();

// Bob can vote on the proposal
suite.vote("bob", proposal_id, Vote::Yes).unwrap();

// Move time forward so voting ends
suite.app.advance_seconds(rules.voting_period_secs());

// Verify proposal is now passed
let prop = suite.query_proposal(proposal_id).unwrap();
assert_eq!(prop.status, Status::Passed);

// Alice can't vote on the proposal
let err = suite.vote("alice", proposal_id, Vote::Yes).unwrap_err();

// cannot vote on proposal as it has expired
assert_eq!(ContractError::Expired {}, err.downcast().unwrap());

// But she can execute the proposal
suite.execute_proposal("alice", proposal_id).unwrap();

// Verify proposal is now 'executed'
let prop = suite.query_proposal(proposal_id).unwrap();
assert_eq!(prop.status, Status::Executed);

// Closing should NOT be possible
let err = suite.close("bob", proposal_id).unwrap_err();
assert_eq!(ContractError::WrongCloseStatus {}, err.downcast().unwrap());
}