-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(mainnet): add gov: collective, membership, motion #448
base: main
Are you sure you want to change the base?
Changes from 28 commits
45ae67b
8d0d7ad
e777676
33e16e9
f8606e3
5a8a374
b5d76fa
c283432
8049245
6196d22
1325b57
6d87835
7207135
31a9f52
508e3aa
f9a573b
1d1304b
85242e8
3c02141
99a4f24
c0cc278
91cca9f
f8e92a1
5cdced0
aebe152
880fb24
5d1d49e
d86f49f
3c57991
01efc94
604bb64
0e07b3c
5319e04
d245359
8790017
a76aa2e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -198,6 +198,8 @@ pub fn mainnet_chain_spec(relay: Relay) -> MainnetChainSpec { | |
], | ||
sudo_account_id, | ||
para_id.into(), | ||
// councillors | ||
vec![], | ||
)) | ||
.with_protocol_id("pop") | ||
.with_properties(properties) | ||
|
@@ -208,6 +210,7 @@ fn mainnet_genesis( | |
invulnerables: Vec<(AccountId, AuraId)>, | ||
root: AccountId, | ||
id: ParaId, | ||
councillors: Vec<AccountId>, | ||
) -> serde_json::Value { | ||
use pop_runtime_mainnet::EXISTENTIAL_DEPOSIT; | ||
|
||
|
@@ -238,7 +241,10 @@ fn mainnet_genesis( | |
"polkadotXcm": { | ||
"safeXcmVersion": Some(SAFE_XCM_VERSION), | ||
}, | ||
"sudo": { "key": Some(root) } | ||
"sudo": { "key": Some(root) }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DQ: do we still need sudo? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right at the start I'd say it could be convenient. |
||
"councilMembership": { | ||
"members": councillors, | ||
} | ||
}) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
[package] | ||
authors = [ "Substrate DevHub <https://github.com/substrate-developer-hub>" ] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
description = "FRAME pallet to wrap council calls providing root origin to the council." | ||
edition = { workspace = true } | ||
homepage = { workspace = true } | ||
license = { workspace = true } | ||
name = "pallet-motion" | ||
publish = false | ||
repository = { workspace = true } | ||
version = "4.0.0-dev" | ||
|
||
[package.metadata.docs.rs] | ||
targets = [ "x86_64-unknown-linux-gnu" ] | ||
|
||
[dependencies] | ||
codec = { workspace = true, default-features = false, features = [ | ||
"derive", | ||
] } | ||
frame-benchmarking = { workspace = true, default-features = false, optional = true } | ||
frame-support = { workspace = true } | ||
frame-system = { workspace = true } | ||
log = { workspace = true, default-features = false } | ||
pallet-collective = { workspace = true, default-features = false } | ||
scale-info = { workspace = true, default-features = false, features = [ | ||
"derive", | ||
] } | ||
sp-runtime = { workspace = true, default-features = false } | ||
|
||
[dev-dependencies] | ||
pallet-balances = { workspace = true, default-features = false } | ||
sp-core = { workspace = true, default-features = false } | ||
sp-io = { workspace = true, default-features = false } | ||
|
||
[features] | ||
default = [ "std" ] | ||
runtime-benchmarks = [ | ||
"frame-benchmarking/runtime-benchmarks", | ||
"pallet-collective/runtime-benchmarks", | ||
] | ||
std = [ | ||
"codec/std", | ||
"frame-benchmarking?/std", | ||
"frame-support/std", | ||
"frame-system/std", | ||
"log/std", | ||
"pallet-balances/std", | ||
"pallet-collective/std", | ||
"scale-info/std", | ||
"sp-runtime/std", | ||
] | ||
try-runtime = [ "frame-support/try-runtime" ] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
# Motion Pallet | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we reference the source of this code, giving proper credit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
`pallet-motion` enables councils (`pallet-collective`) to make root-origin calls by providing three configurable origins: | ||
- `SimpleMajority` (1/2) | ||
- `SuperMajority` (2/3) | ||
- `Unanimous` (1/1) | ||
|
||
It is composed of three associated extrinsics, one for each origin: | ||
- `simple_majority` | ||
- Dispatches the call if and only if the number of votes is greater than `SimpleMajority`. | ||
- `super_majority` | ||
- Dispatches the call if and only if the number of votes is greater than or equal to `SuperMajority`. | ||
- `unanimous` | ||
- Dispatches the call if and only if all collective members have voted yes. | ||
|
||
|
||
## Configuration | ||
|
||
You can configure `pallet-motion` in combination with `pallet-collective` the following way: | ||
|
||
```rust | ||
type CouncilCollective = pallet_collective::Instance1; | ||
impl pallet_motion::Config for Runtime { | ||
// --- | ||
type SimpleMajorityOrigin = | ||
pallet_collective::EnsureProportionMoreThan<AccountId, CouncilCollective, 1, 2>; | ||
type SuperMajorityOrigin = | ||
pallet_collective::EnsureProportionAtLeast<AccountId, CouncilCollective, 2, 3>; | ||
type UnanimousOrigin = | ||
pallet_collective::EnsureProportionAtLeast<AccountId, CouncilCollective, 1, 1>; | ||
} | ||
``` | ||
|
||
--- | ||
|
||
### Credit: | ||
|
||
Original source at: https://github.com/Watr-Protocol/watr/tree/main/pallets/motion |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
//! Benchmarking setup for pallet-motion | ||
|
||
use frame_benchmarking::v2::*; | ||
use frame_support::traits::EnsureOrigin; | ||
|
||
use super::*; | ||
#[allow(unused)] | ||
use crate::Pallet as Motion; | ||
|
||
fn assert_last_event<T: Config>(generic_event: <T as Config>::RuntimeEvent) { | ||
frame_system::Pallet::<T>::assert_last_event(generic_event.into()); | ||
} | ||
|
||
#[benchmarks( | ||
where | ||
<T as Config>::RuntimeCall: From<frame_system::Call<T>>, | ||
)] | ||
mod benchmarks { | ||
use super::*; | ||
|
||
#[benchmark] | ||
fn simple_majority() { | ||
let call: <T as Config>::RuntimeCall = frame_system::Call::remark { remark: vec![] }.into(); | ||
let origin = <T as Config>::SimpleMajorityOrigin::try_successful_origin().unwrap(); | ||
#[extrinsic_call] | ||
al3mart marked this conversation as resolved.
Show resolved
Hide resolved
|
||
_(origin as T::RuntimeOrigin, Box::new(call)); | ||
|
||
assert_last_event::<T>(Event::DispatchSimpleMajority { motion_result: Ok(()) }.into()) | ||
} | ||
|
||
#[benchmark] | ||
fn super_majority() { | ||
let call: <T as Config>::RuntimeCall = frame_system::Call::remark { remark: vec![] }.into(); | ||
let origin = <T as Config>::SuperMajorityOrigin::try_successful_origin().unwrap(); | ||
#[extrinsic_call] | ||
al3mart marked this conversation as resolved.
Show resolved
Hide resolved
|
||
_(origin as T::RuntimeOrigin, Box::new(call)); | ||
|
||
assert_last_event::<T>(Event::DispatchSuperMajority { motion_result: Ok(()) }.into()) | ||
} | ||
|
||
#[benchmark] | ||
fn unanimous() { | ||
let call: <T as Config>::RuntimeCall = frame_system::Call::remark { remark: vec![] }.into(); | ||
let origin = <T as Config>::UnanimousOrigin::try_successful_origin().unwrap(); | ||
#[extrinsic_call] | ||
al3mart marked this conversation as resolved.
Show resolved
Hide resolved
|
||
_(origin as T::RuntimeOrigin, Box::new(call)); | ||
|
||
assert_last_event::<T>(Event::DispatchUnanimous { motion_result: Ok(()) }.into()) | ||
} | ||
impl_benchmark_test_suite!(Motion, crate::mock::new_test_ext(), crate::mock::Test); | ||
al3mart marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,154 @@ | ||
#![cfg_attr(not(feature = "std"), no_std)] | ||
|
||
#[cfg(test)] | ||
mod mock; | ||
#[cfg(test)] | ||
mod tests; | ||
|
||
#[cfg(feature = "runtime-benchmarks")] | ||
mod benchmarking; | ||
|
||
pub mod weights; | ||
pub use pallet::*; | ||
use sp_runtime::DispatchResult; | ||
pub use weights::WeightInfo; | ||
|
||
extern crate alloc; | ||
use alloc::{boxed::Box, vec}; | ||
|
||
#[frame_support::pallet] | ||
pub mod pallet { | ||
use frame_support::{ | ||
dispatch::GetDispatchInfo, pallet_prelude::*, traits::UnfilteredDispatchable, | ||
}; | ||
use frame_system::pallet_prelude::*; | ||
|
||
use super::{DispatchResult, *}; | ||
|
||
#[pallet::pallet] | ||
pub struct Pallet<T>(_); | ||
|
||
#[pallet::config] | ||
pub trait Config: frame_system::Config { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doc comments on these would be appreciated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And the rest of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
/// The runtime event type. | ||
type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can get rid of these Runtime* types now by simply adding bounds on the underlying type, removing the need to define them in the runtime. I have done so successfully elsewhere, YMMV tho. i.e. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think would be a great improvement!
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably as you were using |
||
/// The runtime call type. | ||
type RuntimeCall: Parameter | ||
+ UnfilteredDispatchable<RuntimeOrigin = Self::RuntimeOrigin> | ||
+ GetDispatchInfo; | ||
/// Origin that can act as `Root` origin if a collective has achieved a simple majority | ||
/// consensus. | ||
type SimpleMajorityOrigin: EnsureOrigin<Self::RuntimeOrigin>; | ||
/// Origin that can act as `Root` origin if a collective has achieved a super majority | ||
/// consensus. | ||
type SuperMajorityOrigin: EnsureOrigin<Self::RuntimeOrigin>; | ||
/// Origin that can act as `Root` origin if a collective has achieved a unanimous consensus. | ||
type UnanimousOrigin: EnsureOrigin<Self::RuntimeOrigin>; | ||
/// Type representing the weight of this pallet | ||
type WeightInfo: WeightInfo; | ||
} | ||
|
||
#[pallet::event] | ||
#[pallet::generate_deposit(pub(super) fn deposit_event)] | ||
pub enum Event<T: Config> { | ||
/// A [SimpleMajorityOrigin] motion was executed. [motion_result] contains the call result | ||
DispatchSimpleMajority { motion_result: DispatchResult }, | ||
Check warning on line 55 in pallets/motion/src/lib.rs
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: missing doc comments for struct fields. I would personally add if it were me, but not a blocker. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice catch, thanks! |
||
/// A [SuperMajorityOrigin] motion was executed. [motion_result] contains the call result | ||
DispatchSuperMajority { motion_result: DispatchResult }, | ||
Check warning on line 57 in pallets/motion/src/lib.rs
|
||
/// A [UnanimousOrigin] motion was executed. [motion_result] contains the call result | ||
DispatchUnanimous { motion_result: DispatchResult }, | ||
Check warning on line 59 in pallets/motion/src/lib.rs
|
||
} | ||
|
||
#[pallet::error] | ||
pub enum Error<T> {} | ||
|
||
#[pallet::call] | ||
impl<T: Config> Pallet<T> { | ||
/// Ensures the simple majority is met and dispatches a call with `Root` origin. | ||
/// | ||
/// # <weight> | ||
/// - O(1). | ||
/// - Limited storage reads. | ||
/// - One DB write (event). | ||
/// - Weight of derivative `call` execution + 10,000. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question! My understanding is that it represents the cost of wrapping the actual dispatch. But would love if @peterwht could clarify this so I can incorporate that into the docs or remove it all together if it is not the case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's been a couple years. But pallet-motion was similar to Sudo. If you go back to an older version of Sudo you can find this comment: https://github.com/paritytech/substrate/blob/monthly-2022-02/frame/sudo/src/lib.rs#L136 But now that we have proper benchmarking, the adding 10_000 shouldn't be necessary. |
||
/// # </weight> | ||
#[pallet::weight({ | ||
let dispatch_info = call.get_dispatch_info(); | ||
(T::WeightInfo::simple_majority().saturating_add(dispatch_info.call_weight), dispatch_info.class) | ||
})] | ||
#[pallet::call_index(1)] | ||
pub fn simple_majority( | ||
origin: OriginFor<T>, | ||
call: Box<<T as Config>::RuntimeCall>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most of these There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't pursue as per: #448 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/sudo/src/lib.rs#L226 latest sudo still does it this way. |
||
) -> DispatchResultWithPostInfo { | ||
T::SimpleMajorityOrigin::ensure_origin(origin)?; | ||
|
||
let motion_result = Self::do_dispatch(*call); | ||
Self::deposit_event(Event::DispatchSimpleMajority { motion_result }); | ||
|
||
Ok(Pays::No.into()) | ||
} | ||
|
||
/// Ensures the super majority is met and dispatches a call with `Root` origin. | ||
/// | ||
/// # <weight> | ||
/// - O(1). | ||
/// - Limited storage reads. | ||
/// - One DB write (event). | ||
/// - Weight of derivative `call` execution + 10,000. | ||
/// # </weight> | ||
#[pallet::weight({ | ||
let dispatch_info = call.get_dispatch_info(); | ||
(T::WeightInfo::super_majority().saturating_add(dispatch_info.call_weight), dispatch_info.class) | ||
})] | ||
#[pallet::call_index(2)] | ||
pub fn super_majority( | ||
origin: OriginFor<T>, | ||
call: Box<<T as Config>::RuntimeCall>, | ||
) -> DispatchResultWithPostInfo { | ||
T::SuperMajorityOrigin::ensure_origin(origin)?; | ||
|
||
let motion_result = Self::do_dispatch(*call); | ||
Self::deposit_event(Event::DispatchSuperMajority { motion_result }); | ||
|
||
Ok(Pays::No.into()) | ||
} | ||
|
||
/// Ensures unanimous voting is met and dispatches a call with `Root` origin. | ||
/// | ||
/// # <weight> | ||
/// - O(1). | ||
/// - Limited storage reads. | ||
/// - One DB write (event). | ||
/// - Weight of derivative `call` execution + 10,000. | ||
/// # </weight> | ||
#[pallet::weight({ | ||
let dispatch_info = call.get_dispatch_info(); | ||
(T::WeightInfo::unanimous().saturating_add(dispatch_info.call_weight), dispatch_info.class) | ||
})] | ||
#[pallet::call_index(3)] | ||
pub fn unanimous( | ||
origin: OriginFor<T>, | ||
call: Box<<T as Config>::RuntimeCall>, | ||
) -> DispatchResultWithPostInfo { | ||
T::UnanimousOrigin::ensure_origin(origin)?; | ||
|
||
let motion_result = Self::do_dispatch(*call); | ||
Self::deposit_event(Event::DispatchUnanimous { motion_result }); | ||
|
||
Ok(Pays::No.into()) | ||
} | ||
} | ||
|
||
impl<T: Config> Pallet<T> { | ||
/// Helper to actually dispatch RuntimeCall. | ||
/// | ||
/// Should only be called after the origin is ensured. | ||
/// | ||
/// Returns the `DispatchResult` from the dispatched call. | ||
fn do_dispatch(call: <T as Config>::RuntimeCall) -> DispatchResult { | ||
let res = call.dispatch_bypass_filter(frame_system::RawOrigin::Root.into()); | ||
res.map(|_| ()).map_err(|e| e.error) | ||
} | ||
} | ||
} | ||
Check warning on line 154 in pallets/motion/src/lib.rs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will address when including the genesis presets! 👍