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

Improves XCM barrier for DescendOrigin #2335

Merged
merged 4 commits into from
Jun 8, 2023
Merged

Improves XCM barrier for DescendOrigin #2335

merged 4 commits into from
Jun 8, 2023

Conversation

Agusrodri
Copy link
Contributor

@Agusrodri Agusrodri commented Jun 7, 2023

This PR removes our custom barrier AllowTopLevelPaidExecutionDescendOriginFirst and uses the polkadot barrier WithComputedOrigin instead.

Our custom barrier AllowTopLevelPaidExecutionDescendOriginFirst was originally created to test xcm->evm on moonbase as a copy of AllowTopLevelPaidExecutionFrom but with DescendOrigin instead of ClearOrigin.

At the time, polkadot didn't offer any barriers to support DescendOrigin instruction, so it made sense to create our own, but that's no longer the case.

Parity has developed a generic barrier WithComputedOrigin that wraps existing barriers and handle the prefixed DescendOrigin instruction. Now, all relays and system parachains like statemine use this new barrier this way:

pub type XcmBarrier = (
	// Weight that is paid for may be consumed.
	TakeWeightCredit,
	// Expected responses are OK.
	AllowKnownQueryResponses<XcmPallet>,
	WithComputedOrigin<
		(
			// If the message is one that immediately attemps to pay for execution, then allow it.
			AllowTopLevelPaidExecutionFrom<Everything>,
                        ...
			// Subscriptions for version tracking are OK.
			AllowSubscriptionsFrom<Filter>,
		),
		UniversalLocation,
		ConstU32<8>, // max prefixed instructions (DescendOrigin or UniversalOrigin)
	>,
);

@Agusrodri Agusrodri added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. not-breaking Does not need to be mentioned in breaking changes labels Jun 7, 2023
@Agusrodri Agusrodri requested a review from girazoki June 7, 2023 18:19
@Agusrodri Agusrodri changed the title Rework xcm barrier Fix xcm barrier Jun 7, 2023
Copy link
Collaborator

@librelois librelois left a comment

Choose a reason for hiding this comment

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

@Agusrodri @girazoki I think we should remove our custom AllowTopLevelPaidExecutionDescendOriginFirst barrier, and use WithComputedOrigin like polkadot: https://github.com/paritytech/polkadot/blame/master/runtime/polkadot/src/xcm_config.rs#L163

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2023

Coverage generated "Thu Jun 8 08:40:14 UTC 2023":
https://s3.amazonaws.com/moonbeam-coverage/pulls/2335/html/index.html

Master coverage: 71.22%
Pull coverage: 71.21%

@crystalin crystalin mentioned this pull request Jun 6, 2023
24 tasks
@girazoki
Copy link
Collaborator

girazoki commented Jun 8, 2023

Fully agree @librelois, I forgot about these new Barrier

@moonbeam-foundation moonbeam-foundation deleted a comment from Agusrodri Jun 8, 2023
@librelois librelois merged commit b1c8630 into master Jun 8, 2023
32 checks passed
@librelois librelois deleted the fix-xcm-barrier branch June 8, 2023 11:29
@crystalin crystalin changed the title Fix xcm barrier Improves XCM barrier for DescendOrigin Jun 15, 2023
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants