feat: add `pallet-api/nonfungibles` #432

chungquantin · 2025-01-16T05:53:52Z

The non-fungibles pallet offers a streamlined interface for interacting with non-fungible assets. The goal is to provide a simplified, consistent API that adheres to standards in the smart contract space. Built on top of the optimized version of the pallet-nfts (See #387).

Index	Name	Type
0	BalanceOf	Read
1	OwnerOf	Read
2	Allowance	Read
3	transfer	dispachable
4	approve	dispatchable
5	TotalSupply	Read
6	GetAttribute	Read
7	mint	dispatchable
8	burn	dispatchable
9	Collection	Read
10	NextCollectionId	Read
11	ItemMetadata	Read
12	create	dispatchable
13	destroy	dispatchable
14	set_attribute	dispatchable
15	clear_attribute	dispatchable
16	set_metadata	dispatchable
17	clear_metadata	dispatchable
18	approve_item_attributes	dispatchable
19	cancel_item_attributes_approval	dispatchable
20	set_max_supply	dispatchable
21	clear_all_transfer_approvals	dispatchable
22	clear_collection_approvals	dispatchable

[sc-1242]

codecov-commenter · 2025-01-16T06:39:50Z

Codecov Report

Attention: Patch coverage is 92.55385% with 121 lines in your changes missing coverage. Please review.

Project coverage is 73.93%. Comparing base (dd22ac2) to head (5316da1).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pallets/api/src/nonfungibles/weights.rs	41.55%	90 Missing ⚠️
pallets/api/src/mock.rs	8.69%	21 Missing ⚠️
pallets/api/src/nonfungibles/mod.rs	97.05%	4 Missing and 1 partial ⚠️
runtime/devnet/src/config/api/mod.rs	97.00%	3 Missing ⚠️
pallets/api/src/nonfungibles/tests.rs	99.82%	1 Missing and 1 partial ⚠️

@@            Coverage Diff             @@
##             main     #432      +/-   ##
==========================================
+ Coverage   71.28%   73.93%   +2.64%     
==========================================
  Files          72       76       +4     
  Lines       13626    15237    +1611     
  Branches    13626    15237    +1611     
==========================================
+ Hits         9713    11265    +1552     
- Misses       3646     3711      +65     
+ Partials      267      261       -6

Files with missing lines	Coverage Δ
pallets/api/src/nonfungibles/benchmarking.rs	`100.00% <100.00%> (ø)`
runtime/devnet/src/config/assets.rs	`100.00% <100.00%> (ø)`
runtime/devnet/src/config/proxy.rs	`0.00% <ø> (ø)`
runtime/devnet/src/lib.rs	`5.44% <ø> (ø)`
pallets/api/src/nonfungibles/tests.rs	`99.82% <99.82%> (ø)`
runtime/devnet/src/config/api/mod.rs	`91.17% <97.00%> (+22.12%)`	⬆️
pallets/api/src/nonfungibles/mod.rs	`97.05% <97.05%> (ø)`
pallets/api/src/mock.rs	`62.50% <8.69%> (-37.50%)`	⬇️
pallets/api/src/nonfungibles/weights.rs	`41.55% <41.55%> (ø)`

... and 8 files with indirect coverage changes

Daanvdplas

Looking very good! Left a few comments based on a high level review. Ready to finalise everything as I didn't find anything concerning!

pallets/api/src/nonfungibles/mod.rs

Daanvdplas · 2025-01-17T08:52:38Z

CI also failing which would be good to resolve for final review.

chungquantin · 2025-01-21T05:01:34Z

pallets/api/src/nonfungibles/weights.rs

+		//  Measured:  `606`
+		//  Estimated: `68341938741594600 + a * (3168 ±44_956_152_011_428) + i * (2663 ±729_819_634_434_010)`
+		// Minimum execution time: 13_000_000 picoseconds.
+		Weight::from_parts(25_055_102, 68341938741594600)


Wonder why this is unexpectedly high

Daanvdplas

Stopped reviewing after the events in mod.rs because I have found too many deviations from the spec without any explanation. Could you please give the PR a final review yourself while looking at the spec (especially found different comments) and explain any changes made to prevent confusion :)

pallets/api/src/lib.rs

pallets/api/src/mock.rs

pallets/api/src/nonfungibles/mod.rs

Daanvdplas · 2025-01-21T08:50:50Z

pallets/api/src/nonfungibles/mod.rs

+			/// The creator of the collection.
+			creator: AccountIdOf<T>,
+			/// The administrator of the collection.
+			admin: AccountIdOf<T>,


Any reason for deviating from the owner parameter in pallet-nfts or is this forgotten in the copy paste of the fungibles event?

I made it on purpose. I don't think the owner field in the pallet-nfts makes sense because it is not the owner but the admin. And this also makes it similar to the one we have in pallet-api/fungibles

Won't that be confusing with:

pop-node/pallets/nfts/src/types.rs

Line 100 in 8b2438b

pub(super) owner: AccountId,

I personally see that field is incorrect. Maybe it could be changed in pallet-nfts?

Up to you, we can also create an issue and adapt later as it delays finishing this PR

pallets/api/src/nonfungibles/mod.rs

… error

pop-api/integration-tests/Cargo.lock

chungquantin · 2025-01-22T09:24:38Z

pallets/api/src/nonfungibles/mod.rs

+	/// The events that can be emitted.
+	#[pallet::event]
+	#[pallet::generate_deposit(pub(super) fn deposit_event)]
+	pub enum Event<T: Config> {


Primary change made to the events that deviate from the spec is the introduction of collection field. The PSP34 standard events are designed for a single non-fungible token. Hence, to support multiple tokens, we need the collection field to specify the collection.

pallets/api/src/nonfungibles/mod.rs

chungquantin · 2025-01-22T09:27:33Z

pallets/api/src/nonfungibles/mod.rs

+	}
+
+	#[pallet::call]
+	impl<T: Config> Pallet<T> {


Similar to the events, to support multiple tokens, we also need to accept collection as parameters to dispatchables related to a non-fungible token.

chungquantin · 2025-01-22T09:32:20Z

pallets/api/src/nonfungibles/mod.rs

+		},
+		/// Details of a specified collection.
+		#[codec(index = 9)]
+		Collection(CollectionIdOf<T>),


The following Read operations, along with the others listed below, are additional reads not specified in the standard or spec. However, I believe they are essential to the design of the Pop API's nonfungible feature. Another potential Read to consider is Item(CollectionIdOf<T>, ItemIdOf<T>), which queries the details of a collection item.

chungquantin · 2025-01-22T09:34:01Z

pallets/api/src/nonfungibles/mod.rs

