Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proper value for the HoldIdentifier::VARIANT_COUNT #2674

Closed
NingLin-P opened this issue Apr 8, 2024 · 9 comments · Fixed by #3013
Closed

Proper value for the HoldIdentifier::VARIANT_COUNT #2674

NingLin-P opened this issue Apr 8, 2024 · 9 comments · Fixed by #3013

Comments

@NingLin-P
Copy link
Member

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
Copy link
Member

nazar-pc commented Apr 8, 2024

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
Copy link
Member Author

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
Copy link
Member

nazar-pc commented Apr 8, 2024

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
Copy link
Member Author

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
Copy link
Member

nazar-pc commented Apr 8, 2024

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
Copy link
Member Author

Done paritytech/polkadot-sdk#4033

@nazar-pc
Copy link
Member

@NingLin-P what is the endgame solution here? Are you going to add a trait upstream or are we changing our code to move the need to store ID in hold identifiers on our end?

@NingLin-P
Copy link
Member Author

I have spent some time thinking about this issue. There are actually 2 separate issues here:

Are you going to add a trait upstream

  1. VariantCount vs InstanceCount

This is more about the semantic or readability of the code because all we want is just to change the name VariantCount to InstanceCount.

are we changing our code to move the need to store ID in hold identifiers on our end?

  1. The value of VariantCount::VARIANT_COUNT

This is what I'm more concerned about because this value controls how many holds an account can create and once the limit is met the operation (see the PR description) that requires creating a hold will fail, no matter what the value of VARIANT_COUNT is since we don't have a limit for these operations (e.g. nomination, withdraw).

Remove the ID from the identifier can fix the issue (and also remove the need to rename to InstanceCount) because there will be one hold shared by the same type of operation, e.g. one hold for all nominations of the same nominator on different operators, but we do need a separate storage to track the total stake nominated on a specific operator.

Another solution is to define a limit for the operation that requires creating a hold and setting the VARIANT_COUNT to the sum of the limits.

@nazar-pc
Copy link
Member

While not ideal, I think shared hold is what we are supposed to be doing by design

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants