Transfer with Transact Design Discussion

[ghzlatarev]

Hi guys, we would like to propose a new feature for xtokens and want to start a discussion to see if it is reasonable and can be worked on.

Given that users can privatize any asset on our chain, we want to enable them to move cross-chain assets to our chain and privatize them in one call. Our idea is to allow users to send, along with token transfers, a Transact instruction that includes the encoded privatization extrinsic. Moving some assets and using them in an extrinsic with one call seems like something that can be useful for other use-cases as well.

The design that we've come up with so far is the following:

1. Add new transfer_with_transact extrinsic that will use existing token transfer logic and sends a message to move some assets:
   a. for the encoded call in the Transact instruction (our case to be privatized)
   b. to pay for execution fees
2. All the assets will be deposited to an account created by converting the MultiLocation of the sender user account on the sender chain (from the viewpoint of the receiver chain) with Account32Hash or whatever converter is implemented on the receiver chain.
3. Then an additional xcm message will be sent: (DescendOrigin + WithdrawAsset + BuyExecution + Transact)
   a. first descend the origin to the account from step 2
   b. withdraw the fee assets from 1.b. to holding
   c. buy execution with the assets in holding
   d. Transact the encoded manta-pay extrinsic to privatize the assets from 1.a.
   e. Change can be deposited back to the same account or a different account

Here's a very rough shot implementation of the idea for reference https://github.com/Manta-Network/open-runtime-module-library/pull/4/files
Also some impl details:

1. For starters we can limit the new extrinsic where:
   a. asset is a CurrencyId and fee is the same CurrencyId.
   b. asset and fee are withdrawn separately from sender.
2. The receiver account which will also dispatch the Transact call, should be the origin of the extrinsic on the sender chain:
   a. self_location.append_with(sender_account_origin).reanchor(destination_chain, ancestry)
   b. the above destination won't be valid according to the dest != self_location check
   c. to work around 2.b. we need to construct a "fake" destination to pass the check
   d. then use the maybe_recipient_override argument to override with the "real" destination from 2.a.
3. Ideally we want to deposit change to any account on the destination chain
   a. it would be easier to do a refund to the destination from 2.a.
   b. we can start off without depositing any change

[ghzlatarev]

@xlc @zqhxuyuan @shaunxw @stechu

[zqhxuyuan]

If DescendOrigin is first xcm instruction, I think current Barriers not support it. We've some same needs before: #651

[ghzlatarev]

If DescendOrigin is first xcm instruction, I think current Barriers not support it. We've some same needs before: paritytech/polkadot#651

Right, the DescendOrigin will need a different Barrier, which would be left for the receiver chain to implement.
651 looks exactly that, but should also allow a customizable set of sibling parachain locations. In our use case the messages can come direct from another parachain.

[KiChjang]

I'm a bit hesitant to add an API that allows any signed account to simply provide an encoded call and then have it be executed on a foreign chain. It just sounds to me that there's a high chance that it could be exploited in ways that we don't quite yet understand, the one that I could think of is via a bad runtime upgrade that accidentally added a new dispatchable to modify important state info without having proper access controls. Further, since work is underway to provide an upper limit to all Vecs accepted in call parameters and storage items, it's becoming a bad idea to accept an unbounded Vec<u8> as an argument to any dispatchable function.

I also feel like there's a significant overhead for sending 2 XCMs instead of one, and indeed from the changes linked in the description, it requires adding an XcmSender as a configurable on the ORML XCM pallet, which did not exist before. I hear that the reason was because of the ClearOrigin instruction which makes it impossible to combine the two XCMs into one, and the answer to that is simply to use XCMv3. I don't remember exactly if it would be using DescendOrigin instead of ClearOrigin, but if so, then you can definitely roll the 2 XCMs into 1. splitting into 2 XCMs, one for the reserve and the other for the destination, is the only way in which we can get this to work.

