feat: Additional Proxy Types on shiden #920

gitofdeepanshu · 2023-04-27T15:48:48Z

Pull Request Summary

This PR adds new proxy types on Shiden. Previous Iteration: #778
Related #760

Previous Proxy Types

    CancelProxy,
    DappsStaking,

Added Types

   Any,
   NonTransfer,
   Balances,
   IdentityJudgement,

Dinonard

You will also need to bump semver & spec_version.

Dinonard · 2023-04-28T13:19:32Z

runtime/shiden/src/lib.rs

+            ProxyType::Balances => {
+                matches!(c, RuntimeCall::Balances(..))


Might be a good approach to also allow asset transfers.

good suggestion.

Dinonard · 2023-04-28T13:21:27Z

runtime/shiden/src/lib.rs

            ProxyType::DappsStaking => {
-                matches!(c, RuntimeCall::DappsStaking(..) | RuntimeCall::Utility(..))
+                matches!(c, RuntimeCall::DappsStaking(..))


We will need this filter to support batch call with DappsStaking calls.

RuntimeCall::Utility(..) is always passed through the filter so no need to add it to every variant.

Tbh, I didn't know that! Good!

Dinonard · 2023-04-28T13:21:42Z

runtime/shiden/src/lib.rs

+    Any,
+    NonTransfer,
+    Balances,
+    IdentityJudgement,
    CancelProxy,
    DappsStaking,


Let's add comment to these (also in Shibuya).

Dinonard · 2023-04-28T13:23:18Z

runtime/shiden/src/lib.rs

            }
        }
    }

    fn is_superset(&self, o: &Self) -> bool {
        match (self, o) {
            (x, y) if x == y => true,
+            (ProxyType::Any, _) => true,
+            (_, ProxyType::Any) => false,


Doesn't NonTransafer also cover many (or all?) types except for Any?

Doesn't cover Balances.

Still, you'd need to cover those here, right?

Balance is not a superset or subset of any proxy type except Any which is already covered.

I meant the NonTransfer in the original message 🙂

It's superset of almost everything - expcet Balances as you noted. Right?

Now that we have added pallet Assets, it is also not included.
is_superset() in used to check which proxy types can add/remove other proxy types. As NonTransfer has no jurisdiction over Balances or Assets so we can't add it.
If you really want to add some more cases then you can cover other cases like

(ProxyType::NonTransfer, ProxyType::IdentityJudgement) => true, (ProxyType::NonTransfer, ProxyType::CancelProxy) => true, (ProxyType::NonTransfer, ProxyType::DappStaking) => true,

It is up to us if we want to give NonTransfer that much power, no other parachain is doing it btw.

Dinonard · 2023-04-28T13:24:59Z

runtime/shiden/src/lib.rs

+            // Always allowed RuntimeCall::Utility no matter type.
+            // Only transactions allowed by Proxy.filter can be executed
+            _ if matches!(c, RuntimeCall::Utility(..)) => true,


Is this intentional?

yes, allows for RuntimeCall::Utility to always pass through the filter for every variant.

I see!

Could we add some tests for this below? 🤔

Dinonard · 2023-04-28T13:25:34Z

runtime/shiden/src/lib.rs

    CancelProxy,
    DappsStaking,
 }

 impl Default for ProxyType {
    fn default() -> Self {
-        Self::CancelProxy
+        Self::Any


I kind of preferred the last one - less chance to do some damage 🙂

Well the idea was to make it simpler for users, when someone makes an account proxy, it is expected that the proxy should be of type Any (allows all operations). Everybody uses this as default so it's kind of standard.

Changing this to CancelProxy would be bad UX imo.

Ok, just hope no one loses their account due to this 😶

I just found out that the default has to have full permission.
https://github.com/paritytech/substrate/blob/b2a914c09739478c63d318f6b294901a3290e548/frame/proxy/src/lib.rs#L784

shunsukew

Other than 1 comment I added, it looks good to me

shunsukew · 2023-05-01T04:40:07Z

runtime/shiden/src/lib.rs

+                matches!(
+                    c,
+                    RuntimeCall::System(..)
+                        | RuntimeCall::Utility(..)


This can be also removed

shunsukew

Why not apply changes to Shibuya too first?

gitofdeepanshu · 2023-05-01T07:36:06Z

That makes sense, will do.

gitofdeepanshu · 2023-05-01T08:24:49Z

@shunsukew
Added changes to Shubiya as well here

shunsukew

Looks good to me

Dinonard · 2023-05-02T13:48:10Z

runtime/shiden/src/lib.rs

            }
        }
    }

    fn is_superset(&self, o: &Self) -> bool {
        match (self, o) {
            (x, y) if x == y => true,
+            (ProxyType::Any, _) => true,
+            (_, ProxyType::Any) => false,


Still, you'd need to cover those here, right?

Dinonard · 2023-05-02T13:53:23Z

runtime/shiden/src/lib.rs

            ProxyType::DappsStaking => {
-                matches!(c, RuntimeCall::DappsStaking(..) | RuntimeCall::Utility(..))
+                matches!(c, RuntimeCall::DappsStaking(..))


Tbh, I didn't know that! Good!

Dinonard · 2023-05-02T13:53:54Z

runtime/shiden/src/lib.rs

    CancelProxy,
    DappsStaking,
 }

 impl Default for ProxyType {
    fn default() -> Self {
-        Self::CancelProxy
+        Self::Any


Ok, just hope no one loses their account due to this 😶

Dinonard · 2023-05-02T13:54:32Z

runtime/shiden/src/lib.rs

@@ -848,34 +848,94 @@ impl pallet_xc_asset_config::Config for Runtime {
    scale_info::TypeInfo,
 )]
 pub enum ProxyType {
+    Any,


Please add docs for all types - I believe these are even visible in the UX when you select a type.

Dinonard · 2023-05-02T13:56:02Z

runtime/shiden/src/lib.rs

+            // Always allowed RuntimeCall::Utility no matter type.
+            // Only transactions allowed by Proxy.filter can be executed
+            _ if matches!(c, RuntimeCall::Utility(..)) => true,


I see!

Could we add some tests for this below? 🤔

Dinonard

Nice, now we have some tests for Shiden.

Just a small CR and let's merge it!

Dinonard · 2023-05-04T08:47:11Z

runtime/shiden/src/lib.rs

@@ -940,6 +940,201 @@ impl InstanceFilter<RuntimeCall> for ProxyType {
        }
    }
 }
+#[cfg(test)]


Great! 🙏

Can you please move all the tests at the bottom?
That way we can keep adding on new ones for other pallets too.

Dinonard

LGTM

gitofdeepanshu requested review from Dinonard and shunsukew April 27, 2023 16:06

gitofdeepanshu marked this pull request as ready for review April 27, 2023 16:08

Dinonard reviewed Apr 28, 2023

View reviewed changes

shunsukew approved these changes May 1, 2023

View reviewed changes

shunsukew self-requested a review May 1, 2023 04:42

shunsukew suggested changes May 1, 2023

View reviewed changes

shunsukew self-requested a review May 1, 2023 09:37

shunsukew approved these changes May 1, 2023

View reviewed changes

gitofdeepanshu added 5 commits May 2, 2023 13:26

feat: additional proxy types

f088bfb

chore: fmt

725b7d8

chore: update .toml and spec_version

5df9c9a

chore: add comments for proxy types

dba857a

chore: update .toml version for runtime and collator

3d4957b

gitofdeepanshu force-pushed the feat/pallet-proxy-shiden branch from 2ce20f4 to 3d4957b Compare May 2, 2023 08:09

gitofdeepanshu requested a review from Dinonard May 2, 2023 09:56

Dinonard reviewed May 2, 2023

View reviewed changes

gitofdeepanshu added 2 commits May 3, 2023 21:48

chore: addded docs for ProxyType enum

f71cfdd

test: add test for utility call check

42ff073

gitofdeepanshu requested a review from Dinonard May 4, 2023 03:33

Dinonard reviewed May 4, 2023

View reviewed changes

chore: moved shiden test to bottom

0cf8224

gitofdeepanshu requested a review from Dinonard May 4, 2023 12:01

Dinonard approved these changes May 4, 2023

View reviewed changes

gitofdeepanshu merged commit 66ba05d into master May 4, 2023

gitofdeepanshu deleted the feat/pallet-proxy-shiden branch May 4, 2023 17:55

		ProxyType::Balances => {
		matches!(c, RuntimeCall::Balances(..))

feat: Additional Proxy Types on shiden #920

feat: Additional Proxy Types on shiden #920

Conversation

gitofdeepanshu commented Apr 27, 2023 • edited Loading

Pull Request Summary

Previous Proxy Types

Added Types

Dinonard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitofdeepanshu Apr 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shunsukew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shunsukew left a comment • edited Loading

Choose a reason for hiding this comment

gitofdeepanshu commented May 1, 2023

gitofdeepanshu commented May 1, 2023

shunsukew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dinonard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dinonard left a comment

Choose a reason for hiding this comment

gitofdeepanshu commented Apr 27, 2023 •

edited

Loading

gitofdeepanshu Apr 29, 2023 •

edited

Loading

shunsukew left a comment •

edited

Loading