Pallet-Uniques: Privileged User Actions

[mustermeiszer]

Currently the pallet-uniques allow the following action for privileged accounts of a collection:

• The owner of an nft-class can destroy ALL items of this class
• The admin of an nft-class can burn ANY item of this class (Not if locked since #13054)
• The admin of an nft-class can transfer ANY item of this class (Not if locked since #13054, or frozen collection/item)
• The owner of an nft-class can set the MAX SUPPLY of this class
• The freezer of an nft-class can freeze ANY item of this class
• The freezer of an nft-class can freeze the WHOLE class

IMO for downstream users of this pallet as an implementor of the traits nonfungibles::* this has some negative impact about the assumption one will likely make:

• Owning an NFT means it can not arbitrarily be removed from your ownership

Maybe Solutions

• Destroying a collection is only possible if all items are owned by the owner again
• Lock or Hold as part of nonfungibles::Mutate & destroying a collection is not allowed if any item is still locked/on hold

[jsidorenko]

• I would suggest removing the ability to burn/transfer an item for the collection's admin. This means if I own an item, then no one could take it from me, maybe except for the governance decision (e.g. in case I'll get hacked and will be able to prove that).
• To destroy a collection, I think it would be better to validate the collection has 0 items left. We have batched transactions, so the owner could manually delete all his items in a batch.
• I don't think we need to add the ability to lock an item into the nonfungibles::Mutate trait, since the collection's freezer can easily revert that action. For external locking, it's better to utilise the Locker trait. Here is a possible implementation's example.

[jsidorenko]

P. S. I think it's better to add all those changes to the nfts pallet, while keeping the uniques pallet working in an old way to avoid breaking existing integrations

[mustermeiszer]

I would suggest removing the ability to burn/transfer an item for the collection's admin. This means if I own an item, then no one could take it from me, maybe except for the governance decision (e.g. in case I'll get hacked and will be able to prove that).

Sounds fine

To destroy a collection, I think it would be better to validate the collection has 0 items left. We have batched transactions, so the owner could manually delete all his items in a batch.

Also sounds fine. Especially, given the fact, that otherwise, ownership of an NFT means nothing with respect to representing some permanent ownership of something.

I don't think we need to add the ability to lock an item into the nonfungibles::Mutate trait, since the collection's freezer can easily revert that action. For external locking, it's better to utilise the Locker trait.

Are you referring to the trait Locker or the trait LockableNonfungible? The LockableNonfungible sounds like a good idea. But I think a querying method would be nice. E.g. is_locked() -> Result<bool, DispatchError>. Otherwise, it is hard to reason about the state of an NFT, as no locking information is exposed in the trait Inspect.

[jsidorenko]

I'm referring to the trait Locker, which has the is_locked() method. You can see the possible implementation here

[mustermeiszer]

Isn't it a bit confusing to have it refer to itself?

What is the advantage here over extending the trait LocakableNonfungible with fn is_locked() and implementing this on NFTs. Feels like locking an NFT is quite a standard procedure, and this would make it a lot easier for external integrations to check the locking state (if it is a "readable" field on the item struct).

[mustermeiszer]

Anyways, don't want to criticize the design here. My main reasoning here is to solve the behaviour of uniques/nfts with respect to ensuring locked/owned nfts are not removed by destroying a class.