--- a/pallets/common/src/benchmarking.rs +++ b/pallets/common/src/benchmarking.rs @@ -176,7 +176,7 @@ key: property_key(p as usize), value: property_value(), }).collect::>(); - }: {>::set_collection_properties(&collection, &owner, props)?} + }: {>::set_collection_properties(&collection, &owner, props.into_iter())?} delete_collection_properties { let b in 0..MAX_PROPERTIES_PER_ITEM; @@ -188,7 +188,7 @@ key: property_key(p as usize), value: property_value(), }).collect::>(); - >::set_collection_properties(&collection, &owner, props)?; + >::set_collection_properties(&collection, &owner, props.into_iter())?; let to_delete = (0..b).map(|p| property_key(p as usize)).collect::>(); - }: {>::delete_collection_properties(&collection, &owner, to_delete)?} + }: {>::delete_collection_properties(&collection, &owner, to_delete.into_iter())?} } --- a/pallets/common/src/erc.rs +++ b/pallets/common/src/erc.rs @@ -125,7 +125,7 @@ .map(eth::Property::try_into) .collect::>>()?; - >::set_collection_properties(self, &caller, properties) + >::set_collection_properties(self, &caller, properties.into_iter()) .map_err(dispatch_to_evm::) } @@ -158,7 +158,8 @@ }) .collect::>>()?; - >::delete_collection_properties(self, &caller, keys).map_err(dispatch_to_evm::) + >::delete_collection_properties(self, &caller, keys.into_iter()) + .map_err(dispatch_to_evm::) } /// Get collection property. --- a/pallets/common/src/lib.rs +++ b/pallets/common/src/lib.rs @@ -1198,6 +1198,58 @@ Ok(()) } + /// This function sets or removes a collection 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)`. + /// + /// 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. + #[transactional] + fn modify_collection_properties( + collection: &CollectionHandle, + sender: &T::CrossAccountId, + properties_updates: impl Iterator)>, + ) -> DispatchResult { + collection.check_is_owner_or_admin(sender)?; + + let mut stored_properties = >::get(collection.id); + + for (key, value) in properties_updates { + match value { + Some(value) => { + stored_properties + .try_set(key.clone(), value) + .map_err(>::from)?; + + Self::deposit_event(Event::CollectionPropertySet(collection.id, key)); + >::deposit_log( + erc::CollectionHelpersEvents::CollectionChanged { + collection_id: eth::collection_id_to_address(collection.id), + } + .to_log(T::ContractAddress::get()), + ); + } + None => { + stored_properties.remove(&key).map_err(>::from)?; + + Self::deposit_event(Event::CollectionPropertyDeleted(collection.id, key)); + >::deposit_log( + erc::CollectionHelpersEvents::CollectionChanged { + collection_id: eth::collection_id_to_address(collection.id), + } + .to_log(T::ContractAddress::get()), + ); + } + } + } + + >::set(collection.id, stored_properties); + + Ok(()) + } + /// Set collection property. /// /// * `collection` - Collection handler. @@ -1208,23 +1260,7 @@ sender: &T::CrossAccountId, property: Property, ) -> DispatchResult { - collection.check_is_owner_or_admin(sender)?; - - CollectionProperties::::try_mutate(collection.id, |properties| { - let property = property.clone(); - properties.try_set(property.key, property.value) - }) - .map_err(>::from)?; - - Self::deposit_event(Event::CollectionPropertySet(collection.id, property.key)); - >::deposit_log( - erc::CollectionHelpersEvents::CollectionChanged { - collection_id: eth::collection_id_to_address(collection.id), - } - .to_log(T::ContractAddress::get()), - ); - - Ok(()) + Self::set_collection_properties(collection, sender, [property].into_iter()) } /// Set a scoped collection property, where the scope is a special prefix @@ -1270,17 +1306,16 @@ /// * `collection` - Collection handler. /// * `sender` - The owner or administrator of the collection. /// * `properties` - The properties to set. - #[transactional] pub fn set_collection_properties( collection: &CollectionHandle, sender: &T::CrossAccountId, - properties: Vec, + properties: impl Iterator, ) -> DispatchResult { - for property in properties { - Self::set_collection_property(collection, sender, property)?; - } - - Ok(()) + Self::modify_collection_properties( + collection, + sender, + properties.map(|property| (property.key, Some(property.value))), + ) } /// Delete collection property. @@ -1293,25 +1328,7 @@ sender: &T::CrossAccountId, property_key: PropertyKey, ) -> DispatchResult { - collection.check_is_owner_or_admin(sender)?; - - CollectionProperties::::try_mutate(collection.id, |properties| { - properties.remove(&property_key) - }) - .map_err(>::from)?; - - Self::deposit_event(Event::CollectionPropertyDeleted( - collection.id, - property_key, - )); - >::deposit_log( - erc::CollectionHelpersEvents::CollectionChanged { - collection_id: eth::collection_id_to_address(collection.id), - } - .to_log(T::ContractAddress::get()), - ); - - Ok(()) + Self::delete_collection_properties(collection, sender, [property_key].into_iter()) } /// Delete collection properties. @@ -1319,17 +1336,12 @@ /// * `collection` - Collection handler. /// * `sender` - The owner or administrator of the collection. /// * `properties` - The properties to delete. - #[transactional] pub fn delete_collection_properties( collection: &CollectionHandle, sender: &T::CrossAccountId, - property_keys: Vec, + property_keys: impl Iterator, ) -> DispatchResult { - for key in property_keys { - Self::delete_collection_property(collection, sender, key)?; - } - - Ok(()) + Self::modify_collection_properties(collection, sender, property_keys.map(|key| (key, None))) } /// Set collection propetry permission without any checks. --- a/pallets/fungible/src/lib.rs +++ b/pallets/fungible/src/lib.rs @@ -266,7 +266,7 @@ sender: &T::CrossAccountId, properties: Vec, ) -> DispatchResult { - >::set_collection_properties(collection, sender, properties) + >::set_collection_properties(collection, sender, properties.into_iter()) } /// Delete properties of the collection, associated with the provided keys. @@ -275,7 +275,11 @@ sender: &T::CrossAccountId, property_keys: Vec, ) -> DispatchResult { - >::delete_collection_properties(collection, sender, property_keys) + >::delete_collection_properties( + collection, + sender, + property_keys.into_iter(), + ) } /// Checks if collection has tokens. Return `true` if it has. --- a/pallets/nonfungible/src/lib.rs +++ b/pallets/nonfungible/src/lib.rs @@ -577,20 +577,29 @@ }) } - /// Batch operation to add, edit or remove properties for the token + /// 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)`. /// - /// All affected properties should have mutable permission and sender should have - /// permission to edit those properties. - /// - /// - `nesting_budget`: Limit for searching parents in depth to check ownership. + /// - `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. #[transactional] fn modify_token_properties( collection: &NonfungibleHandle, sender: &T::CrossAccountId, token_id: TokenId, - properties: impl Iterator)>, + properties_updates: impl Iterator)>, is_token_create: bool, nesting_budget: &dyn Budget, ) -> DispatchResult { @@ -614,15 +623,16 @@ }) }; - for (key, value) in properties { - let permission = >::property_permissions(collection.id) + 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 = TokenProperties::::get((collection.id, token_id)) - .get(&key) - .is_some(); + let is_property_exists = stored_properties.get(&key).is_some(); match permission { PropertyPermission { mutable: false, .. } if is_property_exists => { @@ -649,10 +659,9 @@ match value { Some(value) => { - >::try_mutate((collection.id, token_id), |properties| { - properties.try_set(key.clone(), value) - }) - .map_err(>::from)?; + stored_properties + .try_set(key.clone(), value) + .map_err(>::from)?; >::deposit_event(CommonEvent::TokenPropertySet( collection.id, @@ -661,10 +670,9 @@ )); } None => { - >::try_mutate((collection.id, token_id), |properties| { - properties.remove(&key) - }) - .map_err(>::from)?; + stored_properties + .remove(&key) + .map_err(>::from)?; >::deposit_event(CommonEvent::TokenPropertyDeleted( collection.id, @@ -683,6 +691,8 @@ ); } + >::set((collection.id, token_id), stored_properties); + Ok(()) } @@ -784,7 +794,7 @@ sender: &T::CrossAccountId, properties: Vec, ) -> DispatchResult { - >::set_collection_properties(collection, sender, properties) + >::set_collection_properties(collection, sender, properties.into_iter()) } /// Remove properties from the collection @@ -793,7 +803,11 @@ sender: &T::CrossAccountId, property_keys: Vec, ) -> DispatchResult { - >::delete_collection_properties(collection, sender, property_keys) + >::delete_collection_properties( + collection, + sender, + property_keys.into_iter(), + ) } /// Set property permissions for the token. --- a/pallets/nonfungible/src/weights.rs +++ b/pallets/nonfungible/src/weights.rs @@ -184,21 +184,21 @@ .saturating_add(T::DbWeight::get().reads(1 as u64)) .saturating_add(T::DbWeight::get().writes(1 as u64)) } + // Storage: Nonfungible TokenProperties (r:1 w:1) // Storage: Common CollectionPropertyPermissions (r:1 w:0) - // Storage: Nonfungible TokenProperties (r:1 w:1) fn set_token_properties(b: u32, ) -> Weight { - Weight::from_ref_time(4_361_000 as u64) - // Standard Error: 5_349_868 - .saturating_add(Weight::from_ref_time(637_246_356 as u64).saturating_mul(b as u64)) + Weight::from_ref_time(31_850_484 as u64) + // Standard Error: 9_618 + .saturating_add(Weight::from_ref_time(4_721_947 as u64).saturating_mul(b as u64)) .saturating_add(T::DbWeight::get().reads(2 as u64)) .saturating_add(T::DbWeight::get().writes(1 as u64)) } + // Storage: Nonfungible TokenProperties (r:1 w:1) // Storage: Common CollectionPropertyPermissions (r:1 w:0) - // Storage: Nonfungible TokenProperties (r:1 w:1) fn delete_token_properties(b: u32, ) -> Weight { - Weight::from_ref_time(4_489_000 as u64) - // Standard Error: 5_738_954 - .saturating_add(Weight::from_ref_time(689_912_822 as u64).saturating_mul(b as u64)) + Weight::from_ref_time(13_795_000 as u64) + // Standard Error: 28_239 + .saturating_add(Weight::from_ref_time(12_840_446 as u64).saturating_mul(b as u64)) .saturating_add(T::DbWeight::get().reads(2 as u64)) .saturating_add(T::DbWeight::get().writes(1 as u64)) } @@ -354,21 +354,21 @@ .saturating_add(RocksDbWeight::get().reads(1 as u64)) .saturating_add(RocksDbWeight::get().writes(1 as u64)) } + // Storage: Nonfungible TokenProperties (r:1 w:1) // Storage: Common CollectionPropertyPermissions (r:1 w:0) - // Storage: Nonfungible TokenProperties (r:1 w:1) fn set_token_properties(b: u32, ) -> Weight { - Weight::from_ref_time(4_361_000 as u64) - // Standard Error: 5_349_868 - .saturating_add(Weight::from_ref_time(637_246_356 as u64).saturating_mul(b as u64)) + Weight::from_ref_time(31_850_484 as u64) + // Standard Error: 9_618 + .saturating_add(Weight::from_ref_time(4_721_947 as u64).saturating_mul(b as u64)) .saturating_add(RocksDbWeight::get().reads(2 as u64)) .saturating_add(RocksDbWeight::get().writes(1 as u64)) } + // Storage: Nonfungible TokenProperties (r:1 w:1) // Storage: Common CollectionPropertyPermissions (r:1 w:0) - // Storage: Nonfungible TokenProperties (r:1 w:1) fn delete_token_properties(b: u32, ) -> Weight { - Weight::from_ref_time(4_489_000 as u64) - // Standard Error: 5_738_954 - .saturating_add(Weight::from_ref_time(689_912_822 as u64).saturating_mul(b as u64)) + Weight::from_ref_time(13_795_000 as u64) + // Standard Error: 28_239 + .saturating_add(Weight::from_ref_time(12_840_446 as u64).saturating_mul(b as u64)) .saturating_add(RocksDbWeight::get().reads(2 as u64)) .saturating_add(RocksDbWeight::get().writes(1 as u64)) } --- a/pallets/refungible/src/lib.rs +++ b/pallets/refungible/src/lib.rs @@ -515,12 +515,29 @@ 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. #[transactional] fn modify_token_properties( collection: &RefungibleHandle, sender: &T::CrossAccountId, token_id: TokenId, - properties: impl Iterator)>, + properties_updates: impl Iterator)>, is_token_create: bool, nesting_budget: &dyn Budget, ) -> DispatchResult { @@ -544,15 +561,16 @@ Ok(is_bundle_owner) }; - for (key, value) in properties { - let permission = >::property_permissions(collection.id) + 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 = TokenProperties::::get((collection.id, token_id)) - .get(&key) - .is_some(); + let is_property_exists = stored_properties.get(&key).is_some(); match permission { PropertyPermission { mutable: false, .. } if is_property_exists => { @@ -578,10 +596,9 @@ match value { Some(value) => { - >::try_mutate((collection.id, token_id), |properties| { - properties.try_set(key.clone(), value) - }) - .map_err(>::from)?; + stored_properties + .try_set(key.clone(), value) + .map_err(>::from)?; >::deposit_event(CommonEvent::TokenPropertySet( collection.id, @@ -590,10 +607,9 @@ )); } None => { - >::try_mutate((collection.id, token_id), |properties| { - properties.remove(&key) - }) - .map_err(>::from)?; + stored_properties + .remove(&key) + .map_err(>::from)?; >::deposit_event(CommonEvent::TokenPropertyDeleted( collection.id, @@ -612,6 +628,8 @@ ); } + >::set((collection.id, token_id), stored_properties); + Ok(()) } @@ -1353,7 +1371,7 @@ sender: &T::CrossAccountId, properties: Vec, ) -> DispatchResult { - >::set_collection_properties(collection, sender, properties) + >::set_collection_properties(collection, sender, properties.into_iter()) } pub fn delete_collection_properties( @@ -1361,7 +1379,11 @@ sender: &T::CrossAccountId, property_keys: Vec, ) -> DispatchResult { - >::delete_collection_properties(collection, sender, property_keys) + >::delete_collection_properties( + collection, + sender, + property_keys.into_iter(), + ) } pub fn set_token_property_permissions( --- a/pallets/refungible/src/weights.rs +++ b/pallets/refungible/src/weights.rs @@ -246,21 +246,21 @@ .saturating_add(T::DbWeight::get().reads(1 as u64)) .saturating_add(T::DbWeight::get().writes(1 as u64)) } + // Storage: Refungible TokenProperties (r:1 w:1) // Storage: Common CollectionPropertyPermissions (r:1 w:0) - // Storage: Refungible TokenProperties (r:1 w:1) fn set_token_properties(b: u32, ) -> Weight { - Weight::from_ref_time(4_578_000 as u64) - // Standard Error: 5_396_287 - .saturating_add(Weight::from_ref_time(633_314_546 as u64).saturating_mul(b as u64)) + Weight::from_ref_time(25_518_267 as u64) + // Standard Error: 20_451 + .saturating_add(Weight::from_ref_time(5_041_089 as u64).saturating_mul(b as u64)) .saturating_add(T::DbWeight::get().reads(2 as u64)) .saturating_add(T::DbWeight::get().writes(1 as u64)) } + // Storage: Refungible TokenProperties (r:1 w:1) // Storage: Common CollectionPropertyPermissions (r:1 w:0) - // Storage: Refungible TokenProperties (r:1 w:1) fn delete_token_properties(b: u32, ) -> Weight { - Weight::from_ref_time(4_583_000 as u64) - // Standard Error: 5_762_380 - .saturating_add(Weight::from_ref_time(696_007_076 as u64).saturating_mul(b as u64)) + Weight::from_ref_time(13_715_000 as u64) + // Standard Error: 28_323 + .saturating_add(Weight::from_ref_time(13_113_351 as u64).saturating_mul(b as u64)) .saturating_add(T::DbWeight::get().reads(2 as u64)) .saturating_add(T::DbWeight::get().writes(1 as u64)) } @@ -478,21 +478,21 @@ .saturating_add(RocksDbWeight::get().reads(1 as u64)) .saturating_add(RocksDbWeight::get().writes(1 as u64)) } + // Storage: Refungible TokenProperties (r:1 w:1) // Storage: Common CollectionPropertyPermissions (r:1 w:0) - // Storage: Refungible TokenProperties (r:1 w:1) fn set_token_properties(b: u32, ) -> Weight { - Weight::from_ref_time(4_578_000 as u64) - // Standard Error: 5_396_287 - .saturating_add(Weight::from_ref_time(633_314_546 as u64).saturating_mul(b as u64)) + Weight::from_ref_time(25_518_267 as u64) + // Standard Error: 20_451 + .saturating_add(Weight::from_ref_time(5_041_089 as u64).saturating_mul(b as u64)) .saturating_add(RocksDbWeight::get().reads(2 as u64)) .saturating_add(RocksDbWeight::get().writes(1 as u64)) } + // Storage: Refungible TokenProperties (r:1 w:1) // Storage: Common CollectionPropertyPermissions (r:1 w:0) - // Storage: Refungible TokenProperties (r:1 w:1) fn delete_token_properties(b: u32, ) -> Weight { - Weight::from_ref_time(4_583_000 as u64) - // Standard Error: 5_762_380 - .saturating_add(Weight::from_ref_time(696_007_076 as u64).saturating_mul(b as u64)) + Weight::from_ref_time(13_715_000 as u64) + // Standard Error: 28_323 + .saturating_add(Weight::from_ref_time(13_113_351 as u64).saturating_mul(b as u64)) .saturating_add(RocksDbWeight::get().reads(2 as u64)) .saturating_add(RocksDbWeight::get().writes(1 as u64)) } --- a/pallets/unique/src/eth/mod.rs +++ b/pallets/unique/src/eth/mod.rs @@ -33,7 +33,6 @@ }; use pallet_evm::{account::CrossAccountId, OnMethodCall, PrecompileHandle, PrecompileResult}; use pallet_evm_coder_substrate::{dispatch_to_evm, SubstrateRecorder, WithRecorder}; -use sp_std::vec; use up_data_structs::{ CollectionDescription, CollectionMode, CollectionName, CollectionTokenPrefix, CreateCollectionData, @@ -316,13 +315,14 @@ >::set_collection_properties( &collection, &caller, - vec![up_data_structs::Property { + [up_data_structs::Property { key: key::base_uri(), value: base_uri .into_bytes() .try_into() .map_err(|_| "base uri is too large")?, - }], + }] + .into_iter(), ) .map_err(dispatch_to_evm::)?; } --- a/runtime/common/identity.rs +++ b/runtime/common/identity.rs @@ -24,6 +24,9 @@ transaction_validity::{TransactionValidity, ValidTransaction, TransactionValidityError}, }; +#[cfg(feature = "collator-selection")] +use sp_runtime::transaction_validity::InvalidTransaction; + #[derive(Debug, Encode, Decode, PartialEq, Eq, Clone, TypeInfo)] pub struct DisableIdentityCalls; --- a/tests/src/util/playgrounds/unique.ts +++ b/tests/src/util/playgrounds/unique.ts @@ -651,6 +651,9 @@ try { result = await this.signTransaction(sender, this.constructApiCall(extrinsic, params), options, extrinsic) as ITransactionResult; events = this.eventHelper.extractEvents(result.result.events); + const errorEvent = events.find((event) => event.method == 'ExecutedFailed' || event.method == 'CreatedFailed'); + if (errorEvent) + throw Error(errorEvent.method + ': ' + extrinsic); } catch(e) { if(!(e as object).hasOwnProperty('status')) throw e;