Introduce Vaults pallet (part of the pUSD Project) #10699
Introduce Vaults pallet (part of the pUSD Project) #10699lrazovic wants to merge 61 commits intoparitytech:masterfrom
Conversation
Waiting to generate the weights after the implementation is approved.
substrate/frame/vaults/src/lib.rs
Outdated
| /// 365.25 days × 24 hours × 60 minutes × 60 seconds × 1000 milliseconds = 31,557,600,000 | ||
| const MILLIS_PER_YEAR: u64 = 31_557_600_000; |
There was a problem hiding this comment.
Just put the calculation here instead of having it in a comment.
substrate/frame/vaults/src/lib.rs
Outdated
| // Cannot close a vault that's being liquidated | ||
| ensure!(vault.status == VaultStatus::Healthy, Error::<T>::VaultInLiquidation); | ||
|
|
||
| // Update fees |
There was a problem hiding this comment.
Please tell claude to not over comment stuff. The function name is descriptive enough.
substrate/frame/vaults/src/lib.rs
Outdated
| /// This ensures the Insurance Fund account is created with a provider reference so it can | ||
| /// receive any amount (including below ED) without risk of being reaped. | ||
| fn on_runtime_upgrade() -> Weight { | ||
| let on_chain_version = StorageVersion::get::<Pallet<T>>(); | ||
|
|
||
| if on_chain_version < 1 { | ||
| Self::ensure_insurance_fund_exists(); | ||
| StorageVersion::new(1).put::<Pallet<T>>(); | ||
|
|
||
| log::info!( | ||
| target: LOG_TARGET, | ||
| "Migrated storage from version {:?} to 1", | ||
| on_chain_version | ||
| ); | ||
|
|
||
| // Weight: 1 read (storage version) + 1 read (account_exists) + 2 writes | ||
| // (inc_providers + storage version) | ||
| T::DbWeight::get().reads_writes(2, 2) | ||
| } else { | ||
| log::debug!( | ||
| target: LOG_TARGET, | ||
| "No migration needed, on-chain version {:?}", | ||
| on_chain_version | ||
| ); | ||
| // Weight: 1 read (storage version check) | ||
| T::DbWeight::get().reads(1) | ||
| } | ||
| } |
There was a problem hiding this comment.
This should be some extra code in the runtime when we add this pallet.
substrate/frame/vaults/src/lib.rs
Outdated
| // Update cursor for next block | ||
| match last_processed { | ||
| Some(last) => { | ||
| if Vaults::<T>::iter_from(Vaults::<T>::hashed_key_for(&last)).nth(1).is_none() { |
There was a problem hiding this comment.
So we are doing one more iteration that isn't part of the weight? Maybe we should just do this at the top of the function.
substrate/frame/vaults/src/lib.rs
Outdated
| remaining_collateral, | ||
| total_obligation, | ||
| )? | ||
| .expect("total_obligation is non-zero; qed"); |
There was a problem hiding this comment.
Not really a problem to convert this into an error, better safe than sorry here.
There was a problem hiding this comment.
Not sure why these types are not part of the vault crate right now. This code should be clearly not be in this folder.
There was a problem hiding this comment.
These types will be reused by the other pallets in the pUSD family. Currently they're only used by this pallet, but once we start adding the other pallets, they'll be imported from this common crate. If you have a suggestion for better placement, I'm happy to move them accordingly!
There was a problem hiding this comment.
Substrate is not the right place for these items. For Frame pallets common items usually located under support crate. But these traits and types I would locate for now next to their implementations. Later if we have impls for them, we modify them if needed and move to support. But for now I would keep it simple.
There was a problem hiding this comment.
we still need to move this under frame
| } | ||
| } | ||
|
|
||
| /// Mock oracle adapter that provides a fixed price for noe. |
muharem
left a comment
There was a problem hiding this comment.
reviewed the implementation, have not checked the tests and the benchmarks. gonna continue tomorrow.
| type AssetId: Parameter + Member + Copy + MaybeSerializeDeserialize + MaxEncodedLen; | ||
|
|
||
| /// Time provider for fee accrual using UNIX timestamps. | ||
| type TimeProvider: Time; |
There was a problem hiding this comment.
we do not have a working time provider solution. if you have not too, we need to migrate this into block provider. The block provider should be configurable type. You can check society pallet for an example.
There was a problem hiding this comment.
We actually started with block numbers, but we switched to timestamps (via pallet-timestamps) because the interest accrual domain benefits from real time rather than block-number-as-proxy
There was a problem hiding this comment.
its accuracy is prev_timestamp + 3sec < current_timestamp > prev_timestamp + 30sec. not that great, and relies on prev timestamp value. I do not know a case where we used it. I would not do it with this pallet if we do not really know what are we doing.
actually that ^ accuracy only applicable for Relay Chain. for Asset Hubs that 3 sec is 0. tho we probably can pass timestamps from RC to AH, but only from the RC block AH building its current block.
if we use block numbers, on AH its gonna be RC block numbers too.
cc: @kianenigma @seadanda
There was a problem hiding this comment.
Using the AH Timestamp, until AH collators are the same as RC validators (something that is envisioned long term) is a no-go IMO. It is way too little security.
Using the relay Timestamp is IMO a good option. It is ultimately an oraclized value that the runtime can never know exactly what it is, but relying on the validators is as good as it gets. I was under the impression that the Relay Timestamp is part of the PersistedValidationData, but it is not. The state root is, and we can use a state-proof against that. Not ideal though. If we are to use this, I would explore doing it properly and via #6235
Relay block number is also a safe Plan B. although it has its own set of trade-offs: If we ever ever change the relay block time, all hell breaks loose. Kusama/JAM is already being toyed with having shorter relay slots.
Verdict:
- Dispatch claude with a good prompt to fix Customize-able
ParacnainInherentDataProvider#6235. If not overly complicated, this feature will help multiple systems. - If not straightforward, then use relay timestamp
- AH timestamp not a viable option IMO.
There was a problem hiding this comment.
If I understand correctly, paritytech/polkadot-sdk#10678 introduced a KeyToIncludeInRelayProof runtime API that lets parachains request arbitrary relay chain storage keys in the inherent proof. Building on that, accessing the RC Timestamp should now be straightforward. A first draft (untested) is available here: https://gist.github.com/lrazovic/bf701015bcc8e482207c1034b5b7a346
There was a problem hiding this comment.
Nice, without looking at the gist, the approach sounds exactly correct to me.
| let total_issuance = T::Asset::total_issuance(T::StablecoinAssetId::get()); | ||
| ensure!( | ||
| total_issuance.saturating_add(amount) <= | ||
| MaximumIssuance::<T>::get().ok_or(Error::<T>::NotConfigured)?, |
There was a problem hiding this comment.
should we have some type to get PSM reserved capacity and subtract it?
There was a problem hiding this comment.
we can do this when we locate PsmInterface trait in frame/support
| vault.accrued_interest.saturating_accrue(accrued); | ||
| vault.last_fee_update = now; |
There was a problem hiding this comment.
I think I can update my vault lets say every block and have accrued always set to 0 and last_fee_update to now.
There was a problem hiding this comment.
if StabilityFee updated at now, it takes effect for the vaults from last_fee_update, not from now. right? is this expected?
There was a problem hiding this comment.
For correctness, In 97d0989 I introduced a PreviousStabilityFee storage item that lets update_vault_fees split interest accrual at the fee change boundary, so the old rate applies before the change and the new rate only applies after it
substrate/frame/vaults/src/lib.rs
Outdated
|
|
||
| Self::deposit_event(Event::BadDebtRepaid { amount: burned }); | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
we can make this call for free if there was anything to burn
There was a problem hiding this comment.
its should be for free only if the execution really healed some amount. like its done for liquidate_vault call.
There was a problem hiding this comment.
Substrate is not the right place for these items. For Frame pallets common items usually located under support crate. But these traits and types I would locate for now next to their implementations. Later if we have impls for them, we modify them if needed and move to support. But for now I would keep it simple.
substrate/frame/vaults/src/lib.rs
Outdated
| if !keeper_incentive.is_zero() { | ||
| T::Asset::transfer( | ||
| T::StablecoinAssetId::get(), | ||
| &T::InsuranceFund::get(), | ||
| keeper, | ||
| keeper_incentive, | ||
| Preservation::Expendable, | ||
| )?; | ||
| } |
There was a problem hiding this comment.
this doesn't seem right to me, the keeper_incentive transfer using ? propagates any transfer failure, but all of the auction finalization logic, collateral release, CurrentLiquidationAmount decrement, bad-debt recording, and vault removal sits after this point. If the Insurance Fund has insufficient pUSD for the keeper payment, the entire complete_auction call exits early and none of those steps execute.
This seems to leave the vault permanently in VaultStatus::InLiquidation with collateral held under HoldReason::Seized and no way (not entirely sure if this is true) to finalize it. All user-callable vault operations gate on VaultStatus::Healthy, and there's no force_complete_auction or similar recovery extrinsic in the pallet.
The keeper payment should be non-blocking for auction finalization. One option is to attempt the transfer and, on failure, record the unpaid incentive for later settlement rather than aborting the whole completion. Something like:
if !keeper_incentive.is_zero() {
let payment_result = T::Asset::transfer(
T::StablecoinAssetId::get(),
&T::InsuranceFund::get(),
keeper,
keeper_incentive,
Preservation::Expendable,
);
if let Err(_) = payment_result {
// Record as owed; don't block finalization
UnpaidKeeperIncentives::<T>::mutate(keeper, |owed| {
owed.saturating_accrue(keeper_incentive);
});
Self::deposit_event(Event::KeeperIncentiveDeferred {
keeper: keeper.clone(),
amount: keeper_incentive,
});
}
}
// Finalization (collateral release, CLA decrement, vault removal) proceeds unconditionallyAlternatively, I am not sure if a governance-callable force_complete_auction extrinsic would make sense to provide a recovery path for any vault stuck in InLiquidation due to this or similar failures.
There was a problem hiding this comment.
Good point, part of 389590f. But I'm not sure if adding a new UnpaidKeeperIncentives storage item for this edge-case makes sense, we just emit an event for now
substrate/frame/vaults/src/lib.rs
Outdated
| /// The currency used for collateral (native DOT). | ||
| /// Collateral is managed via `pallet_balances` using holds. | ||
| /// The Balance type is derived from this and must implement `FixedPointOperand`. | ||
| type Currency: FungibleMutate<Self::AccountId, Balance: FixedPointOperand> |
There was a problem hiding this comment.
how about Collateral? Currency is outdated naming, refers more to Currency trait
substrate/frame/vaults/src/lib.rs
Outdated
| /// The asset used for pUSD debt. | ||
| /// Constrained to use the same Balance type as Currency. | ||
| /// Also implements `Balanced` for creating credits during surplus transfers. | ||
| type Asset: FungibleMutate<Self::AccountId, Balance = BalanceOf<Self>> |
Description
This PR introduces
pallet-vaults, a new FRAME pallet that implements a Collateralized Debt Position (CDP) system for creating over-collateralized stablecoin loans on Substrate-based blockchains. The pallet allows users to lock up DOT as collateral and mint pUSD against it.Integration
For Runtime Developers
To integrate
pallet-vaultsinto your runtime:Cargo.toml:For Pallet Developers
Other pallets can interact with vaults via the
CollateralManagertrait:Review Notes
Key Features
MutateHold(not transferred to a pallet account)AuctionsHandlerheal()extrinsicVault Lifecycle:
create_vault- Create a new Vault and lock initial collateraldeposit_collateral/withdraw_collateral- Manage collateralmint- Borrow pUSD against collateralrepay- Burn pUSD to reduce debt (interest paid first)close_vault- Close debt-free vault, release collateralliquidate_vault- Liquidate unsafe vaultspoke- Force fee accrual on any vaultHold Reasons:
VaultDeposit- Collateral backing active vaultsSeized- Collateral under liquidation, pending auctionTesting
The pallet includes comprehensive tests covering: