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.
@franciscoaguirre are there perhaps other ways to check asset ID equivalence, across different XCM versions?
There was a problem hiding this comment.
Actually, I assume the check here is unnecessary, as the conversion alone is sufficient to verify whether the token is registered.
yrong
changed the title
acatangiu
added
T15-bridges
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.
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.
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.
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.
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.
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.
|
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.
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.
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.
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.
I prefer not to use VersionedLocation in storage either. I want highly normalized, consistent data in storage.
There was a problem hiding this comment.
Reverted recent updates.
| fn convert_back(_: &Location) -> Option<TokenId> { | ||
| None |
There was a problem hiding this comment.
why use MaybeEquivalence if you don't provide convert_back functionality?
Can you use TryConvert/MaybeConvert instead?
There was a problem hiding this comment.
To keep this PR focused, I've opend a separate PR for the cleanup and refactoring work, which isn't high priority.
Yeah, and I've created a runtime migration in polkadot-fellows/runtimes#730 |
yrong
force-pushed
the
remove-asset-location-check
branch
from
62946bb to
697f4ed
Compare
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 will break, as we store the V4 location in storage.Therefore, we should remove this check for compatibility.