Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Remove all stale on_runtime_upgrade hooks in the runtime #10650

Merged
merged 9 commits into from
Jan 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions frame/contracts/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,12 @@
mod gas;
mod benchmarking;
mod exec;
mod migration;
mod schedule;
mod storage;
mod wasm;

pub mod chain_extension;
pub mod migration;
pub mod weights;

#[cfg(test)]
Expand Down Expand Up @@ -321,10 +321,6 @@ pub mod pallet {
Storage::<T>::process_deletion_queue_batch(weight_limit)
.saturating_add(T::WeightInfo::on_initialize())
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@athei please confirm this is not used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would I know if anyone uses this? It will migrate when appropriate. The code isn't there for fun.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well this is exactly the reason why we are removing these hooks, because it is not clear who and when uses them.

My rephrased question is: are there any teams that you are aware of that would be affected if we remove this code?

note that I will re-expose this migration code as a standalone function, in case anyone needs it so they can manually apply it, and mark the PR is release-note.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its fine then. But I really much preferred the old way where it just worked. Won't I get all these dead code warnings after this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will see the outcome soon.

fn on_runtime_upgrade() -> Weight {
migration::migrate::<T>()
}
}

#[pallet::call]
Expand Down
1 change: 1 addition & 0 deletions frame/contracts/src/migration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ use frame_support::{
};
use sp_std::{marker::PhantomData, prelude::*};

/// Wrapper for all migrations of this pallet, based on `StorageVersion`.
pub fn migrate<T: Config>() -> Weight {
use frame_support::traits::StorageVersion;

Expand Down
10 changes: 1 addition & 9 deletions frame/offences/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
// Ensure we're `no_std` when compiling for Wasm.
#![cfg_attr(not(feature = "std"), no_std)]

mod migration;
pub mod migration;
mod mock;
mod tests;

Expand All @@ -47,7 +47,6 @@ type ReportIdOf<T> = <T as frame_system::Config>::Hash;
pub mod pallet {
use super::*;
use frame_support::pallet_prelude::*;
use frame_system::pallet_prelude::*;

#[pallet::pallet]
#[pallet::generate_store(pub(super) trait Store)]
Expand Down Expand Up @@ -110,13 +109,6 @@ pub mod pallet {
/// \[kind, timeslot\].
Offence { kind: Kind, timeslot: OpaqueTimeSlot },
}

#[pallet::hooks]
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
fn on_runtime_upgrade() -> Weight {
migration::remove_deferred_storage::<T>()
}
}
}

