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 builder pattern safe should allow clear_origin before buy execution #3770

Closed
franciscoaguirre opened this issue Mar 20, 2024 · 1 comment · Fixed by #4777
Closed

XCM builder pattern safe should allow clear_origin before buy execution #3770

franciscoaguirre opened this issue Mar 20, 2024 · 1 comment · Fixed by #4777
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. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. T6-XCM This PR/Issue is related to XCM.

Comments

@franciscoaguirre
Copy link
Contributor

franciscoaguirre commented Mar 20, 2024

The XCM builder pattern provides builders for making XCM messages:

  • builder
  • builder_unpaid
  • builder_unsafe

These builders just restrict the instructions you can use at the start, they force some instructions before others.
The first is meant to be used by regular extrinsics and rust clients.
The second is meant to be used by system functions.
The third is meant to be used for tests, teaching, tinkering and sometimes when the others do not allow you to do what you need.
I found a case of this that should be easy to solve.

Right now we can't do the following:

Xcm::<()>::builder()
  .reserve_asset_deposited(..)
  .clear_origin(..) // <- This is the problem
  .buy_execution(..)
  .deposit_asset(..)
  .build();

We need to use the builder_unsafe variant for it to work.

It is a perfectly safe idea to allow at least one clear_origin between an instruction that loads the holding register and one that pays for fees.
The solution is to implement the clear_origin method in the type XcmBuilder<Call, LoadedHolding> in polkadot/xcm/procedural.

@franciscoaguirre franciscoaguirre 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. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. labels Mar 20, 2024
@orgr
Copy link
Contributor

orgr commented Jun 12, 2024

Hi @franciscoaguirre, I just opened a PR
I'd be happy for any feedback, thanks

github-merge-queue bot pushed a commit that referenced this issue Jul 9, 2024
Fixes #3770 

Added `clear_origin` as an allowed command after commands that load the
holdings register, in the safe xcm builder.

Checklist
- [x] My PR includes a detailed description as outlined in the
"Description" section above
- [x] My PR follows the [labeling
requirements](https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process)
of this project (at minimum one label for T required)
- [x] I have made corresponding changes to the documentation (if
applicable)
- [x] I have added tests that prove my fix is effective or that my
feature works (if applicable)

---------

Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
Co-authored-by: gupnik <mail.guptanikhil@gmail.com>
TomaszWaszczyk pushed a commit to TomaszWaszczyk/polkadot-sdk that referenced this issue Jul 13, 2024
Fixes paritytech#3770 

Added `clear_origin` as an allowed command after commands that load the
holdings register, in the safe xcm builder.

Checklist
- [x] My PR includes a detailed description as outlined in the
"Description" section above
- [x] My PR follows the [labeling
requirements](https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process)
of this project (at minimum one label for T required)
- [x] I have made corresponding changes to the documentation (if
applicable)
- [x] I have added tests that prove my fix is effective or that my
feature works (if applicable)

---------

Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
Co-authored-by: gupnik <mail.guptanikhil@gmail.com>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this issue Aug 2, 2024
Fixes paritytech#3770 

Added `clear_origin` as an allowed command after commands that load the
holdings register, in the safe xcm builder.

Checklist
- [x] My PR includes a detailed description as outlined in the
"Description" section above
- [x] My PR follows the [labeling
requirements](https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process)
of this project (at minimum one label for T required)
- [x] I have made corresponding changes to the documentation (if
applicable)
- [x] I have added tests that prove my fix is effective or that my
feature works (if applicable)

---------

Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
Co-authored-by: gupnik <mail.guptanikhil@gmail.com>
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. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. T6-XCM This PR/Issue is related to XCM.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants