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

[AHM] Stage management #584

Open
wants to merge 8 commits into
base: dev-asset-hub-migration
Choose a base branch
from

Conversation

muharem
Copy link
Contributor

@muharem muharem commented Feb 9, 2025

meant to be merged into ahm dev branch

Stage management for the Asset Hub migration

@muharem muharem marked this pull request as ready for review February 10, 2025 04:35
@@ -283,6 +291,10 @@ pub mod pallet {
{
/// The overarching event type.
type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;
/// The origin that can perform permissioned operations like setting the migration stage.
///
/// This is generally root and Fellows origins.
Copy link
Member

Choose a reason for hiding this comment

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

We could try to tie this in with a point on this plan here: https://docs.google.com/document/d/1urpWdOkvQuhROeXf3FyjkdOcLiC36NpfKVADmig0lQI/edit?tab=t.0#heading=h.nc2w1yrxt8yf
Not sure if Donal sent it to you yet

Copy link
Member

Choose a reason for hiding this comment

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

PS: comment should include Asset Hub as origin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could try to tie this in with a point on this plan here: https://docs.google.com/document/d/1urpWdOkvQuhROeXf3FyjkdOcLiC36NpfKVADmig0lQI/edit?tab=t.0#heading=h.nc2w1yrxt8yf Not sure if Donal sent it to you yet

It opens a doc for me at the first line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the doc

block_number: BlockNumber,
},
/// The migration is initializing.
Initializing,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Initializing,
WaitingForAhReadiness,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

may be I better update the doc? It will be more general and if needed we can include more into it.

Copy link
Member

Choose a reason for hiding this comment

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

Yea, it is not really clear what is initalized and when that stage will be done.

#[pallet::call_index(2)]
pub fn start_data_migration(origin: OriginFor<T>) -> DispatchResult {
<T as Config>::ManagerOrigin::ensure_origin(origin)?;
Self::transition(MigrationStage::AccountsMigrationInit);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe defensive assert to check that we are in the right phase.

@@ -1120,6 +1120,10 @@ impl pallet_claims::Config for Runtime {

impl pallet_ah_migrator::Config for Runtime {
type RuntimeEvent = RuntimeEvent;
type ManagerOrigin = EitherOfDiverse<
EnsureRoot<AccountId>,
Copy link
Member

Choose a reason for hiding this comment

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

The relay chain will come in as root, right? So we dont need to use this EnsureXcm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right

Comment on lines +125 to 127
/// The migration has not yet started but will start in the future.
#[default]
Pending,
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure to change code that was relying on old meaning of Pending (at the very least self.is_ongoing())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants