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

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

I started this investigation/issue based on @liamaharon question
[here](#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](#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`
#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>

bridges/snowbridge/parachain/pallets/inbound-queue/src/mock.rs

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

bridges/snowbridge/parachain/pallets/system/src/mock.rs

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

cumulus/pallets/collator-selection/src/mock.rs

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

cumulus/pallets/xcmp-queue/src/mock.rs

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

cumulus/parachain-template/runtime/src/lib.rs

@@ -337,7 +337,6 @@ impl pallet_balances::Config for Runtime {
 	type RuntimeHoldReason = RuntimeHoldReason;
 	type RuntimeFreezeReason = RuntimeFreezeReason;
 	type FreezeIdentifier = ();
-	type MaxHolds = ConstU32<0>;
 	type MaxFreezes = ConstU32<0>;
 }
 

cumulus/parachains/common/src/impls.rs

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

cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs

@@ -241,10 +241,6 @@ impl pallet_balances::Config for Runtime {
 	type RuntimeHoldReason = RuntimeHoldReason;
 	type RuntimeFreezeReason = RuntimeFreezeReason;
 	type FreezeIdentifier = ();
-	// We allow each account to have holds on it from:
-	//   - `NftFractionalization`: 1
-	//   - `StateTrieMigration`: 1
-	type MaxHolds = ConstU32<2>;
 	type MaxFreezes = ConstU32<0>;
 }
 

cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_balances.rs

@@ -16,10 +16,10 @@
 
 //! Autogenerated weights for `pallet_balances`
 //!
-//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev
-//! DATE: 2024-01-20, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]`
+//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 32.0.0
+//! DATE: 2024-01-31, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]`
 //! WORST CASE MAP SIZE: `1000000`
-//! HOSTNAME: `runner-j8vvqcjr-project-674-concurrent-0`, CPU: `Intel(R) Xeon(R) CPU @ 2.60GHz`
+//! HOSTNAME: `runner-8idpd4bs-project-674-concurrent-0`, CPU: `Intel(R) Xeon(R) CPU @ 2.60GHz`
 //! WASM-EXECUTION: `Compiled`, CHAIN: `Some("asset-hub-rococo-dev")`, DB CACHE: 1024
 
 // Executed Command:
@@ -54,8 +54,8 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
 		//  Estimated: `3593`
-		// Minimum execution time: 45_402_000 picoseconds.
-		Weight::from_parts(46_086_000, 0)
+		// Minimum execution time: 42_706_000 picoseconds.
+		Weight::from_parts(43_378_000, 0)
 			.saturating_add(Weight::from_parts(0, 3593))
 			.saturating_add(T::DbWeight::get().reads(1))
 			.saturating_add(T::DbWeight::get().writes(1))
@@ -66,8 +66,8 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
 		//  Estimated: `3593`
-		// Minimum execution time: 35_707_000 picoseconds.
-		Weight::from_parts(36_107_000, 0)
+		// Minimum execution time: 33_090_000 picoseconds.
+		Weight::from_parts(33_703_000, 0)
 			.saturating_add(Weight::from_parts(0, 3593))
 			.saturating_add(T::DbWeight::get().reads(1))
 			.saturating_add(T::DbWeight::get().writes(1))
@@ -78,8 +78,8 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `103`
 		//  Estimated: `3593`
-		// Minimum execution time: 13_538_000 picoseconds.
-		Weight::from_parts(13_771_000, 0)
+		// Minimum execution time: 12_678_000 picoseconds.
+		Weight::from_parts(13_068_000, 0)
 			.saturating_add(Weight::from_parts(0, 3593))
 			.saturating_add(T::DbWeight::get().reads(1))
 			.saturating_add(T::DbWeight::get().writes(1))
@@ -90,8 +90,8 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `103`
 		//  Estimated: `3593`
-		// Minimum execution time: 18_488_000 picoseconds.
-		Weight::from_parts(19_136_000, 0)
+		// Minimum execution time: 17_336_000 picoseconds.
+		Weight::from_parts(17_824_000, 0)
 			.saturating_add(Weight::from_parts(0, 3593))
 			.saturating_add(T::DbWeight::get().reads(1))
 			.saturating_add(T::DbWeight::get().writes(1))
@@ -102,8 +102,8 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `103`
 		//  Estimated: `6196`
-		// Minimum execution time: 48_168_000 picoseconds.
-		Weight::from_parts(48_874_000, 0)
+		// Minimum execution time: 44_817_000 picoseconds.
+		Weight::from_parts(45_453_000, 0)
 			.saturating_add(Weight::from_parts(0, 6196))
 			.saturating_add(T::DbWeight::get().reads(2))
 			.saturating_add(T::DbWeight::get().writes(2))
@@ -114,8 +114,8 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
 		//  Estimated: `3593`
-		// Minimum execution time: 44_463_000 picoseconds.
-		Weight::from_parts(45_320_000, 0)
+		// Minimum execution time: 41_468_000 picoseconds.
+		Weight::from_parts(42_093_000, 0)
 			.saturating_add(Weight::from_parts(0, 3593))
 			.saturating_add(T::DbWeight::get().reads(1))
 			.saturating_add(T::DbWeight::get().writes(1))
@@ -126,8 +126,8 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `103`
 		//  Estimated: `3593`
-		// Minimum execution time: 16_227_000 picoseconds.
-		Weight::from_parts(16_549_000, 0)
+		// Minimum execution time: 15_344_000 picoseconds.
+		Weight::from_parts(15_878_000, 0)
 			.saturating_add(Weight::from_parts(0, 3593))
 			.saturating_add(T::DbWeight::get().reads(1))
 			.saturating_add(T::DbWeight::get().writes(1))
@@ -139,11 +139,11 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `0 + u * (136 ±0)`
 		//  Estimated: `990 + u * (2603 ±0)`
-		// Minimum execution time: 15_992_000 picoseconds.
-		Weight::from_parts(16_243_000, 0)
+		// Minimum execution time: 15_067_000 picoseconds.
+		Weight::from_parts(15_281_000, 0)
 			.saturating_add(Weight::from_parts(0, 990))
-			// Standard Error: 12_426
-			.saturating_add(Weight::from_parts(13_617_673, 0).saturating_mul(u.into()))
+			// Standard Error: 11_009
+			.saturating_add(Weight::from_parts(13_050_024, 0).saturating_mul(u.into()))
 			.saturating_add(T::DbWeight::get().reads((1_u64).saturating_mul(u.into())))
 			.saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(u.into())))
 			.saturating_add(Weight::from_parts(0, 2603).saturating_mul(u.into()))
@@ -154,8 +154,8 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
 		//  Estimated: `1501`
-		// Minimum execution time: 5_713_000 picoseconds.
-		Weight::from_parts(6_054_000, 0)
+		// Minimum execution time: 5_139_000 picoseconds.
+		Weight::from_parts(5_511_000, 0)
 			.saturating_add(Weight::from_parts(0, 1501))
 			.saturating_add(T::DbWeight::get().reads(1))
 	}

cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs

@@ -199,9 +199,6 @@ impl pallet_balances::Config for Runtime {
 	type RuntimeHoldReason = RuntimeHoldReason;
 	type RuntimeFreezeReason = RuntimeFreezeReason;
 	type FreezeIdentifier = ();
-	// We allow each account to have holds on it from:
-	//   - `NftFractionalization`: 1
-	type MaxHolds = ConstU32<1>;
 	type MaxFreezes = ConstU32<0>;
 }
 

cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_balances.rs

@@ -16,10 +16,10 @@
 
 //! Autogenerated weights for `pallet_balances`
 //!
-//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev
-//! DATE: 2024-01-20, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]`
+//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 32.0.0
+//! DATE: 2024-01-31, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]`
 //! WORST CASE MAP SIZE: `1000000`
-//! HOSTNAME: `runner-j8vvqcjr-project-674-concurrent-0`, CPU: `Intel(R) Xeon(R) CPU @ 2.60GHz`
+//! HOSTNAME: `runner-8idpd4bs-project-674-concurrent-0`, CPU: `Intel(R) Xeon(R) CPU @ 2.60GHz`
 //! WASM-EXECUTION: `Compiled`, CHAIN: `Some("asset-hub-westend-dev")`, DB CACHE: 1024
 
 // Executed Command:
@@ -54,8 +54,8 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
 		//  Estimated: `3593`
-		// Minimum execution time: 42_658_000 picoseconds.
-		Weight::from_parts(43_649_000, 0)
+		// Minimum execution time: 43_122_000 picoseconds.
+		Weight::from_parts(43_640_000, 0)
 			.saturating_add(Weight::from_parts(0, 3593))
 			.saturating_add(T::DbWeight::get().reads(1))
 			.saturating_add(T::DbWeight::get().writes(1))
@@ -66,8 +66,8 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
 		//  Estimated: `3593`
-		// Minimum execution time: 33_810_000 picoseconds.
-		Weight::from_parts(34_322_000, 0)
+		// Minimum execution time: 33_636_000 picoseconds.
+		Weight::from_parts(34_571_000, 0)
 			.saturating_add(Weight::from_parts(0, 3593))
 			.saturating_add(T::DbWeight::get().reads(1))
 			.saturating_add(T::DbWeight::get().writes(1))
@@ -78,8 +78,8 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `103`
 		//  Estimated: `3593`
-		// Minimum execution time: 11_825_000 picoseconds.
-		Weight::from_parts(12_258_000, 0)
+		// Minimum execution time: 12_101_000 picoseconds.
+		Weight::from_parts(12_511_000, 0)
 			.saturating_add(Weight::from_parts(0, 3593))
 			.saturating_add(T::DbWeight::get().reads(1))
 			.saturating_add(T::DbWeight::get().writes(1))
@@ -90,8 +90,8 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `103`
 		//  Estimated: `3593`
-		// Minimum execution time: 16_540_000 picoseconds.
-		Weight::from_parts(17_058_000, 0)
+		// Minimum execution time: 17_077_000 picoseconds.
+		Weight::from_parts(17_362_000, 0)
 			.saturating_add(Weight::from_parts(0, 3593))
 			.saturating_add(T::DbWeight::get().reads(1))
 			.saturating_add(T::DbWeight::get().writes(1))
@@ -102,8 +102,8 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `103`
 		//  Estimated: `6196`
-		// Minimum execution time: 45_138_000 picoseconds.
-		Weight::from_parts(45_481_000, 0)
+		// Minimum execution time: 44_352_000 picoseconds.
+		Weight::from_parts(45_045_000, 0)
 			.saturating_add(Weight::from_parts(0, 6196))
 			.saturating_add(T::DbWeight::get().reads(2))
 			.saturating_add(T::DbWeight::get().writes(2))
@@ -114,8 +114,8 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
 		//  Estimated: `3593`
-		// Minimum execution time: 42_147_000 picoseconds.
-		Weight::from_parts(43_120_000, 0)
+		// Minimum execution time: 41_836_000 picoseconds.
+		Weight::from_parts(43_201_000, 0)
 			.saturating_add(Weight::from_parts(0, 3593))
 			.saturating_add(T::DbWeight::get().reads(1))
 			.saturating_add(T::DbWeight::get().writes(1))
@@ -126,8 +126,8 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `103`
 		//  Estimated: `3593`
-		// Minimum execution time: 14_730_000 picoseconds.
-		Weight::from_parts(14_867_000, 0)
+		// Minimum execution time: 14_413_000 picoseconds.
+		Weight::from_parts(14_743_000, 0)
 			.saturating_add(Weight::from_parts(0, 3593))
 			.saturating_add(T::DbWeight::get().reads(1))
 			.saturating_add(T::DbWeight::get().writes(1))
@@ -139,11 +139,11 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `0 + u * (136 ±0)`
 		//  Estimated: `990 + u * (2603 ±0)`
-		// Minimum execution time: 14_425_000 picoseconds.
-		Weight::from_parts(14_590_000, 0)
+		// Minimum execution time: 14_542_000 picoseconds.
+		Weight::from_parts(14_731_000, 0)
 			.saturating_add(Weight::from_parts(0, 990))
-			// Standard Error: 12_643
-			.saturating_add(Weight::from_parts(13_203_227, 0).saturating_mul(u.into()))
+			// Standard Error: 11_213
+			.saturating_add(Weight::from_parts(13_160_721, 0).saturating_mul(u.into()))
 			.saturating_add(T::DbWeight::get().reads((1_u64).saturating_mul(u.into())))
 			.saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(u.into())))
 			.saturating_add(Weight::from_parts(0, 2603).saturating_mul(u.into()))
@@ -154,8 +154,8 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
 		//  Estimated: `1501`
-		// Minimum execution time: 5_397_000 picoseconds.
-		Weight::from_parts(5_689_000, 0)
+		// Minimum execution time: 5_208_000 picoseconds.
+		Weight::from_parts(5_619_000, 0)
 			.saturating_add(Weight::from_parts(0, 1501))
 			.saturating_add(T::DbWeight::get().reads(1))
 	}

cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs

@@ -307,7 +307,6 @@ impl pallet_balances::Config for Runtime {
 	type RuntimeHoldReason = RuntimeHoldReason;
 	type RuntimeFreezeReason = RuntimeFreezeReason;
 	type FreezeIdentifier = ();
-	type MaxHolds = ConstU32<0>;
 	type MaxFreezes = ConstU32<0>;
 }