Change ensure_actor_origin() signature

- change ensure_actor_origin() signature - remove error
- introduce built-in error messages

runtime-modules/common/src/origin_validator.rs

@@ -1,9 +1,5 @@
 /// Abstract validator for the origin(account_id) and actor_id (eg.: thread author id).
 pub trait ActorOriginValidator<Origin, ActorId, AccountId> {
     /// Check for valid combination of origin and actor_id
-    fn ensure_actor_origin(
-        origin: Origin,
-        actor_id: ActorId,
-        error: &'static str,
-    ) -> Result<AccountId, &'static str>;
+    fn ensure_actor_origin(origin: Origin, actor_id: ActorId) -> Result<AccountId, &'static str>;
 }

runtime-modules/membership/src/origin_validator.rs

@@ -20,7 +20,6 @@ impl<T: crate::members::Trait>
     fn ensure_actor_origin(
         origin: <T as system::Trait>::Origin,
         actor_id: MemberId<T>,
-        error: &'static str,
     ) -> Result<<T as system::Trait>::AccountId, &'static str> {
         // check valid signed account_id
         let account_id = ensure_signed(origin)?;
@@ -32,10 +31,12 @@ impl<T: crate::members::Trait>
             // whether the account_id belongs to the actor
             if profile.root_account == account_id || profile.controller_account == account_id {
                 return Ok(account_id);
+            } else {
+                return Err("Membership validation failed: given account doesn't match with profile accounts");
             }
         }
 
-        Err(error)
+        Err("Membership validation failed: cannot find a profile for a member")
     }
 }
 
@@ -58,13 +59,10 @@ mod tests {
         initial_test_ext().execute_with(|| {
             let origin = RawOrigin::Signed(1);
             let member_id = 1;
-            let error = "Error";
+            let error = "Membership validation failed: cannot find a profile for a member";
 
-            let validation_result = MembershipOriginValidator::<Test>::ensure_actor_origin(
-                origin.into(),
-                member_id,
-                error,
-            );
+            let validation_result =
+                MembershipOriginValidator::<Test>::ensure_actor_origin(origin.into(), member_id);
 
             assert_eq!(validation_result, Err(error));
         });
@@ -75,7 +73,6 @@ mod tests {
         initial_test_ext().execute_with(|| {
             let account_id = 1;
             let origin = RawOrigin::Signed(account_id);
-            let error = "Error";
             let authority_account_id = 10;
             Membership::set_screening_authority(RawOrigin::Root.into(), authority_account_id)
                 .unwrap();
@@ -92,11 +89,8 @@ mod tests {
             .unwrap();
             let member_id = 0; // newly created member_id
 
-            let validation_result = MembershipOriginValidator::<Test>::ensure_actor_origin(
-                origin.into(),
-                member_id,
-                error,
-            );
+            let validation_result =
+                MembershipOriginValidator::<Test>::ensure_actor_origin(origin.into(), member_id);
 
             assert_eq!(validation_result, Ok(account_id));
         });
@@ -106,7 +100,8 @@ mod tests {
     fn membership_origin_validator_fails_with_incompatible_account_id_and_member_id() {
         initial_test_ext().execute_with(|| {
             let account_id = 1;
-            let error = "Errorss";
+            let error =
+                "Membership validation failed: given account doesn't match with profile accounts";
             let authority_account_id = 10;
             Membership::set_screening_authority(RawOrigin::Root.into(), authority_account_id)
                 .unwrap();
@@ -127,7 +122,6 @@ mod tests {
             let validation_result = MembershipOriginValidator::<Test>::ensure_actor_origin(
                 RawOrigin::Signed(invalid_account_id).into(),
                 member_id,
-                error,
             );
 
             assert_eq!(validation_result, Err(error));

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

@@ -109,7 +109,7 @@ impl governance::council::Trait for Test {
 }
 
 impl common::origin_validator::ActorOriginValidator<Origin, u64, u64> for () {
-    fn ensure_actor_origin(_: Origin, _: u64, _: &'static str) -> Result<u64, &'static str> {
+    fn ensure_actor_origin(_: Origin, _: u64) -> Result<u64, &'static str> {
         Ok(1)
     }
 }

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

@@ -8,7 +8,3 @@ pub(crate) const MSG_EMPTY_POST_PROVIDED: &str = "Post cannot be empty";
 pub(crate) const MSG_TOO_LONG_POST: &str = "Post is too long";
 pub(crate) const MSG_MAX_THREAD_IN_A_ROW_LIMIT_EXCEEDED: &str =
     "Max number of threads by same author in a row limit exceeded";
-pub(crate) const MSG_INVALID_THREAD_AUTHOR_ORIGIN: &str =
-    "Invalid combination of the origin and thread_author_id";
-pub(crate) const MSG_INVALID_POST_AUTHOR_ORIGIN: &str =
-    "Invalid combination of the origin and post_author_id";

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

@@ -130,7 +130,6 @@ decl_module! {
             T::PostAuthorOriginValidator::ensure_actor_origin(
                 origin,
                 post_author_id.clone(),
-                errors::MSG_INVALID_POST_AUTHOR_ORIGIN
             )?;
             ensure!(<ThreadById<T>>::exists(thread_id), errors::MSG_THREAD_DOESNT_EXIST);
 
@@ -171,7 +170,6 @@ decl_module! {
             T::PostAuthorOriginValidator::ensure_actor_origin(
                 origin,
                 post_author_id.clone(),
-                errors::MSG_INVALID_POST_AUTHOR_ORIGIN
             )?;
 
             ensure!(<ThreadById<T>>::exists(thread_id), errors::MSG_THREAD_DOESNT_EXIST);
@@ -217,11 +215,7 @@ impl<T: Trait> Module<T> {
         thread_author_id: MemberId<T>,
         title: Vec<u8>,
     ) -> Result<T::ThreadId, &'static str> {
-        T::ThreadAuthorOriginValidator::ensure_actor_origin(
-            origin,
-            thread_author_id.clone(),
-            errors::MSG_INVALID_THREAD_AUTHOR_ORIGIN,
-        )?;
+        T::ThreadAuthorOriginValidator::ensure_actor_origin(origin, thread_author_id.clone())?;
 
         ensure!(!title.is_empty(), errors::MSG_EMPTY_TITLE_PROVIDED);
         ensure!(

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

@@ -97,11 +97,7 @@ impl crate::Trait for Test {
 }
 
 impl ActorOriginValidator<Origin, u64, u64> for () {
-    fn ensure_actor_origin(
-        origin: Origin,
-        actor_id: u64,
-        error: &'static str,
-    ) -> Result<u64, &'static str> {
+    fn ensure_actor_origin(origin: Origin, actor_id: u64) -> Result<u64, &'static str> {
         if system::ensure_none(origin).is_ok() {
             return Ok(1);
         }
@@ -110,7 +106,7 @@ impl ActorOriginValidator<Origin, u64, u64> for () {
             return Ok(1);
         }
 
-        Err(error)
+        Err("Invalid author")
     }
 }
 

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

@@ -268,7 +268,7 @@ fn update_post_call_failes_because_of_the_wrong_author() {
 
         post_fixture = post_fixture.with_author(2);
 
-        post_fixture.update_post_and_assert(Err(MSG_INVALID_POST_AUTHOR_ORIGIN));
+        post_fixture.update_post_and_assert(Err("Invalid author"));
 
         post_fixture = post_fixture.with_origin(RawOrigin::None).with_author(2);
 
@@ -424,11 +424,11 @@ fn add_discussion_thread_fails_because_of_max_thread_by_same_author_in_a_row_lim
 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(MSG_INVALID_THREAD_AUTHOR_ORIGIN));
+        discussion_fixture.create_discussion_and_assert(Err("Invalid author"));
 
         let discussion_fixture = DiscussionFixture::default()
             .with_origin(RawOrigin::Signed(3))
             .with_author(2);
-        discussion_fixture.create_discussion_and_assert(Err(MSG_INVALID_THREAD_AUTHOR_ORIGIN));
+        discussion_fixture.create_discussion_and_assert(Err("Invalid author"));
     });
 }

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

@@ -12,7 +12,5 @@ pub const MSG_STAKE_SHOULD_BE_EMPTY: &str = "Stake should be empty for this prop
 pub const MSG_STAKE_DIFFERS_FROM_REQUIRED: &str = "Stake differs from the proposal requirements";
 pub const MSG_INVALID_PARAMETER_APPROVAL_THRESHOLD: &str = "Approval threshold cannot be zero";
 pub const MSG_INVALID_PARAMETER_SLASHING_THRESHOLD: &str = "Slashing threshold cannot be zero";
-pub const MSG_ONLY_MEMBERS_CAN_PROPOSE: &str = "Only members can make or cancel a proposal";
-pub const MSG_ONLY_COUNCILORS_CAN_VOTE: &str = "Only councilors can vote on proposals";
 
 //pub const MSG_STAKE_IS_GREATER_THAN_BALANCE: &str = "Balance is too low to be staked";

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

@@ -169,7 +169,6 @@ decl_module! {
             T::VoterOriginValidator::ensure_actor_origin(
                 origin,
                 voter_id.clone(),
-                errors::MSG_ONLY_COUNCILORS_CAN_VOTE
             )?;
 
             ensure!(<Proposals<T>>::exists(proposal_id), errors::MSG_PROPOSAL_NOT_FOUND);
@@ -198,7 +197,6 @@ decl_module! {
             T::ProposerOriginValidator::ensure_actor_origin(
                 origin,
                 proposer_id.clone(),
-                errors::MSG_ONLY_MEMBERS_CAN_PROPOSE
             )?;
 
             ensure!(<Proposals<T>>::exists(proposal_id), errors::MSG_PROPOSAL_NOT_FOUND);
@@ -263,11 +261,8 @@ impl<T: Trait> Module<T> {
         proposal_type: u32,
         proposal_code: Vec<u8>,
     ) -> Result<T::ProposalId, &'static str> {
-        let account_id = T::ProposerOriginValidator::ensure_actor_origin(
-            origin,
-            proposer_id.clone(),
-            errors::MSG_ONLY_MEMBERS_CAN_PROPOSE,
-        )?;
+        let account_id =
+            T::ProposerOriginValidator::ensure_actor_origin(origin, proposer_id.clone())?;
 
         Self::ensure_create_proposal_parameters_are_valid(
             &parameters,

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

@@ -133,11 +133,7 @@ impl crate::Trait for Test {
 }
 
 impl common::origin_validator::ActorOriginValidator<Origin, u64, u64> for () {
-    fn ensure_actor_origin(
-        origin: Origin,
-        _account_id: u64,
-        _: &'static str,
-    ) -> Result<u64, &'static str> {
+    fn ensure_actor_origin(origin: Origin, _account_id: u64) -> Result<u64, &'static str> {
         let signed_account_id = system::ensure_signed(origin)?;
 
         Ok(signed_account_id)

runtime-modules/proposals/engine/src/types/council_origin_validator.rs

@@ -19,16 +19,14 @@ impl<T: crate::Trait>
     fn ensure_actor_origin(
         origin: <T as system::Trait>::Origin,
         actor_id: MemberId<T>,
-        error: &'static str,
     ) -> Result<<T as system::Trait>::AccountId, &'static str> {
-        let account_id =
-            <MembershipOriginValidator<T>>::ensure_actor_origin(origin, actor_id, error)?;
+        let account_id = <MembershipOriginValidator<T>>::ensure_actor_origin(origin, actor_id)?;
 
         if <governance::council::Module<T>>::is_councilor(&account_id) {
             return Ok(account_id);
         }
 
-        Err(error)
+        Err("Council validation failed: account id doesn't belong to a council member")
     }
 }
 
@@ -56,10 +54,10 @@ mod tests {
         initial_test_ext().execute_with(|| {
             let origin = RawOrigin::Signed(1);
             let member_id = 1;
-            let error = "Error";
+            let error = "Membership validation failed: cannot find a profile for a member";
 
             let validation_result =
-                CouncilManager::<Test>::ensure_actor_origin(origin.into(), member_id, error);
+                CouncilManager::<Test>::ensure_actor_origin(origin.into(), member_id);
 
             assert_eq!(validation_result, Err(error));
         });
@@ -72,7 +70,6 @@ mod tests {
 
             let account_id = 1;
             let origin = RawOrigin::Signed(account_id);
-            let error = "Error";
             let authority_account_id = 10;
             Membership::set_screening_authority(RawOrigin::Root.into(), authority_account_id)
                 .unwrap();
@@ -90,7 +87,7 @@ mod tests {
             let member_id = 0; // newly created member_id
 
             let validation_result =
-                CouncilManager::<Test>::ensure_actor_origin(origin.into(), member_id, error);
+                CouncilManager::<Test>::ensure_actor_origin(origin.into(), member_id);
 
             assert_eq!(validation_result, Ok(account_id));
         });
@@ -100,7 +97,8 @@ mod tests {
     fn council_origin_validator_fails_with_incompatible_account_id_and_member_id() {
         initial_test_ext().execute_with(|| {
             let account_id = 1;
-            let error = "Errorss";
+            let error =
+                "Membership validation failed: given account doesn't match with profile accounts";
             let authority_account_id = 10;
             Membership::set_screening_authority(RawOrigin::Root.into(), authority_account_id)
                 .unwrap();
@@ -121,7 +119,6 @@ mod tests {
             let validation_result = CouncilManager::<Test>::ensure_actor_origin(
                 RawOrigin::Signed(invalid_account_id).into(),
                 member_id,
-                error,
             );
 
             assert_eq!(validation_result, Err(error));
@@ -133,7 +130,7 @@ mod tests {
         initial_test_ext().execute_with(|| {
             let account_id = 1;
             let origin = RawOrigin::Signed(account_id);
-            let error = "Error";
+            let error = "Council validation failed: account id doesn't belong to a council member";
             let authority_account_id = 10;
             Membership::set_screening_authority(RawOrigin::Root.into(), authority_account_id)
                 .unwrap();
@@ -151,7 +148,7 @@ mod tests {
             let member_id = 0; // newly created member_id
 
             let validation_result =
-                CouncilManager::<Test>::ensure_actor_origin(origin.into(), member_id, error);
+                CouncilManager::<Test>::ensure_actor_origin(origin.into(), member_id);
 
             assert_eq!(validation_result, Err(error));
         });