Investigate what happens when an unkown assets is sent over XCM

[gianfra-t]

This issue is just a placeholder for the discussion about the topic, and possible action points to take.

Upon receiving an XCM message for asset deposit, we would like to find out if any of the existing barriers block the deposit of assets that are not handled by the chain. We may define an asset "handled" by the chain by those which implement the following:

• Metadata on the asset registry
• A map to internal CurrencyId representation from it's XCM one, and vice-versa.

If there is no such barrier that blocks the execution of these messages, then we should define if it is relevant to add a custom one.

[gianfra-t]

So after a quick search, I do not think there is currently a barrier that checks on the assets being sent to us. What happens more or less like this:

• We receive the instructions where the first one is any instruction that deposits assets, like ReserveAssetDeposited. Up to this point the AllowTopLevelPaidExecutionFrom barrier let's the message go through, it does not make any assumptions about the asset being deposited.
• The next instruction should be BuyExecution, where the trader is called. Our AssetRegistryTrader (assuming the latest implementation) will attempt to convert the weight to amount and extract the fee. This operation can be thought as a barrier, since if the asset cannot be converted to fee, we will stop the execution and error with XcmError::TooExpensive. The assets deposited will get trapped (supported or not). The asset cannot be converted to fee if it there is no metadata for that asset (based on the multi-location).
• It is possible that the message contains more than one asset, one of which is supported and the other not. In this case, the trader will attempt to buy the execution in any of them, and so further instructions with unsupported assets can continue being executed.
• Ultimately, the DepositAsset instruction with an unsupported asset may be called. Here, our LocalAssetTransactor will attempt to deposit it. This will finally fail since checks here are performed to transform the asset to our currency_id.

[ebma]

@gianfra-t thanks for the great ticket description and documentation of your findings 🙏

This will finally fail since checks here are performed to transform the asset to our currency_id.

True, it should eventually fail because we are using the default implementation which returns errors, see here.

The next instruction should be BuyExecution, where the trader is called. Our AssetRegistryTrader (assuming the latest https://github.com/pendulum-chain/pendulum/pull/410) will attempt to convert the weight to amount and extract the fee. This operation can be thought as a barrier, since if the asset cannot be converted to fee, we will stop the execution and error with XcmError::TooExpensive.

I agree, this seems to be the place where the execution of the XCM message would stop in the setup using the asset registry and AssetRegistryTrader that you implement in https://github.com/pendulum-chain/pendulum/pull/410. So the question is whether we want to change the Barrier to make the execution of 'invalid' XCM messages fail earlier or if we are fine with it failing in the Trader. I checked the Barrier of Centrifuge as they are also using the AssetRegistryTrader and it doesn't seem like they check early.

[gianfra-t]

So the question is whether we want to change the Barrier to make the execution of 'invalid' XCM messages earlier

Yes! That is a good question. I think it may be difficult to create a barrier that accounts for the many instructions that may have an unsupported asset on them, but that would be in the general sense. If we are talking only about a Deposit instruction, then it should be simple.

Although, I cannot see the immediate benefit other than just save one deposit instruction execution.

[ebma]

I think I'm fine if we keep it as is and don't check for this in the barrier. I don't see other parachains doing so either. Astar doesn't check for assets, Moonbeam doesn't, Interlay doesn't (though they have an additional check for some Transact instructions).

This doesn't mean that we can't do it, it's just an indicator that it doesn't seem so be necessary. What's your opinion here @TorstenStueber?

[bogdanS98]

Thanks @gianfra-t for all the details documented. I'm also fine with keeping everything as is since I think the benefit of one less instruction execution is not worth changing the Barrier.

[prayagd]

Hey team! Please add your planning poker estimate with Zenhub @b-yap @bogdanS98 @ebma @gianfra-t @TorstenStueber

[TorstenStueber]

Thanks for the research. I agree that we don't need to put more effort into this then. I am fine with closing this ticket @gianfra-t.