feat(mainnet): add gov: collective, membership, motion #448

peterwht · 2025-01-23T16:56:05Z

[This is a spike and an incomplete PR. But the governance is implemented and can be used]

Adds a Council based governance that:

Has NO elections (selected councillors, to start)
Uses pallet-motion to allow council to use Root origin

To familiarize yourself with this governance mechanism, please use the following guide.

Guide

Goal Checklist

Create a Motion that requires Root

Launch chain pop up parachain -f networks/mainnet.toml
Navigate to Council page and note Members
Navigate to sub-page for Motions
Press "Propose Motion"
Min threshold is currently 1/2 + 1. I recommend setting threshold to 4 (unanimous)
Set the proposal call to motion.unanimous, providing Root origin
Set the sub-call to something that requires Root. One recommendation is to use balances.forceSetBalance
You will see a council.Proposed event
On the Motions page you will see a new Motion available for vote, along with proposal info
Press the Vote button and vote AYE. Repeat this with 4 dev accounts
After the threshold is surpassed, the Vote button now changes to Close. Press this button and submit. The proposal is now passed! You will see several events such as council.voted, council.Approved, council.Executed

Create a Motion that adds a member

Navigate to the Motions page
Press Propose motion
Set threshold 4 (more realistic)
Select councilMembership.addMember for proposal (we do NOT need to use pallet-motion for this). Note, do not use council to set members.
Add Eve or Ferdie (or other as you please)
Press Propose
Go through the voting process and execute the proposal
Wait a bit and refresh the council page. You should now see a new member added that can propose motions and vote.

Create a Motion that removes a member

Navigate to the Motions page
Press Propose motion
Set threshold 4 (more realistic)
Select councilMembership.removeMember for proposal (we do NOT need to use pallet-motion for this). Note, do not use council to set members.
Add Eve or Ferdie (or other as you please)
Press Propose
Go through the voting process and execute the proposal
Wait a bit and refresh the council page. You should now see one less member.

Notes

One important thing to confirm for ANY governance we choose: do we have to pay DOT to propose a runtime upgrade. This can be costly if so.

node/src/chain_spec.rs

runtime/mainnet/src/config/governance.rs

peterwht · 2025-01-23T17:27:59Z

[sc-2571]

al3mart · 2025-01-24T13:41:47Z

Polakdot js apps seems to crash after inputting an external motion 😓 with the expectation of pallet_democracy being present ?

For the record the call I noted as a preimage was balances.forceSetBalance.

I'd assume this is expected as the runtime doesn't feature a way for non council members to cast votes. I think the app will have just wrapped the noted call as a democracy call.
Though is just ugly that the app presents that message.

al3mart

The inclusion of these seemed like a sensible choice. Provides a little better UX for the accounts voting on the motions.

Also seems more flexible looking ahead.

runtime/mainnet/src/config/governance.rs

peterwht · 2025-01-27T21:56:14Z

[sc-2200]

AlexD10S · 2025-01-30T14:21:26Z

The step-by-step guide in the PR description is great. I've been testing and looks good 👌

al3mart · 2025-02-05T00:14:53Z

I have applied the feedback in this thread.

Introducing a more strict configuration in: 25029be
And adding configuration tests in: 78e9e79

update Cargo.lock

codecov-commenter · 2025-02-05T01:31:24Z

Codecov Report

Attention: Patch coverage is 95.06531% with 34 lines in your changes missing coverage. Please review.

Project coverage is 74.42%. Comparing base (168cf85) to head (a76aa2e).

Files with missing lines	Patch %	Lines
pallets/motion/src/weights.rs	50.00%	21 Missing ⚠️
node/src/chain_spec.rs	0.00%	7 Missing ⚠️
pallets/motion/src/lib.rs	82.14%	3 Missing and 2 partials ⚠️
pallets/motion/src/benchmarking.rs	85.71%	0 Missing and 1 partial ⚠️

