Skip to content
This repository has been archived by the owner on May 30, 2023. It is now read-only.

Feat: liquidity mining #50

Closed
wants to merge 7 commits into from
Closed

Feat: liquidity mining #50

wants to merge 7 commits into from

Conversation

martinfridrich
Copy link
Contributor

@codecov
Copy link

codecov bot commented May 24, 2022

Codecov Report

Merging #50 (bde8c73) into main (718d477) will increase coverage by 2.10%.
The diff coverage is 83.05%.

@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
+ Coverage   75.89%   77.99%   +2.10%     
==========================================
  Files          17       19       +2     
  Lines         892     1409     +517     
==========================================
+ Hits          677     1099     +422     
- Misses        215      310      +95     
Impacted Files Coverage Δ
traits/src/lib.rs 0.00% <ø> (ø)
liquidity-mining/src/lib.rs 82.49% <82.49%> (ø)
liquidity-mining/src/types.rs 100.00% <100.00%> (ø)
nft/src/types.rs 14.28% <0.00%> (-85.72%) ⬇️
nft/src/lib.rs 92.59% <0.00%> (+1.83%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 718d477...bde8c73. Read the comment docs.

Copy link
Contributor

@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.

I havent looked at it all yet.

but here my initial findings. i'll continue again later.

version = "1.0.0"
description = "Liquidity mining"
authors = ["GalacticCouncil"]
edition = "2018"
Copy link
Contributor

Choose a reason for hiding this comment

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

2021

edition = "2018"
homepage = "https://github.com/galacticcouncil/Basilisk-node"
license = "Apache 2.0"
repository = "https://github.com/galacticcouncil/Basilisk-node"
Copy link
Contributor

Choose a reason for hiding this comment

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

warehouse repo

Comment on lines +118 to +125
/// Minimum total rewards to distribute from global pool during liquidity mining.
type MinTotalFarmRewards: Get<Balance>;

/// Minimum number of periods to run liquidity mining program.
type MinPlannedYieldingPeriods: Get<Self::BlockNumber>;

/// Mininum user's deposit to enter liquidity mining pool.
type MinDeposit: Get<Balance>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe all these can be marked as constants

/// `LiquidityPoolYieldFarm` metadata holding: `(existing deposits count, global pool id)`
#[pallet::storage]
#[pallet::getter(fn liq_pool_meta)]
pub type LiquidityPoolMetadata<T: Config> = StorageMap<_, Twox64Concat, PoolId, (u64, GlobalPoolId), OptionQuery>;
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of u64 - perhaps it would be good to create a type alias, so it is more readable.

/// - `owner`: liq. mining farm owner.
/// - `yield_per_period`: percentage return on `reward_currency` of all pools
#[allow(clippy::too_many_arguments)]
#[transactional]
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this will part of another transactional operation.

Suggested change
#[transactional]
#[require_transactional]

Comment on lines +390 to +397
let undistributed_reward = T::MultiCurrency::total_balance(global_pool.reward_currency, &global_pool_account);

T::MultiCurrency::transfer(
global_pool.reward_currency,
&global_pool_account,
&who,
undistributed_reward,
)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that the transfer would fail if there is a locked/reserved amount in global_pool_account.

because the total_balance returns combined balance but the transfer checks only the "free" part.

#[cfg_attr(test, derive(PartialEq))]
pub enum Error<T> {
/// Math computation overflow.
Overflow,
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of custom overflow, you can use sp_runtime::ArithmeticError;

that's what we started to do in other crates where math error is needed/

Comment on lines +242 to +248
Twox64Concat,
GlobalPoolId,
Twox64Concat,
T::AmmPoolId,
LiquidityPoolYieldFarm<T>,
OptionQuery,
>;
Copy link
Contributor

Choose a reason for hiding this comment

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

it is not clear who can create T::AmmPoolIds

so i'd suggest to use "Blake2_128Concat" hasher. Also it might preferable choice in other other storages too.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants