Altair loans sustitution #1266
Altair loans sustitution #1266
Conversation
lemunozm
added
D2-notify
lemunozm
requested review from
mikiquantum,
mustermeiszer,
NunoAlexandre and
hieronx
as code owners
This is fine, on Altair. There's only 1 pool with 1 closed loan. |
pallets/loans-ref/src/migrations.rs
Outdated
| } | ||
|
|
||
| fn on_runtime_upgrade() -> Weight { | ||
| if v0::NextLoanId::<T>::iter_values().count() == 1 { |
There was a problem hiding this comment.
Ideally, I would have wanted to check here if the on_chain_storage_version() was 0, and bump the version to 1, but I could not do it because if I initialize the pallet with STORAGE_VERSION = 1, then it never enters here, because it will be already 1.
I think the issue was not setting the version to 0 in the past.
There was a problem hiding this comment.
I think the issue was not setting the version to 0 in the past.
This makes me think that maybe we should add this version for all pallets, in this way, if in the future we have a new migration, we can easily fetch the correct version number 0 for them.
There was a problem hiding this comment.
Yes, I have been meaning to open an issue for that. Here it is 🌚
| @@ -93,8 +94,11 @@ pub mod pallet { | |||
| <T as Config>::CurrencyId, | |||
| >>::PoolId; | |||
|
|
|||
| const STORAGE_VERSION: StorageVersion = StorageVersion::new(0); | |||
There was a problem hiding this comment.
We initialize the version to 0, for futures migrations
pallets/loans-ref/src/migrations.rs
Outdated
| return T::DbWeight::get().writes(result.unique.into()) | ||
| + T::DbWeight::get().reads(result.loops.into()); |
There was a problem hiding this comment.
It is expected 8 read and 8 writes
|
This PR is ready for review |
lemunozm
force-pushed
the
altair-loans-sustitution
branch
from
3a24836
to
186c627
Compare
pallets/loans-ref/src/migrations.rs
Outdated
| } | ||
|
|
||
| fn on_runtime_upgrade() -> Weight { | ||
| if v0::NextLoanId::<T>::iter_values().count() == 1 { |
There was a problem hiding this comment.
Yes, I have been meaning to open an issue for that. Here it is 🌚
pallets/loans-ref/src/migrations.rs
Outdated
| } else { | ||
| log::warn!("Loans: storage was already clear. This migration can be removed."); | ||
|
|
||
| Weight::zero() |
There was a problem hiding this comment.
AFAIK we still perform a read
| Weight::zero() | |
| T::DbWeight::get().reads(1) |
There was a problem hiding this comment.
Following your hint, I've made a minor change for the sake of purism
| /// If this storage is not found, the nuking process is aborted. | ||
| #[storage_alias] | ||
| pub(crate) type NextLoanId<T: Config> = | ||
| StorageMap<Pallet<T>, Blake2_128Concat, PoolIdOf<T>, u128, ValueQuery>; |
There was a problem hiding this comment.
Do we get the right prefix as pallet-loans-ref and pallet-laons are both using the same prefix (i.e. Loans)?
There was a problem hiding this comment.
They use the same prefix, yes!
Fixes #1265 #1281
Tasks:
pallet-loansbypallet-loans-refinaltair.pallet-loans-refthat nukes the previouspallet-loansstorage only in case it is the old one.pallet-loansdependency and others associated with it inpallet-connectorspallet-loansis now safe to be removed fromcentrifuge-chainif this PR is merged (do not need to wait for the migration).