using Expiration & Duration for timelock; some cleanups

contracts/proposal/dao-proposal-single/src/contract.rs

@@ -62,6 +62,8 @@ pub fn instantiate(
         .pre_propose_info
         .into_initial_policy_and_messages(dao.clone())?;
 
+    // TODO: validate timelock?
+
     let config = Config {
         threshold: msg.threshold,
         max_voting_period,
@@ -309,16 +311,17 @@ pub fn execute_veto(
                 None => Err(ContractError::TimelockError(TimelockError::NoTimelock {})),
             }
         }
-        Status::Timelocked { expires } => {
+        Status::Timelocked { expiration } => {
             match prop.timelock {
                 Some(ref timelock) => {
-                    // Check if the proposal timelock has expired, vetoer cannot veto
-                    // after expiration
-                    timelock.check_is_expired(env.block.time, expires)?;
-
                     // Check sender is vetoer
                     timelock.check_is_vetoer(&info)?;
 
+                    // vetoer can veto the proposal iff the timelock is active/not expired
+                    if expiration.is_expired(&env.block) {
+                        return Err(ContractError::TimelockError(TimelockError::TimelockExpired {  }))
+                    }
+
                     let old_status = prop.status;
 
                     // Update proposal status to vetoed
@@ -391,15 +394,16 @@ pub fn execute_execute(
     let old_status = prop.status;
     match prop.status {
         Status::Passed => (),
-        Status::Timelocked { expires } => {
+        Status::Timelocked { expiration } => {
             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)?;
+                    timelock.check_early_execute_enabled()?;
+                } else if !expiration.is_expired(&env.block) {
+                    // anyone can execute the proposal iff timelock 
+                    // expiration is due. otherwise we error.
+                    return Err(ContractError::TimelockError(TimelockError::Timelocked {}))
                 }
             }
         }
@@ -477,6 +481,7 @@ pub fn execute_vote(
     // cause a different one. This then serves to allow
     // for better tallies of opinions in the event that a
     // proposal passes or is rejected early.
+    // benskey TODO: check for potential timelock issues
     if prop.expiration.is_expired(&env.block) {
         return Err(ContractError::Expired { id: proposal_id });
     }

contracts/proposal/dao-proposal-single/src/proposal.rs

@@ -70,14 +70,17 @@ impl SingleChoiceProposal {
 
     /// Gets the current status of the proposal.
     pub fn current_status(&self, block: &BlockInfo) -> Status {
+        // benskey: make this nicer
         if self.status == Status::Open && self.is_passed(block) {
             // 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 {
+                // derive the timelock `Expiration` by adding the delay
+                // duration to the current block
                 Status::Timelocked {
-                    expires: timelock.calculate_timelock_expiration(block.time),
+                    expiration: timelock.delay.after(block),
                 }
             } else {
                 Status::Passed

contracts/proposal/dao-proposal-single/src/testing/tests.rs

@@ -9,7 +9,7 @@ use cw20::Cw20Coin;
 use cw_denom::CheckedDenom;
 use cw_hooks::{HookError, HooksResponse};
 use cw_multi_test::{next_block, App, Executor};
-use cw_utils::Duration;
+use cw_utils::{Duration, Expiration};
 use dao_interface::{
     state::{Admin, ModuleInstantiateInfo},
     voting::InfoResponse,
@@ -387,13 +387,14 @@ fn test_proposal_message_execution() {
 fn test_proposal_message_timelock_execution() {
     let mut app = App::default();
     let mut instantiate = get_default_token_dao_proposal_module_instantiate(&mut app);
-    instantiate.close_proposal_on_execution_failure = false;
-    instantiate.timelock = Some(Timelock {
-        delay: Timestamp::from_seconds(100),
+    let timelock = Timelock {
+        delay: Duration::Time(100),
         vetoer: "oversight".to_string(),
         early_execute: false,
         veto_before_passed: false,
-    });
+    };
+    instantiate.close_proposal_on_execution_failure = false;
+    instantiate.timelock = Some(timelock.clone());
     let core_addr = instantiate_with_staked_balances_governance(
         &mut app,
         instantiate,
@@ -438,7 +439,7 @@ fn test_proposal_message_timelock_execution() {
     let native_balance = query_balance_native(&app, CREATOR_ADDR, "ujuno");
     assert_eq!(cw20_balance, Uint128::zero());
     assert_eq!(native_balance, Uint128::zero());
-
+    
     vote_on_proposal(
         &mut app,
         &proposal_module,
@@ -447,12 +448,12 @@ fn test_proposal_message_timelock_execution() {
         Vote::Yes,
     );
     let proposal = query_proposal(&app, &proposal_module, proposal_id);
-
-    // Proposal is timelocked
+    
+    // Proposal is timelocked to the moment of prop passing + timelock delay
     assert_eq!(
         proposal.proposal.status,
         Status::Timelocked {
-            expires: Timestamp::from_nanos(1571797519000000000)
+            expiration: timelock.delay.after(&app.block_info()),
         }
     );
 
@@ -508,12 +509,13 @@ fn test_proposal_message_timelock_veto() {
     let mut app = App::default();
     let mut instantiate = get_default_token_dao_proposal_module_instantiate(&mut app);
     instantiate.close_proposal_on_execution_failure = false;
-    instantiate.timelock = Some(Timelock {
-        delay: Timestamp::from_seconds(100),
+    let timelock = Timelock {
+        delay: Duration::Time(100),
         vetoer: "oversight".to_string(),
         early_execute: false,
         veto_before_passed: false,
-    });
+    };
+    instantiate.timelock = Some(timelock.clone());
     let core_addr = instantiate_with_staked_balances_governance(
         &mut app,
         instantiate,
@@ -579,11 +581,11 @@ fn test_proposal_message_timelock_veto() {
     );
     let proposal = query_proposal(&app, &proposal_module, proposal_id);
 
-    // Proposal is timelocked
+    // Proposal is timelocked to the moment of prop passing + timelock delay
     assert_eq!(
         proposal.proposal.status,
         Status::Timelocked {
-            expires: Timestamp::from_nanos(1571797519000000000)
+            expiration: timelock.delay.after(&app.block_info()),
         }
     );
 
@@ -623,12 +625,13 @@ fn test_proposal_message_timelock_early_execution() {
     let mut app = App::default();
     let mut instantiate = get_default_token_dao_proposal_module_instantiate(&mut app);
     instantiate.close_proposal_on_execution_failure = false;
-    instantiate.timelock = Some(Timelock {
-        delay: Timestamp::from_seconds(100),
+    let timelock = Timelock {
+        delay: Duration::Time(100),
         vetoer: "oversight".to_string(),
         early_execute: true,
         veto_before_passed: false,
-    });
+    };
+    instantiate.timelock = Some(timelock.clone());
     let core_addr = instantiate_with_staked_balances_governance(
         &mut app,
         instantiate,
@@ -683,11 +686,11 @@ fn test_proposal_message_timelock_early_execution() {
     );
     let proposal = query_proposal(&app, &proposal_module, proposal_id);
 
-    // Proposal is timelocked
+    // Proposal is timelocked to the moment of prop passing + timelock delay
     assert_eq!(
         proposal.proposal.status,
         Status::Timelocked {
-            expires: Timestamp::from_nanos(1571797519000000000)
+            expiration: timelock.delay.after(&app.block_info()),
         }
     );
 
@@ -705,7 +708,7 @@ fn test_proposal_message_timelock_veto_before_passed() {
     let mut instantiate = get_default_token_dao_proposal_module_instantiate(&mut app);
     instantiate.close_proposal_on_execution_failure = false;
     instantiate.timelock = Some(Timelock {
-        delay: Timestamp::from_seconds(100),
+        delay: Duration::Time(100),
         vetoer: "oversight".to_string(),
         early_execute: false,
         veto_before_passed: true,
@@ -977,7 +980,7 @@ fn test_update_config() {
             contract_addr: proposal_module.to_string(),
             msg: to_binary(&ExecuteMsg::UpdateConfig {
                 timelock: Some(Timelock {
-                    delay: Timestamp::from_seconds(100),
+                    delay: Duration::Time(100),
                     vetoer: CREATOR_ADDR.to_string(),
                     early_execute: false,
                     veto_before_passed: false,
@@ -1011,7 +1014,7 @@ fn test_update_config() {
         config,
         Config {
             timelock: Some(Timelock {
-                delay: Timestamp::from_seconds(100),
+                delay: Duration::Time(100),
                 vetoer: CREATOR_ADDR.to_string(),
                 early_execute: false,
                 veto_before_passed: false,

packages/dao-voting/src/status.rs

@@ -1,5 +1,5 @@
 use cosmwasm_schema::cw_serde;
-use cosmwasm_std::Timestamp;
+use cw_utils::Expiration;
 
 #[cw_serde]
 #[derive(Copy)]
@@ -19,7 +19,7 @@ pub enum Status {
     ExecutionFailed,
     /// Proposal is timelocked and can not be until the timelock expires
     /// During this time the proposal may be vetoed.
-    Timelocked { expires: Timestamp },
+    Timelocked { expiration: Expiration },
     /// The proposal has been vetoed.
     Vetoed,
 }
@@ -33,7 +33,7 @@ impl std::fmt::Display for Status {
             Status::Executed => write!(f, "executed"),
             Status::Closed => write!(f, "closed"),
             Status::ExecutionFailed => write!(f, "execution_failed"),
-            Status::Timelocked { expires } => write!(f, "timelocked {:?}", expires),
+            Status::Timelocked { expiration } => write!(f, "timelocked {:?}", expiration),
             Status::Vetoed => write!(f, "vetoed"),
         }
     }

packages/dao-voting/src/timelock.rs

@@ -1,5 +1,6 @@
 use cosmwasm_schema::cw_serde;
-use cosmwasm_std::{MessageInfo, StdError, Timestamp};
+use cosmwasm_std::{MessageInfo, StdError, Timestamp, BlockInfo};
+use cw_utils::{Duration, Expiration};
 use thiserror::Error;
 
 #[derive(Error, Debug, PartialEq)]
@@ -23,17 +24,17 @@ pub enum TimelockError {
     Timelocked {},
 
     #[error("The timelock duration has expired.")]
-    TimelockedExpired {},
+    TimelockExpired {},
 
     #[error("Only vetoer can veto a proposal.")]
     Unauthorized {},
 }
 
 #[cw_serde]
 pub struct Timelock {
-    /// The time duration to delay proposal execution for
-    pub delay: Timestamp,
-    /// The account able to veto proposals.
+    /// The time duration to delay proposal execution for.
+    pub delay: Duration,
+    /// The address able to veto proposals.
     pub vetoer: String,
     /// Whether or not the vetoer can excute a proposal early before the
     /// timelock duration has expired
@@ -43,13 +44,8 @@ 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 check_early_excute_enabled(&self) -> Result<(), TimelockError> {
+    pub fn check_early_execute_enabled(&self) -> Result<(), TimelockError> {
         if self.early_execute {
             Ok(())
         } else {
@@ -69,19 +65,6 @@ impl Timelock {
         }
     }
 
-    /// Takes two timestamps and returns true if the proposal is locked or not.
-    pub fn check_is_locked(
-        &self,
-        current_time: Timestamp,
-        expires: Timestamp,
-    ) -> Result<(), TimelockError> {
-        if current_time.seconds() > expires.seconds() {
-            Ok(())
-        } else {
-            Err(TimelockError::Timelocked {})
-        }
-    }
-
     /// Checks whether the message sender is the vetoer.
     pub fn check_is_vetoer(&self, info: &MessageInfo) -> Result<(), TimelockError> {
         if self.vetoer == info.sender.to_string() {