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.

Sign up for GitHub

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

Jump to bottom

migrations: state difference guard and recorder #4204

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

migrations: state difference guard and recorder #4204

wants to merge 6 commits into from

Conversation

dastansam
Copy link
Contributor

@dastansam dastansam commented Apr 18, 2024 •
edited
Loading

Motivation

Adds a guard that prevents unexpected storage changes. StateDiffGuard:

  • initializes map of prefixes to the mutation policy (can/not change)
  • upon initialization takes the snapshot of the current state
  • when dropped, iterates every storage key, gets its mutation policy, defaulting to AnythingElse if present.
  • applies the mutation policy, if present

part of #240

polkadot address: 16FqwPZ8GRC5U5D4Fu7W33nA55ZXzXGWHwmbnE1eT6pxuqcT

@dastansam dastansam changed the title add StateDiffGuard initial impl migrations: State difference guard and recorder Apr 18, 2024
@dastansam dastansam changed the title migrations: State difference guard and recorder migrations: state difference guard and recorder Apr 19, 2024
ggwpez
ggwpez reviewed Apr 19, 2024
View reviewed changes
substrate/frame/support/src/storage/state_diff_guard.rs Outdated
#[derive(Default, Debug)]
pub struct StateDiffGuard {
// Storage prefixes that are expected to change.
whitelisted_prefixes: BTreeSet<StoragePrefix>,
Copy link
Member

@ggwpez ggwpez Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
whitelisted_prefixes: BTreeSet<StoragePrefix>,
must_change_prefixes: BTreeSet<StoragePrefix>,

The name here is not really expressing that they must change. It is also possible to make this more abstract by having a Map<Prefix, Change> where Change can be { Can | Must } x { Change | NotChange } (PS: not sure if these enum values are correct - hopefully you get the idea), but i think its fine for the beginning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made some improvements here, let me know if it's in the right direction, thanks)

@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/6079245

@dastansam dastansam requested a review from ggwpez April 29, 2024 00:52
@dastansam dastansam marked this pull request as ready for review April 29, 2024 00:52
@dastansam dastansam requested a review from a team as a code owner April 29, 2024 00:52
@kianenigma
Copy link
Contributor

This looks like a very good PR. @dastansam sorry for the delay, are you still around for finishing? @ggwpez @liamaharon and I can provide reviews.

@kianenigma
Copy link
Contributor

What I see missing is integrating this into polkadot_sdk_docs::reference_docs::migrations. It could be as simple as a basic mention.

@dastansam
Copy link
Contributor Author

This looks like a very good PR. @dastansam sorry for the delay, are you still around for finishing? @ggwpez @liamaharon and I can provide reviews.

thanks, for sure. Looking forward for your reviews)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ggwpez ggwpez Awaiting requested review from ggwpez

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dastansam @paritytech-cicd-pr @kianenigma @ggwpez