@@            Coverage Diff             @@
##             main     #448      +/-   ##
==========================================
+ Coverage   73.50%   74.42%   +0.91%     
==========================================
  Files          78       84       +6     
  Lines       15696    16384     +688     
  Branches    15696    16384     +688     
==========================================
+ Hits        11538    12193     +655     
- Misses       3897     3927      +30     
- Partials      261      264       +3

Files with missing lines	Coverage Δ
pallets/motion/src/mock.rs	`100.00% <100.00%> (ø)`
pallets/motion/src/tests.rs	`100.00% <100.00%> (ø)`
runtime/mainnet/src/config/governance.rs	`100.00% <100.00%> (ø)`
runtime/mainnet/src/lib.rs	`81.37% <ø> (ø)`
pallets/motion/src/benchmarking.rs	`85.71% <85.71%> (ø)`
pallets/motion/src/lib.rs	`82.14% <82.14%> (ø)`
node/src/chain_spec.rs	`9.96% <0.00%> (-0.21%)`	⬇️
pallets/motion/src/weights.rs	`50.00% <50.00%> (ø)`

peterwht

Thanks for taking this on!

I left a few comments -- biggest one about the min origin required for root.

In a separate PR (maybe with the genesis config presets), I think it would make our lives easier to have a Dev version of mainnet so we can use the dev accounts. Just noting for later.

FYI: I can't approve this PR as I am the original author.

peterwht · 2025-02-06T04:46:27Z

runtime/mainnet/src/config/governance.rs

+	type SimpleMajorityOrigin = NeverEnsureOrigin<()>;
+	// SuperMajority origin check will always fail.
+	// Making it not possible for SimpleMajority to dispatch as root.
+	type SuperMajorityOrigin = NeverEnsureOrigin<()>;


Frank made a good point about unanimous only for root.

What happens if a vulnerability is discovered, one of the council members is OOO, and we need to immediately push a fix? We would not be able to do this if just one council is OOO for unanimous.

Based on this a 3/4 would be better (enable super majority)

This makes 👍

runtime/mainnet/src/config/governance.rs

peterwht · 2025-02-06T04:50:38Z

runtime/mainnet/src/config/governance.rs

