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

Polkadot-0.9.38 asset registry migration incorrect #894

Closed
sander2 opened this issue Mar 15, 2023 · 5 comments
Closed

Polkadot-0.9.38 asset registry migration incorrect #894

sander2 opened this issue Mar 15, 2023 · 5 comments

Comments

@sander2
Copy link
Contributor

sander2 commented Mar 15, 2023

The asset registry contains a VersionedMultiLocation here:

pub location: Option<VersionedMultiLocation>,

This type changed in the 0.9.38 upgrade. Here's the old version:

https://github.com/paritytech/polkadot/blob/645723987cf9662244be8faf4e9b63e8b9a1b3a3/xcm/src/lib.rs#L73-L76

And here's the new:
https://github.com/paritytech/polkadot/blob/8deef133d3ca1bdea8c6267c4a66ecabb903a18b/xcm/src/lib.rs#L318-L325

So assetRegistry.metadata needs to be migrated as well.

I'm not sure how best to approach this migration, since the definitions of xcm v0 got removed from polkadot. I considered adding a dependency on polkadot 0.9.37, but this is problematic for project that use a patch section in their tomls.. We may have to redefine the types manually, which is very annoying.. Any other ideas are welcome

@xlc
Copy link
Member

xlc commented Mar 15, 2023

no add deps to 0.9.37 won't work. the only way is copy the definition

@xlc
Copy link
Member

xlc commented Mar 15, 2023

The upside is that V1 should be identical to V2 (you should have some tests to confirm that). So if you don't already have V0 in the storage, you may be able to skip the migration. Otherwise you could upgrade it to V1 before the runtime upgrade.

@sander2
Copy link
Contributor Author

sander2 commented Mar 15, 2023

The upside is that V1 should be identical to V2 (you should have some tests to confirm that). So if you don't already have V0 in the storage, you may be able to skip the migration. Otherwise you could upgrade it to V1 before the runtime upgrade.

V2 multilocation is indeed identical to V1 - it's just an import in 0.9.37: https://github.com/paritytech/polkadot/blob/645723987cf9662244be8faf4e9b63e8b9a1b3a3/xcm/src/v2/mod.rs#L68

A migration would still be required, but it'd be a simple one: OldVersionMultiLocation::V1(x) --> VersionedMultiLocation::V2(x). Without a migration it'd get interpreted as V3.

Anyway, we don't have V0 ourselves, so that part of the migration is not needed for us, but I don't know if this is true for everyone. If you're comfortable with merging a migration that only works for V1, that'd work for me

@xlc
Copy link
Member

xlc commented Mar 15, 2023

I think we all want to avoid doing a lot of work that may not be needed at all. So let's just do the migration for V1. We can make sure there are enough docs and warnings so the developer using the migration know they need to check the onchain data first. For example, the migration can be named like MigrateV1Only_V0NotSupported

@sander2
Copy link
Contributor Author

sander2 commented Mar 20, 2023

Addressed by paritytech/polkadot#6884, which was backported to 0.9.38

@sander2 sander2 closed this as completed Mar 20, 2023
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

No branches or pull requests

2 participants