Ensure correct variant count in Runtime[Hold/Freeze]Reason

[kianenigma]

closes #1882

Breaking Changes

This PR introduces a new item to pallet_balances::Config:

trait Config {
++    type RuntimeFreezeReasons;
}

This value is only used to check it against type MaxFreeze. A similar check has been added for MaxHolds against RuntimeHoldReasons, which is already given to pallet_balances.

In all contexts, you should pass the real RuntimeFreezeReasons generated by construct_runtime to type RuntimeFreezeReasons. Passing () would also work, but it would imply that the runtime uses no freezes at all.

[ggwpez]

If we have many of these lines up to migrate to DefaultConfig, it might be best to wait for that. Thoughts?

I think we would wait forever... i did some replacements and it seems to compile. Will push in a minute.

[franciscoaguirre]

Shouldn't this be TestId?

[kianenigma]

probably me or oliver did too much copy pasta :D I will double check

[kianenigma]

actually probably not, it should be the default one.

[Polkadot-Forum]

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v1-3-0/4614/1

[wischli]

IIUC, there is no migration required for teams which introduce RuntimeFreezeReason and/or switch from the () hold reason to the macro generated one?

- type RuntimeHoldReason = ();
+ type RuntimeHoldReason = RuntimeHoldReason;

I would really appreciate a comment on this and apologize for the minor comment creep.

[bkchr]

Yes that is the correct thing to do.

[wischli]

Yes that is the correct thing to do.

Really appreciate the quick response. Does your statement include the "no migration required" part as well?

Let me elaborate: Say some pallet has used the () hold reason and now introduces some enum HoldEnum as reason. Then it appears to me that all accounts with reserved funds for the () reason need to be migrated to some HoldEnum variant, independent of the number of variants of HoldEnum (e.g. one vs multiple).

Same for the freezes.

[bkontur]

Yes that is the correct thing to do.

Really appreciate the quick response. Does your statement include the "no migration required" part as well?

Let me elaborate: Say some pallet has used the () hold reason and now introduces some enum HoldEnum as reason. Then it appears to me that all accounts with reserved funds for the () reason need to be migrated to some HoldEnum variant, independent of the number of variants of HoldEnum (e.g. one vs multiple).

Same for the freezes.

iirc, VariantCount impl for () returns 0 here: https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/support/src/traits/misc.rs#L46-L48, so with type RuntimeHoldReason = (); you shouldn't be able to store any holds: https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/balances/src/lib.rs#L476-L485. But this change was introduced here: #2657, so I am not sure what is your current polkadot-sdk version.

[bkontur]

Yes that is the correct thing to do.

Really appreciate the quick response. Does your statement include the "no migration required" part as well?
Let me elaborate: Say some pallet has used the () hold reason and now introduces some enum HoldEnum as reason. Then it appears to me that all accounts with reserved funds for the () reason need to be migrated to some HoldEnum variant, independent of the number of variants of HoldEnum (e.g. one vs multiple).
Same for the freezes.

iirc, VariantCount impl for () returns 0 here: https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/support/src/traits/misc.rs#L46-L48, so with type RuntimeHoldReason = (); you shouldn't be able to store any holds: https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/balances/src/lib.rs#L476-L485. But this change was introduced here: #2657, so I am not sure what is your current polkadot-sdk version.

@wischli I just wanted to say that this change shouldn't require migration. However, it could depend on the version of pallet-balances. If your version doesn't include #2657 and you have type MaxHolds = ConstU32<1>;, then you probably need to check the actual stored Holds, for example:

Based on that, you might need to perform some migrations, which should be pretty straightforward.

Also, we have check-migrations CI jobs for runtimes. These jobs check storage or other aspects before and after runtime upgrades. For example, in your case, if you stored holds for a () reason and then a new runtime upgrade changes the reason type from () to RuntimeHoldReason, then without any migration, these CI jobs should fail with an error like "undecodable keys" or something similar. This failure occurs because it won't be able to decode Holds with () to Holds with RuntimeHoldReason. Thus, we will know that we need to add a migration.

[bkchr]

Also, we have check-migrations CI jobs for runtimes.

This is basically just try-runtime with all the checks enabled.