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

Set up AH migration testing framework for single pallets #597

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

Conversation

re-gius
Copy link

@re-gius re-gius commented Feb 20, 2025

Set up a leaner migration testing framework for single pallets and implementing it for pallet preimage.

This PR introduces the traits RcMigrationChecks and AhMigrationChecks for checks before and after a single pallet's migration. Upon implementing these traits for a pallet, you can use rc_migrate and ah_migrate functions to run a test on the correctness of the pallet's migration.

rc_migrate::<Payload, pallet_rc_migrator::preimage::PreimageChunkMigrator<Polkadot>>(rc);
ah_migrate::<
Payload,
pallet_rc_migrator::preimage::PreimageChunkMigrator<Polkadot>,
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
pallet_rc_migrator::preimage::PreimageChunkMigrator<Polkadot>,
_,

Not sure if you tried it, but often the Rust compiler knows what you mean

}

fn post_check(keys: Self::Payload) {
fn post_check(maybe_keys: Option<Self::Payload>) {
let Some(keys) = maybe_keys else { return };
Copy link
Member

Choose a reason for hiding this comment

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

When can this be none?

Copy link
Author

Choose a reason for hiding this comment

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

I don't have any scenario in mind, it was just to leave the possibility to have dummy checks doing nothing or almost nothing, but I can remove it

Copy link
Member

@ggwpez ggwpez Feb 21, 2025

Choose a reason for hiding this comment

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

I meant i this specific case; if there is no way that it should normally happen then there should be a defensive assert or so. Then it will fail in tests and print a log in production.

let Some(keys) = maybe_keys.defensive() else { return };

fn pre_check() -> Self::Payload;
/// In general, the expected output should be Some(payload) for data being transferred out of a
/// non-empty pallet, while it should be None for data being transferred to an empty pallet.
fn pre_check() -> Option<Self::Payload>;
Copy link
Member

Choose a reason for hiding this comment

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

Okay i think in general we need four hooks:

  • pre-RC return RcPayload
  • pre-AH returns AhPayload
  • post-RC, accepts RcPayload
  • post-ah, accepts RcPayload and AhPayload

Did you experiment with this yet? I think that otherwise it will not be possible to test that we migrated all data in and out correctly.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it makes sense, I'll do it this way

}
});
relay_chain.commit_all().unwrap();
// TODO: for some reason this prints some small value (2947), but logs on XCM send and
Copy link
Member

Choose a reason for hiding this comment

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

We batch messages into XCM messages, about 480 for the accounts migration or so, depending on the migration.


AhMigrator::post_check(ah_payload, rc_payload);
// NOTE that the DMP queue is probably not empty because the snapshot that we use
// contains some overweight ones.
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
// contains some overweight ones.
// contains some overweight ones.
// TODO compare with the number of messages before the migration

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

LGTM

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.

2 participants