In general though, I don't feel like a transfer with a Transact instruction is something that should be abstracted and generalized into a template. I understand that it's supposed to be general purpose -- I get the general part but not the purpose part, because it would seem to me that it's no different from a xcmPallet.send, other than the fact that it uses a more privileged XCM origin, which should really be handled with more care. It also seems to me from a fee perspective that it's more beneficial to a user if they do a transfer and interact directly with the chain to submit the dispatchable, rather than through the transfer and transact dispatchable, because it would skip the fees payable to the sending chain.

[stechu]

Hi @KiChjang, Thanks for the feedback, this is really helpful.

Thanks for the XCM V3 and Bounded Vec suggestions. We should definitely leverage them. Here let's address the two major points first.

1. Motivation
2. Security

Motivation

In general though, I don't feel like a transfer with a Transact instruction is something that should be abstracted and generalized into a template. I understand that it's supposed to be general purpose -- I get the general part but not the purpose part, because it would seem to me that it's no different from a xcmPallet.send, other than the fact that it uses a more privileged XCM origin, which should really be handled with more care. It also seems to me from a fee perspective that it's more beneficial to a user if they do a transfer and interact directly with the chain to submit the dispatchable, rather than through the transfer and transact dispatchable, because it would skip the fees payable to the sending chain.

Our motivating application is to streamline the privatization of parachain assets. For example, using transfer_and_transact, a user could 1-click privatize her assets in Acala to private assets. However, the logic is quite general. For example, if a user want to 1-click sending KMA to Acala/Moonbeam/Astar and exchange for some DOT, the same logic could apply.

You can argue that we can always compose these transactions at the front-end/dApp level. However, composing at runtime level has the great advantage of latency and reducing complexity for dApp development. Since these operations are streamlined. dApp developers doesn't need to handle the complexity of composing and waiting for the first extrinsic finish and then sending the second one. Here, we would like to at least to provide a choice to compose at runtime level.

Security

I'm a bit hesitant to add an API that allows any signed account to simply provide an encoded call and then have it be executed on a foreign chain. It just sounds to me that there's a high chance that it could be exploited in ways that we don't quite yet understand, the one that I could think of is via a bad runtime upgrade that accidentally added a new dispatchable to modify important state info without having proper access controls. Further, since work is underway to provide an upper limit to all Vecs accepted in call parameters and storage items, it's becoming a bad idea to accept an unbounded Vec as an argument to any dispatchable function.

This is a very good point, in @ghzlatarev 's original post, he didn't elaborate on the security aspect of the design. The key security guarantees of the design are as follows (BTW, these points are precisely why we want to implement in ORML and get the code potentially audited):

1. transfer_and_transact assume that the destination chain will create a derivative account using Account32Hash upon received the transfer, and the transferred token will be hold in this derivative account. Essentially this is a hash of sender's multi-location.

