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

fix: remove spec_version check for Centrifuge 1023 migrations #1584

Merged
merged 2 commits into from Oct 6, 2023

Conversation

wischli
Copy link
Contributor

@wischli wischli commented Oct 6, 2023

Description

Removes spec_version checks for Anemoy, USDC and EVM Precompile Accountcode migrations.

Apparently, we cannot rely on the result of try-runtime which succeeded for all these migrations. However, in practice neither Dev/Demo nor Catalyst ran the EVM Accountcode migrations (Anemoy & USDC migrations ran on Catalyst in a previous upgrade from 1020 to 1021). My understanding is that at the time of executing on_runtime_upgrade(), the spec_version has been bumped already and LastRuntimeUpgrade updated to the current spec_version. I could not find any way of reading the pre upgrade spec_version.

None of the migrations performs many reads or writes. Moreover, they were adjusted such that no written storage is mutated. Thus, they cannot do harm.

Checklist:

  • I have added Rust doc comments to structs, enums, traits and functions
  • I have made corresponding changes to the documentation
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works

@wischli wischli added I9-release A specific release. D8-migration Pull request touches storage and needs migration code. D0-ready Pull request can be merged without special precaution and notification. labels Oct 6, 2023
@wischli wischli self-assigned this Oct 6, 2023
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

Not an expert in this topic, but changes look good!

@@ -1,6 +1,6 @@
[package]
name = "centrifuge-runtime"
version = "0.10.22"
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand this has not been changed for altair because we are using development as demo now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The correct reason would be that we had not upgraded Algol or Altair yet. In contrast, Catalyst received two upgrades already from 1020 (current Centrifuge version) to 1022.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for extend!

@wischli wischli enabled auto-merge (squash) October 6, 2023 11:17
Copy link
Contributor

@cdamian cdamian left a comment

Choose a reason for hiding this comment

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

Thank you @wischli!

@wischli wischli merged commit 160318c into main Oct 6, 2023
10 checks passed
@NunoAlexandre NunoAlexandre deleted the fix/migrations branch October 9, 2023 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D0-ready Pull request can be merged without special precaution and notification. D8-migration Pull request touches storage and needs migration code. I9-release A specific release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants