paritytech
Migrations to have per-version storage structs
It would be useful to be able to have separate structs for storage items at each version of a migration, so we do not need to amend previously run migrations with different struct variations.
@kianenigma has started exploring this with the nomination pool migration at #13019.
Background
When structs of storage items are changed during a migration, it is challenging to reflect the struct's previous state within previous migrations.
This has been apparent with the work on nomination pool commission (#12511) where a commission field has been added to a BondedPoolsInner struct, which is used in the BondedPools storage item. The current solution is to amend previous migrations to the current structure of the storage item in question, and to comment within the migration that <field_name> was introduced in migration v4.
Yes, more or less what has been shown in https://github.com/paritytech/substrate/pull/13019.
can I attempt the issue? @rossbulat @kianenigma
Yes sure, no issues my side. I am engaged with some other things & @kianenigma is at the Polkadot Academy. Would be good to see some movement.
can I attempt the issue? @rossbulat @kianenigma
Yes sure, no issues my side. I am engaged with some other things & @kianenigma is at the Polkadot Academy. Would be good to see some movement.
I have a question about the code guidelines @kianenigma created.
I tried building cargo build --release the pr he created and I noticed a few things.
crate::BondedPools::<T>::translateexpects a BondedPoolInner struct returned instead of a BondedPoolInnerV1 struct which is being returned. I had thought of 2 approaches to fix this: (i) Explicitly return the BondedPoolInner type when migrating from V0 to V1. (ii) Rename BondedPoolInner Struct to BondedPoolInnerV1 wherever it's referenced to ensure it's coherent.
// ./nomination-pools/src/migration.rs
105. crate::BondedPools::<T>::translate::<BondedPoolInnerV0<T>, _>(|_key, old_value| {
106. translated.saturating_inc();
107. Some(old_value.migrate_to_v1()) <- expects a BondedPoolInner struct, which leads to issues when I try to build.
108. });
Please if you know of any reason why this is happening, I will appreciate it. @rossbulat
Hi @ECJ222, thank you for attempting this. I was experiencing the same issue as you were having.
The problem is, the migration always needs to refer to the current structure of BondedPoolInner, rather than looking back in history and getting the correct structure corresponding to that migration. Like you similarly suggested, I think the only options are:-
- To prepend the version for all structs in the pallet, and keep deprecated ones around - very nasty.
- To maintain the migration history by updating them with the current structures, and leave comments of the updates.
With that said, after some investigation I believe @kianenigma has made the call to not go ahead and maintain these migrations. I imagine they will be removed from the codebase shortly. Other pallets have been following this convention already and are only keeping around the latest migration. I can see why this is the case looking at this issue.