feat: NTRN-308 - Implement burn-based vesting for treasury #15
Conversation
b2eaad7 to
4493411
Compare
There was a problem hiding this comment.
Looks good to me but the PR is blocked by neutron-org/neutron#117, so I'm waiting for it to be reviewed, merged and implementation of querying of TotalBurnedNeutronsAmount in this PR
|
lgtm, waiting for this |
contracts/treasury/src/vesting.rs
Outdated
|
|
||
| pub fn get_burned_coins(deps: Deps<InterchainQueries>, denom: &String) -> StdResult<Uint128> { | ||
| let burned_coins = query_total_burned_neutrons(deps).map_err(|_err| { | ||
| println!("{:?}", _err); |
contracts/treasury/src/vesting.rs
Outdated
| return Ok(Uint128::zero()); | ||
| } | ||
|
|
||
| let current_balance = Decimal::from_atomics(current_balance, 0).map_err(|err| { |
There was a problem hiding this comment.
why zero decimal places?
There was a problem hiding this comment.
to simplify calculations
contracts/treasury/src/vesting.rs
Outdated
| } | ||
|
|
||
| let current_balance = Decimal::from_atomics(current_balance, 0).map_err(|err| { | ||
| StdError::generic_err(format!("Unable to convert Uint128 to Decimal. {}", err)) |
contracts/treasury/src/contract.rs
Outdated
| }); | ||
| resp = resp.add_message(msg) | ||
| } | ||
| if burned_tokens.is_ok() && !current_balance.is_zero() { |
There was a problem hiding this comment.
maybe if burned_tokens returns StdError we should return error from contract, since it's unexpected really?
There was a problem hiding this comment.
while it's meaningful to return no funds to distribute on current_balance.is_zero(), in case of an error the error should be returned indeed, because otherwise we wouldn't be able to figure out why the contract says no funds to distribute whereas there are funds
contracts/treasury/src/vesting.rs
Outdated
| Ok(()) | ||
| } | ||
|
|
||
| pub fn create_distribution_response( |
There was a problem hiding this comment.
Not sure about this, but: Is this responsibility of vesting - to operate on level of Response? how about returning here just an array of messages?
contracts/treasury/src/contract.rs
Outdated
| }); | ||
| } | ||
| let last_burned_coins = LAST_BURNED_COINS_AMOUNT.load(deps.storage)?; | ||
| let burned_tokens = get_burned_coins(deps.as_ref(), &denom); |
There was a problem hiding this comment.
please rename burned_tokens -> burned_coins for identity
contracts/treasury/src/contract.rs
Outdated
| }); | ||
| resp = resp.add_message(msg) | ||
| } | ||
| if burned_tokens.is_ok() && !current_balance.is_zero() { |
There was a problem hiding this comment.
while it's meaningful to return no funds to distribute on current_balance.is_zero(), in case of an error the error should be returned indeed, because otherwise we wouldn't be able to figure out why the contract says no funds to distribute whereas there are funds
a3fbc4d to
236b306
Compare
236b306 to
306c06e
Compare
| @@ -206,14 +218,17 @@ pub fn execute_update_config( | |||
| if let Some(security_dao_address) = security_dao_address { | |||
| config.security_dao_address = deps.api.addr_validate(security_dao_address.as_str())?; | |||
| } | |||
| if let Some(distribution_rate) = distribution_rate { | |||
| if let Some(distribution_rate) = distribution_params.distribution_rate { | |||
There was a problem hiding this comment.
why we do not validate distribution_rate in init message?
| /// Function calculates how many coins should be released for the current period | ||
| /// based on the current balance and the number of coins burned for the period | ||
| /// Implemented vesting function is linear and is defined as: y=x/vesting_denominator | ||
| /// In order to optimize the function, we use the following formula: y=x - (vesting_denominator-1 / vesting_denominator)^<coins for period> * x |
There was a problem hiding this comment.
i think its a cleaner ((vesting_denominator-1) / vesting_denominator). One more brackets level
| pub fn safe_burned_coins_for_period( | ||
| burned_coins: Uint128, | ||
| last_burned_coins: Uint128, | ||
| ) -> StdResult<u32> { |
There was a problem hiding this comment.
What is the reason to use u32
There was a problem hiding this comment.
Decimal.checked_pow works with u32 only
Treasury tokens are vested based on on-chain activity: burnt_tokens * a_multiplier. The multiplier is an linear function of the supply: while initially, one burnt tokens equals multiple NTRN tokens made liquid, the flow of new tokens into the Treasury progressively slows down until the tokens supply is exhausted and the tokenomy becomes deflationary.
Related PRs:
Depends on: