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 4 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
12 changes: 9 additions & 3 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,10 +162,11 @@ 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 {
if proposal.status != Status::Passed {
Copy link

Choose a reason for hiding this comment

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

This is redundant, since previous call current_status is called by update_status anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted back 169 to the way it was but curious would it not be more redundant to to keep it that way as proposal.status uses the result of the first current_status call rather than what I went back to which is calling it again?

return Err(ContractError::WrongExecuteStatus {});
}

Expand Down
50 changes: 49 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,53 @@ 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
let _ = suite.vote("bob", proposal_id, Vote::Yes).unwrap();
0xFable marked this conversation as resolved.
Show resolved Hide resolved

// 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();

// proposal that is open and expired is rejected
assert_eq!(ContractError::Expired {}, err.downcast().unwrap());
Copy link

Choose a reason for hiding this comment

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

That comment is incorrect.
If the proposal was Rejected, it couldn't be closed: https://github.com/confio/poe-contracts/blob/main/packages/voting-contract/src/lib.rs#L199-L201
Can you revisit logic and adjust comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the commentcan make any further changes needed


// Move time forward more so proposal expires
suite.app.advance_seconds(prop.expires.time().seconds());

// But she can execute the proposal
let _ = suite.execute_proposal("alice", proposal_id).unwrap();
0xFable marked this conversation as resolved.
Show resolved Hide resolved

// 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());
}