using proper errors

pallets/living-assets-ownership/src/functions.rs

@@ -5,7 +5,7 @@ use sp_core::{H160, U256};
 
 impl<T: Config> Pallet<T> {
 	/// See [Self::create_collection]
-	pub fn do_create_collection(who: T::AccountId) -> Result<CollectionId, &'static str> {
+	pub fn do_create_collection(who: T::AccountId) -> Result<CollectionId, Error<T>> {
 		// Retrieve the current collection count to use as the new collection's ID
 		let collection_id = Self::collection_counter();
 

pallets/living-assets-ownership/src/lib.rs

@@ -55,8 +55,6 @@ pub mod pallet {
 	// Errors inform users that something went wrong.
 	#[pallet::error]
 	pub enum Error<T> {
-		/// Collection already exists
-		CollectionAlreadyExists,
 		/// Collection id overflow
 		CollectionIdOverflow,
 	}
@@ -83,16 +81,29 @@ pub mod pallet {
 			OwnerOfCollection::<T>::get(collection_id)
 		}
 
-		fn create_collection(owner: T::AccountId) -> Result<CollectionId, &'static str> {
-			Self::do_create_collection(owner)
+		fn create_collection(
+			owner: T::AccountId,
+		) -> Result<CollectionId, traits::CollectionManagerError> {
+			match Self::do_create_collection(owner) {
+				Ok(collection_id) => Ok(collection_id),
+				Err(err) => match err {
+					Error::CollectionIdOverflow => {
+						Err(traits::CollectionManagerError::CollectionIdOverflow)
+					},
+					_ => Err(traits::CollectionManagerError::UnknownError),
+				},
+			}
 		}
 	}
 
 	impl<T: Config> traits::Erc721 for Pallet<T> {
-		fn owner_of(collection_id: CollectionId, asset_id: U256) -> Result<H160, &'static str> {
+		fn owner_of(
+			collection_id: CollectionId,
+			asset_id: U256,
+		) -> Result<H160, traits::Erc721Error> {
 			match OwnerOfCollection::<T>::get(collection_id) {
 				Some(_) => Ok(convert_asset_id_to_owner(asset_id)),
-				None => Err("Collection does not exist"),
+				None => Err(traits::Erc721Error::UnexistentCollection),
 			}
 		}
 	}

pallets/living-assets-ownership/src/tests.rs

@@ -3,10 +3,10 @@ use core::str::FromStr;
 use crate::{
 	address_to_collection_id, collection_id_to_address, is_collection_address,
 	mock::*,
-	traits::{CollectionManager, Erc721},
+	traits::{self, CollectionManager, Erc721},
 	CollectionError, Event,
 };
-use frame_support::assert_ok;
+use frame_support::{assert_err, assert_ok};
 use sp_core::H160;
 
 type AccountId = <Test as frame_system::Config>::AccountId;
@@ -119,10 +119,8 @@ fn living_assets_ownership_trait_id_of_new_collection_should_be_consecutive() {
 #[test]
 fn erc721_owner_of_asset_of_unexistent_collection() {
 	new_test_ext().execute_with(|| {
-		assert_eq!(
-			<LivingAssetsModule as Erc721>::owner_of(0, 2.into()),
-			Err("Collection does not exist")
-		);
+		let result = <LivingAssetsModule as Erc721>::owner_of(0, 2.into());
+		assert_err!(result, traits::Erc721Error::UnexistentCollection);
 	});
 }
 

pallets/living-assets-ownership/src/traits.rs

@@ -1,48 +1,94 @@
-use sp_core::{H160, U256};
-
 use crate::CollectionId;
+use sp_core::{H160, U256};
 
-/// The `CollectionManager` trait provides an interface for managing collections in a
-/// decentralized and non-fungible asset management system. This system allows for the creation of
-/// collections, each of which can be owned by a unique `AccountId`.
+/// The `CollectionManager` trait provides an interface for managing collections in a decentralized system.
 ///
-/// A collection in this context can be thought of as a container for non-fungible assets.
-/// Each collection has an associated `collection_id` which is a unique identifier for the collection
-/// and can be used to retrieve the owner of the collection.
+/// A collection is a container for non-fungible assets, uniquely identified by a `collection_id`. The system allows
+/// for the creation and ownership management of these collections.
 ///
 /// # Methods
 ///
-/// - `owner_of_collection(collection_id: CollectionId) -> Option<AccountId>`: This method retrieves the owner
-/// of a collection given its `collection_id`. If no collection exists with the provided `collection_id`,
-/// the method returns `None`.
-///
-/// - `create_collection(owner: AccountId) -> Result<CollectionId, &'static str>`: This method creates a
-/// new collection and assigns ownership to the provided `AccountId`. The method returns the `collection_id`
-/// of the newly created collection.
-///
+/// - `owner_of_collection`: Retrieve the owner of a specified collection.
+/// - `create_collection`: Create a new collection and assign it to an owner.
 pub trait CollectionManager<AccountId> {
-	/// Get owner of collection
+	/// Retrieves the owner of the specified collection.
+	///
+	/// # Arguments
+	///
+	/// * `collection_id` - The unique identifier of the collection.
+	///
+	/// # Returns
+	///
+	/// The account ID of the collection's owner or `None` if the collection doesn't exist.
 	fn owner_of_collection(collection_id: CollectionId) -> Option<AccountId>;
 
-	/// Create collection
-	fn create_collection(owner: AccountId) -> Result<CollectionId, &'static str>;
+	/// Creates a new collection and assigns it to the specified owner.
+	///
+	/// # Arguments
+	///
+	/// * `owner` - The account ID of the new collection's owner.
+	///
+	/// # Returns
+	///
+	/// A result containing the `collection_id` of the newly created collection or an error.
+	fn create_collection(owner: AccountId) -> Result<CollectionId, CollectionManagerError>;
+}
+
+/// Errors that can occur when managing collections.
+///
+/// - `CollectionAlreadyExists`: A collection with the same ID already exists.
+/// - `CollectionIdOverflow`: The ID for the new collection would overflow.
+/// - `UnknownError`: An unspecified error occurred.
+#[derive(Debug, PartialEq)]
+pub enum CollectionManagerError {
+	CollectionAlreadyExists,
+	CollectionIdOverflow,
+	UnknownError,
 }
 
-/// `Erc721` Trait
+impl AsRef<[u8]> for CollectionManagerError {
+	fn as_ref(&self) -> &[u8] {
+		match self {
+			CollectionManagerError::CollectionAlreadyExists => b"CollectionAlreadyExists",
+			CollectionManagerError::CollectionIdOverflow => b"CollectionIdOverflow",
+			CollectionManagerError::UnknownError => b"UnknownError",
+		}
+	}
+}
+
+/// The `Erc721` trait provides an interface for handling ERC721 tokens in a blockchain environment.
+///
+/// ERC721 tokens are a standard for representing ownership of unique items on the Ethereum blockchain.
 ///
-/// This trait provides an interface for handling ERC721 tokens, a standard for non-fungible tokens on the blockchain.
+/// # Methods
+///
+/// - `owner_of`: Retrieve the owner of a specific asset within a collection.
 pub trait Erc721 {
-	/// Retrieves the owner of a specific asset in a collection.
+	/// Retrieves the owner of a specific asset within the specified collection.
 	///
-	/// # Parameters
+	/// # Arguments
 	///
-	/// * `collection_id`: An identifier for the collection to which the asset belongs.
-	/// * `asset_id`: The unique identifier for the asset within the specified collection.
+	/// * `collection_id` - The unique identifier for the collection.
+	/// * `asset_id` - The unique identifier for the asset within the collection.
 	///
 	/// # Returns
 	///
-	/// * A `Result` which is:
-	///   - `Ok(H160)`: Returns the Ethereum address (`H160`) of the owner of the asset.
-	///   - `Err(&'static str)`: Returns an error message if the asset owner could not be determined.
-	fn owner_of(collection_id: CollectionId, asset_id: U256) -> Result<H160, &'static str>;
+	/// The Ethereum address (`H160`) of the asset's owner or an error.
+	fn owner_of(collection_id: CollectionId, asset_id: U256) -> Result<H160, Erc721Error>;
+}
+
+/// Errors that can occur when interacting with ERC721 tokens.
+///
+/// - `UnexistentCollection`: The specified collection does not exist.
+#[derive(Debug, PartialEq)]
+pub enum Erc721Error {
+	UnexistentCollection,
+}
+
+impl AsRef<[u8]> for Erc721Error {
+	fn as_ref(&self) -> &[u8] {
+		match self {
+			Erc721Error::UnexistentCollection => b"UnexistentCollection",
+		}
+	}
 }

precompile/erc721/src/tests.rs

@@ -1,7 +1,7 @@
 use core::str::FromStr;
 
 use super::*;
-use pallet_living_assets_ownership::CollectionId;
+use pallet_living_assets_ownership::{traits::Erc721Error, CollectionId};
 use precompile_utils::testing::create_mock_handle_from_input;
 use sp_core::{H160, U256};
 
@@ -39,7 +39,7 @@ fn owner_of_asset_should_return_an_address() {
 
 #[test]
 fn if_mock_fails_should_return_the_error() {
-	impl_precompile_mock_simple!(Mock, Err("spaghetti error"));
+	impl_precompile_mock_simple!(Mock, Err(Erc721Error::UnexistentCollection));
 
 	let owner_of_asset_4 =
 		hex::decode("6352211e0000000000000000000000000000000000000000000000000000000000000004")
@@ -48,7 +48,7 @@ fn if_mock_fails_should_return_the_error() {
 	handle.code_address = H160::from_str("ffffffffffffffffffffffff0000000000000005").unwrap();
 	let result = Mock::execute(&mut handle);
 	assert!(result.is_err());
-	assert_eq!(result.unwrap_err(), revert("spaghetti error"),);
+	assert_eq!(result.unwrap_err(), revert(Erc721Error::UnexistentCollection));
 }
 
 #[test]
@@ -91,7 +91,7 @@ mod helpers {
 				fn owner_of(
 					collectio_id: CollectionId,
 					asset_id: U256,
-				) -> Result<AccountId, &'static str> {
+				) -> Result<AccountId, Erc721Error> {
 					($owner_of_collection)(collectio_id, asset_id)
 				}
 			}

precompile/living-assets/src/tests.rs

@@ -4,6 +4,7 @@
 #![allow(clippy::redundant_closure_call)]
 
 use super::*;
+use pallet_living_assets_ownership::traits::CollectionManagerError;
 use precompile_utils::{
 	revert, succeed,
 	testing::{create_mock_handle, create_mock_handle_from_input},
@@ -31,11 +32,15 @@ fn check_log_selectors() {
 
 #[test]
 fn failing_create_collection_should_return_error() {
-	impl_precompile_mock_simple!(Mock, Err("spaghetti code"), Some(H160::zero()));
+	impl_precompile_mock_simple!(
+		Mock,
+		Err(CollectionManagerError::CollectionAlreadyExists),
+		Some(H160::zero())
+	);
 
 	let mut handle = create_mock_handle_from_input(hex::decode(CREATE_COLLECTION).unwrap());
 	let result = Mock::execute(&mut handle);
-	assert_eq!(result.unwrap_err(), revert("spaghetti code"));
+	assert_eq!(result.unwrap_err(), revert(CollectionManagerError::CollectionAlreadyExists));
 }
 
 #[test]
@@ -142,7 +147,9 @@ mod helpers {
 			impl pallet_living_assets_ownership::traits::CollectionManager<AccountId>
 				for CollectionManagerMock
 			{
-				fn create_collection(owner: AccountId) -> Result<CollectionId, &'static str> {
+				fn create_collection(
+					owner: AccountId,
+				) -> Result<CollectionId, CollectionManagerError> {
 					($create_collection_result)(owner)
 				}