migrations: prevent accidentally using unversioned migrations instead of VersionedMigration (#3835)

closes #1324 

#### Problem
Currently, it is possible to accidentally use inner unversioned
migration instead of `VersionedMigration` since both implement
`OnRuntimeUpgrade`.

#### Solution

With this change, we make it clear that value of `Inner` is not intended
to be used directly. It is achieved by bounding `Inner` to new trait
`UncheckedOnRuntimeUpgrade`, which has the same interface (except
`unchecked_` prefix) as `OnRuntimeUpgrade`.

#### `try-runtime` functions

Since developers can implement `try-runtime` for `Inner` value in
`VersionedMigration` and have custom logic for it, I added the same
`try-runtime` functions to `UncheckedOnRuntimeUpgrade`. I looked for a
ways to not duplicate functions, but couldn't find anything that doesn't
significantly change the codebase. So I would appreciate If you have any
suggestions to improve this

cc @liamaharon @xlc 

polkadot address: 16FqwPZ8GRC5U5D4Fu7W33nA55ZXzXGWHwmbnE1eT6pxuqcT

---------

Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
This commit is contained in:
Dastan
2024-04-02 15:43:09 +02:00
committed by GitHub
parent 8e95a3e1aa
commit e54279699b
22 changed files with 255 additions and 187 deletions
@@ -89,7 +89,7 @@
//!
//! See the migration source code for detailed comments.
//!
//! To keep the migration logic organised, it is split across additional modules:
//! Here's a brief overview of modules and types defined in `v1.rs`:
//!
//! ### `mod v0`
//!
@@ -98,28 +98,29 @@
//!
//! This allows reading the old v0 value from storage during the migration.
//!
//! ### `mod version_unchecked`
//! ### `InnerMigrateV0ToV1`
//!
//! Here we define our raw migration logic,
//! `version_unchecked::MigrateV0ToV1` which implements the [`OnRuntimeUpgrade`] trait.
//! `InnerMigrateV0ToV1` which implements the [`UncheckedOnRuntimeUpgrade`] trait.
//!
//! Importantly, it is kept in a private module so that it cannot be accidentally used in a runtime.
//! #### Why [`UncheckedOnRuntimeUpgrade`]?
//!
//! Private modules cannot be referenced in docs, so please read the code directly.
//! Otherwise, we would have two implementations of [`OnRuntimeUpgrade`] which could be confusing,
//! and may lead to accidentally using the wrong one.
//!
//! #### Standalone Struct or Pallet Hook?
//!
//! Note that the storage migration logic is attached to a standalone struct implementing
//! [`OnRuntimeUpgrade`], rather than implementing the
//! [`UncheckedOnRuntimeUpgrade`], rather than implementing the
//! [`Hooks::on_runtime_upgrade`](frame_support::traits::Hooks::on_runtime_upgrade) hook directly on
//! the pallet. The pallet hook is better suited for special types of logic that need to execute on
//! every runtime upgrade, but not so much for one-off storage migrations.
//!
//! ### `pub mod versioned`
//! ### `MigrateV0ToV1`
//!
//! Here, `version_unchecked::MigrateV0ToV1` is wrapped in a
//! Here, `InnerMigrateV0ToV1` is wrapped in a
//! [`VersionedMigration`] to define
//! [`versioned::MigrateV0ToV1`](crate::migrations::v1::versioned::MigrateV0ToV1), which may be used
//! [`MigrateV0ToV1`](crate::migrations::v1::MigrateV0ToV1), which may be used
//! in runtimes.
//!
//! Using [`VersionedMigration`] ensures that
@@ -128,8 +129,6 @@
//! - Reads and writes from checking and setting the on-chain storage version are accounted for in
//! the final [`Weight`](frame_support::weights::Weight)
//!
//! This is the only public module exported from `v1`.
//!
//! ### `mod test`
//!
//! Here basic unit tests are defined for the migration.
@@ -142,7 +141,8 @@
//! [`VersionedMigration`]: frame_support::migrations::VersionedMigration
//! [`GetStorageVersion`]: frame_support::traits::GetStorageVersion
//! [`OnRuntimeUpgrade`]: frame_support::traits::OnRuntimeUpgrade
//! [`MigrateV0ToV1`]: crate::migrations::v1::versioned::MigrationV0ToV1
//! [`UncheckedOnRuntimeUpgrade`]: frame_support::traits::UncheckedOnRuntimeUpgrade
//! [`MigrateV0ToV1`]: crate::migrations::v1::MigrateV0ToV1
// We make sure this pallet uses `no_std` for compiling to Wasm.
#![cfg_attr(not(feature = "std"), no_std)]