From 5b6093e961fb4e20073ad1a648a8737e39aea3a3 Mon Sep 17 00:00:00 2001 From: Noah Saso Date: Thu, 1 Aug 2024 20:37:31 -0400 Subject: [PATCH 1/3] fix cargo errors --- .../external/cw-tokenfactory-issuer/tests/mod.rs | 4 ---- contracts/external/dao-migrator/README.md | 14 ++++++++------ .../dao-proposal-condorcet/src/testing/suite.rs | 2 +- .../src/tests/test_tube/mod.rs | 4 ---- packages/dao-testing/src/test_tube/mod.rs | 4 ---- 5 files changed, 9 insertions(+), 19 deletions(-) diff --git a/contracts/external/cw-tokenfactory-issuer/tests/mod.rs b/contracts/external/cw-tokenfactory-issuer/tests/mod.rs index 73b60899d..8ad6cfb23 100644 --- a/contracts/external/cw-tokenfactory-issuer/tests/mod.rs +++ b/contracts/external/cw-tokenfactory-issuer/tests/mod.rs @@ -1,7 +1,3 @@ -// Ignore integration tests for code coverage since there will be problems with dynamic linking libosmosistesttube -// and also, tarpaulin will not be able read coverage out of wasm binary anyway -#![cfg(not(tarpaulin))] - #[cfg(feature = "test-tube")] mod cases; #[cfg(feature = "test-tube")] diff --git a/contracts/external/dao-migrator/README.md b/contracts/external/dao-migrator/README.md index f61a3bb52..ba51df374 100644 --- a/contracts/external/dao-migrator/README.md +++ b/contracts/external/dao-migrator/README.md @@ -5,7 +5,7 @@ Here is the [discussion](https://github.com/DA0-DA0/dao-contracts/discussions/607). -A migrator module for a DAO DAO DAO which handles migration for DAO modules +A migrator module for a DAO DAO DAO which handles migration for DAO modules and test it went successfully. DAO core migration is handled by a proposal, which adds this module and do @@ -14,6 +14,7 @@ If custom module is found, this TX fails and migration is cancelled, custom module requires a custom migration to be done by the DAO. # General idea + 1. Proposal is made to migrate DAO core to V2, which also adds this module to the DAO. 2. On init of this contract, a callback is fired to do the migration. 3. Then we check to make sure the DAO doesn't have custom modules. @@ -23,9 +24,10 @@ module requires a custom migration to be done by the DAO. 7. In any case where 1 migration fails, we fail the whole TX. # Important notes -* custom modules cannot reliably be migrated by this contract, -because of that we fail the process to avoid any unwanted results. -* If any module migration fails we fail the whole thing, -this is to make sure that we either have a fully working V2, -or we do nothing and make sure the DAO is operational at any time. \ No newline at end of file +- custom modules cannot reliably be migrated by this contract, + because of that we fail the process to avoid any unwanted results. + +- If any module migration fails we fail the whole thing, + this is to make sure that we either have a fully working V2, + or we do nothing and make sure the DAO is operational at any time. diff --git a/contracts/proposal/dao-proposal-condorcet/src/testing/suite.rs b/contracts/proposal/dao-proposal-condorcet/src/testing/suite.rs index 79d18154f..c268bc5c1 100644 --- a/contracts/proposal/dao-proposal-condorcet/src/testing/suite.rs +++ b/contracts/proposal/dao-proposal-condorcet/src/testing/suite.rs @@ -146,7 +146,7 @@ impl SuiteBuilder { if let Some(candidates) = self.with_proposal { suite .propose( - &suite.sender(), + suite.sender(), (0..candidates) .map(|_| vec![unimportant_message()]) .collect(), diff --git a/contracts/voting/dao-voting-token-staked/src/tests/test_tube/mod.rs b/contracts/voting/dao-voting-token-staked/src/tests/test_tube/mod.rs index fe51e9fb6..eb0b4f91b 100644 --- a/contracts/voting/dao-voting-token-staked/src/tests/test_tube/mod.rs +++ b/contracts/voting/dao-voting-token-staked/src/tests/test_tube/mod.rs @@ -1,6 +1,2 @@ -// Ignore integration tests for code coverage since there will be problems with dynamic linking libosmosistesttube -// and also, tarpaulin will not be able read coverage out of wasm binary anyway -#![cfg(not(tarpaulin))] - mod integration_tests; mod test_env; diff --git a/packages/dao-testing/src/test_tube/mod.rs b/packages/dao-testing/src/test_tube/mod.rs index c764df4d1..107ea403d 100644 --- a/packages/dao-testing/src/test_tube/mod.rs +++ b/packages/dao-testing/src/test_tube/mod.rs @@ -1,7 +1,3 @@ -// Ignore integration tests for code coverage since there will be problems with dynamic linking libosmosistesttube -// and also, tarpaulin will not be able read coverage out of wasm binary anyway -#![cfg(not(tarpaulin))] - // Integrationg tests using an actual chain binary, requires // the "test-tube" feature to be enabled // cargo test --features test-tube From 088b10980c6501d79c34d68840ccacc51f1179f7 Mon Sep 17 00:00:00 2001 From: Noah Saso Date: Thu, 1 Aug 2024 23:35:40 -0400 Subject: [PATCH 2/3] clarify scalar values a bit more and update comments --- .../dao-rewards-distributor/src/helpers.rs | 14 +++++++++----- .../dao-rewards-distributor/src/rewards.rs | 10 +++++----- .../dao-rewards-distributor/src/state.rs | 15 ++++++--------- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/contracts/distribution/dao-rewards-distributor/src/helpers.rs b/contracts/distribution/dao-rewards-distributor/src/helpers.rs index 1960845ae..6773bb3aa 100644 --- a/contracts/distribution/dao-rewards-distributor/src/helpers.rs +++ b/contracts/distribution/dao-rewards-distributor/src/helpers.rs @@ -37,8 +37,8 @@ pub fn get_voting_power_at_block( } /// returns underlying scalar value for a given duration. -/// if the duration is in blocks, returns the block height. -/// if the duration is in time, returns the time in seconds. +/// if the duration is a block height, return the number of blocks. +/// if the duration is a time, return the number of seconds. pub fn get_duration_scalar(duration: &Duration) -> u64 { match duration { Duration::Height(h) => *h, @@ -73,9 +73,13 @@ pub(crate) fn scale_factor() -> Uint256 { Uint256::from(10u8).pow(39) } -/// Calculate the duration from start to end. If the end is at or before the -/// start, return 0. The first argument is end, and the second is start. -pub fn get_exp_diff(end: &Expiration, start: &Expiration) -> StdResult { +/// Calculate the duration scalar value from start to end. If the end is at or +/// before the start, return 0. The first argument is end, and the second is +/// start. If start and end are block heights, this returns the number of +/// blocks. If they are times, this returns the number of seconds. If both are +/// never, this returns 0. If start and end have different units, it errors as +/// that should not be possible. +pub fn get_exp_diff_scalar(end: &Expiration, start: &Expiration) -> StdResult { match (end, start) { (Expiration::AtHeight(end), Expiration::AtHeight(start)) => { if end > start { diff --git a/contracts/distribution/dao-rewards-distributor/src/rewards.rs b/contracts/distribution/dao-rewards-distributor/src/rewards.rs index c44ece1fc..951222bd2 100644 --- a/contracts/distribution/dao-rewards-distributor/src/rewards.rs +++ b/contracts/distribution/dao-rewards-distributor/src/rewards.rs @@ -2,8 +2,8 @@ use cosmwasm_std::{Addr, BlockInfo, Deps, DepsMut, Env, StdResult, Uint128, Uint use crate::{ helpers::{ - get_duration_scalar, get_exp_diff, get_prev_block_total_vp, get_voting_power_at_block, - scale_factor, + get_duration_scalar, get_exp_diff_scalar, get_prev_block_total_vp, + get_voting_power_at_block, scale_factor, }, state::{DistributionState, EmissionRate, UserRewardState, DISTRIBUTIONS, USER_REWARDS}, ContractError, @@ -98,7 +98,7 @@ pub fn get_active_total_earned_puvp( // get the duration from the last time rewards were updated to the // last time rewards were distributed. this will be 0 if the rewards // were updated at or after the last time rewards were distributed. - let new_reward_distribution_duration: Uint128 = get_exp_diff( + let new_reward_distribution_duration_scalar: Uint128 = get_exp_diff_scalar( &last_time_rewards_distributed, &distribution.active_epoch.last_updated_total_earned_puvp, )? @@ -106,7 +106,7 @@ pub fn get_active_total_earned_puvp( // no need to query total voting power and do math if distribution // is already up to date. - if new_reward_distribution_duration.is_zero() { + if new_reward_distribution_duration_scalar.is_zero() { return Ok(curr); } @@ -118,7 +118,7 @@ pub fn get_active_total_earned_puvp( } else { // count intervals of the rewards emission that have passed // since the last update which need to be distributed - let complete_distribution_periods = new_reward_distribution_duration + let complete_distribution_periods = new_reward_distribution_duration_scalar .checked_div(get_duration_scalar(&duration).into())?; // It is impossible for this to overflow as total rewards can diff --git a/contracts/distribution/dao-rewards-distributor/src/state.rs b/contracts/distribution/dao-rewards-distributor/src/state.rs index ea90e0b2e..68f9e5992 100644 --- a/contracts/distribution/dao-rewards-distributor/src/state.rs +++ b/contracts/distribution/dao-rewards-distributor/src/state.rs @@ -9,7 +9,7 @@ use cw_utils::Duration; use std::{cmp::min, collections::HashMap}; use crate::{ - helpers::{get_duration_scalar, get_exp_diff, get_prev_block_total_vp, scale_factor}, + helpers::{get_duration_scalar, get_exp_diff_scalar, get_prev_block_total_vp, scale_factor}, rewards::get_active_total_earned_puvp, ContractError, }; @@ -70,7 +70,7 @@ impl EmissionRate { EmissionRate::Linear { amount, duration, .. } => { - if *amount == Uint128::zero() { + if amount.is_zero() { return Err(ContractError::InvalidEmissionRateFieldZero { field: "amount".to_string(), }); @@ -236,16 +236,13 @@ impl DistributionState { EmissionRate::Linear { amount, duration, .. } => { - let epoch_duration = - get_exp_diff(&self.active_epoch.ends_at, &self.active_epoch.started_at)?; + let epoch_duration_scalar = + get_exp_diff_scalar(&self.active_epoch.ends_at, &self.active_epoch.started_at)?; - let emission_rate_duration_scalar = match duration { - Duration::Height(h) => h, - Duration::Time(t) => t, - }; + let emission_rate_duration_scalar = get_duration_scalar(&duration); amount - .checked_multiply_ratio(epoch_duration, emission_rate_duration_scalar) + .checked_multiply_ratio(epoch_duration_scalar, emission_rate_duration_scalar) .map_err(|e| StdError::generic_err(e.to_string())) } } From 4a0768ebe6b77ad0d860481f2f81ec5592798787 Mon Sep 17 00:00:00 2001 From: Noah Saso Date: Mon, 5 Aug 2024 15:57:21 -0400 Subject: [PATCH 3/3] added extensions that make math more readable and maintainable --- .../dao-rewards-distributor/src/contract.rs | 5 +- .../dao-rewards-distributor/src/helpers.rs | 111 +++++++++++------- .../dao-rewards-distributor/src/rewards.rs | 19 ++- .../dao-rewards-distributor/src/state.rs | 20 ++-- 4 files changed, 93 insertions(+), 62 deletions(-) diff --git a/contracts/distribution/dao-rewards-distributor/src/contract.rs b/contracts/distribution/dao-rewards-distributor/src/contract.rs index 3a6e3c6e0..04036540d 100644 --- a/contracts/distribution/dao-rewards-distributor/src/contract.rs +++ b/contracts/distribution/dao-rewards-distributor/src/contract.rs @@ -2,7 +2,7 @@ use cosmwasm_std::entry_point; use cosmwasm_std::{ ensure, from_json, to_json_binary, Binary, Deps, DepsMut, Env, MessageInfo, Order, Response, - StdResult, Uint128, Uint256, + StdError, StdResult, Uint128, Uint256, }; use cw2::{get_contract_version, set_contract_version}; use cw20::{Cw20ReceiveMsg, Denom}; @@ -542,7 +542,8 @@ fn query_pending_rewards( for (id, distribution) in distributions { // first we get the active epoch earned puvp value let active_total_earned_puvp = - get_active_total_earned_puvp(deps, &env.block, &distribution)?; + get_active_total_earned_puvp(deps, &env.block, &distribution) + .map_err(|e| StdError::generic_err(e.to_string()))?; // then we add that to the historical rewards earned puvp let total_earned_puvp = diff --git a/contracts/distribution/dao-rewards-distributor/src/helpers.rs b/contracts/distribution/dao-rewards-distributor/src/helpers.rs index 6773bb3aa..541fc2f35 100644 --- a/contracts/distribution/dao-rewards-distributor/src/helpers.rs +++ b/contracts/distribution/dao-rewards-distributor/src/helpers.rs @@ -36,16 +36,6 @@ pub fn get_voting_power_at_block( Ok(resp.power) } -/// returns underlying scalar value for a given duration. -/// if the duration is a block height, return the number of blocks. -/// if the duration is a time, return the number of seconds. -pub fn get_duration_scalar(duration: &Duration) -> u64 { - match duration { - Duration::Height(h) => *h, - Duration::Time(t) => *t, - } -} - /// Returns the appropriate CosmosMsg for transferring the reward token. pub fn get_transfer_msg(recipient: Addr, amount: Uint128, denom: Denom) -> StdResult { match denom { @@ -73,36 +63,6 @@ pub(crate) fn scale_factor() -> Uint256 { Uint256::from(10u8).pow(39) } -/// Calculate the duration scalar value from start to end. If the end is at or -/// before the start, return 0. The first argument is end, and the second is -/// start. If start and end are block heights, this returns the number of -/// blocks. If they are times, this returns the number of seconds. If both are -/// never, this returns 0. If start and end have different units, it errors as -/// that should not be possible. -pub fn get_exp_diff_scalar(end: &Expiration, start: &Expiration) -> StdResult { - match (end, start) { - (Expiration::AtHeight(end), Expiration::AtHeight(start)) => { - if end > start { - Ok(end - start) - } else { - Ok(0) - } - } - (Expiration::AtTime(end), Expiration::AtTime(start)) => { - if end > start { - Ok(end.seconds() - start.seconds()) - } else { - Ok(0) - } - } - (Expiration::Never {}, Expiration::Never {}) => Ok(0), - _ => Err(StdError::generic_err(format!( - "incompatible expirations: got end {:?}, start {:?}", - end, start - ))), - } -} - pub fn validate_voting_power_contract( deps: &DepsMut, vp_contract: String, @@ -114,3 +74,74 @@ pub fn validate_voting_power_contract( )?; Ok(vp_contract) } + +pub trait ExpirationExt { + /// Compute the duration since the start, flooring at 0 if the current + /// expiration is before the start. If either is never, or if they have + /// different units, returns an error as those cannot be compared. + fn duration_since(&self, start: &Self) -> StdResult; +} + +impl ExpirationExt for Expiration { + fn duration_since(&self, start: &Self) -> StdResult { + match (self, start) { + (Expiration::AtHeight(end), Expiration::AtHeight(start)) => { + if end > start { + Ok(Duration::Height(end - start)) + } else { + Ok(Duration::Height(0)) + } + } + (Expiration::AtTime(end), Expiration::AtTime(start)) => { + if end > start { + Ok(Duration::Time(end.seconds() - start.seconds())) + } else { + Ok(Duration::Time(0)) + } + } + (Expiration::Never {}, _) | (_, Expiration::Never {}) => { + Err(StdError::generic_err(format!( + "can't compute diff between expirations with never: got end {:?} and start {:?}", + self, start + ))) + } + _ => Err(StdError::generic_err(format!( + "incompatible expirations: got end {:?} and start {:?}", + self, start + ))), + } + } +} + +pub trait DurationExt { + /// Returns true if the duration is 0 blocks or 0 seconds. + fn is_zero(&self) -> bool; + + /// Perform checked integer division between two durations, erroring if the + /// units do not match or denominator is 0. + fn checked_div(&self, denominator: &Self) -> Result; +} + +impl DurationExt for Duration { + fn is_zero(&self) -> bool { + match self { + Duration::Height(h) => *h == 0, + Duration::Time(t) => *t == 0, + } + } + + fn checked_div(&self, denominator: &Self) -> Result { + match (self, denominator) { + (Duration::Height(numerator), Duration::Height(denominator)) => { + Ok(Uint128::from(*numerator).checked_div(Uint128::from(*denominator))?) + } + (Duration::Time(numerator), Duration::Time(denominator)) => { + Ok(Uint128::from(*numerator).checked_div(Uint128::from(*denominator))?) + } + _ => Err(ContractError::Std(StdError::generic_err(format!( + "incompatible durations: got numerator {:?} and denominator {:?}", + self, denominator + )))), + } + } +} diff --git a/contracts/distribution/dao-rewards-distributor/src/rewards.rs b/contracts/distribution/dao-rewards-distributor/src/rewards.rs index 951222bd2..926e2f35c 100644 --- a/contracts/distribution/dao-rewards-distributor/src/rewards.rs +++ b/contracts/distribution/dao-rewards-distributor/src/rewards.rs @@ -2,8 +2,8 @@ use cosmwasm_std::{Addr, BlockInfo, Deps, DepsMut, Env, StdResult, Uint128, Uint use crate::{ helpers::{ - get_duration_scalar, get_exp_diff_scalar, get_prev_block_total_vp, - get_voting_power_at_block, scale_factor, + get_prev_block_total_vp, get_voting_power_at_block, scale_factor, DurationExt, + ExpirationExt, }, state::{DistributionState, EmissionRate, UserRewardState, DISTRIBUTIONS, USER_REWARDS}, ContractError, @@ -82,7 +82,7 @@ pub fn get_active_total_earned_puvp( deps: Deps, block: &BlockInfo, distribution: &DistributionState, -) -> StdResult { +) -> Result { match distribution.active_epoch.emission_rate { EmissionRate::Paused {} => Ok(Uint256::zero()), // this is updated manually during funding, so just return it here. @@ -98,15 +98,12 @@ pub fn get_active_total_earned_puvp( // get the duration from the last time rewards were updated to the // last time rewards were distributed. this will be 0 if the rewards // were updated at or after the last time rewards were distributed. - let new_reward_distribution_duration_scalar: Uint128 = get_exp_diff_scalar( - &last_time_rewards_distributed, - &distribution.active_epoch.last_updated_total_earned_puvp, - )? - .into(); + let new_reward_distribution_duration = last_time_rewards_distributed + .duration_since(&distribution.active_epoch.last_updated_total_earned_puvp)?; // no need to query total voting power and do math if distribution // is already up to date. - if new_reward_distribution_duration_scalar.is_zero() { + if new_reward_distribution_duration.is_zero() { return Ok(curr); } @@ -118,8 +115,8 @@ pub fn get_active_total_earned_puvp( } else { // count intervals of the rewards emission that have passed // since the last update which need to be distributed - let complete_distribution_periods = new_reward_distribution_duration_scalar - .checked_div(get_duration_scalar(&duration).into())?; + let complete_distribution_periods = + new_reward_distribution_duration.checked_div(&duration)?; // It is impossible for this to overflow as total rewards can // never exceed max value of Uint128 as total tokens in diff --git a/contracts/distribution/dao-rewards-distributor/src/state.rs b/contracts/distribution/dao-rewards-distributor/src/state.rs index 68f9e5992..3237bbb33 100644 --- a/contracts/distribution/dao-rewards-distributor/src/state.rs +++ b/contracts/distribution/dao-rewards-distributor/src/state.rs @@ -9,7 +9,7 @@ use cw_utils::Duration; use std::{cmp::min, collections::HashMap}; use crate::{ - helpers::{get_duration_scalar, get_exp_diff_scalar, get_prev_block_total_vp, scale_factor}, + helpers::{get_prev_block_total_vp, scale_factor, DurationExt, ExpirationExt}, rewards::get_active_total_earned_puvp, ContractError, }; @@ -75,7 +75,7 @@ impl EmissionRate { field: "amount".to_string(), }); } - if get_duration_scalar(duration) == 0 { + if duration.is_zero() { return Err(ContractError::InvalidEmissionRateFieldZero { field: "duration".to_string(), }); @@ -229,21 +229,23 @@ impl DistributionState { /// get the total rewards to be distributed based on the active epoch's /// emission rate - pub fn get_total_rewards(&self) -> StdResult { + pub fn get_total_rewards(&self) -> Result { match self.active_epoch.emission_rate { EmissionRate::Paused {} => Ok(Uint128::zero()), EmissionRate::Immediate {} => Ok(self.funded_amount), EmissionRate::Linear { amount, duration, .. } => { - let epoch_duration_scalar = - get_exp_diff_scalar(&self.active_epoch.ends_at, &self.active_epoch.started_at)?; + let epoch_duration = self + .active_epoch + .ends_at + .duration_since(&self.active_epoch.started_at)?; - let emission_rate_duration_scalar = get_duration_scalar(&duration); + // count total intervals of the rewards emission that will pass + // based on the start and end times. + let complete_distribution_periods = epoch_duration.checked_div(&duration)?; - amount - .checked_multiply_ratio(epoch_duration_scalar, emission_rate_duration_scalar) - .map_err(|e| StdError::generic_err(e.to_string())) + Ok(amount.checked_mul(complete_distribution_periods)?) } } }