Update service discovery module

- rename AccountInfoByAccountId to the AccountInfoByStorageProviderId
-add module comments
- modify tests
- modify runtime

Cargo.lock

@@ -5375,10 +5375,18 @@ dependencies = [
  "sr-io",
  "sr-primitives",
  "sr-std",
+ "srml-balances",
  "srml-support",
  "srml-system",
+ "srml-timestamp",
+ "substrate-bureaucracy-module",
+ "substrate-common-module",
+ "substrate-hiring-module",
+ "substrate-membership-module",
  "substrate-primitives",
- "substrate-roles-module",
+ "substrate-recurring-reward-module",
+ "substrate-stake-module",
+ "substrate-token-mint-module",
 ]
 
 [[package]]
@@ -5461,7 +5469,6 @@ dependencies = [
  "substrate-membership-module",
  "substrate-primitives",
  "substrate-recurring-reward-module",
- "substrate-roles-module",
  "substrate-stake-module",
  "substrate-token-mint-module",
 ]

runtime-modules/service-discovery/Cargo.toml

@@ -14,7 +14,7 @@ std = [
 	'serde',
     'codec/std',
     'primitives/std',
-    'roles/std',
+    'bureaucracy/std',
 ]
 
 [dependencies.sr-primitives]
@@ -29,6 +29,11 @@ git = 'https://github.com/paritytech/substrate.git'
 package = 'srml-support'
 rev = 'c37bb08535c49a12320af7facfd555ce05cce2e8'
 
+[dependencies.common]
+default_features = false
+package = 'substrate-common-module'
+path = '../common'
+
 [dependencies.system]
 default_features = false
 git = 'https://github.com/paritytech/substrate.git'
@@ -56,7 +61,7 @@ rev = 'c37bb08535c49a12320af7facfd555ce05cce2e8'
 default-features = false
 features = ['derive']
 package = 'parity-scale-codec'
-version = '1.0.0'
+version = '1.1.0'
 
 [dev-dependencies.runtime-io]
 default_features = false
@@ -64,7 +69,44 @@ git = 'https://github.com/paritytech/substrate.git'
 package = 'sr-io'
 rev = 'c37bb08535c49a12320af7facfd555ce05cce2e8'
 
-[dependencies.roles]
+[dependencies.bureaucracy]
+default_features = false
+package = 'substrate-bureaucracy-module'
+path = '../bureaucracy'
+
+[dev-dependencies.balances]
+default_features = false
+git = 'https://github.com/paritytech/substrate.git'
+package = 'srml-balances'
+rev = 'c37bb08535c49a12320af7facfd555ce05cce2e8'
+
+[dev-dependencies.recurringrewards]
+default_features = false
+package = 'substrate-recurring-reward-module'
+path = '../recurring-reward'
+
+[dev-dependencies.hiring]
+default_features = false
+package = 'substrate-hiring-module'
+path = '../hiring'
+
+[dev-dependencies.stake]
+default_features = false
+package = 'substrate-stake-module'
+path = '../stake'
+
+[dev-dependencies.minting]
 default_features = false
-package = 'substrate-roles-module'
-path = '../roles'
+package = 'substrate-token-mint-module'
+path = '../token-minting'
+
+[dev-dependencies.membership]
+default_features = false
+package = 'substrate-membership-module'
+path = '../membership'
+
+[dev-dependencies.timestamp]
+default_features = false
+git = 'https://github.com/paritytech/substrate.git'
+package = 'srml-timestamp'
+rev = 'c37bb08535c49a12320af7facfd555ce05cce2e8'

runtime-modules/service-discovery/src/discovery.rs

