Consider supporting payable ERC721's safeTransferFrom, transferFrom, and approve

[cwhinfrey]

🎉 Description

The ERC721 EIP lists:

• safeTransferFrom(address, address, uint256, bytes)
• safeTransferFrom(address, address, uint256)
• transferFrom(address, address, uint256)
• approve(address, uint256)

as payable functions. The payable modifier should be added to these functions in ERC721Basic.sol and ERC721BasicToken.sol.

• [x] 🐛 This is a bug report.
• [ ] 📈 This is a feature request.

💻 Environment

OpenZeppelin v1.10.0

[shrugs]

The reason that payable was not added by default is that non-payable functions are still standard-compliant, as of the last time I read the standard. In solidity it's much easier to make a non-payable function payable than it is to do the reverse, which I expect is why we went this directly (least specificity). Let me know if you think that should be interpreted differently.

[cwhinfrey]

@shrugs Makes sense. Looks like I missed this part in the standard. Solidity issue #3412: The above interfaces include explicit mutability guarantees for each function. Mutability guarantees are, in order weak to strong: payable, implicit nonpayable, view, and pure. Your implementation MUST meet the mutability guarantee in this interface and you MAY meet a stronger guarantee. For example, a payable function in this interface may be implemented as nonpayble (no state mutability specified) in your contract. We expect a later Solidity release will allow your stricter contract to inherit from this interface, but a workaround for version 0.4.20 is that you can edit this interface to add stricter mutability before inheriting from your contract. I'm going to close this.

[robertmagier]

I check today and ERC721 standard defines those functions as payable. Is it possible to reopen the ticket and modify it ?

[frangio]

@robertmagier For the reasons explained above so far we've decided to keep the functions non-payable. Have you run into a situation where you need them to be payable? If so, please open a new issue and explain the situation so we can discuss possible solutions.

[robertmagier]

@frangio Thank you for your kind reply. I decided to comment on that because of @shrugs comment

The reason that payable was not added by default is that non-payable functions are still standard-compliant, as of the last time I read the standard.

Since standard has changed I thought it is a good enough reason to reopen. I have a case where we implement transferFrom function which has to be payable and we can't overwrite your function because it is giving an error message:

TypeError: Overriding function changes state mutability from "nonpayable" to "payable".
    function safeTransferFrom(

So we can either modify openzeppelin-solidity library on our own or simply violate ERC721 Standard.

I will of course create another issues as required. Thank you.

[nventuro]

@robertmagier I've just checked the standard in its final form, and it still contains the section described above (see here. It references the Solidity compiler issue #3412, where removing the error you encountered is described. From that discussion, however, it looks like it'll be a long time before the feature is implemented (if they decide to implement it): I wouldn't expect it to come out before 0.6.0.

So, I'm not sure what's the right call. As it is, it looks like the only way to get those functions to be payable is to manually edit the original interface, which is of course not what we want. Could you please share some more detail as to why you need that function to be payable, so we can see if we can find a temporary workaround? Thanks!

[ImmuneGit]

Let me describe a use case I've stumbled across. I have to do some post processing after selling tokens (let say get a tax fee). And we want to do the same in case if tokens sell on other platforms. I suppose in this case other platforms will just use our ERC721 contract to interact with our tokens. To realise it all "transfer" functions have to be "payable".

[robertmagier]

@nventuro I think I am more than confused. Standard your linked is still describing function as payable.

function transferFrom(address _from, address _to, uint256 _tokenId) external payable;

However I think you are talking about mutability guarantees which says that you can implement payable function as non payable. This is what you did in your implementation - implemented those functions as non payable. This works for you, but @ImmuneGit and I want to use your library but overwrite transferFrom function as payable.

I also checked on Remix and it seems that there is really no good solution to it as you can't overwrite payable function as nonpayable and also it is not possible to overwrite nonpayable function as payable.

Both implemenations below will fail:

pragma solidity ^0.4.24;

contract A 
{
    function transfer() public   returns (uint) {
        return 7;
    }
}

contract B is A {
    function transfer() public payable   returns (uint)
    {
        return msg.value;
    }
}

browser/test.sol:13:5: TypeError: Overriding function changes state mutability from "nonpayable" to "payable".

pragma solidity ^0.4.24;

contract A 
{
    function transfer() public payable  returns (uint) {
        return 7;
    }
}
contract B is A {

    function transfer() public  returns (uint)
    {
        return msg.value;
    }
}

TypeError: Overriding function changes state mutability from "payable" to "nonpayable".

So I think if you implement your function as payable then people who want to have as nonpayble will have a problem and same for opposite scenario.

The only thing I can say in favor of implementing those functions as payable is that standard define them that way and that it will work for me :)

 function safeTransferFrom(address _from, address _to, uint256 _tokenId, bytes data) external payable;
 function safeTransferFrom(address _from, address _to, uint256 _tokenId) external payable;
 function transferFrom(address _from, address _to, uint256 _tokenId) external payable;
 function approve(address _approved, uint256 _tokenId) external payable;

