ethereum / EIPs

The Ethereum Improvement Proposal repository
https://eips.ethereum.org/
Creative Commons Zero v1.0 Universal
12.88k stars 5.27k forks source link

Discussion for EIP-2981: NFT Royalty Standard #2907

Closed VexyCats closed 2 years ago

VexyCats commented 4 years ago

TL;DR - ERC721s have a ton of creators and a few marketplaces, but no accepted means for transferring royalties from items being sold multiple times on secondary sales. This EIP is proposing a standard method that can be implemented by all marketplaces easily.

// SPDX-License-Identifier: MIT

pragma solidity ^0.6.0;

/**
 * @dev Interface of the ERC165 standard, as defined in the
 * https://eips.ethereum.org/EIPS/eip-165[EIP].
 *
 * Implementers can declare support of contract interfaces, which can then be
 * queried by others ({ERC165Checker}).
 *
 * For an implementation, see {ERC165}.
 */
interface IERC721Royalties {

    /**
    @notice This event is emitted when royalties are received.

    @dev The marketplace would call royaltiesRecieved() function so that the NFT contracts emits this event.

    @param creator The original creator of the NFT entitled to the royalties
    @param buyer The person buying the NFT on a secondary sale
    @param amount The amount being paid to the creator
  */
    event RecievedRoyalties (address indexed creator, address indexed buyer, uint256 indexed amount);

    /**
     * @dev Returns true if this contract implements the interface defined by
     * `interfaceId`. See the corresponding
     * https://eips.ethereum.org/EIPS/eip-165#how-interfaces-are-identified[EIP section]
     * to learn more about how these ids are created.
     *
     * This function call must use less than 30 000 gas.
     */
    function supportsInterface(bytes4 interfaceId) external view returns (bool);

     /**
     * @dev Returns true if implemented
     * 
     * @dev this is how the marketplace can see if the contract has royalties, other than using the supportsInterface() call.
     */
    function hasRoyalties() external view returns (bool);

     /**
     * @dev Returns uint256 of the amount of percentage the royalty is set to. For example, if 1%, would return "1", if 50%, would return "50"
     * 
     * @dev Marketplaces would need to call this during the purchase function of their marketplace - and then implement the transfer of that amount on their end
     */
    function royaltyAmount() external view returns (uint256);

      /**
     * @dev Returns royalty amount as uint256 and address where royalties should go. 
     * 
     * @dev Marketplaces would need to call this during the purchase function of their marketplace - and then implement the transfer of that amount on their end
     */
    function royaltyInfo() external view returns (uint256, address);

      /**
     * @dev Called by the marketplace after the transfer of royalties has happened, so that the contract has a record 
     * @dev emits RecievedRoyalties event;
     * 
     * @param _creator The original creator of the NFT entitled to the royalties
     * @param _buyer The person buying the NFT on a secondary sale
     * @param _amount The amount being paid to the creator
     */
    function royaltiesRecieved(address _creator, address _buyer, uint256 _amount) external view;
}

Flow: (just suggestions, can be implemented however you like)

Constructor/deployment

Creator - the person who gets the royalties for secondary sales is set. Royalty Amount - the percentage amount that the creator gets on each sale, is set.

NFT sold on marketplace

Marketplace checks if the NFT being sold has royalties implemented - if so, call royaltyInfo() to get the amount and the creator's address.

Calculates the amount needed to be transferred and then executes that transfer.

Calls royaltiesRecieved() so that the NFT contract has a record of receiving the funds during a sale.


Thoughts? Anything that should be added or removed to make it easier to be implemented?

The logical code looks something like this:

abstract contract Royalties {
    /*
     * bytes4(keccak256('supportsInterface(bytes4)')) == 0x01ffc9a7
     */
    uint256 private royalty_amount;
    address private creator;

    /**
    @notice This event is emitted when royalties are transfered.

    @dev The marketplace would emit this event from their contracts. Or they would call royaltiesRecieved() function.

    @param creator The original creator of the NFT entitled to the royalties
    @param buyer The person buying the NFT on a secondary sale
    @param amount The amount being paid to the creator
  */

  event RecievedRoyalties (address indexed creator, address indexed buyer, uint256 indexed amount);
    constructor (uint256 _amount, address _creator) internal {
        royalty_amount = _amount;
        creator = _creator;
    }

    function hasRoyalties() public pure returns (bool) {
        return true;
    }

    function royaltyAmount() public view returns (uint256) {
        return royalty_amount;
    }

    function royaltyInfo() external view returns (uint256, address){
        return (royalty_amount, creator);
    }

    function royaltiesRecieved(address _creator, address _buyer, uint256 _amount) external {
        emit RecievedRoyalties(_creator, _buyer, _amount);
    }
}
Lenoftawa commented 3 years ago

@wighawag Yea. That was a very messy paragraph. Does my concern make any sense?

