Skip to content
  • Branislav Kontur's avatar
    [frame] `#[pallet::composite_enum]` improved variant count handling + removed... · bb8ddc46
    Branislav Kontur authored
    [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](https://github.com/paritytech/polkadot-sdk/pull/1801#discussion_r1410452499).
    
    ## 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](https://github.com/paritytech/polkadot-sdk/pull/2657#discussion_r1443324573))
    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`
    https://github.com/paritytech/polkadot-sdk/issues/2997.
    
    ---------
    
    Co-authored-by: command-bot <>
    Co-authored-by: default avatarBastian Köcher <[email protected]>
    Co-authored-by: default avatarDónal Murray <[email protected]>
    Co-authored-by: default avatargupnik <[email protected]>
    bb8ddc46