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.

• 🐛 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;