Migrate data object type registry module with bureaucracy module

- change ensure_root() to the ensure_origin_is_active_leader()
- update tests
- add comments

runtime-modules/bureaucracy/src/lib.rs

@@ -432,7 +432,7 @@ decl_module! {
         ) {
 
             // Ensure lead is set and is origin signer
-            Self::ensure_origin_is_set_lead(origin)?;
+            Self::ensure_origin_is_active_leader(origin)?;
 
             // Ensuring worker actually exists
             let worker = Self::ensure_worker_exists(&worker_id)?;
@@ -463,7 +463,7 @@ decl_module! {
             human_readable_text: Vec<u8>
         ){
             // Ensure lead is set and is origin signer
-            Self::ensure_origin_is_set_lead(origin)?;
+            Self::ensure_origin_is_active_leader(origin)?;
 
             Self::ensure_opening_human_readable_text_is_valid(&human_readable_text)?;
 
@@ -509,7 +509,7 @@ decl_module! {
         pub fn accept_worker_applications(origin, worker_opening_id: WorkerOpeningId<T>)  {
 
             // Ensure lead is set and is origin signer
-            Self::ensure_origin_is_set_lead(origin)?;
+            Self::ensure_origin_is_active_leader(origin)?;
 
             // Ensure opening exists in this working group
             // NB: Even though call to hiring module will have implicit check for
@@ -664,7 +664,7 @@ decl_module! {
         ) {
 
             // Ensure lead is set and is origin signer
-            Self::ensure_origin_is_set_lead(origin)?;
+            Self::ensure_origin_is_active_leader(origin)?;
 
             // Ensuring worker application actually exists
             let (worker_application, _, worker_opening) = Self::ensure_worker_application_exists(&worker_application_id)?;
@@ -691,7 +691,7 @@ decl_module! {
         pub fn begin_worker_applicant_review(origin, worker_opening_id: WorkerOpeningId<T>) {
 
             // Ensure lead is set and is origin signer
-            Self::ensure_origin_is_set_lead(origin)?;
+            Self::ensure_origin_is_active_leader(origin)?;
 
             // Ensure opening exists
             // NB: Even though call to hiring modul will have implicit check for
@@ -721,7 +721,7 @@ decl_module! {
             reward_policy: Option<RewardPolicy<minting::BalanceOf<T>, T::BlockNumber>>
         ) {
             // Ensure lead is set and is origin signer
-            Self::ensure_origin_is_set_lead(origin)?;
+            Self::ensure_origin_is_active_leader(origin)?;
 
             // Ensure worker opening exists
             let (worker_opening, _) = Self::ensure_worker_opening_exists(&worker_opening_id)?;
@@ -862,7 +862,7 @@ decl_module! {
         /// Slashes the worker stake, demands a leader origin. No limits, no actions on zero stake.
         /// If slashing balance greater than the existing stake - stake is slashed to zero.
         pub fn slash_worker_stake(origin, worker_id: WorkerId<T>, balance: BalanceOf<T>) {
-            Self::ensure_origin_is_set_lead(origin)?;
+            Self::ensure_origin_is_active_leader(origin)?;
 
             let worker = Self::ensure_worker_exists(&worker_id)?;
 
@@ -889,7 +889,7 @@ decl_module! {
         /// Decreases the worker stake and returns the remainder to the worker role_account,
         /// demands a leader origin. Can be decreased to zero, no actions on zero stake.
         pub fn decrease_worker_stake(origin, worker_id: WorkerId<T>, balance: BalanceOf<T>) {
-            Self::ensure_origin_is_set_lead(origin)?;
+            Self::ensure_origin_is_active_leader(origin)?;
 
             let worker = Self::ensure_worker_exists(&worker_id)?;
 
@@ -995,7 +995,8 @@ impl<T: Trait<I>, I: Instance> Module<T, I> {
             .map_err(|e| e.into())
     }
 
-    fn ensure_origin_is_set_lead(origin: T::Origin) -> Result<(), Error> {
+    // Ensures origin is signed by the leader.
+    pub fn ensure_origin_is_active_leader(origin: T::Origin) -> Result<(), Error> {
         // Ensure is signed
         let signer = ensure_signed(origin)?;
 

runtime-modules/storage/src/data_object_type_registry.rs

@@ -1,3 +1,20 @@
+//! # Data object type registry module
+//! Data object type registry module for the Joystream platform.
+//! It allows to set constraints for the data objects. All extrinsics require leader
+//!
+//! ## Comments
+//!
+//! Data object type registry module uses bureaucracy module to authorize actions. Only leader can
+//! call extrinsics.
+//!
+//! ## Supported extrinsics
+//!
+//! - [register_data_object_type](./struct.Module.html#method.register_data_object_type) - Registers the new data object type.
+//! - [update_data_object_type](./struct.Module.html#method.update_data_object_type)- Updates existing data object type.
+//! - [activate_data_object_type](./struct.Module.html#method.activate_data_object_type) -  Activates existing data object type.
+//! - [deactivate_data_object_type](./struct.Module.html#method.deactivate_data_object_type) -  Deactivates existing data object type.
+//!
+
 // Clippy linter requirement
 // disable it because of the substrate lib design
 // example:   NextDataObjectTypeId get(next_data_object_type_id) build(|config: &GenesisConfig<T>|
@@ -8,8 +25,11 @@ use codec::{Codec, Decode, Encode};
 use rstd::prelude::*;
 use sr_primitives::traits::{MaybeSerialize, Member, SimpleArithmetic};
 use srml_support::{decl_event, decl_module, decl_storage, Parameter};
-use system::ensure_root;
 
+// Alias for storage working group bureaucracy
+pub(crate) type StorageBureaucracy<T> = bureaucracy::Module<T, bureaucracy::Instance2>;
+
+/// The _Data object type registry_ main _Trait_
 pub trait Trait: system::Trait + bureaucracy::Trait<bureaucracy::Instance2> {
     type Event: From<Event<Self>> + Into<<Self as system::Trait>::Event>;
 
@@ -23,17 +43,21 @@ pub trait Trait: system::Trait + bureaucracy::Trait<bureaucracy::Instance2> {
         + PartialEq;
 }
 
-static MSG_DO_TYPE_NOT_FOUND: &str = "Data Object Type with the given ID not found.";
-
+// TODO: migrate to the Substate error style
+const MSG_DO_TYPE_NOT_FOUND: &str = "Data Object Type with the given ID not found.";
 const DEFAULT_TYPE_DESCRIPTION: &str = "Default data object type for audio and video content.";
+
 const DEFAULT_TYPE_ACTIVE: bool = true;
 const CREATE_DETAULT_TYPE: bool = true;
-
 const DEFAULT_FIRST_DATA_OBJECT_TYPE_ID: u32 = 1;
 
+/// Contains description and constrains for the data object.
 #[derive(Clone, Encode, Decode, PartialEq, Debug)]
 pub struct DataObjectType {
+    /// Data object description.
     pub description: Vec<u8>,
+
+    /// Active/Disabled flag.
     pub active: bool,
     // TODO in future releases
     // - maximum size
@@ -55,36 +79,37 @@ decl_storage! {
 
         // TODO hardcode data object type for ID 1
 
-        // Start at this value
+        /// Data object type ids should start at this value.
         pub FirstDataObjectTypeId get(first_data_object_type_id) config(first_data_object_type_id): T::DataObjectTypeId = T::DataObjectTypeId::from(DEFAULT_FIRST_DATA_OBJECT_TYPE_ID);
 
-        // Increment
+        /// Provides id counter for the data object types.
         pub NextDataObjectTypeId get(next_data_object_type_id) build(|config: &GenesisConfig<T>| config.first_data_object_type_id): T::DataObjectTypeId = T::DataObjectTypeId::from(DEFAULT_FIRST_DATA_OBJECT_TYPE_ID);
 
-        // Mapping of Data object types
+        /// Mapping of Data object types.
         pub DataObjectTypes get(data_object_types): map T::DataObjectTypeId => Option<DataObjectType>;
     }
 }
 
 decl_event! {
+    /// _Data object type registry_ events
     pub enum Event<T> where
         <T as Trait>::DataObjectTypeId {
+        /// Emits on the data object type registration.
+        /// Params:
+        /// - Id of the new data object type.
         DataObjectTypeRegistered(DataObjectTypeId),
-        DataObjectTypeUpdated(DataObjectTypeId),
-    }
-}
 
-impl<T: Trait> traits::IsActiveDataObjectType<T> for Module<T> {
-    fn is_active_data_object_type(which: &T::DataObjectTypeId) -> bool {
-        match Self::ensure_data_object_type(*which) {
-            Ok(do_type) => do_type.active,
-            Err(_err) => false,
-        }
+        /// Emits on the data object type update.
+        /// Params:
+        /// - Id of the updated data object type.
+        DataObjectTypeUpdated(DataObjectTypeId),
     }
 }
 
 decl_module! {
+    /// _Data object type registry_ substrate module.
     pub struct Module<T: Trait> for enum Call where origin: T::Origin {
+        /// Default deposit_event() handler
         fn deposit_event() = default;
 
         fn on_initialize() {
@@ -98,8 +123,10 @@ decl_module! {
             }
         }
 
+        /// Registers the new data object type. Requires leader privileges.
         pub fn register_data_object_type(origin, data_object_type: DataObjectType) {
-            ensure_root(origin)?;
+            <StorageBureaucracy<T>>::ensure_origin_is_active_leader(origin)?;
+
             let new_do_type_id = Self::next_data_object_type_id();
             let do_type: DataObjectType = DataObjectType {
                 description: data_object_type.description.clone(),
@@ -112,9 +139,10 @@ decl_module! {
             Self::deposit_event(RawEvent::DataObjectTypeRegistered(new_do_type_id));
         }
 
-        // TODO use DataObjectTypeUpdate
+        /// Updates existing data object type. Requires leader privileges.
         pub fn update_data_object_type(origin, id: T::DataObjectTypeId, data_object_type: DataObjectType) {
-            ensure_root(origin)?;
+            <StorageBureaucracy<T>>::ensure_origin_is_active_leader(origin)?;
+
             let mut do_type = Self::ensure_data_object_type(id)?;
 
             do_type.description = data_object_type.description.clone();
@@ -125,12 +153,10 @@ decl_module! {
             Self::deposit_event(RawEvent::DataObjectTypeUpdated(id));
         }
 
-        // Activate and deactivate functions as separate functions, because
-        // toggling DO types is likely a more common operation than updating
-        // other aspects.
-        // TODO deprecate or express via update_data_type
+        /// Activates existing data object type. Requires leader privileges.
         pub fn activate_data_object_type(origin, id: T::DataObjectTypeId) {
-            ensure_root(origin)?;
+            <StorageBureaucracy<T>>::ensure_origin_is_active_leader(origin)?;
+
             let mut do_type = Self::ensure_data_object_type(id)?;
 
             do_type.active = true;
@@ -140,8 +166,10 @@ decl_module! {
             Self::deposit_event(RawEvent::DataObjectTypeUpdated(id));
         }
 
+        /// Deactivates existing data object type. Requires leader privileges.
         pub fn deactivate_data_object_type(origin, id: T::DataObjectTypeId) {
-            ensure_root(origin)?;
+            <StorageBureaucracy<T>>::ensure_origin_is_active_leader(origin)?;
+
             let mut do_type = Self::ensure_data_object_type(id)?;
 
             do_type.active = false;
@@ -158,3 +186,12 @@ impl<T: Trait> Module<T> {
         Self::data_object_types(&id).ok_or(MSG_DO_TYPE_NOT_FOUND)
     }
 }
+
+impl<T: Trait> traits::IsActiveDataObjectType<T> for Module<T> {
+    fn is_active_data_object_type(which: &T::DataObjectTypeId) -> bool {
+        match Self::ensure_data_object_type(*which) {
+            Ok(do_type) => do_type.active,
+            Err(_err) => false,
+        }
+    }
+}

runtime-modules/storage/src/tests/data_object_type_registry.rs

@@ -1,8 +1,35 @@
 #![cfg(test)]
 
 use super::mock::*;
+use crate::data_object_type_registry::StorageBureaucracy;
+use system::{self, EventRecord, Phase, RawOrigin};
 
-use system::{self, EventRecord, Phase};
+const DEFAULT_LEADER_ACCOUNT_ID: u64 = 1;
+const DEFAULT_LEADER_MEMBER_ID: u64 = 1;
+
+struct SetLeadFixture;
+impl SetLeadFixture {
+    fn set_default_lead() {
+        let set_lead_result = <StorageBureaucracy<Test>>::set_lead(
+            RawOrigin::Root.into(),
+            DEFAULT_LEADER_MEMBER_ID,
+            DEFAULT_LEADER_ACCOUNT_ID,
+        );
+        assert!(set_lead_result.is_ok());
+    }
+}
+
+fn get_last_data_object_type_id() -> u64 {
+    let dot_id = match System::events().last().unwrap().event {
+        MetaEvent::data_object_type_registry(
+            data_object_type_registry::RawEvent::DataObjectTypeRegistered(dot_id),
+        ) => dot_id,
+        _ => 0xdeadbeefu64, // unlikely value
+    };
+    assert_ne!(dot_id, 0xdeadbeefu64);
+
+    dot_id
+}
 
 #[test]
 fn initial_state() {
@@ -17,12 +44,14 @@ fn initial_state() {
 #[test]
 fn succeed_register() {
     with_default_mock_builder(|| {
+        SetLeadFixture::set_default_lead();
+
         let data: TestDataObjectType = TestDataObjectType {
             description: "foo".as_bytes().to_vec(),
             active: false,
         };
         let res = TestDataObjectTypeRegistry::register_data_object_type(
-            system::RawOrigin::Root.into(),
+            RawOrigin::Signed(DEFAULT_LEADER_ACCOUNT_ID).into(),
             data,
         );
         assert!(res.is_ok());
@@ -30,26 +59,108 @@ fn succeed_register() {
 }
 
 #[test]
-fn update_existing() {
+fn activate_data_object_type_fails_with_invalid_lead() {
     with_default_mock_builder(|| {
+        SetLeadFixture::set_default_lead();
+
         // First register a type
         let data: TestDataObjectType = TestDataObjectType {
             description: "foo".as_bytes().to_vec(),
             active: false,
         };
         let id_res = TestDataObjectTypeRegistry::register_data_object_type(
-            system::RawOrigin::Root.into(),
+            RawOrigin::Signed(DEFAULT_LEADER_ACCOUNT_ID).into(),
             data,
         );
         assert!(id_res.is_ok());
 
-        let dot_id = match System::events().last().unwrap().event {
-            MetaEvent::data_object_type_registry(
-                data_object_type_registry::RawEvent::DataObjectTypeRegistered(dot_id),
-            ) => dot_id,
-            _ => 0xdeadbeefu64, // unlikely value
+        let dot_id = get_last_data_object_type_id();
+
+        let invalid_leader_account_id = 2;
+        let res = TestDataObjectTypeRegistry::activate_data_object_type(
+            RawOrigin::Signed(invalid_leader_account_id).into(),
+            dot_id,
+        );
+        assert_eq!(res, Err(bureaucracy::Error::IsNotLeadAccount.into()));
+    });
+}
+
+#[test]
+fn deactivate_data_object_type_fails_with_invalid_lead() {
+    with_default_mock_builder(|| {
+        SetLeadFixture::set_default_lead();
+
+        // First register a type
+        let data: TestDataObjectType = TestDataObjectType {
+            description: "foo".as_bytes().to_vec(),
+            active: true,
         };
-        assert_ne!(dot_id, 0xdeadbeefu64);
+        let id_res = TestDataObjectTypeRegistry::register_data_object_type(
+            RawOrigin::Signed(DEFAULT_LEADER_ACCOUNT_ID).into(),
+            data,
+        );
+        assert!(id_res.is_ok());
+
+        let dot_id = get_last_data_object_type_id();
+
+        let invalid_leader_account_id = 2;
+        let res = TestDataObjectTypeRegistry::deactivate_data_object_type(
+            RawOrigin::Signed(invalid_leader_account_id).into(),
+            dot_id,
+        );
+        assert_eq!(res, Err(bureaucracy::Error::IsNotLeadAccount.into()));
+    });
+}
+
+#[test]
+fn update_data_object_type_fails_with_invalid_lead() {
+    with_default_mock_builder(|| {
+        SetLeadFixture::set_default_lead();
+
+        // First register a type
+        let data: TestDataObjectType = TestDataObjectType {
+            description: "foo".as_bytes().to_vec(),
+            active: false,
+        };
+        let id_res = TestDataObjectTypeRegistry::register_data_object_type(
+            RawOrigin::Signed(DEFAULT_LEADER_ACCOUNT_ID).into(),
+            data,
+        );
+        assert!(id_res.is_ok());
+
+        let dot_id = get_last_data_object_type_id();
+        let updated1: TestDataObjectType = TestDataObjectType {
+            description: "bar".as_bytes().to_vec(),
+            active: false,
+        };
+
+        let invalid_leader_account_id = 2;
+        let res = TestDataObjectTypeRegistry::update_data_object_type(
+            RawOrigin::Signed(invalid_leader_account_id).into(),
+            dot_id,
+            updated1,
+        );
+        assert_eq!(res, Err(bureaucracy::Error::IsNotLeadAccount.into()));
+    });
+}
+
+#[test]
+fn update_existing() {
+    with_default_mock_builder(|| {
+        SetLeadFixture::set_default_lead();
+
+        // First register a type
+        let data: TestDataObjectType = TestDataObjectType {
+            description: "foo".as_bytes().to_vec(),
+            active: false,
+        };
+        let id_res = TestDataObjectTypeRegistry::register_data_object_type(
+            RawOrigin::Signed(DEFAULT_LEADER_ACCOUNT_ID).into(),
+            data,
+        );
+        assert!(id_res.is_ok());
+
+        let dot_id = get_last_data_object_type_id();
 
         // Now update it with new data - we need the ID to be the same as in
         // returned by the previous call. First, though, try and fail with a bad ID
@@ -58,7 +169,7 @@ fn update_existing() {
             active: false,
         };
         let res = TestDataObjectTypeRegistry::update_data_object_type(
-            system::RawOrigin::Root.into(),
+            RawOrigin::Signed(DEFAULT_LEADER_ACCOUNT_ID).into(),
             dot_id + 1,
             updated1,
         );
@@ -70,7 +181,7 @@ fn update_existing() {
             active: false,
         };
         let res = TestDataObjectTypeRegistry::update_data_object_type(
-            system::RawOrigin::Root.into(),
+            RawOrigin::Signed(DEFAULT_LEADER_ACCOUNT_ID).into(),
             dot_id,
             updated3,
         );
@@ -88,16 +199,34 @@ fn update_existing() {
     });
 }
 
+#[test]
+fn register_data_object_type_failed_with_no_lead() {
+    with_default_mock_builder(|| {
+        // First register a type
+        let data: TestDataObjectType = TestDataObjectType {
+            description: "foo".as_bytes().to_vec(),
+            active: false,
+        };
+        let id_res = TestDataObjectTypeRegistry::register_data_object_type(
+            RawOrigin::Signed(DEFAULT_LEADER_ACCOUNT_ID).into(),
+            data,
+        );
+        assert!(!id_res.is_ok());
+    });
+}
+
 #[test]
 fn activate_existing() {
     with_default_mock_builder(|| {
+        SetLeadFixture::set_default_lead();
+
         // First register a type
         let data: TestDataObjectType = TestDataObjectType {
             description: "foo".as_bytes().to_vec(),
             active: false,
         };
         let id_res = TestDataObjectTypeRegistry::register_data_object_type(
-            system::RawOrigin::Root.into(),
+            RawOrigin::Signed(DEFAULT_LEADER_ACCOUNT_ID).into(),
             data,
         );
         assert!(id_res.is_ok());
@@ -121,7 +250,7 @@ fn activate_existing() {
 
         // Now activate the data object type
         let res = TestDataObjectTypeRegistry::activate_data_object_type(
-            system::RawOrigin::Root.into(),
+            RawOrigin::Signed(DEFAULT_LEADER_ACCOUNT_ID).into(),
             TEST_FIRST_DATA_OBJECT_TYPE_ID,
         );
         assert!(res.is_ok());
@@ -145,7 +274,7 @@ fn activate_existing() {
 
         // Deactivate again.
         let res = TestDataObjectTypeRegistry::deactivate_data_object_type(
-            system::RawOrigin::Root.into(),
+            RawOrigin::Signed(DEFAULT_LEADER_ACCOUNT_ID).into(),
             TEST_FIRST_DATA_OBJECT_TYPE_ID,
         );
         assert!(res.is_ok());

runtime-modules/storage/src/tests/mock.rs

@@ -195,7 +195,7 @@ impl data_object_storage_registry::Trait for Test {
 
 impl members::Trait for Test {
     type Event = MetaEvent;
-    type MemberId = u32;
+    type MemberId = u64;
     type SubscriptionId = u32;
     type PaidTermId = u32;
     type ActorId = u32;