[xcm] Failing to deposit dust (below minimum value) will fail the whole XCM program

[acatangiu]

Description

The implementation of DepositAsset will directly abort if any asset can't be deposited. This can happen if the amount of the asset is below its minimum balance and the receiver doesn't have a balance of it yet, in that case the asset transactor will directly fail with TokenError::BelowMinimum via the following call flow:

• polkadot-sdk/polkadot/xcm/xcm-executor/src/lib.rs

  Line 625 in ac3773e

  Config::AssetTransactor::deposit_asset(

• polkadot-sdk/polkadot/xcm/xcm-builder/src/fungibles_adapter.rs

  Line 390 in 3dece31

  >::deposit_asset(what, who, context)

• polkadot-sdk/polkadot/xcm/xcm-builder/src/fungibles_adapter.rs

  Line 291 in 3dece31

  Assets::mint_into(asset_id, &who, amount)

• polkadot-sdk/substrate/frame/support/src/traits/tokens/fungibles/regular.rs

  Line 264 in bdbe982

  let actual = Self::increase_balance(asset.clone(), who, amount, Exact)?;

• polkadot-sdk/substrate/frame/support/src/traits/tokens/fungibles/regular.rs

  Line 231 in bdbe982

  Err(TokenError::BelowMinimum.into())

This is especially a problem for more complex XCMs that transfer multiple assets, which can completely fail when the leftover fee-asset amount (after paying the fees) is just dust and might not meet "min value" for depositing.

Proposed solution

Treat TokenError::BelowMinimum error as non-fatal in xcm-executor, effectively ignore errors of failures to deposit dust.

Any dust that cannot be deposited, shall be dropped (effectively burned).

[acatangiu]

Also related #901

[wassimans]

Hi @acatangiu,

I recently graduated from the Polkadot Blockchain Academy and I’m now beginning my journey as a contributor to the Polkadot SDK. I’m particularly interested in gaining deep expertise in XCM, and I came across this issue and I’d love to work on it as It’s marked as a good first issue.

Would you mind assigning this issue to me?

[acatangiu]

Hi @wassimans, feel free to start work on this, there is no need to assign the issue.

Let me know if you need more information or help.

[wassimans]

Hi @acatangiu,

I’ve investigated the issue (DepositAsset causing an XCM program to fail entirely when the deposited amount is below the existential deposit (ED) or similar). I realised the code you pointed to was a bit old, some lines are about 2 years old, so I decided to start by testing the behaviour of the current code.

To verify the behavior, I wrote a test that simulates this situation:
• The sender withdraws 5 units of the native asset.
• The remote DepositAsset attempts to deposit that amount into the beneficiary account.
• The mocked deposit_asset implementation returns a BelowMinimum-like error (XcmError::FailedToTransactAsset("BelowMinimum".into())) for anything below 10 (to simulate ED = 10).
• Expected outcome: the whole XCM execution fails and no message is sent to the destination.

Result: The test confirms that the current SDK does fail the entire message when a dust asset is attempted to be deposited. This confirms that the behavior is still present.

Code for reference:
• Test: deposit_asset_below_ed_should_fail_entire_message_in_current_sdk

Of course, I've added a custom AssetTransactor in mock.rs (AssetTransactorWithED) to simulate the ED behaviour.

I'll continue with the fix, but please let me know if at this point there's anything important I'm missing.

Thanks

[acatangiu]

Investigation looks good, you are on the right track!

[wassimans]

At this point I have a clear fix, we need to add a check for the specific error and ignore it (ignoring the dust transfer, trapping would not be good I think) and let the other types of errors or Ok() transfers proceed as usual. This should be done in fn deposit_assets_with_retry in xcm-executor/src/lib.rs.

Clearly we don't want to do that check in fn increase_balance in frame support's tokens/fungibles/regular.rs because it's not XCM related and it's not its responsibility, this function needs to return semantic errors not handling them.

I think though that it would be more cleaner to add a new associated type in XcmConfig trait to make the behaviour of ignoring dust transfers opt in, something like:

type DustHandling: DustPolicy;

and an enum like:

enum DustPolicy {
    FailHard,   // current behavior
    Ignore,     // ignore (or trap maybe!!)
    LogOnly,    // just warn but don’t stop
}

And we can add the Default impl for the type that is the current behaviour (FailHard), so existing runtimes won't break. So in our fix, we just check the config and handle the dust error accordingly.

impl Default for DustHandling {
    fn default() -> Self { DustHandling::FailHard }
}

Your feedback is much appreciated @acatangiu ?

[acatangiu]

At this point I have a clear fix, we need to add a check for the specific error and ignore it (ignoring the dust transfer, trapping would not be good I think) and let the other types of errors or Ok() transfers proceed as usual. This should be done in fn deposit_assets_with_retry in xcm-executor/src/lib.rs.
Clearly we don't want to do that check in fn increase_balance in frame support's tokens/fungibles/regular.rs because it's not XCM related and it's not its responsibility, this function needs to return semantic errors not handling them.

Agree!

I think though that it would be more cleaner to add a new associated type in XcmConfig trait to make the behaviour of ignoring dust transfers opt in, something like:

IMO there are already too many configurable nobody actually use (everyone uses defaults). I would rather we switch the implementation directly from current "FailHard" behavior to "ignore/burn" undepositable dust.

If anyone ever needs this configurable, we/they can add the config option then. In the meantime, KISS is the default.