@@ -1,127 +0,0 @@
-use codec::{Decode, Encode};
-use roles::traits::Roles;
-use rstd::prelude::*;
-#[cfg(feature = "std")]
-use serde::{Deserialize, Serialize};
-
-use srml_support::{decl_event, decl_module, decl_storage, ensure};
-use system::{self, ensure_root, ensure_signed};
-/*
-  Although there is support for ed25519 keys as the IPNS identity key and we could potentially
-  reuse the same key for the role account and ipns (and make this discovery module obselete)
-  it is probably better to separate concerns.
-  Why not to use a fixed size 32byte -> SHA256 hash of public key: because we would have to force
-  specific key type on ipfs side.
-  pub struct IPNSIdentity(pub [u8; 32]); // we loose the key type!
-  pub type IPNSIdentity(pub u8, pub [u8; 32]); // we could add the keytype?
-  can we use rust library in wasm runtime?
-  https://github.com/multiformats/rust-multihash
-  https://github.com/multiformats/multicodec/
-  https://github.com/multiformats/multihash/
-*/
-/// base58 encoded IPNS identity multihash codec
-pub type IPNSIdentity = Vec<u8>;
-
-/// HTTP Url string to a discovery service endpoint
-pub type Url = Vec<u8>;
-
-pub const MINIMUM_LIFETIME: u32 = 600; // 1hr assuming 6s block times
-pub const DEFAULT_LIFETIME: u32 = MINIMUM_LIFETIME * 24; // 24hr
-
-#[cfg_attr(feature = "std", derive(Serialize, Deserialize, Debug))]
-#[derive(Encode, Decode, Default, Clone, PartialEq, Eq)]
-pub struct AccountInfo<BlockNumber> {
-    /// IPNS Identity
-    pub identity: IPNSIdentity,
-    /// Block at which information expires
-    pub expires_at: BlockNumber,
-}
-
-pub trait Trait: system::Trait {
-    type Event: From<Event<Self>> + Into<<Self as system::Trait>::Event>;
-
-    type Roles: Roles<Self>;
-}
-
-decl_storage! {
-    trait Store for Module<T: Trait> as Discovery {
-        /// Bootstrap endpoints maintained by root
-        pub BootstrapEndpoints get(bootstrap_endpoints): Vec<Url>;
-        /// Mapping of service providers' AccountIds to their AccountInfo
-        pub AccountInfoByAccountId get(account_info_by_account_id): map T::AccountId => AccountInfo<T::BlockNumber>;
-        /// Lifetime of an AccountInfo record in AccountInfoByAccountId map
-        pub DefaultLifetime get(default_lifetime) config(): T::BlockNumber = T::BlockNumber::from(DEFAULT_LIFETIME);
-    }
-}
-
-decl_event! {
-    pub enum Event<T> where <T as system::Trait>::AccountId {
-        AccountInfoUpdated(AccountId, IPNSIdentity),
-        AccountInfoRemoved(AccountId),
-    }
-}
-
-impl<T: Trait> Module<T> {
-    pub fn remove_account_info(accountid: &T::AccountId) {
-        if <AccountInfoByAccountId<T>>::exists(accountid) {
-            <AccountInfoByAccountId<T>>::remove(accountid);
-            Self::deposit_event(RawEvent::AccountInfoRemoved(accountid.clone()));
-        }
-    }
-
-    pub fn is_account_info_expired(accountid: &T::AccountId) -> bool {
-        !<AccountInfoByAccountId<T>>::exists(accountid)
-            || <system::Module<T>>::block_number()
-                > <AccountInfoByAccountId<T>>::get(accountid).expires_at
-    }
-}
-
-decl_module! {
-    pub struct Module<T: Trait> for enum Call where origin: T::Origin {
-        fn deposit_event() = default;
-
-        pub fn set_ipns_id(origin, id: Vec<u8>, lifetime: Option<T::BlockNumber>) {
-            let sender = ensure_signed(origin)?;
-            ensure!(T::Roles::is_role_account(&sender), "only role accounts can set ipns id");
-
-            // TODO: ensure id is a valid base58 encoded IPNS identity
-
-            let ttl = match lifetime {
-                Some(value) => if value >= T::BlockNumber::from(MINIMUM_LIFETIME) {
-                    value
-                } else {
-                    T::BlockNumber::from(MINIMUM_LIFETIME)
-                },
-                _ => Self::default_lifetime()
-            };
-
-            <AccountInfoByAccountId<T>>::insert(&sender, AccountInfo {
-                identity: id.clone(),
-                expires_at: <system::Module<T>>::block_number() + ttl,
-            });
-
-            Self::deposit_event(RawEvent::AccountInfoUpdated(sender, id));
-        }
-
-        pub fn unset_ipns_id(origin) {
-            let sender = ensure_signed(origin)?;
-            Self::remove_account_info(&sender);
-        }
-
-        // privileged methods
-
-        pub fn set_default_lifetime(origin, lifetime: T::BlockNumber) {
-            // although not strictly required to have an origin parameter and ensure_root
-            // decl_module! macro takes care of it.. its required for unit tests to work correctly
-            // otherwise it complains the method
-            ensure_root(origin)?;
-            ensure!(lifetime >= T::BlockNumber::from(MINIMUM_LIFETIME), "discovery: default lifetime must be gte minimum lifetime");
-            <DefaultLifetime<T>>::put(lifetime);
-        }
-
-        pub fn set_bootstrap_endpoints(origin, endpoints: Vec<Vec<u8>>) {
-            ensure_root(origin)?;
-            BootstrapEndpoints::put(endpoints);
-        }
-    }
-}

runtime-modules/service-discovery/src/lib.rs

@@ -1,7 +1,192 @@
+//! # Service discovery module
+//! Service discovery module for the Joystream platform supports the storage providers.
+//! It registers their 'pings' in the system with the expiration time, and stores the bootstrap
+//! nodes for the Colossus.
+//!
+//! ## Comments
+//!
+//! Service discovery module uses bureaucracy module to authorize actions. It is generally used by
+//! the Colossus service.
+//!
+//! ## Supported extrinsics
+//!
+//! - [set_ipns_id](./struct.Module.html#method.set_ipns_id) - Creates the AccountInfo to save an IPNS identity for the storage provider.
+//! - [unset_ipns_id](./struct.Module.html#method.unset_ipns_id) - Deletes the AccountInfo with the IPNS identity for the storage provider.
+//! - [set_default_lifetime](./struct.Module.html#method.set_default_lifetime) - Sets default lifetime for storage providers accounts info.
+//! - [set_bootstrap_endpoints](./struct.Module.html#method.set_bootstrap_endpoints) - Sets bootstrap endpoints for the Colossus.
+//!
+
 // Ensure we're `no_std` when compiling for Wasm.
 #![cfg_attr(not(feature = "std"), no_std)]
 
-pub mod discovery;
-
 mod mock;
 mod tests;
+
+use codec::{Decode, Encode};
+use rstd::prelude::*;
+#[cfg(feature = "std")]
+use serde::{Deserialize, Serialize};
+
+use srml_support::{decl_event, decl_module, decl_storage, ensure};
+use system::{self, ensure_root};
+/*
+  Although there is support for ed25519 keys as the IPNS identity key and we could potentially
+  reuse the same key for the role account and ipns (and make this discovery module obselete)
+  it is probably better to separate concerns.
+  Why not to use a fixed size 32byte -> SHA256 hash of public key: because we would have to force
+  specific key type on ipfs side.
+  pub struct IPNSIdentity(pub [u8; 32]); // we loose the key type!
+  pub type IPNSIdentity(pub u8, pub [u8; 32]); // we could add the keytype?
+  can we use rust library in wasm runtime?
+  https://github.com/multiformats/rust-multihash
+  https://github.com/multiformats/multicodec/
+  https://github.com/multiformats/multihash/
+*/
+/// base58 encoded IPNS identity multihash codec
+pub type IPNSIdentity = Vec<u8>;
+
+/// HTTP Url string to a discovery service endpoint
+pub type Url = Vec<u8>;
+
+// Alias for storage working group bureaucracy
+pub(crate) type StorageBureaucracy<T> = bureaucracy::Module<T, bureaucracy::Instance2>;
+
+/// Storage provider is a worker from the bureaucracy module.
+pub type StorageProviderId<T> = bureaucracy::WorkerId<T>;
+
+pub(crate) const MINIMUM_LIFETIME: u32 = 600; // 1hr assuming 6s block times
+pub(crate) const DEFAULT_LIFETIME: u32 = MINIMUM_LIFETIME * 24; // 24hr
+
+/// Defines the expiration date for the storage provider.
+#[cfg_attr(feature = "std", derive(Serialize, Deserialize, Debug))]
+#[derive(Encode, Decode, Default, Clone, PartialEq, Eq)]
+pub struct AccountInfo<BlockNumber> {
+    /// IPNS Identity.
+    pub identity: IPNSIdentity,
+    /// Block at which information expires.
+    pub expires_at: BlockNumber,
+}
+
+/// The _Service discovery_ main _Trait_.
+pub trait Trait: system::Trait + bureaucracy::Trait<bureaucracy::Instance2> {
+    /// _Service discovery_ event type.
+    type Event: From<Event<Self>> + Into<<Self as system::Trait>::Event>;
+}
+
+decl_storage! {
+    trait Store for Module<T: Trait> as Discovery {
+        /// Bootstrap endpoints maintained by root
+        pub BootstrapEndpoints get(bootstrap_endpoints): Vec<Url>;
+
+        /// Mapping of service providers' storage provider id to their AccountInfo
+        pub AccountInfoByStorageProviderId get(account_info_by_storage_provider_id):
+            map StorageProviderId<T> => AccountInfo<T::BlockNumber>;
+
+        /// Lifetime of an AccountInfo record in AccountInfoByAccountId map
+        pub DefaultLifetime get(default_lifetime) config():
+            T::BlockNumber = T::BlockNumber::from(DEFAULT_LIFETIME);
+    }
+}
+
+decl_event! {
+    /// _Service discovery_ events
+    pub enum Event<T> where
+        StorageProviderId = StorageProviderId<T>
+       {
+        /// Emits on updating of the account info.
+        /// Params:
+        /// - Id of the storage provider.
+        /// - Id of the IPNS.
+        AccountInfoUpdated(StorageProviderId, IPNSIdentity),
+
+        /// Emits on removing of the account info.
+        /// Params:
+        /// - Id of the storage provider.
+        AccountInfoRemoved(StorageProviderId),
+    }
+}
+
+decl_module! {
+    /// _Service discovery_ substrate module.
+    pub struct Module<T: Trait> for enum Call where origin: T::Origin {
+        /// Default deposit_event() handler
+        fn deposit_event() = default;
+
+        /// Creates the AccountInfo to save an IPNS identity for the storage provider.
+        /// Requires signed storage provider credentials.
+        pub fn set_ipns_id(
+            origin,
+            storage_provider_id: StorageProviderId<T>,
+            id: Vec<u8>,
+            lifetime: Option<T::BlockNumber>
+        ) {
+            <StorageBureaucracy<T>>::ensure_worker_signed(origin, &storage_provider_id)?;
+
+            // TODO: ensure id is a valid base58 encoded IPNS identity
+
+            let ttl = lifetime.map_or(Self::default_lifetime(), |value| {
+                let min_lifetime = T::BlockNumber::from(MINIMUM_LIFETIME);
+                if value >= min_lifetime {
+                    value
+                } else {
+                    min_lifetime
+                }
+            });
+
+            //
+            // == MUTATION SAFE ==
+            //
+
+            <AccountInfoByStorageProviderId<T>>::insert(storage_provider_id, AccountInfo {
+                identity: id.clone(),
+                expires_at: <system::Module<T>>::block_number() + ttl,
+            });
+
+            Self::deposit_event(RawEvent::AccountInfoUpdated(storage_provider_id, id));
+        }
+
+        /// Deletes the AccountInfo with the IPNS identity for the storage provider.
+        /// Requires signed storage provider credentials.
+        pub fn unset_ipns_id(origin, storage_provider_id: StorageProviderId<T>) {
+            <StorageBureaucracy<T>>::ensure_worker_signed(origin, &storage_provider_id)?;
+
+            // == MUTATION SAFE ==
+
+            if <AccountInfoByStorageProviderId<T>>::exists(storage_provider_id) {
+                <AccountInfoByStorageProviderId<T>>::remove(storage_provider_id);
+                Self::deposit_event(RawEvent::AccountInfoRemoved(storage_provider_id));
+            }
+        }
+
+        // Privileged methods
+
+        /// Sets default lifetime for storage providers accounts info. Requires root privileges.
+        pub fn set_default_lifetime(origin, lifetime: T::BlockNumber) {
+            ensure_root(origin)?;
+            ensure!(lifetime >= T::BlockNumber::from(MINIMUM_LIFETIME),
+                "discovery: default lifetime must be gte minimum lifetime");
+
+            // == MUTATION SAFE ==
+
+            <DefaultLifetime<T>>::put(lifetime);
+        }
+
+        /// Sets bootstrap endpoints for the Colossus. Requires root privileges.
+        pub fn set_bootstrap_endpoints(origin, endpoints: Vec<Vec<u8>>) {
+            ensure_root(origin)?;
+
+            // == MUTATION SAFE ==
+
+            BootstrapEndpoints::put(endpoints);
+        }
+    }
+}
+
+impl<T: Trait> Module<T> {
+    /// Verifies that account info for the storage provider is still valid.
+    pub fn is_account_info_expired(storage_provider_id: &StorageProviderId<T>) -> bool {
+        !<AccountInfoByStorageProviderId<T>>::exists(storage_provider_id)
+            || <system::Module<T>>::block_number()
+                > <AccountInfoByStorageProviderId<T>>::get(storage_provider_id).expires_at
+    }
+}

