Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
1192e68
Rename current ERC721 implementation to BaseERC721
facuspagnuolo Feb 26, 2018
ca163a8
Implement ERC721 optional & approveAll functionality
facuspagnuolo Feb 26, 2018
71cbc51
Support for new ERC721 interface
spalladino Mar 7, 2018
3745025
Add more tests for ERC721
spalladino Mar 8, 2018
559df81
Implement suggestions by @dekz
spalladino Mar 8, 2018
d726c79
Update comments in ERC721 contracts
spalladino Mar 8, 2018
54a1d2e
Implement tokensByIndex extension
spalladino Mar 9, 2018
851685c
Add default implementation for metadata URI
spalladino Mar 9, 2018
3cef880
Allow operators to call approve on a token
spalladino Mar 9, 2018
6f180a6
Remove gas stipend restriction in call to 721 receiver
spalladino Mar 9, 2018
6fbe771
Remove deprecated implementation
spalladino Mar 9, 2018
626742e
Add notice to isContract helper on constract constructors
spalladino Mar 20, 2018
95a1f9a
Change natspec delimiters for consistency
spalladino Mar 21, 2018
15f9556
Minor linting fixes
spalladino Mar 21, 2018
b332995
Add constant modifier to ERC721_RECEIVED magic value
spalladino Mar 21, 2018
f4748da
Use 4-params safeTransferFrom for implementing the 3-params overload
spalladino Mar 21, 2018
fb4f728
Minor text changes in natspec comments
spalladino Mar 21, 2018
6b98e4e
Use address(0) instead of 0 or 0x0
spalladino Mar 21, 2018
3f2ea8a
Use if-statements instead of boolean one-liners for clarity
spalladino Mar 21, 2018
74db03b
Keep ownedTokensCount state var in sync in full ERC721 implementation
spalladino Mar 21, 2018
981c6f7
Fix incorrect comparison when burning ERC721 tokens with metadata
spalladino Mar 21, 2018
73b77ae
Use address(0) instead of 0 in one more place in ERC721
spalladino Mar 21, 2018
eee5b0e
Throw when querying balance for the zero address
spalladino Mar 21, 2018
9deb637
Update links to approved version of EIP721
spalladino Mar 21, 2018
fe6e4ff
Use explicit size for uint
spalladino Mar 22, 2018
4836279
Remove unneeded internal function in ERC721
spalladino Mar 22, 2018
619ae84
Use underscore instead of 'do' prefix for internal methods in ERC721
spalladino Mar 22, 2018
2e593f2
Fix failing test due to events reordering in ERC721 safe transfer
spalladino Mar 22, 2018
6c09d20
Fix bug introduced in 74db03ba06
spalladino Mar 22, 2018
37929c8
Remove do prefix for internal setTokenUri method
spalladino Mar 22, 2018
3676b55
Allow transfers to self in ERC721
spalladino Mar 23, 2018
7815cc5
Merge branch 'master' into feature/full_erc721
frangio Mar 23, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add default implementation for metadata URI
This allows token implementation to be non-abstract
  • Loading branch information
