Issue 156 queue reserve inits #158
Conversation
pool-factory/src/storage.rs
Outdated
| /// TODO: Duplicated from pool/storage.rs. Can this be moved to a common location? | ||
| #[derive(Clone)] | ||
| #[contracttype] | ||
| pub struct ReserveConfig { |
There was a problem hiding this comment.
this can be fetched from the Pool WASM, to avoid duplication. The Pool Factory is already dependent on it.
| /// ### Panics | ||
| /// If the reserve is not queued for initialization | ||
| /// or is already setup | ||
| /// or has invalid metadata |
There was a problem hiding this comment.
we should validate the metadata on queue
There was a problem hiding this comment.
That duplicates code since we reuse the initialize function which validates metadata
pool/src/constants.rs
Outdated
| @@ -8,3 +8,6 @@ pub const SCALAR_7: i128 = 1_0000000; | |||
|
|
|||
| // seconds per year | |||
| pub const SECONDS_PER_YEAR: i128 = 31536000; | |||
|
|
|||
| // approximate week in blocks assuming 5 seconds per block | |||
| pub const WEEK_IN_BLOCKS: u32 = 120960; | |||
There was a problem hiding this comment.
A week seems like a really long time, no?
Consider a DAO doing this:
- Create proposal for queueing the
init_reserve(2 days before voting, 3 day voting period, 2 day timelock) (7 days) - Wait ~ 1 day
- Create another proposal for submitting the init (7 day gov period)
So its a 15 day, 2 proposal process to add a reserve to the pool, assuming they are smart enough to double dip the timelock and proposal period.
There was a problem hiding this comment.
Will most DAO's have a mandatory 2 day period before voting? I made initialization permissionless so I think it's important the queue time is longer than a typical DAO vote -> execution time so they can cancel initializations if a malicious one snuck in without anyone noticing
pool/src/contract.rs
Outdated
| /// (Admin only) Cancels the queued initialization of a reserve in the pool | ||
| /// | ||
| /// ### Arguments | ||
| /// * `asset` - The underlying asset to add as a reserve | ||
| /// | ||
| /// ### Panics | ||
| /// If the caller is not the admin or the reserve is not queued for initialization | ||
| fn cancel_init_reserve(e: Env, asset: Address); |
There was a problem hiding this comment.
I think it's potentially problematic not to have one - consider someone sneaks through a bad DAO proposal or tricks a multisig into adding a reserve - note that executing the queued initialization is not permissioned
| ); | ||
| execute_queued_reserve_initialization(&e, &asset_id_0); | ||
| }); | ||
| } |
There was a problem hiding this comment.
need a test for cancel if we deem it should be kept
There was a problem hiding this comment.
test_execute_cancel_queued_reserve_initialization
| let mut reserve = pool.load_reserve(e, asset); | ||
| reserve.store(e); | ||
| index = reserve.index; | ||
| let reserve_config = storage::get_res_config(e, asset); |
There was a problem hiding this comment.
this is on the reserve object you already read
There was a problem hiding this comment.
no it isn't - we don't expose config on the reserve object
| ); | ||
| execute_queued_reserve_initialization(&e, &asset_id_0); | ||
| }); | ||
| } |
Addresses Issue 156 adding reserve initialization queues by adding reserve init queuing to the Pool contract for new reserve additions and requiring initial pool reserves to be added as part of pool initialization