Add `ERC2981` Implementation
https://eips.ethereum.org/EIPS/eip-2981
Should name it something like ERC721RoyaltyData and add to contracts/token/ERC721/royalty_data/. ERC721Royalty is more concise, but incorrectly implies that royalties are enforced.
Does it make sense to write separate contracts for ERC2981? I've made a few observations since I started looking into this one.
- The spec was written so ERC2981 can be applied to ERC721 and ERC1155 tokens. Writing an
ERC721RoyaltyDataandERC1155RoyaltyDatacontract may create a lot of duplicate code - Royalties can be stored either as a single royalty that is applied to all assets or as a mapping of token ids to royalties. It is possible to have both
royaltyandroyaltiesin the same storage library and simply useroyaltyifroyalties[tokenId]is unset, this is not gas optimal though.
function _royaltyInfo(uint256 tokenId, uint256 salePrice)
internal
view
virtual
returns (address, uint256)
{
if (!ERC721BaseStorage.layout().exists(tokenId)) {
revert ERC721RoyaltyDataInternal__TokenNotFound();
}
ERC721RoyaltyDataStorage.Layout storage l = ERC721RoyaltyDataStorage
.layout();
uint256 royalty = l.royalties[tokenId] > 0
? l.royalties[tokenId]
: l.royalty;
if (0 == royalty) {
revert ERC721RoyaltyDataInternal__RoyaltyNotSet();
}
uint256 royaltyAmount = (royalty * salePrice) / 10000;
return (l.receiver, royaltyAmount);
}
- Can you think of a good place to put it other than
ERC721/royalties/andERC1155/royalties/? - Gas inefficiency is okay if we provide a way for the developer to easily override the function to only check global royalty.
- Can you think of a good place to put it other than ERC721/royalties/ and ERC1155/royalties/?
utils seems appropriate given it's not specific to any token nor is it a token contract on its own, and someone may want to use it with their non-standard token contract.
- Gas inefficiency is okay if we provide a way for the developer to easily override the function to only check global royalty.
That's a good point, in fact, there may be other ways people want the royalty to be calculated, e.g. linear decay as a function of time.
For now, put it in contracts/token/common/ERC2981/. Might update that during review.
I was just thinking about this, looking at some of the other ERC's like ERC165, ERC173, etc they all live within their respective domains, only makes sense to follow that pattern here and add it to /token as you suggested.