polkadot-sdk icon indicating copy to clipboard operation
polkadot-sdk copied to clipboard

[xcm] depositing multiple assets to inexistent account should work if at least one of them satisfies ED

Open acatangiu opened this issue 1 year ago • 4 comments

Is there an existing issue?

  • [X] I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • [X] This is not a support question.

Motivation

One should be able to send a bunch of assets over XCM to some previously inexistent account on the local chain or on some remote chain.

(inspired by https://substrate.stackexchange.com/a/11360/382)

Request

Context: destination/beneficiary account does not (yet) exist/have any assets.

  1. Sending a single asset that satisfies destination ED currently works.
  2. Sending multiple assets also work, if and only if the first asset in the list satisfies ED.
  3. Sending multiple assets where there's at least one that satisfies ED, but not the first in the list fails.

Please note, that the user cannot really control the order of assets in the list since they are automatically sorted by their id/location. So we cannot rely on 2 as a workaround.

3 should "just work", since at least one of the incoming assets will satisfy ED.

Solution

The problem is that asset depositing is done one asset at a time, and account ED is checked for each.

The implementation should treat "not enough ED" errors as transient unless all assets suffer the same.

Are you willing to help with this request?

Yes!

acatangiu avatar Apr 22 '24 13:04 acatangiu

The implementation should treat "not enough ED" errors as transient unless all assets suffer the same.

so, throw iff all assets fail to deposit? for example, if n assets are to be deposited, and asset at index n-1 satisfied ED, what happens to all failed deposits prior to it? they should get deposited again?

dastansam avatar May 03 '24 16:05 dastansam

The implementation should treat "not enough ED" errors as transient unless all assets suffer the same.

so, throw iff all assets fail to deposit? for example, if n assets are to be deposited, and asset at index n-1 satisfied ED, what happens to all failed deposits prior to it? they should get deposited again?

Yes, failed deposits because of ED can be queued for retry.

If at least one deposit succeeds (ED is now satisfied), the retry-queue can be processed knowing the transient error is now gone.

acatangiu avatar May 08 '24 08:05 acatangiu

Hey @acatangiu, is something like this that you are looking for?

			DepositAsset { assets, beneficiary } => {
				let old_holding = self.holding.clone();
				let result = Config::TransactionalProcessor::process(|| {
					let deposited = self.holding.saturating_take(assets);
					let mut all_failed = false;
					let mut failed_deposits = Vec::new();
					for asset in deposited.into_assets_iter() {
						let asset_result = Config::AssetTransactor::deposit_asset(
							&asset,
							&beneficiary,
							Some(&self.context),
						);
						if asset_result.is_ok() {
							all_failed = false;
							continue
						}
						failed_deposits.push(asset);
					}
					if all_failed {
						return Err(Error)
					}
					for asset in failed_deposits {
						Config::AssetTransactor::deposit_asset(
							&asset,
							&beneficiary,
							Some(&self.context),
						)?;
					}
					Ok(())
				});
				if Config::TransactionalProcessor::IS_TRANSACTIONAL && result.is_err() {
					self.holding = old_holding;
				}
				result
			},

In this code I'm not checking the error is "not enough ED", the deposit_asset returns an Error enum but I couldn't find an equivalent to "not enough ED", how can I check this? Can I use the "not enough ED" string?

For an error different from "not enough ED", the behaviour should remain the same that we have now, right?

jpserrat avatar May 14 '24 12:05 jpserrat

why not have this code review on a PR?

acatangiu avatar May 14 '24 14:05 acatangiu