+		/// - `item` - The item of the collection of whose approvals will be cleared.
+		#[pallet::call_index(5)]
+		#[pallet::weight(NftsWeightInfoOf::<T>::clear_all_transfer_approvals())]
+		pub fn clear_all_transfer_approvals(


Additional dispatchables added that are not mentioned in the spec.

Would be nice to hear why

Also, can we follow the order of the dispatchables defined in the spec. Makes it easier to review. The more unknown / not explained changes in a PR the more question marks arise for the reviewer.

These are implemented for the pallet while the spec mainly stick to the PSP34 standard. And we did agree that we should not limit the capabilities of the pallet. Hence, as we already provided this in the fork pallet nfts, implement it for the nonfungibles pallet will enrich the pallet features

It does however make the PR bigger, more to review, etc.

pallets/api/src/nonfungibles/tests.rs

Daanvdplas

Solid PR! Most of the functionality is correct so well done!

Most of the comments are related to differences with the spec which caused confusion as deviating from it means that it needs to be reviewed again which takes time. In addition, having the order of functions in the same order makes the review process streamlined, same counts for the order of tests with the code (i.e. dispatachables order, etc).

Moreover, the additional functions that have been added create review overload (because it is already a very big PR) because this requires much more than reviewing the logic, tests and benchmarks. This is also why I haven't reviewed those functions.

pallets/api/src/nonfungibles/mod.rs

Daanvdplas · 2025-01-22T12:57:26Z

pallets/api/src/nonfungibles/mod.rs

+			/// The creator of the collection.
+			creator: AccountIdOf<T>,
+			/// The administrator of the collection.
+			admin: AccountIdOf<T>,


Won't that be confusing with:

pop-node/pallets/nfts/src/types.rs

Line 100 in 8b2438b

pub(super) owner: AccountId,

pallets/api/src/nonfungibles/mod.rs

runtime/devnet/src/config/api/mod.rs

runtime/devnet/src/config/assets.rs

runtime/devnet/src/lib.rs

chungquantin · 2025-01-24T08:00:11Z

pallets/api/src/mock.rs

@@ -46,7 +52,7 @@ impl frame_system::Config for Test {
 	type BlockHashCount = BlockHashCount;
 	type BlockLength = ();
 	type BlockWeights = ();
-	type DbWeight = ();
+	type DbWeight = RocksDbWeight;


Requires for testing with <Test as frame_system::Config>::DbWeight::get()::reads(1)

Daanvdplas

Just skimmed over and found some improvements in the impl.rs. I will do a proper review monday.

pallets/api/src/nonfungibles/impls.rs

…elease separately (#433)

peterwht

Nice to see how clean this and simple the mapping is to the pallet.

I haven't reviewed everything, but there are several comments I left.

The biggest one is that in several of these functions you are doing reads before calling into pallet-nfts. However, these values are only needed for the event. It also has the side-effect of double charging weight. By moving these reads to AFTER the pallet-nft call we get a few things:

pallet-nfts does the error handling for us
pallet-nfts pulls the storage items into the memory overlay -- hence allowing us to remove the double charged weight

peterwht · 2025-01-24T18:17:48Z

pallets/api/src/nonfungibles/mod.rs

+			item: ItemIdOf<T>,
+			to: AccountIdOf<T>,
+		) -> DispatchResultWithPostInfo {
+			let owner = NftsOf::<T>::owner(collection, item)


I recommend we move this let owner ... to AFTER the pallet-nfts::transfer and do NOT charge for the read. The reason we don't charge for the read is because the queried item is already in the memory overlay and won't be read from storage. Additionally, we can simplify to use a .expect(...) because pallet-nfts will handle the error checking for us.

peterwht · 2025-01-24T18:21:09Z

pallets/api/src/nonfungibles/mod.rs

+		/// - `item` - The item.
+		/// - `to` - The recipient account.
+		#[pallet::call_index(3)]
+		#[pallet::weight(NftsWeightInfoOf::<T>::transfer() + T::DbWeight::get().reads(1))]


similarly, this extra read should not be needed as the Item is already in the memory overlay (hence we are double charging for the item).

If you ran a benchmark with and without the let owner = ... you should see the number of reads stays the same.

peterwht · 2025-01-24T19:09:44Z

pallets/api/src/nonfungibles/mod.rs

+			to: AccountIdOf<T>,
+			collection: CollectionIdOf<T>,
+			item: ItemIdOf<T>,
+			witness: MintWitness<ItemIdOf<T>, ItemPriceOf<T>>,


Would we be able to modify this parameter to something like:

Change name to price

Set type to ItemPriceOf only

If the witness is optional (as is in the actual pallet-nfts::mint), we should do maybe_price

The item id is already a parameter, so we can create the MintWitness instance ourselves in this function.

peterwht · 2025-01-24T19:10:51Z

pallets/api/src/nonfungibles/mod.rs

+		/// - `collection` - The collection identifier.
+		/// - `item` - The item to burn.
+		#[pallet::call_index(8)]
+		#[pallet::weight(NftsWeightInfoOf::<T>::burn() + T::DbWeight::get().reads(1))]


similar to transfer, we are double charging a read here due to the memory overlay.

peterwht · 2025-01-24T19:11:29Z

pallets/api/src/nonfungibles/mod.rs

+			collection: CollectionIdOf<T>,
+			item: ItemIdOf<T>,
+		) -> DispatchResultWithPostInfo {
+			let owner = NftsOf::<T>::owner(collection, item)


Similarly, would recommend simplifying this by moving it after pallet-nfts::burn, use .expect(), and remove the error mapping with weight read.

peterwht · 2025-01-24T19:12:08Z

pallets/api/src/nonfungibles/mod.rs

+		/// - `admin` - The admin of this collection.
+		/// - `config` - The configuration of the collection.
+		#[pallet::call_index(12)]
+		#[pallet::weight(NftsWeightInfoOf::<T>::create() + T::DbWeight::get().reads(1))]


double-charging

peterwht · 2025-01-24T19:13:06Z

pallets/api/src/nonfungibles/mod.rs

+			config: CollectionConfigOf<T>,
+		) -> DispatchResultWithPostInfo {
+			let creator = ensure_signed(origin.clone())?;
+			let id = NextCollectionIdOf::<T>::get()


would recommend moving after nfts::create, use .expect.

It's cleaner to let pallet-nfts handle the errors for us and pull the storage into the memory overlay.

peterwht · 2025-01-24T19:18:54Z

pallets/api/src/nonfungibles/mod.rs

+    		witness.item_configs,
+    		witness.attributes,
+		))]
+		pub fn destroy(


(Preface: I haven't reviewed the destroy witness PR). One thought, why don't we expand the standard a little bit and have functionality similar to:

destroy_without_witness // max weight until post-dispatch

destroy_with_witness // optional choice to provide the witness for accurate weight

Note: I am not saying to use those names, but just representing the functionality.

see here for more context: #445 (comment)

peterwht · 2025-01-24T20:05:39Z

pallets/api/src/nonfungibles/mod.rs

+				collection,
+				item,
+				key: key.to_vec(),
+				data: value.to_vec(),


We are currently cloning each of these twice. Once when you do key.clone(), and once when you do key.to_vec()

I recommend defining two vars at the top of the fn: key_vec = key.to_vec(); and data_vec = data.to_vec(). This then allows you to do only one clone (which occurs with the .to_vec()).

chungquantin added the ready-for-high-level-review label Jan 16, 2025

chungquantin self-assigned this Jan 16, 2025

chungquantin requested a review from Daanvdplas January 16, 2025 07:20

Daanvdplas reviewed Jan 17, 2025

View reviewed changes

chungquantin force-pushed the chungquantin/feat-main_pallet_nonfungibles branch from df829e7 to 13434d3 Compare January 21, 2025 02:39

chungquantin added ready-for-final-review and removed ready-for-high-level-review labels Jan 21, 2025

chungquantin force-pushed the chungquantin/feat-main_pallet_nonfungibles branch from 8af4673 to ac93544 Compare January 21, 2025 04:06

chungquantin requested a review from Daanvdplas January 21, 2025 04:16

chungquantin commented Jan 21, 2025

View reviewed changes

chungquantin requested review from al3mart, peterwht and f-gate January 21, 2025 05:06

Daanvdplas reviewed Jan 21, 2025

View reviewed changes

chungquantin removed request for al3mart, peterwht and f-gate January 21, 2025 11:27

chungquantin added 11 commits January 22, 2025 12:13

chore: rebase

0ae0d7b

chore: rebase nonfungibles pallet to pallet-nfts

b585741

chore: update nonfungibles tests

5a272f9

refactor: do_approve and do_cancel_approval

2fccbcb

chore: naming

3571709

chore: resolve review comments

6777440

chore: resolve review comments & update weights

9cf8502

chore: add tests for nonfungibles api in runtime devnet

9f17d66

fix: integration test & return post dispatch weight for transfer() on…

010b9cf

… error

refactor: reorder Read impl in nonfungibles

c185d36

chore: remove unnecessary clone()

665feea

chungquantin added 4 commits January 22, 2025 12:13

chore: remove description for pallet configure

f65dc98

refactor: approve() tests

15df8b3

chore: resolve review comments

0396644

feat: add AttributeSet event

ea59ba3

chungquantin force-pushed the chungquantin/feat-main_pallet_nonfungibles branch from df27276 to ea59ba3 Compare January 22, 2025 05:13

chungquantin added 2 commits January 22, 2025 15:09

chore: comments and add test for Collection read

728c1df

chore: revert changes made to pallet nfts

bec6b25

chungquantin commented Jan 22, 2025

View reviewed changes

r0gue-io deleted a comment from chungquantin Jan 22, 2025

chore: fix comment

6a8adaa

chungquantin requested a review from Daanvdplas January 22, 2025 14:52

Daanvdplas reviewed Jan 23, 2025

View reviewed changes

chore: resolve review comments

f70e005

chungquantin commented Jan 24, 2025

View reviewed changes

chore: update function order

4bfbafe

Daanvdplas reviewed Jan 24, 2025

View reviewed changes

Daanvdplas and others added 6 commits January 24, 2025 23:22

chore: add release conventions document and refactor CI to run node r…

ac68a1f

…elease separately (#433)

refactor: reuse WeightInfo in nonfungibles pallet test (#450)

a8b2e7e

chore: update collection details field to public (#449)

bbf5915

chore: rebase and remove local collection details type

9604472

chore: remove impls.rs and update burn()

924e520

Merge branch 'main' into chungquantin/feat-main_pallet_nonfungibles

5316da1

chungquantin requested a review from peterwht January 24, 2025 17:14

peterwht requested changes Jan 24, 2025

View reviewed changes

peterwht mentioned this pull request Jan 24, 2025

refactor: remove destroy witness #445

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `pallet-api/nonfungibles` #432

feat: add `pallet-api/nonfungibles` #432

chungquantin commented Jan 16, 2025 •

edited

Loading

codecov-commenter commented Jan 16, 2025 •

edited

Loading

Daanvdplas left a comment

Daanvdplas commented Jan 17, 2025

chungquantin Jan 21, 2025

Daanvdplas left a comment

Daanvdplas Jan 21, 2025

chungquantin Jan 21, 2025

Daanvdplas Jan 22, 2025

chungquantin Jan 23, 2025

Daanvdplas Jan 24, 2025

chungquantin Jan 22, 2025

chungquantin Jan 22, 2025

chungquantin Jan 22, 2025

chungquantin Jan 22, 2025

Daanvdplas Jan 22, 2025

chungquantin Jan 24, 2025

Daanvdplas Jan 24, 2025

Daanvdplas left a comment

Daanvdplas Jan 22, 2025

chungquantin Jan 24, 2025

Daanvdplas left a comment

peterwht left a comment

peterwht Jan 24, 2025

peterwht Jan 24, 2025

peterwht Jan 24, 2025 •

edited

Loading

peterwht Jan 24, 2025

peterwht Jan 24, 2025

peterwht Jan 24, 2025

peterwht Jan 24, 2025

peterwht Jan 24, 2025

peterwht Jan 24, 2025

peterwht Jan 24, 2025 •

edited

Loading

feat: add pallet-api/nonfungibles #432

Are you sure you want to change the base?

feat: add pallet-api/nonfungibles #432

Conversation

chungquantin commented Jan 16, 2025 • edited Loading

codecov-commenter commented Jan 16, 2025 • edited Loading

Codecov Report

Daanvdplas left a comment

Choose a reason for hiding this comment

Daanvdplas commented Jan 17, 2025

Choose a reason for hiding this comment

Daanvdplas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daanvdplas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daanvdplas left a comment

Choose a reason for hiding this comment

peterwht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterwht Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterwht Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

feat: add `pallet-api/nonfungibles` #432

feat: add `pallet-api/nonfungibles` #432

chungquantin commented Jan 16, 2025 •

edited

Loading

codecov-commenter commented Jan 16, 2025 •

edited

Loading

peterwht Jan 24, 2025 •

edited

Loading

peterwht Jan 24, 2025 •

edited

Loading