Not possible to reserve_transfer_assets back from Statemine to Kusama

[NachoPal]

All reserved funds moved from Kusama to Statemine can not make their way back to Kusama.

It is not possible to do a reserve_transfer_assets() from Statemine to Kusama because it does not have any trusted Reserves: https://github.com/paritytech/polkadot/blob/master/runtime/kusama/src/xcm_config.rs#L136

The amount is deposited in the Parent's sovereign account in Statemine, but the UMP message fails in Kusama.

I don't know if it was purposely configured in that way, or we should define TrustedReserves in the same way it was done for TrustedTeleporters (KusamaForStatemine + KusamaForEncointer)

The same applies for Statemint<>Polkadot & Westmint<>Westend.

Let me know in case we need to apply the changes and I'll open a PR.

[gilescope]

Keith said "being a trusted teleporter doesn't necessarily mean that the target is also being trusted as a reserve"

[NachoPal]

This PR #4549 will open the door for users to "lose" their funds getting them trapped in Kusama's sovereign account in Statemine. As explained above, since Statemine is not a reserve from the point of view of Kusama, ReserveAssetDeposited will be dropped and the UMP message will fail with UntrustedReserveLocation.

To solve this issue, we will modify XcmReserveTransferFilter in Statemine from Everything to something like EverythingButParent, to prevent users to reserve transfers to Kusama.

Same applies for Statmint<>Polkadot and Westmint<>Westend.

cc @KiChjang

[KiChjang]

Hold on a sec, why are we worried about moving reserved funds from Kusama to Statemine? If the asset in question is KSM, then it really should be using asset teleportation, not reserved asset transfer. I don't see the circumstances in which a reserve_transfer_asset between Kusama and Statemine makes sense.

[joepetrowski]

Right, people should use teleport, but if there is nothing stopping people from using reserve transfers, then they shouldn't lose access to those KSM, especially as it's a pattern that people use to interact with KSM on other parachains. It either ends up with frustrated users or governance bloat as people try to recover (probably both).

So Kusama should either refuse to reserve transfer to Statemine if Statemine users won't be able to access the reserves (i.e. XcmReserveTransferFilter should be the complement of TrustedTeleporters), or Kusama should work as a reserve even for TrustedTeleporters.

[xlc]

There are countless ways to lost money in limbo. I don't really see a problem here as long as dApps and wallets are doing the right thing. It just not possible to patch all the holes.

This is a question that should Kusama prevent people reserve transfer to a parachain that's don't support that? Statemine is only one of the chain . There will be more parachains doesn't support KSM reserve transfer for various reasons and it is infeasible to blacklist all of them.

[NachoPal]

We (@joepetrowski , @gilescope , @hbulgarini, @NachoPal ) concluded that:

• If Telport is the proper way of moving KSM funds for Kusama<>Statemine, then KSM reserve transfers should be forbidden from Kusama to Statemine. To achieve that, Kusama's XcmReserveTransferFilter should be modified from Everything to something like EverythingButTrustedTeleporters
• In Statemine, users could still try to use the method reserve_transfer_assets to transfer KSM to Kusama. That action would end up with user's KSMs trapped in Kusama's sovereign account in Statemine. To prevent that, XcmReserveTransferFilter in Statemine should be modified from Everything to something like EverythingButNativeParent

[joepetrowski]

There are countless ways to lost money in limbo. I don't really see a problem here as long as dApps and wallets are doing the right thing. It just not possible to patch all the holes.

This is a question that should Kusama prevent people reserve transfer to a parachain that's don't support that? Statemine is only one of the chain . There will be more parachains doesn't support KSM reserve transfer for various reasons and it is infeasible to blacklist all of them.

This isn't a suggestion to patch holes for an individual parachain, but rather to define a pattern. If reserve backed transfers don't work for TrustedTeleporters in general (or rather, a reserve doesn't make sense because even if a user tries to reserve transfer back more than is in the reserve then they can just teleport instead), then the reserve location shouldn't reserve transfer to locations that it won't allow to access the reserves.

[KiChjang]

@NachoPal Can you explain why #4549 would open the door for users to get their funds trapped in Kusama's sovereign account? That PR is simply implementing transfer_asset on CurrencyAdapter, without that implementation, it would still have fallen back to using a two-part process with withdraw_asset and deposit_asset to transfer reserve assets.

[NachoPal]

@NachoPal Can you explain why #4549 would open the door for users to get their funds trapped in Kusama's sovereign account? That PR is simply implementing transfer_asset on CurrencyAdapter, without that implementation, it would still have fallen back to using a two-part process with withdraw_asset and deposit_asset to transfer reserve assets.

It wasn't falling back for a Tuple implementation. More info here: #4548

With #4549 in place, a TransferReserveAsset instruction https://github.com/paritytech/polkadot/blob/master/xcm/xcm-executor/src/lib.rs#L308 will end up successfully executing internal_transfer_asset() for the CurrencyAdapter: https://github.com/paritytech/polkadot/blob/master/xcm/xcm-builder/src/currency_adapter.rs#L166-L181

Before #4549, neither beam_asset() nor transfer_asset() weren't implemented for CurrencyAdapter, consequently failing and because of the Tuple implementation moving on to FungliblesAdapter were transfer_asset() ended up failing also with a FailedToTransactAsset ("AssetIdConversionFailed").

[KiChjang]

The way that you have described it sounded like as if you put the blame solely on the PR and that we should revert it. The fact that it wasn't falling back on a tuple implementation is a bug, so I wouldn't call it as "opening the door" when in fact the code really should have worked in the way that was intended, i.e. perform a reserve asset transfer, especially when CurrencyAdapter does perform a reserve asset transfer when it is not in a tuple.

[NachoPal]

No blames here :) I never said the PR was a bug or it had to be reverted, it is actually a necessary fix. The focus has been always on Statemine, and it has been there where we've been figuring out what were the necessary changes. If there is a bug somewhere, it is in the Statemine XCM config, not in the PR.

The term "opening the door" is from the point of view of Statemine or any other Parachain with Kusama as their reserve. Maybe not the proper term, but what I wanted to describe is that the PR "unlocks" a hidden potential issue that we didn't notice because reserve_transafer_assets was failing before.