Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add EIP-6260: Buyable NFT tokens on-Chain and Royalties #6260

Closed
wants to merge 15 commits into from

Conversation

lth-elm
Copy link

@lth-elm lth-elm commented Jan 5, 2023

This EIP allows a non-fungible token to become buyable and to specifically enforce royalties as a percentage, directly on-chain, without entrusting a third-party. A decentralized marketplace can be built around this interface.

@lth-elm lth-elm requested a review from eth-bot as a code owner January 5, 2023 14:11
@github-actions github-actions bot added c-new Creates a brand new proposal s-draft This EIP is a Draft t-erc labels Jan 5, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Jan 5, 2023

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):


(fail) eip-6260.md

classification
newEIPFile
  • File with name EIPS/eip-6260.md is new and new files must be reviewed
  • This PR requires review from one of [@axic, @SamWilsn, @Pandapip1]

(pass) assets/eip-6260/contracts/ERC721Buyable.sol

classification
ambiguous
  • file assets/eip-6260/contracts/ERC721Buyable.sol is associated with EIP 6260; because there are also changes being made to EIPS/eip-6260.md all changes to corresponding assets are also allowed

(pass) assets/eip-6260/contracts/InterfaceChecker.sol

classification
ambiguous
  • file assets/eip-6260/contracts/InterfaceChecker.sol is associated with EIP 6260; because there are also changes being made to EIPS/eip-6260.md all changes to corresponding assets are also allowed

(pass) assets/eip-6260/contracts/NFTContract.sol

classification
ambiguous
  • file assets/eip-6260/contracts/NFTContract.sol is associated with EIP 6260; because there are also changes being made to EIPS/eip-6260.md all changes to corresponding assets are also allowed

(pass) assets/eip-6260/contracts/NFTContractNONBuyable.sol

classification
ambiguous
  • file assets/eip-6260/contracts/NFTContractNONBuyable.sol is associated with EIP 6260; because there are also changes being made to EIPS/eip-6260.md all changes to corresponding assets are also allowed

(pass) assets/eip-6260/contracts/graphs/ERC721Buyable.svg

classification
ambiguous
  • file assets/eip-6260/contracts/graphs/ERC721Buyable.svg is associated with EIP 6260; because there are also changes being made to EIPS/eip-6260.md all changes to corresponding assets are also allowed

(pass) assets/eip-6260/contracts/graphs/InheritanceERC721Buyable.svg

classification
ambiguous
  • file assets/eip-6260/contracts/graphs/InheritanceERC721Buyable.svg is associated with EIP 6260; because there are also changes being made to EIPS/eip-6260.md all changes to corresponding assets are also allowed

(pass) assets/eip-6260/contracts/graphs/InheritanceNFTContract.svg

classification
ambiguous
  • file assets/eip-6260/contracts/graphs/InheritanceNFTContract.svg is associated with EIP 6260; because there are also changes being made to EIPS/eip-6260.md all changes to corresponding assets are also allowed

(pass) assets/eip-6260/contracts/graphs/NFTContract.svg

classification
ambiguous
  • file assets/eip-6260/contracts/graphs/NFTContract.svg is associated with EIP 6260; because there are also changes being made to EIPS/eip-6260.md all changes to corresponding assets are also allowed

(pass) assets/eip-6260/contracts/interfaces/IERC721Buyable.sol

classification
ambiguous
  • file assets/eip-6260/contracts/interfaces/IERC721Buyable.sol is associated with EIP 6260; because there are also changes being made to EIPS/eip-6260.md all changes to corresponding assets are also allowed

(pass) assets/eip-6260/images/erc721buyable.png

classification
ambiguous
  • file assets/eip-6260/images/erc721buyable.png is associated with EIP 6260; because there are also changes being made to EIPS/eip-6260.md all changes to corresponding assets are also allowed

(pass) assets/eip-6260/images/marketplaces-issues.png

classification
ambiguous
  • file assets/eip-6260/images/marketplaces-issues.png is associated with EIP 6260; because there are also changes being made to EIPS/eip-6260.md all changes to corresponding assets are also allowed

(pass) assets/eip-6260/test/buyableTokenTest.js

classification
ambiguous
  • file assets/eip-6260/test/buyableTokenTest.js is associated with EIP 6260; because there are also changes being made to EIPS/eip-6260.md all changes to corresponding assets are also allowed

@lth-elm
Copy link
Author

lth-elm commented Jan 7, 2023

Hi @axic, @SamWilsn, @Pandapip1

I can't seem to understand why EIP walidator fail on the LICENSE url check while it does works perfectly fine when we check here.

Any idea ?

@numtel
Copy link

numtel commented Jan 10, 2023

Can you compare this to #6105 ?

@lth-elm
Copy link
Author

lth-elm commented Jan 10, 2023

Can you compare this to #6105 ?

I like this idea, could really change the game if it's adopted by most marketplaces unfortunately some will always continue to do it their way.

The key difference here is that the "marketplace" functions is directly within the nft smart-contract, the whole idea is to be able to sell your nfts without having to rely on any marketplace.

