From 6a8b3604350874bf35ce2183a231d5841e76a4d4 Mon Sep 17 00:00:00 2001 From: Yaroslav Bolyukin Date: Mon, 22 Mar 2021 14:07:23 +0000 Subject: [PATCH] refactor: optional collections related review --- --- a/pallets/nft/src/lib.rs +++ b/pallets/nft/src/lib.rs @@ -199,7 +199,7 @@ } } -#[derive(Encode, Decode, Default, Debug, Clone, PartialEq)] +#[derive(Encode, Decode, Debug, Clone, PartialEq)] #[cfg_attr(feature = "std", derive(Serialize, Deserialize))] pub struct NftItemType { pub owner: AccountId, @@ -213,7 +213,7 @@ pub value: u128, } -#[derive(Encode, Decode, Default, Debug, Clone, PartialEq)] +#[derive(Encode, Decode, Debug, Clone, PartialEq)] #[cfg_attr(feature = "std", derive(Serialize, Deserialize))] pub struct ReFungibleItemType { pub owner: Vec>, @@ -537,15 +537,16 @@ /// Amount of items which spender can transfer out of owners account (via transferFrom) /// Collection id (controlled?2), (token id (controlled ?2) + owner account id (real) + spender account id (controlled?3)) + /// TODO: Off chain worker should remove from this map when token gets removed pub Allowances get(fn approved): double_map hasher(blake2_128_concat) CollectionId, hasher(blake2_128_concat) (TokenId, T::AccountId, T::AccountId) => u128; //#region Item collections /// Collection id (controlled?2), token id (controlled?1) - pub NftItemList get(fn nft_item_id) config(): double_map hasher(blake2_128_concat) CollectionId, hasher(blake2_128_concat) TokenId => NftItemType; + pub NftItemList get(fn nft_item_id) config(): double_map hasher(blake2_128_concat) CollectionId, hasher(blake2_128_concat) TokenId => Option>; /// Collection id (controlled?2), owner (controlled?2) pub FungibleItemList get(fn fungible_item_id) config(): double_map hasher(blake2_128_concat) CollectionId, hasher(blake2_128_concat) T::AccountId => FungibleItemType; /// Collection id (controlled?2), token id (controlled?1) - pub ReFungibleItemList get(fn refungible_item_id) config(): double_map hasher(blake2_128_concat) CollectionId, hasher(blake2_128_concat) TokenId => ReFungibleItemType; + pub ReFungibleItemList get(fn refungible_item_id) config(): double_map hasher(blake2_128_concat) CollectionId, hasher(blake2_128_concat) TokenId => Option>; //#endregion //#region Index list @@ -555,6 +556,7 @@ //#region Tokens transfer rate limit baskets /// (Collection id (controlled?2), who created (real)) + /// TODO: Off chain worker should remove from this map when collection gets removed pub CreateItemBasket get(fn create_item_basket): map hasher(blake2_128_concat) (CollectionId, T::AccountId) => T::BlockNumber; /// Collection id (controlled?2), token id (controlled?2) pub NftTransferBasket get(fn nft_transfer_basket): double_map hasher(blake2_128_concat) CollectionId, hasher(blake2_128_concat) TokenId => T::BlockNumber; @@ -570,7 +572,7 @@ //#region Contract Sponsorship and Ownership /// Contract address (real) - pub ContractOwner get(fn contract_owner): map hasher(twox_64_concat) T::AccountId => T::AccountId; + pub ContractOwner get(fn contract_owner): map hasher(twox_64_concat) T::AccountId => Option; /// Contract address (real) pub ContractSelfSponsoring get(fn contract_self_sponsoring): map hasher(twox_64_concat) T::AccountId => bool; /// (Contract address(real), caller (real)) @@ -968,20 +970,17 @@ let sender = ensure_signed(origin)?; let collection = Self::get_collection(collection_id)?; Self::check_owner_or_admin_permissions(&collection, sender)?; - let mut admin_arr: Vec = Vec::new(); + let mut admin_arr = >::get(collection_id); - if >::contains_key(collection_id) - { - admin_arr = >::get(collection_id); - ensure!(!admin_arr.contains(&new_admin_id), Error::::AlreadyAdmin); + match admin_arr.binary_search(&new_admin_id) { + Ok(_) => {}, + Err(idx) => { + let limits = ChainLimit::get(); + ensure!(admin_arr.len() < limits.collections_admins_limit as usize, Error::::CollectionAdminsLimitExceeded); + admin_arr.insert(idx, new_admin_id); + >::insert(collection_id, admin_arr); + } } - - // Number of collection admins - ensure!((admin_arr.len() as u64) < ChainLimit::get().collections_admins_limit, Error::::CollectionAdminsLimitExceeded); - - admin_arr.push(new_admin_id); - >::insert(collection_id, admin_arr); - Ok(()) } @@ -1004,12 +1003,15 @@ let sender = ensure_signed(origin)?; let collection = Self::get_collection(collection_id)?; Self::check_owner_or_admin_permissions(&collection, sender)?; - ensure!(>::contains_key(collection_id), Error::::AdminNotFound); - let mut admin_arr = >::get(collection_id); - admin_arr.retain(|i| *i != account_id); - >::insert(collection_id, admin_arr); + match admin_arr.binary_search(&account_id) { + Ok(idx) => { + admin_arr.remove(idx); + >::insert(collection_id, admin_arr); + }, + Err(_) => {} + } Ok(()) } @@ -1267,7 +1269,7 @@ let sender = ensure_signed(origin)?; let target_collection = Self::get_collection(collection_id)?; - Self::token_exists(&target_collection, item_id, &sender)?; + Self::token_exists(&target_collection, item_id)?; // Transfer permissions check let bypasses_limits = target_collection.limits.owner_can_transfer && @@ -1419,7 +1421,7 @@ let sender = ensure_signed(origin)?; let target_collection = Self::get_collection(collection_id)?; - Self::token_exists(&target_collection, item_id, &sender)?; + Self::token_exists(&target_collection, item_id)?; ensure!(ChainLimit::get().custom_data_limit >= data.len() as u32, Error::::TokenVariableDataLimitExceeded); @@ -1668,7 +1670,11 @@ >::insert(contract_address.clone(), sender.clone()); Self::ensure_contract_owned(sender, &contract_address)?; - >::insert(contract_address, enable); + if enable { + >::insert(contract_address, true); + } else { + >::remove(contract_address); + } Ok(()) } @@ -1901,14 +1907,11 @@ let collection_id = collection.id; // Does new owner already have an account? - let mut balance: u128 = 0; - if >::contains_key(collection_id, owner) { - balance = >::get(collection_id, owner).value; - } + let balance: u128 = >::get(collection_id, owner).value; // Mint let item = FungibleItemType { - value: balance + value + value: balance.checked_add(value).ok_or(Error::::NumOverflow)?, }; >::insert(collection_id, (*owner).clone(), item); @@ -1984,11 +1987,8 @@ ) -> DispatchResult { let collection_id = collection.id; - ensure!( - >::contains_key(collection_id, item_id), - Error::::TokenNotFound - ); - let mut token = >::get(collection_id, item_id); + let mut token = >::get(collection_id, item_id) + .ok_or(Error::::TokenNotFound)?; let rft_balance = token .owner .iter() @@ -2026,11 +2026,8 @@ fn burn_nft_item(collection: &CollectionHandle, item_id: TokenId) -> DispatchResult { let collection_id = collection.id; - ensure!( - >::contains_key(collection_id, item_id), - Error::::TokenNotFound - ); - let item = >::get(collection_id, item_id); + let item = >::get(collection_id, item_id) + .ok_or(Error::::TokenNotFound)?; Self::remove_token_index(collection_id, item_id, &item.owner)?; // update balance @@ -2047,10 +2044,6 @@ fn burn_fungible_item(owner: &T::AccountId, collection: &CollectionHandle, value: u128) -> DispatchResult { let collection_id = collection.id; - ensure!( - >::contains_key(collection_id, owner), - Error::::TokenNotFound - ); let mut balance = >::get(collection_id, owner); ensure!(balance.value >= value, Error::::TokenValueNotEnough); @@ -2094,28 +2087,15 @@ } fn is_owner_or_admin_permissions(collection: &CollectionHandle, subject: T::AccountId) -> bool { - let mut result: bool = subject == collection.owner; - let exists = >::contains_key(collection.id); - - if !result & exists { - if >::get(collection.id).contains(&subject) { - result = true - } - } - - result + subject == collection.owner || >::get(collection.id).contains(&subject) } fn check_owner_or_admin_permissions( collection: &CollectionHandle, subject: T::AccountId, ) -> DispatchResult { - let result = Self::is_owner_or_admin_permissions(collection, subject.clone()); + ensure!(Self::is_owner_or_admin_permissions(collection, subject), Error::::NoPermission); - ensure!( - result, - Error::::NoPermission - ); Ok(()) } @@ -2127,20 +2107,11 @@ let collection_id = target_collection.id; match target_collection.mode { - CollectionMode::NFT => { - if >::get(collection_id, item_id).owner == subject { - return Some(1) - } - None - }, - CollectionMode::Fungible(_) => { - if >::contains_key(collection_id, &subject) { - return Some(>::get(collection_id, &subject) - .value); - } - None - }, - CollectionMode::ReFungible => >::get(collection_id, item_id) + CollectionMode::NFT => (>::get(collection_id, item_id)?.owner == subject) + .then(|| 1), + CollectionMode::Fungible(_) => Some(>::get(collection_id, &subject) + .value), + CollectionMode::ReFungible => >::get(collection_id, item_id)? .owner .iter() .find(|i| i.owner == subject) @@ -2150,22 +2121,9 @@ } fn is_item_owner(subject: T::AccountId, target_collection: &CollectionHandle, item_id: TokenId) -> bool { - let collection_id = target_collection.id; - match target_collection.mode { - CollectionMode::NFT => { - >::get(collection_id, item_id).owner == subject - } - CollectionMode::Fungible(_) => { - >::contains_key(collection_id, &subject) - } - CollectionMode::ReFungible => { - >::get(collection_id, item_id) - .owner - .iter() - .any(|i| i.owner == subject) - } - CollectionMode::Invalid => false, + CollectionMode::Fungible(_) => true, + _ => Self::owned_amount(subject, target_collection, item_id).is_some(), } } @@ -2183,13 +2141,12 @@ fn token_exists( target_collection: &CollectionHandle, item_id: TokenId, - owner: &T::AccountId ) -> DispatchResult { let collection_id = target_collection.id; let exists = match target_collection.mode { CollectionMode::NFT => >::contains_key(collection_id, item_id), - CollectionMode::Fungible(_) => >::contains_key(collection_id, owner), + CollectionMode::Fungible(_) => true, CollectionMode::ReFungible => >::contains_key(collection_id, item_id), _ => false }; @@ -2205,7 +2162,6 @@ recipient: &T::AccountId, ) -> DispatchResult { let collection_id = collection.id; - Self::token_exists(&collection, 0, owner)?; let mut balance = >::get(collection_id, owner); ensure!(balance.value >= value, Error::::TokenValueTooLow); @@ -2236,9 +2192,9 @@ new_owner: T::AccountId, ) -> DispatchResult { let collection_id = collection.id; - Self::token_exists(collection, item_id, &owner)?; + let full_item = >::get(collection_id, item_id) + .ok_or(Error::::TokenNotFound)?; - let full_item = >::get(collection_id, item_id); let item = full_item .owner .iter() @@ -2318,10 +2274,9 @@ new_owner: T::AccountId, ) -> DispatchResult { let collection_id = collection.id; - Self::token_exists(&collection, item_id, &sender)?; + let mut item = >::get(collection_id, item_id) + .ok_or(Error::::TokenNotFound)?; - let mut item = >::get(collection_id, item_id); - ensure!( sender == item.owner, Error::::MustBeTokenOwner @@ -2355,7 +2310,8 @@ data: Vec ) -> DispatchResult { let collection_id = collection.id; - let mut item = >::get(collection_id, item_id); + let mut item = >::get(collection_id, item_id) + .ok_or(Error::::TokenNotFound)?; item.variable_data = data; @@ -2370,7 +2326,8 @@ data: Vec ) -> DispatchResult { let collection_id = collection.id; - let mut item = >::get(collection_id, item_id); + let mut item = >::get(collection_id, item_id) + .ok_or(Error::::TokenNotFound)?; item.variable_data = data; @@ -2533,12 +2490,7 @@ } fn ensure_contract_owned(account: T::AccountId, contract: &T::AccountId) -> DispatchResult { - if >::contains_key(contract.clone()) { - let owner = >::get(contract); - ensure!(account == owner, Error::::NoPermission); - } else { - fail!(Error::::NoPermission); - } + ensure!(>::get(contract) == Some(account), Error::::NoPermission); Ok(()) } @@ -2787,9 +2739,8 @@ let called_contract: T::AccountId = T::Lookup::lookup((*dest).clone()).unwrap_or(T::AccountId::default()); - let owned_contract = >::contains_key(called_contract.clone()) - && >::get(called_contract.clone()) == *who; - let white_list_enabled = >::contains_key(called_contract.clone()) && >::get(called_contract.clone()); + let owned_contract = >::get(called_contract.clone()).as_ref() == Some(who); + let white_list_enabled = >::contains_key(called_contract.clone()); if !owned_contract && white_list_enabled { if !>::contains_key(called_contract.clone(), who) { -- gitstuff