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

Ensure extrinsics in the XCM pallet check for a configurable origin #6442

Closed
KiChjang opened this issue Dec 15, 2022 · 10 comments
Closed

Ensure extrinsics in the XCM pallet check for a configurable origin #6442

KiChjang opened this issue Dec 15, 2022 · 10 comments
Labels
T6-XCM This PR/Issue is related to XCM.

Comments

@KiChjang
Copy link
Contributor

Code patterns like the following:

pub fn force_unsubscribe_version_notify(
	origin: OriginFor<T>,
	location: Box<VersionedMultiLocation>,
) -> DispatchResult {
	ensure_root(origin)?;
	// ...
}

are common in the XCM pallet. This is undesirable as only root can call these extrinsics. Instead, going forward, what we should do is create a new config item called AdminOrigin, and check for that. This will allow the origin to have its own curve in Gov2.

@KiChjang KiChjang added T6-XCM This PR/Issue is related to XCM. Q2-easy labels Dec 15, 2022
serkul pushed a commit to serkul/polkadot that referenced this issue Dec 23, 2022
Add new associated type, AdminOrigin, bounded by EnsureOrigin trait in
XCM-pallet config. Replace ensure_root() with ensure_origin(). Use
EnsureRoot<AccountId> in all implementations of XCM pallet to preserve
the current behavior until Gov2 specific origins are implemented.
@serkul
Copy link

serkul commented Dec 27, 2022

Hi @KiChjang, can you take a look at the PR which I made for this issue?
I see that CI fails in cumulus, because AdminOrigin is missing there in impl pallet_xcm.... I don't know how to address this correctly since it's a separate repo. Maybe I misunderstood the task, but, how I see it, adding a new item in a pallet's config will require changes in all places where the pallet's config is implemented.

@KiChjang
Copy link
Contributor Author

This is not meant to be worked on just yet; it's an issued filed specifically for XCM v3.

@serkul
Copy link

serkul commented Dec 27, 2022

Ok, it wasn't obvious for me. I just saw an "easy" tag and thought it was a good issue to start with :) I'll close the PR then.

@KiChjang
Copy link
Contributor Author

Since XCM v3 has just merged, this is free to be worked upon again.

@KiChjang KiChjang added J7-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be. and removed J7-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be. labels Jan 24, 2023
serkul pushed a commit to serkul/polkadot that referenced this issue Jan 25, 2023
Rebase against XCMv3 changes.
Add new associated type, AdminOrigin, bounded by EnsureOrigin trait in
XCM-pallet config. Replace ensure_root() with ensure_origin(). Use
EnsureRoot<AccountId> in all implementations of XCM pallet to preserve
the current behavior until Gov2 specific origins are implemented.
serkul pushed a commit to serkul/polkadot that referenced this issue Jan 26, 2023
Rebase against XCM v3 changes.
Add new associated type, AdminOrigin, bounded by EnsureOrigin trait in
XCM-pallet config. Replace ensure_root() with ensure_origin(). Use
EnsureRoot<AccountId> in all implementations of XCM pallet to preserve
the current behavior until Gov2 specific origins are implemented.
@serkul
Copy link

serkul commented Jan 26, 2023

@KiChjang, I see this issue stopped being "easy". Can I continue to work on it or should I leave it alone?

@KiChjang
Copy link
Contributor Author

It's simply because we've removed that label, so I can't apply it on this issue. You are welcome to work on it.

@serkul
Copy link

serkul commented Jan 26, 2023

after rebasing, there are Currency, CurrencyMatcher, TrustedLockers, SovereignAccountOf, MaxLockers, WeightInfo types that are missing in pallet_xcm::Config in runtime/rococo. Does it need to be fixed or I just reopen PR with some tests failing?

@KiChjang
Copy link
Contributor Author

I unfortunately do not have the context of what you're talking about. It is likely to be a rebasing issue.

serkul pushed a commit to serkul/polkadot that referenced this issue Jan 26, 2023
Rebase against XCM v3 changes.
Add new associated type, AdminOrigin, bounded by EnsureOrigin trait in
XCM-pallet config. Replace ensure_root() with ensure_origin(). Use
EnsureRoot<AccountId> in all implementations of XCM pallet to preserve
the current behavior until Gov2 specific origins are implemented.
@serkul
Copy link

serkul commented Jan 26, 2023

You are right, Keith , it was a rebasing issue. I opened a new [PR] (#6632). There are two label checks which fail, I guess, I cannot fix them myself.
Another failing check, as before XCM v3 merge ", is continuous-integration/gitlab-check-dependent-cumulus", because of the new config item.

serkul pushed a commit to serkul/polkadot that referenced this issue Jan 31, 2023
Add new associated type, AdminOrigin, bounded by EnsureOrigin trait in
XCM pallet. Replace ensure_root() with ensure_origin() from a
EnsureOrigin trait. Set AdminOrigin as EnsureRoot<AccountId> in xcm
configs.
vstam1 pushed a commit that referenced this issue Mar 21, 2023
5ae05e1

Add new associated type, AdminOrigin, bounded by EnsureOrigin trait in
XCM pallet. Replace ensure_root() with ensure_origin() from a
EnsureOrigin trait. Set AdminOrigin as EnsureRoot<AccountId> in xcm
configs.
gavofyork pushed a commit that referenced this issue Mar 23, 2023
* Ensure for a configurable origin in XCM (#6442), cherry picked from
5ae05e1

Add new associated type, AdminOrigin, bounded by EnsureOrigin trait in
XCM pallet. Replace ensure_root() with ensure_origin() from a
EnsureOrigin trait. Set AdminOrigin as EnsureRoot<AccountId> in xcm
configs.

* cargo fmt

* small stylistic change

---------

Co-authored-by: serkul <[email protected]>
@KiChjang
Copy link
Contributor Author

KiChjang commented Apr 4, 2023

Fixed in #6928.

@KiChjang KiChjang closed this as completed Apr 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T6-XCM This PR/Issue is related to XCM.
Projects
None yet
Development

No branches or pull requests

3 participants