Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
EIP-2981: ERC-721 Royalty standard - Standardized means of accepting royalties for NFT marketplaces across the ecosystem #2981
Changes from 11 commits
0feddf1
547f252
fbfdcd0
591270b
60f40de
0605e9a
3819b57
5bccaac
08b47b8
c771218
a4cc417
513a00c
52f795a
8462c8e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 of100
is that it is easy to just describe the value as a "percentage". Personally, I would just do10^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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By making this an interface, it will force you to write an interface specification rather than an implementation (see other comments throughout for details on this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes thank you. As mentioned above it will be converted to a contract as this was my implementation and interface probably shouldn't be included for the standard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the other way around? Interface standards should never have implementation details in the specification, though sometimes a reference implementation may be provided in the
## Implementation
section.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Private variables are implementation details and not part of an interface specification. They should not be included in the specification. It may be valuable to include a reference implementation down in the
## Implementations
section if desired.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will adjust thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constructors are an implementation detail and should not be part of the specification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger. Will adjust. This is my code of implementation - will remove this and clean it up to remove implementation leaving only specifics for the standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interface Specifications don't specify implementation details, only public interface definitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the tokenId missing from this? Is the idea that all NFTs that are part of a set have the same royalty amount?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No - I considered weighing the royalty per NFT - and there is a few pros and a few cons.
Overall the main reason to go towards a royalty per contract (all the NFTs in this one contract) was due to gas costs during minting.
Setting a royalty value per NFT works - but prohibits the ability to mint multiple NFTs in a single transaction - by increasing the state per NFT. I've redone the 721 standard for batch minting and can achieve upwards of 2,500 NFTs in a single transaction, but if royalties are set per NFT, it would be less than 40 per transaction, a factor of 25 less. Same goes for setting a value like URI instead of using a chronological URI pattern and baseURI. So when making this standard I was focused on ensuring that gas costs remain as low as possible.
I see the value prop of having a royalty per NFT, but there are downsides for the creator. If the creator wanted to set royalties for all their NFTs - in the long run of minting each NFT they would be paying more than if they were able to set the base royalty amount.
Thinking about it further - you could handle it the same way as a baseURI and (this is purely implementation) return the base royalty amount for all NFTs, or individual amount if its set.
Therefore the function could take a tokenId argument, and the returned value only needs to be a uint256, regardless if its a baseURI or not.
Okay - I've convinced myself lol. Will adjust changed and allow for tokenId input. Makes sense as it offers more flexibility, albeit costing more gas during implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think whether royalties are contract wide or not is an implementation detail. One could imagine a token contract where all tokens have the same royalty, in which case no matter what tokenID you pass in you get the same result, but updating it merely requires updating a single variable. Another token may have a default fee that can be overridden by individual tokens, and a third may have a fee rate per token.