Backport #6503 to stable2412#8194
Conversation
## 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>
|
This pull request is amending an existing release. Please proceed with extreme caution,
Emergency Bypass
If you really need to bypass this check: add |
franciscoaguirre
changed the title
franciscoaguirre
added
T6-XCM
seadanda
left a comment
seadanda
left a comment
There was a problem hiding this comment.
Need to work out how to manage the packages that have been released so far, but fixing the indices makes sense
|
|
||
| crates: | ||
| - name: staging-xcm | ||
| bump: patch |
There was a problem hiding this comment.
Is it? We'd need to yank every other version that has Ethereum at index 4 (etc) since it's changing the variant indices of a public enum
There was a problem hiding this comment.
Yeah but not fixing the indices was an oversight and we have to backport it to stable2412 and stable2503. We do need to yank all the old versions
franciscoaguirre
dismissed stale reviews from sandreim, seadanda, and bkontur
via
941bc97
Co-authored-by: Dónal Murray <donal.murray@parity.io>
| /// The Kusama canary-net Relay-chain. | ||
| Kusama, | ||
| /// An Ethereum network specified by its chain ID. | ||
| #[codec(index = 7)] |
There was a problem hiding this comment.
Would it be worth to add some ensuring unit-test, that encoded variants from a new version are backwards compatible, e.g. xcm::v5::NetworkId::Ethereum(chain: 1).encode() == xcm::v4::NetworkId::Ethereum(chain: 1).encode()?
Do we even need/want to be backwards-compatible this way between version? Shouldn't some migration for data fix this?
There was a problem hiding this comment.
We should definitely be compatible. I'm intrigued now about how the conversion functions we have were not enough to fix this
There was a problem hiding this comment.
so definitely we need some tests for that
There was a problem hiding this comment.
We should definitely be compatible. I'm intrigued now about how the convertersion functions we have were not enough to fix this
so where exactly is the issue if we do have conversion functions?
I think this could be fixed on snowbridge code side without needing to change indexes - I am willing to bet they are encoding/decoding without checking actual version and that's why it's failing.
Unfortunately, since the index change has been released in 2503, we have to backport it to 2412 or revert it and patch in 2503 to be consistent with xcm v5 locations encoding. So either way, we're cornered into yanking and patching crates.
I think patching 2412 is less work than patching 2503 so I am approving this PR, but this situation is not ok, and we have to get our shit together with managing these stable branches.
| /// The Kusama canary-net Relay-chain. | ||
| Kusama, | ||
| /// An Ethereum network specified by its chain ID. | ||
| #[codec(index = 7)] |
There was a problem hiding this comment.
We should definitely be compatible. I'm intrigued now about how the convertersion functions we have were not enough to fix this
so where exactly is the issue if we do have conversion functions?
I think this could be fixed on snowbridge code side without needing to change indexes - I am willing to bet they are encoding/decoding without checking actual version and that's why it's failing.
Unfortunately, since the index change has been released in 2503, we have to backport it to 2412 or revert it and patch in 2503 to be consistent with xcm v5 locations encoding. So either way, we're cornered into yanking and patching crates.
I think patching 2412 is less work than patching 2503 so I am approving this PR, but this situation is not ok, and we have to get our shit together with managing these stable branches.
8 participants
Backporting #6503 in order to fix snowbridge.
Needed for polkadot-fellows/runtimes#606