All works fine if the units for _value and _royaltyAmount are the same. If we don't have the units in the signature, we must mandate it in the spec.

dievardump commented 3 years ago

But how can they be different ?

"Someone is selling for 100000000, how much from it do you want and to whom?" "I want 1000000 to 0xdEaD"

even if we passed the unit, that would be used probably only to decide how much %age they want, but the unit returned will always be the same. If the 100000000 were in Doge, Matic, ETH, BTC, is of no concern, the value they will receive is of this currency that is used to buy the NFT, it can't really be something else.

It's very unliekely that a royaltyInfo will ever be able to say "No I don't want your token, give me USDC in place as a royalty payment". That will never be accepted by anyone. At least I'm sure that as a Marketplace I'm never going to support something like this.

Lenoftawa commented 3 years ago

@dievardump Ah, that does explain the impasse somewhat!

So you are saying that it all works fine as long as the NFT does not peg its royalty calculation to a unit of account like USD or ETH.

I thought this capability was mentioned as another advantage of passing in the value.

It does mean that things like "pay me a 1 USD worth every time this NFT changes hands" are off the cards. But that's OK for now.

So all works fine as long as:

In that case, I'm wondering if we need to make statements here about what a minimal Receiver needs to be capable of. As well as what the marketplace can and cannot do under this EIP. The standard expects the NFT to present a Receiver to the marketplace, so it follows that it should have some understanding of what marketplace is going to do with it.

I know we agreed that payment is out of scope, but I see now, that we have opened the issue by making the Receiver address part of the ERC2981 interface.

By not specifying these expectations, can this EIP be usefully deployed? Or do we need to wait until the payment EIP is done?

seibelj commented 3 years ago

Updated the EIP, but auto-merging seems to be broken or disabled now. @VexyCats @jamesmorgan @blmalone can someone approve and merge

The PR with changes: https://github.com/ethereum/EIPs/pull/3555

seibelj commented 3 years ago

Thank you - it is merged now. Everyone please review closely and provide feedback https://eips.ethereum.org/EIPS/eip-2981

I already see a bug in the example constructor - returns Royalties(royalty_amount, msg.sender) { I will remove this

dievardump commented 3 years ago

Thank you - it is merged now. Everyone please review closely and provide feedback https://eips.ethereum.org/EIPS/eip-2981

bytes4(keccak256("royaltyInfo(uint256,uint256,bytes)")) == 0xc155531d

in the signature for the interface

I would also change _INTERFACE_ID_ERC721ROYALTIES since this work as much for 721 as for 1155

I'm not sure I can do a PR or if only a specific list of people can

seibelj commented 3 years ago

@dievardump correct - I pasted wrong from my solidity editor

"c155531d": "royaltyInfo(uint256,uint256,bytes)"

Will fix

And I will rename to _INTERFACE_ID_ERC_721_1155_ROYALTIES

Lenoftawa commented 3 years ago

I would also change _INTERFACE_ID_ERC721ROYALTIES since this work as much for 721 as for 1155

I am probably the last person who should give advice on naming, but looking at the pattern already set by ERC721, it should just be _INTERFACE_ID_ERC2981.

        // register the supported interfaces to conform to ERC721 via ERC165
        _registerInterface(_INTERFACE_ID_ERC721); <------------------------------------------------------
        _registerInterface(_INTERFACE_ID_ERC721_METADATA);
        _registerInterface(_INTERFACE_ID_ERC721_ENUMERABLE);
        // Royalties interface 
        _registerInterface(_INTERFACE_ID_ERC721ROYALTIES);

I had the a similar thought when looking at the text. We mention ERC721 quite a lot. But on reflection, this standard appears to be completely agnostic to the underlying contract. ERC721 (and derived) tokens are the primary use case now, but if some DAO wanted to use it, there is nothing that I can see that would stop it. And if someone improves on ERC721 in a way that breaks it (there are EIPs that propose this), it would make good sense for implementers to add ERC2981 capability.

Lenoftawa commented 3 years ago

Good stuff @seibelj

I have reviewed and have created quite a few proposed text changes. Github is not something I'm particularly familiar with so, correct me if I'm not following process or good practice.

I have created it as draft PR.

If it is better to create a lot of small proposals I can re-submit in chunks. Here's a summary:

dievardump commented 3 years ago

I'm currently implementing this in a marketplace contract, and I came to the questions:

From my point of view, it should be accepted. I can definitely think of cases where I would want part of the price to be burned. But I'm not really sure about the general consensus. Should we add something in the EIP itself for this case?

For me if royaltyAmount is 0 or more than value, I don't process royalties. Else, even if the recipient is address(0) I will send.

Added statement that marketplaces cannot trust the royalty amount if the extended data field is used by and NFT and the marketplace is not supporting an extension that standardizes that data.

Do you mean if there is extra data the marketplace shouldn't send any royalties if they didn't introspect for all and every possible extensions that will come to this EIP?!

seibelj commented 3 years ago

I'm currently implementing this in a marketplace contract, and I came to the questions:

  • Do I check if "receiver" is address(0) or not? (and therefore do we accept address(0) as receiver)

From my point of view, it should be accepted. I can definitely think of cases where I would want part of the price to be burned. But I'm not really sure about the general consensus. Should we add something in the EIP itself for this case?

For me if royaltyAmount is 0 or more than value, I don't process royalties. Else, even if the recipient is address(0) I will send.

If royalty amount is 0, then you don't care where it goes. If > 0 and going to 0x0, yes it would be burned.

Added statement that marketplaces cannot trust the royalty amount if the extended data field is used by and NFT and the marketplace is not supporting an extension that standardizes that data.

Do you mean if there is extra data the marketplace shouldn't send any royalties if they didn't introspect for all and every possible extensions that will come to this EIP?!

I would treat it as undefined behavior. If a contract specifically did not want to support 2189 and only a superior EIP, they would not signal support using 165 for 2189.

Lenoftawa commented 3 years ago

@dievardump Here's the wording in the PR to see if it is better:

Marketplaces that support this standard MUST NOT pay royalties to NFTs that return data in _royaltyPaymentData unless they also support an extension EIP that defines the semantics of this data.

and

Marketplaces will need to use ERC-165 introspection to check whether NFTs support the ERC-2981 interface, and revert to their proprietary methods (where possible) if they do not.

We have said that under ERC2981, this field is reserved for future use (must be unused). We need to define what 'unused' means.

If a marketplace only knows about ERC2981 and NFT signals it supports only ERC2981, and the marketplace sees data in that field, then it can only assume:

@seibelj

I would treat it as undefined behavior. If a contract specifically did not want to support 2189 and only a superior EIP, they would not signal support using 165 for 2189.

We need to go further. I don't think any standard should say that behaviour is 'undefined'. Doing so leaves the door open for different implementers to use it in proprietary ways... and then it's non-standard. RFU is different. It must be left unused (however we define unused).

dievardump commented 3 years ago

We need to go further. I don't think any standard should say that behaviour is 'undefined'. Doing so leaves the door open for different implementers to use it in proprietary ways... and then it's non-standard. RFU is different. It must be left unused (however we define unused).

Contracts declare what interface they support in erc165. So contracts will declare support for 2981 and extensions.

So if it supports 2981 and an extension, it will be able to detect for which one the contract is calling, using the data field that will help for identification.

a contract calling with royaltyInfo(tokenId, amount, '') will only be calling for 2981. And it will only call after introspecting that 2981 is supported. a contract calling with royaltyInfo(tokenId, amount, '0x12') will only be calling for an extension. And it will only call after introspecting for that specific extension support.

So there shouldn't be any need for mentioning this.

Lenoftawa commented 3 years ago

@dievardump Ah! I see we are talking about different things. I'll need to make that clearer.

You are correct for the input data. I am referring to the NFT returning extra data.

If the market place is not aware of the extended EIP then it does not know what interface to introspect for or what to do if it found it.

So if this marketplace receives extra data, it knows that the NFT is signaling something, but has no way of finding out what it all means.

Does that make sense?

dievardump commented 3 years ago

So if this marketplace receives extra data, it knows that the NFT is signaling something, but has no way of finding out what it all means.

The only way the NFT contract will return data is if the marketplace sent data with the call to the function, because else it will consider the call being for 2981 and just return (recipient, amount, '')

no data in, no data out.

data in only if there was introspection to make sure the NFT supports the extension that goes with the data, then maybe data out, it depends on what the extension says.

That's how I understand the data and possible extensions. Any call without data in will be considerer 2981 because there is no way for the NFT contract to know what extension is called else (except if it doesn't support 2981, but then the marketplace knows why it calls it)

Lenoftawa commented 3 years ago

The only way the NFT contract will return data is if the marketplace sent data with the call to the function

Hmmm. I did not see any relationship between the input and output data. The contract should be negotiated through 165 not through the presence of data in these fields.

data in only if there was introspection to make sure the NFT supports the extension that goes with the data, then maybe data out, it depends on what the extension says.

I think I agree with this. And as a result, unless both parties are using an extension (signaled through 165) neither of these fields should be used. And if either uses these fields without both understanding the extension, then that is non-standard.

wighawag commented 3 years ago

Here are my comment on the current draft.

dievardump commented 3 years ago

Hmmm. I did not see any relationship between the input and output data. The contract should be negotiated through 165 not through the presence of data in these fields.

Yes it should. However there are 3 cases:

so in all cases, the data will be empty for 2981 calls and the marketplaces will know if to expect data or not. But after introspection, it can consider all calls to royaltyInfo(tokenId, amount, '') to be valid, since empty data will mean either it's for 2981 either the NFT doesn't support 2981 and the marketplaces knows it

Lenoftawa commented 3 years ago

@wighawag Agree with first two points but surprised by the last few words:

...it is up to the contract implementing EIP-2981 to not use it for no-reason and as @dievardump mention, the future extension will use the input data as a signal mechanism...

So, in your view the extra calling data exists as a substitute for ERC165? I'm guessing that's because we are assuming that extensions will be limited to the existing function and therefore be indistinguishable under ERC165?

It sounds like this is the protocol you are suggesting:

Case A: The marketplace only implements vanilla ERC2981 so it sends no data:

  1. If the NFT only implements vanilla ERC2981 then it is oblivious to the extra data and serves vanilla ERC2981 data.
  2. If the NFT implements ERC2981+ but sees that the marketplace is not using ERC2981+, then it serves vanilla ERC2981 data

Case B: The marketplace declares that it implements ERC2981+ by including some extra data:

  1. If the NFT only implements vanilla ERC2981 - what should it do?
    Just ignore the extra data and serve up vanilla ERC2981 data. The marketplace will see the empty returned data and know that the NFT only implements vanilla ERC2981
  2. If the NFT implements ERC2981+ then it serves the ERC2981+ data

This protocol does place constraints on the participants, even in ERC2981. So it should be recorded. Does this work:

"An EIP-2981 compliant marketplace MUST leave the _data argument 'empty'."

"The contract implementing the ERC-2981 MUST ignore the contents of _data argument"

"The contract implementing ERC-2981 MUST leave the _royaltyPaymentData return value 'empty'"

"An EIP-2981 compliant marketplace MUST ignore the contents of the _royaltyPaymentData return value.

(Where 'empty' must be formally defined).

Extensions to this standard may relax these rules in a way that it backward compatible.

Lenoftawa commented 3 years ago

Hmmm. I did not see any relationship between the input and output data. The contract should be negotiated through 165 not through the presence of data in these fields.

Yes it should.

I was referring to using 165 to determine whether there was an extension as well as whether the NFT supports 2981. But that does require that the extension has more functions, otherwise the interface hash will be the same.

However there are 3 cases:

  • nft contract supports only 2981: it will never return data since 2981 doesn't return any

Agree.

  • nft contract supports 2981 and at least one of the other extensions: extensions will have to have "data" set if it's a call for something else than 2981, because else the nft contract can't know which one the marketplace reach out for

This is an assumption about how extensions will work, which is fine.

If we accept this, we need to define how a 2981 NFT behaves when a marketplace sends extra data that it is not expecting. This can happen when the because it is aware of an extension that the NFT has not implemented.

  • nft contract support an extension and not 2981: then it knows the marketplaces calls for something else than 2981 because the marketplace must have done introspection before calling

I don't understand this, in my mind and NFT can only support an extension of 2981 if it already supports 2981.

VexyCats commented 3 years ago

Looking over the current EIP it seems acceptable. There is more we may need to discuss as @lenifoti brought up related to sending extra data that the NFT is not expecting in the data field. But as it stands this is a good ground to let the standard be built on top of in the future with the data field.

I think we can look to other functions such as safeTransferFrom for handling the data argument

As for the naming - it should be _registerInterface(_INTERFACE_ID_ERC2981) and not ERC721_ERC1155

seibelj commented 3 years ago

I will incorporate all the feedback by Friday morning so during the weekend people can review it whilst they sip their coffee.

dievardump commented 3 years ago

If we accept this, we need to define how a 2981 NFT behaves when a marketplace sends extra data that it is not expecting. This can happen when the because it is aware of an extension that the NFT has not implemented.

I think this shouldn't be possible. Extensions must be made so there is absolutely no confusion possible with the data value. I though it was an obvious requirement else there can be mix up.

I don't understand this, in my mind and NFT can only support an extension of 2981 if it already supports 2981.

yes you are right I exchanged marketplace and NFT: if the marketplace doesn't want to support 2981 but only extension, then it doesn't call except if extension is registered.

I would add that with ERC165, it is possible for NFT contract to only declare supporting this or that interface. There is no problem for it to say "I don't support basic 2981 but only this or that extension of 2981"

So, in your view the extra calling data exists as a substitute for ERC165? I'm guessing that's because we are assuming that extensions will be limited to the existing function and therefore be indistinguishable under ERC165?

No. He means it will be used on top of ERC165, to define what extension (or not) the royaltyInfo call is directed to.

Before calling royaltyInfo it will be mandatory for the marketplace to do ERC165 introspection. Which means if it calls royaltyInfo, it already knows with what parameters.

Case B: The marketplace declares that it implements ERC2981+ by including some extra data:

1. If the NFT only implements vanilla ERC2981 -  what should it do?
   Just ignore the extra data and serve up vanilla ERC2981 data.
   The marketplace will see the empty returned data and know that the NFT only implements vanilla ERC2981

2. If the NFT implements ERC2981+ then it serves the ERC2981+ data
...

I think you complicate things.

Marketplaces will ALWAYS introspect before calling. they have to to know that royaltyInfo is supported.

They will first introspect on the specific extension they want:

This means that the marketplace, with the use of data (empty or not), actually declares "I introspected, and this call correspond to this specific ERC"

seibelj commented 3 years ago

Even if function signature(s) match an interface, if supportsInterface() returns false for the requested interface ID, then it is not supported. So it doesn't matter if 2981 is technically supported by the functions - if the contract doesn't signal support, then it isn't supported.

If we have 2981, 3500, and 3600, with 3500 and 3600 building off of 2981, the contract just returns false for supportsInterface(_INTERFACE_ID_ERC2981) and it is not supported.

Lenoftawa commented 3 years ago

@dievardump

So, in your view the extra calling data exists as a substitute for ERC165? I'm guessing that's because we are assuming that extensions will be limited to the existing function and therefore be indistinguishable under ERC165?

No. He means it will be used on top of ERC165, to define what extension (or not) the royaltyInfo call is directed to.

Correct, this is what I understood. I was talking about signaling extensions.

I think you complicate things.

Marketplaces will ALWAYS introspect before calling. they have to to know that royaltyInfo is supported.

They will first introspect on the specific extension they want:

Absolutely, I was not suggesting that a marketplace would bypass ERC165 for checking for 2981. I was talking about how any extension was signaled. This is where it seemed that an extra protocol was being proposed - one that did not use ERC165, but one where the marketplace used extra data to probe the NFT. This works with the protocol that I described.

wighawag commented 3 years ago

ERC165 only make sense if there is interface. the hash is constructed from the interface function signature. While some EIP-1981 extension might need extra function, it is definitely possible that some don't

What we need is to ensure flexibility so extension can be built. I don't think we need to strictly define how the extension are going to work. It will be in their best interest to be simple and non-conflicting.

What is I think important is that any extension need to be backward compatible with EIP-2981 but this is obvious since they are "extension"

With that in mind:

Case A: The marketplace only implements vanilla ERC2981 so it sends no data: case A1: the token contract does not support any extension: simple case: token contract return no extra data

case A2: the token contract support an extension as no extra data was given in input, the token contract will not add extra return data. it behave as normal EIP-2981. There is probably no harm to return something as the marketplace will not use it. but the token contract to be fully compatible eith EIP-2981 will need to assume that the other data (receiver, royaltyAmount) will be interpreted as specified by EIP-2981. Cannot assume the marketplace will use the extra data to interpret them differently

Case B: The marketplace declares that it implements ERC2981+ an extension by including some extra data:

case B1: the token contract does not support any extension: no data is returned, the marketplace have to assume, that the normal return fields (receiver, royaltyamount) need to be interpreted as raw EIP-2981

case B2: the token contract support the extension signaled by the input data that the marketplace added the token contract will add a signal in the extra data returned and can assume the marketplace will consider it. In that case the royaltyAmount and receiver might be interpreted differently

case B3: the token contract support other extension but not the one signaled by the markerplace this is the same as B1

Unless there is a need for extra function ERC165 does not need to be invoked here. This is why we are adding input data and output data as part of EIP-1981. This is in my opinion more elegant

Lenoftawa commented 3 years ago

Cheers @wighawag. This is in line with my understanding. In particular:

What is I think important is that any extension need to be backward compatible with EIP-2981 but this is obvious since they are "extension"

Agreed, extensions should support fallback to vanilla ERC2981 if one party does not support the extension.

In effect, we are making this EIP future compatible by specifying how extensions must be signaled through extra data, and how ERC2981-compliant NFTs and marketplaces need to behave in order to make this signaling work. It's too late to specify this in extension EIPs after NFTs have been created.

case B3: the token contract support other extension but not the one signaled by the marketplace this is the same as B1

Good catch.

Lenoftawa commented 3 years ago

@seibelj

If we have 2981, 3500, and 3600, with 3500 and 3600 building off of 2981, the contract just returns false for supportsInterface(_INTERFACE_ID_ERC2981) and it is not supported.

If the extension 3500 and 3600 simply add rules about how the extra data is used, and do not add new functions. Then INTERFACE_ID_ERC2981 == INTERFACE_ID_ERC3500 == INTERFACE_ID_ERC3600. The ids are the hash of the function signature which is the same for all. There is no way to tell the difference.

That's where I understood the extra data signaling proposal came from.

Also, I don't think that it makes good sense to allow extensions that do not also support ERC2981 as a fallback. NFTs will be around for a long time and marketplaces should pay these royalties, even as standards evolve.

cliffhall commented 3 years ago

I feel the example on the EIP is confusing:

