paritytech / polkadot-sdk

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

XCM executor should use transfer over withdraw/deposit #738

Open xlc opened 1 year ago

xlc commented 1 year ago

XCM executor is usually using withdraw and deposit to for asset handling. This has two issues:

Acala supports transfer ERC20 to other parachain with XCM. However we have to implement some relative unsafe code to make it work.

It will be good if the implementation uses transfer for most of the token operation and withdraw/deposit only absolutely necessary, and I cannot think of any reason why XCM executor should be allowed to mint/burn token.

Polkadot-Forum commented 1 year ago

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

https://forum.polkadot.network/t/how-to-handle-not-withdrawable-assets-properly-with-xcm/2270/7

gavofyork commented 1 year ago

There is a TransferAsset instruction for when a transfer is meant by the message sender.

xlc commented 1 year ago

XCM executor should not be able to mint/burn token. It is just a bad idea. Right now to my understanding, XCM executor will burn trapped assets and mint them back on claim. This will impact inflation logic for example. Similar issue happens on teleport and the solution is checking account. We need to apply similar solution for trapped assets.

It may be necessary on XCM instruction level to have the ability to withdraw/deposit assets. However on the execution level, it shouldn't require the underlying token have the ability to withdraw/deposit. In fact, I do not want XCM executor to be able to mint token. This is just another attack vector that bad XCM executor implementation could be able to completely destroy a token.

xlc commented 1 year ago

It also does not make sense to require the sender to understand the capability of an asset and determine to use transfer or withdraw/deposit based on that. We should abstract the complex logics and ensure XCM are easy to create and not error-prone.

gavofyork commented 1 year ago

XCM is designed to be general. Any model which allows tokens from one chain to be represented in some way on another requires some sort of "issuance" and "de-issuance" capability. XCM provides this as a general mechanism with DepositAsset and WithdrawAsset and the associated traits used by xcm-executor. You can implement these however you like.

If you never want your chain to be able to do this, I suggest you either implement your own xcm-executor which avoids these actions or introduce additional wrapping adapter around the tokens adapters to disable this behaviour. Unless you're being rather clever, this will break some typical patterns used in XCM.

Ultimately, XCM as a realised system does have a set of design decisions and trade-offs inherent to them. If you feel these trade-offs are so bad as to make XCM unfit for your usage, I'd suggest you propose and implement your own inter-chain language standard. Our message-passing infrastructure is entirely independent of the language used upon it and it should be fairly trivial to support multiple high-level message formats.