feat(pallet-nfts): optimization #387
Conversation
chungquantin
changed the title
…fts' into chungquantin/feat-nfts
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #387 +/- ##
==========================================
+ Coverage 68.41% 70.44% +2.02%
==========================================
Files 70 70
Lines 11838 13109 +1271
Branches 11838 13109 +1271
==========================================
+ Hits 8099 9234 +1135
- Misses 3482 3603 +121
- Partials 257 272 +15
|
There was a problem hiding this comment.
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:
- AccountBalance
- Allowances
- destroy
This will be much more effective. Another thing to look for; there are a lot of clippy warnings which has to be resolved.
Why do I think we should not go with the implementation of |
chungquantin
changed the base branch from
main
to
chungquantin/fix-nfts_clippy
| delegate.clone(), | ||
| None | ||
| ), | ||
| Error::<Test>::NoItemOwned |
There was a problem hiding this comment.
Note: Test error thrown NoItemOwned if force-approving unknown collection.
| @@ -693,6 +736,24 @@ pub mod pallet { | |||
| CollectionNotEmpty, | |||
| /// The witness data should be provided. | |||
| WitnessRequired, | |||
| /// The account owns zero items in the collection. | |||
| NoItemOwned, | |||
There was a problem hiding this comment.
Note: Tested in
- Force-approving an unknown collection: https://github.com/r0gue-io/pop-node/pull/387/files#r1881265589
- Force-approving a zero item collection: https://github.com/r0gue-io/pop-node/pull/387/files#r1881266502
- Approving an unknown collection: https://github.com/r0gue-io/pop-node/pull/387/files#r1881259402
- Approving a zero item collection: https://github.com/r0gue-io/pop-node/pull/387/files#r1881259651
| delegate.clone(), | ||
| None | ||
| ), | ||
| Error::<Test>::NoItemOwned |
There was a problem hiding this comment.
Note: Test error thrown NoItemOwned if force-approving a collection with zero items.
| item: 42, | ||
| owner: account(2), | ||
| collection: collection_id, | ||
| item: Some(item_id), |
There was a problem hiding this comment.
Note: Test AllApprovalsCancelled emitted on clear_all_transfer_approvals
| owner: T::AccountId, | ||
| }, | ||
| /// All approvals of a collection or item were cancelled. | ||
| AllApprovalsCancelled { |
There was a problem hiding this comment.
Note: Test in:
- clear_all_transfer_approvals: https://github.com/r0gue-io/pop-node/pull/387/files#r1881268074
| CollectionApprovals::<Test>::iter_prefix((collection_id, owner.clone())).count(), | ||
| 2 | ||
| ); | ||
| assert!(!events().contains(&Event::<Test>::ApprovalsCancelled { |
There was a problem hiding this comment.
Note: No event ApprovalsCancelled emitted if limit = 0 provided to clear_collection_approvals
| clear_collection_approvals(origin.clone(), maybe_owner.clone(), collection_id, 10), | ||
| Ok(Some(WeightOf::<Test>::clear_collection_approvals(1)).into()) | ||
| ); | ||
| assert!(events().contains(&Event::<Test>::ApprovalsCancelled { |
There was a problem hiding this comment.
Note: ApprovalsCancelled emitted on clear_collection_approvals called.
| /// All approvals of an item got cancelled. | ||
| AllApprovalsCancelled { collection: T::CollectionId, item: T::ItemId, owner: T::AccountId }, | ||
| /// Multiple approvals of a collection or item were cancelled. | ||
| ApprovalsCancelled { |
There was a problem hiding this comment.
Note: Tested in:
- No event emitted if
limit = 0forclear_collection_approvals: https://github.com/r0gue-io/pop-node/pull/387/files#r1881268851 - No event emitted if there are no approvals removed: https://github.com/r0gue-io/pop-node/pull/387/files#r1881281186
- Event emitted successfully if
limit > 0: https://github.com/r0gue-io/pop-node/pull/387/files#r1881269482
chungquantin
force-pushed
the
chungquantin/feat-nfts
branch
from
c756aab to
769f50a
Compare
| clear_collection_approvals(origin.clone(), maybe_owner.clone(), collection_id, 10), | ||
| Ok(Some(WeightOf::<Test>::clear_collection_approvals(0)).into()) | ||
| ); | ||
| assert!(!events().contains(&Event::<Test>::ApprovalsCancelled { |
There was a problem hiding this comment.
Note: Emitting no event ApprovalsCancelled if zero approvals removed.
| item_id, | ||
| delegate.clone() | ||
| )); | ||
| assert!(events().contains(&Event::<Test>::ApprovalCancelled { |
There was a problem hiding this comment.
Note: Test emitting event ApprovalCancelled on cancel_approval successfully called.
| ApprovalCancelled { | ||
| collection: T::CollectionId, | ||
| item: T::ItemId, | ||
| item: Option<T::ItemId>, |
There was a problem hiding this comment.
Note: Tested in:
- cancel_approval_works: https://github.com/r0gue-io/pop-node/pull/387/files#r1881283901
- cancel_collection_approval_works: cancel_collection_approval_works
| delegate.clone() | ||
| )); | ||
| assert_eq!(Balances::reserved_balance(&item_owner), 0); | ||
| assert!(events().contains(&Event::<Test>::ApprovalCancelled { |
There was a problem hiding this comment.
Note: Test emitting event ApprovalCancelled on cancel_collection_approval successfully called.
| CollectionApprovals::<T, I>::try_mutate_exists( | ||
| (&collection, &owner, &delegate), | ||
| |maybe_approval| -> DispatchResult { | ||
| let deposit_required = T::CollectionApprovalDeposit::get(); |
There was a problem hiding this comment.
Note: Reserve deposit CollectionApprovalDeposit on collection approval approved.
| // Key: `sizeof((CollectionId, AccountId, AccountId))` bytes. | ||
| // Value: `sizeof((Option<BlockNumber>, Balance))` bytes. | ||
| #[pallet::constant] | ||
| type CollectionApprovalDeposit: Get<DepositBalanceOf<Self, I>>; |
There was a problem hiding this comment.
Note: Used in
- do_approve_collection_transfer: https://github.com/r0gue-io/pop-node/pull/387/files#r1881288135
| assert_eq!( | ||
| events().last_chunk::<3>(), | ||
| Some(&[ | ||
| Event::TransferApproved { |
There was a problem hiding this comment.
Note: Approving multiple accounts emit TransferApproved events
| @@ -76,12 +77,11 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> { | |||
|
|
|||
| Self::deposit_event(Event::TransferApproved { | |||
| collection, | |||
| item, | |||
| item: Some(item), | |||
There was a problem hiding this comment.
Note: Tested in:
- approving_multiple_works: https://github.com/r0gue-io/pop-node/pull/387/files#r1881289915
- approve_collection_transfer_works: https://github.com/r0gue-io/pop-node/pull/387/files#r1881291415
| @@ -465,21 +498,31 @@ pub mod pallet { | |||
| /// a `delegate`. | |||
| TransferApproved { | |||
| collection: T::CollectionId, | |||
| item: T::ItemId, | |||
| item: Option<T::ItemId>, | |||
There was a problem hiding this comment.
Note: Tested in:
- approving_multiple_works: https://github.com/r0gue-io/pop-node/pull/387/files#r1881289915
- approve_collection_transfer_works: https://github.com/r0gue-io/pop-node/pull/387/files#r1881291415
| delegate.clone(), | ||
| None | ||
| )); | ||
| assert!(events().contains(&Event::<Test>::TransferApproved { |
There was a problem hiding this comment.
Note: Emitting event TransferApproved on approve_collection_transfer_works
| fn integrity_test() { | ||
| use core::any::TypeId; | ||
| assert!( | ||
| TypeId::of::<<T as Config<I>>::ItemId>() != TypeId::of::<u64>() && |
There was a problem hiding this comment.
Note: Context for this integration test is to make sure ItemId won't exceed the u32 type. If so, the overflow case can happen because the type used in AccountBalance is u32.
chungquantin
force-pushed
the
chungquantin/feat-nfts
branch
from
769f50a to
fafb988
Compare
|
My bad for not using
|
|
[sc-1606] |
There was a problem hiding this comment.
Left a few nitpicks and made a refactor myself to fasten the process a little bit: #405
| ); | ||
|
|
||
| // Force-approve unknown collection, throws error `Error::NoItemOwned`. | ||
| assert_noop!( |
There was a problem hiding this comment.
I don't see the point of testing this twice, it is against the whole purpose of merging the tests together?
chungquantin
force-pushed
the
chungquantin/feat-nfts
branch
from
07c9645 to
abc54e8
Compare
Edited: 09/12/2024
DESCRIPTION
This pull request introduces new storage items to the
pallet-nftsto 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 issizeof((CollectionId, AccountId, AccountId))bytes.CollectionApprovalDepositwill be reserved from the owner on collection approval created. The same amount of reservedCollectionApprovalDepositwill be unreserved back to the owner on collection approval cancelled.New storage items and related changes
AccountBalance: Keep track of the total number of collection items an account has. This storage item need to be updated on collection item transferred (fn do_transfer()), burnt (fn do_burn()) and minted (fn do_mint()).CollectionApprovals: Keep track of the collection approval status for a delegated account.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 depositCollectionApprovalDepositfrom theoriginaccount.force_approve_collection_transfer: Force-approve collection items owned by the specifiedownerto be transferred by a delegated third-party account. This function reserves the required depositCollectionApprovalDepositfrom theoriginaccount.cancel_collection_approval: Cancel one of the collection approvals.force_cancel_collection_approval: Force-cancel one of the collection approvals granted by the specifiedowneraccount. Returning the reserved funds to thedelegate.clear_all_collection_approvals: Cancel all the collection approvals. Returning the reserved funds to thedelegate.force_clear_all_collection_approvals: Force-cancel all the collection approvals granted by the specifiedowneraccount. Returning the reserved funds to thedelegate.check_collection_approval
Checks whether the
delegatehas the necessary allowance to transfer items in thecollectionthat are owned by theaccount.check_approval
Checks whether the
delegatehas the necessary allowance to transfer items within the collection or a specific item in the collection. If thedelegatehas an approval to transfer all items in the collection that are owned by theaccount, they can transfer every item without requiring explicit approval for that item.Item = NoneItem = Somedo_approve_collection_transfer
Store the new approval with deadline. Approving a
delegateto transfer items owned by the signedoriginin the collection will reserve some deposit amount (configured viaT::CollectionApprovalDeposit) from theorigin. 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
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
This function is used to clear
limitcollection approvals for the collection items ofowner. After clearing all approvals, the deposit of each collection approval is returned to theowneraccount and theApprovalsCancelledevent is emitted.Weight of this method is calculated by the provided
limit.do_destroy_collection
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_approvalsorcancel_collection_approval. These methods unreserve the deposited funds back to theoriginon called.Introducing new error types
NoItemOwned: Account owns zero item in the collection.DelegateApprovalConflict: Collection approval and item approval conflicts.do_cancel_approval()if there is an existing collection approval with key(collection, account, delegate).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.do_destroy_collection()if there are collection approvals. All collection approvals must be removed first before the method can be called.