Refactor UniqueAssets Trait

Breaking change. Use generic traits and associated types correctly in order to create a usable interface.

Author: danforbes

Cargo.toml

@@ -1,7 +1,7 @@
 [package]
 edition = '2018'
 name = 'pallet-commodities'
-version = '1.0.0-rc6'
+version = '1.0.0-rc7'
 authors = ['Dan Forbes <dan@parity.io>']
 license = 'Unlicense'
 description = 'A unique asset (NFT) interface and a Substrate FRAME implementation optimized for commodity assets.'

README.md

@@ -2,33 +2,24 @@
 
 # Commodities FRAME Pallet: NFTs for Substrate
 
-This is a [FRAME](https://substrate.dev/docs/en/knowledgebase/runtime/frame) pallet that defines and implements a
-[non-fungible token (NFT)](https://en.wikipedia.org/wiki/Non-fungible_token) interface as well as an interface for
-managing a set of such assets, including asset ownership, creation, destruction and transfer.
+This is a [FRAME](https://substrate.dev/docs/en/knowledgebase/runtime/frame) pallet that defines and implements an
+interface for managing a set of [non-fungible tokens (NFTs)](https://en.wikipedia.org/wiki/Non-fungible_token). Assets
+have an owner and can be created, destroyed and transferred.
 
 ## Interface
 
-This package defines [two public traits](src/nft.rs) (Rust interfaces) for working with NFTs: the `NFT` trait and the
-`UniqueAssets` trait.
-
-## `NFT` Trait
-
-The `NFT` trait uses two types to define a unique asset:
-
-- `ID`: a URI for the asset
-- `Info`: a set of attributes that uniquely describe the asset
-
-Assets with equivalent attributes (as defined by the `Info` type) **must** have an equal `ID` and assets with different
-`ID`s **must not** have equivalent attributes.
+This package defines [a public trait](src/nft.rs) (Rust interface) for working with NFTs: the `UniqueAssets` trait.
 
 ## `UniqueAssets` Trait
 
-This trait is generic with respect to a type that implements the `NFT` trait; it defines the type abstractions and
-public functions needed to manage a set of unique assets.
+This trait is generic with respect to a type that is used to identify asset owners - the `AccountId` type. Assets with
+equivalent attributes (as defined by the `AssetInfo` type) **must** have equal `AssetId`s and assets with different
+`AssetId`s **must not** have equivalent attributes.
 
 ### Types
 
-- `AccountId`: the type used to identify asset owners
+- `AssetId`: a URI for an asset
+- `AssetInfo`: a set of attributes that uniquely describes an asset
 - `AssetLimit`: the maximum number of assets, expressed as an unsigned 128-bit integer, that may exist in this set at
   once
 - `UserAssetLimit`: the maximum number of assets, expressed as an unsigned 64-bit integer, that any single account may
@@ -40,15 +31,15 @@ public functions needed to manage a set of unique assets.
 - `burned() -> u128`: returns the total number of assets from this set that have been burned
 - `total_for_account(AccountId) -> u64`: returns the total number of asset from this set that are owned by a given
   account
-- `assets_for_account(AccountId) -> Vec<NFT>`: returns the list of assets from this set that are owned by a given
-  account
-- `owner_of(NFT::Id) -> AccountId`: returns the ID of the account that owns the given asset from this set
-- `mint(AccountId, NFT::Info) -> Result<AssetID, DispatchError>`: use the given attributes to create a new unique asset
+- `assets_for_account(AccountId) -> Vec<(AssetId, AssetInfo)>`: returns the list of assets from this set that are owned
+  by a given account
+- `owner_of(AssetId) -> AccountId`: returns the ID of the account that owns the given asset from this set
+- `mint(AccountId, AssetInfo) -> Result<AssetID, DispatchError>`: use the given attributes to create a new unique asset
   that belongs to this set and assign ownership of it to the given account
   - Failure cases: asset duplication, asset limit reached for set, asset limit for this set reached for account
-- `burn(NFT::Id) -> DispatchResult`: destroy the given asset
+- `burn(AssetId) -> DispatchResult`: destroy the given asset
   - Failure cases: asset doesn't exist
-- `transfer(AccountId, NFT::Id) -> DispatchResult`: transfer ownership of the given asset from this set from its current
+- `transfer(AccountId, AssetId) -> DispatchResult`: transfer ownership of the given asset from this set from its current
   owner to a given target account
   - Failure cases: asset doesn't exist, asset limit for this set reached for target account
 

src/lib.rs

@@ -36,25 +36,18 @@
 
 #![cfg_attr(not(feature = "std"), no_std)]
 
-use codec::{Decode, Encode, FullCodec};
+use codec::FullCodec;
 use frame_support::{
     decl_error, decl_event, decl_module, decl_storage, dispatch, ensure,
     traits::{EnsureOrigin, Get},
     Hashable,
 };
 use frame_system::ensure_signed;
-use sp_runtime::{
-    traits::{Hash, Member},
-    RuntimeDebug,
-};
-use sp_std::{
-    cmp::{Eq, Ordering},
-    fmt::Debug,
-    vec::Vec,
-};
+use sp_runtime::traits::{Hash, Member};
+use sp_std::{cmp::Eq, fmt::Debug, vec::Vec};
 
 pub mod nft;
-pub use crate::nft::{UniqueAssets, NFT};
+pub use crate::nft::UniqueAssets;
 
 #[cfg(test)]
 mod mock;
@@ -66,7 +59,7 @@ pub trait Trait<I = DefaultInstance>: frame_system::Trait {
     /// The dispatch origin that is able to mint new instances of this type of commodity.
     type CommodityAdmin: EnsureOrigin<Self::Origin>;
     /// The data type that is used to describe this type of commodity.
-    type CommodityInfo: Hashable + Member + Debug + Default + FullCodec;
+    type CommodityInfo: Hashable + Member + Debug + Default + FullCodec + Ord;
     /// The maximum number of this type of commodity that may exist (minted - burned).
     type CommodityLimit: Get<u128>;
     /// The maximum number of this type of commodity that any single account may own.
@@ -77,39 +70,8 @@ pub trait Trait<I = DefaultInstance>: frame_system::Trait {
 /// The runtime system's hashing algorithm is used to uniquely identify commodities.
 pub type CommodityId<T> = <T as frame_system::Trait>::Hash;
 
-/// A generic definition of an NFT that will be used by this pallet.
-#[derive(Encode, Decode, Clone, Eq, RuntimeDebug)]
-pub struct Commodity<Hash, CommodityInfo> {
-    pub id: Hash,
-    pub commodity: CommodityInfo,
-}
-
-/// An alias for this pallet's NFT implementation.
-pub type CommodityFor<T, I> = Commodity<CommodityId<T>, <T as Trait<I>>::CommodityInfo>;
-
-impl<CommodityId, CommodityInfo> NFT for Commodity<CommodityId, CommodityInfo> {
-    type Id = CommodityId;
-    type Info = CommodityInfo;
-}
-
-// Needed to maintain a sorted list.
-impl<CommodityId: Ord, CommodityInfo: Eq> Ord for Commodity<CommodityId, CommodityInfo> {
-    fn cmp(&self, other: &Self) -> Ordering {
-        self.id.cmp(&other.id)
-    }
-}
-
-impl<CommodityId: Ord, CommodityInfo> PartialOrd for Commodity<CommodityId, CommodityInfo> {
-    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
-        Some(self.id.cmp(&other.id))
-    }
-}
-
-impl<CommodityId: Eq, CommodityInfo> PartialEq for Commodity<CommodityId, CommodityInfo> {
-    fn eq(&self, other: &Self) -> bool {
-        self.id == other.id
-    }
-}
+/// Associates a commodity with its ID.
+pub type Commodity<T, I> = (CommodityId<T>, <T as Trait<I>>::CommodityInfo);
 
 decl_storage! {
     trait Store for Module<T: Trait<I>, I: Instance = DefaultInstance> as Commodity {
@@ -120,7 +82,7 @@ decl_storage! {
         /// The total number of this type of commodity owned by an account.
         TotalForAccount get(fn total_for_account): map hasher(blake2_128_concat) T::AccountId => u64 = 0;
         /// A mapping from an account to a list of all of the commodities of this type that are owned by it.
-        CommoditiesForAccount get(fn commodities_for_account): map hasher(blake2_128_concat) T::AccountId => Vec<CommodityFor<T, I>>;
+        CommoditiesForAccount get(fn commodities_for_account): map hasher(blake2_128_concat) T::AccountId => Vec<Commodity<T, I>>;
         /// A mapping from a commodity ID to the account that owns it.
         AccountForCommodity get(fn account_for_commodity): map hasher(identity) CommodityId<T> => T::AccountId;
     }
@@ -130,7 +92,7 @@ decl_storage! {
         build(|config: &GenesisConfig<T, I>| {
             for (who, assets) in config.balances.iter() {
                 for asset in assets {
-                    match <Module::<T, I> as UniqueAssets::<Commodity<CommodityId<T>, <T as Trait<I>>::CommodityInfo>>>::mint(who, asset.clone()) {
+                    match <Module::<T, I> as UniqueAssets::<T::AccountId>>::mint(who, asset.clone()) {
                         Ok(_) => {}
                         Err(err) => { panic!(err) },
                     }
@@ -237,10 +199,9 @@ decl_module! {
     }
 }
 
-impl<T: Trait<I>, I: Instance>
-    UniqueAssets<Commodity<CommodityId<T>, <T as Trait<I>>::CommodityInfo>> for Module<T, I>
-{
-    type AccountId = <T as frame_system::Trait>::AccountId;
+impl<T: Trait<I>, I: Instance> UniqueAssets<T::AccountId> for Module<T, I> {
+    type AssetId = CommodityId<T>;
+    type AssetInfo = T::CommodityInfo;
     type AssetLimit = T::CommodityLimit;
     type UserAssetLimit = T::UserCommodityLimit;
 
@@ -256,9 +217,7 @@ impl<T: Trait<I>, I: Instance>
         Self::total_for_account(account)
     }
 
-    fn assets_for_account(
-        account: &T::AccountId,
-    ) -> Vec<Commodity<CommodityId<T>, <T as Trait<I>>::CommodityInfo>> {
+    fn assets_for_account(account: &T::AccountId) -> Vec<Commodity<T, I>> {
         Self::commodities_for_account(account)
     }
 
@@ -287,10 +246,7 @@ impl<T: Trait<I>, I: Instance>
             Error::<T, I>::TooManyCommodities
         );
 
-        let new_commodity = Commodity {
-            id: commodity_id,
-            commodity: commodity_info,
-        };
+        let new_commodity = (commodity_id, commodity_info);
 
         Total::<I>::mutate(|total| *total += 1);
         TotalForAccount::<T, I>::mutate(owner_account, |total| *total += 1);
@@ -312,10 +268,7 @@ impl<T: Trait<I>, I: Instance>
             Error::<T, I>::NonexistentCommodity
         );
 
-        let burn_commodity = Commodity::<CommodityId<T>, <T as Trait<I>>::CommodityInfo> {
-            id: *commodity_id,
-            commodity: <T as Trait<I>>::CommodityInfo::default(),
-        };
+        let burn_commodity = (*commodity_id, <T as Trait<I>>::CommodityInfo::default());
 
         Total::<I>::mutate(|total| *total -= 1);
         Burned::<I>::mutate(|total| *total += 1);
@@ -346,10 +299,7 @@ impl<T: Trait<I>, I: Instance>
             Error::<T, I>::TooManyCommoditiesForAccount
         );
 
-        let xfer_commodity = Commodity::<CommodityId<T>, <T as Trait<I>>::CommodityInfo> {
-            id: *commodity_id,
-            commodity: <T as Trait<I>>::CommodityInfo::default(),
-        };
+        let xfer_commodity = (*commodity_id, <T as Trait<I>>::CommodityInfo::default());
 
         TotalForAccount::<T, I>::mutate(&owner, |total| *total -= 1);
         TotalForAccount::<T, I>::mutate(dest_account, |total| *total += 1);

src/nft.rs

@@ -17,19 +17,14 @@ use frame_support::{
 };
 use sp_std::vec::Vec;
 
-/// A unique asset; assets with equivalent attributes (as defined by the Info type) **must** have an
-/// equal ID and assets with different IDs **must not** have equivalent attributes.
-pub trait NFT {
+/// An interface over a set of unique assets.
+/// Assets with equivalent attributes (as defined by the AssetInfo type) **must** have an equal ID
+/// and assets with different IDs **must not** have equivalent attributes.
+pub trait UniqueAssets<AccountId> {
     /// The type used to identify unique assets.
-    type Id;
+    type AssetId;
     /// The attributes that distinguish unique assets.
-    type Info;
-}
-
-/// An interface over a set of unique assets.
-pub trait UniqueAssets<Asset: NFT> {
-    /// The type used to identify asset owners.
-    type AccountId;
+    type AssetInfo;
     /// The maximum number of this type of asset that may exist (minted - burned).
     type AssetLimit: Get<u128>;
     /// The maximum number of this type of asset that any single account may own.
@@ -40,28 +35,28 @@ pub trait UniqueAssets<Asset: NFT> {
     /// The total number of this type of asset that has been burned (may overflow).
     fn burned() -> u128;
     /// The total number of this type of asset owned by an account.
-    fn total_for_account(account: &Self::AccountId) -> u64;
+    fn total_for_account(account: &AccountId) -> u64;
     /// The set of unique assets owned by an account.
-    fn assets_for_account(account: &Self::AccountId) -> Vec<Asset>;
+    fn assets_for_account(account: &AccountId) -> Vec<(Self::AssetId, Self::AssetInfo)>;
     /// The ID of the account that owns an asset.
-    fn owner_of(asset_id: &Asset::Id) -> Self::AccountId;
+    fn owner_of(asset_id: &Self::AssetId) -> AccountId;
 
     /// Use the provided asset info to create a new unique asset for the specified user.
     /// This method **must** return an error in the following cases:
     /// - The asset, as identified by the asset info, already exists.
     /// - The specified owner account has already reached the user asset limit.
     /// - The total asset limit has already been reached.
     fn mint(
-        owner_account: &Self::AccountId,
-        asset_info: Asset::Info,
-    ) -> Result<Asset::Id, DispatchError>;
+        owner_account: &AccountId,
+        asset_info: Self::AssetInfo,
+    ) -> Result<Self::AssetId, DispatchError>;
     /// Destroy an asset.
     /// This method **must** return an error in the following case:
     /// - The asset with the specified ID does not exist.
-    fn burn(asset_id: &Asset::Id) -> DispatchResult;
+    fn burn(asset_id: &Self::AssetId) -> DispatchResult;
     /// Transfer ownership of an asset to another account.
     /// This method **must** return an error in the following cases:
     /// - The asset with the specified ID does not exist.
     /// - The destination account has already reached the user asset limit.
-    fn transfer(dest_account: &Self::AccountId, asset_id: &Asset::Id) -> DispatchResult;
+    fn transfer(dest_account: &AccountId, asset_id: &Self::AssetId) -> DispatchResult;
 }

src/tests.rs

@@ -29,10 +29,10 @@ fn mint() {
         let commodities_for_account = SUT::commodities_for_account::<u64>(1);
         assert_eq!(commodities_for_account.len(), 1);
         assert_eq!(
-            commodities_for_account[0].id,
+            commodities_for_account[0].0,
             Vec::<u8>::default().blake2_256().into()
         );
-        assert_eq!(commodities_for_account[0].commodity, Vec::<u8>::default());
+        assert_eq!(commodities_for_account[0].1, Vec::<u8>::default());
         assert_eq!(
             SUT::account_for_commodity::<H256>(Vec::<u8>::default().blake2_256().into()),
             1
@@ -151,10 +151,10 @@ fn transfer() {
         let commodities_for_account = SUT::commodities_for_account::<u64>(2);
         assert_eq!(commodities_for_account.len(), 1);
         assert_eq!(
-            commodities_for_account[0].id,
+            commodities_for_account[0].0,
             Vec::<u8>::default().blake2_256().into()
         );
-        assert_eq!(commodities_for_account[0].commodity, Vec::<u8>::default());
+        assert_eq!(commodities_for_account[0].1, Vec::<u8>::default());
         assert_eq!(
             SUT::account_for_commodity::<H256>(Vec::<u8>::default().blake2_256().into()),
             2