From 0ba1adb79fd8521272e2daf9d143d678951a47cb Mon Sep 17 00:00:00 2001 From: Trubnikov Sergey Date: Wed, 01 Feb 2023 07:41:44 +0000 Subject: [PATCH] misk: generalize 2 methods --- --- a/pallets/common/src/lib.rs +++ b/pallets/common/src/lib.rs @@ -66,10 +66,11 @@ ensure, traits::{Imbalance, Get, Currency, WithdrawReasons, ExistenceRequirement}, dispatch::Pays, - transactional, + transactional, fail, }; use pallet_evm::GasWeightMapping; use up_data_structs::{ + AccessMode, COLLECTION_NUMBER_LIMIT, Collection, RpcCollection, @@ -124,6 +125,8 @@ pub use pallet::*; use sp_core::H160; use sp_runtime::{ArithmeticError, DispatchError, DispatchResult}; + +use crate::erc::CollectionHelpersEvents; #[cfg(feature = "runtime-benchmarks")] pub mod benchmarking; pub mod dispatch; @@ -1264,6 +1267,128 @@ Ok(()) } + /// A batch operation to add, edit or remove properties for a token. + /// It sets or removes a token's properties according to + /// `properties_updates` contents: + /// * sets a property under the with the value provided `(, Some())` + /// * removes a property under the if the value is `None` `(, None)`. + /// + /// - `nesting_budget`: Limit for searching parents in-depth to check ownership. + /// - `is_token_create`: Indicates that method is called during token initialization. + /// Allows to bypass ownership check. + /// + /// All affected properties should have `mutable` permission + /// to be **deleted** or to be **set more than once**, + /// and the sender should have permission to edit those properties. + /// + /// This function fires an event for each property change. + /// In case of an error, all the changes (including the events) will be reverted + /// since the function is transactional. + pub fn modify_token_properties( + collection: &CollectionHandle, + sender: &T::CrossAccountId, + token_id: TokenId, + properties_updates: impl Iterator)>, + is_token_create: bool, + mut stored_properties: Properties, + is_token_owner: impl Fn() -> Result, + set_token_properties: impl FnOnce(Properties), + ) -> DispatchResult { + let is_collection_admin = collection.is_owner_or_admin(sender); + let permissions = Self::property_permissions(collection.id); + + for (key, value) in properties_updates { + let permission = permissions + .get(&key) + .cloned() + .unwrap_or_else(PropertyPermission::none); + + let is_property_exists = stored_properties.get(&key).is_some(); + + match permission { + PropertyPermission { mutable: false, .. } if is_property_exists => { + return Err(>::NoPermission.into()); + } + + PropertyPermission { + collection_admin, + token_owner, + .. + } => { + //TODO: investigate threats during public minting. + let is_token_create = + is_token_create && (collection_admin || token_owner) && value.is_some(); + if !(is_token_create + || (collection_admin && is_collection_admin) + || (token_owner && is_token_owner()?)) + { + fail!(>::NoPermission); + } + } + } + + match value { + Some(value) => { + stored_properties + .try_set(key.clone(), value) + .map_err(>::from)?; + + Self::deposit_event(Event::TokenPropertySet(collection.id, token_id, key)); + } + None => { + stored_properties.remove(&key).map_err(>::from)?; + + Self::deposit_event(Event::TokenPropertyDeleted(collection.id, token_id, key)); + } + } + + >::deposit_log( + CollectionHelpersEvents::TokenChanged { + collection_id: eth::collection_id_to_address(collection.id), + token_id: token_id.into(), + } + .to_log(T::ContractAddress::get()), + ); + } + + set_token_properties(stored_properties); + + Ok(()) + } + + /// Sets or unsets the approval of a given operator. + /// + /// The `operator` is allowed to transfer all token pieces of the `owner` on their behalf. + /// - `owner`: Token owner + /// - `operator`: Operator + /// - `approve`: Should operator status be granted or revoked? + pub fn set_allowance_for_all( + collection: &CollectionHandle, + owner: &T::CrossAccountId, + operator: &T::CrossAccountId, + approve: bool, + set_allowance: impl FnOnce(), + log: evm_coder::ethereum::Log, + ) -> DispatchResult { + if collection.permissions.access() == AccessMode::AllowList { + collection.check_allowlist(owner)?; + collection.check_allowlist(operator)?; + } + + Self::ensure_correct_receiver(operator)?; + + set_allowance(); + + >::deposit_log(log); + Self::deposit_event(Event::ApprovedForAll( + collection.id, + owner.clone(), + operator.clone(), + approve, + )); + Ok(()) + } + /// Set collection property. /// /// * `collection` - Collection handler. --- a/pallets/nonfungible/src/lib.rs +++ b/pallets/nonfungible/src/lib.rs @@ -101,18 +101,18 @@ }; use up_data_structs::{ AccessMode, CollectionId, CollectionFlags, CustomDataLimit, TokenId, CreateCollectionData, - CreateNftExData, mapping::TokenAddressMapping, budget::Budget, Property, PropertyPermission, - PropertyKey, PropertyValue, PropertyKeyPermission, Properties, PropertyScope, TrySetProperty, - TokenChild, AuxPropertyValue, PropertiesPermissionMap, + CreateNftExData, mapping::TokenAddressMapping, budget::Budget, Property, PropertyKey, + PropertyValue, PropertyKeyPermission, Properties, PropertyScope, TrySetProperty, TokenChild, + AuxPropertyValue, PropertiesPermissionMap, }; use pallet_evm::{account::CrossAccountId, Pallet as PalletEvm}; use pallet_common::{ Error as CommonError, Pallet as PalletCommon, Event as CommonEvent, CollectionHandle, - eth::collection_id_to_address, erc::CollectionHelpersEvents, + eth::collection_id_to_address, }; use pallet_structure::{Pallet as PalletStructure, Error as StructureError}; use pallet_evm_coder_substrate::{SubstrateRecorder, WithRecorder}; -use sp_core::{H160, Get}; +use sp_core::H160; use sp_runtime::{ArithmeticError, DispatchError, DispatchResult, TransactionOutcome}; use sp_std::{vec::Vec, vec, collections::btree_map::BTreeMap}; use core::ops::Deref; @@ -578,10 +578,6 @@ } /// A batch operation to add, edit or remove properties for a token. - /// It sets or removes a token's properties according to - /// `properties_updates` contents: - /// * sets a property under the with the value provided `(, Some())` - /// * removes a property under the if the value is `None` `(, None)`. /// /// - `nesting_budget`: Limit for searching parents in-depth to check ownership. /// - `is_token_create`: Indicates that method is called during token initialization. @@ -603,97 +599,30 @@ is_token_create: bool, nesting_budget: &dyn Budget, ) -> DispatchResult { - let mut collection_admin_status = None; - let mut token_owner_result = None; - - let mut is_collection_admin = - || *collection_admin_status.get_or_insert_with(|| collection.is_owner_or_admin(sender)); + let is_token_owner = || { + let is_owned = >::check_indirectly_owned( + sender.clone(), + collection.id, + token_id, + None, + nesting_budget, + )?; - let mut is_token_owner = || { - *token_owner_result.get_or_insert_with(|| -> Result { - let is_owned = >::check_indirectly_owned( - sender.clone(), - collection.id, - token_id, - None, - nesting_budget, - )?; - - Ok(is_owned) - }) + Ok(is_owned) }; - - let mut stored_properties = >::get((collection.id, token_id)); - let permissions = >::property_permissions(collection.id); - - for (key, value) in properties_updates { - let permission = permissions - .get(&key) - .cloned() - .unwrap_or_else(PropertyPermission::none); - - let is_property_exists = stored_properties.get(&key).is_some(); - - match permission { - PropertyPermission { mutable: false, .. } if is_property_exists => { - return Err(>::NoPermission.into()); - } - - PropertyPermission { - collection_admin, - token_owner, - .. - } => { - //TODO: investigate threats during public minting. - if is_token_create && (collection_admin || token_owner) && value.is_some() { - // Pass - } else if collection_admin && is_collection_admin() { - // Pass - } else if token_owner && is_token_owner()? { - // Pass - } else { - fail!(>::NoPermission); - } - } - } - match value { - Some(value) => { - stored_properties - .try_set(key.clone(), value) - .map_err(>::from)?; - - >::deposit_event(CommonEvent::TokenPropertySet( - collection.id, - token_id, - key, - )); - } - None => { - stored_properties - .remove(&key) - .map_err(>::from)?; - - >::deposit_event(CommonEvent::TokenPropertyDeleted( - collection.id, - token_id, - key, - )); - } - } - - >::deposit_log( - CollectionHelpersEvents::TokenChanged { - collection_id: collection_id_to_address(collection.id), - token_id: token_id.into(), - } - .to_log(T::ContractAddress::get()), - ); - } - - >::set((collection.id, token_id), stored_properties); + let stored_properties = >::get((collection.id, token_id)); - Ok(()) + >::modify_token_properties( + collection, + sender, + token_id, + properties_updates, + is_token_create, + stored_properties, + is_token_owner, + |properties| >::set((collection.id, token_id), properties), + ) } /// Batch operation to add or edit properties for the token @@ -1418,31 +1347,19 @@ operator: &T::CrossAccountId, approve: bool, ) -> DispatchResult { - if collection.permissions.access() == AccessMode::AllowList { - collection.check_allowlist(owner)?; - collection.check_allowlist(operator)?; - } - - >::ensure_correct_receiver(operator)?; - - // ========= - - >::insert((collection.id, owner, operator), approve); - >::deposit_log( + >::set_allowance_for_all( + collection, + owner, + operator, + approve, + || >::insert((collection.id, owner, operator), approve), ERC721Events::ApprovalForAll { owner: *owner.as_eth(), operator: *operator.as_eth(), approved: approve, } .to_log(collection_id_to_address(collection.id)), - ); - >::deposit_event(CommonEvent::ApprovedForAll( - collection.id, - owner.clone(), - operator.clone(), - approve, - )); - Ok(()) + ) } /// Tells whether the given `owner` approves the `operator`. --- a/pallets/refungible/src/lib.rs +++ b/pallets/refungible/src/lib.rs @@ -92,22 +92,22 @@ use core::ops::Deref; use evm_coder::ToLog; -use frame_support::{ensure, fail, storage::with_transaction, transactional}; +use frame_support::{ensure, storage::with_transaction, transactional}; use pallet_evm::{account::CrossAccountId, Pallet as PalletEvm}; use pallet_evm_coder_substrate::WithRecorder; use pallet_common::{ CommonCollectionOperations, Error as CommonError, eth::collection_id_to_address, - Event as CommonEvent, Pallet as PalletCommon, erc::CollectionHelpersEvents, + Event as CommonEvent, Pallet as PalletCommon, }; use pallet_structure::Pallet as PalletStructure; -use sp_core::{Get, H160}; +use sp_core::H160; use sp_runtime::{ArithmeticError, DispatchError, DispatchResult, TransactionOutcome}; use sp_std::{vec::Vec, vec, collections::btree_map::BTreeMap}; use up_data_structs::{ AccessMode, budget::Budget, CollectionId, CollectionFlags, CreateCollectionData, mapping::TokenAddressMapping, MAX_REFUNGIBLE_PIECES, Property, PropertyKey, - PropertyKeyPermission, PropertyPermission, PropertyScope, PropertyValue, TokenId, - TrySetProperty, PropertiesPermissionMap, CreateRefungibleExMultipleOwners, TokenOwnerError, + PropertyKeyPermission, PropertyScope, PropertyValue, TokenId, TrySetProperty, + PropertiesPermissionMap, CreateRefungibleExMultipleOwners, TokenOwnerError, }; pub use pallet::*; @@ -245,8 +245,8 @@ pub type CollectionAllowance = StorageNMap< Key = ( Key, - Key, - Key, + Key, // Owner + Key, // Operator ), Value = bool, QueryKind = ValueQuery, @@ -541,7 +541,6 @@ is_token_create: bool, nesting_budget: &dyn Budget, ) -> DispatchResult { - let is_collection_admin = || collection.is_owner_or_admin(sender); let is_token_owner = || -> Result { let balance = collection.balance(sender.clone(), token_id); let total_pieces: u128 = @@ -560,77 +559,19 @@ Ok(is_bundle_owner) }; - - let mut stored_properties = >::get((collection.id, token_id)); - let permissions = >::property_permissions(collection.id); - - for (key, value) in properties_updates { - let permission = permissions - .get(&key) - .cloned() - .unwrap_or_else(PropertyPermission::none); - - let is_property_exists = stored_properties.get(&key).is_some(); - - match permission { - PropertyPermission { mutable: false, .. } if is_property_exists => { - return Err(>::NoPermission.into()); - } - PropertyPermission { - collection_admin, - token_owner, - .. - } => { - //TODO: investigate threats during public minting. - let is_token_create = - is_token_create && (collection_admin || token_owner) && value.is_some(); - if !(is_token_create - || (collection_admin && is_collection_admin()) - || (token_owner && is_token_owner()?)) - { - fail!(>::NoPermission); - } - } - } - - match value { - Some(value) => { - stored_properties - .try_set(key.clone(), value) - .map_err(>::from)?; - - >::deposit_event(CommonEvent::TokenPropertySet( - collection.id, - token_id, - key, - )); - } - None => { - stored_properties - .remove(&key) - .map_err(>::from)?; + let stored_properties = >::get((collection.id, token_id)); - >::deposit_event(CommonEvent::TokenPropertyDeleted( - collection.id, - token_id, - key, - )); - } - } - - >::deposit_log( - CollectionHelpersEvents::TokenChanged { - collection_id: collection_id_to_address(collection.id), - token_id: token_id.into(), - } - .to_log(T::ContractAddress::get()), - ); - } - - >::set((collection.id, token_id), stored_properties); - - Ok(()) + >::modify_token_properties( + collection, + sender, + token_id, + properties_updates, + is_token_create, + stored_properties, + is_token_owner, + |properties| >::set((collection.id, token_id), properties), + ) } pub fn set_token_properties( @@ -1465,31 +1406,19 @@ operator: &T::CrossAccountId, approve: bool, ) -> DispatchResult { - if collection.permissions.access() == AccessMode::AllowList { - collection.check_allowlist(owner)?; - collection.check_allowlist(operator)?; - } - - >::ensure_correct_receiver(operator)?; - - // ========= - - >::insert((collection.id, owner, operator), approve); - >::deposit_log( + >::set_allowance_for_all( + collection, + owner, + operator, + approve, + || >::insert((collection.id, owner, operator), approve), ERC721Events::ApprovalForAll { owner: *owner.as_eth(), operator: *operator.as_eth(), approved: approve, } .to_log(collection_id_to_address(collection.id)), - ); - >::deposit_event(CommonEvent::ApprovedForAll( - collection.id, - owner.clone(), - operator.clone(), - approve, - )); - Ok(()) + ) } /// Tells whether the given `owner` approves the `operator`. -- gitstuff