Migrate foreign assets v3::Location to v4::Location #4129

franciscoaguirre · 2024-04-15T10:58:40Z

In the move from XCMv3 to XCMv4, the AssetId for ForeignAssets in asset-hub-rococo and asset-hub-westend was left as v3::Location to be later migrated to v4::Location.

This is that migration PR.

Because the encoding of v3::Location and v4::Location is the same, we don't need to do any data migration, the keys will still be decodable.
The original idea by Jan was to make the v4 changes in v3 since the ABI (the encoding/decoding) didn't change.
Corroborated the ABI is the same iterating over all storage, the code is on another branch.

We will need a data migration when we want to update from v4::Location to v5::Location because of the accepted RFC changing the NetworkId enum.
I'll configure MBMs (Multi-Block Migrations) then and make the actual migration.

Fixes #4128

…3 to V4

acatangiu · 2024-04-16T13:03:52Z

Can we migrate to VersionedLocation instead?

franciscoaguirre · 2024-04-19T08:47:41Z

Can we migrate to VersionedLocation instead?

We can. The only issue with that is we might have two versions of the same asset.
Say we do migrate to VersionedLocation, then we could have both VersionedLocation::V3(usdt) and VersionedLocation::V4(usdt) for example.

We'd still need a migration to change values to the newest version. It's either that or migrating to change types to the newest version. I say fixing the version in this case doesn't sound like a bad idea. What do you think?

acatangiu · 2024-04-19T10:58:07Z

I was thinking to keep assets with mixed versions in storage, but you're right that it won't work (or rather it could work but with many corner cases and ultimately more complexity).

I agree we keep it simple and migrate assets to latest version.

franciscoaguirre · 2024-07-12T22:04:32Z

I was thinking to keep assets with mixed versions in storage, but you're right that it won't work (or rather it could work but with many corner cases and ultimately more complexity).

I agree we keep it simple and migrate assets to latest version.

Rethinking this, it might be better to migrate them to VersionedAssets. The only way to create foreign assets is via XCM and the sender doesn't specify the asset id. We can just use V4 all the time and change it when we migrate. This means we'll have more leeway whenever a new XCM version comes out. What do you think?

acatangiu · 2024-07-15T08:39:40Z

@xlc I remember seeing another thread on this topic where you had some input, but can't find it now.
(I might also be imagining things 😆 , but tagging you here just in case)

bkontur · 2024-07-15T10:03:43Z

@xlc I remember seeing another thread on this topic where you had some input, but can't find it now. (I might also be imagining things 😆 , but tagging you here just in case)

This is the original issue for this foreign asset id problem: #1130,

Rethinking this, it might be better to migrate them to VersionedAssets. The only way to create foreign assets is via XCM and the sender doesn't specify the asset id. We can just use V4 all the time and change it when we migrate. This means we'll have more leeway whenever a new XCM version comes out. What do you think?