[frangio]

I do understand your use cases and would like OpenZeppelin to cater to them, so I'm reopening the issue to keep the discussion going and to consider different options for implementation. But to kickstart the discussion I'll share why our implementation's functions are not payable at the moment.

Functions being non-payable by default is a Solidity security feature that prevents people accidentally sending ether to contracts where it is unrecoverable. Developers have to be very careful when they make a function payable, and ensure that the ether that will enter the contract is somehow used or retrievable later.

A custom ERC721 contract with payable functions can ensure the ether is not lost, e.g. by forwarding it somewhere. While OpenZeppelin can provide a base ERC721 implementation with payable functions, it can't provide a generic mechanism to ensure the ether is put to good use because it would be entirely implementation specific. Naive users of OpenZeppelin ERC721 would not be adding any such mechanism, and all those OpenZeppelin ERC721 tokens would become sinks of unrecoverable ether.

There are some things that come to mind that we could do, but the developer experience of using them would not be very good. Feel free to share any ideas!

[robertmagier]

Thank you for reopening. For the moment we have copied your library and modified you transfer function to be payable. It works for us ( of course ).

Maybe you can make this functions payable and implement internal preTransferValidation function which will be called in transferFrom ( and other functions ). In default implementation this function would only have one line require (msg.value == 0);

Kind of doesn't make sense a bit to make function payable and require value to be 0, but it will guarantee that there is no ETH being send by mistake. From user perspective it would work exactly same as sending ETH to non payable function.

The other solution is to create postTransferUpdate function and send all ETH back to the sender. This is not so good because I think it is better to fail and let user know that ETH is not accepted.

[3esmit]

Making a function payable tells that msg.value should be considered in the call, which is not desirable in that functions. Are those functions expecting any msg.value?

[spalladino]

A use case I've been referred to is charging a small fee per transfer. In this scenario, tokens represent the right to access a resource. To prevent people sharing this access back and forth whenever they need to access it, the minter enforces a small fee.

[nventuro]

@spalladino would the fee be charged on transfer? Who gets the fee? Wouldn't it be easier to simply limit transfers?

[spalladino]

would the fee be charged on transfer?

Yes

Who gets the fee?

The minter of the token. Not sure if there can be more than one minter per contract in this model.

Wouldn't it be easier to simply limit transfers?

The question is how much to limit them. They are considering alternative methods, such as burning part of the time left for accessing a resource (think of a token representing a year-long subscription, transferring it could burn a few days out of it) but it'd be interesting to support ETH-based fees.

[spalladino]

Another data point: https://twitter.com/DennisonBertram/status/1194655772455690241

[enzoferey]

Sorry to bump this issue a few years later. I'm new to Ethereum development and I have stumbled upon the same problem described by other people. I would like users to pay a certain amount for transferring tokens from one to another so the contract itself acts like a marketplace taking a certain commission. I guess it's a very similar use case than @ImmuneGit described.

The solution proposed to @robertmagier seems to cover the security concerns you had, but no answer was given to it.

What's the status on this issue ? Is there a better way to achieve this type of uses cases?

[Schachte]

@enzoferey You might be interested in EIP-2981 spec. This is NFT with Royalties standard. However, I don't know if all marketplaces support the spec yet (IE OpenSea)

[frangio]

Note that we have an implementation of EIP-2981 now: ERC2981 and ERC721Royalty which is just a thin wrapper.

[enzoferey]

Thanks to both of you @Schachte and @frangio 🙌🏻

[sullof]

Note that we have an implementation of EIP-2981 now: ERC2981 and ERC721Royalty which is just a thin wrapper.

I used that in a contract but it does not really solve the problem of royalties, it just suggests royalties to marketplaces if they decide to support the standard.

Instead, making the transfer functions payable would allow the contract to manage royalties and fees on chain.

Anyway, I started a discussion about this issue on https://ethereum-magicians.org/t/proposed-solution-for-nft-royalties-addressing-erc-721-payability/14368 before noticing this issue.