paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.network/
1.62k stars 562 forks source link

Asset Transactor on Asset Hubs not Working With Sufficient Assets #3958

Open joepetrowski opened 2 months ago

joepetrowski commented 2 months ago

An account on AH should be able to transact using only sufficient assets (and not require DOT/KSM). However, something in the XCM Executor is preventing this right now.

Here is a local transfer that worked (using USDT as the fee asset to transfer USDT): https://assethub-polkadot.subscan.io/extrinsic/5982448-2

Here is an attempt to use XCM to send USDT to Moonbeam, which failed: https://assethub-polkadot.subscan.io/extrinsic/5982488-2

However, an account with KSM succeeded (constructing the transaction the same way): https://assethub-kusama.subscan.io/extrinsic/6717729-2


Notes:

cc @muharem @franciscoaguirre

muharem commented 2 months ago

I could reproduce it locally, and the issue is that the fees which should be paid for xcm message delivery (e.g. xcm message sent to deposit reserved funds to a receiving parachain), right now can be paid only in a native currency. XcmRouter which implements SendXcm trait and provides a price for the message delivery, does not take into consideration which assets are provided by user and only attempts to change in a native currency.

In the third example provided above, the sender has some KSMs on it's account, and the fees for the message delivery paid in KSM.

acatangiu commented 2 months ago

Charging delivery fees out of band bites us again 👹

https://github.com/paritytech/polkadot-sdk/issues/3434 will eliminate the problem completely, but that's XCMv5

should we try to fix it in XcmRouter in the meantime? we could go through asset conversion during charge_fees and charge native but swap whatever for that native..

muharem commented 2 months ago

@acatangiu I do not think it can be done just with a different implementation of existing contracts, I think the contract should be changed, to be similar to FeeManager. @franciscoaguirre should know better.

franciscoaguirre commented 2 months ago

I knew it'd be an issue that delivery fees use a completely different mechanism from weight and traders. We already have a good story around those two, now we need to integrate delivery fees into the mix.

This could be fixed in the meantime in the router, since we specify the asset id for paying fees there. We would need to swap USDT for DOT for example.

Polkadot-Forum commented 2 months ago

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/sufficiency-on-assethub/7095/23

muharem commented 2 months ago

I knew it'd be an issue that delivery fees use a completely different mechanism from weight and traders. We already have a good story around those two, now we need to integrate delivery fees into the mix.

Be weight mechanics you mean - WeightTrader, and by traders - FeeManager, right? It would be nice if we can use the same tools like SwapFirstAssetTrader in both cases.

This could be fixed in the meantime in the router, since we specify the asset id for paying fees there. We would need to swap USDT for DOT for example.

We can set it as USDT or DOT, or any other asset, but it can be only one of them. And it does not know what a user have or willing to use as an asset to cover the fee.

joepetrowski commented 2 months ago

How come it can only be one? One of the main ideas of Asset Conversion was that the system technically only accepts one asset (DOT) for fee payment, but you can give it any asset, and if it's not DOT, then it will swap that asset for DOT. Maybe I am thinking at the wrong layer of the call stack, but there should be a "give it anything" entry point and then DOT should come out.

muharem commented 2 months ago

If I understood @franciscoaguirre right, in that sentence he is proposing a quick fix with no changes to the current contract. the current contracts lets as only set it to one asset, which is DOT now

Polkadot-Forum commented 1 month ago

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/asset-hub-brainstorming-session-call-notes-and-discussion-continuation/7260/5

franciscoaguirre commented 1 month ago

@muharem @joepetrowski I have the following PR tackling this: https://github.com/paritytech/polkadot-sdk/pull/4375

It's still work in progress for now. It breaks some API things but pretty minimal. I already have some tests working for transfering only USDT or only a sufficient foreign asset. I'm still fleshing out some other cases.

Polkadot-Forum commented 1 month ago

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/xcm-user-and-developer-experience-improvements/4511/21

Polkadot-Forum commented 1 month ago

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/asset-hub-brainstorming-session-call-notes-and-discussion-continuation/7260/7