Change proposals actor validation

- move memberhip validation to the codex proposal creation extrinsics
- remove actor validation in the create_proposal (engine) and create_thread(discussion) API methods.

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

@@ -1191,7 +1191,7 @@ decl_module! {
             // Increment NextChannelId
             NextChannelId::<T>::mutate(|id| *id += <ChannelId<T> as One>::one());
 
-            /// CREDENTIAL STUFF ///
+            // CREDENTIAL STUFF //
 
             // Dial out to membership module and inform about new role as channe owner.
             let registered_role = <members::Module<T>>::register_role_on_member(owner, &member_in_role).is_ok();

runtime-modules/proposals/codex/Cargo.toml

@@ -101,17 +101,17 @@ default_features = false
 package = 'substrate-proposals-discussion-module'
 path = '../discussion'
 
+[dependencies.common]
+default_features = false
+package = 'substrate-common-module'
+path = '../../common'
+
 [dev-dependencies.runtime-io]
 default_features = false
 git = 'https://github.com/paritytech/substrate.git'
 package = 'sr-io'
 rev = 'c37bb08535c49a12320af7facfd555ce05cce2e8'
 
-[dev-dependencies.common]
-default_features = false
-package = 'substrate-common-module'
-path = '../../common'
-
 [dev-dependencies.governance]
 default_features = false
 package = 'substrate-governance-module'

runtime-modules/proposals/codex/src/lib.rs

@@ -23,6 +23,7 @@ use rstd::vec::Vec;
 use srml_support::{decl_error, decl_module, decl_storage, ensure, print};
 use system::{ensure_root, RawOrigin};
 
+use common::origin_validator::ActorOriginValidator;
 use proposal_engine::ProposalParameters;
 
 /// 'Proposals codex' substrate module Trait
@@ -34,6 +35,13 @@ pub trait Trait:
 
     /// Defines max wasm code length of the runtime upgrade proposal.
     type RuntimeUpgradeWasmProposalMaxLength: Get<u32>;
+
+    /// Validates member id and origin combination
+    type MembershipOriginValidator: ActorOriginValidator<
+        Self::Origin,
+        MemberId<Self>,
+        Self::AccountId,
+    >;
 }
 use srml_support::traits::{Currency, Get};
 
@@ -123,6 +131,8 @@ decl_module! {
             text: Vec<u8>,
             stake_balance: Option<BalanceOf<T>>,
         ) {
+            let account_id = T::MembershipOriginValidator::ensure_actor_origin(origin, member_id.clone())?;
+
             let parameters = proposal_types::parameters::text_proposal::<T>();
 
             ensure!(!text.is_empty(), Error::TextProposalIsEmpty);
@@ -143,16 +153,13 @@ decl_module! {
 
             let proposal_code = <Call<T>>::text_proposal(title.clone(), description.clone(), text);
 
-            let (cloned_origin1, cloned_origin2) =  Self::double_origin(origin);
-
             let discussion_thread_id = <proposal_discussion::Module<T>>::create_thread(
-                cloned_origin1,
                 member_id,
                 title.clone(),
             )?;
 
             let proposal_id = <proposal_engine::Module<T>>::create_proposal(
-                cloned_origin2,
+                account_id,
                 member_id,
                 parameters,
                 title,
@@ -173,6 +180,8 @@ decl_module! {
             wasm: Vec<u8>,
             stake_balance: Option<BalanceOf<T>>,
         ) {
+            let account_id = T::MembershipOriginValidator::ensure_actor_origin(origin, member_id.clone())?;
+
             let parameters = proposal_types::parameters::upgrade_runtime::<T>();
 
             ensure!(!wasm.is_empty(), Error::RuntimeProposalIsEmpty);
@@ -193,16 +202,13 @@ decl_module! {
 
             let proposal_code = <Call<T>>::text_proposal(title.clone(), description.clone(), wasm);
 
-            let (cloned_origin1, cloned_origin2) =  Self::double_origin(origin);
-
             let discussion_thread_id = <proposal_discussion::Module<T>>::create_thread(
-                cloned_origin1,
                 member_id,
                 title.clone(),
             )?;
 
             let proposal_id = <proposal_engine::Module<T>>::create_proposal(
-                cloned_origin2,
+                account_id,
                 member_id,
                 parameters,
                 title,

runtime-modules/proposals/codex/src/tests/mock.rs

@@ -129,7 +129,6 @@ parameter_types! {
 
 impl proposal_discussion::Trait for Test {
     type Event = ();
-    type ThreadAuthorOriginValidator = ();
     type PostAuthorOriginValidator = ();
     type ThreadId = u32;
     type PostId = u32;
@@ -154,6 +153,7 @@ parameter_types! {
 impl crate::Trait for Test {
     type TextProposalMaxLength = TextProposalMaxLength;
     type RuntimeUpgradeWasmProposalMaxLength = RuntimeUpgradeWasmProposalMaxLength;
+    type MembershipOriginValidator = ();
 }
 
 impl system::Trait for Test {

runtime-modules/proposals/discussion/src/lib.rs

@@ -56,13 +56,6 @@ pub trait Trait: system::Trait + membership::members::Trait {
     /// Engine event type.
     type Event: From<Event<Self>> + Into<<Self as system::Trait>::Event>;
 
-    /// Validates thread author id and origin combination
-    type ThreadAuthorOriginValidator: ActorOriginValidator<
-        Self::Origin,
-        MemberId<Self>,
-        Self::AccountId,
-    >;
-
     /// Validates post author id and origin combination
     type PostAuthorOriginValidator: ActorOriginValidator<
         Self::Origin,
@@ -259,12 +252,9 @@ impl<T: Trait> Module<T> {
     /// Create the discussion thread. Cannot add more threads than 'predefined limit = MaxThreadInARowNumber'
     /// times in a row by the same author.
     pub fn create_thread(
-        origin: T::Origin,
         thread_author_id: MemberId<T>,
         title: Vec<u8>,
     ) -> Result<T::ThreadId, Error> {
-        T::ThreadAuthorOriginValidator::ensure_actor_origin(origin, thread_author_id.clone())?;
-
         Self::ensure_can_create_thread(&title, thread_author_id.clone())?;
 
         let next_thread_count_value = Self::thread_count() + 1;

runtime-modules/proposals/discussion/src/tests/mock.rs

@@ -86,7 +86,6 @@ impl membership::members::Trait for Test {
 
 impl crate::Trait for Test {
     type Event = TestEvent;
-    type ThreadAuthorOriginValidator = ();
     type PostAuthorOriginValidator = ();
     type ThreadId = u32;
     type PostId = u32;

runtime-modules/proposals/discussion/src/tests/mod.rs

@@ -81,23 +81,9 @@ impl DiscussionFixture {
         DiscussionFixture { title, ..self }
     }
 
-    fn with_author(self, author_id: u64) -> Self {
-        DiscussionFixture { author_id, ..self }
-    }
-
-    fn with_origin(self, origin: RawOrigin<u64>) -> Self {
-        DiscussionFixture {
-            origin: origin.into(),
-            ..self
-        }
-    }
-
     fn create_discussion_and_assert(&self, result: Result<u32, Error>) -> Option<u32> {
-        let create_discussion_result = Discussions::create_thread(
-            self.origin.clone().into(),
-            self.author_id,
-            self.title.clone(),
-        );
+        let create_discussion_result =
+            Discussions::create_thread(self.author_id, self.title.clone());
 
         assert_eq!(create_discussion_result, result);
 
@@ -413,16 +399,3 @@ fn add_discussion_thread_fails_because_of_max_thread_by_same_author_in_a_row_lim
         discussion_fixture.create_discussion_and_assert(Err(Error::MaxThreadInARowLimitExceeded));
     });
 }
-
-#[test]
-fn add_discussion_thread_fails_because_of_invalid_author_origin() {
-    initial_test_ext().execute_with(|| {
-        let discussion_fixture = DiscussionFixture::default().with_author(2);
-        discussion_fixture.create_discussion_and_assert(Err(Error::Other("Invalid author")));
-
-        let discussion_fixture = DiscussionFixture::default()
-            .with_origin(RawOrigin::Signed(3))
-            .with_author(2);
-        discussion_fixture.create_discussion_and_assert(Err(Error::Other("Invalid author")));
-    });
-}

runtime-modules/proposals/engine/src/lib.rs

@@ -313,7 +313,7 @@ decl_module! {
 impl<T: Trait> Module<T> {
     /// Create proposal. Requires 'proposal origin' membership.
     pub fn create_proposal(
-        origin: T::Origin,
+        account_id: T::AccountId,
         proposer_id: MemberId<T>,
         parameters: ProposalParameters<T::BlockNumber, types::BalanceOf<T>>,
         title: Vec<u8>,
@@ -321,9 +321,6 @@ impl<T: Trait> Module<T> {
         stake_balance: Option<types::BalanceOf<T>>,
         encoded_dispatchable_call_code: Vec<u8>,
     ) -> Result<T::ProposalId, Error> {
-        let account_id =
-            T::ProposerOriginValidator::ensure_actor_origin(origin, proposer_id.clone())?;
-
         Self::ensure_create_proposal_parameters_are_valid(
             &parameters,
             &title,
@@ -377,6 +374,85 @@ impl<T: Trait> Module<T> {
 
         Ok(proposal_id)
     }
+
+    /// Performs all checks for the proposal creation:
+    /// - title, body lengths
+    /// - mac active proposal
+    /// - provided parameters: approval_threshold_percentage and slashing_threshold_percentage > 0
+    /// - provided stake balance and parameters.required_stake are valid
+    pub fn ensure_create_proposal_parameters_are_valid(
+        parameters: &ProposalParameters<T::BlockNumber, types::BalanceOf<T>>,
+        title: &[u8],
+        description: &[u8],
+        stake_balance: Option<types::BalanceOf<T>>,
+    ) -> DispatchResult<Error> {
+        ensure!(!title.is_empty(), Error::EmptyTitleProvided);
+        ensure!(
+            title.len() as u32 <= T::TitleMaxLength::get(),
+            Error::TitleIsTooLong
+        );
+
+        ensure!(!description.is_empty(), Error::EmptyDescriptionProvided);
+        ensure!(
+            description.len() as u32 <= T::DescriptionMaxLength::get(),
+            Error::DescriptionIsTooLong
+        );
+
+        ensure!(
+            (Self::active_proposal_count()) < T::MaxActiveProposalLimit::get(),
+            Error::MaxActiveProposalNumberExceeded
+        );
+
+        ensure!(
+            parameters.approval_threshold_percentage > 0,
+            Error::InvalidParameterApprovalThreshold
+        );
+
+        ensure!(
+            parameters.slashing_threshold_percentage > 0,
+            Error::InvalidParameterSlashingThreshold
+        );
+
+        // check stake parameters
+        if let Some(required_stake) = parameters.required_stake {
+            if let Some(staked_balance) = stake_balance {
+                ensure!(
+                    required_stake == staked_balance,
+                    Error::StakeDiffersFromRequired
+                );
+            } else {
+                return Err(Error::EmptyStake);
+            }
+        }
+
+        if stake_balance.is_some() && parameters.required_stake.is_none() {
+            return Err(Error::StakeShouldBeEmpty);
+        }
+
+        Ok(())
+    }
+
+    //TODO: candidate for invariant break or error saving to the state
+    /// Callback from StakingEventsHandler. Refunds unstaked imbalance back to the source account
+    pub fn refund_proposal_stake(stake_id: T::StakeId, imbalance: NegativeImbalance<T>) {
+        if <StakesProposals<T>>::exists(stake_id) {
+            //TODO: handle non existence
+
+            let proposal_id = Self::stakes_proposals(stake_id);
+
+            if <Proposals<T>>::exists(proposal_id) {
+                let proposal = Self::proposals(proposal_id);
+
+                if let Some(stake_data) = proposal.stake_data {
+                    //TODO: handle the result
+                    let _ = CurrencyOf::<T>::resolve_into_existing(
+                        &stake_data.source_account_id,
+                        imbalance,
+                    );
+                }
+            }
+        }
+    }
 }
 
 impl<T: Trait> Module<T> {
@@ -559,85 +635,6 @@ impl<T: Trait> Module<T> {
             ActiveProposalCount::put(next_active_proposal_count_value);
         };
     }
-
-    /// Performs all checks for the proposal creation:
-    /// - title, body lengths
-    /// - mac active proposal
-    /// - provided parameters: approval_threshold_percentage and slashing_threshold_percentage > 0
-    /// - provided stake balance and parameters.required_stake are valid
-    pub fn ensure_create_proposal_parameters_are_valid(
-        parameters: &ProposalParameters<T::BlockNumber, types::BalanceOf<T>>,
-        title: &[u8],
-        description: &[u8],
-        stake_balance: Option<types::BalanceOf<T>>,
-    ) -> DispatchResult<Error> {
-        ensure!(!title.is_empty(), Error::EmptyTitleProvided);
-        ensure!(
-            title.len() as u32 <= T::TitleMaxLength::get(),
-            Error::TitleIsTooLong
-        );
-
-        ensure!(!description.is_empty(), Error::EmptyDescriptionProvided);
-        ensure!(
-            description.len() as u32 <= T::DescriptionMaxLength::get(),
-            Error::DescriptionIsTooLong
-        );
-
-        ensure!(
-            (Self::active_proposal_count()) < T::MaxActiveProposalLimit::get(),
-            Error::MaxActiveProposalNumberExceeded
-        );
-
-        ensure!(
-            parameters.approval_threshold_percentage > 0,
-            Error::InvalidParameterApprovalThreshold
-        );
-
-        ensure!(
-            parameters.slashing_threshold_percentage > 0,
-            Error::InvalidParameterSlashingThreshold
-        );
-
-        // check stake parameters
-        if let Some(required_stake) = parameters.required_stake {
-            if let Some(staked_balance) = stake_balance {
-                ensure!(
-                    required_stake == staked_balance,
-                    Error::StakeDiffersFromRequired
-                );
-            } else {
-                return Err(Error::EmptyStake);
-            }
-        }
-
-        if stake_balance.is_some() && parameters.required_stake.is_none() {
-            return Err(Error::StakeShouldBeEmpty);
-        }
-
-        Ok(())
-    }
-
-    //TODO: candidate for invariant break or error saving to the state
-    /// Callback from StakingEventsHandler. Refunds unstaked imbalance back to the source account
-    pub fn refund_proposal_stake(stake_id: T::StakeId, imbalance: NegativeImbalance<T>) {
-        if <StakesProposals<T>>::exists(stake_id) {
-            //TODO: handle non existence
-
-            let proposal_id = Self::stakes_proposals(stake_id);
-
-            if <Proposals<T>>::exists(proposal_id) {
-                let proposal = Self::proposals(proposal_id);
-
-                if let Some(stake_data) = proposal.stake_data {
-                    //TODO: handle the result
-                    let _ = CurrencyOf::<T>::resolve_into_existing(
-                        &stake_data.source_account_id,
-                        imbalance,
-                    );
-                }
-            }
-        }
-    }
 }
 
 // Simplification of the 'FinalizedProposalData' type

runtime-modules/proposals/engine/src/tests/mod.rs

@@ -58,7 +58,7 @@ impl Default for ProposalParametersFixture {
 #[derive(Clone)]
 struct DummyProposalFixture {
     parameters: ProposalParameters<u64, u64>,
-    origin: RawOrigin<u64>,
+    account_id: u64,
     proposer_id: u64,
     proposal_code: Vec<u8>,
     title: Vec<u8>,
@@ -83,7 +83,7 @@ impl Default for DummyProposalFixture {
                 grace_period: 0,
                 required_stake: None,
             },
-            origin: RawOrigin::Signed(1),
+            account_id: 1,
             proposer_id: 1,
             proposal_code: dummy_proposal.encode(),
             title,
@@ -106,8 +106,8 @@ impl DummyProposalFixture {
         DummyProposalFixture { parameters, ..self }
     }
 
-    fn with_origin(self, origin: RawOrigin<u64>) -> Self {
-        DummyProposalFixture { origin, ..self }
+    fn with_account_id(self, account_id: u64) -> Self {
+        DummyProposalFixture { account_id, ..self }
     }
 
     fn with_stake(self, stake_balance: BalanceOf<Test>) -> Self {
@@ -126,7 +126,7 @@ impl DummyProposalFixture {
 
     fn create_proposal_and_assert(self, result: Result<u32, Error>) -> Option<u32> {
         let proposal_id_result = ProposalsEngine::create_proposal(
-            self.origin.into(),
+            self.account_id,
             self.proposer_id,
             self.parameters,
             self.title,
@@ -283,14 +283,6 @@ fn create_dummy_proposal_succeeds() {
     });
 }
 
-#[test]
-fn create_dummy_proposal_fails_with_insufficient_rights() {
-    initial_test_ext().execute_with(|| {
-        let dummy_proposal = DummyProposalFixture::default().with_origin(RawOrigin::None);
-        dummy_proposal.create_proposal_and_assert(Err(Error::Other("RequireSignedOrigin")));
-    });
-}
-
 #[test]
 fn vote_succeeds() {
     initial_test_ext().execute_with(|| {
@@ -948,7 +940,7 @@ fn create_dummy_proposal_succeeds_with_stake() {
 
         let dummy_proposal = DummyProposalFixture::default()
             .with_parameters(parameters_fixture.params())
-            .with_origin(RawOrigin::Signed(account_id))
+            .with_account_id(account_id)
             .with_stake(200);
 
         let _imbalance = <Test as stake::Trait>::Currency::deposit_creating(&account_id, 500);
@@ -986,7 +978,7 @@ fn create_dummy_proposal_fail_with_stake_on_empty_account() {
             ProposalParametersFixture::default().with_required_stake(required_stake);
         let dummy_proposal = DummyProposalFixture::default()
             .with_parameters(parameters_fixture.params())
-            .with_origin(RawOrigin::Signed(account_id))
+            .with_account_id(account_id)
             .with_stake(required_stake);
 
         dummy_proposal
@@ -1126,7 +1118,7 @@ fn finalize_proposal_using_stake_mocks_succeeds() {
                 ProposalParametersFixture::default().with_required_stake(stake_amount);
             let dummy_proposal = DummyProposalFixture::default()
                 .with_parameters(parameters_fixture.params())
-                .with_origin(RawOrigin::Signed(account_id))
+                .with_account_id(account_id)
                 .with_stake(stake_amount);
 
             let _proposal_id = dummy_proposal.create_proposal_and_assert(Ok(1)).unwrap();
@@ -1209,7 +1201,7 @@ fn finalize_proposal_using_stake_mocks_failed() {
                 ProposalParametersFixture::default().with_required_stake(stake_amount);
             let dummy_proposal = DummyProposalFixture::default()
                 .with_parameters(parameters_fixture.params())
-                .with_origin(RawOrigin::Signed(account_id))
+                .with_account_id(account_id)
                 .with_stake(stake_amount);
 
             let proposal_id = dummy_proposal.create_proposal_and_assert(Ok(1)).unwrap();

runtime/src/lib.rs

@@ -844,7 +844,6 @@ parameter_types! {
 
 impl proposals_discussion::Trait for Runtime {
     type Event = Event;
-    type ThreadAuthorOriginValidator = MembershipOriginValidator<Self>;
     type PostAuthorOriginValidator = MembershipOriginValidator<Self>;
     type ThreadId = u32;
     type PostId = u32;
@@ -860,6 +859,7 @@ parameter_types! {
 }
 
 impl proposals_codex::Trait for Runtime {
+    type MembershipOriginValidator = MembershipOriginValidator<Self>;
     type TextProposalMaxLength = TextProposalMaxLength;
     type RuntimeUpgradeWasmProposalMaxLength = RuntimeUpgradeWasmProposalMaxLength;
 }