Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ERC1155: Add a simple catch-all implementation of the metadata URI interface #2029

Merged
merged 31 commits into from
Jun 3, 2020
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
20642cc
Initial ERC1155 implementation with some tests (#1803)
cag Nov 3, 2019
f57f16e
Merge branch 'master' into feature-erc1155
nventuro Jan 28, 2020
727512e
Migrate tests to @openzeppelin/test-environment
nventuro Jan 28, 2020
c3f4ae3
Merge branch 'master' into feature-erc1155
nventuro Mar 30, 2020
a7493e9
port ERC1155 to Solidity 0.6
KaiRo-at Mar 16, 2020
067d278
make ERC1155 constructor more similar to ERC721 one
KaiRo-at Mar 16, 2020
267700f
also migrate mock contracts to Solidity 0.6
KaiRo-at Mar 16, 2020
7910e8c
mark all non-view functions as virtual
KaiRo-at Apr 10, 2020
e574206
add simple catch-all implementation for the metadata URI interface
KaiRo-at Dec 13, 2019
6acec05
include an internal function to set the URI so users can implement fu…
KaiRo-at Dec 19, 2019
132728c
add tests for ERC1155 metadata URI
KaiRo-at Jan 27, 2020
85283ab
fix nits, mostly pointed out by linter
KaiRo-at Jan 29, 2020
7e0f247
convert ERC1155 metadata URI work to Solidity 0.6
KaiRo-at Apr 1, 2020
82c7bd7
mark all non-view functions as virtual
KaiRo-at Apr 10, 2020
5fd36e7
Port ERC 1155 branch to Solidity 0.6 (and current master) (#2130)
KaiRo-at Apr 27, 2020
7c2052f
Merge remote-tracking branch 'upstream/feature-erc1155' into feature-…
frangio Apr 27, 2020
fa4e185
Merge branch 'master' into feature-erc1155
nventuro May 8, 2020
77db2a1
Update contracts/token/ERC1155/IERC1155MetadataURI.sol
KaiRo-at May 11, 2020
4457b39
Merge branch 'master' into kairo-uri
nventuro Jun 2, 2020
23cb821
Fix compile errors
nventuro Jun 2, 2020
bc8887b
Merge branch 'master' into kairo-uri
nventuro Jun 2, 2020
473d0ce
Remove URI event
nventuro Jun 2, 2020
d55b138
Merge MetadataCatchAll into ERC1155
nventuro Jun 2, 2020
4a5108c
Improve documentation.
nventuro Jun 2, 2020
56b50f3
Simplify tests
nventuro Jun 2, 2020
22c4cd3
Move tests into ERC1155 tests
nventuro Jun 2, 2020
fd50e27
Update documentation
nventuro Jun 2, 2020
6653018
Bump minimum compiler version for inteface inheritance
nventuro Jun 2, 2020
fa92e6a
Fix holder tests
nventuro Jun 2, 2020
4763213
Improve setUri docs
nventuro Jun 3, 2020
d52171a
Fix docs generation
nventuro Jun 3, 2020
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
22 changes: 22 additions & 0 deletions contracts/mocks/ERC1155MetadataURICatchAllMock.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
pragma solidity ^0.6.0;

import "../token/ERC1155/ERC1155MetadataURICatchAll.sol";

/**
* @title ERC1155MetadataURICatchAllMock
* This mock just publicizes internal functions for testing purposes
* (mint is not specific to the tested contract but required by shouldBehaveLikeERC1155)
*/
contract ERC1155MetadataURICatchAllMock is ERC1155MetadataURICatchAll {
constructor (string memory uri) public ERC1155MetadataURICatchAll(uri) {
// solhint-disable-previous-line no-empty-blocks
}

function mint(address to, uint256 id, uint256 value, bytes memory data) public {
_mint(to, id, value, data);
}

function setURI(string memory newuri) public {
_setURI(newuri);
}
}
46 changes: 46 additions & 0 deletions contracts/token/ERC1155/ERC1155MetadataURICatchAll.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
pragma solidity ^0.6.0;

import "./ERC1155.sol";
import "./IERC1155MetadataURI.sol";
import "../../introspection/ERC165.sol";

contract ERC1155MetadataURICatchAll is ERC165, ERC1155, IERC1155MetadataURI {
// Catch-all URI with placeholders, e.g. https://example.com/{locale}/{id}.json
string private _uri;

/*
* bytes4(keccak256('uri(uint256)')) == 0x0e89341c
*/
bytes4 private constant _INTERFACE_ID_ERC1155_METADATA_URI = 0x0e89341c;

/**
* @dev Constructor function
*/
constructor (string memory uri) public {
_setURI(uri);

// register the supported interfaces to conform to ERC1155 via ERC165
_registerInterface(_INTERFACE_ID_ERC1155_METADATA_URI);
}

/**
* @notice A distinct Uniform Resource Identifier (URI) for a given token.
* @dev URIs are defined in RFC 3986.
* The URI MUST point to a JSON file that conforms to the "ERC-1155 Metadata URI JSON Schema".
* param id uint256 ID of the token to query (ignored in this particular implementation,
* as an {id} parameter in the string is expected)
* @return URI string
*/
function uri(uint256 /*id*/) external view override returns (string memory) {
return _uri;
}

/**
* @dev Internal function to set a new URI
* @param newuri New URI to be set
*/
function _setURI(string memory newuri) internal virtual {
_uri = newuri;
nventuro marked this conversation as resolved.
Show resolved Hide resolved
emit URI(_uri, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems sensible, but I couldn't find anything in the specification about the event's id argument in cases such as this. Given that an id value of 0 is valid, perhaps we shouldn't emit the event at all? The spec seems to allow this:

Changes to the URI MUST emit the URI event if the change can be expressed with an event (i.e. it isn’t dynamic/programmatic).

An implementation MAY emit the URI event during a mint operation but it is NOT mandatory. An observer MAY fetch the metadata uri at mint time from the uri function if it was not emitted.

The uri function SHOULD be used to retrieve values if no event was emitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, you mean "it isn't dynamic" says that the {id} pattern doesn't need it? I interpreted it as it can only be omitted if e.g. the URI is built by a function that depends on, say, other fields and the actually returned URI changes dynamically - but in our case, it can be expressed by an event, and the actually returned URI doesn't change dynamically, it changes only with that call. I have wondered about the ID in that event as well though, unfortunately, the spec doesn't define this case. Maybe we should ask there.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I agree with your interpretation and believe this case might be missing from the spec. Emitting an event feels wrong though because there is no invalid values for the id field we can use to signal what is going on. Asking the spec authors sounds like the way to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

At any rate, given the spec allows for no events to be emitted (like in the case of dynamic URIs), I think it makes sense to not emit anything here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At any rate, given the spec allows for no events to be emitted (like in the case of dynamic URIs), I think it makes sense to not emit anything here.

We definitely interpret the spec differently there, as I read it as non-optional, with the only exception being where it is not static and not possible to emit the event - and it is static in our case and very much possible to emit it. But if you as the reviewer(s) prefer that, I'll remove it even if I disagree. ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

We've also discussed this bit with @frangio and agree that we should not emit any events. The spec clearly allows for situations where no events have been emitted, as seen in these statements:

The uri function SHOULD be used to retrieve values if no event was emitted.
The uri function MUST return the same value as the latest event for an _id if it was emitted.

}
}
11 changes: 11 additions & 0 deletions contracts/token/ERC1155/IERC1155MetadataURI.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
pragma solidity ^0.6.2;

import "./IERC1155.sol";

/**
* @title ERC-1155 Multi Token Standard basic interface, optional metadata URI extension
* @dev See https://eips.ethereum.org/EIPS/eip-1155
*/
interface IERC1155MetadataURI is IERC1155 {
function uri(uint256 id) external view virtual returns (string memory);
}
8 changes: 7 additions & 1 deletion contracts/token/ERC1155/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,15 @@ sections:
contracts:
- IERC1155
- ERC1155
- IERC1155MetadataURI
- ERC1155MetadataURICatchAll
- IERC1155Receiver
- ERC1155Holder
---

This set of interfaces and contracts are all related to the [ERC1155 Multi Token Standard](https://eips.ethereum.org/EIPS/eip-1155).

The EIP consists of two interfaces which fulfill different roles, found here as `IERC1155` and `IERC1155Receiver`. Only `IERC1155` is required for a contract to be ERC1155 compliant. The basic functionality is implemented in `ERC1155`.
The EIP consists of three interfaces which fulfill different roles, found here as `IERC1155`, `IERC1155MetadataURI` and `IERC1155Receiver`. Only `IERC1155` is required for a contract to be ERC1155 compliant. The basic functionality is implemented in `ERC1155`.

Additionally, `ERC1155MetadataURICatchAll` implements functionality for setting and querying a simple catch-all URI (i.e. one URI for all tokens, with the ID as a parameter) for metadata via the `IERC1155MetadataURI` interface.
`ERC1155Holder` implements the `IERC1155Receiver` interface for contracts that can receive (and hold) ERC1155 tokens.
60 changes: 60 additions & 0 deletions test/token/ERC1155/ERC1155MetadataURICatchAll.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
const { accounts, contract } = require('@openzeppelin/test-environment');
const { BN, expectEvent } = require('@openzeppelin/test-helpers');

const { expect } = require('chai');

const { shouldBehaveLikeERC1155 } = require('./ERC1155.behavior');
const ERC1155MetadataURICatchAllMock = contract.fromArtifact('ERC1155MetadataURICatchAllMock');

describe('ERC1155MetadataURICatchAll', function () {
const [creator, ...otherAccounts] = accounts;

const uriInit = 'https://example.com/{id}.json';
const tokenId = new BN(0); // catch-all always uses id 0 in event

beforeEach(async function () {
this.token = await ERC1155MetadataURICatchAllMock.new(uriInit, { from: creator });
});

it('emits a URI event in constructor', async function () {
await expectEvent.inConstruction(this.token, 'URI', {
value: uriInit,
id: tokenId,
});
});

shouldBehaveLikeERC1155(otherAccounts);

it('has a uri', async function () {
expect(await this.token.uri(
tokenId
)).to.be.equal(uriInit);
});

describe('internal functions', function () {
const uriNew = 'https://example.com/{locale}/{id}.json';

describe('_setURI(string memory newuri)', function () {
let receipt;
beforeEach(async function () {
receipt = await this.token.setURI(
uriNew,
{ from: creator }
);
});

it('emits a URI event', function () {
expectEvent(receipt, 'URI', {
value: uriNew,
id: tokenId,
});
});

it('has correct URI set', async function () {
expect(await this.token.uri(
tokenId
)).to.be.equal(uriNew);
});
});
});
});