Proper value for the HoldIdentifier::VARIANT_COUNT

[NingLin-P]

HoldIdentifier::VARIANT_COUNT is used as the max number of holds an account can create, currently the following operation will create a hold on the caller account:

• Staking
  • Nominate operator, a hold for the staking fund
  • Withdraw staking, a hold for the withdrawal storage fund
• Open XDM channel, a hold for ChannelReserveFee
• Instantiate domain, a hole for DomainInstantiationDeposit

which means HoldIdentifier::VARIANT_COUNT serves as a limit of the combination number of the above operation. After the limit is met any of the above operations will fail (i.e. an account can nominate but later fail to withdraw due to this limit).

Ideally, we should define limits for each of the above operations separately and set HoldIdentifier::VARIANT_COUNT to Staking * 2 + OpenXDMChannel + InstantiateDomain.

cc @jfrank-summit @dariolina

[nazar-pc]

You didn't describe the most crucial thing: HoldIdentifier::VARIANT_COUNT is a temporary hack in Substrate due to use of Stable Rust compiler, it is will go away eventually.

HoldIdentifier is an enum and the only reason we have access to that constant at all is that https://doc.rust-lang.org/std/mem/fn.variant_count.html isn't stable yet, but I did use mem::variant_count in our codebase, which broke during Substrate upgrades due to excessive number of holds that our implementation apparently creates.

[NingLin-P]

I don't think it is related to whether we are using HoldIdentifier::VARIANT_COUNT or mem::variant_count, because the variant count is static and small too, it is used as the limit of hold an account can create in pallet-balances, while our usage of hold is dynamic and depend on the number of the above operation, i.e. an account can nominate on 100 of operators and need to create 100 of hold while HoldIdentifier::VARIANT_COUNT/mem::variant_count just 2.

[nazar-pc]

I don't think it is related to whether we are using HoldIdentifier::VARIANT_COUNT or mem::variant_count, because the variant count is static and small too, it is used as the limit of hold an account can create in pallet-balances, while our usage of hold is dynamic and depend on the number of the above operation, i.e. an account can nominate on 100 of operators and need to create 100 of hold while HoldIdentifier::VARIANT_COUNT/mem::variant_count just 2.

I disagree here. There is a trait, it describes the intended contract. I can imagine how in future version of Substrate something will change and they will start relying on that constant for something that will break when contract is violated.

This is a technical debt and ticking bomb waiting to explode at some unknown point in future as far as I'm concerned.

[NingLin-P]

I think the source of conflict is paritytech/polkadot-sdk#2657, which replaces MaxHolds with HoldIdentifier::VARIANT_COUNT it works fine with hold enum that has static number of instance like:

pub enum HoldReason {
    SlashForContinueMigrate,
    SlashForMigrateCustomTop,
    SlashForMigrateCustomChild,
}

but it is not general enough to cover our usage where the number of the hold enum instance is unbounded due to the Id value:

pub enum HoldIdentifier {
    Staked(Id)
}

[nazar-pc]

Agree, can you create an issue about this upstream? Then update PR with TODO and link to that issue everywhere we violate defined contract.

[NingLin-P]

Done paritytech/polkadot-sdk#4033