A implementation for asset disabling #32
Conversation
pool/src/storage.rs
Outdated
| @@ -54,6 +63,7 @@ pub struct ReserveConfig { | |||
| pub r_three: u32, // the R3 value in the interest rate formula scaled expressed in 7 decimals | |||
| pub reactivity: u32, // the reactivity constant for the reserve scaled expressed in 7 decimals | |||
| pub collateral_cap: i128, // the total amount of underlying tokens that can be used as collateral | |||
| pub flags: u32, // the flag of the reserve | |||
There was a problem hiding this comment.
Curious if there was a reason to store this as a u32 instead of a boolean?
At the moment we have no additional flag types planned, and the implementation would be simpler if we opted to do:
pub enabled: bool, // if the reserve is enabled. If the reserve is not enabled, positions can only be removed for this reserve.
There was a problem hiding this comment.
I was just considering struct alignment and future extensibility. Yes, it might be unnecessary and over-designed. Using a bool would be fine
pool/src/pool/actions.rs
Outdated
| @@ -133,6 +134,7 @@ pub fn build_actions_from_request( | |||
| } | |||
| RequestType::Withdraw => { | |||
| let mut reserve = pool.load_reserve(e, &request.address, true); | |||
| reserve.require_action_allowed(e, request.request_type); | |||
There was a problem hiding this comment.
withdrawing assets is always allowed, regardless of any combination of pool or reserve status, thus this call is not needed
pool/src/pool/actions.rs
Outdated
| @@ -207,6 +212,7 @@ pub fn build_actions_from_request( | |||
| } | |||
| RequestType::Repay => { | |||
| let mut reserve = pool.load_reserve(e, &request.address, true); | |||
| reserve.require_action_allowed(e, request.request_type); | |||
There was a problem hiding this comment.
repaying assets is always allowed, regardless of any combination of pool or reserve status, thus this call is not needed.
pool/src/storage.rs
Outdated
| /// Fetch the reserve flags summary | ||
| pub fn get_res_flags_summary(e: &Env) -> Map<Address, u32> { | ||
| get_persistent_default( | ||
| e, | ||
| &Symbol::new(e, RES_FLAGS_SUMMARY), | ||
| || map![e], | ||
| LEDGER_THRESHOLD_SHARED, | ||
| LEDGER_BUMP_SHARED, | ||
| ) | ||
| } | ||
|
|
||
| /// Set the reserve flags summary | ||
| /// | ||
| /// ### Arguments | ||
| /// * `summary` - The map of reserve's status summary by reserve address to a status u32 | ||
| pub fn set_res_flags_summary(e: &Env, summary: &Map<Address, u32>) { | ||
| e.storage() | ||
| .persistent() | ||
| .set::<Symbol, Map<Address, u32>>(&Symbol::new(e, RES_FLAGS_SUMMARY), summary); | ||
| e.storage().persistent().extend_ttl( | ||
| &Symbol::new(e, RES_FLAGS_SUMMARY), | ||
| LEDGER_THRESHOLD_SHARED, | ||
| LEDGER_BUMP_SHARED, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think this optimization is necessary, as we need to load reserve_config for each asset when conduction gulp_emissions anyway. Please see comment on emissions/manager.rs for context.
Thus, don't need to maintain a second source of truth for the asset status, and can remove the res_flag_summary code.
pool/src/emissions/manager.rs
Outdated
| .unwrap_optimized() | ||
| .fixed_mul_floor(new_emissions, SCALAR_7) | ||
| .unwrap_optimized(); | ||
| update_reserve_emission_eps(e, &res_asset_address, res_token_id, new_reserve_emissions); |
There was a problem hiding this comment.
update_reserve_emissions_eps loads the reserve config for each asset anyway.
My suggestion:
- Use
reserve_configto load the asset's enabled status - Pass the
reserve_configintoupdate_reserve_emission_epsinstead of theres_token_idto avoid loading it twice.reserve_config.id === res_token_id
There was a problem hiding this comment.
This will likely require us to build a pool_emis_enabled: Vec<(reserve_config, res_eps_share)> vector during the first pass that determines the total_share, to also avoid loading the reserve_config twice.
remove "res_flag_summary" storage "do_gulp_emissions" modified some other code modified
|
Hello @mootz12 , I have modified the code based on your feedback. Please take a look. Thanks! |
Hi!
I have provided an implementation for the asset disabling feature here. Please take a look. Thanks!
The main changes are:
Looking forward to your reply