From e1dc1411fc154970216eb7ccc3855b1052f81889 Mon Sep 17 00:00:00 2001 From: Matt Condon Date: Tue, 12 Jun 2018 14:46:32 -0700 Subject: [PATCH] WIP: Fix/erc721 (#993) * fix: defer lookup-specific info to implementations * fix: change inheritance order, fix import --- contracts/token/ERC721/ERC721.sol | 30 --------------- contracts/token/ERC721/ERC721Basic.sol | 32 +--------------- contracts/token/ERC721/ERC721BasicToken.sol | 38 ++++++++++++++++--- contracts/token/ERC721/ERC721Token.sol | 24 +++++++++++- .../ERC721/ERC721BasicToken.behaviour.js | 19 ++++++++-- 5 files changed, 73 insertions(+), 70 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 524ce00ec..4a0b4fb75 100644 --- 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 index e7d60c9ea..8dd5ad130 100644 --- 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 index f0aae755c..0f2381bef 100644 --- 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 index cee28ce2c..d48c34b4e 100644 --- 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 index a10d1a018..d66ec172f 100644 --- 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 () {