From 2b4ebe564d611510791f7548e103adb67477fb0c Mon Sep 17 00:00:00 2001 From: Krzysztof Ziobro Date: Thu, 4 Jan 2024 14:04:52 +0100 Subject: [PATCH 01/12] Comments on farm contracts --- farm/contract/Cargo.toml | 2 -- farm/contract/lib.rs | 37 +++++++++++++++++++++++++++---------- farm/trait/lib.rs | 7 +++++++ 3 files changed, 34 insertions(+), 12 deletions(-) diff --git a/farm/contract/Cargo.toml b/farm/contract/Cargo.toml index e0e76791..751ee7fd 100644 --- a/farm/contract/Cargo.toml +++ b/farm/contract/Cargo.toml @@ -17,7 +17,6 @@ scale-info = { version = "2.3", default-features = false, features = [ primitive-types = { version = "0.12.1", default-features = false, features = [ "codec", ] } -sp-arithmetic = { version = "18.0.0", default-features = false } psp22 = { version = "0.2", default-features = false } amm-helpers = { path = "../../helpers", default-features = false } @@ -35,7 +34,6 @@ std = [ "psp22/std", "primitive-types/std", "primitive-types/scale-info", - "sp-arithmetic/std", "amm-helpers/std", "farm-trait/std", ] diff --git a/farm/contract/lib.rs b/farm/contract/lib.rs index 12fcbd54..58be5ec7 100644 --- a/farm/contract/lib.rs +++ b/farm/contract/lib.rs @@ -2,7 +2,6 @@ #[ink::contract] mod farm { - type TokenId = AccountId; type UserId = AccountId; use amm_helpers::{math::casted_mul, types::WrappedU256}; @@ -78,6 +77,7 @@ mod farm { /// cumulative_per_share at the last update for each user. pub user_cumulative_reward_last_update: Mapping>, + // Missing documentation pub user_claimable_rewards: Mapping>, } @@ -101,6 +101,7 @@ mod farm { end: 0, timestamp_at_last_update: 0, farm_distributed_unclaimed_rewards: vec![0; n_reward_tokens], + // I am a bit biased against using defaults, but it's correct, so non-important. farm_cumulative_reward_per_share: vec![WrappedU256::default(); n_reward_tokens], farm_reward_rates: vec![0; n_reward_tokens], user_cumulative_reward_last_update: Mapping::default(), @@ -122,6 +123,7 @@ mod farm { let prev = core::cmp::max(self.timestamp_at_last_update, self.start); let now = core::cmp::min(current_timestamp, self.end); + // right side of this alternative will never happen, because of the check above if prev >= now || self.timestamp_at_last_update == current_timestamp { self.timestamp_at_last_update = current_timestamp; return Ok(()); @@ -130,6 +132,7 @@ mod farm { // At this point we know [prev, now] is the intersection of [self.start, self.end] and [self.timestamp_at_last_update, current_timestamp] // It is non-empty because of the checks above and self.start <= now <= self.end + // why enumerate instead of 0..self.reward_tokens.len()? for (idx, _) in self.reward_tokens.iter().enumerate() { let delta_reward_per_share = rewards_per_share_in_time_interval( self.farm_reward_rates[idx], @@ -150,7 +153,6 @@ mod farm { } self.timestamp_at_last_update = current_timestamp; - Ok(()) } @@ -197,20 +199,20 @@ mod farm { rewards: Vec, ) -> Result, FarmError> { let now = Self::env().block_timestamp(); + let tokens_len = self.reward_tokens.len(); - if start <= self.timestamp_at_last_update - || now >= end - || rewards.len() != self.reward_tokens.len() - { + // Do we also want to check that start >= now? + if start <= self.timestamp_at_last_update || now >= end || rewards.len() != tokens_len { return Err(FarmError::InvalidFarmStartParams); } - let duration = end as u128 - now as u128; - - let tokens_len = self.reward_tokens.len(); + // This substraction should be "checked" + // It's important that we substract "start" and not "now" + let duration = end as u128 - start as u128; let mut reward_rates = Vec::with_capacity(tokens_len); for (token_id, reward_amount) in self.reward_tokens.iter().zip(rewards.iter()) { + // Why can't we start a farm with 0 rewards for one of the tokens? if *reward_amount == 0 { return Err(FarmError::InvalidFarmStartParams); } @@ -228,6 +230,8 @@ mod farm { .checked_div(duration) .ok_or(FarmError::InvalidFarmStartParams)?; + // This also prevents giving zero rewards for any token. + // It would perhaps make more sense to just check if sum of all reward rates is positive if reward_rate == 0 { return Err(FarmError::InvalidFarmStartParams); } @@ -239,6 +243,7 @@ mod farm { fn deposit(&mut self, account: AccountId, amount: u128) -> Result<(), FarmError> { if amount == 0 { + // that's a very confusing error return Err(FarmError::PSP22Error(PSP22Error::InsufficientBalance)); } self.update()?; @@ -262,6 +267,7 @@ mod farm { self.pool_id } + // wouldn't just "total_shares" be a better name, "total_supply" might suggest that farm has its own token? #[ink(message)] fn total_supply(&self) -> u128 { self.total_shares @@ -319,6 +325,8 @@ mod farm { assert!(self.pool_id != token); let mut token_ref = token.into(); + // To me it seems that both "safe" calls in this functions should fail when the error arises. + // Effect is actually the same, but returning that the call was succesfull might be misleading let balance: Balance = safe_balance_of(&token_ref, self.env().account_id()); let balance = if let Some(token_index) = self.reward_tokens.iter().position(|&t| t == token) { @@ -346,6 +354,10 @@ mod farm { if user_reward > 0 { let mut psp22_ref: ink::contract_ref!(PSP22) = self.reward_tokens[idx].into(); self.farm_distributed_unclaimed_rewards[idx] -= user_reward; + // Here we have changed farm_distributed_unchanged_rewards, so we shouldn't just ignore the result of the call + // I see two alternatives: + // - match on the result of the call and don't change storage if the call fails, however that would require us to some data back to "user_claimable_rewards" + // - replace this call with "claim_reward" which would only transfer the reward for the given token safe_transfer(&mut psp22_ref, account, user_reward); } } @@ -372,6 +384,7 @@ mod farm { fn deposit_all(&mut self) -> Result<(), FarmError> { let account = self.env().caller(); let pool: contract_ref!(PSP22) = self.pool_id.into(); + // Why wouldn't we want to fail here? When "balance_of" fails here, then this is an noop, but will be reported as success. let amount = safe_balance_of(&pool, account); self.deposit(account, amount)?; FarmContract::emit_event(self.env(), Event::Deposited(Deposited { account, amount })); @@ -390,6 +403,7 @@ mod farm { self.shares.insert(account, &new_shares); self.total_shares -= amount; } else { + // That suggests insufficient token balance for some on-chain account, when in fact it's insufficent shares (which are tokens, but with balance tracked in the farm contract). return Err(PSP22Error::InsufficientBalance.into()); } @@ -412,6 +426,7 @@ mod farm { } } + // Would suggest adding some explanation why we want this. pub fn safe_transfer(psp22: &mut contract_ref!(PSP22), recipient: AccountId, amount: u128) { match psp22 .call_mut() @@ -453,6 +468,7 @@ mod farm { from_timestamp: u128, to_timestamp: u128, ) -> Result { + // wouldn't right side `from_timestamp >= to_timestamp` make slightly more sense? if total_shares == 0 || from_timestamp > to_timestamp { return Ok(0.into()); } @@ -478,8 +494,9 @@ mod farm { .checked_mul(U256::from(shares)) .ok_or(MathError::Overflow)? .checked_div(U256::from(SCALING_FACTOR)) + // this is not an overflow, but a division by zero, no? (which shouldn't ever happen) .ok_or(MathError::Overflow)? .try_into() .map_err(|_| MathError::CastOverflow) } -} \ No newline at end of file +} diff --git a/farm/trait/lib.rs b/farm/trait/lib.rs index 35159b84..85458863 100644 --- a/farm/trait/lib.rs +++ b/farm/trait/lib.rs @@ -13,6 +13,7 @@ pub enum FarmError { CallerNotOwner, ArithmeticError(MathError), CallerNotFarmer, + // Feel like this error is very generic, and does not really help in identyfying the problem. InvalidFarmStartParams, } @@ -23,6 +24,7 @@ pub struct FarmDetails { pub start: u64, pub end: u64, pub reward_tokens: Vec, + // It's not obvious what it is just from a name, we might want to have a comment on that. pub reward_rates: Vec, } @@ -52,10 +54,12 @@ pub trait Farm { #[ink(message)] fn balance_of(&self, account: AccountId) -> u128; + // This is a bit confusing, because withdrawing "from" caller could imply that tokens are taken from his account. /// Withdraws `amount` of shares from caller. #[ink(message)] fn withdraw(&mut self, amount: u128) -> Result<(), FarmError>; + // Also, it's not clear from the comment what exactly happens here, as "account" usually means on-chain account in which funds are stored. /// Deposits `amount` of shares under caller's account. #[ink(message)] fn deposit(&mut self, amount: u128) -> Result<(), FarmError>; @@ -68,9 +72,12 @@ pub trait Farm { #[ink(message)] fn reward_tokens(&self) -> Vec; + // Don't we want descriptions for these methods? + // They won't be used by the avarage user, so I guess it's not required, but stull would be nice to have (as somebody might want to create his own farm). #[ink(message)] fn owner_start_new_farm( &mut self, + // In implementation, these are `Timestamp`, maybe we should have the same type in both places? start: u64, end: u64, rewards: Vec, From 6ee8ec78b606aaf2e789ca86f3fc65d8160af746 Mon Sep 17 00:00:00 2001 From: deuszx Date: Wed, 10 Jan 2024 18:51:17 +0100 Subject: [PATCH 02/12] Allow for zero rewards (rates). --- Cargo.lock | 1 - farm/contract/lib.rs | 18 ++++++------------ 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d618e748..5c26c0c2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -260,7 +260,6 @@ dependencies = [ "primitive-types", "psp22 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)", "scale-info", - "sp-arithmetic", ] [[package]] diff --git a/farm/contract/lib.rs b/farm/contract/lib.rs index 58be5ec7..0774a860 100644 --- a/farm/contract/lib.rs +++ b/farm/contract/lib.rs @@ -212,11 +212,6 @@ mod farm { let mut reward_rates = Vec::with_capacity(tokens_len); for (token_id, reward_amount) in self.reward_tokens.iter().zip(rewards.iter()) { - // Why can't we start a farm with 0 rewards for one of the tokens? - if *reward_amount == 0 { - return Err(FarmError::InvalidFarmStartParams); - } - let mut psp22_ref: ink::contract_ref!(PSP22) = (*token_id).into(); psp22_ref.transfer_from( @@ -228,16 +223,15 @@ mod farm { let reward_rate = reward_amount .checked_div(duration) - .ok_or(FarmError::InvalidFarmStartParams)?; - - // This also prevents giving zero rewards for any token. - // It would perhaps make more sense to just check if sum of all reward rates is positive - if reward_rate == 0 { - return Err(FarmError::InvalidFarmStartParams); - } + .ok_or(FarmError::ArithmeticError(MathError::DivByZero(3)))?; reward_rates.push(reward_rate); } + + if reward_rates.iter().all(|rr| *rr == 0) { + return Err(FarmError::AllRewardRatesZero); + } + Ok(reward_rates) } From cb90fecd469fcdfa1910c8f7e74239f7edf8f767 Mon Sep 17 00:00:00 2001 From: deuszx Date: Wed, 10 Jan 2024 18:52:07 +0100 Subject: [PATCH 03/12] Address various comments about naming and conditionals --- farm/contract/lib.rs | 65 +++++++++++++++++++++++--------------------- farm/trait/lib.rs | 14 +++++++--- helpers/math.rs | 4 +-- 3 files changed, 46 insertions(+), 37 deletions(-) diff --git a/farm/contract/lib.rs b/farm/contract/lib.rs index 0774a860..79f55724 100644 --- a/farm/contract/lib.rs +++ b/farm/contract/lib.rs @@ -16,7 +16,6 @@ mod farm { use ink::prelude::{vec, vec::Vec}; use primitive_types::U256; - use psp22::PSP22Error; use psp22::PSP22; pub const SCALING_FACTOR: u128 = u128::MAX; @@ -77,7 +76,7 @@ mod farm { /// cumulative_per_share at the last update for each user. pub user_cumulative_reward_last_update: Mapping>, - // Missing documentation + /// Reward rates per user of unclaimed, accumulated users' rewards. pub user_claimable_rewards: Mapping>, } @@ -86,10 +85,10 @@ mod farm { pub fn new(pool_id: AccountId, reward_tokens: Vec) -> Result { let n_reward_tokens = reward_tokens.len(); if n_reward_tokens > MAX_REWARD_TOKENS as usize { - return Err(FarmError::InvalidFarmStartParams); + return Err(FarmError::TooManyRewardTokens); } if reward_tokens.contains(&pool_id) { - return Err(FarmError::InvalidFarmStartParams); + return Err(FarmError::RewardTokenIsPoolToken); } Ok(FarmContract { pool_id, @@ -101,8 +100,7 @@ mod farm { end: 0, timestamp_at_last_update: 0, farm_distributed_unclaimed_rewards: vec![0; n_reward_tokens], - // I am a bit biased against using defaults, but it's correct, so non-important. - farm_cumulative_reward_per_share: vec![WrappedU256::default(); n_reward_tokens], + farm_cumulative_reward_per_share: vec![WrappedU256::ZERO; n_reward_tokens], farm_reward_rates: vec![0; n_reward_tokens], user_cumulative_reward_last_update: Mapping::default(), user_claimable_rewards: Mapping::default(), @@ -123,8 +121,7 @@ mod farm { let prev = core::cmp::max(self.timestamp_at_last_update, self.start); let now = core::cmp::min(current_timestamp, self.end); - // right side of this alternative will never happen, because of the check above - if prev >= now || self.timestamp_at_last_update == current_timestamp { + if prev >= now { self.timestamp_at_last_update = current_timestamp; return Ok(()); } @@ -132,8 +129,7 @@ mod farm { // At this point we know [prev, now] is the intersection of [self.start, self.end] and [self.timestamp_at_last_update, current_timestamp] // It is non-empty because of the checks above and self.start <= now <= self.end - // why enumerate instead of 0..self.reward_tokens.len()? - for (idx, _) in self.reward_tokens.iter().enumerate() { + for idx in 0..self.reward_tokens.len() { let delta_reward_per_share = rewards_per_share_in_time_interval( self.farm_reward_rates[idx], self.total_shares, @@ -201,14 +197,26 @@ mod farm { let now = Self::env().block_timestamp(); let tokens_len = self.reward_tokens.len(); - // Do we also want to check that start >= now? - if start <= self.timestamp_at_last_update || now >= end || rewards.len() != tokens_len { - return Err(FarmError::InvalidFarmStartParams); + if rewards.len() != tokens_len { + return Err(FarmError::RewardsTokensMismatch); } - // This substraction should be "checked" - // It's important that we substract "start" and not "now" - let duration = end as u128 - start as u128; + // NOTE: `timestamp_at_last_update == now` in `self.update()` called before this. + + if start < now { + return Err(FarmError::FarmStartInThePast); + } + + if end <= now { + return Err(FarmError::FarmEndInThePast); + } + + let duration = if let Some(duration) = end.checked_sub(start) { + duration as u128 + } else { + return Err(FarmError::FarmDuration); + }; + let mut reward_rates = Vec::with_capacity(tokens_len); for (token_id, reward_amount) in self.reward_tokens.iter().zip(rewards.iter()) { @@ -237,8 +245,7 @@ mod farm { fn deposit(&mut self, account: AccountId, amount: u128) -> Result<(), FarmError> { if amount == 0 { - // that's a very confusing error - return Err(FarmError::PSP22Error(PSP22Error::InsufficientBalance)); + return Err(FarmError::InsufficientShares); } self.update()?; self.update_account(account); @@ -263,12 +270,12 @@ mod farm { // wouldn't just "total_shares" be a better name, "total_supply" might suggest that farm has its own token? #[ink(message)] - fn total_supply(&self) -> u128 { + fn total_shares(&self) -> u128 { self.total_shares } #[ink(message)] - fn balance_of(&self, owner: AccountId) -> u128 { + fn shares_of(&self, owner: AccountId) -> u128 { self.shares.get(owner).unwrap_or(0) } @@ -378,8 +385,7 @@ mod farm { fn deposit_all(&mut self) -> Result<(), FarmError> { let account = self.env().caller(); let pool: contract_ref!(PSP22) = self.pool_id.into(); - // Why wouldn't we want to fail here? When "balance_of" fails here, then this is an noop, but will be reported as success. - let amount = safe_balance_of(&pool, account); + let amount = pool.balance_of(account); self.deposit(account, amount)?; FarmContract::emit_event(self.env(), Event::Deposited(Deposited { account, amount })); Ok(()) @@ -397,8 +403,7 @@ mod farm { self.shares.insert(account, &new_shares); self.total_shares -= amount; } else { - // That suggests insufficient token balance for some on-chain account, when in fact it's insufficent shares (which are tokens, but with balance tracked in the farm contract). - return Err(PSP22Error::InsufficientBalance.into()); + return Err(FarmError::InsufficientShares); } let mut pool: contract_ref!(PSP22) = self.pool_id.into(); @@ -462,8 +467,7 @@ mod farm { from_timestamp: u128, to_timestamp: u128, ) -> Result { - // wouldn't right side `from_timestamp >= to_timestamp` make slightly more sense? - if total_shares == 0 || from_timestamp > to_timestamp { + if total_shares == 0 || from_timestamp >= to_timestamp { return Ok(0.into()); } @@ -473,9 +477,9 @@ mod farm { casted_mul(reward_rate, time_delta) .checked_mul(U256::from(SCALING_FACTOR)) - .ok_or(MathError::Overflow)? + .ok_or(MathError::Overflow(1))? .checked_div(U256::from(total_shares)) - .ok_or(MathError::DivByZero) + .ok_or(MathError::DivByZero(1)) } /// The formula is: @@ -486,10 +490,9 @@ mod farm { ) -> Result { rewards_per_share .checked_mul(U256::from(shares)) - .ok_or(MathError::Overflow)? + .ok_or(MathError::Overflow(2))? .checked_div(U256::from(SCALING_FACTOR)) - // this is not an overflow, but a division by zero, no? (which shouldn't ever happen) - .ok_or(MathError::Overflow)? + .ok_or(MathError::DivByZero(2))? .try_into() .map_err(|_| MathError::CastOverflow) } diff --git a/farm/trait/lib.rs b/farm/trait/lib.rs index 85458863..37863a31 100644 --- a/farm/trait/lib.rs +++ b/farm/trait/lib.rs @@ -13,8 +13,14 @@ pub enum FarmError { CallerNotOwner, ArithmeticError(MathError), CallerNotFarmer, - // Feel like this error is very generic, and does not really help in identyfying the problem. - InvalidFarmStartParams, + AllRewardRatesZero, + FarmStartInThePast, + FarmEndInThePast, + FarmDuration, + InsufficientShares, + RewardsTokensMismatch, + TooManyRewardTokens, + RewardTokenIsPoolToken, } #[derive(Debug, PartialEq, Eq, scale::Encode, scale::Decode)] @@ -48,11 +54,11 @@ pub trait Farm { /// Returns total supply of LP tokens deposited for this farm. #[ink(message)] - fn total_supply(&self) -> u128; + fn total_shares(&self) -> u128; /// Returns share of LP tokens deposited by the `account` in this farm. #[ink(message)] - fn balance_of(&self, account: AccountId) -> u128; + fn shares_of(&self, account: AccountId) -> u128; // This is a bit confusing, because withdrawing "from" caller could imply that tokens are taken from his account. /// Withdraws `amount` of shares from caller. diff --git a/helpers/math.rs b/helpers/math.rs index 1207c2be..658dbf05 100644 --- a/helpers/math.rs +++ b/helpers/math.rs @@ -7,8 +7,8 @@ pub fn casted_mul(a: u128, b: u128) -> U256 { #[derive(Debug, PartialEq, Eq, scale::Encode, scale::Decode)] #[cfg_attr(feature = "std", derive(scale_info::TypeInfo))] pub enum MathError { - Overflow, + Overflow(u8), Underflow, - DivByZero, + DivByZero(u8), CastOverflow, } From d2ba634a16159a3d6c3241716f329283c260fdfa Mon Sep 17 00:00:00 2001 From: deuszx Date: Wed, 10 Jan 2024 19:09:19 +0100 Subject: [PATCH 04/12] Replace assert! with ensure! --- farm/contract/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/farm/contract/lib.rs b/farm/contract/lib.rs index 79f55724..d1185364 100644 --- a/farm/contract/lib.rs +++ b/farm/contract/lib.rs @@ -4,7 +4,7 @@ mod farm { type TokenId = AccountId; type UserId = AccountId; - use amm_helpers::{math::casted_mul, types::WrappedU256}; + use amm_helpers::{ensure, math::casted_mul, types::WrappedU256}; use farm_trait::{Farm, FarmDetails, FarmError}; use ink::{ codegen::{EmitEvent, TraitCallBuilder}, @@ -320,10 +320,10 @@ mod farm { return Err(FarmError::CallerNotOwner); } self.update()?; - assert!(!self.is_active()); + ensure!(!self.is_active(), FarmError::FarmAlreadyRunning); // Owner should be able to withdraw every token except the pool token. - assert!(self.pool_id != token); + ensure!(self.pool_id != token, FarmError::RewardTokenIsPoolToken); let mut token_ref = token.into(); // To me it seems that both "safe" calls in this functions should fail when the error arises. From 87f1baf4221575638ae147e8336e8a2d35f2420f Mon Sep 17 00:00:00 2001 From: deuszx Date: Wed, 10 Jan 2024 19:15:40 +0100 Subject: [PATCH 05/12] Replace assert! with ensure! --- farm/contract/lib.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/farm/contract/lib.rs b/farm/contract/lib.rs index d1185364..4b65e90f 100644 --- a/farm/contract/lib.rs +++ b/farm/contract/lib.rs @@ -316,9 +316,7 @@ mod farm { #[ink(message)] fn owner_withdraw_token(&mut self, token: TokenId) -> Result<(), FarmError> { - if self.env().caller() != self.owner { - return Err(FarmError::CallerNotOwner); - } + ensure!(self.env().caller() == self.owner, FarmError::CallerNotOwner); self.update()?; ensure!(!self.is_active(), FarmError::FarmAlreadyRunning); From c096e227dd2b6de3d6229a91bc5ac0f2038290e0 Mon Sep 17 00:00:00 2001 From: deuszx Date: Wed, 10 Jan 2024 19:21:20 +0100 Subject: [PATCH 06/12] Rewrite code to get rid of safe_balance_of. It was used only in owner_withdraw_token(reward_token) and we can fail the whole txn there if reward_token.balance_of turns out to be reverting. This is not blocking any loop from execution, since there's just one specific token argument. --- farm/contract/lib.rs | 39 +++++++++++---------------------------- 1 file changed, 11 insertions(+), 28 deletions(-) diff --git a/farm/contract/lib.rs b/farm/contract/lib.rs index 4b65e90f..bb4e7245 100644 --- a/farm/contract/lib.rs +++ b/farm/contract/lib.rs @@ -322,18 +322,17 @@ mod farm { // Owner should be able to withdraw every token except the pool token. ensure!(self.pool_id != token, FarmError::RewardTokenIsPoolToken); - let mut token_ref = token.into(); - - // To me it seems that both "safe" calls in this functions should fail when the error arises. - // Effect is actually the same, but returning that the call was succesfull might be misleading - let balance: Balance = safe_balance_of(&token_ref, self.env().account_id()); - let balance = - if let Some(token_index) = self.reward_tokens.iter().position(|&t| t == token) { - balance.saturating_sub(self.farm_distributed_unclaimed_rewards[token_index]) - } else { - balance - }; - safe_transfer(&mut token_ref, self.owner, balance); + let mut token_ref: contract_ref!(PSP22) = token.into(); + + let total_balance = token_ref.balance_of(self.env().account_id()); + let undistributed_balance = if let Some(token_index) = + self.reward_tokens.iter().position(|&t| t == token) + { + total_balance.saturating_sub(self.farm_distributed_unclaimed_rewards[token_index]) + } else { + total_balance + }; + token_ref.transfer(self.owner, undistributed_balance, vec![])?; Ok(()) } @@ -443,22 +442,6 @@ mod farm { } } - // We don't want to fail the whole transaction if PSP22::balance_of fails with a panic either. - // We choose to use `0` to denote the "panic" scenarios b/c it's a noop for the farm. - pub fn safe_balance_of(psp22: &contract_ref!(PSP22), account: AccountId) -> u128 { - match psp22.call().balance_of(account).try_invoke() { - Err(ink_env_err) => { - ink::env::debug_println!("ink env error: {:?}", ink_env_err); - 0 - } - Ok(Err(ink_lang_err)) => { - ink::env::debug_println!("ink lang error: {:?}", ink_lang_err); - 0 - } - Ok(Ok(res)) => res, - } - } - pub fn rewards_per_share_in_time_interval( reward_rate: u128, total_shares: u128, From a69b72de246cc92336234b9c4b24cc0807687a41 Mon Sep 17 00:00:00 2001 From: deuszx Date: Wed, 10 Jan 2024 20:24:54 +0100 Subject: [PATCH 07/12] Change claim_rewards API to accept reward tokens arg --- farm/contract/lib.rs | 73 +++++++++++++++++++------------------------- farm/trait/lib.rs | 4 ++- 2 files changed, 34 insertions(+), 43 deletions(-) diff --git a/farm/contract/lib.rs b/farm/contract/lib.rs index bb4e7245..1f10c1bd 100644 --- a/farm/contract/lib.rs +++ b/farm/contract/lib.rs @@ -6,12 +6,7 @@ mod farm { type UserId = AccountId; use amm_helpers::{ensure, math::casted_mul, types::WrappedU256}; use farm_trait::{Farm, FarmDetails, FarmError}; - use ink::{ - codegen::{EmitEvent, TraitCallBuilder}, - contract_ref, - reflect::ContractEventBase, - storage::Mapping, - }; + use ink::{codegen::EmitEvent, contract_ref, reflect::ContractEventBase, storage::Mapping}; use ink::prelude::{vec, vec::Vec}; use primitive_types::U256; @@ -39,7 +34,7 @@ mod farm { pub struct RewardsClaimed { #[ink(topic)] account: AccountId, - amounts: Vec, + rewards_claimed: Vec<(AccountId, u128)>, } use amm_helpers::math::MathError; @@ -332,39 +327,53 @@ mod farm { } else { total_balance }; - token_ref.transfer(self.owner, undistributed_balance, vec![])?; - Ok(()) + Ok(token_ref.transfer(self.owner, undistributed_balance, vec![])?) } // To learn how much rewards the user has, it's best to dry-run claim_rewards #[ink(message)] - fn claim_rewards(&mut self) -> Result, FarmError> { + fn claim_rewards(&mut self, tokens: Vec) -> Result, FarmError> { self.update()?; let account = self.env().caller(); self.update_account(account); - let user_rewards = self + let mut user_rewards = self .user_claimable_rewards - .take(account) + .get(account) .ok_or(FarmError::CallerNotFarmer)?; - for (idx, user_reward) in user_rewards.clone().into_iter().enumerate() { - if user_reward > 0 { - let mut psp22_ref: ink::contract_ref!(PSP22) = self.reward_tokens[idx].into(); - self.farm_distributed_unclaimed_rewards[idx] -= user_reward; - // Here we have changed farm_distributed_unchanged_rewards, so we shouldn't just ignore the result of the call - // I see two alternatives: - // - match on the result of the call and don't change storage if the call fails, however that would require us to some data back to "user_claimable_rewards" - // - replace this call with "claim_reward" which would only transfer the reward for the given token - safe_transfer(&mut psp22_ref, account, user_reward); + let mut rewards_claimed: Vec<(TokenId, u128)> = Vec::with_capacity(tokens.len()); + + for token in tokens { + if let Some(idx) = self + .reward_tokens + .iter() + .position(|rtoken| rtoken == &token) + { + let user_reward = user_rewards[idx]; + if user_reward > 0 { + user_rewards[idx] = 0; + let mut psp22_ref: ink::contract_ref!(PSP22) = token.into(); + self.farm_distributed_unclaimed_rewards[idx] -= user_reward; + psp22_ref + .transfer(account, user_reward, vec![]) + .map_err(|e| FarmError::TokenTransferFailed(token, e))?; + } + rewards_claimed.push((token, user_reward)); + } else { + return Err(FarmError::TokenNotReward(token)); } } + if user_rewards.iter().all(|r| *r == 0) { + self.user_claimable_rewards.remove(account) + } + FarmContract::emit_event( self.env(), Event::RewardsClaimed(RewardsClaimed { account, - amounts: user_rewards.clone(), + rewards_claimed, }), ); Ok(user_rewards) @@ -422,26 +431,6 @@ mod farm { } } - // Would suggest adding some explanation why we want this. - pub fn safe_transfer(psp22: &mut contract_ref!(PSP22), recipient: AccountId, amount: u128) { - match psp22 - .call_mut() - .transfer(recipient, amount, vec![]) - .try_invoke() - { - Err(ink_env_err) => { - ink::env::debug_println!("ink env error: {:?}", ink_env_err); - } - Ok(Err(ink_lang_err)) => { - ink::env::debug_println!("ink lang error: {:?}", ink_lang_err); - } - Ok(Ok(Err(psp22_error))) => { - ink::env::debug_println!("psp22 error: {:?}", psp22_error); - } - Ok(Ok(Ok(_))) => {} - } - } - pub fn rewards_per_share_in_time_interval( reward_rate: u128, total_shares: u128, diff --git a/farm/trait/lib.rs b/farm/trait/lib.rs index 37863a31..797d1159 100644 --- a/farm/trait/lib.rs +++ b/farm/trait/lib.rs @@ -21,6 +21,8 @@ pub enum FarmError { RewardsTokensMismatch, TooManyRewardTokens, RewardTokenIsPoolToken, + TokenNotReward(AccountId), + TokenTransferFailed(AccountId, PSP22Error), } #[derive(Debug, PartialEq, Eq, scale::Encode, scale::Decode)] @@ -96,7 +98,7 @@ pub trait Farm { fn owner_withdraw_token(&mut self, token: AccountId) -> Result<(), FarmError>; #[ink(message)] - fn claim_rewards(&mut self) -> Result, FarmError>; + fn claim_rewards(&mut self, tokens: Vec) -> Result, FarmError>; #[ink(message)] fn view_farm_details(&self) -> FarmDetails; From eeff6922c26ece68e39b452f8a1deca6401cb53b Mon Sep 17 00:00:00 2001 From: deuszx Date: Thu, 11 Jan 2024 17:44:48 +0100 Subject: [PATCH 08/12] Address review of the review comments --- farm/contract/lib.rs | 41 +++++++++++++++++++---------------------- farm/trait/lib.rs | 5 ++--- 2 files changed, 21 insertions(+), 25 deletions(-) diff --git a/farm/contract/lib.rs b/farm/contract/lib.rs index 1f10c1bd..2a957fa6 100644 --- a/farm/contract/lib.rs +++ b/farm/contract/lib.rs @@ -193,7 +193,7 @@ mod farm { let tokens_len = self.reward_tokens.len(); if rewards.len() != tokens_len { - return Err(FarmError::RewardsTokensMismatch); + return Err(FarmError::RewardsVecLengthMismatch); } // NOTE: `timestamp_at_last_update == now` in `self.update()` called before this. @@ -327,12 +327,13 @@ mod farm { } else { total_balance }; - Ok(token_ref.transfer(self.owner, undistributed_balance, vec![])?) + token_ref.transfer(self.owner, undistributed_balance, vec![])?; + Ok(()) } - // To learn how much rewards the user has, it's best to dry-run claim_rewards + // To learn how much rewards the user has, it's best to dry-run claim_rewards. #[ink(message)] - fn claim_rewards(&mut self, tokens: Vec) -> Result, FarmError> { + fn claim_rewards(&mut self, tokens: Vec) -> Result, FarmError> { self.update()?; let account = self.env().caller(); self.update_account(account); @@ -344,29 +345,25 @@ mod farm { let mut rewards_claimed: Vec<(TokenId, u128)> = Vec::with_capacity(tokens.len()); - for token in tokens { - if let Some(idx) = self - .reward_tokens - .iter() - .position(|rtoken| rtoken == &token) - { - let user_reward = user_rewards[idx]; - if user_reward > 0 { - user_rewards[idx] = 0; - let mut psp22_ref: ink::contract_ref!(PSP22) = token.into(); - self.farm_distributed_unclaimed_rewards[idx] -= user_reward; - psp22_ref - .transfer(account, user_reward, vec![]) - .map_err(|e| FarmError::TokenTransferFailed(token, e))?; - } + for token_idx in tokens { + let idx = token_idx as usize; + let token = self.reward_tokens[idx]; + let user_reward = user_rewards[idx]; + if user_reward > 0 { + user_rewards[idx] = 0; + let mut psp22_ref: ink::contract_ref!(PSP22) = token.into(); + self.farm_distributed_unclaimed_rewards[idx] -= user_reward; + psp22_ref + .transfer(account, user_reward, vec![]) + .map_err(|e| FarmError::TokenTransferFailed(token, e))?; rewards_claimed.push((token, user_reward)); - } else { - return Err(FarmError::TokenNotReward(token)); } } if user_rewards.iter().all(|r| *r == 0) { - self.user_claimable_rewards.remove(account) + self.user_claimable_rewards.remove(account); + } else { + self.user_claimable_rewards.insert(account, &user_rewards); } FarmContract::emit_event( diff --git a/farm/trait/lib.rs b/farm/trait/lib.rs index 797d1159..9c0dce01 100644 --- a/farm/trait/lib.rs +++ b/farm/trait/lib.rs @@ -18,10 +18,9 @@ pub enum FarmError { FarmEndInThePast, FarmDuration, InsufficientShares, - RewardsTokensMismatch, + RewardsVecLengthMismatch, TooManyRewardTokens, RewardTokenIsPoolToken, - TokenNotReward(AccountId), TokenTransferFailed(AccountId, PSP22Error), } @@ -98,7 +97,7 @@ pub trait Farm { fn owner_withdraw_token(&mut self, token: AccountId) -> Result<(), FarmError>; #[ink(message)] - fn claim_rewards(&mut self, tokens: Vec) -> Result, FarmError>; + fn claim_rewards(&mut self, tokens: Vec) -> Result, FarmError>; #[ink(message)] fn view_farm_details(&self) -> FarmDetails; From 3b69e9a77722a0af695e05d83440a6c9760b0bb3 Mon Sep 17 00:00:00 2001 From: deuszx Date: Fri, 12 Jan 2024 11:43:17 +0100 Subject: [PATCH 09/12] Add comments --- farm/trait/lib.rs | 36 +++++++++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/farm/trait/lib.rs b/farm/trait/lib.rs index 9c0dce01..d356db89 100644 --- a/farm/trait/lib.rs +++ b/farm/trait/lib.rs @@ -24,14 +24,21 @@ pub enum FarmError { TokenTransferFailed(AccountId, PSP22Error), } +/// Summary of the farm's details. +/// +/// Useful for display purposes. #[derive(Debug, PartialEq, Eq, scale::Encode, scale::Decode)] #[cfg_attr(feature = "std", derive(scale_info::TypeInfo))] pub struct FarmDetails { + /// Address of a DEX pair for which this farm is created. pub pool_id: AccountId, + /// Start timestamp of the latest farm instance. pub start: u64, + /// End timestamp of the latest farm instance. pub end: u64, + /// Vector of PSP22 token addresses that are paid out as rewards. pub reward_tokens: Vec, - // It's not obvious what it is just from a name, we might want to have a comment on that. + /// Vector of rewards rates paid out for locking LP tokens per smallest unit of time. pub reward_rates: Vec, } @@ -61,13 +68,11 @@ pub trait Farm { #[ink(message)] fn shares_of(&self, account: AccountId) -> u128; - // This is a bit confusing, because withdrawing "from" caller could imply that tokens are taken from his account. - /// Withdraws `amount` of shares from caller. + /// Withdraws `amount` of shares from caller's stake in the farm. #[ink(message)] fn withdraw(&mut self, amount: u128) -> Result<(), FarmError>; - // Also, it's not clear from the comment what exactly happens here, as "account" usually means on-chain account in which funds are stored. - /// Deposits `amount` of shares under caller's account. + /// Deposits `amount` of LP tokens (shares) under caller's account in the farm. #[ink(message)] fn deposit(&mut self, amount: u128) -> Result<(), FarmError>; @@ -79,8 +84,9 @@ pub trait Farm { #[ink(message)] fn reward_tokens(&self) -> Vec; - // Don't we want descriptions for these methods? - // They won't be used by the avarage user, so I guess it's not required, but stull would be nice to have (as somebody might want to create his own farm). + /// Sets the parameters of the farm (`start`, `end`, `rewards`). + /// + /// NOTE: Implementation should make sure that it's callabled only by a permission account (owner of the farm). #[ink(message)] fn owner_start_new_farm( &mut self, @@ -90,15 +96,31 @@ pub trait Farm { rewards: Vec, ) -> Result<(), FarmError>; + /// Generic method that allows for stopping (a running) farm. + /// Details are implementation-dependent (Common AMM will set the farm's `end` timestamp to current blocktime). + /// + /// NOTE: Implementation should make sure that it's callabled only by a permission account (owner of the farm). #[ink(message)] fn owner_stop_farm(&mut self) -> Result<(), FarmError>; + /// NOTE: Implementation should make sure that it's callabled only by a permission account (owner of the farm). #[ink(message)] fn owner_withdraw_token(&mut self, token: AccountId) -> Result<(), FarmError>; + /// Requests farming rewards that have been accumulated to the caller of this method. + /// + /// Arguments: + /// `tokens` - vector of tokens' indices to be claimed. + /// + /// NOTE: To acquire token indices, one can query the `view_farm_details` + /// and use `reward_tokens` information for that. + /// + /// It may happen that one of the reward tokens is malicious and fails during the operation, + /// in such case it's advised to filter out that token from the `tokens` list. #[ink(message)] fn claim_rewards(&mut self, tokens: Vec) -> Result, FarmError>; + /// Returns information about the current farm instance. #[ink(message)] fn view_farm_details(&self) -> FarmDetails; } From 248579476c9ed7c3d3379eacbc0f4066a6cba959 Mon Sep 17 00:00:00 2001 From: deuszx Date: Fri, 12 Jan 2024 13:34:07 +0100 Subject: [PATCH 10/12] Fix tests --- farm/trait/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/farm/trait/lib.rs b/farm/trait/lib.rs index d356db89..e1994001 100644 --- a/farm/trait/lib.rs +++ b/farm/trait/lib.rs @@ -114,7 +114,6 @@ pub trait Farm { /// /// NOTE: To acquire token indices, one can query the `view_farm_details` /// and use `reward_tokens` information for that. - /// /// It may happen that one of the reward tokens is malicious and fails during the operation, /// in such case it's advised to filter out that token from the `tokens` list. #[ink(message)] From d2e0610106d02aab90e82eed29f44c61ef1bd495 Mon Sep 17 00:00:00 2001 From: deuszx Date: Fri, 12 Jan 2024 13:34:59 +0100 Subject: [PATCH 11/12] Fix typos --- farm/trait/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/farm/trait/lib.rs b/farm/trait/lib.rs index e1994001..c834180a 100644 --- a/farm/trait/lib.rs +++ b/farm/trait/lib.rs @@ -86,7 +86,7 @@ pub trait Farm { /// Sets the parameters of the farm (`start`, `end`, `rewards`). /// - /// NOTE: Implementation should make sure that it's callabled only by a permission account (owner of the farm). + /// NOTE: Implementation should make sure that it's callable only by an authorized account (owner of the farm). #[ink(message)] fn owner_start_new_farm( &mut self, @@ -99,11 +99,11 @@ pub trait Farm { /// Generic method that allows for stopping (a running) farm. /// Details are implementation-dependent (Common AMM will set the farm's `end` timestamp to current blocktime). /// - /// NOTE: Implementation should make sure that it's callabled only by a permission account (owner of the farm). + /// NOTE: Implementation should make sure that it's callable only by an authorized account (owner of the farm). #[ink(message)] fn owner_stop_farm(&mut self) -> Result<(), FarmError>; - /// NOTE: Implementation should make sure that it's callabled only by a permission account (owner of the farm). + /// NOTE: Implementation should make sure that it's callable only by an authorized account (owner of the farm). #[ink(message)] fn owner_withdraw_token(&mut self, token: AccountId) -> Result<(), FarmError>; From d9eea3a5ffb7492eadf517da249b47e17a3e246d Mon Sep 17 00:00:00 2001 From: deuszx <95355183+deuszx@users.noreply.github.com> Date: Fri, 12 Jan 2024 13:46:49 +0100 Subject: [PATCH 12/12] Update farm/trait/lib.rs Co-authored-by: Krzysztof Ziobro <86822080+krzysztofziobro@users.noreply.github.com> --- farm/trait/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/farm/trait/lib.rs b/farm/trait/lib.rs index c834180a..f292363b 100644 --- a/farm/trait/lib.rs +++ b/farm/trait/lib.rs @@ -90,7 +90,6 @@ pub trait Farm { #[ink(message)] fn owner_start_new_farm( &mut self, - // In implementation, these are `Timestamp`, maybe we should have the same type in both places? start: u64, end: u64, rewards: Vec,