spalladino committed Mar 9, 2018
commit 851685c40eb89428fb6582d713dedea5cca38f3e
27 changes: 3 additions & 24 deletions contracts/mocks/ERC721TokenMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import "../token/ERC721/ERC721Token.sol";
/**
* @title ERC721TokenMock
* This mock just provides a public mint and burn functions for testing purposes,
* and a mock metadata URI implementation
* and a public setter for metadata URI
*/
contract ERC721TokenMock is ERC721Token {
function ERC721TokenMock(string name, string symbol)
Expand All @@ -21,28 +21,7 @@ contract ERC721TokenMock is ERC721Token {
doBurn(ownerOf(_tokenId), _tokenId);
}

// Mock implementation for testing.
// Do not use this code in production!
function tokenURI(uint256 _tokenId) public view returns (string) {
require(exists(_tokenId));

bytes memory uri = new bytes(78 + 7);

uint256 i;
uint256 value = _tokenId;

uri[0] = "m";
uri[1] = "o";
uri[2] = "c";
uri[3] = "k";
uri[4] = ":";
uri[5] = "/";
uri[6] = "/";
for (i = 0; i < 78; i++) {
uri[6 + 78 - i] = byte(value % 10 + 48);
value = value / 10;
}

return string(uri);
function setTokenURI(uint256 _tokenId, string _uri) public {
doSetTokenURI(_tokenId, _uri);
}
}
30 changes: 30 additions & 0 deletions contracts/token/ERC721/ERC721Token.sol
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ contract ERC721Token is ERC721, ERC721BasicToken {
// Mapping from token id to position in the allTokens array
mapping(uint256 => uint256) internal allTokensIndex;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be missing something, but why is this mapping needed? As per the mint function, isn't the token's identifier the same thing as its index in the allTokens array?

allTokensIndex[_tokenId] = allTokens.length;

The only other place this gets read is during burn, where you could just use the _tokenId to infer location instead of this mapping.

Copy link
Contributor

@frangio frangio Mar 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a token is burnt, the one with the highest id is put in the place of the burnt one in the allTokens array.

https://github.com/spalladino/zeppelin-solidity/blob/6fbe771c276f83e734256908877c8f9aaa2e1bfc/contracts/token/ERC721/ERC721Token.sol#L189

In this way the index ⇔ id equivalence is invalidated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per the mint function, isn't the token's identifier the same thing as its index in the allTokens array?

Yes, but only because mint generates autoincremental token IDs. I didn't want to rely on this for implementing the enumeration, as anyone should be able to override this behaviour if needed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks guys for the clarification!


// Optional mapping for token URIs
mapping(uint256 => string) internal tokenURIs;

/**
* @dev Constructor function
*/
Expand Down Expand Up @@ -63,6 +66,27 @@ contract ERC721Token is ERC721, ERC721BasicToken {
return symbol_;
}

/**
* @dev Returns an URI for a given token ID
* @dev Throws if the token ID does not exist. May return an empty string.
* @param _tokenId uint256 ID of the token to query
*/
function tokenURI(uint256 _tokenId) public view returns (string) {
require(exists(_tokenId));
return tokenURIs[_tokenId];
}

/**
* @dev Internal function to set the token URI for a given token
* @dev Reverts if the token ID does not exist
* @param _tokenId uint256 ID of the token to set its URI
* @param _uri string URI to assign
*/
function doSetTokenURI(uint256 _tokenId, string _uri) internal {
require(exists(_tokenId));
tokenURIs[_tokenId] = _uri;
}

/**
* @dev Gets the token ID at a given index of the tokens list of the requested owner
* @param _owner address owning the tokens list to be accessed
Expand Down Expand Up @@ -152,6 +176,12 @@ contract ERC721Token is ERC721, ERC721BasicToken {
function doBurn(address _owner, uint256 _tokenId) internal {
super.doBurn(_owner, _tokenId);

// Clear metadata (if any)
if (bytes(tokenURIs[_tokenId]).length == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

== should be !=, but I'd even leave the check out. We can just delete, it might actually be cheaper.

I don't understand the cast to bytes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strings don't have a "length" property, which is why it is cast to bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleting without checking (if there was no value set) costs 10x more gas than checking beforehand. Tested with this contract in Remix:

pragma solidity "0.4.21";

contract Foo {
    mapping(uint256 => string) internal tokenURIs;
    
    function set(uint id, string value) public {
        tokenURIs[id] = value;
    }
    
    function deleteWithoutCheck(uint id) public {
        delete tokenURIs[id];
    }
    
    function deleteWithCheck(uint id) public {
        if (bytes(tokenURIs[id]).length != 0) {
            delete tokenURIs[id];
        }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And thanks for catching the != vs ==!

delete tokenURIs[_tokenId];
}

// Reorg all tokens array
uint256 tokenIndex = allTokensIndex[_tokenId];
uint256 lastTokenIndex = allTokens.length.sub(1);
uint256 lastToken = allTokens[lastTokenIndex];
Expand Down
24 changes: 21 additions & 3 deletions test/token/ERC721/ERC721Token.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ contract('ERC721Token', function (accounts) {
});

describe('metadata', function () {
const sampleUri = 'mock://mytoken';

it('has a name', async function () {
const name = await this.token.name();
name.should.be.equal(name);
Expand All @@ -87,10 +89,26 @@ contract('ERC721Token', function (accounts) {
symbol.should.be.equal(symbol);
});

it('returns metadata for a token id', async function () {
it('sets and returns metadata for a token id', async function () {
await this.token.setTokenURI(firstTokenId, sampleUri);
const uri = await this.token.tokenURI(firstTokenId);
uri.should.be.equal(sampleUri);
});

it('can burn token with metadata', async function () {
await this.token.setTokenURI(firstTokenId, sampleUri);
await this.token.burn(firstTokenId);
const exists = await this.token.exists(firstTokenId);
exists.should.be.false;
});

it('returns empty metadata for token', async function () {
const uri = await this.token.tokenURI(firstTokenId);
const expected = `mock://${firstTokenId.toString().padStart(78, 0)}`;
uri.should.be.equal(expected);
uri.should.be.equal('');
});

it('reverts when querying metadata for non existant token id', async function () {
await assertRevert(this.token.tokenURI(500));
});
});

Expand Down