Merge branch 'development' into proposals_v2_iteration7

# Conflicts:
#	runtime-modules/governance/src/lib.rs

Cargo.lock

@@ -1565,7 +1565,7 @@ dependencies = [
 
 [[package]]
 name = "joystream-node"
-version = "2.1.3"
+version = "2.1.5"
 dependencies = [
  "ctrlc",
  "derive_more 0.14.1",
@@ -1610,7 +1610,7 @@ dependencies = [
 
 [[package]]
 name = "joystream-node-runtime"
-version = "6.9.0"
+version = "6.11.0"
 dependencies = [
  "parity-scale-codec",
  "safe-mix",

node/Cargo.toml

@@ -3,7 +3,7 @@ authors = ['Joystream']
 build = 'build.rs'
 edition = '2018'
 name = 'joystream-node'
-version = '2.1.3'
+version = '2.1.5'
 default-run = "joystream-node"
 
 [[bin]]

runtime-modules/content-working-group/src/lib.rs

@@ -1910,103 +1910,23 @@ decl_module! {
             );
         }
 
-        /*
-         * Root origin routines for managing lead.
-         */
-
-
-        /// Introduce a lead when one is not currently set.
-        pub fn set_lead(origin, member: T::MemberId, role_account: T::AccountId) {
-
+        /// Replace the current lead. First unsets the active lead if there is one.
+        /// If a value is provided for new_lead it will then set that new lead.
+        /// It is responsibility of the caller to ensure the new lead can be set
+        /// to avoid the lead role being vacant at the end of the call.
+        pub fn replace_lead(origin, new_lead: Option<(T::MemberId, T::AccountId)>) {
             // Ensure root is origin
             ensure_root(origin)?;
 
-            // Ensure there is no current lead
-            ensure!(
-                <CurrentLeadId<T>>::get().is_none(),
-                MSG_CURRENT_LEAD_ALREADY_SET
-            );
-
-            // Ensure that member can actually become lead
-            let new_lead_id = <NextLeadId<T>>::get();
-
-            let new_lead_role =
-                role_types::ActorInRole::new(role_types::Role::CuratorLead, new_lead_id);
-
-            let _profile = <members::Module<T>>::can_register_role_on_member(
-                &member,
-                &role_types::ActorInRole::new(role_types::Role::CuratorLead, new_lead_id),
-            )?;
-
-            //
-            // == MUTATION SAFE ==
-            //
-
-            // Construct lead
-            let new_lead = Lead {
-                role_account: role_account.clone(),
-                reward_relationship: None,
-                inducted: <system::Module<T>>::block_number(),
-                stage: LeadRoleState::Active,
-            };
-
-            // Store lead
-            <LeadById<T>>::insert(new_lead_id, new_lead);
-
-            // Update current lead
-            <CurrentLeadId<T>>::put(new_lead_id); // Some(new_lead_id)
-
-            // Update next lead counter
-            <NextLeadId<T>>::mutate(|id| *id += <LeadId<T> as One>::one());
-
-            // Register in role
-            let registered_role =
-                <members::Module<T>>::register_role_on_member(member, &new_lead_role).is_ok();
-
-            assert!(registered_role);
-
-            // Trigger event
-            Self::deposit_event(RawEvent::LeadSet(new_lead_id));
-        }
-
-        /// Evict the currently unset lead
-        pub fn unset_lead(origin) {
-
-            // Ensure root is origin
-            ensure_root(origin)?;
-
-            // Ensure there is a lead set
-            let (lead_id,lead) = Self::ensure_lead_is_set()?;
-
-            //
-            // == MUTATION SAFE ==
-            //
-
-            // Unregister from role in membership model
-            let current_lead_role = role_types::ActorInRole{
-                role: role_types::Role::CuratorLead,
-                actor_id: lead_id
-            };
-
-            let unregistered_role = <members::Module<T>>::unregister_role(current_lead_role).is_ok();
-
-            assert!(unregistered_role);
-
-            // Update lead stage as exited
-            let current_block = <system::Module<T>>::block_number();
-
-            let new_lead = Lead{
-                stage: LeadRoleState::Exited(ExitedLeadRole { initiated_at_block_number: current_block}),
-                ..lead
-            };
-
-            <LeadById<T>>::insert(lead_id, new_lead);
-
-            // Update current lead
-            <CurrentLeadId<T>>::take(); // None
+            // Unset current lead first
+            if Self::ensure_lead_is_set().is_ok() {
+                Self::unset_lead()?;
+            }
 
-            // Trigger event
-            Self::deposit_event(RawEvent::LeadUnset(lead_id));
+            // Try to set new lead
+            if let Some((member_id, role_account)) = new_lead {
+                Self::set_lead(member_id, role_account)?;
+            }
         }
 
         /// Add an opening for a curator role.
@@ -2122,6 +2042,87 @@ impl<T: Trait> versioned_store_permissions::CredentialChecker<T> for Module<T> {
 }
 
 impl<T: Trait> Module<T> {
+    /// Introduce a lead when one is not currently set.
+    fn set_lead(member: T::MemberId, role_account: T::AccountId) -> dispatch::Result {
+        // Ensure there is no current lead
+        ensure!(
+            <CurrentLeadId<T>>::get().is_none(),
+            MSG_CURRENT_LEAD_ALREADY_SET
+        );
+
+        let new_lead_id = <NextLeadId<T>>::get();
+
+        let new_lead_role =
+            role_types::ActorInRole::new(role_types::Role::CuratorLead, new_lead_id);
+
+        //
+        // == MUTATION SAFE ==
+        //
+
+        // Register in role - will fail if member cannot become lead
+        members::Module::<T>::register_role_on_member(member, &new_lead_role)?;
+
+        // Construct lead
+        let new_lead = Lead {
+            role_account: role_account.clone(),
+            reward_relationship: None,
+            inducted: <system::Module<T>>::block_number(),
+            stage: LeadRoleState::Active,
+        };
+
+        // Store lead
+        <LeadById<T>>::insert(new_lead_id, new_lead);
+
+        // Update current lead
+        <CurrentLeadId<T>>::put(new_lead_id); // Some(new_lead_id)
+
+        // Update next lead counter
+        <NextLeadId<T>>::mutate(|id| *id += <LeadId<T> as One>::one());
+
+        // Trigger event
+        Self::deposit_event(RawEvent::LeadSet(new_lead_id));
+
+        Ok(())
+    }
+
+    /// Evict the currently set lead
+    fn unset_lead() -> dispatch::Result {
+        // Ensure there is a lead set
+        let (lead_id, lead) = Self::ensure_lead_is_set()?;
+
+        //
+        // == MUTATION SAFE ==
+        //
+
+        // Unregister from role in membership model
+        let current_lead_role = role_types::ActorInRole {
+            role: role_types::Role::CuratorLead,
+            actor_id: lead_id,
+        };
+
+        <members::Module<T>>::unregister_role(current_lead_role)?;
+
+        // Update lead stage as exited
+        let current_block = <system::Module<T>>::block_number();
+
+        let new_lead = Lead {
+            stage: LeadRoleState::Exited(ExitedLeadRole {
+                initiated_at_block_number: current_block,
+            }),
+            ..lead
+        };
+
+        <LeadById<T>>::insert(lead_id, new_lead);
+
+        // Update current lead
+        <CurrentLeadId<T>>::take(); // None
+
+        // Trigger event
+        Self::deposit_event(RawEvent::LeadUnset(lead_id));
+
+        Ok(())
+    }
+
     fn ensure_member_has_no_active_application_on_opening(
         curator_applications: CuratorApplicationIdSet<T>,
         member_id: T::MemberId,

runtime-modules/content-working-group/src/tests.rs

@@ -1160,7 +1160,10 @@ struct SetLeadFixture {
 
 impl SetLeadFixture {
     fn call(&self) -> Result<(), &'static str> {
-        ContentWorkingGroup::set_lead(self.origin.clone(), self.member_id, self.new_role_account)
+        ContentWorkingGroup::replace_lead(
+            self.origin.clone(),
+            Some((self.member_id, self.new_role_account)),
+        )
     }
 
     pub fn call_and_assert_success(&self) {
@@ -1221,7 +1224,7 @@ struct UnsetLeadFixture {
 
 impl UnsetLeadFixture {
     fn call(&self) -> Result<(), &'static str> {
-        ContentWorkingGroup::unset_lead(self.origin.clone())
+        ContentWorkingGroup::replace_lead(self.origin.clone(), None)
     }
 
     pub fn call_and_assert_success(&self) {
@@ -2121,10 +2124,9 @@ pub fn set_lead(
 
     // Set lead
     assert_eq!(
-        ContentWorkingGroup::set_lead(
+        ContentWorkingGroup::replace_lead(
             mock::Origin::system(system::RawOrigin::Root),
-            member_id,
-            new_role_account
+            Some((member_id, new_role_account))
         )
         .unwrap(),
         ()

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<Seats<Self::AccountId, BalanceOf<Self>>, Self::BlockNumber>;
 }
 
+pub static MSG_CANNOT_CHANGE_PARAMS_DURING_ELECTION: &str = "CannotChangeParamsDuringElection";
+
 #[derive(Clone, Copy, Encode, Decode)]
 pub enum ElectionStage<BlockNumber> {
     Announcing(BlockNumber),
@@ -119,8 +122,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::AccountId, ElectionStake<T>, 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);
@@ -208,7 +226,7 @@ impl<T: Trait> Module<T> {
 
         // 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());
@@ -816,52 +834,19 @@ decl_module! {
             <Stage<T>>::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");
-            <AnnouncingPeriod<T>>::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");
-            <VotingPeriod<T>>::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");
-            <RevealingPeriod<T>>::put(period);
-        }
-        fn set_param_min_council_stake(origin, amount: BalanceOf<T>) {
-            ensure_root(origin)?;
-            ensure!(!Self::is_election_running(), "cannot change params during election");
-            <MinCouncilStake<T>>::put(amount);
-        }
-        fn set_param_new_term_duration(origin, duration: T::BlockNumber) {
+        fn set_election_parameters(origin, params: ElectionParameters<BalanceOf<T>, 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");
-            <NewTermDuration<T>>::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<T>) {
-            ensure_root(origin)?;
-            ensure!(!Self::is_election_running(), "cannot change params during election");
-            <MinVotingStake<T>>::put(amount);
+            ensure!(!Self::is_election_running(), MSG_CANNOT_CHANGE_PARAMS_DURING_ELECTION);
+            params.ensure_valid()?;
+
+            <AnnouncingPeriod<T>>::put(params.announcing_period);
+            <VotingPeriod<T>>::put(params.voting_period);
+            <RevealingPeriod<T>>::put(params.revealing_period);
+            <MinCouncilStake<T>>::put(params.min_council_stake);
+            <NewTermDuration<T>>::put(params.new_term_duration);
+            CouncilSize::put(params.council_size);
+            CandidacyLimit::put(params.candidacy_limit);
+            <MinVotingStake<T>>::put(params.min_voting_stake);
         }
 
         fn force_stop_election(origin) {
@@ -2055,4 +2040,53 @@ mod tests {
             assert_ok!(Election::start_election(vec![]));
         });
     }
+
+    #[test]
+    fn setting_election_parameters() {
+        initial_test_ext().execute_with(|| {
+            let default_parameters: ElectionParameters<u64, u64> = 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!(
+                <AnnouncingPeriod<Test>>::get(),
+                new_parameters.announcing_period
+            );
+            assert_eq!(<VotingPeriod<Test>>::get(), new_parameters.voting_period);
+            assert_eq!(
+                <RevealingPeriod<Test>>::get(),
+                new_parameters.revealing_period
+            );
+            assert_eq!(
+                <MinCouncilStake<Test>>::get(),
+                new_parameters.min_council_stake
+            );
+            assert_eq!(
+                <NewTermDuration<Test>>::get(),
+                new_parameters.new_term_duration
+            );
+            assert_eq!(CouncilSize::get(), new_parameters.council_size);
+            assert_eq!(CandidacyLimit::get(), new_parameters.candidacy_limit);
+            assert_eq!(
+                <MinVotingStake<Test>>::get(),
+                new_parameters.min_voting_stake
+            );
+        });
+    }
 }

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<Balance, BlockNumber> {
+    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<Balance, BlockNumber: PartialOrd + Zero> ElectionParameters<Balance, BlockNumber> {
+    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(())
+    }
+}

runtime-modules/governance/src/lib.rs

@@ -3,6 +3,7 @@
 
 pub mod council;
 pub mod election;
+pub mod election_params;
 
 mod sealed_vote;
 mod stake;

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.11.0'
 
 [features]
 default = ['std']

runtime/src/lib.rs

@@ -125,7 +125,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: 11,
     impl_version: 0,
     apis: RUNTIME_API_VERSIONS,
 };