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

EIP-2981: ERC-721 Royalty standard - Standardized means of accepting royalties for NFT marketplaces across the ecosystem #2981

Merged
merged 14 commits into from
Sep 22, 2020

Conversation

VexyCats
Copy link
Contributor

@VexyCats VexyCats commented Sep 15, 2020

@VexyCats
Copy link
Contributor Author

Hey @MicahZoltu - what am I missing that made this CI check fail? Anything look out of place to you? Says its failing on my last commit which was just changing some spacing on a code bracket

EIPS/EIP-1776.md Outdated
title: ERC-721 Royalty Standard
author: Zach Burks (@vexycats)
discussions-to: https://github.com/ethereum/EIPs/issues/2907
status: First Draft
Copy link
Contributor

@MicahZoltu MicahZoltu Sep 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
status: First Draft
status: Draft

@MicahZoltu
Copy link
Contributor

Also, you have several typos that the bot doesn't like:

./EIPS/EIP-1776.md:15: publically ==> publicly
./EIPS/EIP-1776.md:23: flexibilty ==> flexibility
./EIPS/EIP-1776.md:25: transfering ==> transferring
./EIPS/EIP-1776.md:34: recieving ==> receiving
./EIPS/EIP-1776.md:61: transfered ==> transferred
./EIPS/EIP-1776.md:76: recieve ==> receive
./EIPS/EIP-1776.md:78: transfered ==> transferred
./EIPS/EIP-1776.md:158: Transfering ==> Transferring

@VexyCats
Copy link
Contributor Author

Hey thanks! No spell check in my IDE ;D

Will go through and check everything for typos on an external program and resubmit!

(Didn't know typos affected the PR acceptance)

Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a bunch of feedback which I encourage you to consider, but for getting this merged as a Draft all that is required is that the EIP is structurally sound (meaning you have the right sections and the frontmatter is setup correctly).

Since I left some inline suggestions in here, I'll wait for you to indicate you want me to merge since once I merge you can't do one-click apply of the inline suggestions anymore.

EIPS/EIP-1776.md Outdated

**ERC-721 compliant contracts MAY implement this ERC for royalties to provide a standard method of accepting royalty payments and recieving royalty information**

The `_RoyaltyAmount` **MUST** be calculated as a percentage - such as "5" - for 5%. This is **REQUIRED** to maintain uniformity across the standard.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This limits minimum royalties to 1%. Recommend having a much lower minimum. One option is to have two fields, one for the numerator and one for the denominator. Another option is to just have this be a fixed point percentage (in the range 0 to 1) with a scaling factor of 10^18. A final option would be to have this be fixed point with a scaling factor of 10000 or something.

Note: Currently, this is value is a multiplier on the amount that is a fixed point value with scaling factor of 100.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I choose fixed point with scaling to 100 as all the other marketplaces with royalties do not let you go below 1%, with the average royalty percentage being 1-10%.

I'm not opposed to making it go lower. I'm not sure which would be best, a scaling factor of 10,000, or scaled to 10^18.

Most users would not need/select a percentage of 0.00005% though - as an NFT purchase is a onetime transaction and not like a general fee on all transactions on something like say uniswap.

Continuing to go through these and replying. Thank you for the feedback Michah!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used 10,000 as the scaling factor as I don't see any reason someone would want to go below that for an NFT royalty fee.

Im unsure if its a good move, as for adopting the standard a marketplace really needs to make sure its math is correct and it uses the 10000 scale factor, otherwise it might have a bad time..... I'd like an easier solution where it requires less math/attention to detail for implementation, but alas I don't know of what would be better

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The advantage of 10^18 is that it is very standard across Ethereum, so it is more likely that tools will "just work" with that then with some custom scaling factor. The advantage of 100 is that it is easy to just describe the value as a "percentage". Personally, I would just do 10^18 I think because I believe that 100 is way too limiting (I can imagine people wanting sub percentage royalties) and I think standardization is valuable. You aren't really saving anything by using 10,000 as all numbers in Solidity use the same number of bytes of storage (unless you are bit packing, which I don't think you are or should).

EIPS/EIP-1776.md Outdated Show resolved Hide resolved
EIPS/EIP-1776.md Outdated Show resolved Hide resolved
EIPS/EIP-1776.md Outdated Show resolved Hide resolved
EIPS/EIP-1776.md Outdated Show resolved Hide resolved
EIPS/EIP-1776.md Outdated Show resolved Hide resolved
EIPS/EIP-1776.md Outdated Show resolved Hide resolved
EIPS/EIP-1776.md Outdated Show resolved Hide resolved
EIPS/EIP-1776.md Outdated Show resolved Hide resolved
EIPS/EIP-1776.md Outdated Show resolved Hide resolved
EIPS/EIP-1776.md Outdated Show resolved Hide resolved
EIPS/EIP-1776.md Outdated Show resolved Hide resolved
EIPS/EIP-1776.md Outdated Show resolved Hide resolved
@VexyCats VexyCats changed the title EIP-1776: ERC-721 Royalty standard - Standardized means of accepting royalties for NFT marketplaces across the ecosystem EIP-2981: ERC-721 Royalty standard - Standardized means of accepting royalties for NFT marketplaces across the ecosystem Sep 17, 2020
@VexyCats
Copy link
Contributor Author

@MicahZoltu I think I've made all the changes - please feel free to merge the PR, unless you have any other feedback. As we get more discussions and thoughts on it can always propose changes

Thank you for your feedback so far!

@MicahZoltu MicahZoltu merged commit 00cf410 into ethereum:master Sep 22, 2020
tkstanczak pushed a commit to tkstanczak/EIPs that referenced this pull request Nov 7, 2020
…royalties for NFT marketplaces across the ecosystem (ethereum#2981)

A standardized way to handle royalty payments for ERC-721 tokens, including publicly viewable information and notification of payment event.
Arachnid pushed a commit to Arachnid/EIPs that referenced this pull request Mar 6, 2021
…royalties for NFT marketplaces across the ecosystem (ethereum#2981)

A standardized way to handle royalty payments for ERC-721 tokens, including publicly viewable information and notification of payment event.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants