-
Notifications
You must be signed in to change notification settings - Fork 19
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.
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
feat: add XYK liquidity mining pallet #527
Conversation
Conflicts: Cargo.lock runtime/basilisk/Cargo.toml runtime/basilisk/src/lib.rs runtime/testing-basilisk/Cargo.toml runtime/testing-basilisk/src/lib.rs
Crate versions that have not been updated:
New crates:
Crate versions that have been updated:
Runtime version has been increased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went only through pallets/liquidity-mining/src/lib.rs
. This is not full review. More doc comments probably needs update and it require second look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i havent looked at tests.
however, i have one additional concern and that's dusting of account where shares are transferred.
it should be excluded from dusting imo. eg. xyk pool accounts are excluded.
@@ -0,0 +1,65 @@ | |||
[package] | |||
name = "pallet-xyk-liquidity-mining" | |||
version = "1.3.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the first time the pallet is added ? why 1.3.0 then ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previous version was in the runtime but it was never really used so maybe we can use 1.0.0
who.clone(), | ||
deposit_id, | ||
yield_farm_id, | ||
!fail_on_double_claim, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont quite get the double claim on/off.
accoprding to what i see - you can call claim_rewards and it fails on double claim.
however, instead of calling claim_rewards, you can call withdraw shares, and it calls claim_rewards which does not fail on double claim.
and this leads me to think to either "why it is needed at all" or "there can be a way to double claim potentially?!".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users can claim rewards without withdrawing LP shares. This should fail if the user called claim 2nd times in the same period.
Withdrawing shares is claiming automatically and we don't want to penalise users by waiting for the next period to withdraw if the user has already claimed. So in this case 2nd claim should not fail.
|
||
if is_destroyed { | ||
Self::unlock_lp_tokens(asset_pair, who.clone(), withdrawn_amount)?; | ||
T::NFTHandler::burn(&T::NftCollectionId::get(), &deposit_id, None)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you considered using the maybe_check_owner ? just to make sure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First review (haven't looked at benchmarks and tests in-depth).
Sorry for the delay.
[dependencies] | ||
codec = { package = "parity-scale-codec", version = "3.1.5", features = ["derive", "max-encoded-len"], default-features = false } | ||
scale-info = { version = "2.1.2", default-features = false, features = ["derive"] } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scale-info = { version = "2.1.2", default-features = false, features = ["derive"] } | ||
|
||
|
||
sp-arithmetic = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.28", default-features = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be part of the substrate deps below, no?
hydradx-traits-lm = { package="hydradx-traits", git = "https://github.com/galacticcouncil/warehouse", rev="89bde6d2b8bad2263c554eb3d83b76d877971136", default-features = false } | ||
|
||
pallet-nft = { git = "https://github.com/galacticcouncil/warehouse", branch="polkadot-v0.9.28", default-features = false } | ||
hydradx-traits = { git = "https://github.com/galacticcouncil/warehouse", branch="polkadot-v0.9.28", default-features = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you depend on 2 different versions of the traits crate, is this on purpose? Looks very odd and would warrant an explaining comment if it makes sense, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it's intentional. Other pallets (xyk, nft) are using traits from master and LM is using trait from LM branch.
In my opinion warehouse LM should be merged first before we updates all the pallets to use the same trait. Otherwise we would have to do all the traits changes in the warehouse LM branch
orml-tokens = { git = "https://github.com/open-web3-stack/open-runtime-module-library", branch="polkadot-v0.9.28", default-features = false } | ||
pallet-balances = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.28", default-features = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can remove the default-featues = false
frame-system-benchmarking = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.28", default-features = false } | ||
frame-benchmarking = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.28", default-features = false } | ||
|
||
orml-tokens = { git = "https://github.com/open-web3-stack/open-runtime-module-library", branch = "polkadot-v0.9.28", default-features = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be with the other orml dep?
runtime/testing-basilisk/src/lib.rs
Outdated
@@ -919,6 +946,9 @@ construct_runtime!( | |||
Marketplace: pallet_marketplace = 109, | |||
TransactionPause: pallet_transaction_pause = 110, | |||
|
|||
LiquidityMining: pallet_xyk_liquidity_mining = 111, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: won't we have a stable swap liquidity mining? then this should be XYKLiqduityMining
or similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I was thinking about renaming this but FE is already using it on testnet and my understanding was that we want to deploy it directly to rococo without testnet first.
runtime/testing-basilisk/src/lib.rs
Outdated
@@ -919,6 +946,9 @@ construct_runtime!( | |||
Marketplace: pallet_marketplace = 109, | |||
TransactionPause: pallet_transaction_pause = 110, | |||
|
|||
LiquidityMining: pallet_xyk_liquidity_mining = 111, | |||
WarehouseLM: warehouse_liquidity_mining::<Instance1> = 112, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WarehouseLM: warehouse_liquidity_mining::<Instance1> = 112, | |
XYKWarehouseLM: warehouse_liquidity_mining::<XYKLiquidityMiningInstance> = 112, |
would this be possible? (and would we want to specify the name of the instance on the left?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This didn't work for me(use instance name instead of Instance1
) but maybe I did something wrong I'll try it again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine 🤷
runtime/testing-basilisk/src/lib.rs
Outdated
type BlockNumberProvider = cumulus_pallet_parachain_system::RelaychainBlockNumberProvider<Runtime>; | ||
type NftCollectionId = LiquidityMiningNftCollectionId; | ||
type AMM = XYK; | ||
type WeightInfo = (); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, this is testing so not as important?
runtime/common/src/lib.rs
Outdated
pub const MinDeposit: Balance = 1;//TODO: tmp value | ||
pub const MaxEntriesPerDeposit: u8 = 5; //NOTE: Rebenchmark when this change //TODO: tmp value | ||
pub const MaxYieldFarmsPerGlobalFarm: u8 = 5; //TODO: tmp value | ||
pub const MinPlannedYieldingPeriods: BlockNumber = 100; //TODO: tmp value | ||
pub const MinTotalFarmRewards: Balance = NATIVE_EXISTENTIAL_DEPOSIT * 1_000; //TODO: tmp value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for @jak-pan. I don't know what values we want
runtime/basilisk/src/lib.rs
Outdated
@@ -858,7 +858,7 @@ impl pallet_relaychain_info::Config for Runtime { | |||
impl pallet_xyk_liquidity_mining::Config for Runtime { | |||
type Event = Event; | |||
type MultiCurrency = Currencies; | |||
type CreateOrigin = EnsureRoot<AccountId>; | |||
type CreateOrigin = SuperMajorityTechCommitteeOrRoot; //TODO: check this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do just root or full tech comm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i havent checked closely tests nor benchmarks to see if they cover the worst case scenarios. If you have any doubts, we can dive in as this requires bit of effort to understand the LM itself.
hydradx-traits-lm = { package="hydradx-traits", git = "https://github.com/galacticcouncil/warehouse", rev="89bde6d2b8bad2263c554eb3d83b76d877971136", default-features = false } | ||
|
||
hydradx-traits = { git = "https://github.com/galacticcouncil/warehouse", branch="polkadot-v0.9.28", default-features = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont like this.
why it is not possible to have one ? probably needs to be merged to master ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't lie it either :) but yes, warehosue LM pallets needs to be merged and other pallets(xyk, nft,...) needs to be updated to use the same trait version.
@@ -0,0 +1,72 @@ | |||
[package] | |||
name = "pallet-xyk-liquidity-mining-benchmarking" | |||
version = "1.1.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.0.0?
orml-traits = { git = "https://github.com/open-web3-stack/open-runtime-module-library", branch = "polkadot-v0.9.28", default-features = false } | ||
|
||
# HydraDX dependencies | ||
warehouse-liquidity-mining = { package="pallet-liquidity-mining", git = "https://github.com/galacticcouncil/warehouse", rev="89bde6d2b8bad2263c554eb3d83b76d877971136", default-features = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess it is possible now to use pallet-liquidity-mining as it has been renamed
# HydraDX dependencies | ||
pallet-nft = { git = "https://github.com/galacticcouncil/warehouse", branch = "polkadot-v0.9.28", default-features = false} | ||
pallet-asset-registry = { git = "https://github.com/galacticcouncil/warehouse", branch = "polkadot-v0.9.28", default-features = false } | ||
warehouse-liquidity-mining = { package="pallet-liquidity-mining", git = "https://github.com/galacticcouncil/warehouse", rev="89bde6d2b8bad2263c554eb3d83b76d877971136", default-features = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here. no need of name change
)?; | ||
|
||
Ok(()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
)?; | |
Ok(()) | |
) |
Ok(()) | ||
} | ||
|
||
/// Withdraw LP shares from liq. mining. with reward claiming if possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Withdraw LP shares from liq. mining. with reward claiming if possible. | |
/// Withdraw LP shares from liq. mining with reward claiming if possible. |
|
||
/// Withdraw LP shares from liq. mining. with reward claiming if possible. | ||
/// | ||
/// Cases for transfer LP shares and claimed rewards: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this sentence doesn't make much sense to me.
List of possible cases of transfers of shares and rewards ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for extensive test coverage
2 Points on the benchmarks:
- I think it would be good to add
verify
clauses for the benchmarks. - I suspect the
withdraw_shares
benchmark is not measuring the worst case.
Seems fine to merge once withdraw_shares
benchmark is checked and/or extended.
lm_create_yield_farm::<T>(caller.clone(), GLOBAL_FARM_ID, ASSET_PAIR, FixedU128::one())?; | ||
|
||
lm_deposit_shares::<T>(liq_provider, ASSET_PAIR, 10_000)?; | ||
set_block_number::<T>(200_000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is to make sure that the farm has entered the next period?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this is to change the period and to make sure the farm's RPZ update happens
set_block_number::<T>(200_000); | ||
}: { | ||
LiquidityMining::<T>::destroy_global_farm(RawOrigin::Signed(caller.clone()).into(), GLOBAL_FARM_ID)? | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll want to verify that the farm was removed from storage, no?
lm_create_yield_farm::<T>(caller.clone(), GLOBAL_FARM_ID, ASSET_PAIR, FixedU128::one())?; | ||
|
||
lm_deposit_shares::<T>(liq_provider, ASSET_PAIR, 10_000)?; | ||
set_block_number::<T>(100_000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are to ensure that the global farm needs updating I assume?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
|
||
LiquidityMining::<T>::deposit_shares(RawOrigin::Signed(liq_provider.clone()).into(), GLOBAL_FARM_ID, 3, bsx_dot, shares_amount)?; | ||
|
||
set_block_number::<T>(400_000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is about periods, how about expressing these as such?
E.g. set_block_number::<T>(period_to_blocks(4))
or similar
/// | ||
/// Emits `SharesRedeposited` event when successful. | ||
#[pallet::weight(<T as Config>::WeightInfo::redeposit_lp_shares())] | ||
pub fn redeposit_lp_shares( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this naming is inconsistent with deposit_shares
above
LiquidityMining::<T>::deposit_shares(RawOrigin::Signed(liq_provider.clone()).into(), GLOBAL_FARM_ID, YIELD_FARM_ID, ASSET_PAIR, shares_amount)?; | ||
LiquidityMining::<T>::redeposit_lp_shares(RawOrigin::Signed(liq_provider.clone()).into(), GLOBAL_FARM_ID_2, YIELD_FARM_ID_2, ASSET_PAIR, DEPOSIT_ID)?; | ||
LiquidityMining::<T>::redeposit_lp_shares(RawOrigin::Signed(liq_provider.clone()).into(), 5, 6, ASSET_PAIR, DEPOSIT_ID)?; | ||
LiquidityMining::<T>::redeposit_lp_shares(RawOrigin::Signed(liq_provider.clone()).into(), 7, 8, ASSET_PAIR, DEPOSIT_ID)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surprised at all these (re)deposits. Want to add an explanatory comment (and why this setup is not necessary for the regular deposit benchmark)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll double check but I think there was a difference when there was only 1 entry in the deposit. I'll add comment.
In the regular deposit it's not necessary because it creates new deposit
}: { | ||
LiquidityMining::<T>::withdraw_shares(RawOrigin::Signed(liq_provider.clone()).into(), DEPOSIT_ID, YIELD_FARM_ID, ASSET_PAIR)? | ||
} verify { | ||
assert!(MultiCurrencyOf::<T>::free_balance(BSX, &liq_provider).gt(&liq_provider_bsx_balance)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want to trigger (and then check) the case where it is destroyed?
withdraw_shares
is quite complex so I'm somewhat skeptical that you've covered the worst case with this short benchmark.
You might even want 2 or 3 for the different scenarios and then compare them in terms of their weight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I think I compared case when deposit is destroyed and this one was worse. I'll double check and add comment.
} | ||
|
||
#[test] | ||
fn deposit_shares_should_fail_when_origin_is_not_signed() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fn deposit_shares_should_fail_when_origin_is_not_signed() { | |
fn withdraw_shares_should_fail_when_origin_is_not_signed() { |
pub enum Error<T> { | ||
/// Nft pallet didn't return an owner. | ||
CantFindDepositOwner, | ||
|
||
/// Account balance of amm pool shares is not sufficient. | ||
InsufficientAmmSharesBalance, | ||
|
||
/// AMM pool does not exist | ||
AmmPoolDoesNotExist, | ||
|
||
/// Account is not deposit owner. | ||
NotDepositOwner, | ||
|
||
/// AMM did not return assets for given `amm_pool_id` | ||
CantGetAmmAssets, | ||
|
||
/// Yield farm can not be found | ||
YieldFarmNotFound, | ||
|
||
///Deposit data not found | ||
DepositDataNotFound, | ||
|
||
/// Calculated reward to claim is 0. | ||
ZeroClaimedRewards, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing tests for CantFindDepositOwner
, CantGetAmmAssets
, YieldFarmNotFound
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolve the TODO and lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Added implementation for XYK liquidity mining where we use the extracted liquidity mining pallet from warehouse repo.
TODO:
Related Issue
Motivation and Context
How Has This Been Tested?
Checklist: