[AHM] Stage management #584
Conversation
| @@ -172,6 +193,10 @@ pub mod pallet { | |||
| pub type RcAccounts<T: Config> = | |||
| StorageMap<_, Twox64Concat, T::AccountId, RcAccountFor<T>, OptionQuery>; | |||
|
|
|||
| /// The Asset Hub migration state. | |||
| #[pallet::storage] | |||
| pub type AhMigrationStage<T: Config> = StorageValue<_, MigrationStage, ValueQuery>; | |||
There was a problem hiding this comment.
Not sure if ValueQuery makes sense if our default is Pending which says that the migration will start in the next block.
There was a problem hiding this comment.
I changed the doc for Pending in both pallets (...will start in the future). In both pallets it's ValueQuery. I think its better than the alternative with OptionQuery and no Pending variants at all (otherwise its never reachable stage).
| &old, | ||
| &new | ||
| ); | ||
| Self::deposit_event(Event::StageTransition { old, new }); |
There was a problem hiding this comment.
Maybe this should send an UMP each time there is a stage transition? Then the relay can keep track of progress and we only have to send it from one spot in the code.
There was a problem hiding this comment.
But what we achieve by that? AH does not really know much about the stage (it does not know if all pallets data is sent).
I can see that we can have AH sending an XCM message after it processes every XCM message, but it might increase the migration time significantly.
Or we can let AH know that this message with the data is the last on this pallet, but AH still can get congested just on one pallet with a lot of data.
| #[derive(Encode, Decode)] | ||
| pub enum RcMigratorCall { | ||
| #[codec(index = 2)] | ||
| StartDataMigration, |
There was a problem hiding this comment.
I think this should maybe be something like AhStageChanged(stage) and then define the stage type either in the RC pallet or in a shared crate.
There was a problem hiding this comment.
yes, can be put this way. but if we have only few (e.g. three) stage notification from ah, a separate calls might be simpler
pallets/rc-migrator/src/lib.rs
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
PS: comment should include Asset Hub as origin.
There was a problem hiding this comment.
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.
| /// This is typically called by the Asset Hub to indicate it's readiness to receive the | ||
| /// migration data. | ||
| #[pallet::call_index(2)] | ||
| pub fn start_data_migration(origin: OriginFor<T>) -> DispatchResult { |
There was a problem hiding this comment.
| pub fn start_data_migration(origin: OriginFor<T>) -> DispatchResult { | |
| pub fn ah_ready_for_data(origin: OriginFor<T>) -> DispatchResult { |
? I think it could help to include ah in there to show that it comes from the AH.
There was a problem hiding this comment.
I think can be more general, we might include more. But also no strong opinion. I just trying to not create too many stages to not waste time.
| block_number: BlockNumber, | ||
| }, | ||
| /// The migration is initializing. | ||
| Initializing, |
There was a problem hiding this comment.
| Initializing, | |
| WaitingForAhReadiness, |
There was a problem hiding this comment.
may be I better update the doc? It will be more general and if needed we can include more into it.
| #[pallet::call_index(2)] | ||
| pub fn start_data_migration(origin: OriginFor<T>) -> DispatchResult { | ||
| <T as Config>::ManagerOrigin::ensure_origin(origin)?; | ||
| Self::transition(MigrationStage::AccountsMigrationInit); |
There was a problem hiding this comment.
Maybe defensive assert to check that we are in the right phase.
| Self::transition(MigrationStage::AccountsMigrationInit); | ||
| }, | ||
| MigrationStage::Scheduled { block_number } => | ||
| if now >= block_number { | ||
| match Self::send_xcm(types::AhMigratorCall::<T>::StartMigration) { |
There was a problem hiding this comment.
| match Self::send_xcm(types::AhMigratorCall::<T>::StartMigration) { | |
| match Self::send_xcm(types::AhMigratorCall::<T>::IsAhReadyToMigrate) { |
Sorry for the silly naming suggestions, but i think it can help later to see what is happening.
| } | ||
| }, | ||
| MigrationStage::Initializing => { | ||
| // waiting AH to send a message and to start sending the data |
There was a problem hiding this comment.
We could add some timeout in case AH does not send back the Ok, but should not do now.
| @@ -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>, | |||
There was a problem hiding this comment.
The relay chain will come in as root, right? So we dont need to use this EnsureXcm?
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
meant to be merged into ahm dev branch
Stage management for the Asset Hub migration