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

feat: update xcm config #623

Merged
merged 11 commits into from
Jul 27, 2023
Merged

feat: update xcm config #623

merged 11 commits into from
Jul 27, 2023

Conversation

Roznovjak
Copy link
Contributor

@Roznovjak Roznovjak commented Jul 3, 2023

Based on a discussion with @apopiak, where we reviewed the XCM changes proposed by the Substrate builders programme in this PR #591, we decided to make the following changes:

  1. Use WithComputedOrigin derivative barrier with AllowTopLevelPaidExecutionFrom and AllowSubscriptionsFrom barriers. This outer barrier computes the effective origin (checks for any origin altering operations) and passes this updated origin to inner barriers.
  2. Change XcmExecuteFilter from Everything to Nothing. This change disallows pallet_xcm::execute extrinsic which allows sending arbitrary XCM messages. We are not aware of any issues with this param set to Everything, but better be sure. Moreover, pallet_xcm is blocked by the runtime call filter.

@github-actions
Copy link

github-actions bot commented Jul 3, 2023

Crate versions that have been updated:

  • runtime-integration-tests: v1.8.0 -> v1.8.1
  • hydradx-runtime: v168.0.0 -> v169.0.0

Runtime version has been increased.

@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (3feaf40) 64.77% compared to head (e2c4be3) 64.77%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #623   +/-   ##
=======================================
  Coverage   64.77%   64.77%           
=======================================
  Files         134      134           
  Lines        9749     9749           
=======================================
  Hits         6315     6315           
  Misses       3434     3434           
Files Changed Coverage Δ
runtime/hydradx/src/lib.rs 3.33% <ø> (ø)
runtime/hydradx/src/xcm.rs 78.94% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@enthusiastmartin
Copy link
Contributor

Can you add description why the computedorigin is required ?

@@ -192,7 +199,7 @@ impl pallet_xcm::Config for Runtime {
type SendXcmOrigin = EnsureXcmOrigin<RuntimeOrigin, LocalOriginToLocation>;
type XcmRouter = XcmRouter;
type ExecuteXcmOrigin = EnsureXcmOrigin<RuntimeOrigin, LocalOriginToLocation>;
type XcmExecuteFilter = Everything;
type XcmExecuteFilter = Nothing;
Copy link
Member

Choose a reason for hiding this comment

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

@apopiak @dmoka this effectively breaks the exchange and send back assets scenario correct?

Copy link
Member

@mrq1911 mrq1911 Jul 3, 2023

Choose a reason for hiding this comment

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

if this is the case i would reconsider this change and at least specify some more permissive filter specific to this use-case

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say it makes sense to define the more permissive filter in the context of the ExchangeAsset stuff, not here.

Copy link
Contributor

@apopiak apopiak left a comment

Choose a reason for hiding this comment

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

looks fine after adjusting computed origin count

runtime/hydradx/src/xcm.rs Outdated Show resolved Hide resolved
runtime/hydradx/src/xcm.rs Outdated Show resolved Hide resolved
@Roznovjak Roznovjak merged commit 1b58960 into master Jul 27, 2023
8 checks passed
@Roznovjak Roznovjak deleted the xcm-config branch July 27, 2023 20:53
@mrq1911 mrq1911 mentioned this pull request Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants