feat(sub0): nonfungibles pop-api + pallet implementation #353

chungquantin · 2024-10-24T06:38:44Z

Nonfungible contracts size:

Original wasm size: 61.5K, Optimized: 21.7K

Description

In an effort to provide a unifying experience for developers to access runtime functionality for non-fungible tokens, APIs compliant to the PSP34 standard will be provided, as well as a set of extra functions (see Management). The interface as defined by this specification will be accessible for smart contracts via:

use pop_api::nonfungibles::<function>;

Note that the lack of version equals latest. As an example, the user could explicitly specify the version as:

use pop_api::v0::nonfungibles::<function>;

The PR includes changes made to the forked pallet-nfts following what has been discussed in the internal spec. Introducing two new pallets;

pallets/nfts: Changes made to the forked version to add new storage items AccountBalance and Allowances and update the following logic + unit tests to support the new storage items.
pallets/api/nonfungibles: Built on top of the pallet-nfts to expose READs and DISPATCHs for pop-api library.

The Nonfungible API provides the contract ability to mint / burn a new item, transfer and approve following the PSP34 Standard.
There are additional features to manage the Nonfungible such as create / destroy a collection, set / clear metadata, set / clear / approve / cancel_approve item attribute.

Becnhmarking:

Updated pallet-nfts weight file to cover AccountBalance read case.
Update approved_transfer, transfer and cancel_approval to count Allowances storage read.

…onfungibles' into chungquantin/sub0-nonfungibles

…onfungibles

Daanvdplas

Amazing work! Especially in the short amount of time.

I feel there has to be a way to reduce the amount of changes made throughout the repository. E.g. BoundedSlice implements the PartialEq trait which we might be able to use and therefore don't have to make the changes you made regarding the PartialEq. In addition, I think the changes in the api's pallet mock.rs are not required. This all makes the sub0 branch cleaner in terms of additional changes and reduces the amount of possible errors that can occur.

Not now, but the benchmark and weight results of destroy are incorrect. Just leaving this here for later reference, as well as the other comments starting with "Not now,"

For all, inluding the other comments, please be thoughtful on the amount of time spend as we want this to merge asap. It might also have to be rebased after the sponsorship gets merged.

The integration tests are passing apart from the event assertions for the fungibles, more specifically, in the transfer_works and approve_works test this event assertion is failing, there might be caused due to the accout mapping, unfortunately I couldn't figure it out. This shouildn't stop merging this PR though.

Daanvdplas · 2024-10-30T16:08:29Z

pallets/nfts/src/features/create_delete_collection.rs

+			// TODO: Do we need another storage item to keep track of number of holders of a
+			// collection
+			let _ =
+				AccountBalance::<T, I>::clear_prefix(collection, collection_details.items, None);


Not now, I think this is a storage write per account that is removed here. This means that we might need to add the amount of items in the collection in the witness data.

Daanvdplas · 2024-10-30T16:11:38Z

pallets/nfts/src/features/create_delete_collection.rs

+			// collection
+			let _ =
+				AccountBalance::<T, I>::clear_prefix(collection, collection_details.items, None);
+			let _ = Allowances::<T, I>::clear_prefix((collection,), collection_details.items, None);


Not now, this is not included in the benchmark function and needs to be accommodated for. This again should added to the witness data in some way.

This comment and the above give reason to change the destroying of a collection like done in pallet assets. This needs to be though out how this will look like and whether this improves things.

#335 (comment)

Daanvdplas · 2024-10-30T16:23:04Z

pallets/nfts/src/weights.rs

@@ -141,6 +124,8 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 	/// Proof: `Nfts::CollectionRoleOf` (`max_values`: None, `max_size`: Some(69), added: 2544, mode: `MaxEncodedLen`)
 	/// Storage: `Nfts::Attribute` (r:1001 w:1000)
 	/// Proof: `Nfts::Attribute` (`max_values`: None, `max_size`: Some(479), added: 2954, mode: `MaxEncodedLen`)
+	/// Storage: `Nfts::AccountBalance` (r:1 w:0)


FYI: this showed me that there could be something wrong in the current implementation of destroy's benchmark logic. Always super valuable to see the changes in the weight results.

#335 (comment)

Daanvdplas · 2024-10-30T16:31:09Z

pallets/nfts/src/weights.rs

@@ -644,39 +641,41 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 	/// Proof: `Nfts::CollectionConfigOf` (`max_values`: None, `max_size`: Some(73), added: 2548, mode: `MaxEncodedLen`)
 	/// Storage: `Nfts::ItemConfigOf` (r:1 w:0)
 	/// Proof: `Nfts::ItemConfigOf` (`max_values`: None, `max_size`: Some(48), added: 2523, mode: `MaxEncodedLen`)
+	/// Storage: `Nfts::AccountBalance` (r:2 w:2)


Not now, this is the counter argument for adding this storage map. Nothing to do now but important to re-evaluate later on.

#335 (comment)

pallets/nfts/src/features/approvals.rs

pallets/nfts/src/types.rs

pallets/api/src/mock.rs

pallets/api/src/nonfungibles/mod.rs

refactor: item metadata fix: item metadata tests test(nonfungibles): add mint and burn tests refactor: non fungibles sub0 (#364) Co-authored-by: Tin Chung <[email protected]> fix: pallet tests + integration tests refactor: remove unused types refactor(nonfungibles): CollectionConfig parameter chore: resolve review comments refactor: item metadata fix: item metadata tests test(nonfungibles): add mint and burn tests refactor: non fungibles sub0 (#364) Co-authored-by: Tin Chung <[email protected]> fix: pallet tests + integration tests refactor: remove unused types refactor(nonfungibles): CollectionConfig parameter chore: remove unused code

Co-authored-by: Daanvdplas <[email protected]>

chungquantin added 30 commits October 14, 2024 22:23

chore: forked pallet nfts

ac35a80

fix: formatting

59bd0ec

fix: formatting

0460a44

feat: add nonfungibles implementation

d18123f

chore: add nfts pallet

8832470

feat: add new storage items to pallet-nfts

facbb17

feat: check allowance

9e5e808

test(nfts): check account balance & test total supply

fe1f448

test(nfts): allowance works

709398c

refactor(test): check_allowance_works

35a2839

fix(nfts): cancel / approve collection transfer

501a3c7

test(nfts): cancel approval collection

cebd059

test(nfts): update approval test cases

d235928

feat(nfts): get attribute read method

9015146

feat(nonfungibles + nfts): add new methods to manage attributes

2ad1764

refactor(nfts): toml lock file

48abce6

fix(pallet-api): propagate try-runtime feature

390ca78

fix(api/nonfungibles): unit tests

2653a05

test(api/nonfungibles): encoding_read_result

c41134f

refactor(api/nonfungibles): pallet tests

a32e801

test(nonfungibles): add tests and remove collection owner read

83e8122

fix(runtime): remove PartialEq, add Encode

c635172

feat(nonfungibles): pop-api

f4d1ba4

test(nonfungibles): pallet unit testing

d3c9295

fix: [nonfungibles, NonFungibles]

b6ebd48

fix: base call filter

356a253

feat(nonfungibles): pop-api integration tests utils

b953b42

fix: benchmark listing

914513d

fix: test

6816dff

feat: add integration test contract

0a94a25

chungquantin added 12 commits October 28, 2024 18:48

feat: add integration test contract

ea91eb6

test(nonfungibles): integration tests skeleton

5d43294

fix(nonfungibles): integration tests

a6da1ff

test(nonfungibles): add balance_of_works

34f84e2

test(nonfungibles): add more integration tests

f976c20

test(nonfungibles): add more tests

2abb4ff

fix: devnet api config

fe4ef45

chore: update weight pallet-nfts file

ebbba96

chore: rebase

f303fe9

Merge remote-tracking branch 'refs/remotes/origin/chungquantin/sub0-n…

9ba71d3

…onfungibles' into chungquantin/sub0-nonfungibles

fix: formatting

a9f4ceb

fix: taplo

159d6bc

chungquantin force-pushed the chungquantin/sub0-pallet_nfts branch from f20d191 to 11977d1 Compare October 28, 2024 17:08

chungquantin added 4 commits October 29, 2024 00:10

fix: remove duplicate import

a5a1c8c

chore: nonfungibles rebase (#360)

0ae4535

Merge branch 'chungquantin/sub0-pallet_nfts' into chungquantin/sub0-n…

712122b

…onfungibles

fix: formatting

b879488

Daanvdplas reviewed Oct 30, 2024

View reviewed changes

chungquantin force-pushed the chungquantin/sub0-nonfungibles branch 2 times, most recently from 45ac5ca to 5bcdd66 Compare November 1, 2024 07:12

chungquantin force-pushed the chungquantin/sub0-nonfungibles branch from e00f8be to 8745056 Compare November 1, 2024 11:18

chungquantin requested a review from Daanvdplas November 1, 2024 11:20

Daanvdplas added 2 commits November 1, 2024 14:44

fix: collection id

7d7a2ac

refactor: only add useful helper functions

4ef42fb

Daanvdplas merged commit 3fa6003 into chungquantin/sub0-pallet_nfts Nov 1, 2024
9 of 12 checks passed

Daanvdplas deleted the chungquantin/sub0-nonfungibles branch November 1, 2024 14:22

Daanvdplas added a commit that referenced this pull request Nov 1, 2024

feat(sub0): nonfungibles pop-api + pallet implementation (#353)

3748388

Co-authored-by: Daanvdplas <[email protected]>

chungquantin mentioned this pull request Nov 13, 2024

feat(pop-api): nonfungibles use case #335

Open

20 tasks

Daanvdplas mentioned this pull request Nov 19, 2024

feat(pallet-nfts): optimization #387

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sub0): nonfungibles pop-api + pallet implementation #353

feat(sub0): nonfungibles pop-api + pallet implementation #353

chungquantin commented Oct 24, 2024 •

edited

Loading

Daanvdplas left a comment

Daanvdplas Oct 30, 2024 •

edited

Loading

Daanvdplas Oct 30, 2024 •

edited

Loading

chungquantin Nov 13, 2024

Daanvdplas Oct 30, 2024 •

edited

Loading

chungquantin Nov 13, 2024

Daanvdplas Oct 30, 2024 •

edited

Loading

chungquantin Nov 13, 2024

feat(sub0): nonfungibles pop-api + pallet implementation #353

feat(sub0): nonfungibles pop-api + pallet implementation #353

Conversation

chungquantin commented Oct 24, 2024 • edited Loading

Description

Becnhmarking:

Daanvdplas left a comment

Choose a reason for hiding this comment

Daanvdplas Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Daanvdplas Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

chungquantin Nov 13, 2024

Choose a reason for hiding this comment

Daanvdplas Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

chungquantin Nov 13, 2024

Choose a reason for hiding this comment

Daanvdplas Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

chungquantin Nov 13, 2024

Choose a reason for hiding this comment

chungquantin commented Oct 24, 2024 •

edited

Loading

Daanvdplas Oct 30, 2024 •

edited

Loading

Daanvdplas Oct 30, 2024 •

edited

Loading

Daanvdplas Oct 30, 2024 •

edited

Loading

Daanvdplas Oct 30, 2024 •

edited

Loading