WIP: Fix/erc721 (OpenZeppelin#993)

* fix: defer lookup-specific info to implementations * fix: change inheritance order, fix import
philosup · Jun 12, 2018 · e1dc141 · e1dc141
1 parent 9f1d294
commit e1dc141
Show file tree

Hide file tree

Showing 5 changed files with 73 additions and 70 deletions.
diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol
@@ -8,21 +8,6 @@ import "./ERC721Basic.sol";
  * @dev See https://github.com/ethereum/EIPs/blob/master/EIPS/eip-721.md
  */
 contract ERC721Enumerable is ERC721Basic {
-
-  bytes4 private constant InterfaceId_ERC721Enumerable = 0x780e9d63;
-  /**
-   * 0x780e9d63 ===
-   *   bytes4(keccak256('totalSupply()')) ^
-   *   bytes4(keccak256('tokenOfOwnerByIndex(address,uint256)')) ^
-   *   bytes4(keccak256('tokenByIndex(uint256)'))
-   */
-
-  constructor()
-    public
-  {
-    _registerInterface(InterfaceId_ERC721Enumerable);
-  }
-
   function totalSupply() public view returns (uint256);
   function tokenOfOwnerByIndex(
     address _owner,
@@ -41,21 +26,6 @@ contract ERC721Enumerable is ERC721Basic {
  * @dev See https://github.com/ethereum/EIPs/blob/master/EIPS/eip-721.md
  */
 contract ERC721Metadata is ERC721Basic {
-
-  bytes4 private constant InterfaceId_ERC721Metadata = 0x5b5e139f;
-  /**
-   * 0x5b5e139f ===
-   *   bytes4(keccak256('name()')) ^
-   *   bytes4(keccak256('symbol()')) ^
-   *   bytes4(keccak256('tokenURI(uint256)'))
-   */
-
-  constructor()
-    public
-  {
-    _registerInterface(InterfaceId_ERC721Metadata);
-  }
-
   function name() external view returns (string _name);
   function symbol() external view returns (string _symbol);
   function tokenURI(uint256 _tokenId) public view returns (string);

diff --git a/contracts/token/ERC721/ERC721Basic.sol b/contracts/token/ERC721/ERC721Basic.sol
@@ -1,13 +1,13 @@
 pragma solidity ^0.4.23;
 
-import "../../introspection/SupportsInterfaceWithLookup.sol";
+import "../../introspection/ERC165.sol";
 
 
 /**
  * @title ERC721 Non-Fungible Token Standard basic interface
  * @dev see https://github.com/ethereum/EIPs/blob/master/EIPS/eip-721.md
  */
-contract ERC721Basic is SupportsInterfaceWithLookup {
+contract ERC721Basic is ERC165 {
   event Transfer(
     address indexed _from,
     address indexed _to,
@@ -24,34 +24,6 @@ contract ERC721Basic is SupportsInterfaceWithLookup {
     bool _approved
   );
 
-  bytes4 private constant InterfaceId_ERC721 = 0x80ac58cd;
-  /*
-   * 0x80ac58cd ===
-   *   bytes4(keccak256('balanceOf(address)')) ^
-   *   bytes4(keccak256('ownerOf(uint256)')) ^
-   *   bytes4(keccak256('approve(address,uint256)')) ^
-   *   bytes4(keccak256('getApproved(uint256)')) ^
-   *   bytes4(keccak256('setApprovalForAll(address,bool)')) ^
-   *   bytes4(keccak256('isApprovedForAll(address,address)')) ^
-   *   bytes4(keccak256('transferFrom(address,address,uint256)')) ^
-   *   bytes4(keccak256('safeTransferFrom(address,address,uint256)')) ^
-   *   bytes4(keccak256('safeTransferFrom(address,address,uint256,bytes)'))
-   */
-
-  bytes4 private constant InterfaceId_ERC721Exists = 0x4f558e79;
-  /*
-   * 0x4f558e79 ===
-   *   bytes4(keccak256('exists(uint256)'))
-   */
-
-  constructor ()
-    public
-  {
-    // register the supported interfaces to conform to ERC721 via ERC165
-    _registerInterface(InterfaceId_ERC721);
-    _registerInterface(InterfaceId_ERC721Exists);
-  }
-
   function balanceOf(address _owner) public view returns (uint256 _balance);
   function ownerOf(uint256 _tokenId) public view returns (address _owner);
   function exists(uint256 _tokenId) public view returns (bool _exists);

diff --git a/contracts/token/ERC721/ERC721BasicToken.sol b/contracts/token/ERC721/ERC721BasicToken.sol
@@ -4,13 +4,35 @@ import "./ERC721Basic.sol";
 import "./ERC721Receiver.sol";
 import "../../math/SafeMath.sol";
 import "../../AddressUtils.sol";
+import "../../introspection/SupportsInterfaceWithLookup.sol";
 
 
 /**
  * @title ERC721 Non-Fungible Token Standard basic implementation
  * @dev see https://github.com/ethereum/EIPs/blob/master/EIPS/eip-721.md
  */
-contract ERC721BasicToken is ERC721Basic {
+contract ERC721BasicToken is SupportsInterfaceWithLookup, ERC721Basic {
+
+  bytes4 private constant InterfaceId_ERC721 = 0x80ac58cd;
+  /*
+   * 0x80ac58cd ===
+   *   bytes4(keccak256('balanceOf(address)')) ^
+   *   bytes4(keccak256('ownerOf(uint256)')) ^
+   *   bytes4(keccak256('approve(address,uint256)')) ^
+   *   bytes4(keccak256('getApproved(uint256)')) ^
+   *   bytes4(keccak256('setApprovalForAll(address,bool)')) ^
+   *   bytes4(keccak256('isApprovedForAll(address,address)')) ^
+   *   bytes4(keccak256('transferFrom(address,address,uint256)')) ^
+   *   bytes4(keccak256('safeTransferFrom(address,address,uint256)')) ^
+   *   bytes4(keccak256('safeTransferFrom(address,address,uint256,bytes)'))
+   */
+
+  bytes4 private constant InterfaceId_ERC721Exists = 0x4f558e79;
+  /*
+   * 0x4f558e79 ===
+   *   bytes4(keccak256('exists(uint256)'))
+   */
+
   using SafeMath for uint256;
   using AddressUtils for address;
 
@@ -48,6 +70,14 @@ contract ERC721BasicToken is ERC721Basic {
     _;
   }
 
+  constructor()
+    public
+  {
+    // register the supported interfaces to conform to ERC721 via ERC165
+    _registerInterface(InterfaceId_ERC721);
+    _registerInterface(InterfaceId_ERC721Exists);
+  }
+
   /**
    * @dev Gets the balance of the specified address
    * @param _owner address to query the balance of
@@ -92,10 +122,8 @@ contract ERC721BasicToken is ERC721Basic {
     require(_to != owner);
     require(msg.sender == owner || isApprovedForAll(owner, msg.sender));
 
-    if (getApproved(_tokenId) != address(0) || _to != address(0)) {
-      tokenApprovals[_tokenId] = _to;
-      emit Approval(owner, _to, _tokenId);
-    }
+    tokenApprovals[_tokenId] = _to;
+    emit Approval(owner, _to, _tokenId);
   }
 
   /**

diff --git a/contracts/token/ERC721/ERC721Token.sol b/contracts/token/ERC721/ERC721Token.sol
@@ -2,6 +2,7 @@ pragma solidity ^0.4.23;
 
 import "./ERC721.sol";
 import "./ERC721BasicToken.sol";
+import "../../introspection/SupportsInterfaceWithLookup.sol";
 
 
 /**
@@ -10,7 +11,24 @@ import "./ERC721BasicToken.sol";
  * Moreover, it includes approve all functionality using operator terminology
  * @dev see https://github.com/ethereum/EIPs/blob/master/EIPS/eip-721.md
  */
-contract ERC721Token is ERC721, ERC721BasicToken {
+contract ERC721Token is SupportsInterfaceWithLookup, ERC721BasicToken, ERC721 {
+
+  bytes4 private constant InterfaceId_ERC721Enumerable = 0x780e9d63;
+  /**
+   * 0x780e9d63 ===
+   *   bytes4(keccak256('totalSupply()')) ^
+   *   bytes4(keccak256('tokenOfOwnerByIndex(address,uint256)')) ^
+   *   bytes4(keccak256('tokenByIndex(uint256)'))
+   */
+
+  bytes4 private constant InterfaceId_ERC721Metadata = 0x5b5e139f;
+  /**
+   * 0x5b5e139f ===
+   *   bytes4(keccak256('name()')) ^
+   *   bytes4(keccak256('symbol()')) ^
+   *   bytes4(keccak256('tokenURI(uint256)'))
+   */
+
   // Token name
   string internal name_;
 
@@ -38,6 +56,10 @@ contract ERC721Token is ERC721, ERC721BasicToken {
   constructor(string _name, string _symbol) public {
     name_ = _name;
     symbol_ = _symbol;
+
+    // register the supported interfaces to conform to ERC721 via ERC165
+    _registerInterface(InterfaceId_ERC721Enumerable);
+    _registerInterface(InterfaceId_ERC721Metadata);
   }
 
   /**

diff --git a/test/token/ERC721/ERC721BasicToken.behaviour.js b/test/token/ERC721/ERC721BasicToken.behaviour.js
@@ -294,6 +294,20 @@ export default function shouldBehaveLikeERC721BasicToken (accounts) {
               log.args._tokenId.toNumber().should.be.equal(tokenId);
               log.args._data.should.be.equal(data);
             });
+
+            describe('with an invalid token id', function () {
+              it('reverts', async function () {
+                await assertRevert(
+                  transferFun.call(
+                    this,
+                    owner,
+                    this.to,
+                    unknownTokenId,
+                    { from: owner },
+                  )
+                );
+              });
+            });
           });
         };
 
@@ -366,10 +380,7 @@ export default function shouldBehaveLikeERC721BasicToken (accounts) {
           });
 
           itClearsApproval();
-
-          it('does not emit an approval event', async function () {
-            logs.length.should.be.equal(0);
-          });
+          itEmitsApprovalEvent(ZERO_ADDRESS);
         });
 
         context('when there was a prior approval', function () {