Skip to content

XCMv5: Fix for compatibility with V4#6503

Merged
franciscoaguirre merged 17 commits intoparitytech:masterfrom
yrong:fix-xcm-incompatible
Dec 10, 2024
Merged

XCMv5: Fix for compatibility with V4#6503
franciscoaguirre merged 17 commits intoparitytech:masterfrom
yrong:fix-xcm-incompatible

Conversation

@yrong
Copy link
Contributor

@yrong yrong commented Nov 15, 2024

Description

Our smoke tests transfer WETH from Sepolia to Westend-AssetHub breaks, try to reregister WETH on AH but fails as following:

https://bridgehub-westend.subscan.io/xcm_message/westend-4796d6b3600aca32ef63b9953acf6a456cfd2fbe

https://assethub-westend.subscan.io/extrinsic/9731267-0?event=9731267-2

The reason is that the transact call encoded on BH to register the asset

call: (
create_call_index,
asset_id,
MultiAddress::<[u8; 32], ()>::Id(owner),
MINIMUM_DEPOSIT,
)
.encode()
.into(),

0x3500020209079edaa8020300fff9976782d46cc05630d1f6ebab18b2324d6b1400ce796ae65569a670d0c1cc1ac12515a3ce21b5fbf729d63d7b289baad070139d01000000000000000000000000000000

the asset_id which is the xcm location can't be decoded on AH in V5

Issue initial post in https://matrix.to/#/!qUtSTcfMJzBdPmpFKa:parity.io/$RNMAxIIOKGtBAqkgwiFuQf4eNaYpmOK-Pfw4d6vv1aU?via=parity.io&via=matrix.org&via=web3.foundation

@yrong yrong changed the title For compatibility with V4 Fix for compatibility with V4 Nov 15, 2024
@yrong yrong marked this pull request as ready for review November 19, 2024 22:26
@yrong yrong requested a review from a team as a code owner November 19, 2024 22:26
Copy link
Contributor

@vgeddes vgeddes left a comment

Choose a reason for hiding this comment

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

Can you be more specific about what the problem is? The element link does not seem to be working for me.

XCMv4 and XCMv5 each have their own NetworkId with different encoding layouts, and they need to be explicitly converted to each other using From or TryFrom traits.

So then surely BH needs to take care to send an XCM to AH that is in the correct format?

@acatangiu
Copy link
Contributor

The PR makes sense if we have v4::NetworkId entries in storage that we want to directly interpret as v5::NetworkId entries without migrating them.

Is this the case?

Co-authored-by: Adrian Catangiu <adrian@parity.io>
@yrong
Copy link
Contributor Author

yrong commented Nov 21, 2024

The PR makes sense if we have v4::NetworkId entries in storage that we want to directly interpret as v5::NetworkId entries without migrating them.

Is this the case?

Actually we don't store the call on BH, just send the encoded call to AH directly.

Copy link
Contributor

@alistair-singh alistair-singh left a comment

Choose a reason for hiding this comment

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

+1

@acatangiu acatangiu changed the title Fix for compatibility with V4 XCMv5: Fix for compatibility with V4 Nov 21, 2024
@acatangiu
Copy link
Contributor

cc @franciscoaguirre

@yrong
Copy link
Contributor Author

yrong commented Nov 25, 2024

@acatangiu For AssetIdParameter could we use the VersionedLocation

type AssetId = xcm::v5::Location;
type AssetIdParameter = xcm::v5::Location;
instead?

@yrong
Copy link
Contributor Author

yrong commented Nov 25, 2024

@franciscoaguirre Could you help to approve this PR or anything unclear that I can provide, please let me know.

@franciscoaguirre franciscoaguirre added the T6-XCM This PR/Issue is related to XCM. label Nov 25, 2024
Copy link
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

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

Seems reasonable to keep the same indices

auto-merge was automatically disabled November 26, 2024 06:29

Head branch was pushed to by a user without write access

@acatangiu
Copy link
Contributor

@acatangiu For AssetIdParameter could we use the VersionedLocation

type AssetId = xcm::v5::Location;
type AssetIdParameter = xcm::v5::Location;

instead?

AFAIK this has been discussed before and decided against because the storage still needs an upgrade every time to avoid situations of incorrect duplicate assetids registered: e.g. same ID but once with v3 and once with v4 - in the end it doesn't seem to help and makes code more complicated...

@acatangiu acatangiu enabled auto-merge November 26, 2024 08:47
auto-merge was automatically disabled November 26, 2024 09:20

Pull Request is not mergeable

@franciscoaguirre franciscoaguirre added this pull request to the merge queue Dec 10, 2024
Merged via the queue into paritytech:master with commit fe4846f Dec 10, 2024
141 of 142 checks passed
dudo50 pushed a commit to paraspell-research/polkadot-sdk that referenced this pull request Jan 4, 2025
## Description

Our smoke tests transfer `WETH` from Sepolia to Westend-AssetHub breaks,
try to reregister `WETH` on AH but fails as following:


https://bridgehub-westend.subscan.io/xcm_message/westend-4796d6b3600aca32ef63b9953acf6a456cfd2fbe

https://assethub-westend.subscan.io/extrinsic/9731267-0?event=9731267-2

The reason is that the transact call encoded on BH to register the asset

https://github.com/paritytech/polkadot-sdk/blob/a77940bac783108fcae783c553528c8d5328e5b2/bridges/snowbridge/primitives/router/src/inbound/mod.rs#L282-L289
```
0x3500020209079edaa8020300fff9976782d46cc05630d1f6ebab18b2324d6b1400ce796ae65569a670d0c1cc1ac12515a3ce21b5fbf729d63d7b289baad070139d01000000000000000000000000000000
```

the `asset_id` which is the xcm location can't be decoded on AH in V5

Issue initial post in
https://matrix.to/#/!qUtSTcfMJzBdPmpFKa:parity.io/$RNMAxIIOKGtBAqkgwiFuQf4eNaYpmOK-Pfw4d6vv1aU?via=parity.io&via=matrix.org&via=web3.foundation

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
franciscoaguirre added a commit that referenced this pull request Apr 9, 2025
## Description

Our smoke tests transfer `WETH` from Sepolia to Westend-AssetHub breaks,
try to reregister `WETH` on AH but fails as following:


https://bridgehub-westend.subscan.io/xcm_message/westend-4796d6b3600aca32ef63b9953acf6a456cfd2fbe

https://assethub-westend.subscan.io/extrinsic/9731267-0?event=9731267-2

The reason is that the transact call encoded on BH to register the asset

https://github.com/paritytech/polkadot-sdk/blob/a77940bac783108fcae783c553528c8d5328e5b2/bridges/snowbridge/primitives/router/src/inbound/mod.rs#L282-L289
```
0x3500020209079edaa8020300fff9976782d46cc05630d1f6ebab18b2324d6b1400ce796ae65569a670d0c1cc1ac12515a3ce21b5fbf729d63d7b289baad070139d01000000000000000000000000000000
```

the `asset_id` which is the xcm location can't be decoded on AH in V5

Issue initial post in
https://matrix.to/#/!qUtSTcfMJzBdPmpFKa:parity.io/$RNMAxIIOKGtBAqkgwiFuQf4eNaYpmOK-Pfw4d6vv1aU?via=parity.io&via=matrix.org&via=web3.foundation

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
EgorPopelyaev pushed a commit that referenced this pull request Apr 10, 2025
Backporting #6503 in
order to fix snowbridge.

Needed for polkadot-fellows/runtimes#606

---------

Co-authored-by: Ron <yrong1997@gmail.com>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Dónal Murray <donal.murray@parity.io>
Co-authored-by: alvicsam <alvicsam@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T6-XCM This PR/Issue is related to XCM.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants