feat: harmonize dispatch error names (#441)

## fixes KILTProtocol/ticket#2290

This harmonizes the instance names in the Error enums of our pallets.

Before this change the names were kind of randomly choosen, so we had
for example CTypeNotFound and DidNotPresent. Those names were bad for
thwo reasons:

1) They mean the same thing but use different wording
2) They contain the module name

After this change both instances would be named NotFound.

All the other changes are something along those lines.

Co-authored-by: Albrecht <albrecht@kilt.io>

Author: trusch

pallet_error_naming_convention.md

@@ -0,0 +1,11 @@
+# Pallet Error Naming Conventions
+
+1) Use capitalized camel case in the variant names. For example, instead of using "NOT_FOUND" you should use "NotFound".
+
+2) Avoid using the word "error" as a suffix for the variant names. For example, instead of using "NotFoundError" you should use "NotFound". It's clear from the caller's context that this is an error.
+
+3) Avoid using the pallet name in the variant names. For example instead of "Web3NameNotFound" you should use "NotFound". 
+
+4) Try to take words from the vocabulary already defined in the code base. For example instead of introducing a new variant "NotExisting" you should use, once again, "NotFound". Common vocabulary includes: NotFound, NotAuthorized, AlreadyExists, MaxXYZExceeded.
+
+5) Use descriptive and concise names for the variants. Avoid using abbreviations or acronyms unless they are widely recognized and understood by other developers who may be working on the codebase.

pallets/attestation/src/lib.rs

@@ -195,12 +195,12 @@ pub mod pallet {
 		/// The attestation has already been revoked.
 		AlreadyRevoked,
 		/// No attestation on chain matching the claim hash.
-		AttestationNotFound,
+		NotFound,
 		/// The attestation CType does not match the CType specified in the
 		/// delegation hierarchy root.
 		CTypeMismatch,
 		/// The call origin is not authorized to change the attestation.
-		Unauthorized,
+		NotAuthorized,
 		/// The maximum number of delegated attestations has already been
 		/// reached for the corresponding delegation id such that another one
 		/// cannot be added.
@@ -248,7 +248,7 @@ pub mod pallet {
 
 			ensure!(
 				ctype::Ctypes::<T>::contains_key(ctype_hash),
-				ctype::Error::<T>::CTypeNotFound
+				ctype::Error::<T>::NotFound
 			);
 			ensure!(
 				!Attestations::<T>::contains_key(claim_hash),
@@ -317,13 +317,13 @@ pub mod pallet {
 			let source = <T as Config>::EnsureOrigin::ensure_origin(origin)?;
 			let who = source.subject();
 
-			let attestation = Attestations::<T>::get(claim_hash).ok_or(Error::<T>::AttestationNotFound)?;
+			let attestation = Attestations::<T>::get(claim_hash).ok_or(Error::<T>::NotFound)?;
 
 			ensure!(!attestation.revoked, Error::<T>::AlreadyRevoked);
 
 			if attestation.attester != who {
-				let attestation_auth_id = attestation.authorization_id.as_ref().ok_or(Error::<T>::Unauthorized)?;
-				authorization.ok_or(Error::<T>::Unauthorized)?.can_revoke(
+				let attestation_auth_id = attestation.authorization_id.as_ref().ok_or(Error::<T>::NotAuthorized)?;
+				authorization.ok_or(Error::<T>::NotAuthorized)?.can_revoke(
 					&who,
 					&attestation.ctype_hash,
 					&claim_hash,
@@ -377,11 +377,11 @@ pub mod pallet {
 			let source = <T as Config>::EnsureOrigin::ensure_origin(origin)?;
 			let who = source.subject();
 
-			let attestation = Attestations::<T>::get(claim_hash).ok_or(Error::<T>::AttestationNotFound)?;
+			let attestation = Attestations::<T>::get(claim_hash).ok_or(Error::<T>::NotFound)?;
 
 			if attestation.attester != who {
-				let attestation_auth_id = attestation.authorization_id.as_ref().ok_or(Error::<T>::Unauthorized)?;
-				authorization.ok_or(Error::<T>::Unauthorized)?.can_remove(
+				let attestation_auth_id = attestation.authorization_id.as_ref().ok_or(Error::<T>::NotAuthorized)?;
+				authorization.ok_or(Error::<T>::NotAuthorized)?.can_remove(
 					&who,
 					&attestation.ctype_hash,
 					&claim_hash,
@@ -412,9 +412,9 @@ pub mod pallet {
 		#[pallet::weight(<T as pallet::Config>::WeightInfo::reclaim_deposit())]
 		pub fn reclaim_deposit(origin: OriginFor<T>, claim_hash: ClaimHashOf<T>) -> DispatchResult {
 			let who = ensure_signed(origin)?;
-			let attestation = Attestations::<T>::get(claim_hash).ok_or(Error::<T>::AttestationNotFound)?;
+			let attestation = Attestations::<T>::get(claim_hash).ok_or(Error::<T>::NotFound)?;
 
-			ensure!(attestation.deposit.owner == who, Error::<T>::Unauthorized);
+			ensure!(attestation.deposit.owner == who, Error::<T>::NotAuthorized);
 
 			// *** No Fail beyond this point ***
 
@@ -440,8 +440,8 @@ pub mod pallet {
 			let subject = source.subject();
 			let sender = source.sender();
 
-			let attestation = Attestations::<T>::get(claim_hash).ok_or(Error::<T>::AttestationNotFound)?;
-			ensure!(attestation.attester == subject, Error::<T>::Unauthorized);
+			let attestation = Attestations::<T>::get(claim_hash).ok_or(Error::<T>::NotFound)?;
+			ensure!(attestation.attester == subject, Error::<T>::NotAuthorized);
 
 			AttestationStorageDepositCollector::<T>::change_deposit_owner(&claim_hash, sender)?;
 
@@ -456,8 +456,8 @@ pub mod pallet {
 		pub fn update_deposit(origin: OriginFor<T>, claim_hash: ClaimHashOf<T>) -> DispatchResult {
 			let sender = ensure_signed(origin)?;
 
-			let attestation = Attestations::<T>::get(claim_hash).ok_or(Error::<T>::AttestationNotFound)?;
-			ensure!(attestation.deposit.owner == sender, Error::<T>::Unauthorized);
+			let attestation = Attestations::<T>::get(claim_hash).ok_or(Error::<T>::NotFound)?;
+			ensure!(attestation.deposit.owner == sender, Error::<T>::NotAuthorized);
 
 			AttestationStorageDepositCollector::<T>::update_deposit(&claim_hash)?;
 
@@ -482,7 +482,7 @@ pub mod pallet {
 		fn deposit(
 			key: &ClaimHashOf<T>,
 		) -> Result<Deposit<AccountIdOf<T>, <Self::Currency as Currency<AccountIdOf<T>>>::Balance>, DispatchError> {
-			let attestation = Attestations::<T>::get(key).ok_or(Error::<T>::AttestationNotFound)?;
+			let attestation = Attestations::<T>::get(key).ok_or(Error::<T>::NotFound)?;
 			Ok(attestation.deposit)
 		}
 
@@ -494,7 +494,7 @@ pub mod pallet {
 			key: &ClaimHashOf<T>,
 			deposit: Deposit<AccountIdOf<T>, <Self::Currency as Currency<AccountIdOf<T>>>::Balance>,
 		) -> Result<(), DispatchError> {
-			let attestation = Attestations::<T>::get(key).ok_or(Error::<T>::AttestationNotFound)?;
+			let attestation = Attestations::<T>::get(key).ok_or(Error::<T>::NotFound)?;
 			Attestations::<T>::insert(key, AttestationDetails { deposit, ..attestation });
 
 			Ok(())

pallets/attestation/src/tests.rs

@@ -136,7 +136,7 @@ fn test_attest_ctype_not_found() {
 					ctype_hash,
 					None
 				),
-				ctype::Error::<Test>::CTypeNotFound
+				ctype::Error::<Test>::NotFound
 			);
 		});
 }
@@ -272,7 +272,7 @@ fn test_revoke_not_found() {
 					claim_hash,
 					authorization_info
 				),
-				attestation::Error::<Test>::AttestationNotFound
+				attestation::Error::<Test>::NotFound
 			);
 		});
 }
@@ -374,7 +374,7 @@ fn test_remove_unauthorized() {
 					claim_hash,
 					authorization_info
 				),
-				attestation::Error::<Test>::Unauthorized
+				attestation::Error::<Test>::NotAuthorized
 			);
 		});
 }
@@ -393,7 +393,7 @@ fn test_remove_not_found() {
 			assert!(Balances::reserved_balance(ACCOUNT_00).is_zero());
 			assert_noop!(
 				Attestation::remove(DoubleOrigin(ACCOUNT_00, attester.clone()).into(), claim_hash, None),
-				attestation::Error::<Test>::AttestationNotFound
+				attestation::Error::<Test>::NotFound
 			);
 		});
 }
@@ -465,7 +465,7 @@ fn test_reclaim_unauthorized() {
 		.execute_with(|| {
 			assert_noop!(
 				Attestation::reclaim_deposit(RuntimeOrigin::signed(ACCOUNT_01), claim_hash),
-				attestation::Error::<Test>::Unauthorized,
+				attestation::Error::<Test>::NotAuthorized,
 			);
 		});
 }
@@ -483,7 +483,7 @@ fn test_reclaim_deposit_not_found() {
 		.execute_with(|| {
 			assert_noop!(
 				Attestation::reclaim_deposit(RuntimeOrigin::signed(ACCOUNT_01), claim_hash),
-				attestation::Error::<Test>::AttestationNotFound,
+				attestation::Error::<Test>::NotFound,
 			);
 		});
 }
@@ -564,7 +564,7 @@ fn test_change_deposit_owner_unauthorized() {
 		.execute_with(|| {
 			assert_noop!(
 				Attestation::change_deposit_owner(DoubleOrigin(ACCOUNT_00, evil_actor).into(), claim_hash),
-				attestation::Error::<Test>::Unauthorized,
+				attestation::Error::<Test>::NotAuthorized,
 			);
 		});
 }
@@ -582,7 +582,7 @@ fn test_change_deposit_owner_not_found() {
 		.execute_with(|| {
 			assert_noop!(
 				Attestation::change_deposit_owner(DoubleOrigin(ACCOUNT_00, attester).into(), claim_hash),
-				attestation::Error::<Test>::AttestationNotFound,
+				attestation::Error::<Test>::NotFound,
 			);
 		});
 }
@@ -650,7 +650,7 @@ fn test_update_deposit_unauthorized() {
 			);
 			assert_noop!(
 				Attestation::update_deposit(RuntimeOrigin::signed(ACCOUNT_01), claim_hash),
-				Error::<Test>::Unauthorized
+				Error::<Test>::NotAuthorized
 			);
 		});
 }

pallets/ctype/src/lib.rs

@@ -131,9 +131,9 @@ pub mod pallet {
 	#[pallet::error]
 	pub enum Error<T> {
 		/// There is no CType with the given hash.
-		CTypeNotFound,
+		NotFound,
 		/// The CType already exists.
-		CTypeAlreadyExists,
+		AlreadyExists,
 		/// The paying account was unable to pay the fees for creating a ctype.
 		UnableToPayFees,
 	}
@@ -171,7 +171,7 @@ pub mod pallet {
 
 			let hash = <T as frame_system::Config>::Hashing::hash(&ctype[..]);
 
-			ensure!(!Ctypes::<T>::contains_key(hash), Error::<T>::CTypeAlreadyExists);
+			ensure!(!Ctypes::<T>::contains_key(hash), Error::<T>::AlreadyExists);
 
 			// *** No Fail except during withdraw beyond this point  ***
 
@@ -216,7 +216,7 @@ pub mod pallet {
 					ctype_entry.created_at = block_number;
 					Ok(())
 				} else {
-					Err(Error::<T>::CTypeNotFound)
+					Err(Error::<T>::NotFound)
 				}
 			})?;
 

pallets/ctype/src/tests.rs

@@ -87,7 +87,7 @@ fn check_duplicate_ctype_creation() {
 		.execute_with(|| {
 			assert_noop!(
 				Ctype::add(DoubleOrigin(deposit_owner, creator).into(), ctype),
-				ctype::Error::<Test>::CTypeAlreadyExists
+				ctype::Error::<Test>::AlreadyExists
 			);
 		});
 }
@@ -127,7 +127,7 @@ fn set_block_number_ctype_not_found() {
 	ExtBuilder::default().build().execute_with(|| {
 		assert_noop!(
 			Ctype::set_block_number(RawOrigin::Signed(ACCOUNT_00).into(), ctype_hash, 100u64),
-			ctype::Error::<Test>::CTypeNotFound
+			ctype::Error::<Test>::NotFound
 		);
 	})
 }

pallets/delegation/src/lib.rs

@@ -280,7 +280,7 @@ pub mod pallet {
 		/// The max number of parent checks exceeds the limit for the pallet.
 		MaxParentChecksTooLarge,
 		/// An error that is not supposed to take place, yet it happened.
-		InternalError,
+		Internal,
 		/// The max number of all children has been reached for the
 		/// corresponding delegation node.
 		MaxChildrenExceeded,
@@ -331,7 +331,7 @@ pub mod pallet {
 
 			ensure!(
 				<ctype::Ctypes<T>>::contains_key(ctype_hash),
-				<ctype::Error<T>>::CTypeNotFound
+				<ctype::Error<T>>::NotFound
 			);
 
 			// *** No Fail beyond this point ***

pallets/delegation/src/tests.rs

@@ -114,7 +114,7 @@ fn ctype_not_found_create_root_delegation_error() {
 				operation.id,
 				operation.ctype_hash
 			),
-			ctype::Error::<Test>::CTypeNotFound
+			ctype::Error::<Test>::NotFound
 		);
 	});
 }