From 5fc5ded661f8b6f59763ec26c5c4b8dd7430e369 Mon Sep 17 00:00:00 2001 From: Facundo Spagnuolo Date: Tue, 16 Jan 2018 15:57:44 -0300 Subject: [PATCH 1/5] Provide ERC721 required functionality interface --- contracts/token/ERC721.sol | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 contracts/token/ERC721.sol diff --git a/contracts/token/ERC721.sol b/contracts/token/ERC721.sol new file mode 100644 index 00000000000..f88376f6110 --- /dev/null +++ b/contracts/token/ERC721.sol @@ -0,0 +1,16 @@ +pragma solidity ^0.4.18; + +/** + * @title ERC721 interface + * @dev see https://github.com/ethereum/eips/issues/721 + */ +contract ERC721 { + event Transfer(address indexed _from, address indexed _to, uint256 _tokenId); + event Approval(address indexed _owner, address indexed _approved, uint256 _tokenId); + + function balanceOf(address _owner) public view returns (uint256 _balance); + function ownerOf(uint256 _tokenId) public view returns (address _owner); + function transfer(address _to, uint256 _tokenId) public; + function approve(address _to, uint256 _tokenId) public; + function takeOwnership(uint256 _tokenId) public; +} From b09d7aada41db444368cf3754b1015b9a1d5e7fe Mon Sep 17 00:00:00 2001 From: Facundo Spagnuolo Date: Tue, 16 Jan 2018 15:57:59 -0300 Subject: [PATCH 2/5] Implement ERC721 required functionality --- contracts/token/ERC721Token.sol | 208 ++++++++++++++++++++++++++++++++ 1 file changed, 208 insertions(+) create mode 100644 contracts/token/ERC721Token.sol diff --git a/contracts/token/ERC721Token.sol b/contracts/token/ERC721Token.sol new file mode 100644 index 00000000000..53457dd239d --- /dev/null +++ b/contracts/token/ERC721Token.sol @@ -0,0 +1,208 @@ +pragma solidity ^0.4.18; + +import './ERC721.sol'; +import '../math/SafeMath.sol'; + +/** + * @title ERC721Token + * Generic implementation for the required functionality of the ERC721 standard + */ +contract ERC721Token is ERC721 { + using SafeMath for uint256; + + // Total amount of tokens + uint256 private totalTokens; + + // Mapping from token ID to owner + mapping (uint256 => address) private tokenOwner; + + // Mapping from token ID to approved address + mapping (uint256 => address) private tokenApprovals; + + // Mapping from owner to list of owned token IDs + mapping (address => uint256[]) private ownedTokens; + + // Mapping from token ID to index of the owner tokens list + mapping(uint256 => uint256) private ownedTokensIndex; + + /** + * @dev Guarantees msg.sender is owner of the given token + * @param _tokenId uint256 ID of the token to validate its ownership belongs to msg.sender + */ + modifier onlyOwnerOf(uint256 _tokenId) { + require(ownerOf(_tokenId) == msg.sender); + _; + } + + /** + * @dev Gets the total amount of tokens stored by the contract + * @return uint256 representing the total amount of tokens + */ + function totalSupply() public view returns (uint256) { + return totalTokens; + } + + /** + * @dev Gets the balance of the specified address + * @param _owner address to query the balance of + * @return uint256 representing the amount owned by the passed address + */ + function balanceOf(address _owner) public view returns (uint256) { + return ownedTokens[_owner].length; + } + + /** + * @dev Gets the list of tokens owned by a given address + * @param _owner address to query the tokens of + * @return uint256[] representing the list of tokens owned by the passed address + */ + function tokensOf(address _owner) public view returns (uint256[]) { + return ownedTokens[_owner]; + } + + /** + * @dev Gets the owner of the specified token ID + * @param _tokenId uint256 ID of the token to query the owner of + * @return owner address currently marked as the owner of the given token ID + */ + function ownerOf(uint256 _tokenId) public view returns (address) { + address owner = tokenOwner[_tokenId]; + require(owner != address(0)); + return owner; + } + + /** + * @dev Gets the approved address to take ownership of a given token ID + * @param _tokenId uint256 ID of the token to query the approval of + * @return address currently approved to take ownership of the given token ID + */ + function approvedFor(uint256 _tokenId) public view returns (address) { + return tokenApprovals[_tokenId]; + } + + /** + * @dev Transfers the ownership of a given token ID to another address + * @param _to address to receive the ownership of the given token ID + * @param _tokenId uint256 ID of the token to be transferred + */ + function transfer(address _to, uint256 _tokenId) public onlyOwnerOf(_tokenId) { + clearApprovalAndTransfer(msg.sender, _to, _tokenId); + } + + /** + * @dev Approves another address to claim for the ownership of the given token ID + * @param _to address to be approved for the given token ID + * @param _tokenId uint256 ID of the token to be approved + */ + function approve(address _to, uint256 _tokenId) public onlyOwnerOf(_tokenId) { + address owner = ownerOf(_tokenId); + require(_to != owner); + if(approvedFor(_tokenId) == 0 && _to == 0) return; + tokenApprovals[_tokenId] = _to; + Approval(owner, _to, _tokenId); + } + + /** + * @dev Claims the ownership of a given token ID + * @param _tokenId uint256 ID of the token being claimed by the msg.sender + */ + function takeOwnership(uint256 _tokenId) public { + require(isApprovedFor(_tokenId)); + clearApprovalAndTransfer(ownerOf(_tokenId), msg.sender, _tokenId); + } + + /** + * @dev Mint token function + * @param _to The address that will own the minted token + * @param _tokenId uint256 ID of the token to be minted by the msg.sender + */ + function mint(address _to, uint256 _tokenId) internal { + require(_to != address(0)); + addToken(_to, _tokenId); + Transfer(0x0, _to, _tokenId); + } + + /** + * @dev Burns a specific token + * @param _tokenId uint256 ID of the token being burned by the msg.sender + */ + function burn(uint256 _tokenId) onlyOwnerOf(_tokenId) internal { + if(approvedFor(_tokenId) != 0) { + clearApproval(msg.sender, _tokenId); + } + removeToken(msg.sender, _tokenId); + Transfer(msg.sender, 0x0, _tokenId); + } + + /** + * @dev Internal function to clear current approval and transfer the ownership of a given token ID + * @param _from address which you want to send tokens from + * @param _to address which you want to transfer the token to + * @param _tokenId uint256 ID of the token to be transferred + */ + function clearApprovalAndTransfer(address _from, address _to, uint256 _tokenId) internal { + require(_to != address(0)); + require(_to != ownerOf(_tokenId)); + require(ownerOf(_tokenId) == _from); + + clearApproval(_from, _tokenId); + removeToken(_from, _tokenId); + addToken(_to, _tokenId); + Transfer(_from, _to, _tokenId); + } + + /** + * @dev Internal function to clear current approval of a given token ID + * @param _tokenId uint256 ID of the token to be transferred + */ + function clearApproval(address _owner, uint256 _tokenId) internal { + require(ownerOf(_tokenId) == _owner); + tokenApprovals[_tokenId] = 0; + Approval(_owner, 0, _tokenId); + } + + /** + * @dev Internal function to add a token ID to the list of a given address + * @param _to address representing the new owner of the given token ID + * @param _tokenId uint256 ID of the token to be added to the tokens list of the given address + */ + function addToken(address _to, uint256 _tokenId) internal { + require(tokenOwner[_tokenId] == address(0)); + tokenOwner[_tokenId] = _to; + uint256 length = balanceOf(_to); + ownedTokens[_to].push(_tokenId); + ownedTokensIndex[_tokenId] = length; + totalTokens = totalTokens.add(1); + } + + /** + * @dev Internal function to remove a token ID from the list of a given address + * @param _from address representing the previous owner of the given token ID + * @param _tokenId uint256 ID of the token to be removed from the tokens list of the given address + */ + function removeToken(address _from, uint256 _tokenId) internal { + require(balanceOf(_from) > 0); + require(ownerOf(_tokenId) == _from); + + uint256 tokenIndex = ownedTokensIndex[_tokenId]; + uint256 lastTokenIndex = balanceOf(_from).sub(1); + uint256 lastToken = ownedTokens[_from][lastTokenIndex]; + + tokenOwner[_tokenId] = 0; + ownedTokens[_from][tokenIndex] = lastToken; + ownedTokens[_from][lastTokenIndex] = 0; + ownedTokens[_from].length--; + ownedTokensIndex[_tokenId] = 0; + ownedTokensIndex[lastToken] = tokenIndex; + totalTokens = totalTokens.sub(1); + } + + /** + * @dev Tells whether the msg.sender is approved for the given token ID or not + * @param _tokenId uint256 ID of the token to query the approval of + * @return bool whether the msg.sender is approved for the given token ID or not + */ + function isApprovedFor(uint256 _tokenId) internal view returns (bool) { + return approvedFor(_tokenId) == msg.sender; + } +} From 5b50e99a0df4c197e77f9d195fd0fee785f433ba Mon Sep 17 00:00:00 2001 From: Facundo Spagnuolo Date: Tue, 16 Jan 2018 15:58:09 -0300 Subject: [PATCH 3/5] ERC721 required functionality tests --- contracts/mocks/ERC721TokenMock.sol | 19 + test/token/ERC721Token.test.js | 534 ++++++++++++++++++++++++++++ 2 files changed, 553 insertions(+) create mode 100644 contracts/mocks/ERC721TokenMock.sol create mode 100644 test/token/ERC721Token.test.js diff --git a/contracts/mocks/ERC721TokenMock.sol b/contracts/mocks/ERC721TokenMock.sol new file mode 100644 index 00000000000..b14f6664125 --- /dev/null +++ b/contracts/mocks/ERC721TokenMock.sol @@ -0,0 +1,19 @@ +pragma solidity ^0.4.18; + +import '../token/ERC721Token.sol'; + +/** + * @title ERC721TokenMock + * This mock just provides a public mint and burn functions for testing purposes. + */ +contract ERC721TokenMock is ERC721Token { + function ERC721TokenMock() ERC721Token() public { } + + function publicMint(address _to, uint256 _tokenId) public { + super.mint(_to, _tokenId); + } + + function publicBurn(uint256 _tokenId) public { + super.burn(_tokenId); + } +} diff --git a/test/token/ERC721Token.test.js b/test/token/ERC721Token.test.js new file mode 100644 index 00000000000..d1ec5c81ce4 --- /dev/null +++ b/test/token/ERC721Token.test.js @@ -0,0 +1,534 @@ +import assertRevert from '../helpers/assertRevert'; +const BigNumber = web3.BigNumber; +const ERC721Token = artifacts.require('ERC721TokenMock.sol'); + +require('chai') + .use(require('chai-as-promised')) + .use(require('chai-bignumber')(BigNumber)) + .should(); + +contract('ERC721Token', accounts => { + let token = null; + const _firstTokenId = 1; + const _secondTokenId = 2; + const _unknownTokenId = 3; + const _creator = accounts[0]; + const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'; + + beforeEach(async function () { + token = await ERC721Token.new({ from: _creator }); + await token.publicMint(_creator, _firstTokenId, { from: _creator }); + await token.publicMint(_creator, _secondTokenId, { from: _creator }); + }); + + describe('totalSupply', function () { + it('has a total supply equivalent to the inital supply', async function () { + const totalSupply = await token.totalSupply(); + totalSupply.should.be.bignumber.equal(2); + }); + }); + + describe('balanceOf', function () { + describe('when the given address owns some tokens', function () { + it('returns the amount of tokens owned by the given address', async function () { + const balance = await token.balanceOf(_creator); + balance.should.be.bignumber.equal(2); + }); + }); + + describe('when the given address does not own any tokens', function () { + it('returns 0', async function () { + const balance = await token.balanceOf(accounts[1]); + balance.should.be.bignumber.equal(0); + }); + }); + }); + + describe('ownerOf', function () { + describe('when the given token ID was tracked by this token', function () { + const tokenId = _firstTokenId; + + it('returns the owner of the given token ID', async function () { + const owner = await token.ownerOf(tokenId); + owner.should.be.equal(_creator); + }); + }); + + describe('when the given token ID was not tracked by this token', function () { + const tokenId = _unknownTokenId; + + it('reverts', async function () { + await assertRevert(token.ownerOf(tokenId)); + }); + }); + }); + + describe('mint', function () { + describe('when the given token ID was not tracked by this contract', function () { + const tokenId = _unknownTokenId; + + describe('when the given address is not the zero address', function () { + const to = accounts[1]; + + it('mints the given token ID to the given address', async function () { + const previousBalance = await token.balanceOf(to); + + await token.publicMint(to, tokenId); + + const owner = await token.ownerOf(tokenId); + owner.should.be.equal(to); + + const balance = await token.balanceOf(to); + balance.should.be.bignumber.equal(previousBalance + 1); + }); + + it('adds that token to the token list of the owner', async function () { + await token.publicMint(to, tokenId); + + const tokens = await token.tokensOf(to); + tokens.length.should.be.equal(1); + tokens[0].should.be.bignumber.equal(tokenId); + }); + + it('emits a transfer event', async function () { + const { logs } = await token.publicMint(to, tokenId); + + logs.length.should.be.equal(1); + logs[0].event.should.be.eq('Transfer'); + logs[0].args._from.should.be.equal(ZERO_ADDRESS); + logs[0].args._to.should.be.equal(to); + logs[0].args._tokenId.should.be.bignumber.equal(tokenId); + }); + }); + + describe('when the given address is the zero address', function () { + const to = ZERO_ADDRESS; + + it('reverts', async function () { + await assertRevert(token.publicMint(to, tokenId)); + }); + }); + }); + + describe('when the given token ID was already tracked by this contract', function () { + const tokenId = _firstTokenId; + + it('reverts', async function () { + await assertRevert(token.publicMint(accounts[1], tokenId)); + }); + }); + }); + + describe('burn', function () { + describe('when the given token ID was tracked by this contract', function () { + const tokenId = _firstTokenId; + + describe('when the msg.sender owns given token', function () { + const sender = _creator; + + it('burns the given token ID and adjusts the balance of the owner', async function () { + const previousBalance = await token.balanceOf(sender); + + await token.publicBurn(tokenId, { from: sender }); + + await assertRevert(token.ownerOf(tokenId)); + const balance = await token.balanceOf(sender); + balance.should.be.bignumber.equal(previousBalance - 1); + }); + + it('removes that token from the token list of the owner', async function () { + await token.publicBurn(tokenId, { from: sender }); + + const tokens = await token.tokensOf(sender); + tokens.length.should.be.equal(1); + tokens[0].should.be.bignumber.equal(_secondTokenId); + }); + + it('emits a burn event', async function () { + const { logs } = await token.publicBurn(tokenId, { from: sender }); + + logs.length.should.be.equal(1); + logs[0].event.should.be.eq('Transfer'); + logs[0].args._from.should.be.equal(sender); + logs[0].args._to.should.be.equal(ZERO_ADDRESS); + logs[0].args._tokenId.should.be.bignumber.equal(tokenId); + }); + + describe('when there is an approval for the given token ID', function () { + beforeEach(async function () { + await token.approve(accounts[1], tokenId, { from: sender }); + }); + + it('clears the approval', async function () { + await token.publicBurn(tokenId, { from: sender }); + const approvedAccount = await token.approvedFor(tokenId); + approvedAccount.should.be.equal(ZERO_ADDRESS); + }); + + it('emits an approval event', async function () { + const { logs } = await token.publicBurn(tokenId, { from: sender }); + + logs.length.should.be.equal(2); + + logs[0].event.should.be.eq('Approval'); + logs[0].args._owner.should.be.equal(sender); + logs[0].args._approved.should.be.equal(ZERO_ADDRESS); + logs[0].args._tokenId.should.be.bignumber.equal(tokenId); + }); + }); + }); + + describe('when the msg.sender does not own given token', function () { + const sender = accounts[1]; + + it('reverts', async function () { + await assertRevert(token.publicBurn(tokenId, { from: sender })); + }); + }); + }); + + describe('when the given token ID was not tracked by this contract', function () { + const tokenID = _unknownTokenId; + + it('reverts', async function () { + await assertRevert(token.publicBurn(tokenID, { from: _creator })); + }); + }); + }); + + describe('transfer', function () { + describe('when the address to transfer the token to is not the zero address', function () { + const to = accounts[1]; + + describe('when the given token ID was tracked by this token', function () { + const tokenId = _firstTokenId; + + describe('when the msg.sender is the owner of the given token ID', function () { + const sender = _creator; + + it('transfers the ownership of the given token ID to the given address', async function () { + await token.transfer(to, tokenId, { from: sender }); + + const newOwner = await token.ownerOf(tokenId); + newOwner.should.be.equal(to); + }); + + it('clears the approval for the token ID', async function () { + await token.approve(accounts[2], tokenId, { from: sender }); + + await token.transfer(to, tokenId, { from: sender }); + + const approvedAccount = await token.approvedFor(tokenId); + approvedAccount.should.be.equal(ZERO_ADDRESS); + }); + + it('emits an approval and transfer events', async function () { + const { logs } = await token.transfer(to, tokenId, { from: sender }); + + logs.length.should.be.equal(2); + + logs[0].event.should.be.eq('Approval'); + logs[0].args._owner.should.be.equal(sender); + logs[0].args._approved.should.be.equal(ZERO_ADDRESS); + logs[0].args._tokenId.should.be.bignumber.equal(tokenId); + + logs[1].event.should.be.eq('Transfer'); + logs[1].args._from.should.be.equal(sender); + logs[1].args._to.should.be.equal(to); + logs[1].args._tokenId.should.be.bignumber.equal(tokenId); + }); + + it('adjusts owners balances', async function () { + const previousBalance = await token.balanceOf(sender); + await token.transfer(to, tokenId, { from: sender }); + + const newOwnerBalance = await token.balanceOf(to); + newOwnerBalance.should.be.bignumber.equal(1); + + const previousOwnerBalance = await token.balanceOf(_creator); + previousOwnerBalance.should.be.bignumber.equal(previousBalance - 1); + }); + + it('adds the token to the tokens list of the new owner', async function () { + await token.transfer(to, tokenId, { from: sender }); + + const tokenIDs = await token.tokensOf(to); + tokenIDs.length.should.be.equal(1); + tokenIDs[0].should.be.bignumber.equal(tokenId); + }); + }); + + describe('when the msg.sender is not the owner of the given token ID', function () { + const sender = accounts[2]; + + it('reverts', async function () { + await assertRevert(token.transfer(to, tokenId, { from: sender })); + }); + }); + }); + + describe('when the given token ID was not tracked by this token', function () { + let tokenId = _unknownTokenId; + + it('reverts', async function () { + await assertRevert(token.transfer(to, tokenId, { from: _creator })); + }); + }); + }); + + describe('when the address to transfer the token to is the zero address', function () { + const to = ZERO_ADDRESS; + + it('reverts', async function () { + await assertRevert(token.transfer(to, 0, { from: _creator })); + }); + }); + }); + + describe('approve', function () { + describe('when the given token ID was already tracked by this contract', function () { + const tokenId = _firstTokenId; + + describe('when the sender owns the given token ID', function () { + const sender = _creator; + + describe('when the address that receives the approval is the 0 address', function () { + const to = ZERO_ADDRESS; + + describe('when there was no approval for the given token ID before', function () { + it('clears the approval for that token', async function () { + await token.approve(to, tokenId, { from: sender }); + + const approvedAccount = await token.approvedFor(tokenId); + approvedAccount.should.be.equal(to); + }); + + it('does not emit an approval event', async function () { + const { logs } = await token.approve(to, tokenId, { from: sender }); + + logs.length.should.be.equal(0); + }); + }); + + describe('when the given token ID was approved for another account', function () { + beforeEach(async function () { + await token.approve(accounts[2], tokenId, { from: sender }); + }); + + it('clears the approval for the token ID', async function () { + await token.approve(to, tokenId, { from: sender }); + + const approvedAccount = await token.approvedFor(tokenId); + approvedAccount.should.be.equal(to); + }); + + it('emits an approval event', async function () { + const { logs } = await token.approve(to, tokenId, { from: sender }); + + logs.length.should.be.equal(1); + logs[0].event.should.be.eq('Approval'); + logs[0].args._owner.should.be.equal(sender); + logs[0].args._approved.should.be.equal(to); + logs[0].args._tokenId.should.be.bignumber.equal(tokenId); + }); + }); + }); + + describe('when the address that receives the approval is not the 0 address', function () { + describe('when the address that receives the approval is different than the owner', function () { + const to = accounts[1]; + + describe('when there was no approval for the given token ID before', function () { + it('approves the token ID to the given address', async function () { + await token.approve(to, tokenId, { from: sender }); + + const approvedAccount = await token.approvedFor(tokenId); + approvedAccount.should.be.equal(to); + }); + + it('emits an approval event', async function () { + const { logs } = await token.approve(to, tokenId, { from: sender }); + + logs.length.should.be.equal(1); + logs[0].event.should.be.eq('Approval'); + logs[0].args._owner.should.be.equal(sender); + logs[0].args._approved.should.be.equal(to); + logs[0].args._tokenId.should.be.bignumber.equal(tokenId); + }); + }); + + describe('when the given token ID was approved for the same account', function () { + beforeEach(async function () { + await token.approve(to, tokenId, { from: sender }); + }); + + it('keeps the approval to the given address', async function () { + await token.approve(to, tokenId, { from: sender }); + + const approvedAccount = await token.approvedFor(tokenId); + approvedAccount.should.be.equal(to); + }); + + it('emits an approval event', async function () { + const { logs } = await token.approve(to, tokenId, { from: sender }); + + logs.length.should.be.equal(1); + logs[0].event.should.be.eq('Approval'); + logs[0].args._owner.should.be.equal(sender); + logs[0].args._approved.should.be.equal(to); + logs[0].args._tokenId.should.be.bignumber.equal(tokenId); + }); + }); + + describe('when the given token ID was approved for another account', function () { + beforeEach(async function () { + await token.approve(accounts[2], tokenId, { from: sender }); + }); + + it('changes the approval to the given address', async function () { + await token.approve(to, tokenId, { from: sender }); + + const approvedAccount = await token.approvedFor(tokenId); + approvedAccount.should.be.equal(to); + }); + + it('emits an approval event', async function () { + const { logs } = await token.approve(to, tokenId, { from: sender }); + + logs.length.should.be.equal(1); + logs[0].event.should.be.eq('Approval'); + logs[0].args._owner.should.be.equal(sender); + logs[0].args._approved.should.be.equal(to); + logs[0].args._tokenId.should.be.bignumber.equal(tokenId); + }); + }); + }); + + describe('when the address that receives the approval is the owner', function () { + const to = _creator; + + describe('when there was no approval for the given token ID before', function () { + it('reverts', async function () { + await assertRevert(token.approve(to, tokenId, { from: sender })); + }); + }); + + describe('when the given token ID was approved for another account', function () { + beforeEach(async function () { + await token.approve(accounts[2], tokenId, { from: sender }); + }); + + it('reverts', async function () { + await assertRevert(token.approve(to, tokenId, { from: sender })); + }); + }); + }); + }); + }); + + describe('when the sender does not own the given token ID', function () { + const sender = accounts[1]; + + it('reverts', async function () { + await assertRevert(token.approve(accounts[2], tokenId, { from: sender })); + }); + }); + }); + + describe('when the given token ID was not tracked by the contract before', function () { + const tokenId = _unknownTokenId; + + it('reverts', async function () { + await assertRevert(token.approve(accounts[1], tokenId, { from: _creator })); + }); + }); + }); + + describe('takeOwnership', function () { + describe('when the given token ID was already tracked by this contract', function () { + const tokenId = _firstTokenId; + + describe('when the sender has the approval for the token ID', function () { + const sender = accounts[1]; + + beforeEach(async function () { + await token.approve(sender, tokenId, { from: _creator }); + }); + + it('transfers the ownership of the given token ID to the given address', async function () { + await token.takeOwnership(tokenId, { from: sender }); + + const newOwner = await token.ownerOf(tokenId); + newOwner.should.be.equal(sender); + }); + + it('clears the approval for the token ID', async function () { + await token.takeOwnership(tokenId, { from: sender }); + + const approvedAccount = await token.approvedFor(tokenId); + approvedAccount.should.be.equal(ZERO_ADDRESS); + }); + + it('emits an approval and transfer events', async function () { + const { logs } = await token.takeOwnership(tokenId, { from: sender }); + + logs.length.should.be.equal(2); + + logs[0].event.should.be.eq('Approval'); + logs[0].args._owner.should.be.equal(_creator); + logs[0].args._approved.should.be.equal(ZERO_ADDRESS); + logs[0].args._tokenId.should.be.bignumber.equal(tokenId); + + logs[1].event.should.be.eq('Transfer'); + logs[1].args._from.should.be.equal(_creator); + logs[1].args._to.should.be.equal(sender); + logs[1].args._tokenId.should.be.bignumber.equal(tokenId); + }); + + it('adjusts owners balances', async function () { + const previousBalance = await token.balanceOf(_creator); + + await token.takeOwnership(tokenId, { from: sender }); + + const newOwnerBalance = await token.balanceOf(sender); + newOwnerBalance.should.be.bignumber.equal(1); + + const previousOwnerBalance = await token.balanceOf(_creator); + previousOwnerBalance.should.be.bignumber.equal(previousBalance - 1); + }); + + it('adds the token to the tokens list of the new owner', async function () { + await token.takeOwnership(tokenId, { from: sender }); + + const tokenIDs = await token.tokensOf(sender); + tokenIDs.length.should.be.equal(1); + tokenIDs[0].should.be.bignumber.equal(tokenId); + }); + }); + + describe('when the sender does not have an approval for the token ID', function () { + const sender = accounts[1]; + + it('reverts', async function () { + await assertRevert(token.takeOwnership(tokenId, { from: sender })); + }); + }); + + describe('when the sender is already the owner of the token', function () { + const sender = _creator; + + it('reverts', async function () { + await assertRevert(token.takeOwnership(tokenId, { from: sender })); + }); + }); + }); + + describe('when the given token ID was not tracked by the contract before', function () { + const tokenId = _unknownTokenId; + + it('reverts', async function () { + await assertRevert(token.takeOwnership(tokenId, { from: _creator })); + }); + }); + }); +}); From af337047a4eb323b2c914bf71eeb0b15e33a65a5 Mon Sep 17 00:00:00 2001 From: Facundo Spagnuolo Date: Wed, 17 Jan 2018 12:00:15 -0300 Subject: [PATCH 4/5] Fix solint errors --- contracts/mocks/ERC721TokenMock.sol | 2 +- contracts/token/ERC721Token.sol | 13 +++++++------ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/contracts/mocks/ERC721TokenMock.sol b/contracts/mocks/ERC721TokenMock.sol index b14f6664125..394d408f4e4 100644 --- a/contracts/mocks/ERC721TokenMock.sol +++ b/contracts/mocks/ERC721TokenMock.sol @@ -1,6 +1,6 @@ pragma solidity ^0.4.18; -import '../token/ERC721Token.sol'; +import "../token/ERC721Token.sol"; /** * @title ERC721TokenMock diff --git a/contracts/token/ERC721Token.sol b/contracts/token/ERC721Token.sol index 53457dd239d..8e30fba6e18 100644 --- a/contracts/token/ERC721Token.sol +++ b/contracts/token/ERC721Token.sol @@ -1,7 +1,7 @@ pragma solidity ^0.4.18; -import './ERC721.sol'; -import '../math/SafeMath.sol'; +import "./ERC721.sol"; +import "../math/SafeMath.sol"; /** * @title ERC721Token @@ -97,9 +97,10 @@ contract ERC721Token is ERC721 { function approve(address _to, uint256 _tokenId) public onlyOwnerOf(_tokenId) { address owner = ownerOf(_tokenId); require(_to != owner); - if(approvedFor(_tokenId) == 0 && _to == 0) return; - tokenApprovals[_tokenId] = _to; - Approval(owner, _to, _tokenId); + if (approvedFor(_tokenId) != 0 || _to != 0) { + tokenApprovals[_tokenId] = _to; + Approval(owner, _to, _tokenId); + } } /** @@ -127,7 +128,7 @@ contract ERC721Token is ERC721 { * @param _tokenId uint256 ID of the token being burned by the msg.sender */ function burn(uint256 _tokenId) onlyOwnerOf(_tokenId) internal { - if(approvedFor(_tokenId) != 0) { + if (approvedFor(_tokenId) != 0) { clearApproval(msg.sender, _tokenId); } removeToken(msg.sender, _tokenId); From 23afc74b59ca536661638b09111460a8722b6458 Mon Sep 17 00:00:00 2001 From: Facundo Spagnuolo Date: Wed, 17 Jan 2018 18:26:49 -0300 Subject: [PATCH 5/5] Address feedback comments for ERC721 --- contracts/mocks/ERC721TokenMock.sol | 8 +++---- contracts/token/ERC721Token.sol | 37 ++++++++++++++++------------- test/token/ERC721Token.test.js | 28 +++++++++++----------- 3 files changed, 39 insertions(+), 34 deletions(-) diff --git a/contracts/mocks/ERC721TokenMock.sol b/contracts/mocks/ERC721TokenMock.sol index 394d408f4e4..42b12cc29a0 100644 --- a/contracts/mocks/ERC721TokenMock.sol +++ b/contracts/mocks/ERC721TokenMock.sol @@ -9,11 +9,11 @@ import "../token/ERC721Token.sol"; contract ERC721TokenMock is ERC721Token { function ERC721TokenMock() ERC721Token() public { } - function publicMint(address _to, uint256 _tokenId) public { - super.mint(_to, _tokenId); + function mint(address _to, uint256 _tokenId) public { + super._mint(_to, _tokenId); } - function publicBurn(uint256 _tokenId) public { - super.burn(_tokenId); + function burn(uint256 _tokenId) public { + super._burn(_tokenId); } } diff --git a/contracts/token/ERC721Token.sol b/contracts/token/ERC721Token.sol index 8e30fba6e18..0b86886ee21 100644 --- a/contracts/token/ERC721Token.sol +++ b/contracts/token/ERC721Token.sol @@ -108,7 +108,7 @@ contract ERC721Token is ERC721 { * @param _tokenId uint256 ID of the token being claimed by the msg.sender */ function takeOwnership(uint256 _tokenId) public { - require(isApprovedFor(_tokenId)); + require(isApprovedFor(msg.sender, _tokenId)); clearApprovalAndTransfer(ownerOf(_tokenId), msg.sender, _tokenId); } @@ -117,7 +117,7 @@ contract ERC721Token is ERC721 { * @param _to The address that will own the minted token * @param _tokenId uint256 ID of the token to be minted by the msg.sender */ - function mint(address _to, uint256 _tokenId) internal { + function _mint(address _to, uint256 _tokenId) internal { require(_to != address(0)); addToken(_to, _tokenId); Transfer(0x0, _to, _tokenId); @@ -127,7 +127,7 @@ contract ERC721Token is ERC721 { * @dev Burns a specific token * @param _tokenId uint256 ID of the token being burned by the msg.sender */ - function burn(uint256 _tokenId) onlyOwnerOf(_tokenId) internal { + function _burn(uint256 _tokenId) onlyOwnerOf(_tokenId) internal { if (approvedFor(_tokenId) != 0) { clearApproval(msg.sender, _tokenId); } @@ -135,6 +135,17 @@ contract ERC721Token is ERC721 { Transfer(msg.sender, 0x0, _tokenId); } + /** + * @dev Tells whether the msg.sender is approved for the given token ID or not + * This function is not private so it can be extended in further implementations like the operatable ERC721 + * @param _owner address of the owner to query the approval of + * @param _tokenId uint256 ID of the token to query the approval of + * @return bool whether the msg.sender is approved for the given token ID or not + */ + function isApprovedFor(address _owner, uint256 _tokenId) internal view returns (bool) { + return approvedFor(_tokenId) == _owner; + } + /** * @dev Internal function to clear current approval and transfer the ownership of a given token ID * @param _from address which you want to send tokens from @@ -156,7 +167,7 @@ contract ERC721Token is ERC721 { * @dev Internal function to clear current approval of a given token ID * @param _tokenId uint256 ID of the token to be transferred */ - function clearApproval(address _owner, uint256 _tokenId) internal { + function clearApproval(address _owner, uint256 _tokenId) private { require(ownerOf(_tokenId) == _owner); tokenApprovals[_tokenId] = 0; Approval(_owner, 0, _tokenId); @@ -167,7 +178,7 @@ contract ERC721Token is ERC721 { * @param _to address representing the new owner of the given token ID * @param _tokenId uint256 ID of the token to be added to the tokens list of the given address */ - function addToken(address _to, uint256 _tokenId) internal { + function addToken(address _to, uint256 _tokenId) private { require(tokenOwner[_tokenId] == address(0)); tokenOwner[_tokenId] = _to; uint256 length = balanceOf(_to); @@ -181,8 +192,7 @@ contract ERC721Token is ERC721 { * @param _from address representing the previous owner of the given token ID * @param _tokenId uint256 ID of the token to be removed from the tokens list of the given address */ - function removeToken(address _from, uint256 _tokenId) internal { - require(balanceOf(_from) > 0); + function removeToken(address _from, uint256 _tokenId) private { require(ownerOf(_tokenId) == _from); uint256 tokenIndex = ownedTokensIndex[_tokenId]; @@ -192,18 +202,13 @@ contract ERC721Token is ERC721 { tokenOwner[_tokenId] = 0; ownedTokens[_from][tokenIndex] = lastToken; ownedTokens[_from][lastTokenIndex] = 0; + // Note that this will handle single-element arrays. In that case, both tokenIndex and lastTokenIndex are going to + // be zero. Then we can make sure that we will remove _tokenId from the ownedTokens list since we are first swapping + // the lastToken to the first position, and then dropping the element placed in the last position of the list + ownedTokens[_from].length--; ownedTokensIndex[_tokenId] = 0; ownedTokensIndex[lastToken] = tokenIndex; totalTokens = totalTokens.sub(1); } - - /** - * @dev Tells whether the msg.sender is approved for the given token ID or not - * @param _tokenId uint256 ID of the token to query the approval of - * @return bool whether the msg.sender is approved for the given token ID or not - */ - function isApprovedFor(uint256 _tokenId) internal view returns (bool) { - return approvedFor(_tokenId) == msg.sender; - } } diff --git a/test/token/ERC721Token.test.js b/test/token/ERC721Token.test.js index d1ec5c81ce4..9d03f944b5a 100644 --- a/test/token/ERC721Token.test.js +++ b/test/token/ERC721Token.test.js @@ -17,8 +17,8 @@ contract('ERC721Token', accounts => { beforeEach(async function () { token = await ERC721Token.new({ from: _creator }); - await token.publicMint(_creator, _firstTokenId, { from: _creator }); - await token.publicMint(_creator, _secondTokenId, { from: _creator }); + await token.mint(_creator, _firstTokenId, { from: _creator }); + await token.mint(_creator, _secondTokenId, { from: _creator }); }); describe('totalSupply', function () { @@ -73,7 +73,7 @@ contract('ERC721Token', accounts => { it('mints the given token ID to the given address', async function () { const previousBalance = await token.balanceOf(to); - await token.publicMint(to, tokenId); + await token.mint(to, tokenId); const owner = await token.ownerOf(tokenId); owner.should.be.equal(to); @@ -83,7 +83,7 @@ contract('ERC721Token', accounts => { }); it('adds that token to the token list of the owner', async function () { - await token.publicMint(to, tokenId); + await token.mint(to, tokenId); const tokens = await token.tokensOf(to); tokens.length.should.be.equal(1); @@ -91,7 +91,7 @@ contract('ERC721Token', accounts => { }); it('emits a transfer event', async function () { - const { logs } = await token.publicMint(to, tokenId); + const { logs } = await token.mint(to, tokenId); logs.length.should.be.equal(1); logs[0].event.should.be.eq('Transfer'); @@ -105,7 +105,7 @@ contract('ERC721Token', accounts => { const to = ZERO_ADDRESS; it('reverts', async function () { - await assertRevert(token.publicMint(to, tokenId)); + await assertRevert(token.mint(to, tokenId)); }); }); }); @@ -114,7 +114,7 @@ contract('ERC721Token', accounts => { const tokenId = _firstTokenId; it('reverts', async function () { - await assertRevert(token.publicMint(accounts[1], tokenId)); + await assertRevert(token.mint(accounts[1], tokenId)); }); }); }); @@ -129,7 +129,7 @@ contract('ERC721Token', accounts => { it('burns the given token ID and adjusts the balance of the owner', async function () { const previousBalance = await token.balanceOf(sender); - await token.publicBurn(tokenId, { from: sender }); + await token.burn(tokenId, { from: sender }); await assertRevert(token.ownerOf(tokenId)); const balance = await token.balanceOf(sender); @@ -137,7 +137,7 @@ contract('ERC721Token', accounts => { }); it('removes that token from the token list of the owner', async function () { - await token.publicBurn(tokenId, { from: sender }); + await token.burn(tokenId, { from: sender }); const tokens = await token.tokensOf(sender); tokens.length.should.be.equal(1); @@ -145,7 +145,7 @@ contract('ERC721Token', accounts => { }); it('emits a burn event', async function () { - const { logs } = await token.publicBurn(tokenId, { from: sender }); + const { logs } = await token.burn(tokenId, { from: sender }); logs.length.should.be.equal(1); logs[0].event.should.be.eq('Transfer'); @@ -160,13 +160,13 @@ contract('ERC721Token', accounts => { }); it('clears the approval', async function () { - await token.publicBurn(tokenId, { from: sender }); + await token.burn(tokenId, { from: sender }); const approvedAccount = await token.approvedFor(tokenId); approvedAccount.should.be.equal(ZERO_ADDRESS); }); it('emits an approval event', async function () { - const { logs } = await token.publicBurn(tokenId, { from: sender }); + const { logs } = await token.burn(tokenId, { from: sender }); logs.length.should.be.equal(2); @@ -182,7 +182,7 @@ contract('ERC721Token', accounts => { const sender = accounts[1]; it('reverts', async function () { - await assertRevert(token.publicBurn(tokenId, { from: sender })); + await assertRevert(token.burn(tokenId, { from: sender })); }); }); }); @@ -191,7 +191,7 @@ contract('ERC721Token', accounts => { const tokenID = _unknownTokenId; it('reverts', async function () { - await assertRevert(token.publicBurn(tokenID, { from: _creator })); + await assertRevert(token.burn(tokenID, { from: _creator })); }); }); });