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

Migrate PendingVerification in pallet_registrar from value to map #528

Merged
merged 6 commits into from
Apr 30, 2024
Merged

Migrate PendingVerification in pallet_registrar from value to map #528

merged 6 commits into from
Apr 30, 2024

Conversation

tmpolaczyk
Copy link
Contributor

This allows an unlimited number of para ids to be waiting for verification, by using an unbounded StorageMap<ParaId, ()> instead of a bounded StorageValue<Vec<ParaId>>.

Breaking change, includes a migration.

@tmpolaczyk tmpolaczyk added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Apr 24, 2024
@tmpolaczyk tmpolaczyk force-pushed the tomasz-registrar-migrate-pending-verification branch from 42e3a16 to 7fa3722 Compare April 24, 2024 16:31
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 24, 2024 •
edited
Loading

Coverage Report

(master)

@@                                Coverage Diff                                @@
##           master   tomasz-registrar-migrate-pending-verification      +/-   ##
=================================================================================
- Coverage   65.36%                                          65.25%   -0.11%     
  Files          68                                              68              
+ Lines       10001                                           10024      +23     
=================================================================================
+ Hits         6537                                            6541       +4     
+ Misses       3464                                            3483      +19
Files Changed Coverage
/pallets/registrar/src/lib.rs 82.24% (+0.13%) 🔼

Coverage generated Fri Apr 26 10:11:16 UTC 2024

girazoki
girazoki reviewed Apr 25, 2024
View reviewed changes
runtime/common/src/migrations.rs
@@ -377,6 +378,40 @@ where
}
}

pub struct RegistrarPendingVerificationValueToMap<T>(pub PhantomData<T>);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a test for the migration?

girazoki
girazoki reviewed Apr 25, 2024
View reviewed changes
Copy link
Collaborator

@girazoki girazoki left a comment

Choose a reason for hiding this comment

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

The PR looks good but I would prefer if the migration was tested. Also we can use the pre and post upgrade hooks for this

@tmpolaczyk
Copy link
Contributor Author

Also we can use the pre and post upgrade hooks for this

The problem is that this storage item can be empty if there are no pending chains, so I don't know what to test for. But I can add an integration test similar to test_migration_config_full_rotation_period to check that the key was calculated correctly.

@girazoki
Copy link
Collaborator

Also we can use the pre and post upgrade hooks for this

The problem is that this storage item can be empty if there are no pending chains, so I don't know what to test for. But I can add an integration test similar to test_migration_config_full_rotation_period to check that the key was calculated correctly.

So I think:

  • You manually insert in this storage item certain chains with the previous layout.
  • You run on_runtime_upgrade.
  • You check the new storage map is populated correctly

Yes similar to test_migration_config_full_rotation_period

@tmpolaczyk
Copy link
Contributor Author

Yes, that's already done and it works :)

@tmpolaczyk tmpolaczyk merged commit 757ee00 into master Apr 30, 2024
32 checks passed
@tmpolaczyk tmpolaczyk deleted the tomasz-registrar-migrate-pending-verification branch April 30, 2024 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@girazoki girazoki girazoki approved these changes

@nanocryk nanocryk nanocryk approved these changes

Assignees
No one assigned
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tmpolaczyk @girazoki @nanocryk