centrifuge-chain icon indicating copy to clipboard operation
centrifuge-chain copied to clipboard

Consider dropping `nft-sales` in favour of `pallet_uniques`

Open NunoAlexandre opened this issue 2 years ago • 6 comments

Motivation

Since this PR, the pallet_uniques offers a set_price and a buy_item extrinsics to allow for NFT owners to set their NFTs up for sale for a given asking price and for potential buyers to buy said NFTs for said price. This functionality is exactly what we handcrafted with nft-sales.

Proposal / Consideration

Since nft-sales is essentially just duplicated logic, we can choose to adopt the official pallet_uniques API instead.

Pros

  • Less code to maintain
  • One less pallet in our runtime
  • Following community standards
  • Simpler solution since users don't need to give up ownership of their NFTs to the nft-sales pallet to sell their NFTs

Migration requirements

  • The frontend would need to switch to pulling NFTs for sale from the pallet_uniques
  • migration: active NFTs for sale on nft-sales would need to be removed and their ownership restored to the original owner

Questions

  • How many active NFTs for sale on nft-sales do we have in Kusama?

NunoAlexandre avatar Oct 05 '22 14:10 NunoAlexandre

@onnovisser it would be cool getting your thoughts on this regarding potential work that would need to happen in the frontend that we may not be considering now.

NunoAlexandre avatar Oct 05 '22 14:10 NunoAlexandre

The surface area of NFT sales is pretty small, so shouldn't need much rework. Is there an extrinsic to remove an nft from being listed?

onnovisser avatar Oct 05 '22 15:10 onnovisser

The surface area of NFT sales is pretty small, so shouldn't need much rework. Is there an extrinsic to remove an nft from being listed?

Well, we have a remove extrinsic but that would require every user with NFTs up for sale to call that extrinsic. An alternative would be for us to write a migration script that would basically do that automatically on_runtime_upgrade.

So we could have that and have the UI switch to adopt the pallet_uniques API. Users who had NFTs open for sale would need to call basically putting them up for sale again, which would then happen through pallet_uniques.

Wdyt?

NunoAlexandre avatar Oct 06 '22 13:10 NunoAlexandre

That sounds good to me. With my question, I meant to ask if the new pallet_uniques has an extrinsic to remove an item from sale, as users will need to still be able to do that in the new version, and it only mentions set_price and buy_item. Looking at the code though, it looks like I can call set_price with None to remove an item from sale.

onnovisser avatar Oct 10 '22 09:10 onnovisser

Yes, you can remove the price by setting it to None. Btw, feel free to consider waiting a bit and switching to the new "uniques-v2" pallet that would bring much more features and control over nfts

jsidorenko avatar Oct 12 '22 14:10 jsidorenko

Thanks for chipping in, @jsidorenko. @onnovisser let's wait for Uniques v2 then 👍

NunoAlexandre avatar Oct 12 '22 15:10 NunoAlexandre