ERC721 payable transferFrom

[olegabr]

openzeppelin-contracts/contracts/token/ERC721/IERC721.sol

Line 70 in 378531b

function transferFrom(address from, address to, uint256 tokenId) external;

Hello,

I'm trying to implement royalty in ERC721 token. The idea is to have a wantToSell method to set a price for a token, if you want to allow someone to buy it, and a transferFrom payable method to allow buyer to call it providing enough value ETH.
The royalty is distributed to the token contract owner and token ID author in all subsequent re-sells.

The problem is that the transferFrom function declared without the payable modifier and the solidity compiler complains if I try to override the transferFrom function adding the payable modifier myself.

The standard declares the transferFrom method as a payable https://eips.ethereum.org/EIPS/eip-721

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

1. Is there some reason why openzeppelin implementation deviate from the standard in this regard?
2. Is there some workaround I can use to implement the royalty functionality in the current openzeppelin implementation?
3. Is there a better way to implement the royalty functionality that would not suffer from this issue?

[Amxx]

Hello @olegabr and thanks for the notice.

A reason why we did not make them payable in our implementation is to avoid funds getting stuck since there is no mechanism to withdraw any fund that might be sent by mistake ...
Still, and regardless of whether there is a better way to implement royalties, the fact that the IERC721 we provide is not strictly compatible with the ERC's is disturbing.

[olegabr]

@Amxx Thank you for your prompt response!

[frangio]

There is no clean way for us to make the interface payable, given that our implementation has to remain non-payable (because we can't define a particular way of handling msg.value).

I believe this is a limitation of the language so I've opened an issue about it. ethereum/solidity#11253

In the mean time, I'm going to close the PR because it doesn't allow child contracts to cleanly remove the require(msg.value == 0) check, and we'll resume this issue when Solidity allows us to make the interface function payable.

[fulldecent]

Related issue in the ERC-721 reference implementation: nibbstack/erc721#144

---

Because this is a known issue (read the caveats section in ERC-721 if you want to see a bunch of known Solidity issues), and because we are using Solidity, I recommend this work plan for this issue:

• Acknowledge the upstream Solidity issue in IERC721.sol
• Add a @dev note on each payable function about this
• Close this issue

[frangio]

Thanks for the suggestion @fulldecent. We will keep the issue open with the tag on hold until ethereum/solidity#11253 is resolved.

[0xCardinalError]

@olegabr
what have you decide to do in the end? I would also like to add royalties to transferFrom and safeTransfer so users cant bypass royalties, what would be good way to do it?

[olegabr]

@adriadrop ETH can not be used because of this issue. But ERC20 can :-)

[0xCardinalError]

Ok, but ETH could been use seemlesly, with weth you have UX problem, people needing to have weth or you needing to transact it for them. ned, 15. kol 2021. 05:58 Oleg Abrosimov ***@***.***> je napisao:

…

@adriadrop <https://github.com/adriadrop> ETH can not be used because of this issue. But ERC20 can :-) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2610 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACEN4J2FKNTI5PWDTEA7XODT443NPANCNFSM4ZYZ5NLQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

[olegabr]

The whole crypto space has a huge UI/UX issue. That is why I'm developing tools to hide crypto from users altogether, and let them pay with fiat if they want: https://ethereumico.io/shop/

[alexroat]

Hello. Can you offer an alternative or suggest a pattern ?
It seems so weird that there is no payable support in the whole ERC 721 source set.
Using a token on a platform I'm developing without payable in transferTo is not a big issue at all because I can simply implement a "buy" payable method in order to get the royalties or transfer a payment to the old owner through the contract itself. But .... what about when I "delegate" an operator with approve ? without a payable transferFrom you simply give away the tokens tracked by the contract to another external entity -> this is is why I'm planning to implement the ERC 721 interface but with a locked approve.
Couldn't you simply provide an implementation more compliant to the standard with payable support and another one (let's call it SafeERC721) with the non payable support ?

[fulldecent]

This issue here can track upstream Solidity support for payable/nonayable override. And @frangio is doing a great job advocating over there.

But for all this discussion about royalty (which is a supported NFT use case) perhaps Stack Exchange or similar is a better place to discuss.

[alexroat]

It was in my opinion that you cannot call payable function from non payable function, but it is possible.
This is a pattern that is simply an overloading of _transfer and _mint function in order to let them call a payable function to handle, for example, a fee payout when the token is transfered.
It seems to be working:

function _transfer(
        address from,
        address to,
        uint256 tokenId
    ) internal virtual override {
        payToken(tokenId);
        return super._transfer(from,to,tokenId);
    }

    function _mint(address to, uint256 tokenId) internal virtual override {
         payToken(tokenId);
        return super._mint(to,tokenId);
    }
    
    function payToken(uint256 id) public payable {
        payable(_exists(id)?ownerOf(id):contractOwner).transfer(getTokenPrice(id));
        contractOwner.transfer(getTokenFee(id));
    }

So, the lack of payable version of the transferFrom is no more a problem.

[felixakiragreen]

I had to make my own IERC721Payable.sol. Sharing here so others can use it:

// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts (last updated v4.8.0) (token/ERC721/IERC721.sol)

pragma solidity ^0.8.0;

import '@openzeppelin/contracts/utils/introspection/IERC165.sol';

/**
 * @dev Required interface of an ERC721 compliant contract.
 */
interface IERC721Payable is IERC165 {
	/**
	 * @dev Emitted when `tokenId` token is transferred from `from` to `to`.
	 */
	event Transfer(
		address indexed from,
		address indexed to,
		uint256 indexed tokenId
	);

	/**
	 * @dev Emitted when `owner` enables `approved` to manage the `tokenId` token.
	 */
	event Approval(
		address indexed owner,
		address indexed approved,
		uint256 indexed tokenId
	);

	/**
	 * @dev Emitted when `owner` enables or disables (`approved`) `operator` to manage all of its assets.
	 */
	event ApprovalForAll(
		address indexed owner,
		address indexed operator,
		bool approved
	);

	/**
	 * @dev Returns the number of tokens in ``owner``'s account.
	 */
	function balanceOf(address owner) external view returns (uint256 balance);

	/**
	 * @dev Returns the owner of the `tokenId` token.
	 *
	 * Requirements:
	 *
	 * - `tokenId` must exist.
	 */
	function ownerOf(uint256 tokenId) external view returns (address owner);

	/**
	 * @dev Safely transfers `tokenId` token from `from` to `to`.
	 *
	 * Requirements:
	 *
	 * - `from` cannot be the zero address.
	 * - `to` cannot be the zero address.
	 * - `tokenId` token must exist and be owned by `from`.
	 * - If the caller is not `from`, it must be approved to move this token by either {approve} or {setApprovalForAll}.
	 * - If `to` refers to a smart contract, it must implement {IERC721Receiver-onERC721Received}, which is called upon a safe transfer.
	 *
	 * Emits a {Transfer} event.
	 */
	function safeTransferFrom(
		address from,
		address to,
		uint256 tokenId,
		bytes calldata data
	) external payable;

	/**
	 * @dev Safely transfers `tokenId` token from `from` to `to`, checking first that contract recipients
	 * are aware of the ERC721 protocol to prevent tokens from being forever locked.
	 *
	 * Requirements:
	 *
	 * - `from` cannot be the zero address.
	 * - `to` cannot be the zero address.
	 * - `tokenId` token must exist and be owned by `from`.
	 * - If the caller is not `from`, it must have been allowed to move this token by either {approve} or {setApprovalForAll}.
	 * - If `to` refers to a smart contract, it must implement {IERC721Receiver-onERC721Received}, which is called upon a safe transfer.
	 *
	 * Emits a {Transfer} event.
	 */
	function safeTransferFrom(
		address from,
		address to,
		uint256 tokenId
	) external payable;

	/**
	 * @dev Transfers `tokenId` token from `from` to `to`.
	 *
	 * WARNING: Note that the caller is responsible to confirm that the recipient is capable of receiving ERC721
	 * or else they may be permanently lost. Usage of {safeTransferFrom} prevents loss, though the caller must
	 * understand this adds an external call which potentially creates a reentrancy vulnerability.
	 *
	 * Requirements:
	 *
	 * - `from` cannot be the zero address.
	 * - `to` cannot be the zero address.
	 * - `tokenId` token must be owned by `from`.
	 * - If the caller is not `from`, it must be approved to move this token by either {approve} or {setApprovalForAll}.
	 *
	 * Emits a {Transfer} event.
	 */
	function transferFrom(
		address from,
		address to,
		uint256 tokenId
	) external payable;

	/**
	 * @dev Gives permission to `to` to transfer `tokenId` token to another account.
	 * The approval is cleared when the token is transferred.
	 *
	 * Only a single account can be approved at a time, so approving the zero address clears previous approvals.
	 *
	 * Requirements:
	 *
	 * - The caller must own the token or be an approved operator.
	 * - `tokenId` must exist.
	 *
	 * Emits an {Approval} event.
	 */
	function approve(address to, uint256 tokenId) external;

	/**
	 * @dev Approve or remove `operator` as an operator for the caller.
	 * Operators can call {transferFrom} or {safeTransferFrom} for any token owned by the caller.
	 *
	 * Requirements:
	 *
	 * - The `operator` cannot be the caller.
	 *
	 * Emits an {ApprovalForAll} event.
	 */
	function setApprovalForAll(address operator, bool _approved) external;

	/**
	 * @dev Returns the account approved for `tokenId` token.
	 *
	 * Requirements:
	 *
	 * - `tokenId` must exist.
	 */
	function getApproved(
		uint256 tokenId
	) external view returns (address operator);

	/**
	 * @dev Returns if the `operator` is allowed to manage all of the assets of `owner`.
	 *
	 * See {setApprovalForAll}
	 */
	function isApprovedForAll(
		address owner,
		address operator
	) external view returns (bool);
}

[frangio]

Duplicate of #1015