solidstate-solidity icon indicating copy to clipboard operation
solidstate-solidity copied to clipboard

Add `ERC2981` Implementation

Open ItsNickBarry opened this issue 3 years ago • 6 comments

https://eips.ethereum.org/EIPS/eip-2981

ItsNickBarry avatar Sep 29 '22 15:09 ItsNickBarry

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.

ItsNickBarry avatar Sep 29 '22 16:09 ItsNickBarry

Does it make sense to write separate contracts for ERC2981? I've made a few observations since I started looking into this one.

  1. The spec was written so ERC2981 can be applied to ERC721 and ERC1155 tokens. Writing an ERC721RoyaltyData and ERC1155RoyaltyData contract may create a lot of duplicate code
  2. 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 royalty and royalties in the same storage library and simply use royalty if royalties[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);
}

0xCourtney avatar Oct 05 '22 03:10 0xCourtney

  1. Can you think of a good place to put it other than ERC721/royalties/ and ERC1155/royalties/?
  2. Gas inefficiency is okay if we provide a way for the developer to easily override the function to only check global royalty.

ItsNickBarry avatar Oct 05 '22 04:10 ItsNickBarry

  1. 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.

  1. 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.

0xCourtney avatar Oct 07 '22 20:10 0xCourtney

For now, put it in contracts/token/common/ERC2981/. Might update that during review.

ItsNickBarry avatar Oct 07 '22 21:10 ItsNickBarry

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.

0xCourtney avatar Oct 07 '22 21:10 0xCourtney