Implement vault capacity model #790
Conversation
gregdhill
force-pushed
the
feat/vault-capacity
branch
from
03ae1e4 to
8e58b5b
Compare
gregdhill
force-pushed
the
feat/vault-capacity
branch
from
8e58b5b to
b0470fd
Compare
gregdhill
force-pushed
the
feat/vault-capacity
branch
from
27cc92c to
b24503d
Compare
| nominator_id: &T::AccountId, | ||
| currency_id: CurrencyId<T>, | ||
| ) -> Result<BalanceOf<T>, DispatchError> { | ||
| frame_support::storage::with_transaction::<_, DispatchError, _>(|| { |
There was a problem hiding this comment.
I'm assuming this is safe to use from the RPC interface as long as we always rollback. @sander2 what do you think? I noticed you followed a similar approach in the integration tests. The alternative is to add a distribute_and_compute_reward method to the Rewards interface.
There was a problem hiding this comment.
If this is only used in rpcs, then no rollback should be required at all since rpcs shouldn't be able to make storage changes. If this is also called from with extrinsics, then it is necessary, and afaik it should be safe. The question either way is what happens if you try to (temporarily or permanently) modify chain state in rpcs. We'd need to test that it doesn't just panic, or that it's a no-op
There was a problem hiding this comment.
I tested this using a local standalone node and it seemed to work as expected. As far as I could tell nothing was written to state and the node definitely did not panic.
gregdhill
force-pushed
the
feat/vault-capacity
branch
from
aa508ff to
064719c
Compare
There was a problem hiding this comment.
I know we talked about this before, but I don't quite remember how you answered this previously. Why exactly do we need the PoolId? For the capacity pool, it's (), and for the normal reward layer it's a CurrencyId. However, as far as I can see, the CurrencyId we pass is always the vault's collateral currency so it doesn't seem to be doing anything. Note that the reward pallet already supported multiple reward currencies, independently of the vault's collateral currency.
| #[cfg(feature = "try-runtime")] | ||
| fn pre_upgrade() -> Result<Vec<u8>, &'static str> { | ||
| let prev_count = reward::migration::v0::Stake::<Runtime, VaultRewardsInstance>::iter().count(); | ||
| log::info!(target: TARGET, "{} stake entries will be migrated", prev_count); |
There was a problem hiding this comment.
Instead of comparing the totals, can we compare the balances of each vault after using with_transaction to do a dry-run of withdraw_rewards?
| "Previous rewards should be in staking" | ||
| ); | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
I'd like to have some general integrity checks, that check that the capacity, reward, staking and TotalUserVaultCollateral entries are consistent. This will be useful to check the correctness of the migration, but will also be useful to continuously check the integrity using the new try-state
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
gregdhill
force-pushed
the
feat/vault-capacity
branch
from
6fc7f7c to
4a20c18
Compare
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Picked up from #783, rebased on #781
Compilation is working, integration tests are working (although some tests in test_fee_pool are commented out as they have not been updated yet. However, I am still pretty confident that the code is working correctly, since I updated the randomized test (
test_fee_pool_matches_ideal_implementation) and it seems to be doing good.All stake manipulation is done through the new file
vault-registry/src/pool-manager.rs. I had to add a hook in Loans to update the exchange rate of lend tokens, since they change when the exchange rate of the underlying changes. We'll still need to call the hook in all other places where the exchange rate of lend tokens can change.