initialization of poel & i_asset: descriptions for types #137
Conversation
| use supra_framework::table; | ||
| use supra_framework::string::String; |
There was a problem hiding this comment.
No such types are available. See aptos-stdlib or move-stdlib for lib/utility modules available.
| /// Errors: | ||
|
|
||
| /// Thrown when the calling address is not the poel module address | ||
| const ERR_NOT_POEL_ADDRESS: u64 = 0; |
There was a problem hiding this comment.
Error code must never be zero. Also the convention is just to use E as a prefix, so ENOT_POEL_ADDRESS.
| use supra_framework::table; | ||
| use supra_framework::smart_table::SmartTable; |
There was a problem hiding this comment.
No such types available, check aptos-stdlib, move-stdlib
| const ERR_NOT_OWNER: u64 = 0; | ||
|
|
||
| /// thrown when the wrong asset weight gets calculated | ||
| const ERR_WRONG_WEIGHT: u64 = 1; | ||
|
|
||
| /// thrown when the length of the supplied collaterixation weight argument vector does not match table entries | ||
| const ERR_WRONG_CWV_LENGTH: u64 = 2; | ||
|
|
||
| /// thrown when the length of the desirability score argument vector does not match table entries | ||
| const ERR_WRONG_DESIRABILITY_SCORE_LEN: u64 = 3; | ||
|
|
||
| /// thrown when the asset id supiled is not present in the TotalLiquidityTable | ||
| const ERR_ASSET_NOT_PRESENT: u64 = 4; | ||
|
|
||
| /// thrown If coefficient_X < coefficient_b | ||
| const ERR_COEFA_LT_COEFB: u64 = 5; | ||
|
|
||
| // Structs: |
There was a problem hiding this comment.
DO NOT use 0 for error code. Also the convention is just to use E instead of ERR as prefix for error code.
| ///asset_price: Current market price of the asset. | ||
| asset_price: u64, |
There was a problem hiding this comment.
Current market price against what? If this information is available via oracle, then this should not be stored.
There was a problem hiding this comment.
True, we probably don't need to store the asset price in the struct.
| /// This struct contains contains the field attributes for each asset in the liquidity table | ||
| struct LiquidityTableItems { | ||
| ///Asset_name | ||
| asset_name: vector<u8>, |
There was a problem hiding this comment.
Why do we want to store the name on-chain? ID is the only thing required to distinguish.
There was a problem hiding this comment.
This was present in the initial document, but come to think of it, the name is not used on chain. Will remove the name in the next push.
|
|
||
| /// This struct is applied for the dynamic adjustment of the system's operational parameters. | ||
| /// It allows for flexible management of financial metrics, adapting to market conditions or strategic shifts in policy. | ||
| struct MutableParameters { |
There was a problem hiding this comment.
ability annotations are missing for this struct, are these parameters to be under governance control?
There was a problem hiding this comment.
Yes, This was from the initial type annotation. Currently the structs are not used in the function as they we are only defining the signatures at this point. The types should be included in the next iteration where I start implementing individual functions.
| //The amount of Supra delegated to the validator at this pool. | ||
| delagated_amount: u64, | ||
| //Amount of tokens that are pending deactivation but are currently inactive. | ||
| pending_inactive_balance: u64 |
There was a problem hiding this comment.
This need not be stored if this corresponds to pending_inactive of the stake pool, just query the stake pool.
There was a problem hiding this comment.
let's remove this pending_inactive_balance field, seems we are not using it anywhere
| //Address of the staking pool. | ||
| pool_address: address, | ||
| //The amount of Supra delegated to the validator at this pool. | ||
| delagated_amount: u64, |
There was a problem hiding this comment.
Not sure what this means, does it mean the total of active, and pending_inactive?
| /// inactive balances within the system. | ||
| struct DelegatedAmount { | ||
| // Maps StakingPoolID to tuple StakingPoolMap | ||
| staking_pool_mapping: table::Table<u64, StakingPoolMap>, |
There was a problem hiding this comment.
And who tracks the IDs here? These ID corresponds to what?
There was a problem hiding this comment.
Okay, will do. We will add a view to retrieve all the Ids present with their asset names.
| // Maps StakingPoolID to tuple StakingPoolMap | ||
| staking_pool_mapping: table::Table<u64, StakingPoolMap>, | ||
| // Cumulative amount of Supra delegated across all staking pools | ||
| total_delegated_amount: u64, |
There was a problem hiding this comment.
If this can be computed, then should not be stored.
| // Cumulative amount of Supra delegated across all staking pools | ||
| total_delegated_amount: u64, | ||
| // Total amount of Supra pending deactivation | ||
| pending_inactive_balance: u64, |
There was a problem hiding this comment.
Again, if this can be computed, should not be stored.
There was a problem hiding this comment.
@phydy-dev , let's remove this field
| /// Description: This struct serves as a central repository for tracking delegated assets in various staking pools, | ||
| /// providing essential data for managing staking operations and calculating borrowing limits based on active and | ||
| /// inactive balances within the system. | ||
| struct DelegatedAmount { |
There was a problem hiding this comment.
abilities to be added when we start implementing function logic
| // Total amount of Supra pending deactivation | ||
| pending_inactive_balance: u64, | ||
| // Total amount of Supra that could be borrowed from the PoEL contract | ||
| total_borrowable_amount: u64, |
There was a problem hiding this comment.
out of? do we have a parameter that defines the max?
There was a problem hiding this comment.
Total amount will be based on the supra reserve in the contract which acts as the cap.
There was a problem hiding this comment.
This field stores the maximum amount of Supra tokens that can be borrowed from the PoEL contract.
| } | ||
|
|
||
|
|
||
| struct AdminManagement { |
There was a problem hiding this comment.
adding in the next iteration with function logic.
| /// Adds a staking pool to the existing pools for delegation os $supra | ||
| public fun add_staking_pool( | ||
| _delegated_amount: &mut DelegatedAmount, | ||
| _pool_id: u64, |
There was a problem hiding this comment.
Not sure what is the role of pool_id, why is pool_address not sufficient?
| _delegated_amount_value: u64, | ||
| _pending_inactive_balance: u64 |
There was a problem hiding this comment.
Both these can be computed it seems
There was a problem hiding this comment.
The delegated_amount_value is stored and updated as soon as there is a new delegation from PoEL. As far as I understand, we cannot read it from the delegation pool contracts so we would need to store it.
| } | ||
|
|
||
|
|
||
| public fun submit_borrow_request( |
There was a problem hiding this comment.
Please write a function header comment as to what it is supposed to do and the role of the parameters
| } | ||
|
|
||
|
|
||
| public fun submit_withdraw_request( |
There was a problem hiding this comment.
function header comments missing, without which I have no idea what this is supposed to do
| } | ||
|
|
||
| /// should return a &BorrowRequestTableItems | ||
| public fun get_requests( |
There was a problem hiding this comment.
Why is return type missing? what is it supposed to do?
There was a problem hiding this comment.
The return type is BorrowRequestTableItems which I will add after implementing the function's logic.
| public fun update_desired_weight( | ||
| _collaterisation_rate_vector: vector<u64>, | ||
| _account: &signer |
There was a problem hiding this comment.
signer should always be the first parameter as convention, what does each element of the vector correspond to? Please write function header comments explaining role of each parameter
| public fun batch_update_desirability_score( | ||
| _desirability_score_vector: vector<u64>, | ||
| _account: &signer |
There was a problem hiding this comment.
Improve function header comments, write role of each parameter, signer should come first
| fun sign(_value: u64): u64 { | ||
| 0 | ||
| } |
| public fun set_parameters( | ||
| _account: &signer, | ||
| _coefficient_b: u64, | ||
| _coefficient_X: u64, | ||
| _coefficient_rho: u64, | ||
| _min_collateralisation: u64 | ||
| ) { |
There was a problem hiding this comment.
If these are governance controlled parameter, please specify, ideally should be in config/poel_config.move similar to staking_config.move
| /// Unlocks token from the all delegation pools involved in the system | ||
| public fun unlock_tokens(_supra_amount: u64) {} |
There was a problem hiding this comment.
Anyone can call this. and the unlock happens for all pools.
| /// Delegates token to the all delegation pools involved in the system | ||
| public fun delegate_tokens(_supra_amount: u64) {} |
There was a problem hiding this comment.
This one will be a private function only callable within same module. I should rectify that.
| ); | ||
| let table_obj = borrow_global_mut<LiquidityTableItems>(asset_address(symbol)); | ||
|
|
||
| total_rentable_amount = total_rentable_amount + (table_obj.asset_supply / collateralisation_rate) * asset_price; |
There was a problem hiding this comment.
Get a immutable reference to the BorrowWithdrawRequest struct
total_rentable_amount = total_rentable_amount + ((table_obj.asset_supply + total_borrow_requests - total_withdraw_requests) / collateralisation_rate) * asset_price;
…t to be implemented upon futher discussion"
Updating the poel branch with the changes made in the dev base branch
| delegate_tokens(change_of_borrowed_amount); | ||
| } else if (change_of_borrowed_amount < 0) { | ||
| unlock_tokens(change_of_borrowed_amount); //should be negative? | ||
| } |
There was a problem hiding this comment.
here we need to also add:
a. get a mutable reference to the BorrowWithdrawRequest struct
b. For all asset sets total_borrow_requests and total_withdraw_requests to 0
we need to do this as at this point all the requests are processed
| ///allocated_Rewards: Tracks the total rewards that are allocable to the user. | ||
| allocated_rewards: u64,//not updated at any point?? | ||
| ///unlock_OLC_Index: Registers the index of the lockup cycle when the user last submitted an unlock request. | ||
| unlock_olc_index: u64//not updated at any point?? |
| multiperiod_extra_rewards: u64, | ||
| multiperiod_reward_balance: u64, | ||
| per_period_distributable_rewards: u64, | ||
| singleperiod_reward_balance: u64, |
There was a problem hiding this comment.
yes we need to have asset as parameter
|
|
||
| // Unlock the calculated total_reward_earned for each pool | ||
| unlock(&get_obj_signer(), pool_address, pool_reward); | ||
|
|
There was a problem hiding this comment.
we might withdraw tokens not only during reward distribution but also during collateral price based update of the rented amount, in that case even though we update the withdraw_olc_index, we do not withdraw the rewards
| //let coefficient_rho = mutable_params_ref.coefficient_rho; | ||
| let min_collateralisation = mutable_params_ref.min_collateralisation; | ||
| let max_collateralisation_first = mutable_params_ref.max_collateralisation_first; | ||
|
|
There was a problem hiding this comment.
in the comment below we have a loop where we iterate and add the asset_specific_rented_amount_with_desirability_scores to the total_rented_amount_with_desirability_scores
|
|
||
| let total_unlocked_amount = 0; | ||
|
|
||
|
|
There was a problem hiding this comment.
Instead of unlocking funds from all pools simultaneously, we will unlock funds sequentially—starting with the largest delegation pool and then moving to smaller pools. The process is as follows:
1.Get a mutable reference to the DelegatedAmount struct.
2. Assert that supra_amount is less than or equal to total_delegated_amount (from the DelegatedAmount struct).
3. Set unlockable_amount equal to supra_amount.
4. Compare the delegation amounts and determine the pool with the largest balance, an d
Retrieve the address of this largest pool.
5. Unlocking Process:
a. If the balance of the largest pool is greater than or equal to supra_amount:
1. Execute unlock(&get_obj_signer(), pool_address, supra_amount).
b. If the balance of the largest pool is less than supra_amount:
1.Unlock the entire pool balance by calling unlock(&get_obj_signer(), pool_address, pool_balance).
2. Update unlockable_amount by subtracting the pool's balance: unlockable_amount = supra_amount - pool_balance.
3. Remove the pool address from the vector.
4. Identify the next largest pool (i.e., the pool with the highest remaining delegation amount).
5. Update pool_balance = balance of this new pool.
Repeat this check (i.e., compare pool_balance with the remaining unlockable_amount) and execute the unlocking process accordingly.
Ensure that before each iteration, you assert that the pool balance vector is not empty.
- Once the unlocking process is complete, update the total_delegated_amount in the DelegatedAmount struct:
total_delegated_amount -= supra_amount.
probably we need to add a helper function here to calculate the largest number in a vector
public fun find_max_index(nums: vector): u64 {
let n = vector::length(&nums);
assert!(n > 0, 1);
let mut max_index = 0;
let mut max_val = *vector::borrow(&nums, 0);
let mut i = 1;
while (i < n) {
let current = *vector::borrow(&nums, i);
if (current > max_val) {
max_val = current;
max_index = i;
}
i = i + 1;
};
max_index
}
}
and use the index to fetch the address of the pool with the biggest balance
| max_collateralisation_second: u64, | ||
| reward_reduction_rate: u64, | ||
| reward_distribution_address: address, | ||
| } |
There was a problem hiding this comment.
We need to add an additional parameter that specifies the maximum allowable delegation to a single pool in the system: pool_delegation_size_cap: u64
| let adjusted_supra_amount = supra_amount - remainder; | ||
|
|
||
| let per_pool_unlock_amount = adjusted_supra_amount / pool_num; | ||
|
|
|
|
||
| let remainder = supra_amount % pool_num; | ||
| let adjusted_supra_amount = supra_amount - remainder; | ||
|
|
| /// Total amount of Supra pending deactivation | ||
| pending_inactive_balance: u64, | ||
| /// Total amount of Supra that could be borrowed from the PoEL contract | ||
| total_borrowable_amount: coin::Coin<SupraCoin>, |
There was a problem hiding this comment.
for consistency would be better to rename. this to total_rentable_amount
|
|
||
| let delegated_amount_ref = borrow_global_mut<DelegatedAmount>(get_poel_storage_address()); | ||
|
|
||
| smart_table::for_each_mut<address, StakingPoolMap>( |
There was a problem hiding this comment.
Instead of delegating funds to all pools simultaneously, we will delegate funds sequentially—starting with the smallest delegation pool and then moving to smaller pools if the pool's delegated balance exceeds the pool_max_delegation_cap. The process is as follows:
- Get a mutable reference to the DelegatedAmount struct.
- Get a mutable reference to MutableParameters struct
- assert that total_delegated_amount + supra_amount <= total_borrowable_amount
- Set the delegated_amount= supra_amount.
- Compare the delegation amounts and determine the pool with the smallest balance, and
Retrieve the address of this largest pool. - Unlocking Process:
a. If the balance of the smallest pool + supra_amount <= roundup(total_borrowable_amount * pool_max_delegation_cap/100): - add_stake(&get_obj_signer(), pool_address, supra_amount).
b. If the balance of the smallest pool + supra_amount > roundup(total_borrowable_amount * pool_max_delegation_cap/100):
1.delegate to the pool up to the amount that is supported by calling add_stake(&get_obj_signer(), pool_address, roundup(total_borrowable_amount * pool_max_delegation_cap/100) - smallest_pool_balance). - Update unlockable_amount by subtracting the pool's balance: delegable_amount = supra_amount - roundup(total_borrowable_amount * pool_max_delegation_cap/100) - smallest_pool_balance.
- Remove the pool address from the vector.
- Identify the next smallest pool.
- Update pool_balance = balance of this new pool.
probably we need a helper function to get the ID of the smallest number in a vector of numbers
|
|
||
| // Send withdraw_requested_assets amount of Supra from the PoEL contract to the admin | ||
| coin::transfer<SupraCoin>(&get_obj_signer(), admin_management_ref.admin_address, withdraw_requested_assets); | ||
|
|
There was a problem hiding this comment.
there is need for change here:
coin::transfer(&get_obj_signer(), admin_management_ref.withdrawal_address, withdraw_requested_assets)
| }); | ||
|
|
||
| delegated_amount_ref.total_delegated_amount = delegated_amount_ref.total_delegated_amount + adjusted_supra_amount; | ||
| } |
There was a problem hiding this comment.
here we would need to also update total_borrowable_amount:
total_borrowable_amount -= supra_amount
| } | ||
|
|
||
| #[resource_group_member(group = supra_framework::object::ObjectGroup)] | ||
| /// Liquidity provider struct that holds the overal info on the user's iAssets |
There was a problem hiding this comment.
| /// Liquidity provider struct that holds the overal info on the user's iAssets | |
| /// Liquidity provider struct that holds the overall info on the user's iAssets |
| ///Index of the last observable lockup cycle (OLC), crucial for the minting of preminted tokens and the withdrawal of unlocked tokens. | ||
| current_olc_index: u64, //not update at any point?? | ||
| ///Represents the nominal value of all assets submitted to the system, indicating the total economic stake. | ||
| total_nominal_liquidity: u64 |
There was a problem hiding this comment.
If total_nominal_liquidity can be computed, we should avoid global storage. Even if we want to store intermediate value, if we just need it for a single tx, this should be computed once and passed around rather than being stored.
| /// it stores assets in a table mapping their symbols to a bool | ||
| /// it can change to an asset id or an address depending on which is convenient | ||
| struct AssetTracker has key { | ||
| liquidity_provider_objects: SimpleMap<address, address>, |
There was a problem hiding this comment.
What does key and value signify here?
There was a problem hiding this comment.
Okay, I will remove the SimpleMap. Store the Object in the user address.
| /// it can change to an asset id or an address depending on which is convenient | ||
| struct AssetTracker has key { | ||
| liquidity_provider_objects: SimpleMap<address, address>, | ||
| asset_entry_tracker: SimpleMap<address, address>, |
There was a problem hiding this comment.
Again, please add a description to every field, what is key address and value address mean here?
| option::none(), | ||
| string::utf8(iAsset_name), | ||
| string::utf8(iAsset_symbol), | ||
| 6, |
There was a problem hiding this comment.
Why are we using 6 for decimal places? Why not 8?
|
|
||
| move_to(metadata_object_signer, LiquidityTableItems { | ||
| pair_id, | ||
| asset_supply: 0, |
There was a problem hiding this comment.
Why are we storing supply? This is available at metadata object address inside Supply struct, there is an existing supply method in fungible_asset.move
| /// Description: returns the address of the assets's metadata derived from the creator and its symbol | ||
| /// @param: symbol: the symbol used during the creation of the asset's object | ||
| public fun asset_address(symbol: vector<u8>): address { | ||
| object::create_object_address(&@supra_framework, symbol) |
There was a problem hiding this comment.
This is not enough, a check needs to be added that such a derived address already has the metadata object.
| /// Function: get_asset_price(asset_symbol) | ||
| /// Description: Gets an asset's prce from the price oracle | ||
| /// @param: asset_symbol - vector<u8> | ||
| public fun get_asset_price(asset_symbol: vector<u8>): (u64, u16, u64, u64) acquires LiquidityTableItems { |
There was a problem hiding this comment.
It is very inefficient to use vector<u8> everywhere. Instead just use a unique id: u64, the metadata already has the information about asset_symbol.
| let (value, decimal, timestamp, round) = supra_oracle_storage::get_price(pair_id); | ||
| let value_u64 = safe_u128_to_u64(value); | ||
| assert!(option::is_some(&value_u64), 12); | ||
| (0, decimal, timestamp, round) |
There was a problem hiding this comment.
Why are you returning (0,.. as the first component here? Also, why is type narrowing from u128 to u64 is required?
| /// @param: asset - Object<Metadata> | ||
| /// returns: u64 | ||
| public fun get_iAsset_supply(asset: Object<Metadata>): u64 { | ||
| (option::extract(&mut fungible_asset::supply(asset)) as u64) |
There was a problem hiding this comment.
Why are we narrowing asset supply to u64?
| /// Function: get_assets | ||
| /// Description: Gets all asset names from AssetTracker | ||
| /// returns: vector<vector<u8> | ||
| public fun get_assets(): vector<vector<u8>> acquires AssetTracker { |
There was a problem hiding this comment.
Again, a very bad idea to treat vector<u8> as a primary key to perform all operation, just use u64.
| AssetEntry { | ||
| user_reward_index: 0, | ||
| preminted_iAssets: 0, | ||
| redeem_requested_iAssets: 0, | ||
| preminiting_OLC_index: 0, | ||
|
|
||
| } |
There was a problem hiding this comment.
Do we want AssetEntry on per user, per asset basis? Creating yet another object which is derived from store_address creates yet another level of indirection. Directly perform a move_to on the user address.
There was a problem hiding this comment.
Okay, I will use create_signer.
Co-authored-by: Saurabh Joshi <[email protected]>
| ) acquires BridgeAddresses { | ||
|
|
||
| let bridge_addresses = borrow_global<BridgeAddresses>(get_poel_storage_address()); | ||
|
|
There was a problem hiding this comment.
The current logic allows only bridges to submit borrow requests, which means that the assets deposited into the IntraLayer vaults are exclusively non-native. However, since native assets can also be deposited into the scheme, we need to consider the logic so that other contracts deployed on the Supra chain are also permitted to call the borrow_request function . So some interlayer vaults themselves can be deployed on Supra chain, where users can deposit their Supra native assets
| /// Facilitates the creation of borrow requests following the deposition of the original asset into | ||
| /// an intermediary vault. One of the main reasons why the borrow_request function has been suggested | ||
| /// in the flow to borrow is because the pending_active coins do not earn rewards. | ||
| public fun borrow_request( |
There was a problem hiding this comment.
let's rename this function rent_request to be consistent
|
|
||
| entry fun add_multiperiod_extra_rewards(amount: u64, account: &signer) acquires ExtraReward { | ||
| let extra_reward_ref = borrow_global_mut<ExtraReward>(get_poel_storage_address()); | ||
|
|
There was a problem hiding this comment.
I think here we need to assert that signer is the admin
| // Calculate the total stake by summing values from the pool | ||
| let (active_stake, inactive_stake, _) = get_stake(pool_address, get_poel_storage_address()); | ||
|
|
||
| let total_stake = active_stake + inactive_stake; |
There was a problem hiding this comment.
total_stake = active_stake + pending_inactive_stake + inactive_stake
| { | ||
| let pool_address = *key; | ||
| // Calculate the total stake by summing values from the pool | ||
| let (active_stake, inactive_stake, _) = get_stake(pool_address, get_poel_storage_address()); |
There was a problem hiding this comment.
here we need to fetch also 'pending_inactive_stake'
| total_rentable_amount = total_rentable_amount + ( | ||
| ((table_obj.asset_supply + table_obj.total_borrow_requests) - table_obj.total_withdraw_requests) / collateralisation_rate | ||
| ) * asset_price; | ||
| }); |
There was a problem hiding this comment.
update_single_asset_supply(asset_symbol, table_obj.total_borrow_requests)
I added the descriptions to the Proof of Efficiency Liquidity and IAsset modules with the initial type declaration guidelines for the struct objects and the initial errors to be expected. Next I should add the reward distribution guidelines (types and errors) and documented function signatures expected.