feat(pallet-nfts): optimization

[chungquantin]

Edited: 09/12/2024

DESCRIPTION

This pull request introduces new storage items to the pallet-nfts to optimize the performance for the use case of the Pop API contract library implementation.

DESIGN DECISION

New configuration parameters

• CollectionApprovalDeposit: The basic amount of funds that must be reserved for collection approvals.

This is held for an additional storage item whose value size is sizeof((Option<BlockNumber>, Balance)) bytes and whose key size is sizeof((CollectionId, AccountId, AccountId)) bytes.

CollectionApprovalDeposit will be reserved from the owner on collection approval creation. The same amount of reserved CollectionApprovalDeposit will be unreserved back to the owner on collection approval cancellation.

pub const NftsCollectionApprovalDeposit: Balance = deposit(1, 137);

How the bytes are calculated?

Key

• Blake128Concat + CollectionId (u32) = 16 + 4
• Blake128Concat + Account = 16 + 32
• Blake128Concat + Account = 16 + 32

Value

• Option (Option) = 1 + 4
• Balance (u128) = 16

Key = 16 + 4 + 16 + 32 + 16 +32 = 116 bytes
Value = 1 + 4 + 16 = 21 bytes

Total bytes = 137 bytes

New storage items and related changes

• AccountBalance: Keep track of the total number of collection items an account has. This storage item needs to be updated on collection item transferred (fn do_transfer()), burnt (fn do_burn()) and minted (fn do_mint()).

Reason for the storage item?

A custom storage map has to be created because the owned_in_collection method in pallet-nfts is not optimised for the frequently used method PSP34::balance_of. The method has to read every account that is owned instead of a single read for the amount.

• CollectionApprovals: Keep track of the collection approval status for a delegated account.

Reason for the storage item?

Inspired by Aleph Zero | PSP34 and Unique Network | pallet-nonfungibles.

First, no api or storage read is currently available to support the method PSP34::allowance. The scenario where item == None, it needs to return whether the operator is approved for all items within a given collection. Without the CollectionApprovals storage map this would require a storage read per item the owner owns in the collection.

Changes made to dispatchable functions

Introducing new dispatchable functions and update the pallet call indices of most of functions:

• approve_collection_transfer: Approve collection items owned by the origin to be transferred by a delegated third-party account. This function reserves the required deposit CollectionApprovalDeposit from the origin account.
• force_approve_collection_transfer: Force-approve collection items owned by the specified owner to be transferred by a delegated third-party account. This function reserves the required deposit CollectionApprovalDeposit from the origin account.
• cancel_collection_approval: Cancel one of the collection approvals.
• force_cancel_collection_approval: Force-cancel one of the collection approvals granted by the specified owner account. Returning the reserved funds to the delegate.
• clear_all_collection_approvals: Cancel all the collection approvals. Returning the reserved funds to the delegate.
• force_clear_all_collection_approvals: Force-cancel all the collection approvals granted by the specified owner account. Returning the reserved funds to the delegate.

check_collection_approval

// New method added.
fn check_collection_approval(collection: &T::CollectionId, account: &T::AccountId, delegate: &T::AccountId) -> DispatchResult 

Checks whether the delegate has the necessary allowance to transfer items in the collection that are owned by the account.

check_approval

// New method added.
fn check_approval(collection: &T::CollectionId, maybe_item: &Option<T::ItemId>, account: &T::AccountId, delegate: &T::AccountId) -> DispatchResult

Checks whether the delegate has the necessary allowance to transfer items within the collection or a specific item in the collection. If the delegate has an approval to transfer all items in the collection that are owned by the account, they can transfer every item without requiring explicit approval for that item.

• If Item = None

Collection Approval	Item Approval	Status
True	False	True
True	True	True
False	True	False
False	False	False

• If Item = Some

Collection Approval	Item Approval	Status
True	False	True
True	True	True
False	True	True
False	False	False

do_approve_collection_transfer

// New method added.
fn do_approve_collection_transfer(origin: T::AccountId, collection: T::CollectionId, delegate: T::AccountId, maybe_deadline: Option<BlockNumberFor<T>>) -> DispatchResult

NOTE: Weight diff before and after removing the CollectionApprovalCount

Store the new approval with deadline. Approving a delegate to transfer items owned by the signed origin in the collection will reserve some deposit amount (configured via T::CollectionApprovalDeposit) from the origin. With the reserved deposit, we don't need to worry about the unbounded storage map and the depositor is incentivised to remove the collection approval to unblock the collection from destruction.

do_cancel_collection_approval

pub(crate) fn do_cancel_collection_approval(origin: T::AccountId, collection: T::CollectionId, delegate: T::AccountId) -> DispatchResult 

NOTE: Weight diff before and after removing the CollectionApprovalCount

Cancels the transfer of items in the collection that owned by the origin to a delegate. This method will remove the collection approval granted to a delegate and unreserve the deposited fund to the delegate.

do_clear_all_collection_approvals

NOTE: Weight diff before and after removing the CollectionApprovalCount

// New method added.
fn do_clear_all_collection_approvals(origin: T::AccountId, collection: T::CollectionId, limit: u32) -> Result<u32, DispatchError> 

This function is used to clear limit collection approvals for the collection items of owner. After clearing all approvals, the deposit of each collection approval is returned to the owner account and the ApprovalsCancelled event is emitted.

ApprovalsCancelled is a new event type emitted when multiple approvals of a collection or item were cancelled.

Weight of this method is calculated by the provided limit.

do_destroy_collection

// Changes made to the existing methods.
pub fn do_destroy_collection(collection: T::CollectionId, witness: DestroyWitness, maybe_check_owner: Option<T::AccountId>) -> Result<DestroyWitness, DispatchError>;

NOTE: Weight diff before and after removing the CollectionApprovalCount

To destroy a collection, all collection approvals must be removed first. Destroying a collection can only be called when there is no collection approval exists. If yes, requires the accounts that granted those collection approvals to remove all them first through new methods clear_all_collection_approvals or cancel_collection_approval. These methods unreserve the deposited funds back to the origin on called.

Introducing new error types

• NoItemOwned: Account owns zero item in the collection.
• DelegateApprovalConflict: Collection approval and item approval conflicts.
  • Thrown in do_cancel_approval() if there is an existing collection approval with key (collection, account, delegate).
  • Thrown in do_clear_all_transfer_approvals() if there are collection approvals exist. All collection approvals must be removed first before the method can be called.
• CollectionApprovalsExist: There are collection approvals exist.
  • Thrown in do_destroy_collection() if there are collection approvals. All collection approvals must be removed first before the method can be called.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 79.13330% with 443 lines in your changes missing coverage. Please review.

Project coverage is 71.39%. Comparing base (3476994) to head (f83bdc5).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
pallets/nfts/src/weights.rs	5.52%	410 Missing ⚠️
pallets/nfts/src/lib.rs	78.33%	4 Missing and 9 partials ⚠️
pallets/nfts/src/common_functions.rs	93.85%	6 Missing and 1 partial ⚠️
pallets/nfts/src/impl_nonfungibles.rs	0.00%	5 Missing ⚠️
pallets/nfts/src/features/create_delete_item.rs	83.33%	0 Missing and 3 partials ⚠️
pallets/nfts/src/features/approvals.rs	98.42%	1 Missing and 1 partial ⚠️
pallets/nfts/src/features/transfer.rs	83.33%	0 Missing and 2 partials ⚠️
pallets/nfts/src/features/atomic_swap.rs	88.88%	0 Missing and 1 partial ⚠️

@@            Coverage Diff             @@
##             main     #387      +/-   ##
==========================================
+ Coverage   68.41%   71.39%   +2.97%     
==========================================
  Files          70       72       +2     
  Lines       11838    13520    +1682     
  Branches    11838    13520    +1682     
==========================================
+ Hits         8099     9652    +1553     
- Misses       3482     3595     +113     
- Partials      257      273      +16     

Files with missing lines	Coverage Δ
pallets/nfts/src/benchmarking.rs	85.79% <ø> (ø)
pallets/nfts/src/features/buy_sell.rs	90.69% <100.00%> (ø)
...lets/nfts/src/features/create_delete_collection.rs	84.88% <100.00%> (+0.54%)	⬆️
pallets/nfts/src/mock.rs	93.75% <ø> (-6.25%)	⬇️
pallets/nfts/src/tests.rs	99.91% <100.00%> (+0.03%)	⬆️
pallets/nfts/src/types.rs	66.43% <ø> (-0.68%)	⬇️
runtime/devnet/src/config/assets.rs	100.00% <100.00%> (ø)
runtime/devnet/src/lib.rs	5.30% <ø> (-0.23%)	⬇️
runtime/testnet/src/config/assets.rs	100.00% <100.00%> (ø)
pallets/nfts/src/features/atomic_swap.rs	90.78% <88.88%> (+0.26%)	⬆️
... and 7 more

... and 12 files with indirect coverage changes

[Daanvdplas]

Great progress but there are things that need more consideration. As for the comments that need discussion and decision making, please lead this effort. I would advise to tackle them one by one, separately, so that it is easiest for other team members to understand the problem. What is always hugely helpful is to research yourself and find all the possible solutions. Then provide the best solutions to the team to make a decision as effective as possible.

Besides my comments in the code you also have to reconsider the destroy process. Right now we are removing the AccountBalance and Allowances when destroying the collection. This should be done earlier in the burning process. One potential solution which you'd have to research more is burning is only possible when there is no allowance set for the item. As for the account balance, this should already be 0 as all the items should already be burned before destroying the collection. Note the changes you made for the curious implementation, if my suggestion is correct these have to be removed again.

Moreover, we might want to consider to change the destroy process like done in pallet assets. This needs discussion and decision making with the team as well.

Finally, I would really appreciate if we could separate this PR in three PRs:

1. AccountBalance
2. Allowances
3. destroy

This will be much more effective. Another thing to look for; there are a lot of clippy warnings which has to be resolved.

[chungquantin]

Moreover, we might want to consider to change the destroy process like done in pallet assets

Why do I think we should not go with the implementation of destroy similar to pallet_assets? This requires us to make more changes to the audited pallet destroy_collection method, and also the benchmarking and the test. So I tried to keep the changes as minimal as I can

[chungquantin]

Not changed

[peterwht]

Amazing work! Very clean, and I can definitely see how this will be better for smart contract devs.

One thing I noted (likely already known), but this applies to both PSP-34 and ERC-721. So, upstreaming this change will be useful to us and to Plaza.

I have not reviewed tests or benchmarks yet.

I have left various comments for improvements. The biggest concerns are:

• Force granting approval rights seems wrong. See comment for more details
• If we are not allowing collection destruction until all approvals have been cleared, then an Admin account of the collection should be able to force clear as well.

[peterwht]

Brain's a little fuzzy right now so will need to check back. But will leave this comment in case anyone has an answer.

check_origin is an Option (it may not exist). But what happens if maybe_check_origin == None, BUT there is some approval over the entire collection allowing the transfer of this item.

My guess is that check_origin is always provided anyway, so it's likely a non-issue.

[chungquantin]

IMO, this should be not a case. If maybe_check_origin == None (such as ForceOrigin), we will allow transfer approvals to be removed anyway and bypass the check. This does not bring any vulnerabilities and only authorized account is allowed to take an action.

[peterwht]

leaving a review comment for future reference. This error is unused in the original pallet and is leftover from pallet-uniques. Hence, we are removing it.

[chungquantin]

Wonder if we need to hard code the codec indices for these error variants?

[peterwht]

Right now, I would say no. When we try to upstream this to P-SDK, the hardcoded indices likely won't be accepted.

[peterwht]

Approving! Really great job with this.

Before merging, could you please just change the PR title to something like: "feat(nfts): add CollectionApprovals and AccountBalance storages, and general improvements"

Not a blocker, but if we are upstreaming to P-SDK, we could fix the outdated comment on the Admin role and change the name of the misleading test: cancel_approval_works_with_admin. Up to you if you make those changes though.