diff --git a/Cargo.lock b/Cargo.lock index eea76befb9..4fd6571eb9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1554,7 +1554,7 @@ dependencies = [ [[package]] name = "joystream-node" -version = "2.1.3" +version = "2.1.4" dependencies = [ "ctrlc", "derive_more 0.14.1", @@ -1599,7 +1599,7 @@ dependencies = [ [[package]] name = "joystream-node-runtime" -version = "6.9.0" +version = "6.10.0" dependencies = [ "parity-scale-codec", "safe-mix", diff --git a/node/Cargo.toml b/node/Cargo.toml index 12aa4b4890..62394af5a0 100644 --- a/node/Cargo.toml +++ b/node/Cargo.toml @@ -3,7 +3,7 @@ authors = ['Joystream'] build = 'build.rs' edition = '2018' name = 'joystream-node' -version = '2.1.3' +version = '2.1.4' default-run = "joystream-node" [[bin]] diff --git a/runtime-modules/governance/src/election.rs b/runtime-modules/governance/src/election.rs index 7e8c5dcf82..bcb4bf4185 100644 --- a/runtime-modules/governance/src/election.rs +++ b/runtime-modules/governance/src/election.rs @@ -14,6 +14,7 @@ use super::sealed_vote::SealedVote; use super::stake::Stake; use super::council; +use crate::election_params::ElectionParameters; pub use common::currency::{BalanceOf, GovernanceCurrency}; pub trait Trait: @@ -24,6 +25,8 @@ pub trait Trait: type CouncilElected: CouncilElected>, Self::BlockNumber>; } +pub static MSG_CANNOT_CHANGE_PARAMS_DURING_ELECTION: &str = "CannotChangeParamsDuringElection"; + #[derive(Clone, Copy, Encode, Decode)] pub enum ElectionStage { Announcing(BlockNumber), @@ -106,8 +109,23 @@ decl_storage! { // TODO value type of this map looks scary, is there any way to simplify the notation? Votes get(votes): map T::Hash => SealedVote, T::Hash, T::AccountId>; - // Current Election Parameters - default "zero" values are not meaningful. Running an election without - // settings reasonable values is a bad idea. Parameters can be set in the TriggerElection hook. + // Current Election Parameters. + // Should we replace all the individual values with a single ElectionParameters type? + // Having them individually makes it more flexible to add and remove new parameters in future + // without dealing with migration issues. + + // We don't currently handle zero periods, zero council term, zero council size and candidacy + // limit in any special way. The behaviour in such cases: + // Setting any period to 0 will mean the election getting stuck in that stage, until force changing + // the state. + // Council Size of 0 - no limit to size of council, all applicants that move beyond + // announcing stage would become council members, so effectively the candidacy limit will + // be the size of the council, voting and revealing have no impact on final results. + // If candidacy limit is zero and council size > 0, council_size number of applicants will reach the voting stage. + // and become council members, voting will have no impact on final results. + // If both candidacy limit and council size are zero then all applicant become council members + // since no filtering occurs at end of announcing stage. + // We only guard against these edge cases in the set_election_parameters() call. AnnouncingPeriod get(announcing_period) config(): T::BlockNumber = T::BlockNumber::from(100); VotingPeriod get(voting_period) config(): T::BlockNumber = T::BlockNumber::from(100); RevealingPeriod get(revealing_period) config(): T::BlockNumber = T::BlockNumber::from(100); @@ -195,7 +213,7 @@ impl Module { // Take snapshot of seat and backing stakes of an existing council // Its important to note that the election system takes ownership of these stakes, and is responsible - // to return any unused stake to original owners and the end of the election. + // to return any unused stake to original owners at the end of the election. Self::initialize_transferable_stakes(current_council); Self::deposit_event(RawEvent::ElectionStarted()); @@ -803,52 +821,19 @@ decl_module! { >::put(ElectionStage::Voting(ends_at)); } - fn set_param_announcing_period(origin, period: T::BlockNumber) { - ensure_root(origin)?; - ensure!(!Self::is_election_running(), "cannot change params during election"); - ensure!(!period.is_zero(), "period cannot be zero"); - >::put(period); - } - fn set_param_voting_period(origin, period: T::BlockNumber) { - ensure_root(origin)?; - ensure!(!Self::is_election_running(), "cannot change params during election"); - ensure!(!period.is_zero(), "period cannot be zero"); - >::put(period); - } - fn set_param_revealing_period(origin, period: T::BlockNumber) { - ensure_root(origin)?; - ensure!(!Self::is_election_running(), "cannot change params during election"); - ensure!(!period.is_zero(), "period cannot be zero"); - >::put(period); - } - fn set_param_min_council_stake(origin, amount: BalanceOf) { - ensure_root(origin)?; - ensure!(!Self::is_election_running(), "cannot change params during election"); - >::put(amount); - } - fn set_param_new_term_duration(origin, duration: T::BlockNumber) { + fn set_election_parameters(origin, params: ElectionParameters, T::BlockNumber>) { ensure_root(origin)?; - ensure!(!Self::is_election_running(), "cannot change params during election"); - ensure!(!duration.is_zero(), "new term duration cannot be zero"); - >::put(duration); - } - fn set_param_council_size(origin, council_size: u32) { - ensure_root(origin)?; - ensure!(!Self::is_election_running(), "cannot change params during election"); - ensure!(council_size > 0, "council size cannot be zero"); - ensure!(council_size <= Self::candidacy_limit(), "council size cannot greater than candidacy limit"); - CouncilSize::put(council_size); - } - fn set_param_candidacy_limit(origin, limit: u32) { - ensure_root(origin)?; - ensure!(!Self::is_election_running(), "cannot change params during election"); - ensure!(limit >= Self::council_size(), "candidacy limit cannot be less than council size"); - CandidacyLimit::put(limit); - } - fn set_param_min_voting_stake(origin, amount: BalanceOf) { - ensure_root(origin)?; - ensure!(!Self::is_election_running(), "cannot change params during election"); - >::put(amount); + ensure!(!Self::is_election_running(), MSG_CANNOT_CHANGE_PARAMS_DURING_ELECTION); + params.ensure_valid()?; + + >::put(params.announcing_period); + >::put(params.voting_period); + >::put(params.revealing_period); + >::put(params.min_council_stake); + >::put(params.new_term_duration); + CouncilSize::put(params.council_size); + CandidacyLimit::put(params.candidacy_limit); + >::put(params.min_voting_stake); } fn force_stop_election(origin) { @@ -2042,4 +2027,53 @@ mod tests { assert_ok!(Election::start_election(vec![])); }); } + + #[test] + fn setting_election_parameters() { + initial_test_ext().execute_with(|| { + let default_parameters: ElectionParameters = ElectionParameters::default(); + // default all zeros is invalid + assert!(default_parameters.ensure_valid().is_err()); + + let new_parameters = ElectionParameters { + announcing_period: 1, + voting_period: 2, + revealing_period: 3, + council_size: 4, + candidacy_limit: 5, + min_voting_stake: 6, + min_council_stake: 7, + new_term_duration: 8, + }; + + assert_ok!(Election::set_election_parameters( + Origin::ROOT, + new_parameters + )); + + assert_eq!( + >::get(), + new_parameters.announcing_period + ); + assert_eq!(>::get(), new_parameters.voting_period); + assert_eq!( + >::get(), + new_parameters.revealing_period + ); + assert_eq!( + >::get(), + new_parameters.min_council_stake + ); + assert_eq!( + >::get(), + new_parameters.new_term_duration + ); + assert_eq!(CouncilSize::get(), new_parameters.council_size); + assert_eq!(CandidacyLimit::get(), new_parameters.candidacy_limit); + assert_eq!( + >::get(), + new_parameters.min_voting_stake + ); + }); + } } diff --git a/runtime-modules/governance/src/election_params.rs b/runtime-modules/governance/src/election_params.rs new file mode 100644 index 0000000000..99d4404b49 --- /dev/null +++ b/runtime-modules/governance/src/election_params.rs @@ -0,0 +1,45 @@ +use codec::{Decode, Encode}; +use sr_primitives::traits::Zero; +use srml_support::{dispatch::Result, ensure}; + +pub static MSG_PERIOD_CANNOT_BE_ZERO: &str = "PeriodCannotBeZero"; +pub static MSG_COUNCIL_SIZE_CANNOT_BE_ZERO: &str = "CouncilSizeCannotBeZero"; +pub static MSG_CANDIDACY_LIMIT_WAS_LOWER_THAN_COUNCIL_SIZE: &str = + "CandidacyWasLessThanCouncilSize"; + +/// Combined Election parameters, as argument for set_election_parameters +#[derive(Clone, Copy, Encode, Decode, Default, PartialEq, Debug)] +pub struct ElectionParameters { + pub announcing_period: BlockNumber, + pub voting_period: BlockNumber, + pub revealing_period: BlockNumber, + pub council_size: u32, + pub candidacy_limit: u32, + pub new_term_duration: BlockNumber, + pub min_council_stake: Balance, + pub min_voting_stake: Balance, +} + +impl ElectionParameters { + pub fn ensure_valid(&self) -> Result { + self.ensure_periods_are_valid()?; + self.ensure_council_size_and_candidacy_limit_are_valid()?; + Ok(()) + } + + fn ensure_periods_are_valid(&self) -> Result { + ensure!(!self.announcing_period.is_zero(), MSG_PERIOD_CANNOT_BE_ZERO); + ensure!(!self.voting_period.is_zero(), MSG_PERIOD_CANNOT_BE_ZERO); + ensure!(!self.revealing_period.is_zero(), MSG_PERIOD_CANNOT_BE_ZERO); + Ok(()) + } + + fn ensure_council_size_and_candidacy_limit_are_valid(&self) -> Result { + ensure!(self.council_size > 0, MSG_COUNCIL_SIZE_CANNOT_BE_ZERO); + ensure!( + self.council_size <= self.candidacy_limit, + MSG_CANDIDACY_LIMIT_WAS_LOWER_THAN_COUNCIL_SIZE + ); + Ok(()) + } +} diff --git a/runtime-modules/governance/src/lib.rs b/runtime-modules/governance/src/lib.rs index 9e1d712f8b..9b39780d8a 100644 --- a/runtime-modules/governance/src/lib.rs +++ b/runtime-modules/governance/src/lib.rs @@ -3,6 +3,7 @@ pub mod council; pub mod election; +pub mod election_params; pub mod proposals; mod sealed_vote; diff --git a/runtime/Cargo.toml b/runtime/Cargo.toml index 6ff953659c..74b1a68aa3 100644 --- a/runtime/Cargo.toml +++ b/runtime/Cargo.toml @@ -5,7 +5,7 @@ edition = '2018' name = 'joystream-node-runtime' # Follow convention: https://github.com/Joystream/substrate-runtime-joystream/issues/1 # {Authoring}.{Spec}.{Impl} of the RuntimeVersion -version = '6.9.0' +version = '6.10.0' [features] default = ['std'] diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 92c4bbb9f0..5829a0b3b3 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -115,7 +115,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { spec_name: create_runtime_str!("joystream-node"), impl_name: create_runtime_str!("joystream-node"), authoring_version: 6, - spec_version: 9, + spec_version: 10, impl_version: 0, apis: RUNTIME_API_VERSIONS, };