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

Proxy for Nomination Pools #6846

Merged
merged 3 commits into from
Mar 27, 2023
Merged

Proxy for Nomination Pools #6846

merged 3 commits into from
Mar 27, 2023

Conversation

Ank4n
Copy link
Contributor

@Ank4n Ank4n commented Mar 8, 2023

closes #6211

@paritytech-ci paritytech-ci requested review from a team March 8, 2023 23:04
@Ank4n Ank4n added C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. B1-note_worthy Changes should be noted in the release notes A0-pleasereview T1-runtime This PR/Issue is related to the topic “runtime”. labels Mar 8, 2023
@@ -1044,6 +1045,9 @@ impl InstanceFilter<RuntimeCall> for ProxyType {
RuntimeCall::FastUnstake(..)
)
},
ProxyType::NominationPools => {
matches!(c, RuntimeCall::NominationPools(..) | RuntimeCall::Utility(..))
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we should support all utility calls. We should filter here explicitly on what we want to support.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably he took it for the same reason that Utility is part of Staking proxy as well.

Isn't this an issue that now with Staking proxy you can do utility::batch[transfer]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to test this and the batched calls itself are filtered again. So a proxy can only do utility::batch[some_nomination_call] if they are only allowed delegation for NominationPools .

Perhaps there are some utility calls that are unsafe (.i.e. does not pass along the filters)? It looks to me we are already supporting all utility calls in few other proxies as well.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the batched calls are itself checked again!

Something like aw_derivative is maybe not that good? I would have said that we only allow batching calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

In either case, this is beyond this PR per se, I suggest we move on and revise including Utility elsewhere.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure how this can be beyond this pr? This is a security issue if not done correctly.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, checked the code again as_derivative is taking the same filters into account. So, it is safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Ank4n
Copy link
Contributor Author

Ank4n commented Mar 26, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@bkchr bkchr merged commit 2c4627d into master Mar 27, 2023
@bkchr bkchr deleted the ankan/nomination-pools-proxy branch March 27, 2023 20:47
ordian added a commit that referenced this pull request Mar 29, 2023
* master:
  Companion: wasm-builder support stable Rust (#6967)
  Fix feature (#6966)
  configuration: backport async backing parameters from the feature branch (#6961)
  Histogram support in runtime metrics (#6935)
  Bump openssl from 0.10.38 to 0.10.48 (#6955)
  Proxy for Nomination Pools (#6846)
  Nomination Pools migration v5: RewardPool fix (#6957)
  bump timestamp script to v0.2 (#6954)
  Subsystem channel tweaks (#6905)
  Companion for #13683 (#6944)
  inherent disputes: remove per block initializer and disputes timeout event (#6937)
ordian added a commit that referenced this pull request Mar 29, 2023
…slashing-client

* ao-past-session-slashing-runtime:
  Companion: wasm-builder support stable Rust (#6967)
  Fix feature (#6966)
  configuration: backport async backing parameters from the feature branch (#6961)
  Histogram support in runtime metrics (#6935)
  Bump openssl from 0.10.38 to 0.10.48 (#6955)
  Proxy for Nomination Pools (#6846)
  Nomination Pools migration v5: RewardPool fix (#6957)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

New proxy type for NominationPools
6 participants