+		use super::*;
+
+		#[test]
+		fn simple_majority_always_fails() {


Suggested change

fn simple_majority_always_fails() {

fn simple_majority_is_never_origin() {

Maybe this is a more accurate name?

peterwht · 2025-02-06T04:50:55Z

runtime/mainnet/src/config/governance.rs

+		}
+
+		#[test]
+		fn super_majority_always_fails() {


if we update to 3/4 for super-majority, this would change.

Co-authored-by: Peter White <petras9789@gmail.com>

evilrobot-01

Saw that prior comments were never submitted, so submitting now in case they are helpful.

evilrobot-01 · 2025-01-27T14:42:40Z

pallets/motion/README.md

@@ -0,0 +1,32 @@
+# Motion Pallet


Can we reference the source of this code, giving proper credit.

evilrobot-01 · 2025-01-27T14:43:25Z

pallets/motion/src/lib.rs

+pub mod weights;
+pub use pallet::*;
+use sp_runtime::DispatchResult;
+use sp_std::prelude::*;


Think sp_std is deprecated, should be an easy fix to update.

evilrobot-01 · 2025-01-27T14:44:26Z

pallets/motion/src/lib.rs

+	pub struct Pallet<T>(_);
+
+	#[pallet::config]
+	pub trait Config: frame_system::Config {


Doc comments on these would be appreciated.

And the rest of the pub types and traits

Minor: sorting these alphabetically would also be appreciated (but not essential), mostly as our rust formatting rules for trait impls do as much automatically, meaning that the ordering is consistent.

evilrobot-01 · 2025-01-27T14:45:33Z

pallets/motion/src/lib.rs

+	#[pallet::event]
+	#[pallet::generate_deposit(pub(super) fn deposit_event)]
+	pub enum Event<T: Config> {
+		/// A SimpleMajority motion was executed. motion_result contains the call result


Suggested change

/// A SimpleMajority motion was executed. motion_result contains the call result

/// A [SimpleMajorityOrigin] motion was executed. [motion_result] contains the call result

Assuming this allows control-clicking on motion_result and it takes one to the field. Same for other variants in the enum.

The origin reference is an assumption.

evilrobot-01 · 2025-01-27T15:10:16Z

networks/mainnet.toml

@@ -34,6 +34,15 @@ balances = [
    ["5CiPPseXPECbkjWCa6MnjNokrgYjMqmKndv2rSnekmSK2DjL", 10000000000000000],
 ]

+[parachains.genesis_overrides.councilMembership]
+members = [


Nice!

Assuming we proceed with this, can we add these directly to the genesis preset/chain-spec for the devnet runtime too? Rationale is having devnet as close to mainnet as practically possible, so devs are familiar with the process from the ground floor. This is based on the assumption that we can still have a 1/1 threshold for devnet so as quick as sudo would be.

will address when including the genesis presets! 👍

evilrobot-01 · 2025-01-27T15:12:15Z

pallets/motion/src/weights.rs

@@ -0,0 +1,90 @@
+


We should ideally regenerate these before merging, only to ensure that it all still works - especially with the mainnet config.

Can be addressed along with the rest of benchmarks that need to be run.

evilrobot-01 · 2025-01-27T15:13:43Z

runtime/mainnet/Cargo.toml

@@ -82,6 +82,11 @@ pallet-collator-selection.workspace = true
 parachain-info.workspace = true
 parachains-common.workspace = true

+# Governance
+pallet-collective.workspace = true


Fine for initial spike, but should be moved to Substrate block before merge.

evilrobot-01 · 2025-02-06T13:36:32Z

runtime/mainnet/src/lib.rs

+	#[runtime::pallet_index(17)]
+	pub type CouncilMembership = pallet_membership::Pallet<Runtime, Instance1>;
+	#[runtime::pallet_index(18)]
+	pub type Motion = pallet_motion::Pallet<Runtime>;


Suggested change

pub type Motion = pallet_motion::Pallet<Runtime>;

pub type Motion = pallet_motion;

Syntax can be simplified.

evilrobot-01 · 2025-02-06T13:37:49Z

runtime/mainnet/src/config/governance.rs

+impl pallet_motion::Config for Runtime {
+	type RuntimeCall = RuntimeCall;
+	type RuntimeEvent = RuntimeEvent;
+	// SimpleMajority origin check will always fail.


That was an unfortunate comment from my side.
I meant to say that the origin check as configured with NeverEnsureOrigin will always fail.

evilrobot-01 · 2025-02-06T13:43:29Z

runtime/mainnet/src/config/governance.rs

+	// Making it not possible for SimpleMajority to dispatch as root.
+	type SimpleMajorityOrigin = NeverEnsureOrigin<()>;
+	// At least 3/4 of the council vote is needed.
+	type SuperMajorityOrigin = EnsureProportionAtLeast<AccountId, CouncilCollective, 3, 4>;


Duplication with type aliases defined above - can they not be reused/shared with pallet_membership?

They can and it looks way better: 6d87835

…aultVote & include test

Daanvdplas

Looking good! Left a few comments

pallets/motion/Cargo.toml

Daanvdplas · 2025-02-07T06:33:55Z

pallets/motion/src/lib.rs

+	pub struct Pallet<T>(_);
+
+	#[pallet::config]
+	pub trait Config: frame_system::Config {


And the rest of the pub types and traits

runtime/mainnet/src/config/governance.rs

Daanvdplas

LGTM!

test(governance): improve test

evilrobot-01

High level review only, with various suggestions which is left up to author to decide on whether they should be addressed or not.

Note: did not look at any of the tests.

evilrobot-01 · 2025-02-07T11:04:21Z

pallets/motion/Cargo.toml

@@ -0,0 +1,51 @@
+[package]
+authors = [ "Substrate DevHub <https://github.com/substrate-developer-hub>" ]


This differs from the credited source? https://github.com/Watr-Protocol/watr/blob/12f372af121e2b62532664eff89c7325427f2165/pallets/motion/Cargo.toml#L4

Suggest it is set to the same as the forked pallet and then possibly add us as well if there have been any improvements made.

pallets/motion/src/benchmarking.rs

evilrobot-01 · 2025-02-07T11:22:20Z

pallets/motion/src/lib.rs

+		#[pallet::call_index(1)]
+		pub fn simple_majority(
+			origin: OriginFor<T>,
+			call: Box<<T as Config>::RuntimeCall>,


Most of these <T as Config>::RuntimeCall can be simplified to T::RuntimeCall if the additional associated types are removed, leaving just the one from the underlying frame_system trait.

Didn't pursue as per: #448 (comment)

https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/sudo/src/lib.rs#L226

latest sudo still does it this way.

evilrobot-01 · 2025-02-07T11:25:58Z

runtime/mainnet/Cargo.toml

@@ -36,6 +37,8 @@ frame-try-runtime.workspace = true
 pallet-aura.workspace = true
 pallet-authorship.workspace = true
 pallet-balances.workspace = true
+pallet-collective.workspace = true
+pallet-membership.workspace = true


FYI - Could be addressed in a follow up, but I spiked a similar implementation without the membership pallet and it seems to work just fine for a small set, which I imagine we will be for the first foreseeable period. Simplifies the implementation.

runtime/mainnet/src/config/governance.rs

evilrobot-01 · 2025-02-07T11:33:17Z

runtime/mainnet/src/config/governance.rs

+
+pub type CouncilMembership = pallet_collective::Instance1;
+impl pallet_membership::Config<CouncilMembership> for Runtime {
+	type AddOrigin = UnanimousCouncilVote;


Interesting. Why does the whole council need to agree to add a member, but only 3/4 to remove? I get that a member may vote nay and block removal, but the same could be said for adding. I vote nay to an add as my voting power/control is being reduced.

I guess this configuration is heavily opinionated.

I'll start by the removal as it's simpler. My assumption was that in case the council needs to remove a bad actor, this will always vote against being removed. Maybe a configuration more similar to "all votes but one" could be way better for this case.

Regarding unanimous additions: My thought was that if we are seeding the council with actors that just want to retain power (not adding member because it results in reduced control) we might as well just run with pallet sudo only. At the same time, more members means more nuanced scenarios, that's why I though that the current councilors should strongly agree on it.

Membership brings flexibility, but also more questions.

RemoveOrigin could look like this: 604bb64.
But it's pretty rigid and can't adapt to on chain activity.

Co-authored-by: Frank Bell <60948618+evilrobot-01@users.noreply.github.com> Update pallets/motion/src/benchmarking.rs Co-authored-by: Frank Bell <60948618+evilrobot-01@users.noreply.github.com> Update pallets/motion/src/benchmarking.rs Co-authored-by: Frank Bell <60948618+evilrobot-01@users.noreply.github.com> Update pallets/motion/src/benchmarking.rs Co-authored-by: Frank Bell <60948618+evilrobot-01@users.noreply.github.com> Update pallets/motion/src/benchmarking.rs Co-authored-by: Frank Bell <60948618+evilrobot-01@users.noreply.github.com>

chore(motion): add R0GUE as author

evilrobot-01

Seems good to me. I have not looked at the tests or config in any detail so would not be comfortable giving it an approval.

Great work in gettting it all neatened up and ready for prime time!

peterwht · 2025-02-07T18:31:48Z

runtime/mainnet/src/config/governance.rs

+type AllMembersButOne = EitherOfDiverse<
+	EnsureRoot<AccountId>,
+	// Currently we set 4 members at genesis.
+	EnsureMembers<AccountId, CouncilCollective, 3>,


what happens if we add or remove a new member. Wouldn't this break?

It will keep on ensuring that at least the provided amount of members voted aye. Which is not ideal if the number of total members changes :/

I'm not specially happy with that option because 3 will only be every member minus one under that concrete condition.

I guess whether this is useful or not depends on how we foresee the council to change in the short term.

I'd be more comfortable merging with AtLeastThreeFourthsOfCouncil than this fixed amount of members.

runtime/mainnet/src/config/governance.rs

Co-authored-by: Peter White <petras9789@gmail.com>

Daanvdplas · 2025-02-11T13:59:36Z

node/src/chain_spec.rs

@@ -238,7 +241,10 @@ fn mainnet_genesis(
 		"polkadotXcm": {
 			"safeXcmVersion": Some(SAFE_XCM_VERSION),
 		},
-		"sudo": { "key": Some(root) }
+		"sudo": { "key": Some(root) },


DQ: do we still need sudo?

Right at the start I'd say it could be convenient.
The pallet conveniently includes a way to kill the SUDO key.

peterwht commented Jan 23, 2025

View reviewed changes

node/src/chain_spec.rs Outdated Show resolved Hide resolved

peterwht commented Jan 23, 2025

View reviewed changes

runtime/mainnet/src/config/governance.rs Outdated Show resolved Hide resolved

peterwht commented Jan 23, 2025

View reviewed changes

runtime/mainnet/src/config/governance.rs Outdated Show resolved Hide resolved

al3mart reviewed Jan 24, 2025

View reviewed changes

peterwht changed the base branch from main to al3mart/refactor-mainnet-governance January 24, 2025 17:44

peterwht changed the base branch from al3mart/refactor-mainnet-governance to main January 24, 2025 17:44

al3mart mentioned this pull request Jan 31, 2025

refactor(mainnet): ready runtime config #458

Draft

12 tasks

peterwht and others added 8 commits February 5, 2025 01:17

feat(pallets): add pallet-motion

45ae67b

build(mainnet): add gov pallets: collective, membership, motion

8d0d7ad

feat(mainnet): add gov configs: collective, membership, motion

e777676

feat(node): add councillors at genesis

33e16e9

TEMPORARY COMMIT: Sets mainnet chain-spec to Development

f8606e3

refactor(governance): apply feedback configuration changes

5a8a374

test(governance): add council, membership & motion tests

b5d76fa

refactor(governance): change mainnet ChainType to Live

c283432

update Cargo.lock

al3mart force-pushed the peter/add-gov branch from ab5418d to c283432 Compare February 5, 2025 00:29

al3mart marked this pull request as ready for review February 5, 2025 03:39

peterwht commented Feb 6, 2025

View reviewed changes

al3mart and others added 3 commits February 6, 2025 14:06

Update runtime/mainnet/src/config/governance.rs

8049245

Co-authored-by: Peter White <petras9789@gmail.com>

refactor(governance): SuperMajorityOrigin is at least 3/4

6196d22

docs(governance): update comments.

1325b57

al3mart requested a review from Daanvdplas February 6, 2025 13:33

al3mart added the ready-for-final-review label Feb 6, 2025

evilrobot-01 reviewed Feb 6, 2025

View reviewed changes

al3mart added 3 commits February 7, 2025 00:41

refactor(motion): revert simple-majority feature

91cca9f

refactor(ci): reenable zepter

f8e92a1

refactor(governance): use MoreThanMajorityThenPrimeDefaultVote as Def…

5cdced0

…aultVote & include test

al3mart requested a review from evilrobot-01 February 7, 2025 00:55

test(governance): runtime upgrade doesn't saturare weight limit

aebe152

Daanvdplas reviewed Feb 7, 2025

View reviewed changes

al3mart added 3 commits February 7, 2025 09:44

fix(motion): remve sp-std dependendency

880fb24

style(governance): use change variable name

5d1d49e

test(governance): remove irrelevant tests

d86f49f

Daanvdplas self-requested a review February 7, 2025 10:11

Daanvdplas approved these changes Feb 7, 2025

View reviewed changes

docs(motion): add documentation to public elements

3c57991

test(governance): improve test

al3mart force-pushed the peter/add-gov branch from fed0d92 to 3c57991 Compare February 7, 2025 11:34

evilrobot-01 reviewed Feb 7, 2025

View reviewed changes

al3mart and others added 2 commits February 7, 2025 12:56

refactor(governance): AllMembersButOne as RemoveOrigin

604bb64

al3mart force-pushed the peter/add-gov branch from 0d639e2 to 604bb64 Compare February 7, 2025 12:22

al3mart added 3 commits February 7, 2025 13:37

chore(motion): update pallet author

0e07b3c

chore(motion): add R0GUE as author

docs(motion): sort documentation

5319e04

chore(motion): fmt

d245359

evilrobot-01 reviewed Feb 7, 2025

View reviewed changes

peterwht commented Feb 7, 2025

View reviewed changes

runtime/mainnet/src/config/governance.rs Outdated Show resolved Hide resolved

peterwht commented Feb 7, 2025

View reviewed changes

runtime/mainnet/src/config/governance.rs Outdated Show resolved Hide resolved

al3mart and others added 2 commits February 7, 2025 21:27

Update runtime/mainnet/src/config/governance.rs

8790017

Co-authored-by: Peter White <petras9789@gmail.com>

revert 604bb64

a76aa2e

al3mart requested review from Daanvdplas, evilrobot-01 and a team February 10, 2025 16:54

Daanvdplas reviewed Feb 11, 2025

View reviewed changes

	fn simple_majority_always_fails() {
	fn simple_majority_is_never_origin() {

	/// A SimpleMajority motion was executed. motion_result contains the call result
	/// A [SimpleMajorityOrigin] motion was executed. [motion_result] contains the call result

	pub type Motion = pallet_motion::Pallet<Runtime>;
	pub type Motion = pallet_motion;

		@@ -0,0 +1,51 @@
		[package]
		authors = [ "Substrate DevHub <https://github.com/substrate-developer-hub>" ]

feat(mainnet): add gov: collective, membership, motion #448

Are you sure you want to change the base?

feat(mainnet): add gov: collective, membership, motion #448

Conversation

peterwht commented Jan 23, 2025 • edited Loading

Guide

Goal Checklist

Create a Motion that requires Root

Create a Motion that adds a member

Create a Motion that removes a member

Notes

peterwht commented Jan 23, 2025

al3mart commented Jan 24, 2025 • edited Loading

al3mart left a comment

Choose a reason for hiding this comment

peterwht commented Jan 27, 2025

AlexD10S commented Jan 30, 2025

al3mart commented Feb 5, 2025 • edited Loading

codecov-commenter commented Feb 5, 2025 • edited Loading

Codecov Report

peterwht 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

evilrobot-01 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

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

Daanvdplas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daanvdplas left a comment

Choose a reason for hiding this comment

evilrobot-01 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

Choose a reason for hiding this comment

al3mart Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

evilrobot-01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

al3mart Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

al3mart Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

peterwht commented Jan 23, 2025 •

edited

Loading

al3mart commented Jan 24, 2025 •

edited

Loading

al3mart commented Feb 5, 2025 •

edited

Loading

codecov-commenter commented Feb 5, 2025 •

edited

Loading

al3mart Feb 7, 2025 •

edited

Loading

al3mart Feb 7, 2025 •

edited

Loading

al3mart Feb 11, 2025 •

edited

Loading