membership runtime module: all dispatch error return typed Error

runtime-modules/membership/src/lib.rs

@@ -10,8 +10,9 @@ pub(crate) mod mock;
 mod tests;
 
 use codec::{Codec, Decode, Encode};
+use frame_support::dispatch::DispatchResult;
 use frame_support::traits::{Currency, Get, LockableCurrency, WithdrawReason};
-use frame_support::{decl_event, decl_module, decl_storage, ensure, Parameter};
+use frame_support::{decl_error, decl_event, decl_module, decl_storage, ensure, Parameter};
 use sp_arithmetic::traits::{BaseArithmetic, One};
 use sp_runtime::traits::{MaybeSerialize, Member, Zero};
 use sp_std::borrow::ToOwned;
@@ -20,11 +21,6 @@ use sp_std::vec::Vec;
 use system::{ensure_root, ensure_signed};
 
 use common::currency::{BalanceOf, GovernanceCurrency};
-
-//TODO: Convert errors to the Substrate decl_error! macro.
-/// Result with string error message. This exists for backward compatibility purpose.
-pub type DispatchResult = Result<(), &'static str>;
-
 pub trait Trait: system::Trait + GovernanceCurrency + pallet_timestamp::Trait {
     type Event: From<Event<Self>> + Into<<Self as system::Trait>::Event>;
 
@@ -277,13 +273,13 @@ decl_module! {
             let who = ensure_signed(origin)?;
 
             // make sure we are accepting new memberships
-            ensure!(Self::new_memberships_allowed(), "new members not allowed");
+            ensure!(Self::new_memberships_allowed(), Error::<T>::NewMembershipsNotAllowed);
 
             // ensure paid_terms_id is active
             let terms = Self::ensure_active_terms_id(paid_terms_id)?;
 
             // ensure enough free balance to cover terms fees
-            ensure!(T::Currency::can_slash(&who, terms.fee), "not enough balance to buy membership");
+            ensure!(T::Currency::can_slash(&who, terms.fee), Error::<T>::NotEnoughBalanceToBuyMembership);
 
             let user_info = Self::check_user_registration_info(handle, avatar_uri, about)?;
 
@@ -308,7 +304,7 @@ decl_module! {
 
             let membership = Self::ensure_membership(member_id)?;
 
-            ensure!(membership.controller_account == sender, "only controller account can update member about text");
+            ensure!(membership.controller_account == sender, Error::<T>::ControllerAccountRequired);
 
             Self::_change_member_about_text(member_id, &text)?;
         }
@@ -320,7 +316,7 @@ decl_module! {
 
             let membership = Self::ensure_membership(member_id)?;
 
-            ensure!(membership.controller_account == sender, "only controller account can update member avatar");
+            ensure!(membership.controller_account == sender, Error::<T>::ControllerAccountRequired);
 
             Self::_change_member_avatar(member_id, &uri)?;
         }
@@ -333,7 +329,7 @@ decl_module! {
 
             let membership = Self::ensure_membership(member_id)?;
 
-            ensure!(membership.controller_account == sender, "only controller account can update member handle");
+            ensure!(membership.controller_account == sender, Error::<T>::ControllerAccountRequired);
 
             Self::_change_member_handle(member_id, handle)?;
         }
@@ -351,7 +347,7 @@ decl_module! {
 
             let membership = Self::ensure_membership(member_id)?;
 
-            ensure!(membership.controller_account == sender, "only controller account can update member info");
+            ensure!(membership.controller_account == sender, Error::<T>::ControllerAccountRequired);
 
             if let Some(uri) = avatar_uri {
                 Self::_change_member_avatar(member_id, &uri)?;
@@ -370,7 +366,7 @@ decl_module! {
 
             let mut membership = Self::ensure_membership(member_id)?;
 
-            ensure!(membership.root_account == sender, "only root account can set new controller account");
+            ensure!(membership.root_account == sender, Error::<T>::RootAccountRequired);
 
             // only update if new_controller_account is different than current one
             if membership.controller_account != new_controller_account {
@@ -394,7 +390,7 @@ decl_module! {
 
             let mut membership = Self::ensure_membership(member_id)?;
 
-            ensure!(membership.root_account == sender, "only root account can set new root account");
+            ensure!(membership.root_account == sender, Error::<T>::RootAccountRequired);
 
             // only update if new root account is different than current one
             if membership.root_account != new_root_account {
@@ -429,10 +425,10 @@ decl_module! {
             let sender = ensure_signed(origin)?;
 
             if <ScreeningAuthority<T>>::exists() {
-                ensure!(sender == Self::screening_authority(), "not screener");
+                ensure!(sender == Self::screening_authority(), Error::<T>::NotScreeningAuthority);
             } else {
                 // no screening authority defined. Cannot accept this request
-                return Err("no screening authority defined".into());
+                return Err(Error::<T>::NoScreeningAuthorityDefined.into());
             }
 
             // make sure we are accepting new memberships
@@ -443,18 +439,18 @@ decl_module! {
             if let Some(initial_balance) = initial_balance {
                 ensure!(
                     T::ScreenedMemberMaxInitialBalance::get() >= initial_balance,
-                    "InitialBalanceExceedsMaxInitialBalance"
+                    Error::<T>::InitialBalanceExceedsMaxInitialBalance
                 );
 
                 // Only allow "new" accounts with 0 balance
                 ensure!(
                     T::Currency::free_balance(&new_member_account).is_zero(),
-                    "OnlyNewAccountsCanBeUsedForScreenedMembers"
+                    Error::<T>::OnlyNewAccountsCanBeUsedForScreenedMembers
                 );
 
                 ensure!(
                     system::Module::<T>::account_nonce(&new_member_account).is_zero(),
-                    "OnlyNewAccountsCanBeUsedForScreenedMembers"
+                    Error::<T>::OnlyNewAccountsCanBeUsedForScreenedMembers
                 );
 
                 // Check account nonce
@@ -515,11 +511,11 @@ pub enum MemberRootAccountMismatch {
 
 impl<T: Trait> Module<T> {
     /// Provided that the member_id exists return its membership. Returns error otherwise.
-    pub fn ensure_membership(id: T::MemberId) -> Result<Membership<T>, &'static str> {
+    pub fn ensure_membership(id: T::MemberId) -> Result<Membership<T>, Error<T>> {
         if <MembershipById<T>>::contains_key(&id) {
             Ok(Self::membership(&id))
         } else {
-            Err("member profile not found")
+            Err(Error::<T>::MemberProfileNotFound)
         }
     }
 
@@ -549,37 +545,37 @@ impl<T: Trait> Module<T> {
 
     fn ensure_active_terms_id(
         terms_id: T::PaidTermId,
-    ) -> Result<PaidMembershipTerms<BalanceOf<T>>, &'static str> {
+    ) -> Result<PaidMembershipTerms<BalanceOf<T>>, Error<T>> {
         let active_terms = Self::active_paid_membership_terms();
         ensure!(
             active_terms.iter().any(|&id| id == terms_id),
-            "paid terms id not active"
+            Error::<T>::PaidTermIdNotActive
         );
 
         if <PaidMembershipTermsById<T>>::contains_key(terms_id) {
             Ok(Self::paid_membership_terms_by_id(terms_id))
         } else {
-            Err("paid membership term id does not exist")
+            Err(Error::<T>::PaidTermIdNotFound)
         }
     }
 
     #[allow(clippy::ptr_arg)] // cannot change to the "&[u8]" suggested by clippy
-    fn ensure_unique_handle(handle: &Vec<u8>) -> DispatchResult {
+    fn ensure_unique_handle(handle: &Vec<u8>) -> Result<(), Error<T>> {
         ensure!(
             !<MemberIdByHandle<T>>::contains_key(handle),
-            "handle already registered"
+            Error::<T>::HandleAlreadyRegistered
         );
         Ok(())
     }
 
-    fn validate_handle(handle: &[u8]) -> DispatchResult {
+    fn validate_handle(handle: &[u8]) -> Result<(), Error<T>> {
         ensure!(
             handle.len() >= Self::min_handle_length() as usize,
-            "handle too short"
+            Error::<T>::HandleTooShort
         );
         ensure!(
             handle.len() <= Self::max_handle_length() as usize,
-            "handle too long"
+            Error::<T>::HandleTooLong
         );
         Ok(())
     }
@@ -590,10 +586,10 @@ impl<T: Trait> Module<T> {
         text
     }
 
-    fn validate_avatar(uri: &[u8]) -> DispatchResult {
+    fn validate_avatar(uri: &[u8]) -> Result<(), Error<T>> {
         ensure!(
             uri.len() <= Self::max_avatar_uri_length() as usize,
-            "avatar uri too long"
+            Error::<T>::AvatarUriTooLong
         );
         Ok(())
     }
@@ -603,9 +599,9 @@ impl<T: Trait> Module<T> {
         handle: Option<Vec<u8>>,
         avatar_uri: Option<Vec<u8>>,
         about: Option<Vec<u8>>,
-    ) -> Result<ValidatedUserInfo, &'static str> {
+    ) -> Result<ValidatedUserInfo, Error<T>> {
         // Handle is required during registration
-        let handle = handle.ok_or("handle must be provided during registration")?;
+        let handle = handle.ok_or(Error::<T>::HandleMustBeProvidedDuringRegistration)?;
         Self::validate_handle(&handle)?;
 
         let about = Self::validate_text(&about.unwrap_or_default());
@@ -626,7 +622,7 @@ impl<T: Trait> Module<T> {
         entry_method: EntryMethod<T::PaidTermId, T::AccountId>,
         registered_at_block: T::BlockNumber,
         registered_at_time: T::Moment,
-    ) -> Result<T::MemberId, &'static str> {
+    ) -> Result<T::MemberId, Error<T>> {
         Self::ensure_unique_handle(&user_info.handle)?;
 
         let new_member_id = Self::members_created();
@@ -740,3 +736,59 @@ impl<T: Trait> Module<T> {
         Ok(())
     }
 }
+
+decl_error! {
+    /// Membership module predefined errors
+    pub enum Error for Module<T: Trait> {
+        /// New memberships not allowed.
+        NewMembershipsNotAllowed,
+
+        /// A screening authority is not defined.
+        NoScreeningAuthorityDefined,
+
+        /// Origin is not the screeing authority.
+        NotScreeningAuthority,
+
+        /// Not enough balance to buy membership.
+        NotEnoughBalanceToBuyMembership,
+
+        /// Screening authority attempting to endow more that maximum allowed.
+        InitialBalanceExceedsMaxInitialBalance,
+
+        /// Only new accounts can be used for screened members.
+        OnlyNewAccountsCanBeUsedForScreenedMembers,
+
+        /// Controller account required.
+        ControllerAccountRequired,
+
+        /// Root account required.
+        RootAccountRequired,
+
+        /// Invalid origin.
+        UnsignedOrigin,
+
+        /// Member profile not found (invalid member id).
+        MemberProfileNotFound,
+
+        /// Handle already registered.
+        HandleAlreadyRegistered,
+
+        /// Handle must be provided during registration.
+        HandleMustBeProvidedDuringRegistration,
+
+        /// Handle too short.
+        HandleTooShort,
+
+        /// Handle too long.
+        HandleTooLong,
+
+        /// Avatar url is too long.
+        AvatarUriTooLong,
+
+        /// Paid term id not found.
+        PaidTermIdNotFound,
+
+        /// Paid term id not active.
+        PaidTermIdNotActive,
+    }
+}

runtime-modules/membership/src/tests.rs

@@ -2,9 +2,9 @@
 
 use super::genesis;
 use super::mock::*;
+use crate::*;
 
 use frame_support::*;
-// use sp_core::traits;
 
 fn get_membership_by_id(member_id: u64) -> crate::Membership<Test> {
     if <crate::MembershipById<Test>>::contains_key(member_id) {
@@ -14,12 +14,6 @@ fn get_membership_by_id(member_id: u64) -> crate::Membership<Test> {
     }
 }
 
-fn assert_dispatch_error_message(result: Result<(), &'static str>, expected_message: &'static str) {
-    assert!(result.is_err());
-    let message = result.err().unwrap();
-    assert_eq!(message, expected_message);
-}
-
 #[derive(Clone, Debug, PartialEq)]
 pub struct TestUserInfo {
     pub handle: Option<Vec<u8>>,
@@ -150,9 +144,9 @@ fn buy_membership_fails_without_enough_balance() {
             let initial_balance = DEFAULT_FEE - 1;
             set_alice_free_balance(initial_balance);
 
-            assert_dispatch_error_message(
+            assert_err!(
                 buy_default_membership_as_alice(),
-                "not enough balance to buy membership",
+                Error::<Test>::NotEnoughBalanceToBuyMembership,
             );
         });
 }
@@ -174,9 +168,9 @@ fn new_memberships_allowed_flag() {
 
             crate::NewMembershipsAllowed::put(false);
 
-            assert_dispatch_error_message(
+            assert_err!(
                 buy_default_membership_as_alice(),
-                "new members not allowed",
+                Error::<Test>::NewMembershipsNotAllowed,
             );
         });
 }
@@ -201,9 +195,9 @@ fn unique_handles() {
             <crate::MemberIdByHandle<Test>>::insert(get_alice_info().handle.unwrap(), 1);
 
             // should not be allowed to buy membership with that handle
-            assert_dispatch_error_message(
+            assert_err!(
                 buy_default_membership_as_alice(),
-                "handle already registered",
+                Error::<Test>::HandleAlreadyRegistered,
             );
         });
 }