constructor (string memory name, string memory symbol, string memory baseURI)  public  Royalties(royalty_amount, msg.sender) {
        _name = name;
        _symbol = symbol;
        _setBaseURI(baseURI);
        // register the supported interfaces to conform to ERC721 via ERC165
        _registerInterface(_INTERFACE_ID_ERC721);
        _registerInterface(_INTERFACE_ID_ERC721_METADATA);
        _registerInterface(_INTERFACE_ID_ERC721_ENUMERABLE);
        // Royalties interface 
        _registerInterface(_INTERFACE_ID_ERC721ROYALTIES);
    }

Presumably we have extended a contract called Royalties and are passing it some constructor values? If so, it isn't mentioned anywhere else on the page. If the super contract is just notional, why not just make a simpler example that does everything in the same constructor:

constructor (string memory _name, string memory _symbol, string memory _baseURI, uint256 _royaltyAmount) {
        name = _name;
        symbol = _symbol;
        _setBaseURI(_baseURI);
        royaltyAmount = _royaltyAmount
        // register the supported interfaces to conform to ERC721 via ERC165
        _registerInterface(_INTERFACE_ID_ERC721);
        _registerInterface(_INTERFACE_ID_ERC721_METADATA);
        _registerInterface(_INTERFACE_ID_ERC721_ENUMERABLE);
        // Royalties interface 
        _registerInterface(_INTERFACE_ID_ERC2981);
    }

When I read the example, I get immediately hung up on whether there's some Royalties contract that I'm supposed to be aware of and what we're inheriting from it. IMHO, it adds friction to the review of what basic steps are required to implement.

cliffhall commented 3 years ago

@lenifoti Related to your outstanding PR, one more little suggestion:

In the interface code:

pragma solidity ^0.6.0;
import "./ERC165.sol";

///
/// @dev Implementation of royalties for 721s
///
interface IERC2981 is ERC165 {
    /// ERC165 bytes to add to interface array - set in parent contract
    /// implementing this standard
    /// ...
}

We should change the Implementation of royalties for 721s to Interface for the Royalty Signaling Standard

pragma solidity ^0.6.0;
import "./ERC165.sol";

///
/// @dev Interface for the Royalty Signaling Standard
///
interface IERC2981 is ERC165 {
    /// ERC165 bytes to add to interface array - set in parent contract
    /// implementing this standard
    /// ...
}
seibelj commented 3 years ago

Updated PR here https://github.com/ethereum/EIPs/pull/3575

@blmalone @VexyCats @jamesmorgan auto-merge is still disabled, need a merge approval

Changes

I am leaving it focused on NFT royalty payments, but reworded it to support both 721 and 1155. I added a new Rationale section about (potential) universal royalty support for other asset types, but given this EIP requires a tokenId (or something similar) and we have not discussed at all the concept of royalties outside of the NFT universe we live in, I do not think making that a primary focus is a wise decision. So it is in there, but not the focus.

Lenoftawa commented 3 years ago

Thanks @seibelj Looks good.

I don't see anything that contradicts my understanding, but there was wording in the original that I think can still be improved.

I also think that there are still areas which are open to interpretations that we want to address. We need to close them with MUST and or MUST NOT statements. For example, the sale price, the value passed to the NFT and the royalty payment, must be in the same unit of exchange.

I also had suggestions on other parts of the text that have been discussed with @cliffhall. I will suggest them separately once this PR has been merged.

jamesmorgan commented 3 years ago

Hi all, what state do we think this EIP is in, is it ready to move to a final call or the next stage of the EIP process?

cliffhall commented 3 years ago

I just noticed a couple of link markup fails:

Screen Shot 2021-05-21 at 3 33 57 PM Screen Shot 2021-05-21 at 3 34 50 PM
cliffhall commented 3 years ago

Also, here's a line that I think needs to be removed as it referred to a previous implementation example:

Screen Shot 2021-05-21 at 3 37 54 PM
cliffhall commented 3 years ago

Also, I think it would be reasonable to reformat the long method signature in the interface, considering the comments are formatted to short lines:

Screen Shot 2021-05-21 at 3 48 20 PM
function royaltyInfo(uint256 _tokenId, uint256 _value, bytes calldata _data) 
external 
returns (address _receiver, uint256 _royaltyAmount, bytes memory _royaltyPaymentData);
seibelj commented 3 years ago

Sorry have been extraordinarily busy the past week. Will get an updated version out with all comments tonight.

seibelj commented 3 years ago

Hi all, what state do we think this EIP is in, is it ready to move to a final call or the next stage of the EIP process?

I will make the final changes but everything after this last round is polish and perfection. Next round should be after the latest merge IMO.

seibelj commented 3 years ago

Final PR open https://github.com/ethereum/EIPs/pull/3591

@blmalone @jamesmorgan @VexyCats for šŸ‘

cliffhall commented 3 years ago

From this PR:

Implementers of this standard MUST require that the sale price, the value passed to royaltyInfo(), and the royalty payment must always be in the same unit of exchange.

This is kind of a thorny bit. Implementers can't enforce this in code, and it would be difficult for a contract to figure out what unit of exchange the original contract wants without another call that gets the required unit of exchange.

