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

Improve XCM Config #131

Merged
merged 6 commits into from
Aug 30, 2022
Merged

Improve XCM Config #131

merged 6 commits into from
Aug 30, 2022

Conversation

echevrier
Copy link
Contributor

@echevrier echevrier commented May 24, 2022

To undestand the different filter and check how to configure them: see https://blog.quarkslab.com/a-brief-overview-of-auditing-xcmv2.html

Tested: Filter changes still allow to manually open an HRMP channel with Acala's rococo parachain and transfer TEER token.

@echevrier echevrier marked this pull request as draft May 24, 2022 15:21
echevrier added 2 commits August 25, 2022 16:19
- Remove ParentAsSuperuser from XCM origin converter
- Disallow generic XCM execution, do not allow teleport and allow transfer
@echevrier echevrier marked this pull request as ready for review August 25, 2022 16:09
@echevrier echevrier requested a review from haerdib August 29, 2022 07:17
@echevrier echevrier linked an issue Aug 29, 2022 that may be closed by this pull request
Copy link
Contributor

@haerdib haerdib left a comment

Choose a reason for hiding this comment

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

Comment enhancement suggestions only. Makes it easier to understand, I hope :)

You tested the xtoken transfer & the HRMP channel, correct? Or should I test this as well?

If it's not too much work (if possible at all), would it be possible to shortly describe in the PR description from where you got your information from? Might come in handy in the future.

polkadot-parachains/integritee-runtime/src/xcm_config.rs Outdated Show resolved Hide resolved
polkadot-parachains/integritee-runtime/src/xcm_config.rs Outdated Show resolved Hide resolved
polkadot-parachains/integritee-runtime/src/xcm_config.rs Outdated Show resolved Hide resolved
echevrier and others added 3 commits August 29, 2022 12:49
Co-authored-by: haerdib <73821294+haerdib@users.noreply.github.com>
Co-authored-by: haerdib <73821294+haerdib@users.noreply.github.com>
Co-authored-by: haerdib <73821294+haerdib@users.noreply.github.com>
@echevrier
Copy link
Contributor Author

You tested the xtoken transfer & the HRMP channel, correct? Or should I test this as well?
I've tested it. See #117 (comment)

@echevrier echevrier requested a review from haerdib August 29, 2022 11:29
Copy link
Contributor

@haerdib haerdib left a comment

Choose a reason for hiding this comment

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

Cool! Thanks for your perseverance on this issue :D

@echevrier echevrier requested a review from haerdib August 29, 2022 13:39
Copy link
Contributor

@haerdib haerdib left a comment

Choose a reason for hiding this comment

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

Cool, thanks!

Copy link
Contributor

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

I have one question, but it looks good otherwise. :)

Comment on lines +269 to +270
type ExecuteXcmOrigin = EnsureXcmOrigin<Origin, LocalOriginToLocation>; // Allow any local origin in XCM execution.
type XcmExecuteFilter = Nothing; // Disable generic XCM execution. This does not affect Teleport or Reserve Transfer.
Copy link
Contributor

@clangenb clangenb Aug 30, 2022

Choose a reason for hiding this comment

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

Just for my understanding, is the combination of these two lines exactly the same as?

type ExecuteXcmOrigin = EnsureXcmOrigin<Origin, ()>; // Allow any local origin in XCM execution.
                                               //^^^ changed the line here
type XcmExecuteFilter = Nothing; // Disable generic XCM execution. This does not affect Teleport or Reserve Transfer.

Or would this also disable Teleport and Reserve Transfer then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From https://github.com/paritytech/polkadot/blob/99d6b557c0594a35198be84d03833e5af692d109/xcm/pallet-xcm/src/lib.rs#L86 , i think it would also disable Teleport and Transfer for users of this chain. ExecuteXcmOrigin is called in execute, do_reserve_transfer_assets and do_teleport_assets.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see. Thanks!

@clangenb clangenb merged commit 43b74f6 into master Aug 30, 2022
@clangenb clangenb deleted the ec_xcm_filter_117 branch September 6, 2022 06:15
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.

XCM pallet execution filter settings
3 participants