From 49042f2b1ae76eb9befa12000b98211981a139ec Mon Sep 17 00:00:00 2001 From: Ignacio Mazzara Date: Fri, 15 Nov 2019 20:22:37 -0300 Subject: [PATCH] feat: add baseTokenURI to ERC721Metadata (#1970) * feat: add baseTokenURI * fix: tests * chore: dev notation * chore: changelog * chore: typo * Remove extra getters, return empty URI by default * Update docs * Rename baseTokenURI to baseURI * Roll back visibility change of tokenURI * Update changelog entry * Version setBaseURI docs * Improve internal names and comments * Fix compilation errors * Add an external getter for baseURI --- CHANGELOG.md | 1 + contracts/mocks/ERC721FullMock.sol | 4 ++ contracts/token/ERC721/ERC721Metadata.sol | 53 ++++++++++++--- test/token/ERC721/ERC721Full.test.js | 81 ++++++++++++++++------- 4 files changed, 108 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 33da09d441d..5b6f67671e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### New features * `SafeCast.toUintXX`: new library for integer downcasting, which allows for safe operation on smaller types (e.g. `uint32`) when combined with `SafeMath`. ([#1926](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1926)) + * `ERC721Metadata`: added `baseURI`, which can be used for dramatic gas savings when all token URIs share a prefix (e.g. `http://api.myapp.com/tokens/`). ([#1970](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1970)) ### Improvements * `ERC777`: `_burn` is now internal, providing more flexibility and making it easier to create tokens that deflate. ([#1908](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1908)) diff --git a/contracts/mocks/ERC721FullMock.sol b/contracts/mocks/ERC721FullMock.sol index b0f00918bdf..bbd1e32bc33 100644 --- a/contracts/mocks/ERC721FullMock.sol +++ b/contracts/mocks/ERC721FullMock.sol @@ -26,4 +26,8 @@ contract ERC721FullMock is ERC721Full, ERC721Mintable, ERC721MetadataMintable, E function setTokenURI(uint256 tokenId, string memory uri) public { _setTokenURI(tokenId, uri); } + + function setBaseURI(string memory baseURI) public { + _setBaseURI(baseURI); + } } diff --git a/contracts/token/ERC721/ERC721Metadata.sol b/contracts/token/ERC721/ERC721Metadata.sol index 26f4706bd91..e0e516065db 100644 --- a/contracts/token/ERC721/ERC721Metadata.sol +++ b/contracts/token/ERC721/ERC721Metadata.sol @@ -12,6 +12,9 @@ contract ERC721Metadata is Context, ERC165, ERC721, IERC721Metadata { // Token symbol string private _symbol; + // Base URI + string private _baseURI; + // Optional mapping for token URIs mapping(uint256 => string) private _tokenURIs; @@ -52,24 +55,58 @@ contract ERC721Metadata is Context, ERC165, ERC721, IERC721Metadata { } /** - * @dev Returns an URI for a given token ID. - * Throws if the token ID does not exist. May return an empty string. - * @param tokenId uint256 ID of the token to query + * @dev Returns the URI for a given token ID. May return an empty string. + * + * If the token's URI is non-empty and a base URI was set (via + * {_setBaseURI}), it will be added to the token ID's URI as a prefix. + * + * Reverts if the token ID does not exist. */ function tokenURI(uint256 tokenId) external view returns (string memory) { require(_exists(tokenId), "ERC721Metadata: URI query for nonexistent token"); - return _tokenURIs[tokenId]; + + string memory _tokenURI = _tokenURIs[tokenId]; + + // Even if there is a base URI, it is only appended to non-empty token-specific URIs + if (bytes(_tokenURI).length == 0) { + return ""; + } else { + // abi.encodePacked is being used to concatenate strings + return string(abi.encodePacked(_baseURI, _tokenURI)); + } + } + + /** + * @dev Returns the base URI set via {_setBaseURI}. This will be + * automatically added as a preffix in {tokenURI} to each token's URI, when + * they are non-empty. + */ + function baseURI() external view returns (string memory) { + return _baseURI; } /** * @dev Internal function to set the token URI for a given token. + * * 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 + * + * TIP: if all token IDs share a prefix (e.g. if your URIs look like + * `http://api.myproject.com/token/`), use {_setBaseURI} to store + * it and save gas. */ - function _setTokenURI(uint256 tokenId, string memory uri) internal { + function _setTokenURI(uint256 tokenId, string memory _tokenURI) internal { require(_exists(tokenId), "ERC721Metadata: URI set of nonexistent token"); - _tokenURIs[tokenId] = uri; + _tokenURIs[tokenId] = _tokenURI; + } + + /** + * @dev Internal function to set the base URI for all token IDs. It is + * automatically added as a prefix to the value returned in {tokenURI}. + * + * _Available since v2.5.0._ + */ + function _setBaseURI(string memory baseURI) internal { + _baseURI = baseURI; } /** diff --git a/test/token/ERC721/ERC721Full.test.js b/test/token/ERC721/ERC721Full.test.js index dbbf9af6a83..878488f6dd6 100644 --- a/test/token/ERC721/ERC721Full.test.js +++ b/test/token/ERC721/ERC721Full.test.js @@ -72,8 +72,6 @@ contract('ERC721Full', function ([ }); describe('metadata', function () { - const sampleUri = 'mock://mytoken'; - it('has a name', async function () { expect(await this.token.name()).to.be.equal(name); }); @@ -82,31 +80,68 @@ contract('ERC721Full', function ([ expect(await this.token.symbol()).to.be.equal(symbol); }); - it('sets and returns metadata for a token id', async function () { - await this.token.setTokenURI(firstTokenId, sampleUri); - expect(await this.token.tokenURI(firstTokenId)).to.be.equal(sampleUri); - }); + describe('token URI', function () { + const baseURI = 'https://api.com/v1/'; + const sampleUri = 'mock://mytoken'; - it('reverts when setting metadata for non existent token id', async function () { - await expectRevert( - this.token.setTokenURI(nonExistentTokenId, sampleUri), 'ERC721Metadata: URI set of nonexistent token' - ); - }); + it('it is empty by default', async function () { + expect(await this.token.tokenURI(firstTokenId)).to.be.equal(''); + }); - it('can burn token with metadata', async function () { - await this.token.setTokenURI(firstTokenId, sampleUri); - await this.token.burn(firstTokenId, { from: owner }); - expect(await this.token.exists(firstTokenId)).to.equal(false); - }); + it('reverts when queried for non existent token id', async function () { + await expectRevert( + this.token.tokenURI(nonExistentTokenId), 'ERC721Metadata: URI query for nonexistent token' + ); + }); - it('returns empty metadata for token', async function () { - expect(await this.token.tokenURI(firstTokenId)).to.be.equal(''); - }); + it('can be set for a token id', async function () { + await this.token.setTokenURI(firstTokenId, sampleUri); + expect(await this.token.tokenURI(firstTokenId)).to.be.equal(sampleUri); + }); - it('reverts when querying metadata for non existent token id', async function () { - await expectRevert( - this.token.tokenURI(nonExistentTokenId), 'ERC721Metadata: URI query for nonexistent token' - ); + it('reverts when setting for non existent token id', async function () { + await expectRevert( + this.token.setTokenURI(nonExistentTokenId, sampleUri), 'ERC721Metadata: URI set of nonexistent token' + ); + }); + + it('base URI can be set', async function () { + await this.token.setBaseURI(baseURI); + expect(await this.token.baseURI()).to.equal(baseURI); + }); + + it('base URI is added as a prefix to the token URI', async function () { + await this.token.setBaseURI(baseURI); + await this.token.setTokenURI(firstTokenId, sampleUri); + + expect(await this.token.tokenURI(firstTokenId)).to.be.equal(baseURI + sampleUri); + }); + + it('token URI can be changed by changing the base URI', async function () { + await this.token.setBaseURI(baseURI); + await this.token.setTokenURI(firstTokenId, sampleUri); + + const newBaseURI = 'https://api.com/v2/'; + await this.token.setBaseURI(newBaseURI); + expect(await this.token.tokenURI(firstTokenId)).to.be.equal(newBaseURI + sampleUri); + }); + + it('token URI is empty for tokens with no URI but with base URI', async function () { + await this.token.setBaseURI(baseURI); + + expect(await this.token.tokenURI(firstTokenId)).to.be.equal(''); + }); + + it('tokens with URI can be burnt ', async function () { + await this.token.setTokenURI(firstTokenId, sampleUri); + + await this.token.burn(firstTokenId, { from: owner }); + + expect(await this.token.exists(firstTokenId)).to.equal(false); + await expectRevert( + this.token.tokenURI(firstTokenId), 'ERC721Metadata: URI query for nonexistent token' + ); + }); }); });