Imagine OpenSea implements the standard as it stands after merging #3591. A contract wants creator royalties in ETH, but the seller on OpenSea has asked for $WHALE. OpenSea now needs to check with the original contract to figure out what amount to send into royaltyInfo. It has no idea what the original contract wants creators to be paid in.

That makes it a two step process:

It will be onerous and error prone to expect OpenSea and others adopting the standard to handle that first step "out-of-band". It seems like the standard needs this extra step.

If royalties were expected to be percentage based, this problem would not exist. But since it is not (for good reasons), this issue will probably represent friction to adoption.

From an implementer's perspective, last week, I realized that the creator's contract could return 100% of the sale amount if it wanted to, so I can't just automatically pay the amount returned. I added a maximumRoyaltyPercentage in the system that's checking so that if a royalty amount comes back that is greater than the platform policy, e.g., "we won't pay more than 50%" then we just pay that capped amount. I now realize that's going to be a good hedge against a creator contract returning a fixed amount of $WHALE as the royalty, when I am sending in an ETH value. At least a mismatch of the two will not lead to our contract paying an inordinate royalty.

seibelj commented 3 years ago

This is kind of a thorny bit. Implementers can't enforce this in code, and it would be difficult for a contract to figure out what unit of exchange the original contract wants without another call that gets the required unit of exchange.

I agree, and our royaltyInfo function call is unit-less. So it can't be enforced. But I added it because the caller should know if they pass in 1000, and it returns 100, they should pay the royalty payment receiver address in the same thing they received payment in. It's unenforceable but it's in the spec.

From an implementer's perspective, last week, I realized that the creator's contract could return 100% of the sale amount if it wanted to, so I can't just automatically pay the amount returned.

That's correct, and yes you should. If the royalty fee is 100%, then according to spec, you should pay it. The market will devalue the NFT appropriately - its value should be zero or close to it (IMO). You can simply ban NFTs from your marketplace entirely if they exceed some royalty amount. This is up to each integrator to decide.

cliffhall commented 3 years ago

The call to check for the unit of exchange could look like:

function royaltyUnits(uint256 _tokenId, bytes calldata _data) 
external 
returns (address _coin, bytes memory _royaltyPaymentData);

If _coins the zero address, it's ETH, otherwise _coin points to the ERC20 contract for the royalty to be paid in.

seibelj commented 3 years ago

The call to check for the unit of exchange could look like:

function royaltyUnits(uint256 _tokenId, bytes calldata _data) 
external 
returns (address _coin, bytes memory _royaltyPaymentData);

If _coins the zero address, it's ETH, otherwise _coin points to the ERC20 contract for the royalty to be paid in.

We have spoken of that topic in great length if you scroll to past comments. We came to consensus that all extra information - specific tokens, how people get paid, etc. will be in future EIPs.

cliffhall commented 3 years ago

the caller should know if they pass in 1000, and it returns 100, they should pay the royalty payment receiver address in the same thing they received payment in. It's unenforceable but it's in the spec.

The possibility that the 100 means ETH and the 1000 you sent in was $WHALE is high. At scale, this won't work. Both systems must be in agreement on the unit of exchange. This can be booted to a future EIP, but adoption of this one would probably be higher if it solved this very large known issue out of the gate, IMHO.

seibelj commented 3 years ago

the caller should know if they pass in 1000, and it returns 100, they should pay the royalty payment receiver address in the same thing they received payment in. It's unenforceable but it's in the spec.

The possibility that the 100 means ETH and the 1000 you sent in was $WHALE is high. At scale, this won't work. Both systems must be in agreement on the unit of exchange. This can be booted to a future EIP, but adoption of this one would probably be higher if it solved this very large known issue out of the gate, IMHO.

I'm not sure I follow. The reason I added that

Implementers of this standard **MUST** require that the sale price, the value passed to `royaltyInfo()`, and the royalty payment must always be in the same unit of exchange.

was added to the spec is to solve that problem. If you sent in 1000 valueless units, but the sale is in 1000 ETH, and royaltyInfo() returns 100 valueless units, you must pay the receiver in ETH not in $SCAM.

Of course it is unenforceable, but so are royalty payments themselves. This is a voluntary EIP for a voluntary system implemented voluntarily by marketplaces who want to support artists and other NFT creators.

cliffhall commented 3 years ago

If you sent in 1000 valueless units, but the sale is in 1000 ETH, and royaltyInfo() returns 100 valueless units, you must pay the receiver in ETH not in $SCAM.

That only works if you expect the royaltyInfo method to calculate royalty as a percentage. If so, valueless units are a perfectly fine way of doing this. But the EIP goes out of its way to promote royalty payment amounts over percentages. That steers us directly into the the iceberg of units having value and needing to be agreed upon . If the creator contract expects 300 $WHALE as royalty and returns that in all cases for any valueless unit passed in for sale amount, the caller has no way of knowing that it is $WHALE that's expected. And thus any payment on that returned value of 300 can only be correct if paid in $WHALE.

Of course it is unenforceable, but so are royalty payments themselves. This is a voluntary EIP for a voluntary system implemented voluntarily by marketplaces who want to support artists and other NFT creators.

This is true but a bit hand-wavy when it comes to this basic issue. The EIP is a step in the right direction, but the ability to agree on on units of exchange would make it actually work. If not, we just assume the other contract is returning the proper royalty amount the units we passed in.

seibelj commented 3 years ago

@cliffhall you are bringing up many issues that were discussed previously. See an example here https://github.com/ethereum/EIPs/issues/2907#issuecomment-830269594

I believe > 99% of contracts will use fixed-percentage royalties, as that is the model already supported in existing marketplaces like OpenSea, Rarible, and Mintable. More exotic use-cases could potentially be supported by this EIP but you should consider drafting a new EIP to standardize it.

For anyone reading and wanting to suggest significant changes this late in the process, I highly recommend you read the entire thread (246 comments) as we are basically wrapping this up. It is very, very unlikely we change anything fundamental at this point in the EIP process. You can get the exotic features you desire by building a new EIP on top of this that uses the data fields which we added to future-proof such features.

Unused data parameters in function signature

This EIP mandates the inclusion of the _data parameter and the _royaltyPaymentData return value, and also mandates that these are unused. The purpose of this is to have a seamless path to build on this ERC by further EIP standards that specify payment notifications, fee splitting, and other advanced use cases.

cliffhall commented 3 years ago

Ok, makes sense.

Lenoftawa commented 3 years ago

Thanks @seibelj

I'm glad to see the latest updates. I was trying to formulate a message to support them, which is why I have been silent. I think the spec is close to complete.

I have a few remaining concerns. Happy to call them 'polish' if you prefer.

To ensure that we have a good standard, it needs to be tested, and that means trying to find ways that it could be used that result in behaviour that defeats the objective. I can find 4, I challenge others to try and find more.

With the exception of 2. (the 'unused' issue below), I can propose a PR with changes after the latest PR has been accepted.

  1. To pick up on @cliffhall's concern, I think this is very misleading: " this EIP does not mandate a fixed-percentage fee model." Without an agreement on a unit of exchange, anything other than a fixed percentage is meaningless. The simplest case that demonstrates this is a percentage that reduces at 100 units and 1000 units. The reader of the spec will not have read the 250 or so messages in this thread, and if they did would still have difficulty understanding the issue surrounding percentages. It is partially addressed in the text, but leaves it up to the reader to work it out from first principles. We need to spell it out.

    • This EIP does not specify a unit of exchange or a way to negotiate it, that is for a future EIP. Therefore ...
    • Under this specification, returning something that is not a fixed percentage will result in royalties being underpaid or overpaid. The interface is designed to allow more flexible royalties to be specified in future extension EIPS.
  2. We do not specify what 'unused' means. There may be a standard, but I cannot find one. If there is a standard, then we should reference it. I have been experimenting with passing various values in the _data field and it is difficult to find something that is unequivocal. This function could be called from a contract in Vyper or even in EVM byte code.

  3. We do not specify what should happen if the NFT returns 0x0 for _receiver. The fact that this has been raised in this thread is an indication that it should be documented. If I were a marketplace, I would be inclined to keep the royalty rather than burn it! If I were an NFT that wanted to burn it, I would give my own address and burn it myself, to prevent this. There is also a risk that someone will use this to signal a special condition, we should prevent that, in the same way as we prevent use of _data...

  4. We do not specify what should happen if the NFT returns 0 for _value. It may be obvious to us that the marketplace should do nothing, but it is quite possible that someone will rely on marketplace to actually make the transfer of 0 tokens. Or that a marketplace does not check this and attempts to send 0 tokens to an address that doesn't expect it. Crazier things have happened with standards that make assumptions about implementers perspectives. And again, there is also the possibility that the NFT and marketplace use this as a way to signal something; we need to prevent that.

VexyCats commented 3 years ago

For points 3 and 4 @lenifoti we cannot force implementation and that goes beyond the scope of the EIP anyways. How a marketplace chooses to deal with situations, is up to that contract and cannot be required.

"To pick up on @cliffhall's concern, I think this is very misleading: " this EIP does not mandate a fixed-percentage fee model." Without an agreement on a unit of exchange, anything other than a fixed percentage is meaningless. The simplest case that demonstrates this is a percentage that reduces at 100 units and 1000 units."

Can you explain what you mean here? I don't follow. The royalty amount returned is up to the NFT contract to calculate, we cannot cover a use case where the NFT contract wants all royalties paid in $WHALE while payments for the transfer of NFT might be made in ETH or DAI. This.... again is one of the million examples of ways to implement royalties that cannot be covered via an EIP. Is this what you and @cliffhall are referring to or am I misunderstanding?