Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature/farming interface for frontend #12

Merged
merged 4 commits into from
Oct 19, 2022
Merged

Feature/farming interface for frontend #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b286a1c
Provide tentative stake/unstake features
HyunggyuJang Oct 18, 2022
0c0d7a0
Implement add correctly
HyunggyuJang Oct 18, 2022
bcc6192
Fix deposit (initialization is needed)
HyunggyuJang Oct 18, 2022
8e2dd3a
Refactor rewarders structure
HyunggyuJang Oct 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions farming/logics/traits/data.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ pub const STORAGE_KEY: u32 = openbrush::storage_unique_key!(Data);
pub struct Pool {
pub acc_arsw_per_share: u128,
pub last_reward_block: u32,
pub alloc_point: u64,
pub alloc_point: u32,
}

#[derive(Default, Debug)]
Expand Down Expand Up @@ -47,7 +47,7 @@ pub struct Data {
pub lp_tokens: Vec<AccountId>,

/// Address of each `rewarder` contract in MasterChef.
pub rewarders: Vec<AccountId>,
pub rewarders: Mapping<u32, AccountId>,

/// Total allocation points. Must be the sum of all allocation points in all pools.
pub total_alloc_point: u32,
Expand Down
4 changes: 2 additions & 2 deletions farming/logics/traits/events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ pub trait FarmingEvents {
fn _emit_log_pool_addition_event(
&self,
_pool_id: u32,
_alloc_point: u128,
_alloc_point: u32,
_lp_token: AccountId,
_rewarder: AccountId,
_rewarder: Option<AccountId>,
) {
}

Expand Down
128 changes: 122 additions & 6 deletions farming/logics/traits/farming.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,17 @@ pub use crate::traits::{
Data,
Pool,
},
events::FarmingEvents,
getters::FarmingGetters,
};
use ink_prelude::vec::Vec;
use openbrush::{
contracts::{
ownable::*,
traits::psp22::PSP22Ref,
traits::psp22::{
PSP22Error,
PSP22Ref,
},
},
modifiers,
traits::{
Expand All @@ -25,17 +30,113 @@ pub const MAX_PERIOD: u8 = 23u8;
pub const FIRST_PERIOD_REWERD_SUPPLY: Balance = 151629858171523000000u128;

#[openbrush::trait_definition]
pub trait Farming: Storage<Data> + Storage<ownable::Data> + FarmingGetters {
pub trait Farming: Storage<Data> + Storage<ownable::Data> + FarmingGetters + FarmingEvents {
#[ink(message)]
#[modifiers(only_owner)]
fn add(
&mut self,
alloc_point: u128,
alloc_point: u32,
lp_token: AccountId,
rewarder: AccountId,
rewarder: Option<AccountId>,
) -> Result<(), FarmingError> {
self._check_pool_duplicate(lp_token)?;
self._update_all_pools()?;
self.data::<Data>().total_alloc_point = self
.data::<Data>()
.total_alloc_point
.checked_add(alloc_point)
.ok_or(FarmingError::AddOverflow2)?;
self.data::<Data>().lp_tokens.push(lp_token);
let pool_length = self.pool_length();
if let Some(rewarder_address) = rewarder {
self.data::<Data>()
.rewarders
.insert(&pool_length, &rewarder_address);
}
self.data::<Data>().pool_info.insert(
&pool_length,
&Pool {
acc_arsw_per_share: 0,
last_reward_block: Self::env().block_number(),
alloc_point,
},
);
self.data::<Data>().pool_info_length = pool_length
.checked_add(1)
.ok_or(FarmingError::AddOverflow2)?;
self._emit_log_pool_addition_event(pool_length, alloc_point, lp_token, rewarder);
Ok(())
}

#[ink(message)]
fn pending_arsw(&self, _pool_id: u32, _user: AccountId) -> Result<Balance, FarmingError> {
Ok(1_000_000_000_000_000_000)
}

#[ink(message)]
fn deposit(
&mut self,
pool_id: u32,
amount: Balance,
to: AccountId,
) -> Result<(), FarmingError> {
let (prev_amount, prev_reward_debt) = self.get_user_info(pool_id, to).unwrap_or((0, 0));
// TODO: Fix reward_debt
self.data::<Data>().user_info.insert(
&(pool_id, to),
&(
prev_amount
.checked_add(amount)
.ok_or(FarmingError::AddOverflow1)?,
prev_reward_debt,
),
);
let lp_token = self
.get_lp_token(pool_id)
.ok_or(FarmingError::PoolNotFound2)?;
PSP22Ref::transfer_from(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should allow reentrancy for this or it will not work:

        PSP22Ref::transfer_from_builder(&lp_token, Self::env().caller(), Self::env().account_id(), value, Vec::<u8>::new())
            .call_flags(CallFlags::default().set_allow_reentry(true))
            .fire()
            .unwrap()?;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I missed that! Thanks a lot. Will fix it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think we don't need to allow reentrancy. Pair contract only calls itself rather than transfer_from caller. Hence, we shouldn't allow reentrancy actually in this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I need to update in Pair contract. But here it is still relevant

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, from the pair contract, you will callback the caller. But for what?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is LP token, the pair contract is the implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, not the farming contract itself, the caller for deposit might call back the operator part of before_received: https://github.com/Supercolony-net/openbrush-contracts/blob/dc666f08c4e3bec2bf6057bb083cbf5223a28063/contracts/src/traits/psp22/psp22.rs#L160. So we need to allow reentrancy, is it correct?

If it were the case, still think we don't need to allow reentrancy.

Copy link
Member

@PierreOssun PierreOssun Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's working for Pair because transfer_from is overridden and there is no _do_safe_transfer_check

wasm-showcase-dapps/uniswap-v2/contracts/pair/lib.rs

Line 147 in 9aa96d2

fn _transfer_from_to(

But other PSP22 implementation might impl the default Internal Trait - in this one there is _do_safe_transfer_check that will do a callback and will panic if we didn't allow reentrancy with flags.

Now you can still keep it like this (as LP are Pair contract) but it will not be compatible with standard psp22

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly. For this function, I think we'd better not to allow reentrancy to suppress from using invalid LP tokens.

Copy link
Collaborator Author

@HyunggyuJang HyunggyuJang Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A note for myself:

Ah, not the farming contract itself, the caller for deposit might call back the operator part of before_received: https://github.com/Supercolony-net/openbrush-contracts/blob/dc666f08c4e3bec2bf6057bb083cbf5223a28063/contracts/src/traits/psp22/psp22.rs#L160. So we need to allow reentrancy, is it correct?

Is generally correct, but is not what happening in this situation. For the foregoing issue, the to of transfer_from is the contract itself, hence, if the PSP22 implementation is default one, then _do_safe_transfer_check calls before_received with to as a callee.
Plus, reentrancy check happens even before checking whether the callee has specified method or not, hence from foregoing context, it will raise an error saying reentrancy is not allowed.

&lp_token,
Self::env().caller(),
Self::env().account_id(),
amount,
Vec::new(),
)?;
Ok(())
}

#[ink(message)]
fn withdraw(
&mut self,
pool_id: u32,
amount: Balance,
to: AccountId,
) -> Result<(), FarmingError> {
if amount == 0 {
return Err(FarmingError::ZeroWithdrawal)
}
let caller = Self::env().caller();
let (prev_amount, prev_reward_debt) = self
.get_user_info(pool_id, caller)
.ok_or(FarmingError::UserNotFound)?;
// TODO: Fix reward_debt
self.data::<Data>().user_info.insert(
&(pool_id, caller),
&(
prev_amount
.checked_sub(amount)
.ok_or(FarmingError::SubUnderflow2)?,
prev_reward_debt,
),
);
let lp_token = self
.get_lp_token(pool_id)
.ok_or(FarmingError::PoolNotFound3)?;
PSP22Ref::transfer(&lp_token, to, amount, Vec::new())?;
Ok(())
}

#[ink(message)]
fn harvest(&mut self, _pool_id: u32, _to: AccountId) -> Result<(), FarmingError> {
Ok(())
}

Expand All @@ -58,7 +159,7 @@ pub trait Farming: Storage<Data> + Storage<ownable::Data> + FarmingGetters {
fn _update_pool(&mut self, pool_id: u32) -> Result<(), FarmingError> {
let pool = self
.get_pool_infos(pool_id)
.ok_or(FarmingError::PoolNotFound)?;
.ok_or(FarmingError::PoolNotFound1)?;
let current_block = Self::env().block_number();
if current_block > pool.last_reward_block {
let lp_token = self
Expand Down Expand Up @@ -104,16 +205,31 @@ pub trait Farming: Storage<Data> + Storage<ownable::Data> + FarmingGetters {
#[cfg_attr(feature = "std", derive(scale_info::TypeInfo))]
pub enum FarmingError {
OwnableError(OwnableError),
PSP22Error(PSP22Error),
DuplicateLPToken,
PoolNotFound,
PoolNotFound1,
PoolNotFound2,
PoolNotFound3,
UserNotFound,
ZeroWithdrawal,
LpTokenNotFound,
LpSupplyIsZero,
BlockNumberLowerThanOriginBlock,
SubUnderflow1,
SubUnderflow2,
AddOverflow1,
AddOverflow2,
AddOverflow3,
}

impl From<OwnableError> for FarmingError {
fn from(error: OwnableError) -> Self {
FarmingError::OwnableError(error)
}
}

impl From<PSP22Error> for FarmingError {
fn from(error: PSP22Error) -> Self {
FarmingError::PSP22Error(error)
}
}
5 changes: 5 additions & 0 deletions farming/logics/traits/getters.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ pub trait FarmingGetters: Storage<Data> {
self.data::<Data>().pool_info.get(&pool_id)
}

#[ink(message)]
fn get_user_info(&self, pool_id: u32, user: AccountId) -> Option<(u128, i128)> {
self.data::<Data>().user_info.get(&(pool_id, user))
}

#[ink(message)]
fn get_lp_token(&self, pool_id: u32) -> Option<AccountId> {
self.data::<Data>().lp_tokens.get(pool_id as usize).copied()
Expand Down