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

Don't allow reserve transfers for the relay chain (as this can be trusted teleported to) #1144

Closed
wants to merge 5 commits into from
Closed

Don't allow reserve transfers for the relay chain (as this can be trusted teleported to) #1144

wants to merge 5 commits into from

Conversation

gilescope
Copy link
Contributor

Potential fix for paritytech/polkadot#5233

Maybe the definition should be in parachains-common...

@gilescope gilescope added A3-inprogress B0-silent Changes should not be mentioned in any release notes labels Apr 5, 2022
@hbulgarini hbulgarini added the T7-system_parachains This PR/Issue is related to System Parachains. label Apr 5, 2022
@hbulgarini hbulgarini added this to the statemint-v9.0.0 milestone Apr 5, 2022
gilescope and others added 2 commits April 5, 2022 17:30
@gilescope @joepetrowski
Update polkadot-parachains/canvas-kusama/src/xcm_config.rs
3914280 
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@gilescope
Stop canvas from being a host of reserve transfers
fb85cdf
@gilescope gilescope added B1-releasenotes and removed B0-silent Changes should not be mentioned in any release notes labels Apr 6, 2022
@gilescope gilescope mentioned this pull request Apr 6, 2022
Closed
HCastano
HCastano reviewed Apr 6, 2022
View reviewed changes
polkadot-parachains/canvas-kusama/src/xcm_config.rs
@@ -170,7 +176,7 @@ impl pallet_xcm::Config for Runtime {
type XcmExecuteFilter = Nothing;
type XcmExecutor = XcmExecutor<XcmConfig>;
type XcmTeleportFilter = Everything;
type XcmReserveTransferFilter = Everything;
type XcmReserveTransferFilter = Nothing;
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay so confirm the intention of this change, this will mean that anybody will still be able to Teleport any asset in and out of Canvas, but will not be able to perform RevervedTransfers, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

They should probably both be set to Nothing at genesis.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we set both to Nothing then users won't be able to get ROC tokens on Canvas to pay for fees. So for Teleport maybe it should be EverythingBut<IsParent> instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I didn't know this was what is used on Rococo. Then yes.

polkadot-parachains/canvas-kusama/src/xcm_config.rs
@@ -170,7 +176,7 @@ impl pallet_xcm::Config for Runtime {
type XcmExecuteFilter = Nothing;
type XcmExecutor = XcmExecutor<XcmConfig>;
type XcmTeleportFilter = Everything;
type XcmReserveTransferFilter = Everything;
type XcmReserveTransferFilter = Nothing;
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay so confirm the intention of this change, this will mean that anybody will still be able to Teleport any asset in and out of Canvas, but will not be able to perform RevervedTransfers, correct?

@gilescope gilescope changed the title XcmReserveTransferFilter=EverythingBut<IsParent> Don't allow reserve transfers for the relay chain (as this can be trusted teleported to) Apr 7, 2022
@KiChjang
Copy link
Contributor

KiChjang commented Apr 8, 2022

Do remember that this method doesn't actually prevent users from creating a DepositReserveAsset or TransferReserveAsset XCM instruction and executing it via the execute dispatchable on the XCM pallet -- all this PR does is limit the usage of the reserve_asset_transfer dispatchable on the XCM pallet.

@NachoPal
Copy link
Contributor

NachoPal commented Apr 8, 2022 •
edited
Loading

Do remember that this method doesn't actually prevent users from creating a DepositReserveAsset or TransferReserveAsset XCM instruction and executing it via the execute dispatchable on the XCM pallet -- all this PR does is limit the usage of the reserve_asset_transfer dispatchable on the XCM pallet.

It should be fine also. XcmExecuteFilter is set to Nothing

@gilescope
Copy link
Contributor Author

Problem: Tested it and it the other way around. It's the source rather than the dest that gets passed into the filter and you can't tell from the source that the dest is the relay chain. I think dest and beneficiary ought to be added to the filter arguments (as part of XCM3?).

@gilescope
Copy link
Contributor Author

closing this in favour of Barrier

@gilescope gilescope closed this Apr 12, 2022
@joepetrowski joepetrowski deleted the giles-filter branch April 22, 2022 11:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@HCastano HCastano HCastano left review comments

@joepetrowski joepetrowski joepetrowski left review comments

@KiChjang KiChjang KiChjang approved these changes

@athei athei Awaiting requested review from athei

@cmichi cmichi Awaiting requested review from cmichi

Assignees
No one assigned
Labels
T7-system_parachains This PR/Issue is related to System Parachains.
Projects
None yet
Milestone
statemint-v9.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@gilescope @KiChjang @NachoPal @HCastano @joepetrowski @hbulgarini