Update EIP-6454: Use overloaded methods

Merged by EIP-Bot.

EIPS/eip-6454.md

@@ -1,7 +1,7 @@
 ---
 eip: 6454
-title: Minimalistic Non-Transferable NFTs
-description: An interface for Non-Transferable Non-Fungible Tokens extension allowing for tokens to be non-transferable.
+title: Minimal Transferable NFT detection interface
+description: A minimal extension to identify the transferability of Non-Fungible Tokens.
 author: Bruno Škvorc (@Swader), Francesco Sullo (@sullof), Steven Pineda (@steven2308), Stevan Bogosavljevic (@stevyhacker), Jan Turk (@ThunderDeliverer)
 discussions-to: https://ethereum-magicians.org/t/minimalistic-transferable-interface/12517
 status: Last Call
@@ -14,9 +14,9 @@ requires: 165, 721
 
 ## Abstract
 
-The Minimalistic Non-Transferable interface for Non-Fungible Tokens standard extends [ERC-721](./eip-721.md) by preventing NFTs from being transferred.
+The Minimalistic Transferable interface for Non-Fungible Tokens standard extends [ERC-721](./eip-721.md) by introducing the ability to identify whether an NFT can be transferred or not.
 
-This proposal introduces the ability to prevent a token from being transferred from their owner, making them bound to the externally owned account, smart contract or token that owns it.
+This proposal introduces the ability to prevent a token from being transferred from their owner, making them bound to the externally owned account, abstracted account, smart contract or token that owns it.
 
 ## Motivation
 
@@ -54,39 +54,53 @@ pragma solidity ^0.8.16;
 
 interface IERC6454 /* is IERC165 */ {
     /**
-     * @notice Used to check whether the given token is non-transferable or not.
-     * @dev If this function returns `true`, the transfer of the token MUST revert execution
+     * @notice Used to check whether the given token is transferable or not.
+     * @dev If this function returns `false`, the transfer of the token MUST revert execution
      * @dev If the tokenId does not exist, this method MUST revert execution
      * @param tokenId ID of the token being checked
-     * @return Boolean value indicating whether the given token is non-transferable
+     * @return Boolean value indicating whether the given token is transferable
      */
-    function isNonTransferable(uint256 tokenId) external view returns (bool);
+    function isTransferable(uint256 tokenId) external view returns (bool);
+
+    /**
+     * @notice Used to check whether the given token is transferable or not based on source and destination address.
+     * @dev If this function returns `false`, the transfer of the token MUST revert execution
+     * @dev If the tokenId does not exist, this method MUST revert execution
+     * @param tokenId ID of the token being checked
+     * @param from Address from which the token is being transferred
+     * @param to Address to which the token is being transferred
+     * @return Boolean value indicating whether the given token is transferable
+     */
+    function isTransferable(uint256 tokenId, address from, address to) external view returns (bool);
 }
 ```
 
 ## Rationale
 
 Designing the proposal, we considered the following questions:
 
-1. **Should we propose another Non-Transferable NFT proposal given the existence of existing ones, some even final, and how does this proposal compare to them?**\
+1. **Should we propose another (Non-)Transferable NFT proposal given the existence of existing ones, some even final, and how does this proposal compare to them?**\
    This proposal aims to provide the minimum necessary specification for the implementation of non-transferable NFTs, we feel none of the existing proposals have presented the minimal required interface. Unlike other proposals that address the same issue, this proposal requires fewer methods in its specification, providing a more streamlined solution.
 2. **Why is there no event marking the token as Non-Transferable in this interface?**\
    The token can become non-transferable either at its creation, after being marked as non-transferable, or after a certain condition is met. This means that some cases of tokens becoming non-transferable cannot emit an event, such as if the token becoming non-transferable is determined by a block number. Requiring an event to be emitted upon the token becoming non-transferable is not feasible in such cases.
-3. **Should the non-transferable state management function be included in this proposal?**\
-   A function that marks a token as non-transferable or releases the binding is referred to as the non-transferable management function. To maintain the objective of designing an agnostic non-transferable proposal, we have decided not to specify the non-transferable management function. This allows for a variety of custom implementations that require the tokens to be non-transferable.
-4. **Why should this be an EIP if it only contains one method?**\
-   One could argue that since the core of this proposal is to only prevent ERC-721 tokens to be transferred, this could be done by overriding the transfer function. While this is true, the only way to assure that the token is non-transferable before the smart contract execution, is for it to have the non-transferable interface.\
-   This also allows for smart contract to validate that the token is non-transferable and not attempt transferring it as this would result in failed transactions and wasted gas.
-5. **Why does this proposal use `isNotTransferable` instead of `isTransferable`?**\
-   ERC-721 tokens are usually transferable, but this interface focuses on the use case where NFTs may not be transferable. The method name was chosen to reflect this.
+3. **Should the transferability state management function be included in this proposal?**\
+   A function that marks a token as non-transferable or releases the binding is referred to as the transferability management function. To maintain the objective of designing an agnostic minimal transferable proposal, we have decided not to specify the transferability management function. This allows for a variety of custom implementations that require the tokens to be non-transferable.
+4. **Why should this be an EIP if it only contains two methods?**\
+   One could argue that since the core of this proposal is to only prevent ERC-721 tokens to be transferred, this could be done by overriding the transfer function. While this is true, the only way to assure that the token is non-transferable before the smart contract execution, is for it to have the transferable interface.\
+   This also allows for smart contract to validate whether the token is not transferable and not attempt transferring it as this would result in failed transactions and wasted gas.
+5. **Why does this proposal contain two methods with the same name?**\
+   Both methods defined in this proposal address the same issue, but in different ways. The first method is used to check whether the token is transferable or not in general, while the second method is used to check whether the token is conditionally transferable or not based on the source and destination addresses. The second method is useful in cases where the transferability of the token is dependent on the source and destination addresses.
+6. **What is the best user experience for frontend?**\
+   The best user experience for the front end is having a single method that checks whether the token is transferable. This method should handle both cases of transferability, general and conditional. This is why the second method is defined as an overload of the first method.\
+   The front end should also be able to handle the case where the token is not transferable and the transfer is attempted. This can be done by checking the return value of the transfer function, which will be false if the token is not transferable. If the token would just be set as non-transferable, without a standardized interface to check whether the token is transferable, the only way to validate transferability would be to attempt a gas calculation and check whether the transaction would revert. This is a bad user experience and should be avoided.
 
 ## Backwards Compatibility
 
 The Minimalistic Non-Transferable token standard is fully compatible with [ERC-721](./eip-721.md) and with the robust tooling available for implementations of ERC-721 as well as with the existing ERC-721 infrastructure.
 
 ## Test Cases
 
-Tests are included in [`nonTransferable.ts`](../assets/eip-6454/test/nonTransferable.ts).
+Tests are included in [`transferable.ts`](../assets/eip-6454/test/transferable.ts).
 
 To run them in terminal, you can use the following commands:
 
@@ -98,15 +112,15 @@ npx hardhat test
 
 ## Reference Implementation
 
-See [`ERC721NonTransferableMock.sol`](../assets/eip-6454/contracts/mocks/ERC721NonTransferableMock.sol).
+See [`ERC721TransferableMock.sol`](../assets/eip-6454/contracts/mocks/ERC721TransferableMock.sol).
 
 ## Security Considerations
 
 The same security considerations as with [ERC-721](./eip-721.md) apply: hidden logic may be present in any of the functions, including burn, add asset, accept asset, and more.
 
-A smart contract can implement the proposal interface but returns fraudulent values, i.e., returning `true` for `isNotTransferable` when the token is transferable. Such a contract would trick other contracts into thinking that the token is non-transferable when it is transferable. If such a contract exists, we suggest not interacting with it. Much like fraudulent [ERC-20](./eip-20.md) or [ERC-721](./eip-721.md) smart contracts, it is not possible to prevent such contracts from existing. We suggest that you verify all of the external smart contracts you interact with and not interact with contracts you do not trust.
+A smart contract can implement the proposal interface but returns fraudulent values, i.e., returning `false` for `isTransferable` when the token is transferable. Such a contract would trick other contracts into thinking that the token is non-transferable when it is transferable. If such a contract exists, we suggest not interacting with it. Much like fraudulent [ERC-20](./eip-20.md) or [ERC-721](./eip-721.md) smart contracts, it is not possible to prevent such contracts from existing. We suggest that you verify all of the external smart contracts you interact with and not interact with contracts you do not trust.
 
-Since the non-transferable state can change over time, verifying that the state of the token is non-transferable before interacting with it is essential. Therefore, a dApp, marketplace, or wallet implementing this interface should verify the state of the token every time the token is displayed.
+Since the transferability state can change over time, verifying that the state of the token is transferable before interacting with it is essential. Therefore, a dApp, marketplace, or wallet implementing this interface should verify the state of the token every time the token is displayed.
 
 Caution is advised when dealing with non-audited contracts.
 

assets/eip-6454/contracts/IERC6454.sol

@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: CC0-1.0
+
+pragma solidity ^0.8.16;
+
+interface IERC6454 {
+
+    /**
+     * @notice Used to check whether the given token is transferable or not.
+     * @dev If this function returns `false`, the transfer of the token MUST revert execution
+     * @dev If the tokenId does not exist, this method MUST revert execution
+     * @param tokenId ID of the token being checked
+     * @return Boolean value indicating whether the given token is transferable
+     */
+    function isTransferable(uint256 tokenId) external view returns (bool);
+
+    /**
+     * @notice Used to check whether the given token is transferable or not based on source and destination address.
+     * @dev If this function returns `false`, the transfer of the token MUST revert execution
+     * @dev If the tokenId does not exist, this method MUST revert execution
+     * @param tokenId ID of the token being checked
+     * @param from Address from which the token is being transferred
+     * @param to Address to which the token is being transferred
+     * @return Boolean value indicating whether the given token is transferable
+     */
+    function isTransferable(uint256 tokenId, address from, address to) external view returns (bool);
+}

assets/eip-6454/contracts/mocks/ERC721NonTransferableMock.sol → assets/eip-6454/contracts/mocks/ERC721TransferableMock.sol

@@ -3,7 +3,7 @@
 pragma solidity ^0.8.16;
 
 import "@openzeppelin/contracts/token/ERC721/ERC721.sol";
-import "../INonTransferable.sol";
+import "../IERC6454.sol";
 import "hardhat/console.sol";
 
 error CannotTransferNonTransferable();
@@ -12,11 +12,15 @@ error CannotTransferNonTransferable();
  * @title ERC721NonTransferableMock
  * Used for tests
  */
-contract ERC721NonTransferableMock is INonTransferable, ERC721 {
+contract ERC721NonTransferableMock is IERC6454, ERC721 {
+    address public owner;
+
     constructor(
         string memory name,
         string memory symbol
-    ) ERC721(name, symbol) {}
+    ) ERC721(name, symbol) {
+        owner = msg.sender;
+    }
 
     function mint(address to, uint256 amount) public {
         _mint(to, amount);
@@ -26,9 +30,17 @@ contract ERC721NonTransferableMock is INonTransferable, ERC721 {
         _burn(tokenId);
     }
 
-    function isNonTransferable(uint256 tokenId) public view returns (bool) {
+    function isTransferable(uint256 tokenId) public view returns (bool) {
         _requireMinted(tokenId);
-        return true;
+        return false;
+    }
+
+    function isTransferable(uint256 tokenId, address from, address to) public view returns (bool) {
+        if (from == owner) {
+            return true;
+        } else {
+            return isTransferable(tokenId);
+        }
     }
 
     function _beforeTokenTransfer(
@@ -42,9 +54,8 @@ contract ERC721NonTransferableMock is INonTransferable, ERC721 {
         // exclude minting and burning
         if ( from != address(0) && to != address(0)) {
             uint256 lastTokenId = firstTokenId + batchSize;
-            for (uint256 i = firstTokenId; i < lastTokenId; i++) {
-                uint256 tokenId = firstTokenId + i;
-                if (isNonTransferable(tokenId)) {
+            for (uint256 i = firstTokenId; i < lastTokenId; ) {
+                if (!isTransferable(i, from, to)) {
                     revert CannotTransferNonTransferable();
                 }
                 unchecked {
@@ -57,7 +68,7 @@ contract ERC721NonTransferableMock is INonTransferable, ERC721 {
     function supportsInterface(
         bytes4 interfaceId
     ) public view virtual override(ERC721) returns (bool) {
-        return interfaceId == type(INonTransferable).interfaceId
+        return interfaceId == type(IERC6454).interfaceId
             || super.supportsInterface(interfaceId);
     }
 }

assets/eip-6454/package.json

@@ -1,5 +1,5 @@
 {
-  "name": "non-transferable-tokens",
+  "name": "transferable-tokens",
   "scripts": {
     "test": "yarn typechain && hardhat test",
     "typechain": "hardhat typechain",

assets/eip-6454/test/nonTransferable.ts → assets/eip-6454/test/transferable.ts

@@ -5,7 +5,7 @@ import { loadFixture } from "@nomicfoundation/hardhat-network-helpers";
 import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers";
 import { ERC721NonTransferableMock } from "../typechain-types";
 
-async function nonTransferableTokenFixture(): Promise<ERC721NonTransferableMock> {
+async function transferableTokenFixture(): Promise<ERC721NonTransferableMock> {
   const factory = await ethers.getContractFactory("ERC721NonTransferableMock");
   const token = await factory.deploy("Chunky", "CHNK");
   await token.deployed();
@@ -23,13 +23,14 @@ describe("NonTransferable", async function () {
     const signers = await ethers.getSigners();
     owner = signers[0];
     otherOwner = signers[1];
-    nonTransferable = await loadFixture(nonTransferableTokenFixture);
+    nonTransferable = await loadFixture(transferableTokenFixture);
 
     await nonTransferable.mint(owner.address, 1);
+    await nonTransferable.mint(otherOwner.address, 2);
   });
 
   it("can support IRMRKNonTransferable", async function () {
-    expect(await nonTransferable.supportsInterface("0xa7331ab1")).to.equal(true);
+    expect(await nonTransferable.supportsInterface("0x23f06346")).to.equal(true);
   });
 
   it("does not support other interfaces", async function () {
@@ -43,16 +44,18 @@ describe("NonTransferable", async function () {
         ["safeTransferFrom(address,address,uint256)"](
           owner.address,
           otherOwner.address,
-          tokenId
+          tokenId + 1
         )
     ).to.be.revertedWithCustomError(nonTransferable, "CannotTransferNonTransferable");
   });
 
+  it("returns the expected transferability state", async function () {
+    expect(await nonTransferable['isTransferable(uint256)'](tokenId)).to.equal(false);
+    expect(await nonTransferable['isTransferable(uint256,address,address)'](tokenId, owner.address, otherOwner.address)).to.equal(true);
+  })
+
   it("reverts if token does not exist", async function () {
-    expect(
-      nonTransferable
-        .isNonTransferable(10)
-    ).to.be.revertedWith("CannotTransferNonTransferable");
+    await expect(nonTransferable['isTransferable(uint256)'](10)).to.be.revertedWith("ERC721: invalid token ID");
   });
 
   it("can burn", async function () {