Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
2 tasks done
acatangiu opened this issue Apr 22, 2024 · 4 comments · Fixed by #4460
Closed
2 tasks done
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I5-enhancement An additional feature request. T6-XCM This PR/Issue is related to XCM.

Comments

@acatangiu
Copy link
Contributor

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • 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 acatangiu added the I5-enhancement An additional feature request. label Apr 22, 2024
@acatangiu acatangiu moved this to Todo in Bridges + XCM Apr 22, 2024
@acatangiu acatangiu added T6-XCM This PR/Issue is related to XCM. C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. labels Apr 22, 2024
@dastansam
Copy link
Contributor

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?

@acatangiu
Copy link
Contributor Author

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.

@jpserrat
Copy link
Contributor

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?

@acatangiu
Copy link
Contributor Author

why not have this code review on a PR?

github-merge-queue bot pushed a commit that referenced this issue Aug 12, 2024
…m satisfies ED (#4460)

Closes #4242

XCM programs that deposit assets to some new (empty) account will now
succeed if at least one of the deposited assets satisfies ED. Before
this change, the requirement was that the _first_ asset had to satisfy
ED, but assets order can be changed during reanchoring so it is not
reliable.

With this PR, ordering doesn't matter, any one(s) of them can satisfy ED
for the whole deposit to work.

Kusama address: FkB6QEo8VnV3oifugNj5NeVG3Mvq1zFbrUu4P5YwRoe5mQN

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
Co-authored-by: command-bot <>
@github-project-automation github-project-automation bot moved this from Todo to Done in Bridges + XCM Aug 12, 2024
@github-project-automation github-project-automation bot moved this from Todo to Shipped in Asset Improvement Board Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I5-enhancement An additional feature request. T6-XCM This PR/Issue is related to XCM.
Projects
Status: Shipped
Status: Done
3 participants