Refactor UniqueAssets Trait

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

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 <[email protected]>']
 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