From ac5c1255025c3ebf2594d4652623aff5656fb044 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Mon, 18 Oct 2021 10:34:34 +0200 Subject: [PATCH 01/12] fix order --- .../procedural/src/construct_runtime/mod.rs | 31 ++++++++- frame/support/test/tests/pallet.rs | 65 +++++++++++++++++++ frame/support/test/tests/pallet_instance.rs | 13 ++-- 3 files changed, 101 insertions(+), 8 deletions(-) diff --git a/frame/support/procedural/src/construct_runtime/mod.rs b/frame/support/procedural/src/construct_runtime/mod.rs index 04bb2ead645d2..11a3eb313128a 100644 --- a/frame/support/procedural/src/construct_runtime/mod.rs +++ b/frame/support/procedural/src/construct_runtime/mod.rs @@ -204,15 +204,32 @@ fn decl_all_pallets<'a>( types.extend(type_decl); names.push(&pallet_declaration.name); } - // Make nested tuple structure like (((Babe, Consensus), Grandpa), ...) + + // Make nested tuple structure like `((FirstPallet, (SecondPallet, ( ... , (LastPallet) ... ))))` // But ignore the system pallet. let all_pallets = names .iter() .filter(|n| **n != SYSTEM_PALLET_NAME) + .rev() .fold(TokenStream2::default(), |combined, name| quote!((#name, #combined))); + // Make nested tuple structure like `((FirstPallet, (SecondPallet, ( ... , (LastPallet) ... ))))` let all_pallets_with_system = names .iter() + .rev() + .fold(TokenStream2::default(), |combined, name| quote!((#name, #combined))); + + // Make nested tuple structure like `((LastPallet, (SecondLastPallet, ( ... , (FirstPallet) ... ))))` + // But ignore the system pallet. + let all_pallets_reversed = names + .iter() + .filter(|n| **n != SYSTEM_PALLET_NAME) + .fold(TokenStream2::default(), |combined, name| quote!((#name, #combined))); + + // Make nested tuple structure like `((LastPallet, (SecondLastPallet, ( ... , (FirstPallet) ... ))))` + let all_pallets_with_system_reversed = names + .iter() + .filter(|n| **n != SYSTEM_PALLET_NAME) .fold(TokenStream2::default(), |combined, name| quote!((#name, #combined))); quote!( @@ -220,9 +237,21 @@ fn decl_all_pallets<'a>( /// All pallets included in the runtime as a nested tuple of types. /// Excludes the System pallet. pub type AllPallets = ( #all_pallets ); + /// All pallets included in the runtime as a nested tuple of types. pub type AllPalletsWithSystem = ( #all_pallets_with_system ); + /// All pallets included in the runtime as a nested tuple of types in reversed order. + /// Excludes the System pallet. + #[deprecated(note = "use `AllPallets` instead")] + #[allow(dead_code)] + pub type AllPalletsReversed = ( #all_pallets_reversed ); + + /// All pallets included in the runtime as a nested tuple of types in reversed order. + #[deprecated(note = "use `AllPalletsWithSystem` instead")] + #[allow(dead_code)] + pub type AllPalletsWithSystemReversed = ( #all_pallets_with_system_reversed ); + /// All modules included in the runtime as a nested tuple of types. /// Excludes the System pallet. #[deprecated(note = "use `AllPallets` instead")] diff --git a/frame/support/test/tests/pallet.rs b/frame/support/test/tests/pallet.rs index dc72be3ebdd49..5d4891f0481ef 100644 --- a/frame/support/test/tests/pallet.rs +++ b/frame/support/test/tests/pallet.rs @@ -455,6 +455,17 @@ pub mod pallet2 { impl Hooks> for Pallet where T::AccountId: From + SomeAssociation1 { + fn on_initialize(_: BlockNumberFor) -> Weight { + Self::deposit_event(Event::Something(11)); + 0 + } + fn on_finalize(_: BlockNumberFor) { + Self::deposit_event(Event::Something(21)); + } + fn on_runtime_upgrade() -> Weight { + Self::deposit_event(Event::Something(31)); + 0 + } } #[pallet::call] @@ -468,6 +479,7 @@ pub mod pallet2 { CountedStorageMap; #[pallet::event] + #[pallet::generate_deposit(fn deposit_event)] pub enum Event { /// Something Something(u32), @@ -950,15 +962,68 @@ fn pallet_hooks_expand() { ); assert_eq!( frame_system::Pallet::::events()[1].event, + Event::Example2(pallet2::Event::Something(11)), + ); + assert_eq!( + frame_system::Pallet::::events()[2].event, Event::Example(pallet::Event::Something(20)), ); + assert_eq!( + frame_system::Pallet::::events()[3].event, + Event::Example2(pallet2::Event::Something(21)), + ); + assert_eq!( + frame_system::Pallet::::events()[4].event, + Event::Example(pallet::Event::Something(30)), + ); + assert_eq!( + frame_system::Pallet::::events()[5].event, + Event::Example2(pallet2::Event::Something(31)), + ); + }) +} + +#[test] +fn all_pallets_type_reversed_order_is_correct() { + TestExternalities::default().execute_with(|| { + frame_system::Pallet::::set_block_number(1); + + #[allow(deprecated)] + { + assert_eq!(AllPalletsReversed::on_initialize(1), 10); + AllPalletsReversed::on_finalize(1); + + assert_eq!(AllPalletsReversed::on_runtime_upgrade(), 30); + } + + assert_eq!( + frame_system::Pallet::::events()[0].event, + Event::Example2(pallet2::Event::Something(11)), + ); + assert_eq!( + frame_system::Pallet::::events()[1].event, + Event::Example(pallet::Event::Something(10)), + ); assert_eq!( frame_system::Pallet::::events()[2].event, + Event::Example2(pallet2::Event::Something(21)), + ); + assert_eq!( + frame_system::Pallet::::events()[3].event, + Event::Example(pallet::Event::Something(20)), + ); + assert_eq!( + frame_system::Pallet::::events()[4].event, + Event::Example2(pallet2::Event::Something(31)), + ); + assert_eq!( + frame_system::Pallet::::events()[5].event, Event::Example(pallet::Event::Something(30)), ); }) } + #[test] fn pallet_on_genesis() { TestExternalities::default().execute_with(|| { diff --git a/frame/support/test/tests/pallet_instance.rs b/frame/support/test/tests/pallet_instance.rs index 34586e8414216..b5ce0526e2b11 100644 --- a/frame/support/test/tests/pallet_instance.rs +++ b/frame/support/test/tests/pallet_instance.rs @@ -515,30 +515,29 @@ fn pallet_hooks_expand() { assert_eq!(AllPallets::on_runtime_upgrade(), 61); - // The order is indeed reversed due to https://github.com/paritytech/substrate/issues/6280 assert_eq!( frame_system::Pallet::::events()[0].event, - Event::Instance1Example(pallet::Event::Something(11)), + Event::Example(pallet::Event::Something(10)), ); assert_eq!( frame_system::Pallet::::events()[1].event, - Event::Example(pallet::Event::Something(10)), + Event::Instance1Example(pallet::Event::Something(11)), ); assert_eq!( frame_system::Pallet::::events()[2].event, - Event::Instance1Example(pallet::Event::Something(21)), + Event::Example(pallet::Event::Something(20)), ); assert_eq!( frame_system::Pallet::::events()[3].event, - Event::Example(pallet::Event::Something(20)), + Event::Instance1Example(pallet::Event::Something(21)), ); assert_eq!( frame_system::Pallet::::events()[4].event, - Event::Instance1Example(pallet::Event::Something(31)), + Event::Example(pallet::Event::Something(30)), ); assert_eq!( frame_system::Pallet::::events()[5].event, - Event::Example(pallet::Event::Something(30)), + Event::Instance1Example(pallet::Event::Something(31)), ); }) } From 8486f8241cfdbc67c82d39deff51f602e3baa8cb Mon Sep 17 00:00:00 2001 From: Guillaume Thiolliere Date: Mon, 18 Oct 2021 11:05:38 +0200 Subject: [PATCH 02/12] Update frame/support/procedural/src/construct_runtime/mod.rs Co-authored-by: Alexander Popiak --- frame/support/procedural/src/construct_runtime/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/frame/support/procedural/src/construct_runtime/mod.rs b/frame/support/procedural/src/construct_runtime/mod.rs index 11a3eb313128a..3c05622d50ee6 100644 --- a/frame/support/procedural/src/construct_runtime/mod.rs +++ b/frame/support/procedural/src/construct_runtime/mod.rs @@ -229,7 +229,6 @@ fn decl_all_pallets<'a>( // Make nested tuple structure like `((LastPallet, (SecondLastPallet, ( ... , (FirstPallet) ... ))))` let all_pallets_with_system_reversed = names .iter() - .filter(|n| **n != SYSTEM_PALLET_NAME) .fold(TokenStream2::default(), |combined, name| quote!((#name, #combined))); quote!( From 7a1e047cebb89dfa8000eef73ae60b2651bddbf3 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Mon, 18 Oct 2021 11:08:29 +0200 Subject: [PATCH 03/12] format --- .../support/procedural/src/construct_runtime/mod.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/frame/support/procedural/src/construct_runtime/mod.rs b/frame/support/procedural/src/construct_runtime/mod.rs index 3c05622d50ee6..7e607f8231761 100644 --- a/frame/support/procedural/src/construct_runtime/mod.rs +++ b/frame/support/procedural/src/construct_runtime/mod.rs @@ -205,7 +205,8 @@ fn decl_all_pallets<'a>( names.push(&pallet_declaration.name); } - // Make nested tuple structure like `((FirstPallet, (SecondPallet, ( ... , (LastPallet) ... ))))` + // Make nested tuple structure like: + // `((FirstPallet, (SecondPallet, ( ... , (LastPallet) ... ))))` // But ignore the system pallet. let all_pallets = names .iter() @@ -213,20 +214,23 @@ fn decl_all_pallets<'a>( .rev() .fold(TokenStream2::default(), |combined, name| quote!((#name, #combined))); - // Make nested tuple structure like `((FirstPallet, (SecondPallet, ( ... , (LastPallet) ... ))))` + // Make nested tuple structure like: + // `((FirstPallet, (SecondPallet, ( ... , (LastPallet) ... ))))` let all_pallets_with_system = names .iter() .rev() .fold(TokenStream2::default(), |combined, name| quote!((#name, #combined))); - // Make nested tuple structure like `((LastPallet, (SecondLastPallet, ( ... , (FirstPallet) ... ))))` + // Make nested tuple structure like: + // `((LastPallet, (SecondLastPallet, ( ... , (FirstPallet) ... ))))` // But ignore the system pallet. let all_pallets_reversed = names .iter() .filter(|n| **n != SYSTEM_PALLET_NAME) .fold(TokenStream2::default(), |combined, name| quote!((#name, #combined))); - // Make nested tuple structure like `((LastPallet, (SecondLastPallet, ( ... , (FirstPallet) ... ))))` + // Make nested tuple structure like: + // `((LastPallet, (SecondLastPallet, ( ... , (FirstPallet) ... ))))` let all_pallets_with_system_reversed = names .iter() .fold(TokenStream2::default(), |combined, name| quote!((#name, #combined))); From ba1c95ae77eb8a65c5c162a0fa35ee07406e8d7d Mon Sep 17 00:00:00 2001 From: thiolliere Date: Mon, 18 Oct 2021 11:26:46 +0200 Subject: [PATCH 04/12] more accurate description --- frame/support/procedural/src/construct_runtime/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/support/procedural/src/construct_runtime/mod.rs b/frame/support/procedural/src/construct_runtime/mod.rs index 7e607f8231761..382122697beff 100644 --- a/frame/support/procedural/src/construct_runtime/mod.rs +++ b/frame/support/procedural/src/construct_runtime/mod.rs @@ -206,7 +206,7 @@ fn decl_all_pallets<'a>( } // Make nested tuple structure like: - // `((FirstPallet, (SecondPallet, ( ... , (LastPallet) ... ))))` + // `((FirstPallet, (SecondPallet, ( ... , LastPallet) ... ))))` // But ignore the system pallet. let all_pallets = names .iter() @@ -215,14 +215,14 @@ fn decl_all_pallets<'a>( .fold(TokenStream2::default(), |combined, name| quote!((#name, #combined))); // Make nested tuple structure like: - // `((FirstPallet, (SecondPallet, ( ... , (LastPallet) ... ))))` + // `((FirstPallet, (SecondPallet, ( ... , LastPallet) ... ))))` let all_pallets_with_system = names .iter() .rev() .fold(TokenStream2::default(), |combined, name| quote!((#name, #combined))); // Make nested tuple structure like: - // `((LastPallet, (SecondLastPallet, ( ... , (FirstPallet) ... ))))` + // `((LastPallet, (SecondLastPallet, ( ... , FirstPallet) ... ))))` // But ignore the system pallet. let all_pallets_reversed = names .iter() @@ -230,7 +230,7 @@ fn decl_all_pallets<'a>( .fold(TokenStream2::default(), |combined, name| quote!((#name, #combined))); // Make nested tuple structure like: - // `((LastPallet, (SecondLastPallet, ( ... , (FirstPallet) ... ))))` + // `((LastPallet, (SecondLastPallet, ( ... , FirstPallet) ... ))))` let all_pallets_with_system_reversed = names .iter() .fold(TokenStream2::default(), |combined, name| quote!((#name, #combined))); From d94ff4aef919de23a31bc460862a6a07143cda5b Mon Sep 17 00:00:00 2001 From: thiolliere Date: Mon, 18 Oct 2021 11:26:59 +0200 Subject: [PATCH 05/12] format --- frame/support/test/tests/pallet.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/support/test/tests/pallet.rs b/frame/support/test/tests/pallet.rs index 5d4891f0481ef..7a6dc4737a0cd 100644 --- a/frame/support/test/tests/pallet.rs +++ b/frame/support/test/tests/pallet.rs @@ -452,8 +452,9 @@ pub mod pallet2 { pub struct Pallet(_); #[pallet::hooks] - impl Hooks> for Pallet where - T::AccountId: From + SomeAssociation1 + impl Hooks> for Pallet + where + T::AccountId: From + SomeAssociation1, { fn on_initialize(_: BlockNumberFor) -> Weight { Self::deposit_event(Event::Something(11)); @@ -1023,7 +1024,6 @@ fn all_pallets_type_reversed_order_is_correct() { }) } - #[test] fn pallet_on_genesis() { TestExternalities::default().execute_with(|| { From 15dc877128988a97f75d77672192fe6b06ebdbc5 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Sat, 30 Oct 2021 14:54:47 +0200 Subject: [PATCH 06/12] better explicit types --- bin/node-template/runtime/src/lib.rs | 2 +- bin/node/runtime/src/lib.rs | 2 +- frame/executive/src/lib.rs | 58 +++++-------------- .../procedural/src/construct_runtime/mod.rs | 35 +++++------ frame/support/test/tests/pallet.rs | 6 +- frame/support/test/tests/pallet_instance.rs | 6 +- 6 files changed, 37 insertions(+), 72 deletions(-) diff --git a/bin/node-template/runtime/src/lib.rs b/bin/node-template/runtime/src/lib.rs index 4b49cb48ef352..2a6273e24df0a 100644 --- a/bin/node-template/runtime/src/lib.rs +++ b/bin/node-template/runtime/src/lib.rs @@ -323,7 +323,7 @@ pub type Executive = frame_executive::Executive< Block, frame_system::ChainContext, Runtime, - AllPallets, + AllPalletsWithSystem, >; impl_runtime_apis! { diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index c7920629bf356..665dcb49d6873 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1327,7 +1327,7 @@ pub type Executive = frame_executive::Executive< Block, frame_system::ChainContext, Runtime, - AllPallets, + AllPalletsWithSystem, (), >; diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index b1bdf357ec07d..cb397b3037656 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -148,7 +148,8 @@ pub type OriginOf = as Dispatchable>::Origin; /// - `Block`: The block type of the runtime /// - `Context`: The context that is used when checking an extrinsic. /// - `UnsignedValidator`: The unsigned transaction validator of the runtime. -/// - `AllPallets`: Tuple that contains all modules. Will be used to call e.g. `on_initialize`. +/// - `AllPallets`: Tuple that contains all pallets including frame system pallet. Will be used to +/// call hooks e.g. `on_initialize`. /// - `OnRuntimeUpgrade`: Custom logic that should be called after a runtime upgrade. Modules are /// already called by `AllPallets`. It will be called before all modules will be called. pub struct Executive( @@ -210,14 +211,7 @@ where { /// Execute all `OnRuntimeUpgrade` of this runtime, and return the aggregate weight. pub fn execute_on_runtime_upgrade() -> frame_support::weights::Weight { - let mut weight = 0; - weight = weight.saturating_add(COnRuntimeUpgrade::on_runtime_upgrade()); - weight = weight.saturating_add( - as OnRuntimeUpgrade>::on_runtime_upgrade(), - ); - weight = weight.saturating_add(::on_runtime_upgrade()); - - weight + <(COnRuntimeUpgrade, AllPallets) as OnRuntimeUpgrade>::on_runtime_upgrade() } /// Execute given block, but don't do any of the [`final_checks`]. @@ -256,19 +250,10 @@ where /// This should only be used for testing. #[cfg(feature = "try-runtime")] pub fn try_runtime_upgrade() -> Result { - < - (frame_system::Pallet::, COnRuntimeUpgrade, AllPallets) - as - OnRuntimeUpgrade - >::pre_upgrade().unwrap(); - + <(COnRuntimeUpgrade, AllPallets) as OnRuntimeUpgrade>::pre_upgrade().unwrap(); let weight = Self::execute_on_runtime_upgrade(); - < - (frame_system::Pallet::, COnRuntimeUpgrade, AllPallets) - as - OnRuntimeUpgrade - >::post_upgrade().unwrap(); + <(COnRuntimeUpgrade, AllPallets) as OnRuntimeUpgrade>::post_upgrade().unwrap(); Ok(weight) } @@ -306,9 +291,6 @@ where digest, frame_system::InitKind::Full, ); - weight = weight.saturating_add( as OnInitialize< - System::BlockNumber, - >>::on_initialize(*block_number)); weight = weight.saturating_add( >::on_initialize(*block_number), ); @@ -416,29 +398,19 @@ where fn idle_and_finalize_hook(block_number: NumberFor) { let weight = >::block_weight(); let max_weight = >::get().max_block; - let mut remaining_weight = max_weight.saturating_sub(weight.total()); + let remaining_weight = max_weight.saturating_sub(weight.total()); if remaining_weight > 0 { - let mut used_weight = - as OnIdle>::on_idle( - block_number, - remaining_weight, - ); - remaining_weight = remaining_weight.saturating_sub(used_weight); - used_weight = >::on_idle( + let used_weight = >::on_idle( block_number, remaining_weight, - ) - .saturating_add(used_weight); + ); >::register_extra_weight_unchecked( used_weight, DispatchClass::Mandatory, ); } - as OnFinalize>::on_finalize( - block_number, - ); >::on_finalize(block_number); } @@ -850,7 +822,7 @@ mod tests { Block, ChainContext, Runtime, - AllPallets, + AllPalletsWithSystem, CustomOnRuntimeUpgrade, >; @@ -1361,23 +1333,19 @@ mod tests { )); // All weights that show up in the `initialize_block_impl` - let frame_system_upgrade_weight = frame_system::Pallet::::on_runtime_upgrade(); let custom_runtime_upgrade_weight = CustomOnRuntimeUpgrade::on_runtime_upgrade(); - let runtime_upgrade_weight = ::on_runtime_upgrade(); - let frame_system_on_initialize_weight = - frame_system::Pallet::::on_initialize(block_number); + let runtime_upgrade_weight = + ::on_runtime_upgrade(); let on_initialize_weight = - >::on_initialize(block_number); + >::on_initialize(block_number); let base_block_weight = ::BlockWeights::get().base_block; // Weights are recorded correctly assert_eq!( frame_system::Pallet::::block_weight().total(), - frame_system_upgrade_weight + - custom_runtime_upgrade_weight + + custom_runtime_upgrade_weight + runtime_upgrade_weight + - frame_system_on_initialize_weight + on_initialize_weight + base_block_weight, ); }); diff --git a/frame/support/procedural/src/construct_runtime/mod.rs b/frame/support/procedural/src/construct_runtime/mod.rs index 382122697beff..3248eb1b250d7 100644 --- a/frame/support/procedural/src/construct_runtime/mod.rs +++ b/frame/support/procedural/src/construct_runtime/mod.rs @@ -208,7 +208,7 @@ fn decl_all_pallets<'a>( // Make nested tuple structure like: // `((FirstPallet, (SecondPallet, ( ... , LastPallet) ... ))))` // But ignore the system pallet. - let all_pallets = names + let all_pallets_without_system = names .iter() .filter(|n| **n != SYSTEM_PALLET_NAME) .rev() @@ -224,7 +224,7 @@ fn decl_all_pallets<'a>( // Make nested tuple structure like: // `((LastPallet, (SecondLastPallet, ( ... , FirstPallet) ... ))))` // But ignore the system pallet. - let all_pallets_reversed = names + let all_pallets_without_system_reversed = names .iter() .filter(|n| **n != SYSTEM_PALLET_NAME) .fold(TokenStream2::default(), |combined, name| quote!((#name, #combined))); @@ -238,32 +238,29 @@ fn decl_all_pallets<'a>( quote!( #types /// All pallets included in the runtime as a nested tuple of types. - /// Excludes the System pallet. - pub type AllPallets = ( #all_pallets ); + #[deprecated(note = "The type definition has changed from representing all pallets \ + excluding system, in reversed order to become the representation of all pallets \ + including system pallet in regular order. For this reason it is encouraged to use \ + explicitly one of `AllPalletsWithSystem`, `AllPalletsWithoutSystem`, \ + `AllPalletsWithSystemReversed`, `AllPalletsWithoutSystemReversed`. \ + Note that the type `frame_executive::Executive` expects one of `AllPalletsWithSystem` \ + and `AllPalletsWithSystemReversed. More details in \ + https://github.com/paritytech/substrate/pull/10043")] + pub type AllPallets = AllPalletsWithSystem; /// All pallets included in the runtime as a nested tuple of types. pub type AllPalletsWithSystem = ( #all_pallets_with_system ); + /// All pallets included in the runtime as a nested tuple of types. + /// Excludes the System pallet. + pub type AllPalletsWithoutSystem = ( #all_pallets_without_system ); + /// All pallets included in the runtime as a nested tuple of types in reversed order. /// Excludes the System pallet. - #[deprecated(note = "use `AllPallets` instead")] - #[allow(dead_code)] - pub type AllPalletsReversed = ( #all_pallets_reversed ); + pub type AllPalletsWithoutSystemReversed = ( #all_pallets_without_system_reversed ); /// All pallets included in the runtime as a nested tuple of types in reversed order. - #[deprecated(note = "use `AllPalletsWithSystem` instead")] - #[allow(dead_code)] pub type AllPalletsWithSystemReversed = ( #all_pallets_with_system_reversed ); - - /// All modules included in the runtime as a nested tuple of types. - /// Excludes the System pallet. - #[deprecated(note = "use `AllPallets` instead")] - #[allow(dead_code)] - pub type AllModules = ( #all_pallets ); - /// All modules included in the runtime as a nested tuple of types. - #[deprecated(note = "use `AllPalletsWithSystem` instead")] - #[allow(dead_code)] - pub type AllModulesWithSystem = ( #all_pallets_with_system ); ) } diff --git a/frame/support/test/tests/pallet.rs b/frame/support/test/tests/pallet.rs index 7a6dc4737a0cd..19ef13bae41bf 100644 --- a/frame/support/test/tests/pallet.rs +++ b/frame/support/test/tests/pallet.rs @@ -952,10 +952,10 @@ fn pallet_hooks_expand() { TestExternalities::default().execute_with(|| { frame_system::Pallet::::set_block_number(1); - assert_eq!(AllPallets::on_initialize(1), 10); - AllPallets::on_finalize(1); + assert_eq!(AllPalletsWithSystem::on_initialize(1), 10); + AllPalletsWithSystem::on_finalize(1); - assert_eq!(AllPallets::on_runtime_upgrade(), 30); + assert_eq!(AllPalletsWithSystem::on_runtime_upgrade(), 30); assert_eq!( frame_system::Pallet::::events()[0].event, diff --git a/frame/support/test/tests/pallet_instance.rs b/frame/support/test/tests/pallet_instance.rs index b5ce0526e2b11..e0e0f985a971b 100644 --- a/frame/support/test/tests/pallet_instance.rs +++ b/frame/support/test/tests/pallet_instance.rs @@ -510,10 +510,10 @@ fn pallet_hooks_expand() { TestExternalities::default().execute_with(|| { frame_system::Pallet::::set_block_number(1); - assert_eq!(AllPallets::on_initialize(1), 21); - AllPallets::on_finalize(1); + assert_eq!(AllPalletsWithSystem::on_initialize(1), 21); + AllPalletsWithSystem::on_finalize(1); - assert_eq!(AllPallets::on_runtime_upgrade(), 61); + assert_eq!(AllPalletsWithSystem::on_runtime_upgrade(), 61); assert_eq!( frame_system::Pallet::::events()[0].event, From 2edfaf7bb746a3fbe57c0c4f1696c1c485065e0b Mon Sep 17 00:00:00 2001 From: thiolliere Date: Sat, 30 Oct 2021 15:05:27 +0200 Subject: [PATCH 07/12] fix tests --- frame/support/test/tests/pallet.rs | 12 ++++++------ frame/support/test/tests/pallet_instance.rs | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/frame/support/test/tests/pallet.rs b/frame/support/test/tests/pallet.rs index 19ef13bae41bf..061d27486efcd 100644 --- a/frame/support/test/tests/pallet.rs +++ b/frame/support/test/tests/pallet.rs @@ -952,10 +952,10 @@ fn pallet_hooks_expand() { TestExternalities::default().execute_with(|| { frame_system::Pallet::::set_block_number(1); - assert_eq!(AllPalletsWithSystem::on_initialize(1), 10); - AllPalletsWithSystem::on_finalize(1); + assert_eq!(AllPalletsWithoutSystem::on_initialize(1), 10); + AllPalletsWithoutSystem::on_finalize(1); - assert_eq!(AllPalletsWithSystem::on_runtime_upgrade(), 30); + assert_eq!(AllPalletsWithoutSystem::on_runtime_upgrade(), 30); assert_eq!( frame_system::Pallet::::events()[0].event, @@ -991,10 +991,10 @@ fn all_pallets_type_reversed_order_is_correct() { #[allow(deprecated)] { - assert_eq!(AllPalletsReversed::on_initialize(1), 10); - AllPalletsReversed::on_finalize(1); + assert_eq!(AllPalletsWithoutSystemReversed::on_initialize(1), 10); + AllPalletsWithoutSystemReversed::on_finalize(1); - assert_eq!(AllPalletsReversed::on_runtime_upgrade(), 30); + assert_eq!(AllPalletsWithoutSystemReversed::on_runtime_upgrade(), 30); } assert_eq!( diff --git a/frame/support/test/tests/pallet_instance.rs b/frame/support/test/tests/pallet_instance.rs index e0e0f985a971b..ea21b66018aca 100644 --- a/frame/support/test/tests/pallet_instance.rs +++ b/frame/support/test/tests/pallet_instance.rs @@ -510,10 +510,10 @@ fn pallet_hooks_expand() { TestExternalities::default().execute_with(|| { frame_system::Pallet::::set_block_number(1); - assert_eq!(AllPalletsWithSystem::on_initialize(1), 21); - AllPalletsWithSystem::on_finalize(1); + assert_eq!(AllPalletsWithoutSystem::on_initialize(1), 21); + AllPalletsWithoutSystem::on_finalize(1); - assert_eq!(AllPalletsWithSystem::on_runtime_upgrade(), 61); + assert_eq!(AllPalletsWithoutSystem::on_runtime_upgrade(), 61); assert_eq!( frame_system::Pallet::::events()[0].event, From b41e9359a4a156fdf42c91db031ef25456081278 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Sat, 30 Oct 2021 16:24:56 +0200 Subject: [PATCH 08/12] address feedback --- frame/executive/src/lib.rs | 62 ++++++++++++++++++++++++-------------- 1 file changed, 39 insertions(+), 23 deletions(-) diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index cb397b3037656..c5fc4d76730c7 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -59,7 +59,7 @@ //! # type Context = frame_system::ChainContext; //! # pub type Block = generic::Block; //! # pub type Balances = u64; -//! # pub type AllPallets = u64; +//! # pub type AllPalletsWithSystem = u64; //! # pub enum Runtime {}; //! # use sp_runtime::transaction_validity::{ //! # TransactionValidity, UnknownTransaction, TransactionSource, @@ -73,7 +73,7 @@ //! # } //! # } //! /// Executive: handles dispatch to the various modules. -//! pub type Executive = executive::Executive; +//! pub type Executive = executive::Executive; //! ``` //! //! ### Custom `OnRuntimeUpgrade` logic @@ -90,7 +90,7 @@ //! # type Context = frame_system::ChainContext; //! # pub type Block = generic::Block; //! # pub type Balances = u64; -//! # pub type AllPallets = u64; +//! # pub type AllPalletsWithSystem = u64; //! # pub enum Runtime {}; //! # use sp_runtime::transaction_validity::{ //! # TransactionValidity, UnknownTransaction, TransactionSource, @@ -111,7 +111,7 @@ //! } //! } //! -//! pub type Executive = executive::Executive; +//! pub type Executive = executive::Executive; //! ``` #![cfg_attr(not(feature = "std"), no_std)] @@ -148,12 +148,26 @@ pub type OriginOf = as Dispatchable>::Origin; /// - `Block`: The block type of the runtime /// - `Context`: The context that is used when checking an extrinsic. /// - `UnsignedValidator`: The unsigned transaction validator of the runtime. -/// - `AllPallets`: Tuple that contains all pallets including frame system pallet. Will be used to -/// call hooks e.g. `on_initialize`. +/// - `AllPalletsWithSystem`: Tuple that contains all pallets including frame system pallet. Will be +/// used to call hooks e.g. `on_initialize`. /// - `OnRuntimeUpgrade`: Custom logic that should be called after a runtime upgrade. Modules are -/// already called by `AllPallets`. It will be called before all modules will be called. -pub struct Executive( - PhantomData<(System, Block, Context, UnsignedValidator, AllPallets, OnRuntimeUpgrade)>, +/// already called by `AllPalletsWithSystem`. It will be called before all modules will be called. +pub struct Executive< + System, + Block, + Context, + UnsignedValidator, + AllPalletsWithSystem, + OnRuntimeUpgrade = (), +>( + PhantomData<( + System, + Block, + Context, + UnsignedValidator, + AllPalletsWithSystem, + OnRuntimeUpgrade, + )>, ); impl< @@ -161,14 +175,14 @@ impl< Block: traits::Block
, Context: Default, UnsignedValidator, - AllPallets: OnRuntimeUpgrade + AllPalletsWithSystem: OnRuntimeUpgrade + OnInitialize + OnIdle + OnFinalize + OffchainWorker, COnRuntimeUpgrade: OnRuntimeUpgrade, > ExecuteBlock - for Executive + for Executive where Block::Extrinsic: Checkable + Codec, CheckedOf: Applyable + GetDispatchInfo, @@ -183,7 +197,7 @@ where Block, Context, UnsignedValidator, - AllPallets, + AllPalletsWithSystem, COnRuntimeUpgrade, >::execute_block(block); } @@ -194,13 +208,13 @@ impl< Block: traits::Block
, Context: Default, UnsignedValidator, - AllPallets: OnRuntimeUpgrade + AllPalletsWithSystem: OnRuntimeUpgrade + OnInitialize + OnIdle + OnFinalize + OffchainWorker, COnRuntimeUpgrade: OnRuntimeUpgrade, - > Executive + > Executive where Block::Extrinsic: Checkable + Codec, CheckedOf: Applyable + GetDispatchInfo, @@ -211,7 +225,7 @@ where { /// Execute all `OnRuntimeUpgrade` of this runtime, and return the aggregate weight. pub fn execute_on_runtime_upgrade() -> frame_support::weights::Weight { - <(COnRuntimeUpgrade, AllPallets) as OnRuntimeUpgrade>::on_runtime_upgrade() + <(COnRuntimeUpgrade, AllPalletsWithSystem) as OnRuntimeUpgrade>::on_runtime_upgrade() } /// Execute given block, but don't do any of the [`final_checks`]. @@ -250,10 +264,10 @@ where /// This should only be used for testing. #[cfg(feature = "try-runtime")] pub fn try_runtime_upgrade() -> Result { - <(COnRuntimeUpgrade, AllPallets) as OnRuntimeUpgrade>::pre_upgrade().unwrap(); + <(COnRuntimeUpgrade, AllPalletsWithSystem) as OnRuntimeUpgrade>::pre_upgrade().unwrap(); let weight = Self::execute_on_runtime_upgrade(); - <(COnRuntimeUpgrade, AllPallets) as OnRuntimeUpgrade>::post_upgrade().unwrap(); + <(COnRuntimeUpgrade, AllPalletsWithSystem) as OnRuntimeUpgrade>::post_upgrade().unwrap(); Ok(weight) } @@ -291,9 +305,9 @@ where digest, frame_system::InitKind::Full, ); - weight = weight.saturating_add( - >::on_initialize(*block_number), - ); + weight = weight.saturating_add(>::on_initialize(*block_number)); weight = weight.saturating_add( >::get().base_block, ); @@ -401,7 +415,7 @@ where let remaining_weight = max_weight.saturating_sub(weight.total()); if remaining_weight > 0 { - let used_weight = >::on_idle( + let used_weight = >::on_idle( block_number, remaining_weight, ); @@ -411,7 +425,7 @@ where ); } - >::on_finalize(block_number); + >::on_finalize(block_number); } /// Apply extrinsic outside of the block execution function. @@ -540,7 +554,9 @@ where // as well. frame_system::BlockHash::::insert(header.number(), header.hash()); - >::offchain_worker(*header.number()) + >::offchain_worker( + *header.number(), + ) } } From 95a34608fa1aba581eac6daf141cd686e56b3f13 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Sat, 30 Oct 2021 16:59:59 +0200 Subject: [PATCH 09/12] add a type to ease non breaking implementation --- .../procedural/src/construct_runtime/mod.rs | 20 ++++++++++++++++++- frame/support/test/tests/pallet.rs | 9 +++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/frame/support/procedural/src/construct_runtime/mod.rs b/frame/support/procedural/src/construct_runtime/mod.rs index ed24bfc360483..a2e3d3f4e911c 100644 --- a/frame/support/procedural/src/construct_runtime/mod.rs +++ b/frame/support/procedural/src/construct_runtime/mod.rs @@ -235,6 +235,17 @@ fn decl_all_pallets<'a>( .iter() .fold(TokenStream2::default(), |combined, name| quote!((#name, #combined))); + let system_pallet = match names.iter().find(|n| **n == SYSTEM_PALLET_NAME) { + Some(name) => name, + None => + return syn::Error::new( + proc_macro2::Span::call_site(), + "`System` pallet declaration is missing. \ + Please add this line: `System: frame_system::{Pallet, Call, Storage, Config, Event},`", + ) + .into_compile_error(), + }; + quote!( #types @@ -245,7 +256,7 @@ fn decl_all_pallets<'a>( explicitly one of `AllPalletsWithSystem`, `AllPalletsWithoutSystem`, \ `AllPalletsWithSystemReversed`, `AllPalletsWithoutSystemReversed`. \ Note that the type `frame_executive::Executive` expects one of `AllPalletsWithSystem` \ - and `AllPalletsWithSystemReversed. More details in \ + , `AllPalletsWithSystemReversed`, `AllPalletsReversedWithSystemFirst`. More details in \ https://github.com/paritytech/substrate/pull/10043")] pub type AllPallets = AllPalletsWithSystem; @@ -262,6 +273,13 @@ fn decl_all_pallets<'a>( /// All pallets included in the runtime as a nested tuple of types in reversed order. pub type AllPalletsWithSystemReversed = ( #all_pallets_with_system_reversed ); + + /// All pallets included in the runtime as a nested tuple of types in reversed order. + /// With the system pallet first. + pub type AllPalletsReversedWithSystemFirst = ( + #system_pallet, + AllPalletsWithoutSystemReversed + ); ) } diff --git a/frame/support/test/tests/pallet.rs b/frame/support/test/tests/pallet.rs index 061d27486efcd..3dc823f085b9c 100644 --- a/frame/support/test/tests/pallet.rs +++ b/frame/support/test/tests/pallet.rs @@ -1540,3 +1540,12 @@ fn test_storage_info() { ], ); } + +#[test] +fn assert_type_is_correct() { + // Just ensure the 2 types are same. + fn _a(_t: AllPalletsReversedWithSystemFirst) {} + fn _b(t: (System, (Example2, (Example,)))) { + _a(t) + } +} From 221499b4bee7c0c01d7499ec03fc25a8b0ada738 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Wed, 17 Nov 2021 11:59:05 +0900 Subject: [PATCH 10/12] add comment about constraint --- bin/node/runtime/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index adb4e30555982..d12737e91ff69 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1253,6 +1253,8 @@ construct_runtime!( Utility: pallet_utility, Babe: pallet_babe, Timestamp: pallet_timestamp, + // Authorship must be before session in order to note author in the correct session and era + // for im-online and staking. Authorship: pallet_authorship, Indices: pallet_indices, Balances: pallet_balances, From 45981cccb40c6b61db2f0de92f42cfb39793a71f Mon Sep 17 00:00:00 2001 From: thiolliere Date: Wed, 17 Nov 2021 15:43:54 +0900 Subject: [PATCH 11/12] fix test --- frame/support/test/tests/pallet.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/support/test/tests/pallet.rs b/frame/support/test/tests/pallet.rs index 436da064eaa5c..4a69bc9d80342 100644 --- a/frame/support/test/tests/pallet.rs +++ b/frame/support/test/tests/pallet.rs @@ -1569,7 +1569,7 @@ fn test_storage_info() { fn assert_type_is_correct() { // Just ensure the 2 types are same. fn _a(_t: AllPalletsReversedWithSystemFirst) {} - fn _b(t: (System, (Example2, (Example,)))) { + fn _b(t: (System, (Example4, (Example2, (Example,))))) { _a(t) } } From 2180da699391960358275dfd454de71a0e342159 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Mon, 22 Nov 2021 13:21:10 +0900 Subject: [PATCH 12/12] add test for generated types --- frame/support/test/tests/pallet.rs | 38 +++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/frame/support/test/tests/pallet.rs b/frame/support/test/tests/pallet.rs index 4a69bc9d80342..509e3217ddf96 100644 --- a/frame/support/test/tests/pallet.rs +++ b/frame/support/test/tests/pallet.rs @@ -1566,10 +1566,46 @@ fn test_storage_info() { } #[test] -fn assert_type_is_correct() { +fn assert_type_all_pallets_reversed_with_system_first_is_correct() { // Just ensure the 2 types are same. fn _a(_t: AllPalletsReversedWithSystemFirst) {} fn _b(t: (System, (Example4, (Example2, (Example,))))) { _a(t) } } + +#[test] +fn assert_type_all_pallets_with_system_is_correct() { + // Just ensure the 2 types are same. + fn _a(_t: AllPalletsWithSystem) {} + fn _b(t: (System, (Example, (Example2, (Example4,))))) { + _a(t) + } +} + +#[test] +fn assert_type_all_pallets_without_system_is_correct() { + // Just ensure the 2 types are same. + fn _a(_t: AllPalletsWithoutSystem) {} + fn _b(t: (Example, (Example2, (Example4,)))) { + _a(t) + } +} + +#[test] +fn assert_type_all_pallets_with_system_reversed_is_correct() { + // Just ensure the 2 types are same. + fn _a(_t: AllPalletsWithSystemReversed) {} + fn _b(t: (Example4, (Example2, (Example, (System,))))) { + _a(t) + } +} + +#[test] +fn assert_type_all_pallets_without_system_reversed_is_correct() { + // Just ensure the 2 types are same. + fn _a(_t: AllPalletsWithoutSystemReversed) {} + fn _b(t: (Example4, (Example2, (Example,)))) { + _a(t) + } +}