impl<T: Config, O: Offence<T::IdentificationTuple>>
Expand Down
5 changes: 2 additions & 3 deletions frame/offences/src/migration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,7 @@ pub fn remove_deferred_storage<T: Config>() -> Weight {
#[cfg(test)]
mod test {
use super::*;
use crate::mock::{new_test_ext, with_on_offence_fractions, Offences, Runtime as T};
use frame_support::traits::OnRuntimeUpgrade;
use crate::mock::{new_test_ext, with_on_offence_fractions, Runtime as T};
use sp_runtime::Perbill;
use sp_staking::offence::OffenceDetails;

Expand Down Expand Up @@ -87,7 +86,7 @@ mod test {

// when
assert_eq!(
Offences::on_runtime_upgrade(),
remove_deferred_storage::<T>(),
<T as frame_system::Config>::DbWeight::get().reads_writes(1, 1),
);

Expand Down
23 changes: 3 additions & 20 deletions frame/staking/src/pallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@ mod impls;
pub use impls::*;

use crate::{
log, migrations, slashing, weights::WeightInfo, ActiveEraInfo, BalanceOf, EraPayout,
EraRewardPoints, Exposure, Forcing, NegativeImbalanceOf, Nominations, PositiveImbalanceOf,
Releases, RewardDestination, SessionInterface, StakingLedger, UnappliedSlash, UnlockChunk,
log, slashing, weights::WeightInfo, ActiveEraInfo, BalanceOf, EraPayout, EraRewardPoints,
Exposure, Forcing, NegativeImbalanceOf, Nominations, PositiveImbalanceOf, Releases,
RewardDestination, SessionInterface, StakingLedger, UnappliedSlash, UnlockChunk,
ValidatorPrefs,
};

Expand Down Expand Up @@ -660,23 +660,6 @@ pub mod pallet {

#[pallet::hooks]
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
fn on_runtime_upgrade() -> Weight {
if StorageVersion::<T>::get() == Releases::V6_0_0 {
migrations::v7::migrate::<T>()
} else {
T::DbWeight::get().reads(1)
}
}

#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<(), &'static str> {
if StorageVersion::<T>::get() == Releases::V6_0_0 {
migrations::v7::pre_migrate::<T>()
} else {
Ok(())
}
}

fn on_initialize(_now: BlockNumberFor<T>) -> Weight {
// just return the weight of the on_finalize.
T::DbWeight::get().reads(1)
Expand Down
9 changes: 7 additions & 2 deletions frame/support/src/traits/hooks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,10 +244,15 @@ pub trait Hooks<BlockNumber> {
/// # Warning
///
/// This function will be called before we initialized any runtime state, aka `on_initialize`
/// wasn't called yet. So, information like the block number and any other
/// block local data are not accessible.
/// wasn't called yet. So, information like the block number and any other block local data are
/// not accessible.
///
/// Return the non-negotiable weight consumed for runtime upgrade.
///
/// While this function can be freely implemented, using `on_runtime_upgrade` from inside the
/// pallet is discouraged and might get deprecated in the future. Alternatively, export the same
/// logic as a free-function from your pallet, and pass it to `type Executive` from the
/// top-level runtime.
fn on_runtime_upgrade() -> crate::weights::Weight {
0
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
error[E0277]: the trait bound `pallet::GenesisConfig: std::default::Default` is not satisfied
--> $DIR/genesis_default_not_satisfied.rs:22:18
--> tests/pallet_ui/genesis_default_not_satisfied.rs:22:18
|
22 | impl<T: Config> GenesisBuild<T> for GenesisConfig {}
| ^^^^^^^^^^^^^^^ the trait `std::default::Default` is not implemented for `pallet::GenesisConfig`
|
note: required by a bound in `GenesisBuild`
--> $DIR/hooks.rs:297:36
--> $WORKSPACE/frame/support/src/traits/hooks.rs
|
297 | pub trait GenesisBuild<T, I = ()>: Default + sp_runtime::traits::MaybeSerializeDeserialize {
| pub trait GenesisBuild<T, I = ()>: Default + sp_runtime::traits::MaybeSerializeDeserialize {
| ^^^^^^^ required by this bound in `GenesisBuild`
13 changes: 3 additions & 10 deletions frame/uniques/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,13 @@ mod benchmarking;
pub mod mock;
#[cfg(test)]
mod tests;
pub mod weights;

mod functions;
mod impl_nonfungibles;
mod types;
pub use types::*;

mod migration;
pub mod migration;
pub mod weights;

use codec::{Decode, Encode, HasCompact};
use frame_support::traits::{BalanceStatus::Reserved, Currency, ReservableCurrency};
Expand All @@ -52,6 +51,7 @@ use sp_runtime::{
use sp_std::prelude::*;

pub use pallet::*;
pub use types::*;
pub use weights::WeightInfo;

#[frame_support::pallet]
Expand Down Expand Up @@ -316,13 +316,6 @@ pub mod pallet {
Unapproved,
}

#[pallet::hooks]
impl<T: Config<I>, I: 'static> Hooks<BlockNumberFor<T>> for Pallet<T, I> {
fn on_runtime_upgrade() -> frame_support::weights::Weight {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hamidra please confirm this is not used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed for the next statemine/statemint release to add ClassAccount storage to uniques.
Is this PR just a cleanup or we need to get rid of all migrations in the pallets?!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay @hamidra you need to make sure that you we manually add this migration now to the next release of statemine. cc @joepetrowski

migration::migrate_to_v1::<T, I, Self>()
}
}

impl<T: Config<I>, I: 'static> Pallet<T, I> {
/// Get the owner of the asset instance, if the asset exists.
pub fn owner(class: T::ClassId, instance: T::InstanceId) -> Option<T::AccountId> {
Expand Down
1 change: 1 addition & 0 deletions frame/uniques/src/migration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ use frame_support::{
weights::Weight,
};

/// Migrate the pallet storage to v1.
pub fn migrate_to_v1<T: Config<I>, I: 'static, P: GetStorageVersion + PalletInfoAccess>(
) -> frame_support::weights::Weight {
let on_chain_storage_version = <P as GetStorageVersion>::on_chain_storage_version();
Expand Down
26 changes: 2 additions & 24 deletions frame/vesting/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,14 @@
#![cfg_attr(not(feature = "std"), no_std)]

mod benchmarking;
mod migrations;

#[cfg(test)]
mod mock;
#[cfg(test)]
mod tests;
mod vesting_info;

pub mod migrations;
pub mod weights;

use codec::{Decode, Encode, MaxEncodedLen};
Expand Down Expand Up @@ -179,29 +180,6 @@ pub mod pallet {

#[pallet::hooks]
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
#[cfg(feature = "try-runtime")]
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
fn pre_upgrade() -> Result<(), &'static str> {
if StorageVersion::<T>::get() == Releases::V0 {
migrations::v1::pre_migrate::<T>()
} else {
Ok(())
}
}

fn on_runtime_upgrade() -> Weight {
if StorageVersion::<T>::get() == Releases::V0 {
StorageVersion::<T>::put(Releases::V1);
migrations::v1::migrate::<T>().saturating_add(T::DbWeight::get().reads_writes(1, 1))
} else {
T::DbWeight::get().reads(1)
}
}

#[cfg(feature = "try-runtime")]
fn post_upgrade() -> Result<(), &'static str> {
migrations::v1::post_migrate::<T>()
}

fn integrity_test() {
assert!(T::MAX_VESTING_SCHEDULES > 0, "`MaxVestingSchedules` must ge greater than 0");
}
Expand Down
8 changes: 4 additions & 4 deletions frame/vesting/src/migrations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@
use super::*;

// Migration from single schedule to multiple schedules.
pub(crate) mod v1 {
pub mod v1 {
use super::*;

#[cfg(feature = "try-runtime")]
pub(crate) fn pre_migrate<T: Config>() -> Result<(), &'static str> {
pub fn pre_migrate<T: Config>() -> Result<(), &'static str> {
assert!(StorageVersion::<T>::get() == Releases::V0, "Storage version too high.");

log::debug!(
Expand All @@ -37,7 +37,7 @@ pub(crate) mod v1 {

/// Migrate from single schedule to multi schedule storage.
/// WARNING: This migration will delete schedules if `MaxVestingSchedules < 1`.
pub(crate) fn migrate<T: Config>() -> Weight {
pub fn migrate<T: Config>() -> Weight {
let mut reads_writes = 0;

Vesting::<T>::translate::<VestingInfo<BalanceOf<T>, T::BlockNumber>, _>(
Expand Down Expand Up @@ -65,7 +65,7 @@ pub(crate) mod v1 {
}

#[cfg(feature = "try-runtime")]
pub(crate) fn post_migrate<T: Config>() -> Result<(), &'static str> {
pub fn post_migrate<T: Config>() -> Result<(), &'static str> {
assert_eq!(StorageVersion::<T>::get(), Releases::V1);

for (_key, schedules) in Vesting::<T>::iter() {
Expand Down