Add Mutability to Properties

[HashWarlock]

Currently only the issuer can set a Property & if a NFT is locked then the Property cannot be changed or removed. Based on the spec setproperty.md, the user should be able to customize these properties if they are mutable. We have also run into this problem with flexibility for a project on Phala utilizing RMRK NFTs.

If we take a look at the logic here, the only way to update a Property for a NFT is if the Collection issuer is the sender AND the issuer owns the NFT.

rmrk-substrate/pallets/rmrk-core/src/functions.rs

Lines 48 to 69 in a3abdac

	fn property_set(
	sender: T::AccountId,
	collection_id: CollectionId,
	maybe_nft_id: Option<NftId>,
	key: KeyLimitOf<T>,
	value: ValueLimitOf<T>,
	) -> DispatchResult {
	let collection =
	Collections::<T>::get(&collection_id).ok_or(Error::<T>::CollectionUnknown)?;
	ensure!(collection.issuer == sender, Error::<T>::NoPermission);
	if let Some(nft_id) = &maybe_nft_id {
	// Check NFT lock status
	ensure!(
	!Pallet::<T>::is_locked(collection_id, *nft_id),
	pallet_uniques::Error::<T>::Locked
	);
	let (root_owner, _) = Pallet::<T>::lookup_root_owner(collection_id, *nft_id)?;
	ensure!(root_owner == collection.issuer, Error::<T>::NoPermission);
	}
	Properties::<T>::insert((&collection_id, maybe_nft_id, &key), &value);
	Ok(())
	}

[HashWarlock]

This can also start the discussion on Logic for properties.

[bmacer]

Removing the Locked error sounds like a good idea.

So it seems the Property should map to an object that is {isMutable: bool, value: something} and just do a check on its existence or mutability?

I assume the issuer should be able to update the properties if the property is mutable but the issuer is not the owner, correct?

[bmacer]

initial attempts to implement this failed, running into an issue refactoring because of query_properties.

rmrk-substrate/pallets/rmrk-core/src/functions.rs

Lines 753 to 764 in d2d508d

	pub fn query_properties(
	collection_id: CollectionId,
	nft_id: Option<NftId>,
	filter_keys: Option<BTreeSet<BoundedVec<u8, <T as pallet_uniques::Config>::KeyLimit>>>,
	) -> impl Iterator<Item = PropertyInfoOf<T>> {
	Properties::<T>::iter_prefix((collection_id, nft_id))
	.filter(move |(key, _)| match &filter_keys {
	Some(filter_keys) => filter_keys.contains(key),
	None => true,
	})
	.map(|(key, value)| PropertyInfoOf::<T> { key, value })
	}

tried changing the .map line to
.map(|(key, mutable, value)| PropertyInfoOf::<T> { key, mutable, value })

after changing PropertyInfo to

pub struct PropertyInfo<BoundedValue> {
	/// Key of the property
	// #[cfg_attr(feature = "std", serde(with = "serialize::vec"))]
	pub key: BoundedKey,

	/// Value of the property
	// #[cfg_attr(feature = "std", serde(with = "serialize::bool"))]
	pub mutable: bool,

	/// Value of the property
	#[cfg_attr(feature = "std", serde(with = "serialize::vec"))]
	pub value: BoundedValue,
}

but there's some type mismatch happening here that I can't figure out.