Implement transfer_asset on CurrencyAdapter

[KiChjang]

Fixes #4548.

This makes it so that a tuple of (CurrencyAdapter, FungiblesAdapter) will attempt to call the transfer_asset method on CurrencyAdapter first and see if it fails, before moving onto FungiblesAdapter.

Without this change, the transfer_asset method on CurrencyAdapter uses the default implementation, which returns an XcmError::Unimplemented and is thus skipped, which may not be the desired behaviour.

[xlc]

Should have some test to improve the poor coverage bit more.

[shawntabrizi]

yeah, looks totally sensible

[gavofyork]

So this "issue" comes about due to the transfer_asset/beam_asset duality. Basically, transfer_asset wasn't really meant to be used directly - it's meant to implement a "correct" transfer - i.e. one which the backend actually implements as a transfer internally.

Instead, the API to be used was really beam_asset: this did a transfer by any means necessary - meaning that it would try to use transfer_asset (the "correct" way of doing it) and that was unimplemented then fall back on a withdraw_asset and deposit_asset combination. If withdraw_asset (or transfer_asset) returned an AssetNotFound error, then it would propagate that to the tuple instance which would allow it to move on to the following impls.

I suspect if we renamed beam_asset to transfer_asset and then transfer_asset to native_transfer_asset, we might end up fixing the problem without introducing unneeded implementations.

[xlc]

maybe we want the rename, but I think we still want the native transfer asset implemented when it is possible as an optimization. it also generates a nicer event

[KiChjang]

I agree that we should implement native_transfer_asset for the currency adapter -- using Currency::transfer instead of a Currency::withdraw and a Currency::deposit sounds like it is exactly a "correct" internal transfer method for the backend. I might even call it internal_transfer_asset instead of native_transfer_asset as I think it may reflect the intention better.

[apopiak]

Just a quick note that my impression is that this blocks reserve transfers of KSM out of Statemint/e so we might want to resolve it.

[gilescope]

Keith says this is ready for review again. It would be great to have this land, so that people could do limited teleport assets from one parachain to another.

[shawntabrizi]

Still makes sense to me from the perspective of optimizing the internal process of doing a transfer, and generating a better event.

[gavofyork]

I don't know why I didn't implement the transfer_native function myself for Currency impls. That bothers me a little. But I don't see anything wrong with the logic.

[KiChjang]

bot merge