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

Add xcm context to weight trader #7561

Closed
wants to merge 7 commits into from
Closed

Add xcm context to weight trader #7561

wants to merge 7 commits into from

Conversation

gilescope
Copy link
Contributor

@gilescope gilescope commented Jul 30, 2023 •
edited
Loading

(This is needed for paying xcm fees via asset conversion)

Cumulus companion: paritytech/cumulus#2954

@gilescope gilescope mentioned this pull request Jul 30, 2023
Closed
@gilescope gilescope added A0-please_review Pull request needs code review. 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. T1-runtime This PR/Issue is related to the topic “runtime”. B1-note_worthy Changes should be noted in the release notes labels Jul 30, 2023
@gilescope
Copy link
Contributor Author

Should be an optional context as there's not always an xcm context.

@gilescope gilescope added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Jul 30, 2023
@bkontur
Copy link
Contributor

bkontur commented Jul 31, 2023

I have this also on my TODO list :),
I discussed this few weeks ago with Gav here: paritytech/cumulus#2601 (comment)
so traits used by XcmExecutor should have XcmContext (not optional)

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3296888

@bkontur
Copy link
Contributor

bkontur commented Jul 31, 2023

@gilescope
there is no reason to make it Optional for trader: #7563,
and I think we should better do it required like here #7563
and create a clean companion to Cumulus master directly

@bkontur
Copy link
Contributor

bkontur commented Jul 31, 2023

there is only one case with &Option<XcmContext> here,
but Trader is not that case and XcmContext is always there

@gilescope
Copy link
Contributor Author

Superseded by #7563 - apparently we don't need to make it optional

@gilescope gilescope closed this Jul 31, 2023
muharem
muharem reviewed Jul 31, 2023
View reviewed changes
xcm/xcm-executor/src/traits/weight.rs
fn buy_weight(&mut self, weight: Weight, payment: Assets) -> Result<Assets, XcmError>;
fn buy_weight(
&mut self,
ctx: Option<&XcmContext>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why as first arg? since we extending more logical to see it a last arg. Also in other contracts it appears to be the last arg.
Also we do not have ctx naming for context so far.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@muharem muharem muharem left review comments

@KiChjang KiChjang Awaiting requested review from KiChjang

@joepetrowski joepetrowski Awaiting requested review from joepetrowski

@PatricioNapoli PatricioNapoli Awaiting requested review from PatricioNapoli

Assignees
No one assigned
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. B1-note_worthy Changes should be noted in the 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. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gilescope @bkontur @paritytech-cicd-pr @muharem @PatricioNapoli