-
Notifications
You must be signed in to change notification settings - Fork 766
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
Comments
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. |
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 For an error different from "not enough ED", the behaviour should remain the same that we have now, right? |
why not have this code review on a PR? |
…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 <>
Is there an existing issue?
Experiencing problems? Have you tried our Stack Exchange first?
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.
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!
The text was updated successfully, but these errors were encountered: