From 90e379ab1a4a99f8a3e61bc975731af9a95c4e0f Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Mon, 14 Aug 2023 03:53:54 -0700 Subject: [PATCH] add `frame_system::DefaultConfig` to individual pallet `DefaultConfigs` (#14453) * add frame_system::DefaultConfig to individual pallet DefaultConfigs * Fixes tests * Minor fix * ".git/.scripts/commands/fmt/fmt.sh" * Adds UI Tests --------- Co-authored-by: Nikhil Gupta <17176722+gupnik@users.noreply.github.com> Co-authored-by: command-bot <> --- Cargo.lock | 1 - frame/balances/src/lib.rs | 32 +++++++- frame/examples/default-config/src/lib.rs | 82 ++++++++++++------- frame/multisig/Cargo.toml | 1 - frame/multisig/src/tests.rs | 50 ++++------- frame/proxy/src/tests.rs | 46 +++-------- .../procedural/src/pallet/expand/config.rs | 47 ++++++----- .../procedural/src/pallet/parse/config.rs | 36 ++++++-- ...efault_config_with_no_default_in_system.rs | 15 ++++ ...lt_config_with_no_default_in_system.stderr | 5 ++ .../tests/pallet_ui/pass/default_config.rs | 15 ++++ frame/system/src/lib.rs | 4 +- 12 files changed, 201 insertions(+), 133 deletions(-) create mode 100644 frame/support/test/tests/pallet_ui/default_config_with_no_default_in_system.rs create mode 100644 frame/support/test/tests/pallet_ui/default_config_with_no_default_in_system.stderr create mode 100644 frame/support/test/tests/pallet_ui/pass/default_config.rs diff --git a/Cargo.lock b/Cargo.lock index b6c9dd138b6f6..b6717aa21abc9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6572,7 +6572,6 @@ dependencies = [ "pallet-balances", "parity-scale-codec", "scale-info", - "sp-core", "sp-io", "sp-runtime", "sp-std", diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index e77b0f5e5d956..90bee7d99e121 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -213,7 +213,33 @@ pub mod pallet { pub type CreditOf = Credit<::AccountId, Pallet>; - #[pallet::config] + /// Default implementations of [`DefaultConfig`], which can be used to implement [`Config`]. + pub mod config_preludes { + use super::*; + use frame_support::derive_impl; + + pub struct TestDefaultConfig; + + #[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)] + impl frame_system::DefaultConfig for TestDefaultConfig {} + + #[frame_support::register_default_impl(TestDefaultConfig)] + impl DefaultConfig for TestDefaultConfig { + type Balance = u64; + + type ReserveIdentifier = (); + type FreezeIdentifier = (); + + type MaxLocks = (); + type MaxReserves = (); + type MaxFreezes = (); + type MaxHolds = (); + + type WeightInfo = (); + } + } + + #[pallet::config(with_default)] pub trait Config: frame_system::Config { /// The overarching event type. type RuntimeEvent: From> @@ -236,6 +262,7 @@ pub mod pallet { + FixedPointOperand; /// Handler for the unbalanced reduction when removing a dust account. + #[pallet::no_default] type DustRemoval: OnUnbalanced>; /// The minimum amount required to keep an account open. MUST BE GREATER THAN ZERO! @@ -247,9 +274,11 @@ pub mod pallet { /// /// Bottom line: Do yourself a favour and make it at least one! #[pallet::constant] + #[pallet::no_default] type ExistentialDeposit: Get; /// The means of storing the balances of an account. + #[pallet::no_default] type AccountStore: StoredMap>; /// The ID type for reserves. @@ -258,6 +287,7 @@ pub mod pallet { type ReserveIdentifier: Parameter + Member + MaxEncodedLen + Ord + Copy; /// The overarching hold reason. + #[pallet::no_default] type RuntimeHoldReason: Parameter + Member + MaxEncodedLen + Ord + Copy; /// The ID type for freezes. diff --git a/frame/examples/default-config/src/lib.rs b/frame/examples/default-config/src/lib.rs index 63a6a941b829f..8715b8c45ff0e 100644 --- a/frame/examples/default-config/src/lib.rs +++ b/frame/examples/default-config/src/lib.rs @@ -55,11 +55,20 @@ pub mod pallet { /// in our tests below. type OverwrittenDefaultValue: Get; - /// An input parameter that relies on `::AccountId`. As of - /// now, such types cannot have defaults and need to be annotated as such, iff - /// `#[pallet::config(with_default)]` is enabled: + /// An input parameter that relies on `::AccountId`. This can + /// too have a default, as long as as it is present in `frame_system::DefaultConfig`. + type CanDeriveDefaultFromSystem: Get; + + /// We might chose to declare as one that doesn't have a default, for whatever semantical + /// reason. #[pallet::no_default] - type CannotHaveDefault: Get; + type HasNoDefault: Get; + + /// Some types can technically have no default, such as those the rely on + /// `frame_system::Config` but are not present in `frame_system::DefaultConfig`. For + /// example, a `RuntimeCall` cannot reasonably have a default. + #[pallet::no_default] // if we skip this, there will be a compiler error. + type CannotHaveDefault: Get; /// Something that is a normal type, with default. type WithDefaultType; @@ -73,24 +82,41 @@ pub mod pallet { pub mod config_preludes { // This will help use not need to disambiguate anything when using `derive_impl`. use super::*; + use frame_support::derive_impl; /// A type providing default configurations for this pallet in testing environment. pub struct TestDefaultConfig; + + #[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)] + impl frame_system::DefaultConfig for TestDefaultConfig {} + #[frame_support::register_default_impl(TestDefaultConfig)] impl DefaultConfig for TestDefaultConfig { type WithDefaultValue = frame_support::traits::ConstU32<42>; type OverwrittenDefaultValue = frame_support::traits::ConstU32<42>; + // `frame_system::config_preludes::TestDefaultConfig` declares account-id as u64. + type CanDeriveDefaultFromSystem = frame_support::traits::ConstU64<42>; + type WithDefaultType = u32; type OverwrittenDefaultType = u32; } - /// A type providing default configurations for this pallet in a parachain environment. - pub struct ParachainDefaultConfig; - #[frame_support::register_default_impl(ParachainDefaultConfig)] - impl DefaultConfig for ParachainDefaultConfig { + /// A type providing default configurations for this pallet in another environment. Examples + /// could be a parachain, or a solo-chain. + /// + /// Appropriate derive for `frame_system::DefaultConfig` needs to be provided. In this + /// example, we simple derive `frame_system::config_preludes::TestDefaultConfig` again. + pub struct OtherDefaultConfig; + + #[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)] + impl frame_system::DefaultConfig for OtherDefaultConfig {} + + #[frame_support::register_default_impl(OtherDefaultConfig)] + impl DefaultConfig for OtherDefaultConfig { type WithDefaultValue = frame_support::traits::ConstU32<66>; type OverwrittenDefaultValue = frame_support::traits::ConstU32<66>; + type CanDeriveDefaultFromSystem = frame_support::traits::ConstU64<42>; type WithDefaultType = u32; type OverwrittenDefaultType = u32; } @@ -106,28 +132,25 @@ pub mod pallet { #[cfg(any(test, doc))] pub mod tests { use super::*; - use frame_support::derive_impl; - use sp_runtime::traits::ConstU64; - - use super::pallet as pallet_default_config_example; + use frame_support::{derive_impl, parameter_types}; + use pallet::{self as pallet_default_config_example, config_preludes::*}; - type Block = frame_system::mocking::MockBlock; + type Block = frame_system::mocking::MockBlock; frame_support::construct_runtime!( - pub enum Test - { + pub struct Runtime { System: frame_system, DefaultPallet: pallet_default_config_example, } ); #[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)] - impl frame_system::Config for Test { + impl frame_system::Config for Runtime { // these items are defined by frame-system as `no_default`, so we must specify them here. // Note that these are types that actually rely on the outer runtime, and can't sensibly // have an _independent_ default. type Block = Block; - type BlockHashCount = ConstU64<10>; + type BlockHashCount = frame_support::traits::ConstU64<10>; type BaseCallFilter = frame_support::traits::Everything; type RuntimeOrigin = RuntimeOrigin; type RuntimeCall = RuntimeCall; @@ -139,9 +162,6 @@ pub mod tests { // type Nonce = u32; // type BlockNumber = u32; - // type Header = - // sp_runtime::generic::Header, - // Self::Hashing>; // type Hash = sp_core::hash::H256; // type Hashing = sp_runtime::traits::BlakeTwo256; // type AccountId = u64; @@ -162,15 +182,17 @@ pub mod tests { type SS58Prefix = frame_support::traits::ConstU16<456>; } - // Similarly, we use the defaults provided by own crate as well. - use pallet::config_preludes::*; + parameter_types! { + pub const SomeCall: RuntimeCall = RuntimeCall::System(frame_system::Call::::remark { remark: vec![] }); + } + #[derive_impl(TestDefaultConfig as pallet::DefaultConfig)] - impl crate::pallet::Config for Test { + impl pallet_default_config_example::Config for Runtime { // These two both cannot have defaults. type RuntimeEvent = RuntimeEvent; - // Note that the default account-id type in - // `frame_system::config_preludes::TestDefaultConfig` is `u64`. - type CannotHaveDefault = frame_support::traits::ConstU64<1>; + + type HasNoDefault = frame_support::traits::ConstU32<1>; + type CannotHaveDefault = SomeCall; type OverwrittenDefaultValue = frame_support::traits::ConstU32<678>; type OverwrittenDefaultType = u128; @@ -183,22 +205,22 @@ pub mod tests { // assert one of the value types that is not overwritten. assert_eq!( - <::WithDefaultValue as Get>::get(), + <::WithDefaultValue as Get>::get(), <::WithDefaultValue as Get>::get() ); // assert one of the value types that is overwritten. - assert_eq!(<::OverwrittenDefaultValue as Get>::get(), 678u32); + assert_eq!(<::OverwrittenDefaultValue as Get>::get(), 678u32); // assert one of the types that is not overwritten. assert_eq!( - std::any::TypeId::of::<::WithDefaultType>(), + std::any::TypeId::of::<::WithDefaultType>(), std::any::TypeId::of::<::WithDefaultType>() ); // assert one of the types that is overwritten. assert_eq!( - std::any::TypeId::of::<::OverwrittenDefaultType>(), + std::any::TypeId::of::<::OverwrittenDefaultType>(), std::any::TypeId::of::() ) } diff --git a/frame/multisig/Cargo.toml b/frame/multisig/Cargo.toml index d88cbb8b9fd90..68b5be6b49cec 100644 --- a/frame/multisig/Cargo.toml +++ b/frame/multisig/Cargo.toml @@ -27,7 +27,6 @@ log = { version = "0.4.17", default-features = false } [dev-dependencies] pallet-balances = { version = "4.0.0-dev", path = "../balances" } -sp-core = { version = "21.0.0", path = "../../primitives/core" } [features] default = ["std"] diff --git a/frame/multisig/src/tests.rs b/frame/multisig/src/tests.rs index 8e704b923742a..e7fc5b3e4aaea 100644 --- a/frame/multisig/src/tests.rs +++ b/frame/multisig/src/tests.rs @@ -23,16 +23,12 @@ use super::*; use crate as pallet_multisig; use frame_support::{ - assert_noop, assert_ok, + assert_noop, assert_ok, derive_impl, traits::{ConstU32, ConstU64, Contains}, }; -use sp_core::H256; -use sp_runtime::{ - traits::{BlakeTwo256, IdentityLookup}, - BuildStorage, TokenError, -}; +use sp_runtime::{BuildStorage, TokenError}; -type Block = frame_system::mocking::MockBlock; +type Block = frame_system::mocking::MockBlockU32; frame_support::construct_runtime!( pub enum Test @@ -43,46 +39,28 @@ frame_support::construct_runtime!( } ); +#[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)] impl frame_system::Config for Test { - type BaseCallFilter = TestBaseCallFilter; - type BlockWeights = (); - type BlockLength = (); - type DbWeight = (); + type Block = Block; + type BlockHashCount = ConstU32<250>; type RuntimeOrigin = RuntimeOrigin; - type Nonce = u64; - type Hash = H256; type RuntimeCall = RuntimeCall; - type Hashing = BlakeTwo256; - type AccountId = u64; - type Lookup = IdentityLookup; - type Block = Block; type RuntimeEvent = RuntimeEvent; - type BlockHashCount = ConstU64<250>; - type Version = (); + type BaseCallFilter = TestBaseCallFilter; type PalletInfo = PalletInfo; - type AccountData = pallet_balances::AccountData; - type OnNewAccount = (); - type OnKilledAccount = (); - type SystemWeightInfo = (); - type SS58Prefix = (); type OnSetCode = (); - type MaxConsumers = ConstU32<16>; + + type AccountData = pallet_balances::AccountData; } +#[derive_impl(pallet_balances::config_preludes::TestDefaultConfig as pallet_balances::DefaultConfig)] impl pallet_balances::Config for Test { - type MaxLocks = (); - type MaxReserves = (); - type ReserveIdentifier = [u8; 8]; - type Balance = u64; type RuntimeEvent = RuntimeEvent; + type RuntimeHoldReason = (); + type ReserveIdentifier = [u8; 8]; type DustRemoval = (); - type ExistentialDeposit = ConstU64<1>; type AccountStore = System; - type WeightInfo = (); - type FreezeIdentifier = (); - type MaxFreezes = (); - type RuntimeHoldReason = (); - type MaxHolds = (); + type ExistentialDeposit = ConstU64<1>; } pub struct TestBaseCallFilter; @@ -120,7 +98,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities { ext } -fn now() -> Timepoint { +fn now() -> Timepoint { Multisig::timepoint() } diff --git a/frame/proxy/src/tests.rs b/frame/proxy/src/tests.rs index 1f4d2617700e6..4b055a3935b95 100644 --- a/frame/proxy/src/tests.rs +++ b/frame/proxy/src/tests.rs @@ -24,16 +24,13 @@ use super::*; use crate as proxy; use codec::{Decode, Encode}; use frame_support::{ - assert_noop, assert_ok, + assert_noop, assert_ok, derive_impl, dispatch::DispatchError, traits::{ConstU32, ConstU64, Contains}, RuntimeDebug, }; use sp_core::H256; -use sp_runtime::{ - traits::{BlakeTwo256, IdentityLookup}, - BuildStorage, -}; +use sp_runtime::{traits::BlakeTwo256, BuildStorage}; type Block = frame_system::mocking::MockBlock; @@ -47,47 +44,30 @@ frame_support::construct_runtime!( } ); +#[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)] impl frame_system::Config for Test { - type BaseCallFilter = BaseFilter; - type BlockWeights = (); - type BlockLength = (); - type DbWeight = (); + type Block = Block; + type BlockHashCount = ConstU64<250>; type RuntimeOrigin = RuntimeOrigin; - type Nonce = u64; - type Hash = H256; type RuntimeCall = RuntimeCall; - type Hashing = BlakeTwo256; - type AccountId = u64; - type Lookup = IdentityLookup; - type Block = Block; type RuntimeEvent = RuntimeEvent; - type BlockHashCount = ConstU64<250>; - type Version = (); type PalletInfo = PalletInfo; - type AccountData = pallet_balances::AccountData; - type OnNewAccount = (); - type OnKilledAccount = (); - type SystemWeightInfo = (); - type SS58Prefix = (); type OnSetCode = (); - type MaxConsumers = ConstU32<16>; + + type BaseCallFilter = BaseFilter; + type AccountData = pallet_balances::AccountData; } +#[derive_impl(pallet_balances::config_preludes::TestDefaultConfig as pallet_balances::DefaultConfig)] impl pallet_balances::Config for Test { - type MaxLocks = (); - type MaxReserves = (); - type ReserveIdentifier = [u8; 8]; - type Balance = u64; type RuntimeEvent = RuntimeEvent; + type RuntimeHoldReason = (); + type ReserveIdentifier = [u8; 8]; type DustRemoval = (); - type ExistentialDeposit = ConstU64<1>; type AccountStore = System; - type WeightInfo = (); - type FreezeIdentifier = (); - type MaxFreezes = (); - type RuntimeHoldReason = (); - type MaxHolds = (); + type ExistentialDeposit = ConstU64<1>; } + impl pallet_utility::Config for Test { type RuntimeEvent = RuntimeEvent; type RuntimeCall = RuntimeCall; diff --git a/frame/support/procedural/src/pallet/expand/config.rs b/frame/support/procedural/src/pallet/expand/config.rs index 4ae0afdbbc8e3..c1b8eb022471f 100644 --- a/frame/support/procedural/src/pallet/expand/config.rs +++ b/frame/support/procedural/src/pallet/expand/config.rs @@ -49,24 +49,33 @@ Consequently, a runtime that wants to include this pallet must implement this tr ); // we only emit `DefaultConfig` if there are trait items, so an empty `DefaultConfig` is - // impossible consequently - if config.default_sub_trait.len() > 0 { - let trait_items = &config.default_sub_trait; - quote!( - /// Based on [`Config`]. Auto-generated by - /// [`#[pallet::config(with_default)]`](`frame_support::pallet_macros::config`). - /// Can be used in tandem with - /// [`#[register_default_config]`](`frame_support::register_default_config`) and - /// [`#[derive_impl]`](`frame_support::derive_impl`) to derive test config traits - /// based on existing pallet config traits in a safe and developer-friendly way. - /// - /// See [here](`frame_support::pallet_macros::config`) for more information and caveats about - /// the auto-generated `DefaultConfig` trait and how it is generated. - pub trait DefaultConfig { - #(#trait_items)* - } - ) - } else { - Default::default() + // impossible consequently. + match &config.default_sub_trait { + Some(default_sub_trait) if default_sub_trait.items.len() > 0 => { + let trait_items = &default_sub_trait.items; + + let type_param_bounds = if default_sub_trait.has_system { + let system = &def.frame_system; + quote::quote!(: #system::DefaultConfig) + } else { + quote::quote!() + }; + + quote!( + /// Based on [`Config`]. Auto-generated by + /// [`#[pallet::config(with_default)]`](`frame_support::pallet_macros::config`). + /// Can be used in tandem with + /// [`#[register_default_config]`](`frame_support::register_default_config`) and + /// [`#[derive_impl]`](`frame_support::derive_impl`) to derive test config traits + /// based on existing pallet config traits in a safe and developer-friendly way. + /// + /// See [here](`frame_support::pallet_macros::config`) for more information and caveats about + /// the auto-generated `DefaultConfig` trait and how it is generated. + pub trait DefaultConfig #type_param_bounds { + #(#trait_items)* + } + ) + }, + _ => Default::default(), } } diff --git a/frame/support/procedural/src/pallet/parse/config.rs b/frame/support/procedural/src/pallet/parse/config.rs index 6a3693a3ab0f2..c60bf1f3cbea7 100644 --- a/frame/support/procedural/src/pallet/parse/config.rs +++ b/frame/support/procedural/src/pallet/parse/config.rs @@ -37,6 +37,12 @@ mod keyword { syn::custom_keyword!(constant); } +#[derive(Default)] +pub struct DefaultTrait { + pub items: Vec, + pub has_system: bool, +} + /// Input definition for the pallet config. pub struct ConfigDef { /// The index of item in pallet module. @@ -58,8 +64,8 @@ pub struct ConfigDef { /// /// Contains default sub-trait items (instantiated by `#[pallet::config(with_default)]`). /// Vec will be empty if `#[pallet::config(with_default)]` is not specified or if there are - /// no trait items - pub default_sub_trait: Vec, + /// no trait items. + pub default_sub_trait: Option, } /// Input definition for a constant in pallet config. @@ -339,9 +345,21 @@ impl ConfigDef { false }; + let has_frame_system_supertrait = item.supertraits.iter().any(|s| { + syn::parse2::(s.to_token_stream()) + .map_or(false, |b| b.0 == *frame_system) + }); + let mut has_event_type = false; let mut consts_metadata = vec![]; - let mut default_sub_trait = vec![]; + let mut default_sub_trait = if enable_default { + Some(DefaultTrait { + items: Default::default(), + has_system: has_frame_system_supertrait, + }) + } else { + None + }; for trait_item in &mut item.items { let is_event = check_event_type(frame_system, trait_item, has_instance)?; has_event_type = has_event_type || is_event; @@ -382,13 +400,18 @@ impl ConfigDef { "Duplicate #[pallet::no_default] attribute not allowed.", )) } + already_no_default = true; }, } } if !already_no_default && !is_event && enable_default { - default_sub_trait.push(trait_item.clone()); + default_sub_trait + .as_mut() + .expect("is 'Some(_)' if 'enable_default'; qed") + .items + .push(trait_item.clone()); } } @@ -396,11 +419,6 @@ impl ConfigDef { helper::take_first_item_pallet_attr(&mut item.attrs)?; let disable_system_supertrait_check = attr.is_some(); - let has_frame_system_supertrait = item.supertraits.iter().any(|s| { - syn::parse2::(s.to_token_stream()) - .map_or(false, |b| b.0 == *frame_system) - }); - if !has_frame_system_supertrait && !disable_system_supertrait_check { let found = if item.supertraits.is_empty() { "none".to_string() diff --git a/frame/support/test/tests/pallet_ui/default_config_with_no_default_in_system.rs b/frame/support/test/tests/pallet_ui/default_config_with_no_default_in_system.rs new file mode 100644 index 0000000000000..7127ecf78594e --- /dev/null +++ b/frame/support/test/tests/pallet_ui/default_config_with_no_default_in_system.rs @@ -0,0 +1,15 @@ +#[frame_support::pallet] +mod pallet { + use frame_support::pallet_prelude::*; + + #[pallet::config(with_default)] + pub trait Config: frame_system::Config { + #[pallet::constant] + type MyGetParam2: Get; + } + + #[pallet::pallet] + pub struct Pallet(_); +} + +fn main() {} diff --git a/frame/support/test/tests/pallet_ui/default_config_with_no_default_in_system.stderr b/frame/support/test/tests/pallet_ui/default_config_with_no_default_in_system.stderr new file mode 100644 index 0000000000000..bc657f8f654ec --- /dev/null +++ b/frame/support/test/tests/pallet_ui/default_config_with_no_default_in_system.stderr @@ -0,0 +1,5 @@ +error[E0220]: associated type `RuntimeCall` not found for `Self` + --> tests/pallet_ui/default_config_with_no_default_in_system.rs:8:31 + | +8 | type MyGetParam2: Get; + | ^^^^^^^^^^^ associated type `RuntimeCall` not found diff --git a/frame/support/test/tests/pallet_ui/pass/default_config.rs b/frame/support/test/tests/pallet_ui/pass/default_config.rs new file mode 100644 index 0000000000000..9f90ae67d5779 --- /dev/null +++ b/frame/support/test/tests/pallet_ui/pass/default_config.rs @@ -0,0 +1,15 @@ +#[frame_support::pallet] +mod pallet { + use frame_support::pallet_prelude::*; + + #[pallet::config(with_default)] + pub trait Config: frame_system::Config { + #[pallet::constant] + type MyGetParam2: Get; + } + + #[pallet::pallet] + pub struct Pallet(_); +} + +fn main() {} diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index 98e9716958279..c5a36c5059e95 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -203,7 +203,7 @@ pub mod pallet { use crate::{self as frame_system, pallet_prelude::*, *}; use frame_support::pallet_prelude::*; - /// Contains default types suitable for various environments + /// Default implementations of [`DefaultConfig`], which can be used to implement [`Config`]. pub mod config_preludes { use super::DefaultConfig; @@ -342,8 +342,6 @@ pub mod pallet { /// /// Expects the `PalletInfo` type that is being generated by `construct_runtime!` in the /// runtime. - /// - /// For tests it is okay to use `()` as type, however it will provide "useless" data. #[pallet::no_default] type PalletInfo: PalletInfo;