I was proposing there something like this - to use VersionedLocation for Transact(ForeignAssets::create(asset_id as VersionedLocation, ...):

        type AssetId = xcm::v3::Location;                    // requires storage migration
	type AssetIdParameter = VersionedLocation;           // this is used for extrinsics

which would solve xcm::Transact versioning problem from other parachains,
but another part of the problem is how the dapps will read ForeignAssets with v3/v4::Location:

if they read directly from storage, they could have a problem when we migrate to new version
if they use new dedicated runtime API (with input parameter as desired XcmVersion - as we do for query_acceptable_payment_assets(xcm_version: xcm::Version)), where we can return VersionedLocation -> no problem
something else?

If we use VersionedLocation for AssetId:

        type AssetId = VersionedLocation

I don't know, it could lead maybe to other problems, at least we would need to use something like for pallet_xcm with LatestVersionedLocation

franciscoaguirre · 2024-07-15T11:20:34Z

I was initially convinced by the argument that if we use VersionedLocation, we could have two versions of the same asset. However, now I'm not so sure. Since we can only create foreign assets via an XCM and the runtime assigns the id, we can always assign the latest version. When we upgrade, we bump the version and migrate all ids to VersionedLocation::V{n + 1}.

xlc · 2024-07-15T11:43:25Z

@xlc I remember seeing another thread on this topic where you had some input, but can't find it now. (I might also be imagining things 😆 , but tagging you here just in case)

Yeah I made a comment somewhere but also can't find it anymore...
But basically there isn't much reason to migrate from v3 to v4 as they are exactly the same thing except version number.
Yeah there is no harm to do so but also no benefit and less work is better than more work.
Also I always use a fixed XCM version for storage item and Versioned for extrinsic (this applies to Acala and ORML).
This means it is less likely to have unexpected storage item format change as a fixed version type should never change but Versioned type is a moving target. e.g. when XCM V2 is removed, runtime using V2 type will not be able to compile but runtime using Versioned type that is V2 will get bad decoding error. It also means there could be multiple representation of a same thing and that can makes consumer of the storage item harder (e.g. UI, block explorer). It could be a critical issue when such thing is used as storage key.

It is possible to have runtime migration to keep the version is always at latest but we might as well use fixed version and do the migration at our own pace. i.e. we can upgrade from v3 to v4 anytime we want before v3 is removed but with Versioned type, we have to upgrade as soon as a new version is introduced or there will be unexpected things.

In the end, there are little upside to use Versioned type but many downsides.

cumulus/parachains/runtimes/assets/migrations/src/v1/benchmarks.rs

cumulus/parachains/runtimes/assets/migrations/src/v1/mod.rs

cumulus/parachains/runtimes/assets/migrations/src/v1/tests.rs

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/lib.rs

cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs

serban300

Looks good to me, modulo Adrian's comment related to pallet_collator_selection::migration::v1::MigrateToV1<Runtime>,

prdoc/pr_4129.prdoc

acatangiu · 2024-08-13T16:09:57Z

needs a review from @paritytech/frame-coders

bkontur · 2024-08-13T08:24:53Z

cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/send.rs

 			.appended_with(asset_location_on_penpal)
 			.unwrap();

 	// Encoded `create_asset` call to be executed in AssetHub
 	let call = AssetHubRococo::create_foreign_asset_call(
-		foreign_asset_at_asset_hub,
+		foreign_asset_at_asset_hub.clone(),


@franciscoaguirre just thinking, if we add xcm::v5 this won't compile again, I am not sure, but maybe:
foreign_asset_at_asset_hub.clone().try_into().unwrap() would do the trick?

bkontur · 2024-08-13T08:35:08Z

cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs

@@ -1781,64 +1784,3 @@ cumulus_pallet_parachain_system::register_validate_block! {
 	Runtime = Runtime,
 	BlockExecutor = cumulus_pallet_aura_ext::BlockExecutor::<Runtime, Executive>,
 }
-
-#[cfg(test)]
-mod tests {


@franciscoaguirre I'm not aware of any specific rule here. All other runtimes have tests in this file as well as in separate tests.rs files or in a separate tests directory. If that's the case, we should also clean up the other runtimes. Since these tests only check some basic constants, I think we could keep them here.

Or even just a tests.rs file, instead of tests/mod.rs.

Guess I ended up moving tests early in the PR for some reason. I'll put them back

seadanda · 2024-08-14T08:01:20Z

cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs

@@ -1781,64 +1784,3 @@ cumulus_pallet_parachain_system::register_validate_block! {
 	Runtime = Runtime,
 	BlockExecutor = cumulus_pallet_aura_ext::BlockExecutor::<Runtime, Executive>,
 }
-
-#[cfg(test)]
-mod tests {


Or even just a tests.rs file, instead of tests/mod.rs.

* master: (35 commits) Fix OurViewChange small race (#5356) Make ticket non-optional and add ensure_successful method to Consideration trait (#5359) [tests] dedup test code, add more tests, improve naming and docs (#5338) Stop running the wishlist workflow on forks (#5297) Migrate foreign assets v3::Location to v4::Location (#4129) Minor clean up (#5284) [Pools] Ensure members can always exit the pool gracefully (#4998) StorageWeightReclaim: set to node pov size if higher (#5281) [Bot] Add prdoc generation (#5331) Small nits found accidentally along the way (#5341) Create subsystem-benchmarks.yml (#5325) Bump libp2p-identity from 0.2.8 to 0.2.9 (#5232) Bump authoring duration for async backing to 2s. (#5195) Fix spelling issues (#5206) Bump the known_good_semver group across 1 directory with 3 updates (#5315) `polkadot-node-core-pvf-common`: Fix test compilation error (#5310) ci: Paused `cmd-action` commenter (#5287) Remove unnecessary mut (#5318) chain-spec: minor clarification on the genesis config patch (#5324) fix av-distribution Jaeger spans mem leak (#5321) ...

serban300 · 2024-08-14T15:48:39Z

@franciscoaguirre I think this PR broke the zombienet-bridges-0001-asset-transfer-works test.

Can you take a look please ?

franciscoaguirre · 2024-08-14T16:11:51Z

Sure, I'll take a look

bkontur · 2024-08-14T19:27:00Z

Sure, I'll take a look

hmm, strange, https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6991678

2024-08-13 14:37:42             VEC: Unable to decode on index 0 createType(Lookup37):: Struct: failed on interior: {"_enum":{"Here":"Null","X1":"[Lookup40;1]","X2":"[Lookup40;2]","X3":"[Lookup40;3]","X4":"[Lookup40;4]","X5":"[Lookup40;5]","X6":"[Lookup40;6]","X7":"[Lookup40;7]","X8":"[Lookup40;8]"}}:: Enum(X1):: decodeU8aVec: failed at 0x5b6f626a656374204f626a6563745d… (index 1/1): {"_enum":{"Parachain":"Compact<u32>","AccountId32":"{\"network\":\"Option<StagingXcmV4JunctionNetworkId>\",\"id\":\"[u8;32]\"}","AccountIndex64":"{\"network\":\"Option<StagingXcmV4JunctionNetworkId>\",\"index\":\"Compact<u64>\"}","AccountKey20":"{\"network\":\"Option<StagingXcmV4JunctionNetworkId>\",\"key\":\"[u8;20]\"}","PalletInstance":"u8","GeneralIndex":"Compact<u128>","GeneralKey":"{\"length\":\"u8\",\"data\":\"[u8;32]\"}","OnlyChild":"Null","Plurality":"{\"id\":\"XcmV3JunctionBodyId\",\"part\":\"XcmV3JunctionBodyPart\"}","GlobalConsensus":"StagingXcmV4JunctionNetworkId"}}:: Unable to create Enum via index 91, in Parachain, AccountId32, AccountIndex64, AccountKey20, PalletInstance, GeneralIndex, GeneralKey, OnlyChild, Plurality, GlobalConsensus
2024-08-13 14:37:42        RPC-CORE: queryStorageAt(keys: Vec<StorageKey>, at?: BlockHash): Vec<StorageChangeSet>:: createType(Vec<StorageKey>):: createType(Lookup37):: Struct: failed on interior: {"_enum":{"Here":"Null","X1":"[Lookup40;1]","X2":"[Lookup40;2]","X3":"[Lookup40;3]","X4":"[Lookup40;4]","X5":"[Lookup40;5]","X6":"[Lookup40;6]","X7":"[Lookup40;7]","X8":"[Lookup40;8]"}}:: Enum(X1):: decodeU8aVec: failed at 0x5b6f626a656374204f626a6563745d… (index 1/1): {"_enum":{"Parachain":"Compact<u32>","AccountId32":"{\"network\":\"Option<StagingXcmV4JunctionNetworkId>\",\"id\":\"[u8;32]\"}","AccountIndex64":"{\"network\":\"Option<StagingXcmV4JunctionNetworkId>\",\"index\":\"Compact<u64>\"}","AccountKey20":"{\"network\":\"Option<StagingXcmV4JunctionNetworkId>\",\"key\":\"[u8;20]\"}","PalletInstance":"u8","GeneralIndex":"Compact<u128>","GeneralKey":"{\"length\":\"u8\",\"data\":\"[u8;32]\"}","OnlyChild":"Null","Plurality":"{\"id\":\"XcmV3JunctionBodyId\",\"part\":\"XcmV3JunctionBodyPart\"}","GlobalConsensus":"StagingXcmV4JunctionNetworkId"}}:: Unable to create Enum via index 91, in Parachain, AccountId32, AccountIndex64, AccountKey20, PalletInstance, GeneralIndex, GeneralKey, OnlyChild, Plurality, GlobalConsensus
 Error running script: /home/nonroot/bridges-polkadot-sdk/bridges/testing/framework/js-helpers/wrapped-assets-balance.js

probably this lines: https://github.com/paritytech/polkadot-sdk/blob/master/bridges/testing/framework/js-helpers/wrapped-assets-balance.js#L10-L11:

const foreignAssetAccount = await api.query.foreignAssets.account(
            { parents: 2, interior: { X1: { GlobalConsensus: bridgedNetworkName } } },

are not compatible with xcm::v4?

* master: (167 commits) Upgrade accidentally downgraded deps (#5365) [Pools] Fix issues with member migration to `DelegateStake` (#4822) Unify `no_genesis` check (#5360) [CI] Fix prdoc command (#5358) Beefy: add benchmarks for `report_fork_voting()` (#5188) Fix OurViewChange small race (#5356) Make ticket non-optional and add ensure_successful method to Consideration trait (#5359) [tests] dedup test code, add more tests, improve naming and docs (#5338) Stop running the wishlist workflow on forks (#5297) Migrate foreign assets v3::Location to v4::Location (#4129) Minor clean up (#5284) [Pools] Ensure members can always exit the pool gracefully (#4998) StorageWeightReclaim: set to node pov size if higher (#5281) [Bot] Add prdoc generation (#5331) Small nits found accidentally along the way (#5341) Create subsystem-benchmarks.yml (#5325) Bump libp2p-identity from 0.2.8 to 0.2.9 (#5232) Bump authoring duration for async backing to 2s. (#5195) Fix spelling issues (#5206) Bump the known_good_semver group across 1 directory with 3 updates (#5315) ...

franciscoaguirre · 2024-08-15T11:36:51Z

Ahh indeed, that must be the issue. In v4, interior is now an array, so it should be:

const foreignAssetAccount = await api.query.foreignAssets.account(
            { parents: 2, interior: { X1: [{ GlobalConsensus: bridgedNetworkName }] } },

I'll try it and see if that solves it

After #4129, a zombienet bridge test was broken. This is because XCMv4 locations have an array in the `interior` field, which also has to appear in PJS. --------- Co-authored-by: Serban Iorga <serban@parity.io>

feat(asset-hub-rococo-runtime): add a migration from foreign assets V…

2b39b01

…3 to V4

franciscoaguirre added 2 commits June 26, 2024 19:01

Merge branch 'master' into migrate-assets-to-v4

da042a3

fix: fmt

94738d1

franciscoaguirre added the T6-XCM This PR/Issue is related to XCM. label Jun 26, 2024

franciscoaguirre added 2 commits June 26, 2024 20:55

feat(assets-common): add foreign assets migration

f0356f0

feat(foreign-assets-migration): move to assets common

ca8c876

franciscoaguirre added 3 commits July 29, 2024 14:00

Merge branch 'master' into migrate-assets-to-v4

dce5ce1

fix: another v3 -> v4 change

e2a5398

feat(assets-common-migrations): create a pallet for asset migrations

a656f4b

franciscoaguirre commented Jul 31, 2024

View reviewed changes

cumulus/parachains/runtimes/assets/migrations/src/v1/benchmarks.rs Outdated Show resolved Hide resolved

franciscoaguirre commented Jul 31, 2024

View reviewed changes

cumulus/parachains/runtimes/assets/migrations/src/v1/mod.rs Outdated Show resolved Hide resolved

franciscoaguirre commented Jul 31, 2024

View reviewed changes

cumulus/parachains/runtimes/assets/migrations/src/v1/tests.rs Outdated Show resolved Hide resolved

ggwpez and others added 10 commits August 1, 2024 12:23

nearly works

1457f3f

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

nearly works

211207e

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

make compile

d1efea4

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

feat(assets-common-migrations): benchmark tests passing

e610689

feat(asset-hub-westend): migrate from v3 to v4

ec88a62

fix(assets-common-migrations): tests work

310b1bf

doc: prdoc

a619548

fix(assets-common-migrations): some lints

2e3931a

fix(assets-common-migrations): some fixes and a license

435a412

fix(asset-hub-westend): actually run the migration

b43ae14

franciscoaguirre added 2 commits August 8, 2024 19:55

doc(prdoc): update now that there's no data migration

f5a3eb5

Merge branch 'master' into migrate-assets-to-v4

280ff6b

franciscoaguirre changed the title ~~Add a migration from foreign assets v3::Location to v4::Location~~ Migrate foreign assets v3::Location to v4::Location Aug 8, 2024

franciscoaguirre added 4 commits August 9, 2024 11:05

Merge branch 'master' into migrate-assets-to-v4

d4ccba2

Merge branch 'master' into migrate-assets-to-v4

11f7c7d

Merge branch 'master' into migrate-assets-to-v4

917e2f9

Merge branch 'master' into migrate-assets-to-v4

e0f40cb

acatangiu self-requested a review August 13, 2024 10:50

acatangiu approved these changes Aug 13, 2024

View reviewed changes

cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/lib.rs Outdated Show resolved Hide resolved

cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs Outdated Show resolved Hide resolved

serban300 approved these changes Aug 13, 2024

View reviewed changes

prdoc/pr_4129.prdoc Outdated Show resolved Hide resolved

franciscoaguirre added 2 commits August 13, 2024 16:12

chore: feedback

ae7e326

Merge branch 'master' into migrate-assets-to-v4

2dca782

franciscoaguirre enabled auto-merge August 13, 2024 14:19

bkontur approved these changes Aug 14, 2024

View reviewed changes

seadanda approved these changes Aug 14, 2024

View reviewed changes

franciscoaguirre added this pull request to the merge queue Aug 14, 2024

Merged via the queue into master with commit be74fe9 Aug 14, 2024
175 of 178 checks passed

franciscoaguirre deleted the migrate-assets-to-v4 branch August 14, 2024 09:06

franciscoaguirre mentioned this pull request Aug 15, 2024

Fix zombienet bridges test #5373

Merged

github-actions bot mentioned this pull request Oct 8, 2024

Update polkadot-sdk from stable2407 to stable2409 moonbeam-foundation/moonbeam#2994

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate foreign assets v3::Location to v4::Location #4129

Migrate foreign assets v3::Location to v4::Location #4129

franciscoaguirre commented Apr 15, 2024 •

edited

Loading

acatangiu commented Apr 16, 2024

franciscoaguirre commented Apr 19, 2024

acatangiu commented Apr 19, 2024

franciscoaguirre commented Jul 12, 2024

acatangiu commented Jul 15, 2024

bkontur commented Jul 15, 2024

franciscoaguirre commented Jul 15, 2024

xlc commented Jul 15, 2024 •

edited

Loading

serban300 left a comment

acatangiu commented Aug 13, 2024

bkontur Aug 13, 2024

bkontur Aug 13, 2024

seadanda Aug 14, 2024

franciscoaguirre Aug 14, 2024

seadanda Aug 14, 2024

serban300 commented Aug 14, 2024

franciscoaguirre commented Aug 14, 2024

bkontur commented Aug 14, 2024

franciscoaguirre commented Aug 15, 2024 •

edited

Loading

Migrate foreign assets v3::Location to v4::Location #4129

Migrate foreign assets v3::Location to v4::Location #4129

Conversation

franciscoaguirre commented Apr 15, 2024 • edited Loading

acatangiu commented Apr 16, 2024

franciscoaguirre commented Apr 19, 2024

acatangiu commented Apr 19, 2024

franciscoaguirre commented Jul 12, 2024

acatangiu commented Jul 15, 2024

bkontur commented Jul 15, 2024

franciscoaguirre commented Jul 15, 2024

xlc commented Jul 15, 2024 • edited Loading

serban300 left a comment

Choose a reason for hiding this comment

acatangiu commented Aug 13, 2024

bkontur Aug 13, 2024

Choose a reason for hiding this comment

bkontur Aug 13, 2024

Choose a reason for hiding this comment

seadanda Aug 14, 2024

Choose a reason for hiding this comment

franciscoaguirre Aug 14, 2024

Choose a reason for hiding this comment

seadanda Aug 14, 2024

Choose a reason for hiding this comment

serban300 commented Aug 14, 2024

franciscoaguirre commented Aug 14, 2024

bkontur commented Aug 14, 2024

franciscoaguirre commented Aug 15, 2024 • edited Loading

franciscoaguirre commented Apr 15, 2024 •

edited

Loading

xlc commented Jul 15, 2024 •

edited

Loading

franciscoaguirre commented Aug 15, 2024 •

edited

Loading