From fc0c96747adce5d1ff25530d1e7616b35c2ec2b5 Mon Sep 17 00:00:00 2001 From: Daniel Shiposha Date: Mon, 04 Jul 2022 10:08:48 +0000 Subject: [PATCH] refactor: use modify_token_properties --- --- a/pallets/nonfungible/src/common.rs +++ b/pallets/nonfungible/src/common.rs @@ -229,7 +229,7 @@ self, &sender, token_id, - properties, + properties.into_iter(), false, nesting_budget, ), @@ -251,7 +251,7 @@ self, &sender, token_id, - property_keys, + property_keys.into_iter(), nesting_budget, ), weight, --- a/pallets/nonfungible/src/erc.rs +++ b/pallets/nonfungible/src/erc.rs @@ -37,7 +37,7 @@ use crate::{ AccountBalance, Config, CreateItemData, NonfungibleHandle, Pallet, TokenData, TokensMinted, - SelfWeightOf, weights::WeightInfo, TokenProperties, property_guard::*, + SelfWeightOf, weights::WeightInfo, TokenProperties, }; #[solidity_interface(name = "TokenProperties")] @@ -82,21 +82,18 @@ .map_err(|_| "key too long")?; let value = value.try_into().map_err(|_| "value too long")?; - let is_token_create = false; let nesting_budget = self .recorder .weight_calls_budget(>::find_parent()); - let mut guard = PropertyGuard::new(PropertyGuardData { - sender: &caller, - collection: self, - token_id: TokenId(token_id), - is_token_create, - nesting_budget: &nesting_budget, - }); - - >::set_token_property(Property { key, value }, &mut guard) - .map_err(dispatch_to_evm::) + >::set_token_property( + self, + &caller, + TokenId(token_id), + Property { key, value }, + &nesting_budget, + ) + .map_err(dispatch_to_evm::) } fn delete_property(&mut self, token_id: uint256, caller: caller, key: string) -> Result<()> { @@ -106,20 +103,12 @@ .try_into() .map_err(|_| "key too long")?; - let is_token_create = false; let nesting_budget = self .recorder .weight_calls_budget(>::find_parent()); - let mut guard = PropertyGuard::new(PropertyGuardData { - sender: &caller, - collection: self, - token_id: TokenId(token_id), - is_token_create, - nesting_budget: &nesting_budget, - }); - - >::delete_token_property(key, &mut guard).map_err(dispatch_to_evm::) + >::delete_token_property(self, &caller, TokenId(token_id), key, &nesting_budget) + .map_err(dispatch_to_evm::) } /// Throws error if key not found --- a/pallets/nonfungible/src/lib.rs +++ b/pallets/nonfungible/src/lib.rs @@ -28,7 +28,8 @@ use up_data_structs::{ AccessMode, CollectionId, CustomDataLimit, TokenId, CreateCollectionData, CreateNftExData, mapping::TokenAddressMapping, budget::Budget, Property, PropertyPermission, PropertyKey, - PropertyKeyPermission, Properties, PropertyScope, TrySetProperty, TokenChild, AuxPropertyValue, + PropertyValue, PropertyKeyPermission, Properties, PropertyScope, TrySetProperty, TokenChild, + AuxPropertyValue, }; use pallet_evm::{account::CrossAccountId, Pallet as PalletEvm}; use pallet_common::{ @@ -51,10 +52,6 @@ pub mod common; pub mod erc; pub mod weights; - -mod property_guard; - -use property_guard::*; pub type CreateItemData = CreateNftExData<::CrossAccountId>; pub(crate) type SelfWeightOf = ::WeightInfo; @@ -89,6 +86,8 @@ NonfungibleItemsHaveNoAmount, /// Unable to burn NFT with children CantBurnNftWithChildren, + /// Unable to create an empty property + UnableToCreateEmptyProperty, } #[pallet::config] @@ -487,113 +486,152 @@ pays_fee: Pays::Yes, }) } - - pub fn set_token_property( - property: Property, - guard: &mut PropertyGuard<'_, T>, - ) -> DispatchResult { - Self::check_token_change_permission(&property.key, guard)?; - >::try_mutate((guard.collection.id, guard.token_id), |properties| { - let property = property.clone(); - properties.try_set(property.key, property.value) - }) - .map_err(>::from)?; - - >::deposit_event(CommonEvent::TokenPropertySet( - guard.collection.id, - guard.token_id, - property.key, - )); - - Ok(()) - } - #[transactional] - pub fn set_token_properties( + fn modify_token_properties( collection: &NonfungibleHandle, sender: &T::CrossAccountId, token_id: TokenId, - properties: Vec, + properties: impl Iterator)>, is_token_create: bool, nesting_budget: &dyn Budget, ) -> DispatchResult { - let mut guard = PropertyGuard::new(PropertyGuardData { - sender, - collection, - token_id, - is_token_create, - nesting_budget, - }); + let mut collection_admin_result = None; + let mut token_owner_result = None; - for property in properties { - Self::set_token_property(property, &mut guard)?; - } + let mut check_collection_admin = || { + *collection_admin_result + .get_or_insert_with(|| collection.check_is_owner_or_admin(sender)) + }; - Ok(()) - } + let mut check_token_owner = || { + *token_owner_result.get_or_insert_with(|| { + let is_owned = >::check_indirectly_owned( + sender.clone(), + collection.id, + token_id, + None, + nesting_budget, + )?; - pub fn delete_token_property( - property_key: PropertyKey, - guard: &mut PropertyGuard<'_, T>, - ) -> DispatchResult { - Self::check_token_change_permission(&property_key, guard)?; + if is_owned { + Ok(()) + } else { + Err(>::NoPermission.into()) + } + }) + }; - >::try_mutate((guard.collection.id, guard.token_id), |properties| { - properties.remove(&property_key) - }) - .map_err(>::from)?; + for (key, value) in properties { + let permission = >::property_permissions(collection.id) + .get(&key) + .cloned() + .unwrap_or_else(PropertyPermission::none); - >::deposit_event(CommonEvent::TokenPropertyDeleted( - guard.collection.id, - guard.token_id, - property_key, - )); + let is_property_exists = TokenProperties::::get((collection.id, token_id)) + .get(&key) + .is_some(); - Ok(()) - } + match permission { + PropertyPermission { mutable: false, .. } if is_property_exists => { + return Err(>::NoPermission.into()); + } - fn check_token_change_permission( - property_key: &PropertyKey, - guard: &mut PropertyGuard<'_, T>, - ) -> DispatchResult { - let permission = >::property_permissions(guard.collection.id) - .get(property_key) - .cloned() - .unwrap_or_else(PropertyPermission::none); + PropertyPermission { + collection_admin, + token_owner, + .. + } => { + //TODO: investigate threats during public minting. + if is_token_create && (collection_admin || token_owner) { + if value.is_some() { + return Ok(()); + } else { + return Err(>::UnableToCreateEmptyProperty.into()); + } + } - let is_property_exists = TokenProperties::::get((guard.collection.id, guard.token_id)) - .get(property_key) - .is_some(); + let mut check_result = Err(>::NoPermission.into()); - match permission { - PropertyPermission { mutable: false, .. } if is_property_exists => { - Err(>::NoPermission.into()) - } + if collection_admin { + check_result = check_collection_admin(); + } - PropertyPermission { - collection_admin, - token_owner, - .. - } => { - //TODO: investigate threats during public minting. - if guard.is_token_create && (collection_admin || token_owner) { - return Ok(()); + if token_owner { + check_result = check_result.or_else(|_| check_token_owner()) + } + + check_result?; } + } - let mut check_result = Err(>::NoPermission.into()); + match value { + Some(value) => { + >::try_mutate((collection.id, token_id), |properties| { + properties.try_set(key.clone(), value) + }) + .map_err(>::from)?; - if collection_admin { - check_result = guard.check_collection_admin(); + >::deposit_event(CommonEvent::TokenPropertySet( + collection.id, + token_id, + key, + )); } + None => { + >::try_mutate((collection.id, token_id), |properties| { + properties.remove(&key) + }) + .map_err(>::from)?; - if token_owner { - check_result.or_else(|_| guard.check_token_owner()) - } else { - check_result + >::deposit_event(CommonEvent::TokenPropertyDeleted( + collection.id, + token_id, + key, + )); } } } + + Ok(()) + } + + #[transactional] + pub fn set_token_properties( + collection: &NonfungibleHandle, + sender: &T::CrossAccountId, + token_id: TokenId, + properties: impl Iterator, + is_token_create: bool, + nesting_budget: &dyn Budget, + ) -> DispatchResult { + Self::modify_token_properties( + collection, + sender, + token_id, + properties.map(|p| (p.key, Some(p.value))), + is_token_create, + nesting_budget, + ) + } + + pub fn set_token_property( + collection: &NonfungibleHandle, + sender: &T::CrossAccountId, + token_id: TokenId, + property: Property, + nesting_budget: &dyn Budget, + ) -> DispatchResult { + let is_token_create = false; + + Self::set_token_properties( + collection, + sender, + token_id, + [property].into_iter(), + is_token_create, + nesting_budget, + ) } #[transactional] @@ -601,24 +639,35 @@ collection: &NonfungibleHandle, sender: &T::CrossAccountId, token_id: TokenId, - property_keys: Vec, + property_keys: impl Iterator, nesting_budget: &dyn Budget, ) -> DispatchResult { let is_token_create = false; - let mut guard = PropertyGuard::new(PropertyGuardData { + Self::modify_token_properties( + collection, sender, - collection, token_id, + property_keys.into_iter().map(|key| (key, None)), is_token_create, nesting_budget, - }); + ) + } - for key in property_keys { - Self::delete_token_property(key, &mut guard)?; - } - - Ok(()) + pub fn delete_token_property( + collection: &NonfungibleHandle, + sender: &T::CrossAccountId, + token_id: TokenId, + property_key: PropertyKey, + nesting_budget: &dyn Budget, + ) -> DispatchResult { + Self::delete_token_properties( + collection, + sender, + token_id, + [property_key].into_iter(), + nesting_budget, + ) } pub fn set_collection_properties( @@ -829,7 +878,7 @@ collection, sender, TokenId(token), - data.properties.clone().into_inner(), + data.properties.clone().into_iter(), true, nesting_budget, ) { --- a/pallets/nonfungible/src/property_guard.rs +++ /dev/null @@ -1,59 +0,0 @@ -use super::*; - -pub struct PropertyGuard<'a, T: Config> { - pub sender: &'a T::CrossAccountId, - pub collection: &'a NonfungibleHandle, - pub token_id: TokenId, - pub is_token_create: bool, - nesting_budget: &'a dyn Budget, - - collection_admin_result: Option, - token_owner_result: Option, -} - -pub struct PropertyGuardData<'a, T: Config> { - pub sender: &'a T::CrossAccountId, - pub collection: &'a NonfungibleHandle, - pub token_id: TokenId, - pub is_token_create: bool, - pub nesting_budget: &'a dyn Budget, -} - -impl<'a, T: Config> PropertyGuard<'a, T> { - pub fn new(data: PropertyGuardData<'a, T>) -> Self { - Self { - sender: data.sender, - collection: data.collection, - token_id: data.token_id, - is_token_create: data.is_token_create, - nesting_budget: data.nesting_budget, - - collection_admin_result: None, - token_owner_result: None, - } - } - - pub fn check_collection_admin(&mut self) -> DispatchResult { - *self - .collection_admin_result - .get_or_insert_with(|| self.collection.check_is_owner_or_admin(self.sender)) - } - - pub fn check_token_owner(&mut self) -> DispatchResult { - *self.token_owner_result.get_or_insert_with(|| { - let is_owned = >::check_indirectly_owned( - self.sender.clone(), - self.collection.id, - self.token_id, - None, - self.nesting_budget, - )?; - - if is_owned { - Ok(()) - } else { - Err(>::NoPermission.into()) - } - }) - } -} -- gitstuff