-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Snowbridge: Remove asset location check for compatibility #8473
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
Snowbridge: Remove asset location check for compatibility #8473
Conversation
| let expected_asset_id = ConvertAssetId::convert(&token_id).ok_or(InvalidAsset)?; | ||
| ensure!(asset_id == expected_asset_id, InvalidAsset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@franciscoaguirre are there perhaps other ways to check asset ID equivalence, across different XCM versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I assume the check here is unnecessary, as the conversion alone is sufficient to verify whether the token is registered.
Strictly speaking, |
| let token_id = TokenIdOf::convert_location(&asset_id).ok_or(InvalidAsset)?; | ||
|
|
||
| let expected_asset_id = ConvertAssetId::convert(&token_id).ok_or(InvalidAsset)?; | ||
|
|
||
| ensure!(asset_id == expected_asset_id, InvalidAsset); | ||
| ConvertAssetId::convert(&token_id).ok_or(InvalidAsset)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the location has already been registered as a PNA and a foreign token id has been allocated in storage for it, why not just use ConvertAssetId::convert_back to retrieve the stored id?
Recreating the token id deterministically using TokenIdOf::convert_location is dangerous for the reason I mentioned in #8473 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as newer versions of XCM can add or remove various enum variants for NetworkId
I assume removal is not allowed, or that the codec index should not chage. That's why I created PR#6503
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just use ConvertAssetId::convert_back to retrieve the stored id?
As mentioned the token registered (in V4) is not the same as the token that was transferred (in V5). However, this reminds me that NativeToForeignId storage is actually unused, so I performed some cleanup accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned the token registered (in V4) is not the same as the token that was transferred (in V5).
Can you give an example scenario so that we can make the discussion more concrete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confusion earlier. Yes, technically we can use convert_back to obtain the tokenID from the location. But in my opinion, it’s unnecessary — the tokenID is already stored on Ethereum and must be permanent. This means that TokenIdOf::convert_location(&location) should remain stable across different XCM versions.
In my view, changes like those in this PR#5390 are not good practice. Adding new fields or enums is fine, but replacing or removing existing ones should not be allowed. This should be a core principle for all upgrades — whether on Substrate or Ethereum.
To ensure the tokenID remains stable, I’ve added tests in fellow-runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my view, changes like those in this #5390 are not good practice. Adding new fields or enums is fine, but replacing or removing existing ones should not be allowed. This should be a core principle for all upgrades — whether on Substrate or Ethereum.
Well XCM is explicitly versioned. Parity's contract to us developers is that V4::NetworkId and V5::NetworkId are allowed to ABI-incompatible, as long as they are infallibly convertible to each other. Accordingly, there is no guarantee that #5390 won't again happen again for V6.
In light of this, I think using convert_back is more secure and robust.
Deterministic id generation is mathematically neat, but I feel there too many compatibility problems for me to feel safe using it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no guarantee that #5390 won't again happen again for V6.
That's why I added the tests as mentioned in this comment - the tests will fail before any issues go alive to production.
Actually, #5390 is not relevant to PNA transfers, since the codec index for the NetworkId of the Polkadot enum remains unchanged in V5. However, it will break ENA registration on AH, which is another incompatability issue we want to avoid - hence the need for PR#6503.
So I'd suggest avoiding incompatibility issues if possible.
|
By the way, in a recent commit, I changed the storage to use IMHO, it’s always safer to use VersionedLocation in a storage context. Although this requires a runtime upgrade to migrate from a V4 location to VersionedLocation::V4 (and pinged as V4), no further migration is needed - as long as the conversion (from V4 to V5 or V6) works correctly. @acatangiu @bkontur Please let me know what you think. |
| StorageMap<_, Blake2_128Concat, xcm::v5::Location, TokenId, OptionQuery>; | ||
| StorageMap<_, Blake2_128Concat, TokenId, VersionedLocation, OptionQuery>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, in a recent commit, I changed the storage to use VersionedLocation.
I am slightly leaning against this.
We support a sliding window of 3 XCM versions: [lastest, latest - 2]. So over time you still need migrations to migrate from e.g. VersionedLocation::V3() storage entries to VersionedLocation::V6() when we upgrade to XCMv6.
You get the advantage of somewhat smaller migrations (only oldest entries need migrating), but
you get the disadvantage of mixed storage holding various versions.
Neither option is truly better IMO, but I prefer consistency across pallets and storing base/unversioned types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disadvantage of mixed storage holding various versions.
IMHO, this may not be too problematic for this specific use case, as long as the derived TokenId is (and is intended to be) XCM version-agnostic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a sliding window of 3 XCM versions: [lastest, latest - 2]
That's good. As mentioned, the current storage version is V4. Assuming we migrate from Location(V4) to VersionLocation::V5, that means we don't need to worry about a runtime migration until XCM V8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer not to use VersionedLocation in storage either. I want highly normalized, consistent data in storage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted recent updates.
| fn convert_back(_: &Location) -> Option<TokenId> { | ||
| None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why use MaybeEquivalence if you don't provide convert_back functionality?
Can you use TryConvert/MaybeConvert instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep this PR focused, I've opend a separate PR for the cleanup and refactoring work, which isn't high priority.
vgeddes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yrong must this be backported to stable2503? If so, lets focus on the immediate issue at hand - removal of the asset location check.
Based on the tests demonstrated in polkadot-fellows/runtimes#730 (comment), both the cleanup in this PR and the migration in polkadot-fellows/runtimes#730 are technically not required. However, I still prefer to merge these PRs and include the runtime migration to reduce inconsistencies and ensure compatibility with future versions. @acatangiu Please let me know what you think. |
62946bb to
697f4ed
Compare
I agree that we should merge these prs to reduce inconsistencies. |
|
you should include yrong#19 here as well then merge and backport as |
Also, for these situations where we override the automated semver recommendation, you should add |
prdoc/pr_8473.prdoc
Outdated
| - name: bridge-hub-westend-runtime | ||
| bump: patch | ||
| validate: false | ||
| - name: bridge-hub-westend-integration-tests | ||
| bump: patch | ||
| validate: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
| - name: bridge-hub-westend-runtime | |
| bump: patch | |
| validate: false | |
| - name: bridge-hub-westend-integration-tests | |
| bump: patch | |
| validate: false | |
| - name: bridge-hub-westend-runtime | |
| bump: none | |
| validate: false | |
| - name: bridge-hub-westend-integration-tests | |
| bump: none | |
| validate: false |
test runtimes are not published so can be bump: none - as a general rule, if it has westend in the name, we don't publish.
64aed49
The `TokenIdOf` [convert](https://github.com/paritytech/polkadot-sdk/blob/4b83d24f4bc96a7b17964be94b178dd7b8f873b5/bridges/snowbridge/primitives/core/src/location.rs#L40) is XCM version-agnostic, meaning we will get the same token ID for both V5 and legacy V4 asset. However, the extra check is unnecessary, as the`ConvertAssetId::convert(&token_id).ok_or(InvalidAsset)?;` alone is sufficient to verify whether the token is registered. (cherry picked from commit 64aed49)
|
Successfully created backport PR for |
* master: (99 commits) Snowbridge: Remove asset location check for compatibility (#8473) add poke_deposit extrinsic to pallet-bounties (#8382) litep2p/peerset: Reject non-reserved peers in the reserved-only mode (#8650) Charge deposit based on key length (#8648) [pallet-revive] make subscription task panic on error (#8587) tx/metrics: Add metrics for the RPC v2 `transactionWatch_v1_submitAndWatch` (#8345) Bridges: Fix - Improve try-state for pallet-xcm-bridge-hub (#8615) Introduce CreateBare, deprecated CreateInherent (#7597) Use hashbrown hashmap/hashset in validation context (#8606) ci: rm gitlab config (#8622) 🔪 flaky and Zombienet tests (#8600) cumulus: adjust unincluded segment size metric buckets (#8617) Benchmark storage access on block validation (#8069) Revert 7934 es/remove tj changes (#8611) collator-protocol: add more collation observability (#8230) `fatxpool`: add fallback for ready at light (#8533) txpool: fix tx removal from unlocks set (#8500) XCMP weight metering: account for the MQ page position (#8344) fix epmb solution duplicate issue + add remote mining apparatus to epm (#8585) Fix generated address returned by Substrate RPC runtime call (#8504) ...
Backport #8473 into `stable2503` from yrong. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> --------- Co-authored-by: Ron <[email protected]> Co-authored-by: Egor_P <[email protected]>
The `TokenIdOf` [convert](https://github.com/paritytech/polkadot-sdk/blob/4b83d24f4bc96a7b17964be94b178dd7b8f873b5/bridges/snowbridge/primitives/core/src/location.rs#L40) is XCM version-agnostic, meaning we will get the same token ID for both V5 and legacy V4 asset. However, the extra check is unnecessary, as the`ConvertAssetId::convert(&token_id).ok_or(InvalidAsset)?;` alone is sufficient to verify whether the token is registered.
Backport paritytech#8473 into `stable2503` from yrong. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> --------- Co-authored-by: Ron <[email protected]> Co-authored-by: Egor_P <[email protected]>
|
@acatangiu I think this change makes the system be less flexible to errors to win only one less read when doing the conversions. I think the conversion from I am going to put a few examples in which things can go wrong:
In any of those cases the hash of the token-location will not be the same anymore. meaning that you lose the opportunity to transfer the tokens to ethereum. I understand this is coming from the fact that all conversions from
I think this is less prone to errors and adds much more flexibility than the current scenario. I am sorry I did not realize about this change before. I could have provided earlier feedback |
all of the above are deeply destructive, breaking changes :) if any of them is attempted maaany things will break, not just this ethereum token id conversion (think thousands of derived accounts, derived locations, and all the hardcoded stuff off-chain) having said that, I am not against making |
The Token ID (in V4) ↔ Address mapping is already persisted on Ethereum. Since V5 is fully compatible with V4, this should be fine. If any of the XCM primitives used to generate the ID were to change, as you mentioned, we could consider performing a storage migration in both the System Pallet and on the Ethereum side to ensure both ID formats are recognized and map to the same token address. That said, I strongly recommend avoiding this if possible, for the reasons Adrian mentioned. @girazoki Besides that, I’m also curious — did you encounter any specific issues, or are there changes you want to make that the current implementation is preventing? |
I agree that those are destructive changes. However they are no the first time that I see them :). the location change for assethub assets already happened in the past, possibly with the system not being mature enough and therefore without too many catastrophic consequences. But my point is that with the current mechanism in place, those destructive changes are out of the table. In the case of
I am happy to implement these changes if you guys agree. The point of them would be to tie the fate of the location to token-id to whatever |
@acatangiu @yrong how does this sound? do u want me to open a draft PR (without the migration) so that u guys see what it looks like? |
Cool. I’m currently on vacation and will be back early next week. I’ll review it then. |
The `TokenIdOf` [convert](https://github.com/paritytech/polkadot-sdk/blob/4b83d24f4bc96a7b17964be94b178dd7b8f873b5/bridges/snowbridge/primitives/core/src/location.rs#L40) is XCM version-agnostic, meaning we will get the same token ID for both V5 and legacy V4 asset. However, the extra check is unnecessary, as the`ConvertAssetId::convert(&token_id).ok_or(InvalidAsset)?;` alone is sufficient to verify whether the token is registered.
The
TokenIdOfconvert is XCM version-agnostic, meaning we will get the same token ID for both V5 and legacy V4 asset.However, the extra check is unnecessary, as the
ConvertAssetId::convert(&token_id).ok_or(InvalidAsset)?;alone is sufficient to verify whether the token is registered.