2. In the XCM message that transfer_and_transact send, the code ensures that OriginKind in the Transact instruction is SoverignAccount, and then assume a reasonable implemented ConvertOrigin should covert it to the "correct" multilocation, i.e, the default one that polkadot uses (https://github.com/paritytech/polkadot/blob/master/xcm/xcm-builder/src/origin_conversion.rs#L30).

Transact { origin_type, require_weight_at_most, mut call } => {
    // We assume that the Relay-chain is allowed to use transact on this parachain.
    let origin = self.origin.clone().ok_or(XcmError::BadOrigin)?;
    // TODO: paritytech/polkadot#2841 #TRANSACTFILTER allow the trait to issue filters for the relay-chain
    let message_call = call.take_decoded().map_err(|_| XcmError::FailedToDecode)?;
    let dispatch_origin = Config::OriginConverter::convert_origin(origin, origin_type)
                                        .map_err(|_| XcmError::BadOrigin)?;`

and in the AssetTransactor implementation, the Convert(Multilocation, Account) implementation did the "correct" thing to convert the Multilocation to the derivative account.

I think these are kind of strong assumptions that we need to enforce to make sure the current transfer_and_transact design is secure. We would like to have your idea on what is the proper way to enforce and handle these assumptions or what is a good alternative design here.

[KiChjang]

What I would argue instead is whether you can think of a way to introduce a new XCM instruction that sidesteps this issue. Remember, any XCM instruction is capable of having different semantics determined by the receiving chain, so if this is something that you feel like it can be generalized, then perhaps it's a really good idea to add such an operation at the XCM instruction level, rather than trying to operate around the current security constraints and instruction set. I don't have a good name for the operation that you are trying to introduce, but I will say that there is already an ExchangeAsset instruction.

[stechu]

What I would argue instead is whether you can think of a way to introduce a new XCM instruction that sidesteps this issue. Remember, any XCM instruction is capable of having different semantics determined by the receiving chain, so if this is something that you feel like it can be generalized, then perhaps it's a really good idea to add such an operation at the XCM instruction level, rather than trying to operate around the current security constraints and instruction set. I don't have a good name for the operation that you are trying to introduce, but I will say that there is already an ExchangeAsset instruction.

I think the fundamental issue of current XCM design that prevents an elegant solution for something like this is the forcefully ClearOrigin in every instruction on the executor and doesn't provide a proper (AFAIK) way to let authorization of low privileged transaction happens naturally. I think there is a moonbeam-LIDO usecase that requires two consecutive XCM calls as well. The Transact interface itself is general enough, the problem is many usecases requires a kind of Delegated execution with proper privilege setting.

[girazoki]

Hi, hadnt seen this thread before but I would like to share my thoughts on this matter:

• In general, regardless whether its tied to a Transfer or not, I think having a Transact extrinsic (that would be composed of DescendOrigin + WithdrawAsset + BuyExecution + Transact) opens a whole new set of possibilities. For instance, this allows a smart contract in parachain A to interact with a smart contract in parachain B. We have an extrinsic in Moonbeam that does something similar (only in our test networks for now) https://github.com/PureStake/moonbeam/blob/1f1b300af5c3ae3695e7666712aed5c5af4dcb3e/pallets/xcm-transactor/src/lib.rs#L532.

• With respect to the security side of things, I still cannot imagine a scenario in which this exploits an extrinsic
  that is not exploitable itself by regular means. The only part where I see a bit of friction is the LengthToFee mechanisms we have for non-xcm usage, which would be skipped in the case of XCM (since only Weight is taken into account for XCM). DescendOrigin is supposed to take charge of making the sovereign account secure. Indeed, correct me if I am wrong but rococo already allows to do this https://github.com/paritytech/polkadot/blob/56e9b675407d33b34bfde4c28032386306b901fb/runtime/rococo/src/xcm_config.rs#L155 via pallet-xcm.

• In the moonbeam-lido case, we dont do 2 xcm messages. First, the token you are going to use to pay for the xcm fees is burned in Moonbeam. Second, a xcm message is sent to the relay chain in the form WithdrawAsset | BUyExecution | Transact. But we wrap the transaction to be done in an as_derivative call. Essentially, Lido is using a derivative address from the Moonbeam sovereign address in the relay chain.

• We are working towards making frontier also compatible with XCM-transact calls (see Ethereum Transact Xcm polkadot-evm/frontier#733). We consider this a key-usecase for the interoperability narrative of the polkadot ecosystem.

[KiChjang]

Just to quickly add, for the user to 1-click transfer assets, I view this as a UX problem rather than an XCM issue. I don't see why XCM interactions should correlate 1-to-1 with user actions.

[KiChjang]

Also, I now recall that ClearOrigin is not in fact required to construct an XCM that passes AllowTopLevelPaidExecutionsFrom -- a missing ClearOrigin is also permitted.

[ghzlatarev]

Thanks for the input guys! Some more thoughts from my side:

Just to quickly add, for the user to 1-click transfer assets, I view this as a UX problem rather than an XCM issue. I don't see why XCM interactions should correlate 1-to-1 with user actions.

Indeed this is a UX issue, and our thinking is that it will probably have to be dealt with on different front-ends, so that's why it could be beneficial to have the solution in xtokens directly.

Also, I now recall that ClearOrigin is not in fact required to construct an XCM that passes AllowTopLevelPaidExecutionsFrom -- a missing ClearOrigin is also permitted.

The issue with ClearOrigin is not the barrier. The default xcm-executor code attaches ClearOrigin when it handles instructions used by xtokens extrinsics like DepositReserveAsset or InitiateReserveWithdraw - https://github.com/paritytech/polkadot/blob/master/xcm/xcm-executor/src/lib.rs#L398-L421 So I cannot descend and create a unique origin for the Transact dispatchable. So that's why the second xcm message is needed. But if I understood your previous comment correctly, the XCM v3 PR will resolve this issue.

In the moonbeam-lido case, we dont do 2 xcm messages. First, the token you are going to use to pay for the xcm fees is burned in Moonbeam. Second, a xcm message is sent to the relay chain in the form WithdrawAsset | BUyExecution | Transact. But we wrap the transaction to be done in an as_derivative call. Essentially, Lido is using a derivative address from the Moonbeam sovereign address in the relay chain.

This is an example where one wouldn't need the second XCM call, the amount is burned on Moonbeam's side and withdrawn from Moonbeam's sovereign account on the relay chain and used by a derivative of the sovereign account. However this assumes that there already was a reserve transfer from relay to Moonbeam. If I'm doing a self_reserve transfer (as it's called in xtokens) this won't work, as I won't be able to withdraw anything from the sovereign account of the sender chain on the receiver chain, since there are no assumed prior transfers.

In general, regardless whether its tied to a Transfer or not, I think having a Transact extrinsic (that would be composed of DescendOrigin + WithdrawAsset + BuyExecution + Transact) opens a whole new set of possibilities. For instance, this allows a smart contract in parachain A to interact with a smart contract in parachain B. We have an extrinsic in Moonbeam that does something similar (only in our test networks for now) https://github.com/PureStake/moonbeam/blob/1f1b300af5c3ae3695e7666712aed5c5af4dcb3e/pallets/xcm-transactor/src/lib.rs#L532.

I think the Transact must be tied to a transfer in order to not assume any prefunded accounts, from which funds for tx fees can be withdrawn, sovereign or not. This way it can work for all 3 xtokens transfer cases self_reserve, to_reserver and to_non_reserve

[girazoki]

I think the Transact must be tied to a transfer in order to not assume any prefunded accounts, from which funds for tx fees can be withdrawn, sovereign or not. This way it can work for all 3 xtokens transfer cases self_reserve, to_reserver and to_non_reserve

While in general I agree with this (and we might move to this in the future), we had some concerns around this, specifically being:

• A user willing to transact paying fees in a token not yet accepted by the origin chain. In this case there would be nothing to burn on the origin chain side.

• For simplicity, lets assume there is no transfer, but rather only fees for paying the XCM messages are burnt on the origin chain and withdrawn from the sovereign on the destination side. In order to work with DescendOrigin, your message would look something like: WithdrawAsset + ClearOrigin + BuyExecution + Transact. Meaning that WithdrawAsset should happen with the parachain origin, while Transact should happen with the descended origin. Although there should (in theory) not be any issue with this, we preferred decoupling actions that required different origins.

• Doing a transfer alongisde transact involves most likely additional instructions (at least DepositAsset) which some users might not be willing to pay every time they do a transact. Some users might be willing to just transfer funds once to their controlled account, and then issuing a transact assuming fees will be paid from the previously transferred funds.

Dont know if these are sufficient concerns, but maybe we can make both cases co-exist (e.g., a extrinsic with transfer_and_transact, and another one with just transact).

[KiChjang]

The issue with ClearOrigin is not the barrier. The default xcm-executor code attaches ClearOrigin when it handles instructions used by xtokens extrinsics like DepositReserveAsset or InitiateReserveWithdraw - https://github.com/paritytech/polkadot/blob/master/xcm/xcm-executor/src/lib.rs#L398-L421 So I cannot descend and create a unique origin for the Transact dispatchable. So that's why the second xcm message is needed. But if I understood your previous comment correctly, the XCM v3 PR will resolve this issue.

As it turns out, XCMv3 doesn't resolve this issue, as we can't be certain that the reserve is accurately representing the origin when it tries to modify the origin register with DescendOrigin, so sending an extra XCM directly to the destination is likely to be the only way that works.

[ghzlatarev]

A user willing to transact paying fees in a token not yet accepted by the origin chain. In this case there would be nothing to burn on the origin chain side.

Indeed, that's another assumption in my initial design, mostly for simplicity of the prototype though. The fee is the same as asset, and must be accepted on the receiver chain, otherwise it won't work. The design can probably also be extended to support transfers where the asset and fee are different.

Doing a transfer alongisde transact involves most likely additional instructions (at least DepositAsset) which some users might not be willing to pay every time they do a transact. Some users might be willing to just transfer funds once to their controlled account, and then issuing a transact assuming fees will be paid from the previously transferred funds.

Yeah the cost of all the deposits and withdrawals into the buffer accounts might become non-negligible at some point. But again that's due to the generality of execute_and_send_reserve_kind_xcm. Anyway I see the value of transact_only extrinsic which will just need the second message of this design, and the user will have fund their account separately. But the account will again have to be derived from the multilocation of the sender. I'm all in to add such an extrinsic to the implementation.

As it turns out, XCMv3 doesn't resolve this issue, as we can't be certain that the reserve is accurately representing the origin when it tries to modify the origin register with DescendOrigin, so sending an extra XCM directly to the destination is likely to be the only way that works.

All right so I'm just going to try and do a proper implementation of this 2 message design to see what you guys think. With a bounded input and xcm-sender, as well as a second transact_only extrinsic.

If you feel strongly for dropping this idea, please let me know so I can try and focus on an alternative approach like paritytech/polkadot-sdk#801

[ghzlatarev]

Ok guys thanks for the attention, so here is a much cleaner draft PR against my own fork. If it has potential I can reopen against orml master:

How this code can be used:

• Manta's concrete use-case is for front-end dApps (including ours) to be able to transfer any public asset from any chain to Manta and privatize them in one call. So the steps would be like below:
  • ParachainA (on user's behalf, say Alice) sends an XCM message containing both the ParachainA token transfer request and Transact with Manta's to_private extrinsic.
  • Manta, upon receiving ParachainA's message, directly mint the correct amount of pParachainA (private ParachainA token) to Alice's account and dispatch the to_private call with it.
• Also as girozaki mentioned this will allow smart contract in parachain A to interact with a smart contract in parachain B, which should have many possibilities.

How the code change from first iteration:

• Made the Transact input encoded call data bounded.
• Added an example receiver Barrier that expects the exact set of instrucitons DescendOrigin + WithdrawAsset + BuyExecution + Transact + RefundSurplus + DepositAsset
• Added 3 test cases to illustrate the functionality transacting a remark call after a self-reserve, to-rserve, and to-non-reserve transfers
• Added a transact-only extrinsic, which does the transact without the transfer, to reduce fees.
• Added initial docs
• Added extrinsic weights for the orml extrinsics

Current stage of code is that the sender sends 2 xcm messages:

1. First is token transfer to a sender derivative account.
   1.1. Sender derivative account is crated from the sender multilocation as seen by the receiver chain.
   1.2. Concretely in the mock runtime and tests the multilocation is simply hashed via a blak256 hasher Account32Hash
2. Xcm message with 6 instructions. What happens on the receiver side is:
   2.1 DescendOrigin is used to descend into a unique sender origin, which was used to create the sender derivative account in the first message.
   2.2. WithdrawAsset is used to withdraw the funds sent with the first xcm message.
   2.3. BuyExecution is used to buy execution time.
   2.4. The sender derivative account is also used as local origin to dispatch the call within the Transact instruction.
   2.5. RefundSurplus + DepositAsset is used to deposit any change.

TODOs:

-todo: This code is also similar to this idea https://gist.github.com/xlc/ebc2476afb7ecacdaa5ce95ae3b991c8#xcm-builder and similarly could use a more comprehensive solution to calculate weights on the sender side.
-todo: barrier code can be improved by checking the length of the encoded call data
-todo: in barrier code if we can decode a Call then it can be matched against a configurable set of allowed Calls
-todo: in the barrier code we can can do weight and fee checks of the decoded call via query_call_info and query_call_fee_details paritytech/substrate#11819
-todo: deposit change into any account

Please let me know what you think.

@KiChjang @xlc @zqhxuyuan @girazoki