[asset-hubs] Investigate usage of MultiLocation vs VersionedMultiLocation

bkontur commented 1 year ago

Summary

Investigate and setup proper usage of MultiLocation for storage items.

Problem 1 - ForeignAssets - asset_id as `xcm::prelude::MultiLocation`

MultiLocation is used as pallet_assets::AssetId.

// AssetId type definition
pub type MultiLocationForAssetId = xcm::prelude::MultiLocation;

// ForeignAssets pallet instance
pub type ForeignAssetsInstance = pallet_assets::Instance2;
impl pallet_assets::Config<ForeignAssetsInstance> for Runtime {
  ...
    type AssetId = MultiLocationForAssetId;
  ...
}

//  `pallet_assets::AssetId` used by storage item
pub(super) type Asset<T: Config<I>, I: 'static = ()> = StorageMap<
        _,
        Blake2_128Concat,
        T::AssetId,
        ...
    >;

Problem 2 - asset-conversion uses `Box<xcm::opaque::v3::MultiLocation>` for storage item

See asset conversion pallet MultiAssetId definition:

impl pallet_asset_conversion::Config for Runtime {
...
    type MultiAssetId = Box<xcm::opaque::v3::MultiLocation>;
...
}

// MultiAssetId is used for PoolId
pub(super) type PoolIdOf<T> = (<T as Config>::MultiAssetId, <T as Config>::MultiAssetId);

// PoolId is used by storage item 
pub type Pools<T: Config> =
        StorageMap<_, Blake2_128Concat, PoolIdOf<T>, PoolInfo<T::PoolAssetId>, OptionQuery>;

Problem 3 - asset-conversion uses `Box<xcm::opaque::v3::MultiLocation>` for public runtime api

See more description here.

Questions:

How should we handle MultiLocation vs VersionedMultiLocation in proper way?

TODO

[ ] should we change xcm::prelude::MultiLocation to xcm::VersionedMultiLocation ?
- [ ] prepare some migration for that?
[ ] add some test that ensures compatibility of actually used xcm::v3::MultiLocation against xcm::prelude::MultiLocation ?

joepetrowski commented 1 year ago

We have to be careful here, because if/when XCMv4 comes out, it will be a huge pain if all the asset IDs change. Hopefully the MultiLocation data structure does not change with future versions as it would cause the meaning of the Asset ID to change as well.

add some test that ensures compatibility of actually used xcm::v3::MultiLocation against xcm::prelude::MultiLocation

This seems sensible in any case.

franciscoaguirre commented 1 year ago

I also wrote in https://github.com/paritytech/polkadot-sdk/pull/1230#discussion_r1338731874 but we should definitely use Versioned* types for storage items

bkontur commented 1 year ago

@franciscoaguirre ok, thank you, I will continue this direction with PoC for VersionedLocation,

We just need to think that this two locations below are not the same - they point to the same but have different encodings because of V3/V4:

let location1 = VersionedMultiLocation::V3(v3::MultiLocation::new(1, v3::Junctions::Here));
let location2 = VersionedMultiLocation::V4(v4::MultiLocation::new(1, v4::Junctions::Here));

So maybe we will need something like LatestVersionedMultiLocation in pallet_xcm.

And the other thing is that xcm stuff like FungiblesTransferAdapter works with use xcm::latest::prelude::*; and if we use Versioned(Multi)Location for pallet_assets::Config::AssetId = VersionedMultiLocation it could start to be interesting. Maybe we will use LatestVersionedMultiLocation-like feature instead of VersionedMultiLocation. Will see :)

franciscoaguirre commented 1 year ago

If we use VersionedLocation across the board, we can always convert it into the latest version before dealing with it. The main issue is/was encoding and decoding. v3 should almost always be able to convert to v4, and we always expect users to upgrade to the latest version.

muharem commented 1 year ago

I think there are different cases, for example for treasury AssetKind which stored together with spend proposal in the storage, I used VersionedMultiLocation, because a spend proposal will eventually expire and removed from the storage, so we can avoid storage migrations when new xcm version comes. And we wont have to support some old xcm versions.

In case with assets pallet, I see it different way. Some assets will be there for a while, and if we use versioned type, that asset can be identified only be specific version and that xcm version has to be supported "forever". I think for this case it is more reasonable to have some generic storage migration, letting as to do that migration every time we migrate to new xcm version.

In addition we can have some label for the new version which can tell the clients if encodings are changed with new version. And tests proofing it.

bkontur commented 1 year ago

In case with assets pallet, I see it different way. Some assets will be there for a while, and if we use versioned type, that asset can be identified only be specific version and that xcm version has to be supported "forever". I think for this case it is more reasonable to have some generic storage migration, letting as to do that migration every time we migrate to new xcm version.

yeah, this seems simple way to go, but maybe not used xcm::latest::MultiLocation but instead use exact version xcm::v3::MultiLocation and add compatiblity test to the AssetHub runtimes which check at least several predefined expected MultiLocations encoding for actual used xcm::v3 vs xcm::latest, if test fails, we need to apply migration.

But I am not sure about consequences and how this could affect e.g. external wallets, if versions wont be compatible. If we use VersionedMultiLocation, we also need to add migration to the Latest because older versions are removed.

bkontur commented 9 months ago

Just some hints for the future (when XCMv5 will come):

What about changing pallet_assets:

- type AssetIdParameter: Parameter + From<Self::AssetId> + Into<Self::AssetId>
+ type AssetIdParameter: Parameter + TryFrom<Self::AssetId> + TryInto<Self::AssetId>

This could escalate to enable setting up the runtime like this:

impl pallet_assets::Config<ForeignAssetsInstance> for Runtime {
   ...
    type AssetId = xcm::v3::Location;
    type AssetIdParameter = VersionedLocation;
   ...

AssetIdParameter is utilized for extrinsic parameters. Therefore, if it were VersionedLocation, external users such as wallets wouldn't need to make any changes when we release a new XCM version. This resolves only half of the problem.

The second problem concerns data retrieval. I'm unsure about how wallets are currently accessing or reading data, such as ForeignAssets. If they directly read from storage, they might encounter issues when we release a new XCM version with a different xcm::v*::Location encoding.

However, for data retrieval, we have the runtime API, which already utilizes VersionedAssets. This runtime_api is aware of new XCM versions, so there shouldn't be any issues:

impl assets_common::runtime_api::FungiblesApi<Block, AccountId,> for Runtime {
    fn query_account_balances(account: AccountId) -> Result<xcm::VersionedAssets, assets_common::runtime_api::FungiblesAccessError> {

xlc commented 9 months ago

dapps are usually subscribing to storage directly because it allows subscription, which is not yet possible for runtime API calls

bkontur commented 8 months ago

dapps are usually subscribing to storage directly because it allows subscription, which is not yet possible for runtime API calls

@xlc something like this https://github.com/paritytech/polkadot-sdk/issues/3594?

paritytech / polkadot-sdk