From a438be5614ae01099e41a5ada977ee450977f6ae Mon Sep 17 00:00:00 2001 From: Jake Hartnell Date: Tue, 19 Sep 2023 09:20:37 -0700 Subject: [PATCH] Refactor timelock Should break less of the existing tests, doesn't lead to weird migration state (passed at time zero), and if timelock configuration is updated it won't affect currently open proposals. --- .../dao-proposal-single/src/contract.rs | 38 +++++----- .../proposal/dao-proposal-single/src/msg.rs | 2 +- .../dao-proposal-single/src/proposal.rs | 22 +++++- .../src/testing/adversarial_tests.rs | 21 +----- .../dao-proposal-single/src/testing/tests.rs | 72 ++++--------------- packages/dao-testing/src/tests.rs | 62 +++++----------- packages/dao-voting/src/status.rs | 9 ++- packages/dao-voting/src/timelock.rs | 15 ++-- 8 files changed, 91 insertions(+), 150 deletions(-) diff --git a/contracts/proposal/dao-proposal-single/src/contract.rs b/contracts/proposal/dao-proposal-single/src/contract.rs index df1373f07..31bbcec4b 100644 --- a/contracts/proposal/dao-proposal-single/src/contract.rs +++ b/contracts/proposal/dao-proposal-single/src/contract.rs @@ -218,6 +218,7 @@ pub fn execute_propose( status: Status::Open, votes: Votes::zero(), allow_revoting: config.allow_revoting, + timelock: config.timelock, }; // Update the proposal's status. Addresses case where proposal // expires on the same block as it is created. @@ -266,21 +267,19 @@ pub fn execute_veto( info: MessageInfo, proposal_id: u64, ) -> Result { - let config = CONFIG.load(deps.storage)?; - let mut prop = PROPOSALS .may_load(deps.storage, proposal_id)? .ok_or(ContractError::NoSuchProposal { id: proposal_id })?; match prop.status { - Status::Passed { at_time } => { - match config.timelock { - Some(timelock) => { - // Check if the proposal is timelocked - timelock.is_locked(at_time, env.block.time)?; + Status::Timelocked { expires } => { + match prop.timelock { + Some(ref timelock) => { + // Check if the proposal is still timelocked + timelock.check_is_locked(env.block.time, expires)?; // Check sender is vetoer - timelock.is_vetoer(&info)?; + timelock.check_is_vetoer(&info)?; let old_status = prop.status; @@ -323,7 +322,7 @@ pub fn execute_veto( .add_attribute("proposal_id", proposal_id.to_string()) .add_submessages(hooks)) } - // If timelock is not configured throw error. + // If timelock is not configured throw error. This should never happen. None => Err(ContractError::TimelockError(TimelockError::NoTimelock {})), } } @@ -358,17 +357,19 @@ pub fn execute_execute( // Check here that the proposal is passed. Allow it to be executed // even if it is expired so long as it passed during its voting // period. - let old_status = prop.status; prop.update_status(&env.block); + let old_status = prop.status; match prop.status { - Status::Passed { at_time } => { - if let Some(timelock) = config.timelock { - // Check proposal is not timelocked - timelock.is_locked(at_time, env.block.time)?; - - // If the sender is the vetoer, check if they can execute early - if timelock.is_vetoer(&info).is_ok() { - timelock.early_excute_enabled()?; + Status::Passed {} => (), + Status::Timelocked { expires } => { + if let Some(ref timelock) = prop.timelock { + // Check if the sender is the vetoer + if timelock.check_is_vetoer(&info).is_ok() { + // check if they can execute early + timelock.check_early_excute_enabled()?; + } else { + // Check if proposal is timelocked + timelock.check_is_locked(env.block.time, expires)?; } } } @@ -1011,6 +1012,7 @@ pub fn migrate(deps: DepsMut, _env: Env, msg: MigrateMsg) -> Result>, + /// The proposal status pub status: Status, + /// Votes on a particular proposal pub votes: Votes, + /// Whether or not revoting is enabled. If revoting is enabled, a proposal + /// cannot pass until the voting period has elapsed. pub allow_revoting: bool, + /// Timelock info, if configured + pub timelock: Option, } pub fn next_proposal_id(store: &dyn Storage) -> StdResult { @@ -62,8 +71,16 @@ impl SingleChoiceProposal { /// Gets the current status of the proposal. pub fn current_status(&self, block: &BlockInfo) -> Status { if self.status == Status::Open && self.is_passed(block) { - Status::Passed { - at_time: block.time, + // If time lock is configured for the proposal, calculate lock + // expiration and set status to Timelocked. + // + // Otherwise the proposal is simply passed + if let Some(timelock) = &self.timelock { + Status::Timelocked { + expires: timelock.calculate_timelock_expiration(block.time), + } + } else { + Status::Passed } } else if self.status == Status::Open && (self.expiration.is_expired(block) || self.is_rejected(block)) @@ -277,6 +294,7 @@ mod test { msgs: vec![], status: Status::Open, threshold, + timelock: None, total_power, votes, }; diff --git a/contracts/proposal/dao-proposal-single/src/testing/adversarial_tests.rs b/contracts/proposal/dao-proposal-single/src/testing/adversarial_tests.rs index 66186f97f..7c285568a 100644 --- a/contracts/proposal/dao-proposal-single/src/testing/adversarial_tests.rs +++ b/contracts/proposal/dao-proposal-single/src/testing/adversarial_tests.rs @@ -139,12 +139,7 @@ fn test_execute_proposal_more_than_once() { // assert proposal is passed, execute it let proposal = query_proposal(&app, &proposal_module, proposal_id); - assert_eq!( - proposal.proposal.status, - Status::Passed { - at_time: Timestamp::from_nanos(1571797419879305533) - } - ); + assert_eq!(proposal.proposal.status, Status::Passed {}); execute_proposal(&mut app, &proposal_module, CREATOR_ADDR, proposal_id); app.update_block(next_block); @@ -338,12 +333,7 @@ pub fn test_passed_prop_state_remains_after_vote_swing() { // assert proposal is passed with 20 votes in favor and none opposed let proposal = query_proposal(&app, &proposal_module, proposal_id); - assert_eq!( - proposal.proposal.status, - Status::Passed { - at_time: Timestamp::from_nanos(1571797419879305533) - } - ); + assert_eq!(proposal.proposal.status, Status::Passed {}); assert_eq!(proposal.proposal.votes.yes, Uint128::new(20)); assert_eq!(proposal.proposal.votes.no, Uint128::zero()); @@ -370,12 +360,7 @@ pub fn test_passed_prop_state_remains_after_vote_swing() { // assert that the late votes have been counted and proposal // is still in passed state before executing it let proposal = query_proposal(&app, &proposal_module, proposal_id); - assert_eq!( - proposal.proposal.status, - Status::Passed { - at_time: Timestamp::from_nanos(1571797419879305533) - } - ); + assert_eq!(proposal.proposal.status, Status::Passed {}); assert_eq!(proposal.proposal.votes.yes, Uint128::new(20)); assert_eq!(proposal.proposal.votes.no, Uint128::new(80)); diff --git a/contracts/proposal/dao-proposal-single/src/testing/tests.rs b/contracts/proposal/dao-proposal-single/src/testing/tests.rs index 4b291b513..6b9505206 100644 --- a/contracts/proposal/dao-proposal-single/src/testing/tests.rs +++ b/contracts/proposal/dao-proposal-single/src/testing/tests.rs @@ -131,6 +131,7 @@ fn test_simple_propose_staked_balances() { total_power: Uint128::new(100_000_000), msgs: vec![], status: Status::Open, + timelock: None, votes: Votes::zero(), }; @@ -180,6 +181,7 @@ fn test_simple_proposal_cw4_voting() { total_power: Uint128::new(1), msgs: vec![], status: Status::Open, + timelock: None, votes: Votes::zero(), }; @@ -285,6 +287,7 @@ fn test_instantiate_with_non_voting_module_cw20_deposit() { msgs: vec![], status: Status::Open, votes: Votes::zero(), + timelock: None, }; assert_eq!(created.proposal, expected); @@ -350,12 +353,7 @@ fn test_proposal_message_execution() { Vote::Yes, ); let proposal = query_proposal(&app, &proposal_module, proposal_id); - assert_eq!( - proposal.proposal.status, - Status::Passed { - at_time: Timestamp::from_nanos(1571797419879305533) - } - ); + assert_eq!(proposal.proposal.status, Status::Passed {}); // Can't use library function because we expect this to fail due // to insufficent balance in the bank module. @@ -367,12 +365,7 @@ fn test_proposal_message_execution() { ) .unwrap_err(); let proposal = query_proposal(&app, &proposal_module, proposal_id); - assert_eq!( - proposal.proposal.status, - Status::Passed { - at_time: Timestamp::from_nanos(1571797419879305533) - } - ); + assert_eq!(proposal.proposal.status, Status::Passed {}); mint_natives(&mut app, core_addr.as_str(), coins(10, "ujuno")); execute_proposal(&mut app, &proposal_module, CREATOR_ADDR, proposal_id); @@ -457,12 +450,7 @@ fn test_proposal_cant_close_after_expiry_is_passed() { // Expire the proposal. This should pass it. app.update_block(|b| b.time = b.time.plus_seconds(604800)); let proposal = query_proposal(&app, &proposal_module, proposal_id); - assert_eq!( - proposal.proposal.status, - Status::Passed { - at_time: Timestamp::from_nanos(1572402219879305533) - } - ); + assert_eq!(proposal.proposal.status, Status::Passed {},); // Make sure it can't be closed. let err = close_proposal_should_fail(&mut app, &proposal_module, CREATOR_ADDR, proposal_id); @@ -746,6 +734,7 @@ fn test_anyone_may_propose_and_proposal_listing() { no: Uint128::zero(), abstain: Uint128::zero() }, + timelock: None } } ) @@ -1079,12 +1068,7 @@ fn test_min_voting_period_no_early_pass() { app.update_block(|block| block.height += 10); let proposal_response = query_proposal(&app, &proposal_module, proposal_id); - assert_eq!( - proposal_response.proposal.status, - Status::Passed { - at_time: Timestamp::from_nanos(1571797419879305533) - } - ); + assert_eq!(proposal_response.proposal.status, Status::Passed {}); } // Setting the min duration the same as the proposal duration just @@ -1122,12 +1106,7 @@ fn test_min_duration_same_as_proposal_duration() { app.update_block(|b| b.height += 100); let proposal_response = query_proposal(&app, &proposal_module, proposal_id); - assert_eq!( - proposal_response.proposal.status, - Status::Passed { - at_time: Timestamp::from_nanos(1571797419879305533) - } - ); + assert_eq!(proposal_response.proposal.status, Status::Passed {}); } #[test] @@ -1186,12 +1165,7 @@ fn test_revoting_playthrough() { // Expire the proposal allowing the votes to be tallied. app.update_block(|b| b.time = b.time.plus_seconds(604800)); let proposal_response = query_proposal(&app, &proposal_module, proposal_id); - assert_eq!( - proposal_response.proposal.status, - Status::Passed { - at_time: Timestamp::from_nanos(1572402219879305533) - } - ); + assert_eq!(proposal_response.proposal.status, Status::Passed {}); execute_proposal(&mut app, &proposal_module, CREATOR_ADDR, proposal_id); // Can't vote once the proposal is passed. @@ -1264,12 +1238,7 @@ fn test_allow_revoting_config_changes() { // Proposal without revoting should have passed. let proposal_resp = query_proposal(&app, &proposal_module, no_revoting_proposal); - assert_eq!( - proposal_resp.proposal.status, - Status::Passed { - at_time: Timestamp::from_nanos(1571797419879305533) - } - ); + assert_eq!(proposal_resp.proposal.status, Status::Passed {}); // Proposal with revoting should not have passed. let proposal_resp = query_proposal(&app, &proposal_module, revoting_proposal); @@ -1347,12 +1316,7 @@ fn test_three_of_five_multisig() { vote_on_proposal(&mut app, &proposal_module, "three", proposal_id, Vote::Yes); let proposal: ProposalResponse = query_proposal(&app, &proposal_module, 1); - assert_eq!( - proposal.proposal.status, - Status::Passed { - at_time: Timestamp::from_nanos(1571797419879305533) - } - ); + assert_eq!(proposal.proposal.status, Status::Passed {}); execute_proposal(&mut app, &proposal_module, "four", proposal_id); @@ -1479,9 +1443,7 @@ fn test_absolute_count_threshold_non_multisig() { Threshold::AbsoluteCount { threshold: Uint128::new(11), }, - Status::Passed { - at_time: Timestamp::from_nanos(1571797419879305533), - }, + Status::Passed {}, None, ); } @@ -1985,12 +1947,7 @@ fn test_execution_failed() { // Even though this proposal was created before the config change // was made it still gets retroactively applied. let proposal = query_proposal(&app, &proposal_module, proposal_id); - assert_eq!( - proposal.proposal.status, - Status::Passed { - at_time: Timestamp::from_nanos(1571797419879305533) - } - ); + assert_eq!(proposal.proposal.status, Status::Passed {}); // This proposal's deposit should not have been returned. It will // not be returnable until this is executed, or close on execution @@ -2026,6 +1983,7 @@ fn test_reply_proposal_mock() { total_power: Uint128::new(100_000_000), msgs: vec![], status: Status::Open, + timelock: None, votes: Votes::zero(), }, ) diff --git a/packages/dao-testing/src/tests.rs b/packages/dao-testing/src/tests.rs index 4d9b5c223..17a9b6023 100644 --- a/packages/dao-testing/src/tests.rs +++ b/packages/dao-testing/src/tests.rs @@ -1,4 +1,4 @@ -use cosmwasm_std::{Decimal, Timestamp, Uint128}; +use cosmwasm_std::{Decimal, Uint128}; use dao_voting::status::Status; use dao_voting::threshold::{PercentageThreshold, Threshold}; use dao_voting::voting::Vote; @@ -40,9 +40,7 @@ where Threshold::AbsolutePercentage { percentage: PercentageThreshold::Percent(Decimal::percent(100)), }, - Status::Passed { - at_time: Timestamp::from_nanos(1572402219879305533), - }, + Status::Passed {}, None, ); @@ -75,9 +73,7 @@ where Threshold::AbsolutePercentage { percentage: PercentageThreshold::Percent(Decimal::percent(100)), }, - Status::Passed { - at_time: Timestamp::from_nanos(1572402219879305533), - }, + Status::Passed {}, None, ); } @@ -96,9 +92,7 @@ where Threshold::AbsolutePercentage { percentage: PercentageThreshold::Percent(Decimal::percent(100)), }, - Status::Passed { - at_time: Timestamp::from_nanos(1572402219879305533), - }, + Status::Passed {}, None, ); @@ -120,9 +114,7 @@ where Threshold::AbsolutePercentage { percentage: PercentageThreshold::Percent(Decimal::percent(99)), }, - Status::Passed { - at_time: Timestamp::from_nanos(1572402219879305533), - }, + Status::Passed {}, None, ) } @@ -216,9 +208,7 @@ where Threshold::AbsolutePercentage { percentage: PercentageThreshold::Percent(Decimal::percent(1)), }, - Status::Passed { - at_time: Timestamp::from_nanos(1572402219879305533), - }, + Status::Passed {}, Some(Uint128::new(100)), ); @@ -232,9 +222,7 @@ where Threshold::AbsolutePercentage { percentage: PercentageThreshold::Percent(Decimal::percent(1)), }, - Status::Passed { - at_time: Timestamp::from_nanos(1572402219879305533), - }, + Status::Passed {}, Some(Uint128::new(1000)), ); @@ -330,9 +318,7 @@ where Threshold::AbsolutePercentage { percentage: PercentageThreshold::Percent(Decimal::percent(50)), }, - Status::Passed { - at_time: Timestamp::from_nanos(1572402219879305533), - }, + Status::Passed {}, None, ); @@ -354,9 +340,7 @@ where Threshold::AbsolutePercentage { percentage: PercentageThreshold::Percent(Decimal::percent(50)), }, - Status::Passed { - at_time: Timestamp::from_nanos(1572402219879305533), - }, + Status::Passed {}, None, ); @@ -385,9 +369,7 @@ where Threshold::AbsolutePercentage { percentage: PercentageThreshold::Percent(Decimal::percent(50)), }, - Status::Passed { - at_time: Timestamp::from_nanos(1572402219879305533), - }, + Status::Passed {}, None, ); } @@ -415,9 +397,7 @@ where threshold: PercentageThreshold::Percent(Decimal::percent(10)), quorum: PercentageThreshold::Majority {}, }, - Status::Passed { - at_time: Timestamp::from_nanos(1572402219879305533), - }, + Status::Passed {}, None, ); @@ -447,9 +427,7 @@ where threshold: PercentageThreshold::Percent(Decimal::percent(10)), quorum: PercentageThreshold::Majority {}, }, - Status::Passed { - at_time: Timestamp::from_nanos(1572402219879305533), - }, + Status::Passed {}, None, ); } @@ -477,9 +455,7 @@ where threshold: PercentageThreshold::Percent(Decimal::percent(50)), quorum: PercentageThreshold::Majority {}, }, - Status::Passed { - at_time: Timestamp::from_nanos(1572402219879305533), - }, + Status::Passed {}, None, ); @@ -559,9 +535,7 @@ where threshold: PercentageThreshold::Majority {}, quorum: PercentageThreshold::Percent(Decimal::percent(60)), }, - Status::Passed { - at_time: Timestamp::from_nanos(1572402219879305533), - }, + Status::Passed {}, Some(Uint128::new(100)), ); do_votes( @@ -590,9 +564,7 @@ where threshold: PercentageThreshold::Majority {}, quorum: PercentageThreshold::Percent(Decimal::percent(60)), }, - Status::Passed { - at_time: Timestamp::from_nanos(1572402219879305533), - }, + Status::Passed {}, Some(Uint128::new(100)), ); do_votes( @@ -627,9 +599,7 @@ where std::cmp::Ordering::Less => Status::Rejected, // Depends on which reaches the threshold first. Ignore for now. std::cmp::Ordering::Equal => Status::Rejected, - std::cmp::Ordering::Greater => Status::Passed { - at_time: Timestamp::from_nanos(1572402219879305533), - }, + std::cmp::Ordering::Greater => Status::Passed {}, }; let yes = yes diff --git a/packages/dao-voting/src/status.rs b/packages/dao-voting/src/status.rs index 1295ff270..f75147a79 100644 --- a/packages/dao-voting/src/status.rs +++ b/packages/dao-voting/src/status.rs @@ -1,7 +1,6 @@ use cosmwasm_schema::cw_serde; use cosmwasm_std::Timestamp; -// TODO add more timestamps for consistency? #[cw_serde] #[derive(Copy)] pub enum Status { @@ -10,7 +9,7 @@ pub enum Status { /// The proposal has been rejected. Rejected, /// The proposal has been passed but has not been executed. - Passed { at_time: Timestamp }, + Passed, /// The proposal has been passed and executed. Executed, /// The proposal has failed or expired and has been closed. A @@ -18,6 +17,9 @@ pub enum Status { Closed, /// The proposal's execution failed. ExecutionFailed, + /// Proposal is timelocked and can not be until the timelock expires + /// During this time the proposal may be vetoed. + Timelocked { expires: Timestamp }, /// The proposal has been vetoed. Vetoed, } @@ -27,10 +29,11 @@ impl std::fmt::Display for Status { match self { Status::Open => write!(f, "open"), Status::Rejected => write!(f, "rejected"), - Status::Passed { at_time } => write!(f, "passed {:?}", at_time), + Status::Passed => write!(f, "passed"), Status::Executed => write!(f, "executed"), Status::Closed => write!(f, "closed"), Status::ExecutionFailed => write!(f, "execution_failed"), + Status::Timelocked { expires } => write!(f, "timelocked {:?}", expires), Status::Vetoed => write!(f, "vetoed"), } } diff --git a/packages/dao-voting/src/timelock.rs b/packages/dao-voting/src/timelock.rs index 30f8dbf4d..967807955 100644 --- a/packages/dao-voting/src/timelock.rs +++ b/packages/dao-voting/src/timelock.rs @@ -35,8 +35,13 @@ pub struct Timelock { } impl Timelock { + /// Calculate the expiration time for the timelock + pub fn calculate_timelock_expiration(&self, current_time: Timestamp) -> Timestamp { + Timestamp::from_seconds(current_time.seconds() + self.delay.seconds()) + } + /// Whether early execute is enabled - pub fn early_excute_enabled(&self) -> Result<(), TimelockError> { + pub fn check_early_excute_enabled(&self) -> Result<(), TimelockError> { if self.early_execute { Ok(()) } else { @@ -45,12 +50,12 @@ impl Timelock { } /// Takes two timestamps and returns true if the proposal is locked or not. - pub fn is_locked( + pub fn check_is_locked( &self, - proposal_passed: Timestamp, current_time: Timestamp, + expires: Timestamp, ) -> Result<(), TimelockError> { - if proposal_passed.seconds() + self.delay.seconds() < current_time.seconds() { + if current_time.seconds() > expires.seconds() { Ok(()) } else { Err(TimelockError::Timelocked {}) @@ -58,7 +63,7 @@ impl Timelock { } /// Checks whether the message sender is the vetoer. - pub fn is_vetoer(&self, info: &MessageInfo) -> Result<(), TimelockError> { + pub fn check_is_vetoer(&self, info: &MessageInfo) -> Result<(), TimelockError> { if self.vetoer == info.sender.to_string() { Ok(()) } else {