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 improvement - Accept impl Into<T> instead of just T #3708

Merged
merged 12 commits into from
Apr 4, 2024

Conversation

franciscoaguirre
Copy link
Contributor

The XCM builder pattern lets you build xcms like so:

let xcm = Xcm::builder()
    .withdraw_asset((Parent, 100u128).into())
    .buy_execution((Parent, 1u128).into())
    .deposit_asset(All.into(), AccountId32 { id: [0u8; 32], network: None }.into())
    .build();

All the .into() become quite annoying to have to write.
I accepted impl Into<T> instead of T in the generated methods from the macro.
Now the previous example can be simplified as follows:

let xcm = Xcm::builder()
    .withdraw_asset((Parent, 100u128))
    .buy_execution((Parent, 1u128))
    .deposit_asset(All, [0u8; 32])
    .build();

@franciscoaguirre franciscoaguirre requested a review from a team as a code owner March 14, 2024 22:57
@franciscoaguirre franciscoaguirre added the T6-XCM This PR/Issue is related to XCM. label Mar 15, 2024
Copy link
Contributor

@gupnik gupnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add the builder pattern here as well? https://paritytech.github.io/xcm-docs/

@bkchr bkchr enabled auto-merge April 3, 2024 19:22
@acatangiu
Copy link
Contributor

bot fmt

prdoc/pr_3708.prdoc Outdated Show resolved Hide resolved
@paritytech paritytech deleted a comment from command-bot bot Apr 4, 2024
@paritytech paritytech deleted a comment from command-bot bot Apr 4, 2024
@bkchr bkchr added this pull request to the merge queue Apr 4, 2024
Merged via the queue into master with commit c130ea9 Apr 4, 2024
122 of 133 checks passed
@bkchr bkchr deleted the xcm-builder-pattern-improvement branch April 4, 2024 13:10
@pgherveou
Copy link
Contributor

just curious, @franciscoaguirre did you check whether or not this change impact the binary size ?

@franciscoaguirre
Copy link
Contributor Author

Should we add the builder pattern here as well? https://paritytech.github.io/xcm-docs/

Definitely! I'm planning on moving those docs to rust docs to get them closer to the actual code and then I'll use the builder pattern there

@franciscoaguirre
Copy link
Contributor Author

just curious, @franciscoaguirre did you check whether or not this change impact the binary size ?

No. Is it because of the impl that it might increase binary size much?

@pgherveou
Copy link
Contributor

Yeah probably not a concern but monomorphization might generate some extra code

Ank4n pushed a commit that referenced this pull request Apr 9, 2024
…st `T` (#3708)

The XCM builder pattern lets you build xcms like so:

```rust
let xcm = Xcm::builder()
    .withdraw_asset((Parent, 100u128).into())
    .buy_execution((Parent, 1u128).into())
    .deposit_asset(All.into(), AccountId32 { id: [0u8; 32], network: None }.into())
    .build();
```

All the `.into()` become quite annoying to have to write.
I accepted `impl Into<T>` instead of `T` in the generated methods from
the macro.
Now the previous example can be simplified as follows:

```rust
let xcm = Xcm::builder()
    .withdraw_asset((Parent, 100u128))
    .buy_execution((Parent, 1u128))
    .deposit_asset(All, [0u8; 32])
    .build();
```

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: command-bot <>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this pull request Apr 9, 2024
…st `T` (paritytech#3708)

The XCM builder pattern lets you build xcms like so:

```rust
let xcm = Xcm::builder()
    .withdraw_asset((Parent, 100u128).into())
    .buy_execution((Parent, 1u128).into())
    .deposit_asset(All.into(), AccountId32 { id: [0u8; 32], network: None }.into())
    .build();
```

All the `.into()` become quite annoying to have to write.
I accepted `impl Into<T>` instead of `T` in the generated methods from
the macro.
Now the previous example can be simplified as follows:

```rust
let xcm = Xcm::builder()
    .withdraw_asset((Parent, 100u128))
    .buy_execution((Parent, 1u128))
    .deposit_asset(All, [0u8; 32])
    .build();
```

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: command-bot <>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T6-XCM This PR/Issue is related to XCM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants