Add Registrar proxy type #358

nanocryk · 2023-12-04T09:55:43Z

Adds new Registrar and SudoRegistrar proxy types.

…xy-type

github-actions · 2023-12-04T15:15:36Z

Coverage Report

@@                      Coverage Diff                       @@
##           master   jeremy-registar-proxy-type      +/-   ##
==============================================================
+ Coverage   73.94%                       74.06%   +0.12%     
  Files          91                           91              
+ Lines       20536                        20637     +101     
==============================================================
+ Hits        15184                        15283      +99     
+ Misses       5352                         5354       +2

Files Changed	Coverage
/container-chains/templates/frontier/runtime/src/lib.rs	42.72% (-0.42%)	🔽
/container-chains/templates/simple/runtime/src/lib.rs	57.43% (-0.88%)	🔽
/pallets/registrar/src/lib.rs	92.18% (+0.25%)	🔼
/runtime/dancebox/src/lib.rs	81.00% (+1.27%)	🔼
/runtime/dancebox/tests/integration_test.rs	99.70% (+0.01%)	🔼

Coverage generated Tue Dec 19 15:14:25 UTC 2023

tmpolaczyk · 2023-12-04T15:26:07Z

Let's add a typescript test, I guess it will be a combination of these 2:

test/suites/common/proxy/test-proxy-balances.ts
test/suites/dev-tanssi/registrar/test_registrar_register.ts

girazoki · 2023-12-06T16:11:01Z

test/suites/dev-tanssi/registrar/test_registrar_proxy.ts

+                    null,
+                    polkadotJs.tx.registrar.register(2002, containerChainGenesisData)
+                );
+                const tx3 = polkadotJs.tx.registrar.markValidForCollating(2002);


Isnt it sufficient for the test that we test only that the genesis data gets set? the rest is made with sudo

girazoki · 2023-12-06T16:12:52Z

runtime/dancebox/src/lib.rs

@@ -940,6 +942,9 @@ impl InstanceFilter<RuntimeCall> for ProxyType {
            ProxyType::Balances => {
                matches!(c, RuntimeCall::Balances(..) | RuntimeCall::Utility(..))
            }
+            ProxyType::Registrar => {


Can we also add a proxy type called SUdoRegistrar that is sable to call with sudo any of the registrar calls? similar to https://github.com/paritytech/polkadot-sdk/blob/e7651cf41b2d775dedcc897e17ead16e200a4fff/polkadot/runtime/westend/src/lib.rs#L1064

tmpolaczyk · 2023-12-11T10:46:02Z

test/suites/dev-tanssi/registrar/test_registrar_proxy.ts

+                const onChainBootnodes = await polkadotJs.query.registrar.bootNodes(2002);
+                // TODO: fix once we have types
+                expect(onChainBootnodes.toHuman()).to.deep.equal(["dummy"]);
+            },


This test could also check that the registrar deposit is set to alice when charlie registers the chain using proxy. And also add a test for sudoRegistrar, it should be easy, try calling sudo(markValidForCollating) through the proxy.

runtime/dancebox/src/lib.rs

tmpolaczyk · 2023-12-11T10:49:21Z

runtime/dancebox/src/lib.rs

+                    RuntimeCall::Sudo(pallet_sudo::Call::sudo {call: ref x})
+                    if matches!(x.as_ref(), RuntimeCall::Registrar(..))


Compared to SudoBalances this is missing RuntimeCall::Utility, but I guess that's good, we don't want the proxy to be able to call sudo(utility.dispatchAs)... But what about allowing utility.batch*, that should be ok?

I am actually not entirely sure what would happen if we enable utility here, but I see that parity has it enabled in a similar proxy type: https://github.com/paritytech/polkadot-sdk/blob/4c4407a8930267e139db93019c01444acf49e1ce/polkadot/runtime/westend/src/lib.rs#L1066

@nanocryk can you try it out?

I was wrong, it depends on how you define the proxy:

Sudo(Registrar) || Utility this is fine
Sudo(Registrar || Utility) this is bad

The current way of always allowing utility should be ok as well

tmpolaczyk · 2023-12-14T14:05:57Z

Cargo.lock

@@ -853,7 +853,7 @@ dependencies = [
 [[package]]
 name = "binary-merkle-tree"
 version = "4.0.0-dev"
-source = "git+https://github.com/moondance-labs/polkadot-sdk?branch=tanssi-polkadot-v1.3.0#c3d18c8c2f4c5462b752b98eb2908dc32b5ec985"


Can you merge master? It should remove the changes in Cargo.lock

…xy-type

tmpolaczyk · 2023-12-14T14:19:00Z

runtime/dancebox/tests/integration_test.rs

+        ProxyType::Balances,
+        ProxyType::Registrar,
+        ProxyType::SudoRegistrar,
+    ];


There are some external crates that allow iterating over enums, not sure if we already have one, but as a nice hack we could use the Decode trait (not tested but should work):

for x in 0u8.. { if let Ok(x) = x.decode::<ProxyType>() { proxy_types.push(x); } else { break; } }

tmpolaczyk · 2023-12-14T14:29:46Z

test/suites/dev-tanssi/registrar/test_registrar_proxy.ts

+                const tx3 = polkadotJs.tx.proxy.proxy(
+                    bob.address,
+                    null,
+                    polkadotJs.tx.registrar.setBootNodes(2002, ["dummy"])


Sorry, in #341 we moved the setBootNodes to a different pallet, pallet_data_preservers. I guess we want that to be included in the SudoRegistrar proxy as well. So this should work:

Suggested change

polkadotJs.tx.registrar.setBootNodes(2002, ["dummy"])

polkadotJs.tx.dataPreservers.setBootNodes(2002, ["dummy"])

yes exactly

runtime/dancebox/src/lib.rs

girazoki · 2023-12-15T09:13:15Z

Please put a description in the PR and let's merge master back in

…xy-type

runtime/dancebox/src/lib.rs

Co-authored-by: tmpolaczyk <[email protected]>

…xy-type

runtime/dancebox/src/lib.rs

Co-authored-by: tmpolaczyk <[email protected]>

tmpolaczyk · 2023-12-19T09:31:48Z

runtime/dancebox/tests/integration_test.rs

+                    None,
+                    Box::new(
+                        pallet_utility::Call::batch {
+                            calls: vec![pallet_balances::Call::force_set_balance {


force_set_balance will always fail because it needs root origin, maybe this test is missing a call to sudo first?

yes the test needs to be batch->sudo.>force_set_balacne

But ProxyType::Any should allow this, right? Then why doesn't the test fail?

hmm that's a good question

its probably becaues we are still not going through the sudo pallet right?

registrar proxy

547c918

nanocryk added a0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes not-breaking Does not need to be mentioned in breaking changes labels Dec 4, 2023

Merge remote-tracking branch 'origin/master' into jeremy-registar-pro…

5f9deee

…xy-type

typescript api

3c3ab73

wip test

bd9ad7a

girazoki reviewed Dec 6, 2023

View reviewed changes

girazoki and others added 2 commits December 6, 2023 18:02

simplify test

bf6add5

proxy sudo registar

0fe62d1

tmpolaczyk reviewed Dec 11, 2023

View reviewed changes

runtime/dancebox/src/lib.rs Outdated Show resolved Hide resolved

tmpolaczyk reviewed Dec 11, 2023

View reviewed changes

nanocryk added 4 commits December 11, 2023 15:51

wip test for sudo registrar proxy

2b0ced8

wip

58d5901

fix tests

75389dd

allow utility call for all proxies (with non privilege escalation test)

2ed805d

tmpolaczyk reviewed Dec 14, 2023

View reviewed changes

Merge remote-tracking branch 'origin/master' into jeremy-registar-pro…

4670784

…xy-type

tmpolaczyk reviewed Dec 14, 2023

View reviewed changes

wip fix test after merge

ab9ebca

tmpolaczyk reviewed Dec 14, 2023

View reviewed changes

runtime/dancebox/src/lib.rs Show resolved Hide resolved

nanocryk added 2 commits December 15, 2023 11:08

apply proxy utility change to all runtimes + fmt

d2e3b73

Merge remote-tracking branch 'origin/master' into jeremy-registar-pro…

0e835ac

…xy-type

tmpolaczyk reviewed Dec 15, 2023

View reviewed changes

runtime/dancebox/src/lib.rs Outdated Show resolved Hide resolved

nanocryk and others added 3 commits December 18, 2023 14:52

Add DataPreservers calls to filter

188d93b

Co-authored-by: tmpolaczyk <[email protected]>

Merge remote-tracking branch 'origin/master' into jeremy-registar-pro…

ae69abe

…xy-type

fmt

2de2437

tmpolaczyk reviewed Dec 18, 2023

View reviewed changes

runtime/dancebox/src/lib.rs Outdated Show resolved Hide resolved

nanocryk and others added 3 commits December 18, 2023 17:45

Add DataPreservers calls to Registar proxy

3232944

Co-authored-by: tmpolaczyk <[email protected]>

lint:fix

c4927e2

fmt

d10da31

tmpolaczyk reviewed Dec 19, 2023

View reviewed changes

girazoki approved these changes Dec 19, 2023

View reviewed changes

nanocryk and others added 2 commits December 19, 2023 13:52

improve test

383761c

fix sudo key set it

c3114c5

tmpolaczyk approved these changes Dec 19, 2023

View reviewed changes

nanocryk merged commit aad62b8 into master Dec 19, 2023
26 checks passed

nanocryk deleted the jeremy-registar-proxy-type branch December 19, 2023 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Registrar proxy type #358

Add Registrar proxy type #358

nanocryk commented Dec 4, 2023 •

edited

Loading

github-actions bot commented Dec 4, 2023 •

edited

Loading

tmpolaczyk commented Dec 4, 2023

girazoki Dec 6, 2023

girazoki Dec 6, 2023

tmpolaczyk Dec 11, 2023

tmpolaczyk Dec 11, 2023

girazoki Dec 13, 2023

tmpolaczyk Dec 14, 2023

tmpolaczyk Dec 14, 2023

tmpolaczyk Dec 14, 2023

tmpolaczyk Dec 14, 2023

girazoki Dec 14, 2023

girazoki commented Dec 15, 2023

tmpolaczyk Dec 19, 2023

girazoki Dec 19, 2023

tmpolaczyk Dec 19, 2023

girazoki Dec 19, 2023

girazoki Dec 19, 2023

		RuntimeCall::Sudo(pallet_sudo::Call::sudo {call: ref x})
		if matches!(x.as_ref(), RuntimeCall::Registrar(..))

	polkadotJs.tx.registrar.setBootNodes(2002, ["dummy"])
	polkadotJs.tx.dataPreservers.setBootNodes(2002, ["dummy"])

Add Registrar proxy type #358

Add Registrar proxy type #358

Conversation

nanocryk commented Dec 4, 2023 • edited Loading

github-actions bot commented Dec 4, 2023 • edited Loading

tmpolaczyk commented Dec 4, 2023

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

girazoki commented Dec 15, 2023

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

nanocryk commented Dec 4, 2023 •

edited

Loading

github-actions bot commented Dec 4, 2023 •

edited

Loading