@lth-elm lth-elm dismissed a stale review via 2d4c5b5 January 10, 2023 09:57
@lth-elm lth-elm changed the title Add EIP-6212: Buyable NFT tokens on-Chain and Royalties Add EIP-6260: Buyable NFT tokens on-Chain and Royalties Jan 10, 2023
@lth-elm lth-elm closed this Jan 10, 2023
@github-actions github-actions bot added w-ci Waiting on CI to pass and removed c-new Creates a brand new proposal t-erc s-draft This EIP is a Draft labels Jan 10, 2023
@lth-elm lth-elm reopened this Jan 10, 2023
@github-actions github-actions bot added c-new Creates a brand new proposal s-draft This EIP is a Draft t-erc labels Jan 10, 2023
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Jan 10, 2023
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Jan 10, 2023
@numtel
Copy link

numtel commented Jan 10, 2023

The key difference here is that the "marketplace" functions is directly within the nft smart-contract, the whole idea is to be able to sell your nfts without having to rely on any marketplace.

That's exactly what #6105 does. The extension interface you propose is almost exactly the same:

  • setPrice -> listItem
  • removeTokenSale -> delistItem
  • buyToken -> buyItem

Also, why are you implementing a new royalty calculation format that is less extensible than EIP-2981?

@lth-elm
Copy link
Author

lth-elm commented Jan 11, 2023

Indeed, I had come accross one of your response and misinterpret the goal of the EIP. It does actually share the same idea.

Since nothing has been published on the ethereum-magicians forum I haven't seen any discussions associated with this kind of topic. However, I notice that you've been sharing quite divided thoughts under the pull request. If I can bring some context as to why we thought of this EIP I invite you to read this message posted on the forum. We've also provided materials under the assets folder.

As for the integration of the EIP-2981, we've thought about it but unfortunately it does not allow to update the % of royalties while we believe that it would be relevant to enable it, but only downwards.

@Lenoftawa
Copy link

As for the integration of the EIP-2981, we've thought about it but unfortunately it does not allow to update the % of royalties while we believe that it would be relevant to enable it, but only downwards.

Eip2981 did not allow the royalty to be changed. That was not considered a good thing to do after an NFT had been minted because it does not seem a fair thing to do to an owner of an NFT. It is effectively unilaterally modifying the contract.
So setting the royalty should happen before the first sale. That is something that could be added at mint time perhaps.
As for having a royalty that changed according to a schedule or value etc. That is precisely why 2981 does not return a percentage. The discussion was long and painful and lasted a year.
I would have less of a concern if the royalty info function in this spec had the same semantics as 2981. At least then it allows the implementation to support both models.
But I do believe that that this is equivalent to the earlier eip and it makes more sense to extend it to support any functions that you feel it is lacking rather than create an incomparable standard.

@Lenoftawa
Copy link

Since nothing has been published on the ethereum-magicians forum I haven't seen any discussions associated with this kind of topic. However, I notice that you've been sharing quite divided thoughts under the pull request.
I also find the comments process rather confusing. Few proposals are discussed in the wizards forum I find. But watching the eip repo, which is the only sure way of catching everything is also far from ideal as it generates a lot of noise. Not sure what the solution is but this is a real problem.

@5660-eth
Copy link
Contributor

As for the integration of the EIP-2981, we've thought about it but unfortunately it does not allow to update the % of royalties while we believe that it would be relevant to enable it, but only downwards.

Eip2981 did not allow the royalty to be changed. That was not considered a good thing to do after an NFT had been minted because it does not seem a fair thing to do to an owner of an NFT. It is effectively unilaterally modifying the contract. So setting the royalty should happen before the first sale. That is something that could be added at mint time perhaps. As for having a royalty that changed according to a schedule or value etc. That is precisely why 2981 does not return a percentage. The discussion was long and painful and lasted a year. I would have less of a concern if the royalty info function in this spec had the same semantics as 2981. At least then it allows the implementation to support both models. But I do believe that that this is equivalent to the earlier eip and it makes more sense to extend it to support any functions that you feel it is lacking rather than create an incomparable standard.

The method to change the royalty is already given in the following reference implementation.This is why we do not give the royalty changing method in EIP6105.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/common/ERC2981.sol
function _setDefaultRoyalty(address receiver, uint96 feeNumerator)

@SamWilsn SamWilsn closed this Feb 21, 2023
@SamWilsn SamWilsn reopened this Feb 21, 2023
@SamWilsn
Copy link
Contributor

Please move technical discussion to the discussions-to thread. Everything here will get hidden when this PR is merged.

@github-actions
Copy link

The commit 78b4537 (as a parent of cdc1cb4) contains errors.
Please inspect the Run Summary for details.

Using the `ERC` prefix for references to proposals with a `category` of `ERC`
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Feb 22, 2023
Referring people to 165 and using backticks for code in headings
Remove implementation detail
Moving paragraphs to `Motivation` and add technical summary in `Abstract`.
@github-actions
Copy link

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the w-stale Waiting on activity label Apr 28, 2023
@github-actions
Copy link

github-actions bot commented Jun 9, 2023

This pull request was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

@github-actions github-actions bot closed this Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new Creates a brand new proposal s-draft This EIP is a Draft t-erc w-stale Waiting on activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants