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

migrations: prevent accidentally using unversioned migrations instead of VersionedMigration #3835

Merged

Conversation

dastansam
Copy link
Contributor

@dastansam dastansam commented Mar 25, 2024

closes #1324

Problem

Currently, it is possible to accidentally use inner unversioned migration instead of VersionedMigration since both implement OnRuntimeUpgrade.

Solution

With this change, we make it clear that value of Inner is not intended to be used directly. It is achieved by bounding Inner to new trait UncheckedOnRuntimeUpgrade, which has the same interface (except unchecked_ prefix) as OnRuntimeUpgrade.

try-runtime functions

Since developers can implement try-runtime for Inner value in VersionedMigration and have custom logic for it, I added the same try-runtime functions to UncheckedOnRuntimeUpgrade. I looked for a ways to not duplicate functions, but couldn't find anything that doesn't significantly change the codebase. So I would appreciate If you have any suggestions to improve this

cc @liamaharon @xlc

polkadot address: 16FqwPZ8GRC5U5D4Fu7W33nA55ZXzXGWHwmbnE1eT6pxuqcT

@dastansam dastansam changed the title migrations: prevent accidentally using unversioned migrations instead of VersionedMigration migrations: prevent accidentally using unversioned migrations instead of VersionedMigration Mar 25, 2024
@dastansam
Copy link
Contributor Author

dastansam commented Mar 26, 2024

check-runtime-migration-westend is failing but it seems it's been failing for several runs and error message didn't change.
https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5651070

@dastansam dastansam marked this pull request as ready for review March 26, 2024 00:21
@dastansam dastansam requested review from andresilva and a team as code owners March 26, 2024 00:21
@liamaharon liamaharon self-requested a review March 27, 2024 08:37
@dastansam dastansam requested a review from xlc March 27, 2024 14:07
@liamaharon liamaharon added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Mar 29, 2024
Copy link
Contributor

@liamaharon liamaharon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks very much for this! Just some minor comments.

substrate/frame/support/src/migrations.rs Outdated Show resolved Hide resolved
substrate/frame/support/src/migrations.rs Outdated Show resolved Hide resolved
substrate/frame/support/src/traits/hooks.rs Outdated Show resolved Hide resolved
substrate/frame/support/src/traits/hooks.rs Outdated Show resolved Hide resolved
prdoc/pr_3835.prdoc Show resolved Hide resolved
prdoc/pr_3835.prdoc Show resolved Hide resolved
@paritytech-review-bot paritytech-review-bot bot requested a review from a team March 29, 2024 09:20
…dastansam/polkadot-sdk into feat/unchecked-on-runtime-upgrade
@liamaharon
Copy link
Contributor

/tip medium

Copy link

@liamaharon A referendum for a medium (80 DOT) tip was successfully submitted for @dastansam (16FqwPZ8GRC5U5D4Fu7W33nA55ZXzXGWHwmbnE1eT6pxuqcT on polkadot).

Referendum number: 638.
tip

Copy link

The referendum has appeared on Polkassembly.

Copy link
Contributor

@liamaharon liamaharon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah actually could you please make one more change, to the single-block-migration example?

We no longer need to wrap the Migration in a mod version_unchecked.

auto-merge was automatically disabled April 2, 2024 11:47

Head branch was pushed to by a user without write access

@paritytech-review-bot paritytech-review-bot bot requested a review from a team April 2, 2024 11:48
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5730616

@ggwpez
Copy link
Member

ggwpez commented Apr 2, 2024

I think the normal OnRuntimeUpgrade could just be a marker trait now.

@liamaharon liamaharon added this pull request to the merge queue Apr 2, 2024
Merged via the queue into paritytech:master with commit e542796 Apr 2, 2024
129 of 134 checks passed
Ank4n pushed a commit that referenced this pull request Apr 9, 2024
… of `VersionedMigration` (#3835)

closes #1324 

#### Problem
Currently, it is possible to accidentally use inner unversioned
migration instead of `VersionedMigration` since both implement
`OnRuntimeUpgrade`.

#### Solution

With this change, we make it clear that value of `Inner` is not intended
to be used directly. It is achieved by bounding `Inner` to new trait
`UncheckedOnRuntimeUpgrade`, which has the same interface (except
`unchecked_` prefix) as `OnRuntimeUpgrade`.

#### `try-runtime` functions

Since developers can implement `try-runtime` for `Inner` value in
`VersionedMigration` and have custom logic for it, I added the same
`try-runtime` functions to `UncheckedOnRuntimeUpgrade`. I looked for a
ways to not duplicate functions, but couldn't find anything that doesn't
significantly change the codebase. So I would appreciate If you have any
suggestions to improve this

cc @liamaharon @xlc 

polkadot address: 16FqwPZ8GRC5U5D4Fu7W33nA55ZXzXGWHwmbnE1eT6pxuqcT

---------

Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this pull request Apr 9, 2024
… of `VersionedMigration` (paritytech#3835)

closes paritytech#1324 

#### Problem
Currently, it is possible to accidentally use inner unversioned
migration instead of `VersionedMigration` since both implement
`OnRuntimeUpgrade`.

#### Solution

With this change, we make it clear that value of `Inner` is not intended
to be used directly. It is achieved by bounding `Inner` to new trait
`UncheckedOnRuntimeUpgrade`, which has the same interface (except
`unchecked_` prefix) as `OnRuntimeUpgrade`.

#### `try-runtime` functions

Since developers can implement `try-runtime` for `Inner` value in
`VersionedMigration` and have custom logic for it, I added the same
`try-runtime` functions to `UncheckedOnRuntimeUpgrade`. I looked for a
ways to not duplicate functions, but couldn't find anything that doesn't
significantly change the codebase. So I would appreciate If you have any
suggestions to improve this

cc @liamaharon @xlc 

polkadot address: 16FqwPZ8GRC5U5D4Fu7W33nA55ZXzXGWHwmbnE1eT6pxuqcT

---------

Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update VersionedMigration to ensure it is not possible to use the unchecked migration
6 participants