runtime-modules/service-discovery/src/mock.rs

@@ -1,8 +1,6 @@
 #![cfg(test)]
 
-pub use super::discovery;
-pub use roles::actors;
-use roles::traits::Roles;
+pub use crate::*;
 
 pub use primitives::{Blake2Hasher, H256};
 pub use sr_primitives::{
@@ -13,6 +11,20 @@ pub use sr_primitives::{
 
 use srml_support::{impl_outer_event, impl_outer_origin, parameter_types};
 
+mod bureaucracy_mod {
+    pub use bureaucracy::Event;
+    pub use bureaucracy::Instance2;
+    pub use bureaucracy::Trait;
+}
+
+mod membership_mod {
+    pub use membership::members::Event;
+}
+
+mod discovery {
+    pub use crate::Event;
+}
+
 impl_outer_origin! {
     pub enum Origin for Test {}
 }
@@ -20,6 +32,9 @@ impl_outer_origin! {
 impl_outer_event! {
     pub enum MetaEvent for Test {
         discovery<T>,
+        balances<T>,
+        membership_mod<T>,
+        bureaucracy_mod Instance2 <T>,
     }
 }
 
@@ -31,6 +46,12 @@ parameter_types! {
     pub const MaximumBlockWeight: u32 = 1024;
     pub const MaximumBlockLength: u32 = 2 * 1024;
     pub const AvailableBlockRatio: Perbill = Perbill::one();
+    pub const MinimumPeriod: u64 = 5;
+    pub const InitialMembersBalance: u64 = 2000;
+    pub const StakePoolId: [u8; 8] = *b"joystake";
+    pub const ExistentialDeposit: u32 = 0;
+    pub const TransferFee: u32 = 0;
+    pub const CreationFee: u32 = 0;
 }
 
 impl system::Trait for Test {
@@ -51,31 +72,69 @@ impl system::Trait for Test {
     type Version = ();
 }
 
-pub fn alice_account() -> u64 {
-    1
+impl Trait for Test {
+    type Event = MetaEvent;
+}
+
+impl hiring::Trait for Test {
+    type OpeningId = u64;
+    type ApplicationId = u64;
+    type ApplicationDeactivatedHandler = ();
+    type StakeHandlerProvider = hiring::Module<Self>;
 }
-pub fn bob_account() -> u64 {
-    2
+
+impl minting::Trait for Test {
+    type Currency = Balances;
+    type MintId = u64;
 }
 
-impl discovery::Trait for Test {
+impl stake::Trait for Test {
+    type Currency = Balances;
+    type StakePoolId = StakePoolId;
+    type StakingEventsHandler = ();
+    type StakeId = u64;
+    type SlashId = u64;
+}
+
+impl membership::members::Trait for Test {
     type Event = MetaEvent;
-    type Roles = MockRoles;
+    type MemberId = u64;
+    type PaidTermId = u64;
+    type SubscriptionId = u64;
+    type ActorId = u64;
+    type InitialMembersBalance = InitialMembersBalance;
 }
 
-pub struct MockRoles {}
-impl Roles<Test> for MockRoles {
-    fn is_role_account(account_id: &u64) -> bool {
-        *account_id == alice_account()
-    }
+impl common::currency::GovernanceCurrency for Test {
+    type Currency = Balances;
+}
 
-    fn account_has_role(_account_id: &u64, _role: actors::Role) -> bool {
-        false
-    }
+impl balances::Trait for Test {
+    type Balance = u64;
+    type OnFreeBalanceZero = ();
+    type OnNewAccount = ();
+    type TransferPayment = ();
+    type DustRemoval = ();
+    type Event = MetaEvent;
+    type ExistentialDeposit = ExistentialDeposit;
+    type TransferFee = TransferFee;
+    type CreationFee = CreationFee;
+}
 
-    fn random_account_for_role(_role: actors::Role) -> Result<u64, &'static str> {
-        Err("not implemented")
-    }
+impl recurringrewards::Trait for Test {
+    type PayoutStatusHandler = ();
+    type RecipientId = u64;
+    type RewardRelationshipId = u64;
+}
+
+impl bureaucracy::Trait<bureaucracy::Instance2> for Test {
+    type Event = MetaEvent;
+}
+
+impl timestamp::Trait for Test {
+    type Moment = u64;
+    type OnTimestampSet = ();
+    type MinimumPeriod = MinimumPeriod;
 }
 
 pub fn initial_test_ext() -> runtime_io::TestExternalities {
@@ -86,5 +145,25 @@ pub fn initial_test_ext() -> runtime_io::TestExternalities {
     t.into()
 }
 
+pub type Balances = balances::Module<Test>;
 pub type System = system::Module<Test>;
-pub type Discovery = discovery::Module<Test>;
+pub type Discovery = Module<Test>;
+
+pub(crate) fn hire_storage_provider() -> (u64, u64) {
+    let storage_provider_id = 1;
+    let role_account_id = 1;
+
+    let storage_provider = bureaucracy::Worker {
+        member_id: 1,
+        role_account: role_account_id,
+        reward_relationship: None,
+        role_stake_profile: None,
+    };
+
+    <bureaucracy::WorkerById<Test, bureaucracy::Instance2>>::insert(
+        storage_provider_id,
+        storage_provider,
+    );
+
+    (role_account_id, storage_provider_id)
+}

runtime-modules/service-discovery/src/tests.rs

@@ -11,16 +11,25 @@ fn set_ipns_id() {
         let current_block_number = 1000;
         System::set_block_number(current_block_number);
 
-        let alice = alice_account();
-        let identity = "alice".as_bytes().to_vec();
-        let ttl = <Test as system::Trait>::BlockNumber::from(discovery::MINIMUM_LIFETIME + 100);
-        assert!(Discovery::set_ipns_id(Origin::signed(alice), identity.clone(), Some(ttl)).is_ok());
+        let (storage_provider_account_id, storage_provider_id) = hire_storage_provider();
 
-        assert!(<discovery::AccountInfoByAccountId<Test>>::exists(&alice));
-        let account_info = Discovery::account_info_by_account_id(&alice);
+        let identity = "alice".as_bytes().to_vec();
+        let ttl = <Test as system::Trait>::BlockNumber::from(MINIMUM_LIFETIME + 100);
+        assert!(Discovery::set_ipns_id(
+            Origin::signed(storage_provider_account_id),
+            storage_provider_id,
+            identity.clone(),
+            Some(ttl)
+        )
+        .is_ok());
+
+        assert!(<AccountInfoByStorageProviderId<Test>>::exists(
+            &storage_provider_id
+        ));
+        let account_info = Discovery::account_info_by_storage_provider_id(&storage_provider_id);
         assert_eq!(
             account_info,
-            discovery::AccountInfo {
+            AccountInfo {
                 identity: identity.clone(),
                 expires_at: current_block_number + ttl
             }
@@ -30,78 +39,106 @@ fn set_ipns_id() {
             *System::events().last().unwrap(),
             EventRecord {
                 phase: Phase::ApplyExtrinsic(0),
-                event: MetaEvent::discovery(discovery::RawEvent::AccountInfoUpdated(
-                    alice,
+                event: MetaEvent::discovery(RawEvent::AccountInfoUpdated(
+                    storage_provider_id,
                     identity.clone()
                 )),
                 topics: vec![]
             }
         );
 
-        // Non role account trying to set account into should fail
-        let bob = bob_account();
-        assert!(Discovery::set_ipns_id(Origin::signed(bob), identity.clone(), None).is_err());
-        assert!(!<discovery::AccountInfoByAccountId<Test>>::exists(&bob));
+        // Invalid storage provider data
+        let invalid_storage_provider_id = 2;
+        let invalid_storage_provider_account_id = 2;
+        assert!(Discovery::set_ipns_id(
+            Origin::signed(invalid_storage_provider_id),
+            invalid_storage_provider_account_id,
+            identity.clone(),
+            None
+        )
+        .is_err());
+        assert!(!<AccountInfoByStorageProviderId<Test>>::exists(
+            &invalid_storage_provider_id
+        ));
     });
 }
 
 #[test]
 fn unset_ipns_id() {
     initial_test_ext().execute_with(|| {
-        let alice = alice_account();
+        let (storage_provider_account_id, storage_provider_id) = hire_storage_provider();
 
-        <discovery::AccountInfoByAccountId<Test>>::insert(
-            &alice,
-            discovery::AccountInfo {
+        <AccountInfoByStorageProviderId<Test>>::insert(
+            &storage_provider_id,
+            AccountInfo {
                 expires_at: 1000,
                 identity: "alice".as_bytes().to_vec(),
             },
         );
 
-        assert!(<discovery::AccountInfoByAccountId<Test>>::exists(&alice));
+        assert!(<AccountInfoByStorageProviderId<Test>>::exists(
+            &storage_provider_account_id
+        ));
 
-        assert!(Discovery::unset_ipns_id(Origin::signed(alice)).is_ok());
-        assert!(!<discovery::AccountInfoByAccountId<Test>>::exists(&alice));
+        assert!(Discovery::unset_ipns_id(
+            Origin::signed(storage_provider_account_id),
+            storage_provider_id
+        )
+        .is_ok());
+        assert!(!<AccountInfoByStorageProviderId<Test>>::exists(
+            &storage_provider_account_id
+        ));
 
         assert_eq!(
             *System::events().last().unwrap(),
             EventRecord {
                 phase: Phase::ApplyExtrinsic(0),
-                event: MetaEvent::discovery(discovery::RawEvent::AccountInfoRemoved(alice)),
+                event: MetaEvent::discovery(RawEvent::AccountInfoRemoved(storage_provider_id)),
                 topics: vec![]
             }
         );
+
+        // Invalid storage provider data
+        let invalid_storage_provider_id = 2;
+        let invalid_storage_provider_account_id = 2;
+        assert!(Discovery::unset_ipns_id(
+            Origin::signed(invalid_storage_provider_id),
+            invalid_storage_provider_account_id,
+        )
+        .is_err());
+        assert!(!<AccountInfoByStorageProviderId<Test>>::exists(
+            &invalid_storage_provider_id
+        ));
     });
 }
 
 #[test]
 fn is_account_info_expired() {
     initial_test_ext().execute_with(|| {
-        let alice = alice_account();
+        let storage_provider_id = 1;
         let expires_at = 1000;
         let id = "alice".as_bytes().to_vec();
-        <discovery::AccountInfoByAccountId<Test>>::insert(
-            &alice,
-            discovery::AccountInfo {
+        <AccountInfoByStorageProviderId<Test>>::insert(
+            &storage_provider_id,
+            AccountInfo {
                 expires_at,
                 identity: id.clone(),
             },
         );
 
         System::set_block_number(expires_at - 10);
-        assert!(!Discovery::is_account_info_expired(&alice));
+        assert!(!Discovery::is_account_info_expired(&storage_provider_id));
 
         System::set_block_number(expires_at + 10);
-        assert!(Discovery::is_account_info_expired(&alice));
+        assert!(Discovery::is_account_info_expired(&storage_provider_id));
     });
 }
 
 #[test]
 fn set_default_lifetime() {
     initial_test_ext().execute_with(|| {
-        let lifetime =
-            <Test as system::Trait>::BlockNumber::from(discovery::MINIMUM_LIFETIME + 2000);
-        // priviliged method should fail if not from root origin
+        let lifetime = <Test as system::Trait>::BlockNumber::from(MINIMUM_LIFETIME + 2000);
+        // privileged method should fail if not from root origin
         assert!(
             Discovery::set_default_lifetime(Origin::signed(1), lifetime).is_err(),
             ""
@@ -113,10 +150,10 @@ fn set_default_lifetime() {
         assert_eq!(Discovery::default_lifetime(), lifetime, "");
 
         // cannot set default lifetime to less than minimum
-        let less_than_min_liftime =
-            <Test as system::Trait>::BlockNumber::from(discovery::MINIMUM_LIFETIME - 1);
+        let less_than_min_lifetime =
+            <Test as system::Trait>::BlockNumber::from(MINIMUM_LIFETIME - 1);
         assert!(
-            Discovery::set_default_lifetime(Origin::ROOT, less_than_min_liftime).is_err(),
+            Discovery::set_default_lifetime(Origin::ROOT, less_than_min_lifetime).is_err(),
             ""
         );
     });
@@ -126,7 +163,7 @@ fn set_default_lifetime() {
 fn set_bootstrap_endpoints() {
     initial_test_ext().execute_with(|| {
         let endpoints = vec!["endpoint1".as_bytes().to_vec()];
-        // priviliged method should fail if not from root origin
+        // privileged method should fail if not from root origin
         assert!(
             Discovery::set_bootstrap_endpoints(Origin::signed(1), endpoints.clone()).is_err(),
             ""

runtime-modules/storage/Cargo.toml

@@ -17,7 +17,6 @@ std = [
 	'primitives/std',
 	'common/std',
 	'membership/std',
-	'roles/std',
 	'bureaucracy/std',
 ]
 
@@ -79,11 +78,6 @@ default_features = false
 package = 'substrate-common-module'
 path = '../common'
 
-[dependencies.roles]
-default_features = false
-package = 'substrate-roles-module'
-path = '../roles'
-
 [dependencies.bureaucracy]
 default_features = false
 package = 'substrate-bureaucracy-module'

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

@@ -3,7 +3,6 @@
 pub use crate::{data_directory, data_object_storage_registry, data_object_type_registry};
 pub use common::currency::GovernanceCurrency;
 use membership::members;
-use roles::actors;
 pub use system;
 
 pub use primitives::{Blake2Hasher, H256};
@@ -32,7 +31,6 @@ impl_outer_event! {
         data_object_type_registry<T>,
         data_directory<T>,
         data_object_storage_registry<T>,
-        actors<T>,
         balances<T>,
         members<T>,
         bureaucracy_mod Instance2 <T>,
@@ -219,15 +217,6 @@ impl hiring::Trait for Test {
     type StakeHandlerProvider = hiring::Module<Self>;
 }
 
-impl actors::Trait for Test {
-    type Event = MetaEvent;
-    type OnActorRemoved = ();
-}
-
-impl actors::ActorRemoved<Test> for () {
-    fn actor_removed(_: &u64) {}
-}
-
 pub struct ExtBuilder {
     first_data_object_type_id: u64,
     first_content_id: u64,
@@ -297,7 +286,6 @@ pub type TestDataObjectTypeRegistry = data_object_type_registry::Module<Test>;
 pub type TestDataObjectType = data_object_type_registry::DataObjectType;
 pub type TestDataDirectory = data_directory::Module<Test>;
 pub type TestDataObjectStorageRegistry = data_object_storage_registry::Module<Test>;
-pub type TestActors = actors::Module<Test>;
 
 pub fn with_default_mock_builder<R, F: FnOnce() -> R>(f: F) -> R {
     ExtBuilder::default()
@@ -306,15 +294,7 @@ pub fn with_default_mock_builder<R, F: FnOnce() -> R>(f: F) -> R {
         .first_relationship_id(TEST_FIRST_RELATIONSHIP_ID)
         .first_metadata_id(TEST_FIRST_METADATA_ID)
         .build()
-        .execute_with(|| {
-            let roles: Vec<actors::Role> = vec![actors::Role::StorageProvider];
-            assert!(
-                TestActors::set_available_roles(system::RawOrigin::Root.into(), roles).is_ok(),
-                ""
-            );
-
-            f()
-        })
+        .execute_with(|| f())
 }
 
 pub(crate) fn hire_storage_provider() -> (u64, u32) {

runtime/src/integration/storage.rs

@@ -1,5 +1,6 @@
 use crate::{ActorId, Runtime};
 
+//TODO : SWG - implement
 pub struct StorageProviderHelper;
 
 impl storage::data_directory::StorageProviderHelper<Runtime> for StorageProviderHelper {

runtime/src/lib.rs

@@ -435,7 +435,6 @@ pub use versioned_store;
 pub use content_working_group as content_wg;
 mod migration;
 use roles::actors;
-use service_discovery::discovery;
 
 /// Alias for ContentId, used in various places.
 pub type ContentId = primitives::H256;
@@ -734,7 +733,7 @@ impl roles::traits::Roles<Runtime> for LookupRoles {
 
         let live_ids: Vec<<Runtime as system::Trait>::AccountId> = ids
             .into_iter()
-            .filter(|id| !<discovery::Module<Runtime>>::is_account_info_expired(id))
+            //            .filter(|id| !<service_discovery::Module<Runtime>>::is_account_info_expired(id)) //TODO : SWG  restore
             .collect();
 
         if live_ids.is_empty() {
@@ -809,19 +808,16 @@ impl bureaucracy::Trait<bureaucracy::Instance2> for Runtime {
 
 impl actors::Trait for Runtime {
     type Event = Event;
-    type OnActorRemoved = HandleActorRemoved;
+    type OnActorRemoved = ();
 }
 
-pub struct HandleActorRemoved {}
-impl actors::ActorRemoved<Runtime> for HandleActorRemoved {
-    fn actor_removed(actor: &<Runtime as system::Trait>::AccountId) {
-        Discovery::remove_account_info(actor);
-    }
+//TODO: SWG -  remove with roles module deletion
+impl actors::ActorRemoved<Runtime> for () {
+    fn actor_removed(_: &AccountId) {}
 }
 
-impl discovery::Trait for Runtime {
+impl service_discovery::Trait for Runtime {
     type Event = Event;
-    type Roles = LookupRoles;
 }
 
 parameter_types! {
@@ -913,10 +909,6 @@ construct_runtime!(
         Members: members::{Module, Call, Storage, Event<T>, Config<T>},
         Forum: forum::{Module, Call, Storage, Event<T>, Config<T>},
         Actors: actors::{Module, Call, Storage, Event<T>, Config},
-        DataObjectTypeRegistry: data_object_type_registry::{Module, Call, Storage, Event<T>, Config<T>},
-        DataDirectory: data_directory::{Module, Call, Storage, Event<T>},
-        DataObjectStorageRegistry: data_object_storage_registry::{Module, Call, Storage, Event<T>, Config<T>},
-        Discovery: discovery::{Module, Call, Storage, Event<T>},
         VersionedStore: versioned_store::{Module, Call, Storage, Event<T>, Config},
         VersionedStorePermissions: versioned_store_permissions::{Module, Call, Storage},
         Stake: stake::{Module, Call, Storage},
@@ -924,6 +916,11 @@ construct_runtime!(
         RecurringRewards: recurringrewards::{Module, Call, Storage},
         Hiring: hiring::{Module, Call, Storage},
         ContentWorkingGroup: content_wg::{Module, Call, Storage, Event<T>, Config<T>},
+        // --- Storage
+        DataObjectTypeRegistry: data_object_type_registry::{Module, Call, Storage, Event<T>, Config<T>},
+        DataDirectory: data_directory::{Module, Call, Storage, Event<T>},
+        DataObjectStorageRegistry: data_object_storage_registry::{Module, Call, Storage, Event<T>, Config<T>},
+        Discovery: service_discovery::{Module, Call, Storage, Event<T>},
         // --- Proposals
         ProposalsEngine: proposals_engine::{Module, Call, Storage, Event<T>},
         ProposalsDiscussion: proposals_discussion::{Module, Call, Storage, Event<T>},