Skip to content
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

Enhance interoperability for pallet-assets #5969

Open
bkontur opened this issue Oct 8, 2024 · 2 comments
Open

Enhance interoperability for pallet-assets #5969

bkontur opened this issue Oct 8, 2024 · 2 comments
Labels
T6-XCM This PR/Issue is related to XCM.

Comments

@bkontur
Copy link
Contributor

bkontur commented Oct 8, 2024

Relates to old comment: #1130 (comment)
Relates to: #4826 (new XCMv5)

Potential problem

Let's imagine a simple scenario where a sibling parachain of AssetHub wants to create/register an asset on AssetHub's foreign assets instance, which uses xcm::v3::Location as the AssetId:

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

This means that the sibling needs to construct an XCM Transact with an encoded call that uses xcm::v3::Location:

Xcm([Transact {
	origin_kind: ...,
	require_weight_at_most: ...,
	call: pallet_assets::Call::create {
		id: xcm::v3::Location::new(1, Parachain(...), ...),    // <- XCMv3     
		admin: ...,
		min_balance: ...,
	}.encode()                                                     // <- encoded
}]),

Now, let's imagine we upgrade AssetHub to use xcm::v4::Location. At this point, the sibling parachain could encounter problems if it doesn't upgrade at the same time as AssetHub. And also upgrading at the "same" time is not 100%, because there could be some (e.g. HRMP) messages on the way. The real issue could arise if the new XCM version used by AssetHub has encoding/decoding that is incompatible with the older version. Essentially, AssetHub would be unable to decode the Transact's encoded call and would discard the messages.

Solution

I suggest 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 used for extrinsic parameters. Therefore, if it were VersionedLocation, external users, such as wallets or sibling parachains, wouldn’t need to make any changes when a new XCM version is released.
Additionally, there is an unwritten consensus that we should use VersionedLocation for extrinsic parameters.

Yes, this solution is a kind of 'best-effort' approach and assumes that we can convert/decode older XCM Location versions to the new one.

@bkontur bkontur added the T6-XCM This PR/Issue is related to XCM. label Oct 8, 2024
@bkontur bkontur changed the title Enhance interoperability for pallet_assets Enhance interoperability for pallet-assets Oct 8, 2024
@xlc
Copy link
Contributor

xlc commented Oct 8, 2024

there is an unwritten consensus that we should use VersionedLocation for extrinsic parameters

someone please make this a written requirement

@acatangiu
Copy link
Contributor

there is an unwritten consensus that we should use VersionedLocation for extrinsic parameters

someone please make this a written requirement

yes, @bkontur please create an "epic" along the lines of "Use versioned parameters across all FRAME pallets"

this issue concerning only pallet-assets would fall under that

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: Todo
Development

No branches or pull requests

3 participants