Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Tweak to active total migrations #12832

Merged
merged 4 commits into from
Dec 3, 2022
Merged

Tweak to active total migrations #12832

merged 4 commits into from
Dec 3, 2022

Conversation

gavofyork
Copy link
Member

One extra migration, in case there are multiple accounts with inactive funds.

@gavofyork gavofyork added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Dec 3, 2022
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

I think the many variant is sufficient for all cases, as you can always provide a single element Vec.

Copy link
Contributor

@KiChjang KiChjang left a comment

Choose a reason for hiding this comment

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

A reminder that we need to add the MigrateToTrackInactive migration struct to the runtimes after this is done.

That does however bring up a question: which migration struct should each runtime use? MigrateToTrackInactive or MigrateManyToTrackInactive? As it is written, each runtime can only use one or the other, but not both at the same time.

@gavofyork
Copy link
Member Author

Right. We will only need the first one for all chains we use, however I cannot rule out the possibility that ecosystem chains have multiple accounts which contain inactive funds so the second variant is provided.

Of course the second is strictly more general, however using it would require additional verbosity since the Checking Account is always stored in a single already-exposed type param which fits neatly into the first variant and not the second. So it's provided in the interest of easing developability and reducing the chance of bugs creeping in from tedious boilerplate.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@gavofyork gavofyork merged commit 1a0af36 into master Dec 3, 2022
@gavofyork gavofyork deleted the gav-active-total-tweaks branch December 3, 2022 16:36
ltfschoen pushed a commit to ltfschoen/substrate that referenced this pull request Feb 22, 2023
* Tweak to active total migrations

* Formatting

* Expose trait

* Remove empty pre_ and post_upgrade hooks.

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

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Tweak to active total migrations

* Formatting

* Expose trait

* Remove empty pre_ and post_upgrade hooks.

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

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants