pallet_nfts attributes not cleared after burning the nft

[darkfriend77]

Is there an existing issue?

• [X] I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

• [X] This is not a support question.

Motivation

Currently, when you burn an NFT the attributes of this NFT remain in the storage, which seems strange. Since there is no reference for those Attributes. This behavior seems also inconsistent with the item's metadata, which is cleared on a successful burn.

The current pallet, creates a very hard-to-maintain attribute storage, as there is no functionality given to properly clean the attribute references of a certain item, and a namespace, leading to unreferenced attributes filling the storage for no purpose.

Request

Clear all references in the attributes storage of the burned NFT.

Solution

Extend the do_burn function in the pallet_nfts with clear all attributes that reference the burned NFT. Similar to the Clear metadata.

In my opinion, the minimum is that do_burn has an option to allow clearing the attributes of the collection Owner if called by the collection owner

Are you willing to help with this request?

Yes!

[bkchr]

CC @jsidorenko

[jsidorenko]

Clearing the attributes is a tricky thing since the number of created attributes might simply be pretty big and not fit into one block once you decide to clear them in one go.

Right now, you can manually loop over the externally owned attributes and call cancel_item_attributes_approval() which will remove all the records in a provided namespace. Then do a batch call clear_attribute() to clear all the records in the personal namespace.

Let's explore the possible improvements: 1) the most obvious is to clear everything in one go, this could be an optional true/false param whether the user wants to do that + he would need to provide the number of attributes an item has. That number will be capped and the transaction will get reverted if we detect the real number of attributes is higher. 2) we can change the burning logic (like we recently did with the destroy() method to delete the collection) and allow to burn an item only when there are no associated attributes stored. This means a user would need to clean the attributes storage first and then call the burn() method. Obviously, all the calls could be batched.

I would be happy to hear about other possible approaches.

[darkfriend77]

Yep, it seems quite a challenge, to do a proper cleanup.

One of the major issues I see for the second option is that an Item owner might miss the privilege to clear attributes in certain namespaces, but the burn privilege is considered an owner right. This somehow eliminates option 2, as this would remove the burn privileges as it binds it to the ability to clear all attributes in all namespaces.

Correct me if I'm wrong, but my assumption is that an Item can have attributes set by the Collection Owner, and can be burnt by somebody who acquired it, which can't clear those attributes.

For option 1, I'm somehow troubled by the possibility that there is an Item, with too many attributes for one block as this will leave us burning the item, without clearing the attributes.

For this purpose, if an attribute has no existing item reference any user should be able to clear that attribute independent of the namespace, as an attribute without an item is considered an invalid entry. (It should even be rewarded for doing free garbaging)

I think also the relations between Collection and Item, are not similar to Item and Attribute, therefore having a different approach seems in my opinion legit.

Short summary:

• do_burn extent with option 1
• do_clear_attribute extend to not check privileges on invalid entries (attributes without item)

Benefit:

• we allow clean up on burn
• we allow manual clean-up, for leftovers

[darkfriend77]

@jsidorenko what is needed to move this forward?

[jsidorenko]

I will try to find time next week and tackle the issue

[jsidorenko]

Hi @darkfriend77 I started to think about the changes in the burn() method, and maybe, instead of modifying it, it'd be better to create a new extrinsic, smth like burn_with_attributes()? But after allowing anyone to clear those attributes for the deleted items, I'm starting to doubt whether we really need that new method. Basically, you could simply batch the burn() + clear_attribute() methods to achieve the same. What do you think?

[darkfriend77]

@jsidorenko Considering that a use case that mints and burns to wrapped NFTs as an example will inflate the storage with unlinked attributes, pretty fast, without cleaning the attributes. So keeping the unlinked attributes is not a sustainable option.

I agree that we can give it into the hand of the pallet (pallet using the interface), or the user itself (client logic), so we don't need to care for the too many attributes to clear in 1 block possibility. A Parachain could schedule a maintenance batch_call every day to clean up unlinked attributes, or just leave them till they create an issue.

Can any entity/account clear_attribute of burned/unlinked NFT, independent of the namespace, and independent of the depositing account, without getting a NoPermission or LockedCollectionAttributes?

• I very much vote for your suggested solution of creating a call burn_with_attributes(maxAttributes) It just makes it so much easier for pallets that are built on top of pallet_nfts and for the

• client's executing a batch burn() + clear_attribute(), will also increase the fee, which might be higher than the unlocked deposits. In addition, the client will have to call the storage, for that attribute, making a burn operation, quite complex, to get first all attributes in all namespaces.

• all deposits unlocked by a clear_attribute should go to the executing entity, independent of whether he was the depositing account or not. (makes sense to me)

Is there a use case where attributes need to remain on the chain when their NFT has been burned?

Let me know what you think.

[jsidorenko]

Can any entity/account clear_attribute of burned/unlinked NFT, independent of the namespace, and independent of the depositing account, without getting a NoPermission or LockedCollectionAttributes?

with the new proposed changes in #3064 you'll be able to clean the attributes from any namespace if the item is burned but only if the attributes were not locked before (btw, this lock applies to the CollectionOwner namespace only).

The idea behind disallowing to clean the locked attributes was to prevent the cases where I create a valuable NFT, set the metadata and attributes, lock it and then the owner decides to burn it. This gives me the ability to re-mint a new NFT with the same ID but with different attributes to myself and sell it...

Ghmm.. while writing this, I realised there was another problem that got resolved right before the release: it was possible to burn anyone's NFT and re-mint the same to another address, and probably that's why we've added that locking logic.

Let's think again: shall we allow the removal of the locked CollectionOwner attributes for destroyed items or not? What could be the concerns?

[jsidorenko]

What if we: 1) extend the traits functionality to allow other pallets to set/clear an attribute in any given namespace? 2) create a batch clear method/extrinsic that anyone will be able to call for free (if there are attributes to remove), but the deposit will get returned to the depositor and not the caller.

[darkfriend77]

1. Yes, that would allow pallets to take care of cleaning up, on burning an NFT. How would the usage of the trait be in terms of storage read and write operations, for the pallet that does the cleaning up? Since that has been a challenge so far. Could we have something like

fn clear_item_attributes(collection: &Self::CollectionId, item: &Self::ItemId, ns: Option<AttributeNamespace>) -> DispatchResult;

2. Offering this seems fine, even though there seems to be no incentive to execute it, except if you are the depositing account. But seems good as long the free, can be handled to not open a attack vector, and only executes on a cleared attribute, as you mention.

[jsidorenko]

Offering this seems fine, even though there seems to be no incentive to execute it, except if you are the depositing account. But seems good as long the free, can be handled to not open a attack vector, and only executes on a cleared attribute, as you mention.

You can build a relayer service, charge as per usage, and integrate it into some collection management UI.

[jsidorenko]

How would the usage of the trait be in terms of storage read and write operations, for the pallet that does the cleaning up?

I assume you would need to additionally provide the exact number of attributes you're willing to clean. For extrinsics, it's up to a caller to get that info from the storage; for pallets, that would probably require an additional storage read... We can also use CountedStorageNMap instead of StorageNMap for attributes.

[darkfriend77]

Currently not tightly coupling to the pallet_nft, will result in not being able to access the pallet NFT storage. Having a solution, that allows to inspect the count of attributes an item has in a certain namespace or overall, and a function to clear_attributes in a certain namespace or all, could help to keep the storage from inflating.

[jsidorenko]

Hi @darkfriend77 , as I no longer work at Parity, it's hard to find some free time to solve this issue. Maybe you could take it from here and submit a PR with the proposed fixes?