pallet-xcm transfer_assets should take fees separately instead of using the index in assets

[franciscoaguirre]

transfer_assets' signature looks like this:

pub fn transfer_assets(
	origin: OriginFor<T>,
	dest: Box<VersionedLocation>,
	beneficiary: Box<VersionedLocation>,
	assets: Box<VersionedAssets>,
	fee_asset_item: u32,
	weight_limit: WeightLimit,
) -> DispatchResult;

The idea of passing the fee_asset_item index is not going to cut it anymore, because of various reasons:

• Assets are ordered when converted from a vector, so the index might be wrong
• We want users to specify the max assets they're willing to spend for fees only, this means it's preferable to separate assets even if they are the same one. If passing only one assets instance, two instances of the same asset will be merged into one.
• Fees are something completely different from the assets that the user wants to transfer, so they should be clearly separated.

The solution is to change the fee_asset_item parameter with a new parameter of type Assets. The signature would look like so:

pub fn transfer_assets(
	origin: OriginFor<T>,
	dest: Box<VersionedLocation>,
	beneficiary: Box<VersionedLocation>,
	assets: Box<VersionedAssets>,
	fees: Box<VersionedAssets>,
	weight_limit: WeightLimit,
) -> DispatchResult;

This is clearly a breaking change, so a new extrinsic would have to be created and this one deprecated.
The same situation happens with limited_reserve_asset_transfer and limited_teleport_assets.

[acatangiu]

+1

the new extrinsic I'm adding here is already following this proposed approach: #3695

The inner logic for buying execution or generally paying fees is using single asset, and with asset conversions available that should still be enough. So the new API will probably just use fees: Box<VersionedAsset> (singular, not plural).

Regarding the deprecations and adding new extrinsics, we might get away with just doing it for generic/flexible transfer_assets and maybe long-term completely dropping limited_reserve_transfer_assets and limited_teleport_assets.

[franciscoaguirre]

It might be useful putting more than one type of asset, for example for paying for execution with one asset and for delivery with another one.
Execution can be paid with multiple assets with the new Traders we have, but delivery is still constrained to only one type of asset per sender.

Regarding conversion, you might also not want to convert if you already have the correct assets, since it'd be more expensive to convert.

I think Box<VersionedAssets> would allow us to not have to change this in the future. We can enforce len() == 1 at first but then without breaking the API allow multiple assets.

[xlc]

I feel this complexity is getting out of control... I don't know what exactly is broken and what do we need to do but it is clear to me something is not right. I was going to improve xtokens the other day to support some more use cases but it is just so complicated to the point I don't know if we should continue on this direction.

[franciscoaguirre]

What complexity? Using the index of the assets has a bunch of problems. This issue just wants to simplify and have one parameter for the assets you actually want to transfer and another one for the fees.

[xlc]

My rant isn’t exactly about this issue but XCM as a whole. This issue is one example of the complexity of XCM transfers.

Ideally to make XCM transfer, we just need to specify dest and the assets. But we also need to deal with type of transfer and fees and weights etc.

[franciscoaguirre]

Yeah I agree.

I think extrinsics that figure out the type of transfer for the user are the right way to go. I think the complexity is at the right level of the stack though. Different models of transfers do exist, depending on the trust assumptions and so on, we can only choose who deals with that complexity. Right now it's pallets like pallet-xcm and xtokens.

Regarding fees, I hope this complexity gets taken from the parameters of the calls and we can just deal with them transparently. It shouldn't be that the user has to think about fees. Fees are just something that gets deducted and you're good to go. The solution right now is to expose it but I hope in the future we can remove it.