Skip to content

Conversation

@martinfridrich
Copy link
Contributor

Description

Implementation of liquidity mining pallet

Related Issue

#242

@martinfridrich martinfridrich changed the title Feat: liquidity mining feat: liquidity mining Nov 18, 2021
Copy link
Member

@enthusiastmartin enthusiastmartin left a comment

Choose a reason for hiding this comment

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

Just initial quick review.

I have a feeling that there is a mixture of concrete types and generic types which however expect traits such From and Into to be implement by that type ( eg. balance and currency).

If it is defined like that - there are not many options for types anyways and I would consider to set balance and currency types to u128, or u32 respectively.

This approach will make certain things easier to do - but it limits the "re-use" of the pallet ( but i dont think that's an issue and we should not be worries about that).

eg. XYK is currently done in similar way where balance and currency id is se to u128 and u32.

Copy link
Member

@enthusiastmartin enthusiastmartin left a comment

Choose a reason for hiding this comment

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

Few more comments.

);

// update global pool accumulated RPZ
let now_period = Self::get_now_period(global_pool.blocks_per_period)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

current?

@jak-pan
Copy link
Contributor

jak-pan commented Mar 22, 2022

I've reviewed the core functionality so far only naming nitpicks that can also be addressed later (I'd rather do now since it's probably not hard). I'll continue with logic / tests and benchmarks tmrw

/// Liquidity pool yield farm details.
#[pallet::storage]
#[pallet::getter(fn liquidity_pool)]
type LiquidityPoolData<T: Config> = StorageDoubleMap<
Copy link
Contributor

Choose a reason for hiding this comment

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

I would personally set some constants for max amount of pools (any map really) and then benchmark against that max number since maps/double maps might take longer to read/write when there is a lot of entries. Something like 255 should be sufficient.

Copy link
Contributor

@jak-pan jak-pan left a comment

Choose a reason for hiding this comment

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

I have some nitpicks in naming which don't need to be addressed now but we should have an issue for this open.

One benchmarking concern with maps.
2 tests I'd like to see.

Other than that, very nice job 👍🏻

@green-jay
Copy link
Contributor

It should be checked if nft class id is less than ReserveClassIdUpTo from the NFT pallet

@mrq1911 mrq1911 requested a review from jak-pan April 7, 2022 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants