DepositAsset need failure recover of holding

[zqhxuyuan]

Current the logic of DepositAsset is first take assets from holding, then do AssetTransactor::deposit_asset(). If deposit_asset() throw Error, for example receiver not recognize token from sender, and for now it has no mechanism of recover holding, thus Config::AssetTrap would't be executed, in this case it'll cause token lossing.

I've test this case in orml: paraA transfer tokenA to paraB, and we let paraB not recognize tokenA temporary which will throw an Error. and in my test result, when execute xcm on paraB side,it will loss tokenA, this's not we wanted.

So maybe we need recover holding data when deposit_asset cause an Error. and this ability may extend to other instructions? cc @gavofyork @KiChjang

[zqhxuyuan]

Ah, I found my testcase missing the Appendix which have RefundSurplus instruction inside. and RefundSurplus will refund asset in total_surplus to holding, then Config::AssetTrap can continue executed.

[KiChjang]

Yeah, using SetErrorHandler and combining it with RefundSurplus or DepositAsset was my instinct as well, and as you have correctly observed, RefundSurplus would also need to have a proper AssetTrap configured in order to deal with excess assets found in the holding register after XCM execution has ended.

Closing this for now, feel free to reopen if you think there's anything more that we need to address here.

[zqhxuyuan]

@KiChjang I've add SetAppendix(RefundSurplus) before DepositAsset, and the receiver xcm message:

Xcm([
    ReserveAssetDeposited(MultiAssets([MultiAsset { 
        id: Concrete(MultiLocation { parents: 1, interior: X2(Parachain(1), GeneralKey([65])) }), fun: Fungible(500) }])
    ), 
    ClearOrigin, 
    BuyExecution { 
        fees: MultiAsset { id: Concrete(MultiLocation { parents: 1, interior: X2(Parachain(1), GeneralKey([65])) }), fun: Fungible(500) },
        weight_limit: Limited(100) 
    }, 
    SetAppendix(Xcm([
        RefundSurplus
    ])), 
    DepositAsset { 
        assets: Wild(All), 
        max_assets: 1, 
        beneficiary: MultiLocation { parents: 0, interior: X1(AccountId32 { network: Any, id: [1111111] }) } 
	}
])

and the log shows total_surplus/refunded: 0/0 which means refund_surplus() executed without any effect on holding?

how can I recover the holding before execute deposit_asset() into holding again?

[KiChjang]

Okay, it looks like that all the error messages that deposit_asset can emit needs to accept an additional parameter to indicate the amount and the kind of asset that had errored. Without reporting the assets, there is currently no way of recovering those funds as they are neither brought back into the holding register, nor are they put into the surplus register.

[KiChjang]

My instinct is telling me that the best way to handle this is to add a new "recovery register" for cases where XCM execution failed and some assets were taken from holding. Additionally, it may also be a good idea to allow the appendix or the error handler to somehow gain access to these funds and operate upon them.

[KiChjang]

Actually, now that I think about it, the AssetTransactor is configurable, so the recovery process is 100% controllable by the implementer. I don't think we should change the XcmError type to include the assets that errored, nor should we create a recovery register, since the XCM executor has no way of guaranteeing that the total issuance of a particular asset has unchanged -- there's nothing stopping from an AssetTransactor to keep a local record of the errored out assets and then proceeding to send the errored out assets again to the XCM executor. Such a scenario could then be exploited to increase the total issuance unwittingly, as both the AssetTransactor and the XCM executor now have and act upon the same assets that have errored.

[KiChjang]

Closing, as this is the responsibility of the sender to ensure that the recipient handles the asset kind that's being sent over -- the recipient has no way of representing the asset otherwise.