Expose collection attributes from Inspect trait (#1914)

# Description

- What does this PR do?

While working with `pallet_nfts` through `nonfungibles_v2` traits
`Inspect, Mutate`, I found out that once you have set the collection
attribute with `<Nfts as Mutate>::set_collection_attribute()`, it's not
possible to read it with `<Nfts as Inspect>::collection_attribute()`
since they use different `namespace` values. When setting the attribute,
`AttributeNamespace::Pallet` is used, while
`AttributeNamespace::CollectionOwner` is used when reading.

more context:
freeverseio/laos#7 (comment)

This PR makes `item` an optional parameter in
`Inspect::system_attribute()`, to be able to read collection attributes.

- Why are these changes needed?

To be able to read collection level attributes when reading attributes
of the collection. It will be possible to read collection attributes by
passing `None` for `item`

- How were these changes implemented and what do they affect?

`NftsApi` is also affected and `NftsApi::system_attribute()` now accepts
optional `item` parameter.

## Breaking change

Because of the change in the `NftsApi::system_attribute()` method's
`item` param, parachains who integrated the `NftsApi` need to update
their API code and frontend integrations accordingly. AssetHubs are
unaffected since the NftsApi wasn't released on those parachains yet.

cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs

@@ -1081,10 +1081,10 @@ impl_runtime_apis! {
 
 		fn system_attribute(
 			collection: u32,
-			item: u32,
+			item: Option<u32>,
 			key: Vec<u8>,
 		) -> Option<Vec<u8>> {
-			<Nfts as Inspect<AccountId>>::system_attribute(&collection, &item, &key)
+			<Nfts as Inspect<AccountId>>::system_attribute(&collection, item.as_ref(), &key)
 		}
 
 		fn collection_attribute(collection: u32, key: Vec<u8>) -> Option<Vec<u8>> {

substrate/bin/node/runtime/src/lib.rs

@@ -2630,10 +2630,10 @@ impl_runtime_apis! {
 
 		fn system_attribute(
 			collection: u32,
-			item: u32,
+			item: Option<u32>,
 			key: Vec<u8>,
 		) -> Option<Vec<u8>> {
-			<Nfts as Inspect<AccountId>>::system_attribute(&collection, &item, &key)
+			<Nfts as Inspect<AccountId>>::system_attribute(&collection, item.as_ref(), &key)
 		}
 
 		fn collection_attribute(collection: u32, key: Vec<u8>) -> Option<Vec<u8>> {

substrate/frame/nfts/runtime-api/src/lib.rs

@@ -48,7 +48,7 @@ sp_api::decl_runtime_apis! {
 
 		fn system_attribute(
 			collection: CollectionId,
-			item: ItemId,
+			item: Option<ItemId>,
 			key: Vec<u8>,
 		) -> Option<Vec<u8>>;
 

substrate/frame/nfts/src/impl_nonfungibles.rs

@@ -79,17 +79,19 @@ impl<T: Config<I>, I: 'static> Inspect<<T as SystemConfig>::AccountId> for Palle
 		Attribute::<T, I>::get((collection, Some(item), namespace, key)).map(|a| a.0.into())
 	}
 
-	/// Returns the system attribute value of `item` of `collection` corresponding to `key`.
+	/// Returns the system attribute value of `item` of `collection` corresponding to `key` if
+	/// `item` is `Some`. Otherwise, returns the system attribute value of `collection`
+	/// corresponding to `key`.
 	///
 	/// By default this is `None`; no attributes are defined.
 	fn system_attribute(
 		collection: &Self::CollectionId,
-		item: &Self::ItemId,
+		item: Option<&Self::ItemId>,
 		key: &[u8],
 	) -> Option<Vec<u8>> {
 		let namespace = AttributeNamespace::Pallet;
 		let key = BoundedSlice::<_, _>::try_from(key).ok()?;
-		Attribute::<T, I>::get((collection, Some(item), namespace, key)).map(|a| a.0.into())
+		Attribute::<T, I>::get((collection, item, namespace, key)).map(|a| a.0.into())
 	}
 
 	/// Returns the attribute value of `item` of `collection` corresponding to `key`.

substrate/frame/nfts/src/tests.rs

@@ -22,7 +22,7 @@ use enumflags2::BitFlags;
 use frame_support::{
 	assert_noop, assert_ok,
 	traits::{
-		tokens::nonfungibles_v2::{Create, Destroy, Mutate},
+		tokens::nonfungibles_v2::{Create, Destroy, Inspect, Mutate},
 		Currency, Get,
 	},
 };
@@ -982,6 +982,86 @@ fn set_collection_owner_attributes_should_work() {
 	});
 }
 
+#[test]
+fn set_collection_system_attributes_should_work() {
+	new_test_ext().execute_with(|| {
+		Balances::make_free_balance_be(&account(1), 100);
+
+		assert_ok!(Nfts::force_create(
+			RuntimeOrigin::root(),
+			account(1),
+			collection_config_with_all_settings_enabled()
+		));
+		assert_ok!(Nfts::mint(RuntimeOrigin::signed(account(1)), 0, 0, account(1), None));
+
+		let collection_id = 0;
+		let attribute_key = [0u8];
+		let attribute_value = [0u8];
+
+		assert_ok!(<Nfts as Mutate<AccountIdOf<Test>, ItemConfig>>::set_collection_attribute(
+			&collection_id,
+			&attribute_key,
+			&attribute_value
+		));
+
+		assert_eq!(attributes(0), vec![(None, AttributeNamespace::Pallet, bvec![0], bvec![0])]);
+
+		assert_eq!(
+			<Nfts as Inspect<AccountIdOf<Test>>>::system_attribute(
+				&collection_id,
+				None,
+				&attribute_key
+			),
+			Some(attribute_value.to_vec())
+		);
+
+		// test typed system attribute
+		let typed_attribute_key = [0u8; 32];
+		#[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug)]
+		struct TypedAttributeValue(u32);
+		let typed_attribute_value = TypedAttributeValue(42);
+
+		assert_ok!(
+			<Nfts as Mutate<AccountIdOf<Test>, ItemConfig>>::set_typed_collection_attribute(
+				&collection_id,
+				&typed_attribute_key,
+				&typed_attribute_value
+			)
+		);
+
+		assert_eq!(
+			<Nfts as Inspect<AccountIdOf<Test>>>::typed_system_attribute(
+				&collection_id,
+				None,
+				&typed_attribute_key
+			),
+			Some(typed_attribute_value)
+		);
+
+		// check storage
+		assert_eq!(
+			attributes(collection_id),
+			[
+				(None, AttributeNamespace::Pallet, bvec![0], bvec![0]),
+				(
+					None,
+					AttributeNamespace::Pallet,
+					bvec![
+						0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+						0, 0, 0, 0, 0, 0, 0
+					],
+					bvec![42, 0, 0, 0]
+				)
+			]
+		);
+
+		assert_ok!(Nfts::burn(RuntimeOrigin::root(), collection_id, 0));
+		let w = Nfts::get_destroy_witness(&0).unwrap();
+		assert_ok!(Nfts::destroy(RuntimeOrigin::signed(account(1)), collection_id, w));
+		assert_eq!(attributes(collection_id), vec![]);
+	})
+}
+
 #[test]
 fn set_item_owner_attributes_should_work() {
 	new_test_ext().execute_with(|| {

substrate/frame/support/src/traits/tokens/nonfungible_v2.rs

@@ -226,7 +226,7 @@ impl<
 		<F as nonfungibles::Inspect<AccountId>>::custom_attribute(account, &A::get(), item, key)
 	}
 	fn system_attribute(item: &Self::ItemId, key: &[u8]) -> Option<Vec<u8>> {
-		<F as nonfungibles::Inspect<AccountId>>::system_attribute(&A::get(), item, key)
+		<F as nonfungibles::Inspect<AccountId>>::system_attribute(&A::get(), Some(item), key)
 	}
 	fn typed_attribute<K: Encode, V: Decode>(item: &Self::ItemId, key: &K) -> Option<V> {
 		<F as nonfungibles::Inspect<AccountId>>::typed_attribute(&A::get(), item, key)
@@ -244,7 +244,7 @@ impl<
 		)
 	}
 	fn typed_system_attribute<K: Encode, V: Decode>(item: &Self::ItemId, key: &K) -> Option<V> {
-		<F as nonfungibles::Inspect<AccountId>>::typed_system_attribute(&A::get(), item, key)
+		<F as nonfungibles::Inspect<AccountId>>::typed_system_attribute(&A::get(), Some(item), key)
 	}
 	fn can_transfer(item: &Self::ItemId) -> bool {
 		<F as nonfungibles::Inspect<AccountId>>::can_transfer(&A::get(), item)

substrate/frame/support/src/traits/tokens/nonfungibles_v2.rs

@@ -75,12 +75,14 @@ pub trait Inspect<AccountId> {
 		None
 	}
 
-	/// Returns the system attribute value of `item` of `collection` corresponding to `key`.
+	/// Returns the system attribute value of `item` of `collection` corresponding to `key` if
+	/// `item` is `Some`. Otherwise, returns the system attribute value of `collection`
+	/// corresponding to `key`.
 	///
 	/// By default this is `None`; no attributes are defined.
 	fn system_attribute(
 		_collection: &Self::CollectionId,
-		_item: &Self::ItemId,
+		_item: Option<&Self::ItemId>,
 		_key: &[u8],
 	) -> Option<Vec<u8>> {
 		None
@@ -113,13 +115,14 @@ pub trait Inspect<AccountId> {
 			.and_then(|v| V::decode(&mut &v[..]).ok())
 	}
 
-	/// Returns the strongly-typed system attribute value of `item` of `collection` corresponding to
-	/// `key`.
+	/// Returns the strongly-typed system attribute value of `item` corresponding to `key` if
+	/// `item` is `Some`. Otherwise, returns the strongly-typed system attribute value of
+	/// `collection` corresponding to `key`.
 	///
 	/// By default this just attempts to use `system_attribute`.
 	fn typed_system_attribute<K: Encode, V: Decode>(
 		collection: &Self::CollectionId,
-		item: &Self::ItemId,
+		item: Option<&Self::ItemId>,
 		key: &K,
 	) -> Option<V> {
 		key.using_encoded(|d| Self::system_attribute(collection, item, d))