Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Fix TransactAsset Implementation #3345

Merged
8 commits merged into from
Jul 26, 2021
Merged

Fix TransactAsset Implementation #3345

8 commits merged into from
Jul 26, 2021

Conversation

apopiak
Copy link
Contributor

@apopiak apopiak commented Jun 22, 2021

This PR aims to fix the TransactAsset (tuple) implementation.
It converts local AssetNotFound errors into XcmError::AssetNotFound to allow the tuple implementation of TransactAsset to iterate properly. It also adjusts the other tuple implementations (notably withdraw_asset and deposit_asset) to have the same semantics.

Also adds some doc comments to XCM errors.

aims allow the tuple implementation of TransactAsset to iterate properly
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jun 22, 2021
@apopiak apopiak added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. labels Jun 22, 2021
@gavofyork
Copy link
Member

Would be good to add tests with this.

@apopiak
Copy link
Contributor Author

apopiak commented Jun 23, 2021

As mentioned in chat, maybe we should remove the tuple implementation of TransactAsset to avoid the associated footguns?

@apopiak apopiak changed the title Convert local AssetNotFound errors into XcmError::AssetNotFound Fix TransactAsset Implementation Jun 23, 2021
@apopiak
Copy link
Contributor Author

apopiak commented Jun 23, 2021

I adjusted the TransactAsset tuple implementation to something that I believe will be less error-prone.

@apopiak
Copy link
Contributor Author

apopiak commented Jun 23, 2021

Would be good to add tests with this.

Do you mean integration tests?
This only shows up for the tuple implementation.

@apopiak
Copy link
Contributor Author

apopiak commented Jul 21, 2021

@shawntabrizi Another issue I see with the Tuple impl is that the check_in and check_out implementations have no way of signalling to each other, so an asset might be checked in twice which might or might not be what you want.
(Of course your point still stands that people can write their own combinator that does the correct thing.)

@apopiak apopiak requested a review from pepyakin July 21, 2021 16:23
xcm/src/v0/traits.rs Outdated Show resolved Hide resolved
Co-authored-by: Andronik Ordian <write@reusable.software>
@shawntabrizi
Copy link
Member

shawntabrizi commented Jul 21, 2021

Sounds like one bigger solution for all of this is a continue boolean which allows the user to directly control the behavior here

@apopiak
Copy link
Contributor Author

apopiak commented Jul 22, 2021

@gavofyork Fine to merge?

@apopiak
Copy link
Contributor Author

apopiak commented Jul 26, 2021

bot merge

@ghost
Copy link

ghost commented Jul 26, 2021

Trying merge.

@ghost ghost merged commit 34ad3ab into master Jul 26, 2021
@ghost ghost deleted the apopiak/asset-not-found branch July 26, 2021 10:43
ordian added a commit that referenced this pull request Jul 26, 2021
* master:
  enable disputes (#3478)
  Do not leak active head data in statement distribution (#3519)
  Fix TransactAsset Implementation (#3345)
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants