Ensure correct variant count in `Runtime[Hold/Freeze]Reason` by kianenigma · Pull Request #1900 · paritytech/polkadot-sdk

kianenigma · 2023-10-17T07:21:54Z

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.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

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.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

franciscoaguirre · 2023-10-18T08:36:22Z

 	type ReserveIdentifier = TestId;
 	type WeightInfo = ();
 	type RuntimeHoldReason = TestId;
+	type RuntimeFreezeReason = RuntimeFreezeReason;


Shouldn't this be TestId?

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

actually probably not, it should be the default one.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

…k into kiz-variant-count

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

…k into kiz-variant-count

* tsv-disabling: (36 commits) Removed TODO from test-case for hard-coded delivery fee estimation (#2042) Expose collection attributes from `Inspect` trait (#1914) `polkadot-parachain-primitives` should not depend on `frame-support`. (#1897) [testnet] Align testnet system parachain runtimes using `RelayTreasuryLocation` and `SystemParachains` in the same way (#2023) Sort the benchmarks before listing them (#2026) publish pallet-root-testing (#2017) Contracts: Add benchmarks to include files (#2022) Small optimisation to `--profile dev` wasm builds (#1851) basic-authorship: Improve time recording and logging (#2010) Application Crypto and BEEFY Support for paired (ECDSA,BLS) crypto (#1815) [ci] Run check-rust-feature-propagation in pr and master (#2012) Improve features dev-ex (#1831) Remove obsolete comment. (#2008) Refactor candidates test in paras_inherent (#2004) PVF: Add worker check during tests and benches (#1771) Bump actions/setup-node from 3.8.1 to 4.0.0 (#1997) polkadot: enable tikv-jemallocator/unprefixed_malloc_on_supported_platforms (#2002) Make `IdentityInfo` generic in `pallet-identity` (#1661) Ensure correct variant count in `Runtime[Hold/Freeze]Reason` (#1900) `CheckWeight`: Add more logging (#1996) ...

closes #1882 ## Breaking Changes This PR introduces a new item to `pallet_balances::Config`: ```diff 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. --------- Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Polkadot-Forum · 2023-11-15T10:59:56Z

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

…ech#1900) closes paritytech#1882 ## Breaking Changes This PR introduces a new item to `pallet_balances::Config`: ```diff 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. --------- Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

wischli · 2024-05-15T15:46:37Z

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 · 2024-05-15T19:14:16Z

Yes that is the correct thing to do.

wischli · 2024-05-16T09:03:36Z

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 · 2024-05-16T09:33:51Z

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 · 2024-05-16T10:41:52Z

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 · 2024-05-16T12:59:38Z

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

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

Signed-off-by: Xavier Lau <xavier@inv.cafe>

* Setup deps * Remove Koi from account migration test * paritytech/polkadot-sdk#1495 * Bump * paritytech/polkadot-sdk#1524 * !! paritytech/polkadot-sdk#1363 * paritytech/polkadot-sdk#1492 * paritytech/polkadot-sdk#1911 * paritytech/polkadot-sdk#1900 Signed-off-by: Xavier Lau <xavier@inv.cafe> * paritytech/polkadot-sdk#1661 * paritytech/polkadot-sdk#2144 * paritytech/polkadot-sdk#2048 * paritytech/polkadot-sdk#1672 * paritytech/polkadot-sdk#2303 * paritytech/polkadot-sdk#1256 * Remove identity and vesting * Fixes * paritytech/polkadot-sdk#2657 * paritytech/polkadot-sdk#1313 * paritytech/polkadot-sdk#2331 * paritytech/polkadot-sdk#2409 part.1 * paritytech/polkadot-sdk#2767 * paritytech/polkadot-sdk#2521 Signed-off-by: Xavier Lau <xavier@inv.cafe> * paritytech/polkadot-sdk#1222 * paritytech/polkadot-sdk#1234 part.1 * Satisfy compiler * XCM V4 part.1 * paritytech/polkadot-sdk#1246 * Remove pallet-democracy part.1 * paritytech/polkadot-sdk#2142 * paritytech/polkadot-sdk#2428 * paritytech/polkadot-sdk#3228 * XCM V4 part.2 * Bump * Build all runtimes * Build node * Remove pallet-democracy Signed-off-by: Xavier Lau <xavier@inv.cafe> * Format * Fix pallet tests * Fix precompile tests * Format * Fixes * Async, remove council, common pallet config * Fix `ethtx-forward` test case (#1519) * Fix ethtx-forward tests * Format * Fix following the review * Fixes * Fixes * Use default impl * Benchmark helper * Bench part.1 * Bench part.2 * Bench part.3 * Fix all tests * Typo * Feat * Fix EVM tracing build * Reuse upstream `proof_size_base_cost()` (#1521) * Format issue * Fixes * Fix CI --------- Signed-off-by: Xavier Lau <xavier@inv.cafe> Co-authored-by: Bear Wang <boundless.forest@outlook.com>

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

706d5ac

kianenigma added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Oct 17, 2023

kianenigma requested review from a team October 17, 2023 07:21

liamaharon approved these changes Oct 17, 2023

View reviewed changes

ggwpez added 3 commits October 17, 2023 12:11

Fix balances mock

88a01c4

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Fix all other mocks

7fa85df

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

fmt

3a58cf2

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ggwpez reviewed Oct 17, 2023

View reviewed changes

Comment thread substrate/frame/support/src/traits/misc.rs Outdated

set MaxHolds to 5 for kitchensink

2cd54be

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ggwpez requested a review from a team October 17, 2023 10:21

ggwpez requested review from a team, acatangiu, andresilva, athei, cheme and jsidorenko as code owners October 17, 2023 10:21

gupnik approved these changes Oct 18, 2023

View reviewed changes

Comment thread substrate/frame/balances/src/lib.rs

franciscoaguirre reviewed Oct 18, 2023

View reviewed changes

kianenigma and others added 7 commits October 18, 2023 10:13

fix

67c624e

fix westend

50aa9e8

Fix more

0e483d5

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

fix more runtimes

4c5d0a7

more

ce34c6f

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Merge branch 'kiz-variant-count' of github.com:paritytech/polkadot-sd…

23055ac

…k into kiz-variant-count

Merge remote-tracking branch 'origin/master' into kiz-variant-count

0ce8932

paritytech-ci requested review from a team October 18, 2023 09:51

ggwpez and others added 2 commits October 18, 2023 12:16

Fix more

ae3fc72

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Merge branch 'kiz-variant-count' of github.com:paritytech/polkadot-sd…

d5d0baf

…k into kiz-variant-count

kianenigma enabled auto-merge (squash) October 24, 2023 09:29

kianenigma merged commit 35eb133 into master Oct 24, 2023

kianenigma deleted the kiz-variant-count branch October 24, 2023 10:01

svyatonik added a commit to svyatonik/runtimes that referenced this pull request Nov 3, 2023

adapt paritytech/polkadot-sdk#1900

a012d32

kiltbot mentioned this pull request Nov 7, 2023

[AUTOMATIC] Update Polkadot dependencies from 1.2.0 to 1.3.0 KILTprotocol/kilt-node#575

Closed

juangirini mentioned this pull request Nov 10, 2023

Ref docs for frame_currency paritytech/polkadot-sdk-docs#49

Open

ahmadkaouk mentioned this pull request Nov 16, 2023

Update polkadot-sdk from v.1.1.0 to v1.3.0 moonbeam-foundation/moonbeam#2565

Closed

dnjscksdn98 mentioned this pull request Jan 12, 2024

NODE-65, deps: polkadot sdk migration bifrost-platform/bifrost-node#36

Merged

13 tasks

nazar-pc mentioned this pull request Jan 18, 2024

Upgrade Substrate autonomys/subspace#2424

Merged

1 task

kianenigma mentioned this pull request Mar 25, 2024

Tokens in FRAME Docs #2802

Merged

6 tasks

Dinonard mentioned this pull request Mar 28, 2024

Feat/polkadot v1.3.0 uplift AstarNetwork/Astar#1207

Merged

bkchr pushed a commit that referenced this pull request Apr 10, 2024

Fix init-bridge (#1900)

e7debc4

ukint-vs mentioned this pull request Apr 25, 2024

feat: Update substrate to v1.3.0 gear-tech/gear#3921

Merged

This was referenced Jun 5, 2024

Update polkadot-sdk from v1.7.0 to v1.11.0 moondance-labs/tanssi#573

Closed

Update polkadot-sdk from v1.10.0 to v1.11.0 moondance-labs/tanssi#577

Closed

aurexav added a commit to darwinia-network/darwinia that referenced this pull request Jun 21, 2024

paritytech/polkadot-sdk#1900

ce3c174

Signed-off-by: Xavier Lau <xavier@inv.cafe>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Ensure correct variant count in `Runtime[Hold/Freeze]Reason`#1900
kianenigma merged 23 commits into
masterfrom
kiz-variant-count

kianenigma commented Oct 17, 2023 •

edited

Loading

Uh oh!

ggwpez left a comment

Uh oh!

Uh oh!

Uh oh!

franciscoaguirre Oct 18, 2023

Uh oh!

kianenigma Oct 18, 2023

Uh oh!

kianenigma Oct 18, 2023

Uh oh!

Polkadot-Forum commented Nov 15, 2023

Uh oh!

wischli commented May 15, 2024 •

edited

Loading

Uh oh!

bkchr commented May 15, 2024

Uh oh!

wischli commented May 16, 2024 •

edited

Loading

Uh oh!

bkontur commented May 16, 2024

Uh oh!

bkontur commented May 16, 2024 •

edited

Loading

Uh oh!

bkchr commented May 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

Conversation

kianenigma commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking Changes

Uh oh!

ggwpez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

franciscoaguirre Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

kianenigma Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

kianenigma Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

Polkadot-Forum commented Nov 15, 2023

Uh oh!

wischli commented May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkchr commented May 15, 2024

Uh oh!

wischli commented May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkontur commented May 16, 2024

Uh oh!

bkontur commented May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkchr commented May 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

kianenigma commented Oct 17, 2023 •

edited

Loading

wischli commented May 15, 2024 •

edited

Loading

wischli commented May 16, 2024 •

edited

Loading

bkontur commented May 16, 2024 •

edited

Loading