[frame] #[pallet::composite_enum] improved variant count handling +…

… removed `pallet_balances`'s `MaxHolds` config (paritytech#2657)

I started this investigation/issue based on @liamaharon question
[here](paritytech#1801 (comment)).

## Problem

The `pallet_balances` integrity test should correctly detect that the
runtime has correct distinct `HoldReasons` variant count. I assume the
same situation exists for RuntimeFreezeReason.

It is not a critical problem, if we set `MaxHolds` with a sufficiently
large value, everything should be ok. However, in this case, the
integrity_test check becomes less useful.

**Situation for "any" runtime:**
- `HoldReason` enums from different pallets:
```rust
        /// from pallet_nis
        #[pallet::composite_enum]
	pub enum HoldReason {
		NftReceipt,
	}

        /// from pallet_preimage
        #[pallet::composite_enum]
	pub enum HoldReason {
		Preimage,
	}

        // from pallet_state-trie-migration
        #[pallet::composite_enum]
	pub enum HoldReason {
		SlashForContinueMigrate,
		SlashForMigrateCustomTop,
		SlashForMigrateCustomChild,
	}
```

- generated `RuntimeHoldReason` enum looks like:
```rust
pub enum RuntimeHoldReason {

    #[codec(index = 32u8)]
    Preimage(pallet_preimage::HoldReason),

    #[codec(index = 38u8)]
    Nis(pallet_nis::HoldReason),

    #[codec(index = 42u8)]
    StateTrieMigration(pallet_state_trie_migration::HoldReason),
}
```

- composite enum `RuntimeHoldReason` variant count is detected as `3`
- we set `type MaxHolds = ConstU32<3>`
- `pallet_balances::integrity_test` is ok with `3`(at least 3)

However, the real problem can occur in a live runtime where some
functionality might stop working. This is due to a total of 5 distinct
hold reasons (for pallets with multi-instance support, it is even more),
and not all of them can be used because of an incorrect `MaxHolds`,
which is deemed acceptable according to the `integrity_test`:
  ```
  // pseudo-code - if we try to call all of these:

T::Currency::hold(&pallet_nis::HoldReason::NftReceipt.into(),
&nft_owner, deposit)?;
T::Currency::hold(&pallet_preimage::HoldReason::Preimage.into(),
&nft_owner, deposit)?;

T::Currency::hold(&pallet_state_trie_migration::HoldReason::SlashForContinueMigrate.into(),
&nft_owner, deposit)?;

  // With `type MaxHolds = ConstU32<3>` these two will fail

T::Currency::hold(&pallet_state_trie_migration::HoldReason::SlashForMigrateCustomTop.into(),
&nft_owner, deposit)?;

T::Currency::hold(&pallet_state_trie_migration::HoldReason::SlashForMigrateCustomChild.into(),
&nft_owner, deposit)?;
  ```  


## Solutions

A macro `#[pallet::*]` expansion is extended of `VariantCount`
implementation for the `#[pallet::composite_enum]` enum type. This
expansion generates the `VariantCount` implementation for pallets'
`HoldReason`, `FreezeReason`, `LockId`, and `SlashReason`. Enum variants
must be plain enum values without fields to ensure a deterministic
count.

The composite runtime enum, `RuntimeHoldReason` and
`RuntimeFreezeReason`, now sets `VariantCount::VARIANT_COUNT` as the sum
of pallets' enum `VariantCount::VARIANT_COUNT`:
```rust
#[frame_support::pallet(dev_mode)]
mod module_single_instance {

	#[pallet::composite_enum]
	pub enum HoldReason {
		ModuleSingleInstanceReason1,
		ModuleSingleInstanceReason2,
	}
...
}

#[frame_support::pallet(dev_mode)]
mod module_multi_instance {

	#[pallet::composite_enum]
	pub enum HoldReason<I: 'static = ()> {
		ModuleMultiInstanceReason1,
		ModuleMultiInstanceReason2,
		ModuleMultiInstanceReason3,
	}
...
}


impl self::sp_api_hidden_includes_construct_runtime::hidden_include::traits::VariantCount
    for RuntimeHoldReason
{
    const VARIANT_COUNT: u32 = 0
        + module_single_instance::HoldReason::VARIANT_COUNT
        + module_multi_instance::HoldReason::<module_multi_instance::Instance1>::VARIANT_COUNT
        + module_multi_instance::HoldReason::<module_multi_instance::Instance2>::VARIANT_COUNT
        + module_multi_instance::HoldReason::<module_multi_instance::Instance3>::VARIANT_COUNT;
}
```

In addition, `MaxHolds` is removed (as suggested
[here](paritytech#2657 (comment)))
from `pallet_balances`, and its `Holds` are now bounded to
`RuntimeHoldReason::VARIANT_COUNT`. Therefore, there is no need to let
the runtime specify `MaxHolds`.


## For reviewers

Relevant changes can be found here:
- `substrate/frame/support/procedural/src/lib.rs` 
-  `substrate/frame/support/procedural/src/pallet/parse/composite.rs`
-  `substrate/frame/support/procedural/src/pallet/expand/composite.rs`
-
`substrate/frame/support/procedural/src/construct_runtime/expand/composite_helper.rs`
-
`substrate/frame/support/procedural/src/construct_runtime/expand/hold_reason.rs`
-
`substrate/frame/support/procedural/src/construct_runtime/expand/freeze_reason.rs`
- `substrate/frame/support/src/traits/misc.rs`

And the rest of the files is just about removed `MaxHolds` from
`pallet_balances`

## Next steps

Do the same for `MaxFreezes`
paritytech#2997.

---------

Co-authored-by: command-bot <>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Dónal Murray <donal.murray@parity.io>
Co-authored-by: gupnik <nikhilgupta.iitk@gmail.com>

substrate/bin/node-template/runtime/src/lib.rs

@@ -228,7 +228,6 @@ impl pallet_balances::Config for Runtime {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 parameter_types! {

substrate/bin/node/runtime/src/lib.rs

@@ -533,7 +533,6 @@ impl pallet_balances::Config for Runtime {
 	type WeightInfo = pallet_balances::weights::SubstrateWeight<Runtime>;
 	type FreezeIdentifier = RuntimeFreezeReason;
 	type MaxFreezes = ConstU32<1>;
-	type MaxHolds = ConstU32<7>;
 }
 
 parameter_types! {

substrate/frame/alliance/src/mock.rs

@@ -72,7 +72,6 @@ impl pallet_balances::Config for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 const MOTION_DURATION_IN_BLOCKS: BlockNumber = 3;

substrate/frame/asset-conversion/src/mock.rs

@@ -95,7 +95,6 @@ impl pallet_balances::Config for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 impl pallet_assets::Config<Instance1> for Test {

substrate/frame/asset-rate/src/mock.rs

@@ -79,7 +79,6 @@ impl pallet_balances::Config for Test {
 	type RuntimeHoldReason = RuntimeHoldReason;
 	type RuntimeFreezeReason = RuntimeFreezeReason;
 	type FreezeIdentifier = ();
-	type MaxHolds = ();
 	type MaxFreezes = ();
 }
 

substrate/frame/assets/src/mock.rs

@@ -86,7 +86,6 @@ impl pallet_balances::Config for Test {
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
 	type FreezeIdentifier = ();
-	type MaxHolds = ();
 	type MaxFreezes = ();
 }
 

substrate/frame/atomic-swap/src/tests.rs

@@ -82,7 +82,6 @@ impl pallet_balances::Config for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 impl Config for Test {

substrate/frame/babe/src/mock.rs

@@ -126,7 +126,6 @@ impl pallet_balances::Config for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 pallet_staking_reward_curve::build! {

substrate/frame/balances/src/lib.rs

@@ -206,7 +206,7 @@ pub mod pallet {
 	use super::*;
 	use frame_support::{
 		pallet_prelude::*,
-		traits::{fungible::Credit, tokens::Precision, VariantCount},
+		traits::{fungible::Credit, tokens::Precision, VariantCount, VariantCountOf},
 	};
 	use frame_system::pallet_prelude::*;
 
@@ -242,7 +242,6 @@ pub mod pallet {
 			type MaxLocks = ConstU32<100>;
 			type MaxReserves = ConstU32<100>;
 			type MaxFreezes = ConstU32<100>;
-			type MaxHolds = ConstU32<100>;
 
 			type WeightInfo = ();
 		}
@@ -316,10 +315,6 @@ pub mod pallet {
 		#[pallet::constant]
 		type MaxReserves: Get<u32>;
 
-		/// The maximum number of holds that can exist on an account at any time.
-		#[pallet::constant]
-		type MaxHolds: Get<u32>;
-
 		/// The maximum number of individual freeze locks that can exist on an account at any time.
 		#[pallet::constant]
 		type MaxFreezes: Get<u32>;
@@ -407,7 +402,7 @@ pub mod pallet {
 		DeadAccount,
 		/// Number of named reserves exceed `MaxReserves`.
 		TooManyReserves,
-		/// Number of holds exceed `MaxHolds`.
+		/// Number of holds exceed `VariantCountOf<T::RuntimeHoldReason>`.
 		TooManyHolds,
 		/// Number of freezes exceed `MaxFreezes`.
 		TooManyFreezes,
@@ -487,7 +482,10 @@ pub mod pallet {
 		_,
 		Blake2_128Concat,
 		T::AccountId,
-		BoundedVec<IdAmount<T::RuntimeHoldReason, T::Balance>, T::MaxHolds>,
+		BoundedVec<
+			IdAmount<T::RuntimeHoldReason, T::Balance>,
+			VariantCountOf<T::RuntimeHoldReason>,
+		>,
 		ValueQuery,
 	>;
 
@@ -556,13 +554,6 @@ pub mod pallet {
 				"The existential deposit must be greater than zero!"
 			);
 
-			assert!(
-				T::MaxHolds::get() >= <T::RuntimeHoldReason as VariantCount>::VARIANT_COUNT,
-				"MaxHolds should be greater than or equal to the number of hold reasons: {} < {}",
-				T::MaxHolds::get(),
-				<T::RuntimeHoldReason as VariantCount>::VARIANT_COUNT
-			);
-
 			assert!(
 				T::MaxFreezes::get() >= <T::RuntimeFreezeReason as VariantCount>::VARIANT_COUNT,
 				"MaxFreezes should be greater than or equal to the number of freeze reasons: {} < {}",

substrate/frame/balances/src/tests/mod.rs

@@ -142,7 +142,6 @@ impl Config for Test {
 	type RuntimeFreezeReason = RuntimeFreezeReason;
 	type FreezeIdentifier = TestId;
 	type MaxFreezes = ConstU32<2>;
-	type MaxHolds = ConstU32<3>;
 }
 
 #[derive(Clone)]

substrate/frame/beefy/src/mock.rs

@@ -133,7 +133,6 @@ impl pallet_balances::Config for Test {
 	type WeightInfo = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 	type FreezeIdentifier = ();
 	type MaxFreezes = ();
 }

substrate/frame/bounties/src/tests.rs

@@ -100,7 +100,6 @@ impl pallet_balances::Config for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 parameter_types! {
 	pub const ProposalBond: Permill = Permill::from_percent(5);

substrate/frame/child-bounties/src/tests.rs

@@ -103,7 +103,6 @@ impl pallet_balances::Config for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 parameter_types! {
 	pub const ProposalBond: Permill = Permill::from_percent(5);

substrate/frame/contracts/mock-network/src/parachain.rs

@@ -95,7 +95,6 @@ impl pallet_balances::Config for Runtime {
 	type ExistentialDeposit = ExistentialDeposit;
 	type FreezeIdentifier = ();
 	type MaxFreezes = ConstU32<0>;
-	type MaxHolds = ConstU32<1>;
 	type MaxLocks = MaxLocks;
 	type MaxReserves = MaxReserves;
 	type ReserveIdentifier = [u8; 8];

substrate/frame/contracts/mock-network/src/relay_chain.rs

@@ -93,7 +93,6 @@ impl pallet_balances::Config for Runtime {
 	type MaxReserves = MaxReserves;
 	type ReserveIdentifier = [u8; 8];
 	type FreezeIdentifier = ();
-	type MaxHolds = ConstU32<0>;
 	type MaxFreezes = ConstU32<0>;
 	type RuntimeHoldReason = RuntimeHoldReason;
 	type RuntimeFreezeReason = RuntimeFreezeReason;

substrate/frame/contracts/src/tests.rs

@@ -373,7 +373,6 @@ impl pallet_balances::Config for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = RuntimeHoldReason;
 	type RuntimeFreezeReason = RuntimeFreezeReason;
-	type MaxHolds = ConstU32<1>;
 }
 
 impl pallet_timestamp::Config for Test {

substrate/frame/conviction-voting/src/tests.rs

@@ -92,7 +92,6 @@ impl pallet_balances::Config for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 #[derive(Clone, PartialEq, Eq, Debug)]

substrate/frame/democracy/src/tests.rs

@@ -143,7 +143,6 @@ impl pallet_balances::Config for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 parameter_types! {
 	pub static PreimageByteDeposit: u64 = 0;

substrate/frame/election-provider-multi-phase/src/mock.rs

@@ -259,7 +259,6 @@ impl pallet_balances::Config for Runtime {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 #[derive(Default, Eq, PartialEq, Debug, Clone, Copy)]

substrate/frame/election-provider-multi-phase/test-staking-e2e/src/mock.rs

@@ -111,7 +111,6 @@ impl pallet_balances::Config for Runtime {
 	type DustRemoval = ();
 	type ExistentialDeposit = ExistentialDeposit;
 	type AccountStore = System;
-	type MaxHolds = ConstU32<1>;
 	type MaxFreezes = traits::ConstU32<1>;
 	type RuntimeHoldReason = RuntimeHoldReason;
 	type RuntimeFreezeReason = RuntimeFreezeReason;

substrate/frame/elections-phragmen/src/lib.rs

@@ -1362,7 +1362,6 @@ mod tests {
 		type MaxFreezes = ();
 		type RuntimeHoldReason = ();
 		type RuntimeFreezeReason = ();
-		type MaxHolds = ();
 	}
 
 	frame_support::parameter_types! {

substrate/frame/examples/basic/src/tests.rs

@@ -86,7 +86,6 @@ impl pallet_balances::Config for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 impl Config for Test {

substrate/frame/examples/dev-mode/src/tests.rs

@@ -80,7 +80,6 @@ impl pallet_balances::Config for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = RuntimeHoldReason;
 	type RuntimeFreezeReason = RuntimeFreezeReason;
-	type MaxHolds = ();
 }
 
 impl Config for Test {

substrate/frame/examples/kitchensink/src/tests.rs

@@ -57,7 +57,6 @@ impl pallet_balances::Config for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 parameter_types! {

substrate/frame/executive/src/lib.rs

@@ -959,7 +959,6 @@ mod tests {
 		type MaxFreezes = ConstU32<1>;
 		type RuntimeHoldReason = ();
 		type RuntimeFreezeReason = ();
-		type MaxHolds = ConstU32<1>;
 	}
 
 	parameter_types! {

substrate/frame/fast-unstake/src/mock.rs

@@ -77,7 +77,6 @@ impl pallet_balances::Config for Runtime {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 pallet_staking_reward_curve::build! {

substrate/frame/grandpa/src/mock.rs

@@ -143,7 +143,6 @@ impl pallet_balances::Config for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 impl pallet_timestamp::Config for Test {

substrate/frame/identity/src/tests.rs

@@ -94,7 +94,6 @@ impl pallet_balances::Config for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 parameter_types! {

substrate/frame/indices/src/mock.rs

@@ -79,7 +79,6 @@ impl pallet_balances::Config for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 impl Config for Test {

substrate/frame/lottery/src/mock.rs

@@ -88,7 +88,6 @@ impl pallet_balances::Config for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 parameter_types! {

substrate/frame/nft-fractionalization/src/mock.rs

@@ -89,7 +89,6 @@ impl pallet_balances::Config for Test {
 	type ReserveIdentifier = [u8; 8];
 	type RuntimeHoldReason = RuntimeHoldReason;
 	type RuntimeFreezeReason = RuntimeFreezeReason;
-	type MaxHolds = ConstU32<1>;
 	type FreezeIdentifier = ();
 	type MaxFreezes = ();
 }

substrate/frame/nfts/src/mock.rs

@@ -87,7 +87,6 @@ impl pallet_balances::Config for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 parameter_types! {

substrate/frame/nis/src/mock.rs

@@ -91,7 +91,6 @@ impl pallet_balances::Config<Instance1> for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = RuntimeHoldReason;
 	type RuntimeFreezeReason = RuntimeFreezeReason;
-	type MaxHolds = ConstU32<1>;
 }
 
 impl pallet_balances::Config<Instance2> for Test {
@@ -112,7 +111,6 @@ impl pallet_balances::Config<Instance2> for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 parameter_types! {

substrate/frame/nomination-pools/benchmarking/src/mock.rs

@@ -79,7 +79,6 @@ impl pallet_balances::Config for Runtime {
 	type MaxFreezes = ConstU32<1>;
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 pallet_staking_reward_curve::build! {

substrate/frame/nomination-pools/src/mock.rs

@@ -254,7 +254,6 @@ impl pallet_balances::Config for Runtime {
 	type MaxFreezes = ConstU32<1>;
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 pub struct BalanceToU256;

substrate/frame/nomination-pools/test-staking/src/mock.rs

@@ -90,7 +90,6 @@ impl pallet_balances::Config for Runtime {
 	type MaxFreezes = ConstU32<1>;
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 pallet_staking_reward_curve::build! {

substrate/frame/offences/benchmarking/src/mock.rs

@@ -81,7 +81,6 @@ impl pallet_balances::Config for Test {
 	type MaxFreezes = ();
 	type RuntimeHoldReason = ();
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ();
 }
 
 impl pallet_timestamp::Config for Test {

substrate/frame/preimage/src/mock.rs

@@ -21,7 +21,7 @@ use super::*;
 
 use crate as pallet_preimage;
 use frame_support::{
-	derive_impl, ord_parameter_types,
+	derive_impl, ord_parameter_types, parameter_types,
 	traits::{fungible::HoldConsideration, ConstU32, ConstU64, Everything},
 	weights::constants::RocksDbWeight,
 };
@@ -82,15 +82,18 @@ impl pallet_balances::Config for Test {
 	type ReserveIdentifier = [u8; 8];
 	type FreezeIdentifier = ();
 	type MaxFreezes = ConstU32<1>;
-	type RuntimeHoldReason = ();
+	type RuntimeHoldReason = RuntimeHoldReason;
 	type RuntimeFreezeReason = ();
-	type MaxHolds = ConstU32<2>;
 }
 
 ord_parameter_types! {
 	pub const One: u64 = 1;
 }
 
+parameter_types! {
+	pub const PreimageHoldReason: RuntimeHoldReason = RuntimeHoldReason::Preimage(pallet_preimage::HoldReason::Preimage);
+}
+
 pub struct ConvertDeposit;
 impl Convert<Footprint, u64> for ConvertDeposit {
 	fn convert(a: Footprint) -> u64 {
@@ -103,7 +106,7 @@ impl Config for Test {
 	type RuntimeEvent = RuntimeEvent;
 	type Currency = Balances;
 	type ManagerOrigin = EnsureSignedBy<One, u64>;
-	type Consideration = HoldConsideration<u64, Balances, (), ConvertDeposit>;
+	type Consideration = HoldConsideration<u64, Balances, PreimageHoldReason, ConvertDeposit>;
 }
 
 pub fn new_test_ext() -> sp_io::TestExternalities {