From 5cbd541dfbc62c298613da601305f63ab22b8942 Mon Sep 17 00:00:00 2001 From: Noah Saso Date: Mon, 17 Feb 2025 13:11:17 -0500 Subject: [PATCH] fixed individual vote count revote bug --- Cargo.lock | 1 + .../delegation/dao-vote-delegation/Cargo.toml | 1 + .../dao-vote-delegation/src/testing/tests.rs | 182 ++++++++++++++++++ .../dao-proposal-multiple/src/contract.rs | 4 +- .../dao-proposal-multiple/src/proposal.rs | 6 +- .../src/testing/adversarial_tests.rs | 8 +- .../src/testing/tests.rs | 39 +--- .../dao-proposal-single/src/contract.rs | 4 +- packages/dao-testing/src/suite/base.rs | 131 +++++++++++++ packages/dao-voting/src/multiple_choice.rs | 16 +- packages/dao-voting/src/proposal.rs | 4 + 11 files changed, 355 insertions(+), 41 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2a213e804..82254ffc0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2397,6 +2397,7 @@ dependencies = [ "cw721-base 0.18.0", "dao-hooks 2.7.0-alpha.2", "dao-interface 2.7.0-alpha.2", + "dao-proposal-multiple 2.7.0-alpha.2", "dao-proposal-single 2.7.0-alpha.2", "dao-proposal-sudo", "dao-testing", diff --git a/contracts/delegation/dao-vote-delegation/Cargo.toml b/contracts/delegation/dao-vote-delegation/Cargo.toml index ba734ca9a..052736422 100644 --- a/contracts/delegation/dao-vote-delegation/Cargo.toml +++ b/contracts/delegation/dao-vote-delegation/Cargo.toml @@ -47,3 +47,4 @@ dao-voting-cw721-staked = { workspace = true, features = ["library"] } dao-testing = { workspace = true } dao-proposal-sudo = { workspace = true } dao-proposal-single = { workspace = true } +dao-proposal-multiple = { workspace = true } diff --git a/contracts/delegation/dao-vote-delegation/src/testing/tests.rs b/contracts/delegation/dao-vote-delegation/src/testing/tests.rs index 56f201f67..7d819972b 100644 --- a/contracts/delegation/dao-vote-delegation/src/testing/tests.rs +++ b/contracts/delegation/dao-vote-delegation/src/testing/tests.rs @@ -3,6 +3,7 @@ use cosmwasm_std::{ to_json_binary, Addr, Decimal, Empty, Uint128, }; use cw_multi_test::{Contract, ContractWrapper}; +use cw_utils::Duration; use dao_interface::helpers::OptionalUpdate; use dao_testing::{ADDR0, ADDR1, ADDR2, ADDR3, ADDR4}; @@ -1499,3 +1500,184 @@ fn test_gas_limits() { ); } } + +#[test] +fn test_revote() { + let mut suite = Cw4DaoVoteDelegationTestingSuite::new().build(); + let dao = suite.dao.clone(); + + // Enable revoting on both proposal modules. + suite + .execute_smart( + &dao.core_addr, + &dao.proposal_modules[0].1, + &dao_proposal_single::msg::ExecuteMsg::UpdateConfig { + threshold: dao_voting::threshold::Threshold::AbsolutePercentage { + percentage: dao_voting::threshold::PercentageThreshold::Majority {}, + }, + max_voting_period: Duration::Height(10), + min_voting_period: None, + only_members_execute: true, + allow_revoting: true, + dao: dao.core_addr.to_string(), + close_proposal_on_execution_failure: true, + veto: None, + }, + &[], + ) + .unwrap(); + suite + .execute_smart( + &dao.core_addr, + &dao.proposal_modules[1].1, + &dao_proposal_multiple::msg::ExecuteMsg::UpdateConfig { + voting_strategy: dao_voting::multiple_choice::VotingStrategy::SingleChoice { + quorum: dao_voting::threshold::PercentageThreshold::Majority {}, + }, + max_voting_period: Duration::Height(10), + min_voting_period: None, + only_members_execute: true, + allow_revoting: true, + dao: dao.core_addr.to_string(), + close_proposal_on_execution_failure: true, + veto: None, + }, + &[], + ) + .unwrap(); + + // register member as delegate + suite.register(ADDR0); + + // delegate 100% of voting power to ADDR0 + suite.delegate(ADDR1, ADDR0, Decimal::percent(100)); + + // delegations take effect on the next block + suite.advance_block(); + + // SINGLE CHOICE PROPOSAL + + // propose a single choice proposal + let (proposal_module, proposal_id, _) = + suite.propose_single_choice(&dao, ADDR0, "test proposal", vec![]); + + // delegate votes on the proposal + suite.vote_single_choice(&dao, ADDR0, proposal_id, dao_voting::voting::Vote::Yes); + + // assert vote count on single choice proposal + suite.assert_single_choice_votes_count( + &proposal_module, + proposal_id, + dao_voting::voting::Vote::Yes, + // both members' weight is used by delegate + suite.members[0].weight + suite.members[1].weight, + ); + // assert individual vote count on single choice proposal + suite.assert_single_choice_individual_votes_count( + &proposal_module, + proposal_id, + dao_voting::voting::Vote::Yes, + // only delegate weight is counted in individual votes + suite.members[0].weight, + ); + + // revote on the proposal + suite.vote_single_choice(&dao, ADDR0, proposal_id, dao_voting::voting::Vote::No); + + // assert vote count on single choice proposal + suite.assert_single_choice_votes_count( + &proposal_module, + proposal_id, + dao_voting::voting::Vote::Yes, + 0u128, + ); + suite.assert_single_choice_votes_count( + &proposal_module, + proposal_id, + dao_voting::voting::Vote::No, + // both members' weight is used by delegate + suite.members[0].weight + suite.members[1].weight, + ); + // assert individual vote count on single choice proposal + suite.assert_single_choice_individual_votes_count( + &proposal_module, + proposal_id, + dao_voting::voting::Vote::Yes, + 0u128, + ); + suite.assert_single_choice_individual_votes_count( + &proposal_module, + proposal_id, + dao_voting::voting::Vote::No, + // only delegate weight is counted in individual votes + suite.members[0].weight, + ); + + // MULTIPLE CHOICE PROPOSAL + + // propose a multiple choice proposal + let (proposal_module, proposal_id, _) = suite.propose_multiple_choice( + &dao, + ADDR0, + "test proposal", + vec![ + dao_voting::multiple_choice::MultipleChoiceOption { + title: "Option 1".to_string(), + description: "Option 1 description".to_string(), + msgs: vec![], + }, + dao_voting::multiple_choice::MultipleChoiceOption { + title: "Option 2".to_string(), + description: "Option 2 description".to_string(), + msgs: vec![], + }, + dao_voting::multiple_choice::MultipleChoiceOption { + title: "Option 3".to_string(), + description: "Option 3 description".to_string(), + msgs: vec![], + }, + ], + ); + + // delegate votes on the proposal + suite.vote_multiple_choice(&dao, ADDR0, proposal_id, 1); + + // assert vote count on multiple choice proposal + suite.assert_multiple_choice_votes_count( + &proposal_module, + proposal_id, + 1, + // both members' weight is used by delegate + suite.members[0].weight + suite.members[1].weight, + ); + // assert individual vote count on multiple choice proposal + suite.assert_multiple_choice_individual_votes_count( + &proposal_module, + proposal_id, + 1, + // only delegate weight is counted in individual votes + suite.members[0].weight, + ); + + // revote on the proposal + suite.vote_multiple_choice(&dao, ADDR0, proposal_id, 2); + + // assert vote count on multiple choice proposal + suite.assert_multiple_choice_votes_count(&proposal_module, proposal_id, 1, 0u128); + suite.assert_multiple_choice_votes_count( + &proposal_module, + proposal_id, + 2, + // both members' weight is used by delegate + suite.members[0].weight + suite.members[1].weight, + ); + // assert individual vote count on multiple choice proposal + suite.assert_multiple_choice_individual_votes_count(&proposal_module, proposal_id, 1, 0u128); + suite.assert_multiple_choice_individual_votes_count( + &proposal_module, + proposal_id, + 2, + // only delegate weight is counted in individual votes + suite.members[0].weight, + ); +} diff --git a/contracts/proposal/dao-proposal-multiple/src/contract.rs b/contracts/proposal/dao-proposal-multiple/src/contract.rs index fa2996e4b..86707ea4e 100644 --- a/contracts/proposal/dao-proposal-multiple/src/contract.rs +++ b/contracts/proposal/dao-proposal-multiple/src/contract.rs @@ -435,9 +435,10 @@ pub fn execute_vote( prop.votes .remove_vote(current_ballot.vote, current_ballot.power)?; prop.individual_votes - .remove_vote(current_ballot.vote, current_ballot.power)?; + .remove_vote(current_ballot.vote, current_ballot.individual_power)?; Ok(Ballot { power: vote_power.total, + individual_power: vote_power.individual, vote, rationale: rationale.clone(), }) @@ -449,6 +450,7 @@ pub fn execute_vote( None => Ok(Ballot { vote, power: vote_power.total, + individual_power: vote_power.individual, rationale: rationale.clone(), }), })?; diff --git a/contracts/proposal/dao-proposal-multiple/src/proposal.rs b/contracts/proposal/dao-proposal-multiple/src/proposal.rs index 63c9f51fd..a507703fd 100644 --- a/contracts/proposal/dao-proposal-multiple/src/proposal.rs +++ b/contracts/proposal/dao-proposal-multiple/src/proposal.rs @@ -314,7 +314,7 @@ impl MultipleChoiceProposal { votes_to_consider: &MultipleChoiceVotes, winning_choice: &CheckedMultipleChoiceOption, ) -> StdResult { - let winning_choice_power = votes_to_consider.vote_weights[winning_choice.index as usize]; + let winning_choice_power = votes_to_consider.get_id(winning_choice.index); if let Some(second_choice_power) = votes_to_consider .vote_weights .iter() @@ -841,7 +841,7 @@ mod tests { ); // Everyone voted and proposal is in a tie... assert_eq!(prop.total_power, prop.votes.total()); - assert_eq!(prop.votes.vote_weights[0], prop.votes.vote_weights[1]); + assert_eq!(prop.votes.get_id(0), prop.votes.get_id(1)); // ... but proposal is still active => no rejection assert!(!prop.is_rejected(&env.block).unwrap()); @@ -854,7 +854,7 @@ mod tests { true, ); // Proposal has expired and ended in a tie => rejection - assert_eq!(prop.votes.vote_weights[0], prop.votes.vote_weights[1]); + assert_eq!(prop.votes.get_id(0), prop.votes.get_id(1)); assert!(prop.is_rejected(&env.block).unwrap()); } diff --git a/contracts/proposal/dao-proposal-multiple/src/testing/adversarial_tests.rs b/contracts/proposal/dao-proposal-multiple/src/testing/adversarial_tests.rs index 483b89759..2c9f6e597 100644 --- a/contracts/proposal/dao-proposal-multiple/src/testing/adversarial_tests.rs +++ b/contracts/proposal/dao-proposal-multiple/src/testing/adversarial_tests.rs @@ -361,8 +361,8 @@ pub fn test_allow_voting_after_proposal_execution_pre_expiration_cw20() { // assert proposal is passed with expected votes let prop = query_proposal(&app, &proposal_module, proposal_id); assert_eq!(prop.proposal.status, Status::Passed); - assert_eq!(prop.proposal.votes.vote_weights[0], Uint128::new(100000000)); - assert_eq!(prop.proposal.votes.vote_weights[1], Uint128::new(0)); + assert_eq!(prop.proposal.votes.get_id(0), Uint128::new(100000000)); + assert_eq!(prop.proposal.votes.get_id(1), Uint128::new(0)); // someone wakes up and casts their vote to express their // opinion (not affecting the result of proposal) @@ -384,8 +384,8 @@ pub fn test_allow_voting_after_proposal_execution_pre_expiration_cw20() { // assert proposal is passed with expected votes let prop = query_proposal(&app, &proposal_module, proposal_id); assert_eq!(prop.proposal.status, Status::Passed); - assert_eq!(prop.proposal.votes.vote_weights[0], Uint128::new(100000000)); - assert_eq!(prop.proposal.votes.vote_weights[1], Uint128::new(50000000)); + assert_eq!(prop.proposal.votes.get_id(0), Uint128::new(100000000)); + assert_eq!(prop.proposal.votes.get_id(1), Uint128::new(50000000)); // execute the proposal expecting app.execute_contract( diff --git a/contracts/proposal/dao-proposal-multiple/src/testing/tests.rs b/contracts/proposal/dao-proposal-multiple/src/testing/tests.rs index 0b64c8a6b..af7ac3a91 100644 --- a/contracts/proposal/dao-proposal-multiple/src/testing/tests.rs +++ b/contracts/proposal/dao-proposal-multiple/src/testing/tests.rs @@ -3533,14 +3533,8 @@ fn test_revoting() { // Assert that both vote options have equal vote weights at some block let proposal: ProposalResponse = query_proposal(&app, &govmod, 1); assert_eq!(proposal.proposal.status, Status::Open); - assert_eq!( - proposal.proposal.votes.vote_weights[0], - Uint128::new(100_000_000), - ); - assert_eq!( - proposal.proposal.votes.vote_weights[1], - Uint128::new(100_000_000), - ); + assert_eq!(proposal.proposal.votes.get_id(0), Uint128::new(100_000_000),); + assert_eq!(proposal.proposal.votes.get_id(1), Uint128::new(100_000_000),); // More time passes.. app.update_block(|b| b.height += 3); @@ -3564,11 +3558,8 @@ fn test_revoting() { // Assert that revote succeeded let proposal: ProposalResponse = query_proposal(&app, &govmod, 1); assert_eq!(proposal.proposal.status, Status::Passed); - assert_eq!( - proposal.proposal.votes.vote_weights[0], - Uint128::new(200_000_000), - ); - assert_eq!(proposal.proposal.votes.vote_weights[1], Uint128::new(0),); + assert_eq!(proposal.proposal.votes.get_id(0), Uint128::new(200_000_000),); + assert_eq!(proposal.proposal.votes.get_id(1), Uint128::new(0),); } /// Tests that revoting is stored at a per-proposal level. @@ -3920,14 +3911,8 @@ fn test_invalid_revote_does_not_invalidate_initial_vote() { // Assert that both vote options have equal vote weights at some block let proposal: ProposalResponse = query_proposal(&app, &proposal_module, 1); assert_eq!(proposal.proposal.status, Status::Open); - assert_eq!( - proposal.proposal.votes.vote_weights[0], - Uint128::new(100_000_000), - ); - assert_eq!( - proposal.proposal.votes.vote_weights[1], - Uint128::new(100_000_000), - ); + assert_eq!(proposal.proposal.votes.get_id(0), Uint128::new(100_000_000),); + assert_eq!(proposal.proposal.votes.get_id(1), Uint128::new(100_000_000),); // Time passes.. app.update_block(|b| b.height += 3); @@ -3949,14 +3934,8 @@ fn test_invalid_revote_does_not_invalidate_initial_vote() { .downcast() .unwrap(); // Assert that prior votes remained the same - assert_eq!( - proposal.proposal.votes.vote_weights[0], - Uint128::new(100_000_000), - ); - assert_eq!( - proposal.proposal.votes.vote_weights[1], - Uint128::new(100_000_000), - ); + assert_eq!(proposal.proposal.votes.get_id(0), Uint128::new(100_000_000),); + assert_eq!(proposal.proposal.votes.get_id(1), Uint128::new(100_000_000),); assert!(matches!(err, ContractError::InvalidVote {})); } @@ -4550,7 +4529,7 @@ pub fn test_not_allow_voting_on_expired_proposal() { // assert the vote got rejected and did not count towards the votes let proposal = query_proposal(&app, &proposal_module, 1); assert_eq!(proposal.proposal.status, Status::Rejected); - assert_eq!(proposal.proposal.votes.vote_weights[0], Uint128::zero()); + assert_eq!(proposal.proposal.votes.get_id(0), Uint128::zero()); assert!(matches!(err, ContractError::Expired { id: _proposal_id })); } diff --git a/contracts/proposal/dao-proposal-single/src/contract.rs b/contracts/proposal/dao-proposal-single/src/contract.rs index 0b27de547..deec8264b 100644 --- a/contracts/proposal/dao-proposal-single/src/contract.rs +++ b/contracts/proposal/dao-proposal-single/src/contract.rs @@ -539,9 +539,10 @@ pub fn execute_vote( prop.votes .remove_vote(current_ballot.vote, current_ballot.power); prop.individual_votes - .remove_vote(current_ballot.vote, current_ballot.power); + .remove_vote(current_ballot.vote, current_ballot.individual_power); Ok(Ballot { power: vote_power.total, + individual_power: vote_power.individual, vote, // Roll over the previous rationale. If // you're changing your vote, you've also @@ -555,6 +556,7 @@ pub fn execute_vote( } None => Ok(Ballot { power: vote_power.total, + individual_power: vote_power.individual, vote, rationale: rationale.clone(), }), diff --git a/packages/dao-testing/src/suite/base.rs b/packages/dao-testing/src/suite/base.rs index a389ace43..2b29927e4 100644 --- a/packages/dao-testing/src/suite/base.rs +++ b/packages/dao-testing/src/suite/base.rs @@ -557,6 +557,72 @@ impl DaoTestingSuiteBase { ); } + /// propose a multiple choice proposal and return the proposal module + /// address, proposal ID, and proposal + pub fn propose_multiple_choice( + &mut self, + dao: &TestDao, + proposer: impl Into, + title: impl Into, + options: Vec, + ) -> ( + Addr, + u64, + dao_proposal_multiple::proposal::MultipleChoiceProposal, + ) { + let pre_propose_msg = dao_pre_propose_multiple::ExecuteMsg::Propose { + msg: dao_pre_propose_multiple::ProposeMessage::Propose { + title: title.into(), + description: "".to_string(), + vote: None, + choices: dao_voting::multiple_choice::MultipleChoiceOptions { options }, + }, + }; + + let (pre_propose_module, proposal_module) = &dao.proposal_modules[1]; + + self.execute_smart_ok( + proposer, + pre_propose_module.as_ref().unwrap(), + &pre_propose_msg, + &[], + ); + + let proposal_id: u64 = self + .querier() + .query_wasm_smart( + proposal_module.clone(), + &dao_proposal_multiple::msg::QueryMsg::ProposalCount {}, + ) + .unwrap(); + + let proposal = self.get_multiple_choice_proposal(proposal_module, proposal_id); + + (proposal_module.clone(), proposal_id, proposal) + } + + /// vote on a multiple choice proposal + pub fn vote_multiple_choice( + &mut self, + dao: &TestDao, + voter: impl Into, + proposal_id: u64, + vote_option_id: u32, + ) { + self.execute_smart_ok( + voter, + &dao.proposal_modules[1].1, + &dao_proposal_multiple::msg::ExecuteMsg::Vote { + proposal_id, + vote: dao_voting::multiple_choice::MultipleChoiceVote { + option_id: vote_option_id, + }, + rationale: None, + }, + &[], + ); + } + /// add vote hook to all proposal modules pub fn add_vote_hook(&mut self, dao: &TestDao, addr: impl Into) { let address = addr.into(); @@ -614,6 +680,21 @@ impl DaoTestingSuiteBase { .proposal } + /// get a multiple choice proposal + pub fn get_multiple_choice_proposal( + &self, + proposal_module: impl Into, + proposal_id: u64, + ) -> dao_proposal_multiple::proposal::MultipleChoiceProposal { + self.querier() + .query_wasm_smart::( + Addr::unchecked(proposal_module), + &dao_proposal_multiple::msg::QueryMsg::Proposal { proposal_id }, + ) + .unwrap() + .proposal + } + /// assert vote count on single choice proposal pub fn assert_single_choice_votes_count( &self, @@ -626,6 +707,18 @@ impl DaoTestingSuiteBase { assert_eq!(proposal.votes.get(vote), count.into()); } + /// assert individual vote count on single choice proposal + pub fn assert_single_choice_individual_votes_count( + &self, + proposal_module: impl Into, + proposal_id: u64, + vote: dao_voting::voting::Vote, + count: impl Into, + ) { + let proposal = self.get_single_choice_proposal(proposal_module, proposal_id); + assert_eq!(proposal.individual_votes.get(vote), count.into()); + } + /// assert status on single choice proposal pub fn assert_single_choice_status( &self, @@ -636,4 +729,42 @@ impl DaoTestingSuiteBase { let proposal = self.get_single_choice_proposal(proposal_module, proposal_id); assert_eq!(proposal.status, status); } + + /// assert vote count on multiple choice proposal + pub fn assert_multiple_choice_votes_count( + &self, + proposal_module: impl Into, + proposal_id: u64, + vote_option_id: u32, + count: impl Into, + ) { + let proposal = self.get_multiple_choice_proposal(proposal_module, proposal_id); + assert_eq!(proposal.votes.get_id(vote_option_id), count.into()); + } + + /// assert individual vote count on multiple choice proposal + pub fn assert_multiple_choice_individual_votes_count( + &self, + proposal_module: impl Into, + proposal_id: u64, + vote_option_id: u32, + count: impl Into, + ) { + let proposal = self.get_multiple_choice_proposal(proposal_module, proposal_id); + assert_eq!( + proposal.individual_votes.get_id(vote_option_id), + count.into() + ); + } + + /// assert status on multiple choice proposal + pub fn assert_multiple_choice_status( + &self, + proposal_module: impl Into, + proposal_id: u64, + status: dao_voting::status::Status, + ) { + let proposal = self.get_multiple_choice_proposal(proposal_module, proposal_id); + assert_eq!(proposal.status, status); + } } diff --git a/packages/dao-voting/src/multiple_choice.rs b/packages/dao-voting/src/multiple_choice.rs index a096281dd..63dd1fc5c 100644 --- a/packages/dao-voting/src/multiple_choice.rs +++ b/packages/dao-voting/src/multiple_choice.rs @@ -58,7 +58,8 @@ impl MultipleChoiceVotes { // Add a vote to the tally pub fn add_vote(&mut self, vote: MultipleChoiceVote, weight: Uint128) -> StdResult<()> { - self.vote_weights[vote.option_id as usize] = self.vote_weights[vote.option_id as usize] + self.vote_weights[vote.option_id as usize] = self + .get(vote) .checked_add(weight) .map_err(StdError::overflow)?; Ok(()) @@ -66,7 +67,8 @@ impl MultipleChoiceVotes { // Remove a vote from the tally pub fn remove_vote(&mut self, vote: MultipleChoiceVote, weight: Uint128) -> StdResult<()> { - self.vote_weights[vote.option_id as usize] = self.vote_weights[vote.option_id as usize] + self.vote_weights[vote.option_id as usize] = self + .get(vote) .checked_sub(weight) .map_err(StdError::overflow)?; Ok(()) @@ -78,6 +80,16 @@ impl MultipleChoiceVotes { vote_weights: vec![Uint128::zero(); num_choices], } } + + /// Returns the number of votes for a given vote option. + pub fn get(&self, vote: MultipleChoiceVote) -> Uint128 { + self.get_id(vote.option_id) + } + + /// Returns the number of votes for a given vote option ID. + pub fn get_id(&self, id: u32) -> Uint128 { + self.vote_weights[id as usize] + } } /// Represents the type of Multiple choice option. "None of the above" has a special diff --git a/packages/dao-voting/src/proposal.rs b/packages/dao-voting/src/proposal.rs index d3f2f8d66..0a2484dde 100644 --- a/packages/dao-voting/src/proposal.rs +++ b/packages/dao-voting/src/proposal.rs @@ -75,6 +75,10 @@ pub struct Ballot { /// The amount of voting power behind the vote, including any delegated VP. /// This is the amount tallied in the proposal for this ballot. pub power: Uint128, + /// The amount of individual voting power behind the vote, excluding any + /// delegated VP. This is the amount counted in individual votes and used to + /// determine if the proposal finished early. + pub individual_power: Uint128, /// The position. pub vote: Vote,