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

KaiRo-at · 2019-12-19T16:20:50Z

As mentioned in #2014 (comment) this is a simple implementation of the metadata URI part of ERC1155.

I'm still a bit confused on how to do tests for OpenZeppelin (this is my first PR to the project), so I have not added tests in this PR for now. Help would be appreciated.

* Initial ERC1155 implementation with some tests * Remove mocked isERC1155TokenReceiver * Revert reason edit nit * Remove parameters associated with isERC1155TokenReceiver call * Add tests for approvals and single transfers * Add tests for transferring to contracts * Add tests for batch transfers * Make expectEvent.inTransaction tests async * Renamed "owner" to "account" and "holder" * Document unspecified balanceOfBatch reversion on zero behavior * Ensure accounts can't set their own operator status * Specify descriptive messages for underflow errors * Bring SafeMath.add calls in line with OZ style * Explicitly prevent _burn on the zero account * Implement batch minting/burning * Refactored operator approval check into isApprovedForAll calls * Renamed ERC1155TokenReceiver to ERC1155Receiver * Added ERC1155Holder * Fix lint issues

frangio · 2019-12-19T22:21:44Z

Thanks @KaiRo-at! You should add a new file in the test/token/ERC1155 directory for your new contract. It should be similar to ERC1155.test.js but testing only your specific behavior.

If you have any specific doubts please ask!

frangio · 2019-12-19T22:57:46Z

One more thing. We recently migrated our tests to OpenZeppelin Test Environment. The feature-erc1155 branch had forked before that, so it was behind with these changes. I have just updated it. Please update your branch and write your tests using the new tooling! The experience is almost identical anyway, but the performance is better.

Keep in mind Mocha's .only modifier to run only the tests that are relevant to this PR while you're writing them: describe.only('ERC1155Metadata....

stale · 2020-01-03T23:44:31Z

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

stale · 2020-01-19T16:29:58Z

Hi folks!
This Pull Request is being closed as there was no response to the previous prompt. However, please leave a comment whenever you're ready to resume, so it can be reopened.
Thanks again!

KaiRo-at · 2020-01-27T20:38:43Z

I just added a test to my repo, but I have issues running tests, I get an Error: Cannot find module 'openzeppelin-test-helpers' when running npm test, how can I fix that?
npm run lint runs fine but there are warnings in ERC1155.sol which are not my fault. ;-)

nventuro · 2020-01-27T20:52:43Z

Hello @KaiRo-at! From your description, it sounds like your import statement is wrong: you should import @openzeppelin/test-helpers, not openzeppelin-test-helpers. If that still doesn't work, please share a snippet or push your commits here so we can take a look.

And yes, feel free to ignore linting errors for now :)

KaiRo-at · 2020-01-28T14:23:46Z

From your description, it sounds like your import statement is wrong: you should import @openzeppelin/test-helpers, not openzeppelin-test-helpers.

I guess so - I copied from the other ERC1155 tests on that feature branch and it looks like it's wrong there as well... ;-)
And thanks for reopening this one!

nventuro · 2020-01-28T14:47:15Z

Looks like you're right! The feature branch was created before we migrated our testing setup to use Test Environment.

I updated the branch to bring the ERC1155 tests in line with the old ones, and then updated your own branch to use those. You should now be able to get your tests working!

Just remember to run npm install first, so that the new dependencies are installed.

KaiRo-at · 2020-01-28T15:49:04Z

Thanks for that, needed a bit of merging as I was working in parallel, but I should have tests up and running now. :)
The only detail is that I don't know how to get a receipt for the contract .new() to test for the event being emitted from the contructor as well (it's using the same function as being called and tested later though) - otherwise I think this should cover the whole functionality of the added contract/interface.
I'm slightly confused by circleci test failing as that test works fine locally though :(

nventuro · 2020-01-28T15:55:34Z

The only detail is that I don't know how to get a receipt for the contract .new() to test for the event being emitted from the contructor as well

Have you tried using expectEvent.inConstruction? You should be able to use it as follows:

const contract = await MyContract.new();
await expectEvent.inConstruction(contract, 'Foo', { value: 'bar' });

KaiRo-at · 2020-01-29T14:59:44Z

Have you tried using expectEvent.inConstruction?

Thanks, that's great! From what I see, everything should be complete for adding the URI feature now. :)

stale · 2020-02-13T15:41:25Z

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

nventuro · 2020-02-13T16:18:33Z

Sorry @KaiRo-at for taking so long to review this! I'll take a look tomorrow.

stale · 2020-02-28T16:32:23Z

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

stale · 2020-03-14T17:19:49Z

Hi folks!
This Pull Request is being closed as there was no response to the previous prompt. However, please leave a comment whenever you're ready to resume, so it can be reopened.
Thanks again!

KaiRo-at · 2020-03-14T19:02:58Z

Erm, @nventuro - I don't think this should have been closed...

nventuro · 2020-03-16T16:16:01Z

It shouldn't have, no - I'm reopening it. I'm very sorry @KaiRo-at, I've been working on the v3.0 pending changes and haven't managed to get around to reviewing 1155-related PRs yet - the same thing applies to #2107.

I'll take a look at this as soon as I can.

The base branch was changed.

nventuro

Sorry it took so long to get here @KaiRo-at, but the time for ERC1155 has finally come!

Given the work we've done on including optional extensions into the base implementation in ERC20 and ERC721, and the fact that URIs are an almost mandatory feature of NFTs, I was wondering if we could figure out a way to include this in the default ERC1155 contract, perhaps following a scheme similar to the baseURI from ERC721. What if we had a 'fallback' URI, allowing users to replace it with a token-id specific one if they desire? Concatenating the token id doesn't seem necessary, given the string substitution requirements of the spec.

contracts/token/ERC1155/IERC1155MetadataURI.sol

nventuro · 2020-05-08T17:13:47Z

contracts/token/ERC1155/ERC1155MetadataURICatchAll.sol

+     */
+    function _setURI(string memory newuri) internal virtual {
+        _uri = newuri;
+        emit URI(_uri, 0);


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.

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.

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.

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.

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. ;-)

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.

contracts/token/ERC1155/ERC1155MetadataURICatchAll.sol

Starting on Solidity v0.6.2, interfaces can now inherit. \o/ Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>

KaiRo-at · 2020-05-11T16:09:25Z

Sorry it took so long to get here @KaiRo-at, but the time for ERC1155 has finally come!

Given the work we've done on including optional extensions into the base implementation in ERC20 and ERC721, and the fact that URIs are an almost mandatory feature of NFTs, I was wondering if we could figure out a way to include this in the default ERC1155 contract, perhaps following a scheme similar to the baseURI from ERC721. What if we had a 'fallback' URI, allowing users to replace it with a token-id specific one if they desire? Concatenating the token id doesn't seem necessary, given the string substitution requirements of the spec.

We could do that. That said, given that the ERC1155 spec contains this {id} thing directly in the standard, I'd guess it's preferred to use that there (though the URIs don't look as nice for sure and the web service behind it needs to do a bit more work to parse it into a normal number). Also, the question is how we'd differentiate between the baseURI scheme and the scheme with {id}.

nventuro · 2020-05-12T17:08:22Z

Also, the question is how we'd differentiate between the baseURI scheme and the scheme with {id}.

Sorry, I should've been clearer. I wasn't suggesting using baseURI directly, but rather the idea from that PR where there is a 'default' URI that can be overridden by setting a specific one for the token. Given string substitution in 1155, there seems to be no need for the baseURI concatenation we have in 721, but we might want to implement this as follows:

function _setDefaultURI();
function _setURI(id);

function uri(id) view {
  if uris[id]
    return uris[id];
  else
    return defaultURI;
}

KaiRo-at · 2020-05-20T18:45:14Z

Sorry I have been silent on this and the other issue for a bit, but we just shipped (and sold out) our Crypto stamp 2 presale today using the code from here and the other ERC1155 contributions: https://etherscan.io/address/presale.cs2.cryptostamp.eth#code
I hope I'll get back to looking into this in the next days now that the immediate release stress is gone.

nventuro · 2020-05-20T20:11:02Z

No worries, and congratulations on your release! 🎉

KaiRo-at · 2020-05-29T12:39:10Z

function uri(id) view {
  if uris[id]
    return uris[id];
  else
    return defaultURI;
}

I definitely can do that. I concentrated on the single catch-all solution for now due to #2014 (comment) and I was planning to look into extending that for per-token URIs once ERC1155 was on master if my time allowed for it. But I can look into the right here as well.

nventuro · 2020-05-29T20:19:50Z

Good point, thanks for reminding me of that. I've discussed this with @frangio and came to the conclusion that, given there's a clear pattern on how to use URIs with string substituion, we can do without _setDefaultURI() and can just have a generic _setUri().

nventuro · 2020-06-02T21:24:06Z

@KaiRo-at we're aiming to release v3.1 soon and to include initial support for ERC1155 there, so if you don't mind I'll go ahead and make the final changes required for this PR myself :)

nventuro · 2020-06-02T21:56:33Z

I added the optional extension to ERC1155, inline with how we've implemented other token standards. This let me simplify the tests and mocks quite a bit.

I additionally changed some abstract contracts into interfaces, using the new interface inheritance mechanism, which resulted in some superficial changes in some files.

KaiRo-at · 2020-06-03T10:54:20Z

@KaiRo-at we're aiming to release v3.1 soon and to include initial support for ERC1155 there, so if you don't mind I'll go ahead and make the final changes required for this PR myself :)

Sure, I don't feel territorial about this, happy I could push it along some way. Sorry I'm pretty short on time recently (due to a lot of ongoing work for Crypto stamp 2), I'm happy this work here gets picked up and driven into a release!

nventuro · 2020-06-03T14:56:51Z

Reviewed offline by @frangio.

KaiRo-at mentioned this pull request Dec 19, 2019

ERC1155 feature pending tasks #2014

Merged

6 tasks

stale bot added the stale label Jan 3, 2020

stale bot closed this Jan 19, 2020

nventuro reopened this Jan 27, 2020

stale bot removed the stale label Jan 27, 2020

nventuro added 2 commits January 28, 2020 11:32

Merge branch 'master' into feature-erc1155

f57f16e

Migrate tests to @openzeppelin/test-environment

727512e

stale bot added the stale label Feb 13, 2020

stale bot removed the stale label Feb 13, 2020

KaiRo-at mentioned this pull request Feb 14, 2020

"{" in base contract constructor parameter string confuses instumentation sc-forks/solidity-coverage#478

Closed

stale bot added the stale label Feb 28, 2020

stale bot closed this Mar 14, 2020

Merge branch 'master' into feature-erc1155

fa4e185

nventuro reviewed May 8, 2020

View reviewed changes

Update contracts/token/ERC1155/IERC1155MetadataURI.sol

77db2a1

Starting on Solidity v0.6.2, interfaces can now inherit. \o/ Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>

nventuro added 4 commits June 2, 2020 16:32

Merge branch 'master' into kairo-uri

4457b39

Fix compile errors

23cb821

Merge branch 'master' into kairo-uri

bc8887b

Remove URI event

473d0ce

nventuro added 6 commits June 2, 2020 18:30

Merge MetadataCatchAll into ERC1155

d55b138

Improve documentation.

4a5108c

Simplify tests

56b50f3

Move tests into ERC1155 tests

22c4cd3

Update documentation

fd50e27

Bump minimum compiler version for inteface inheritance

6653018

nventuro requested a review from frangio June 2, 2020 21:56

Fix holder tests

fa92e6a

nventuro added 2 commits June 3, 2020 11:47

Improve setUri docs

4763213

Fix docs generation

d52171a

nventuro merged commit a81e948 into OpenZeppelin:master Jun 3, 2020

abcoathup changed the title ~~Add a simple catch-all implementation of the metadata URI interface~~ ERC1155: Add a simple catch-all implementation of the metadata URI interface Jun 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

KaiRo-at commented Dec 19, 2019

frangio commented Dec 19, 2019

frangio commented Dec 19, 2019 •

edited

Loading

stale bot commented Jan 3, 2020

stale bot commented Jan 19, 2020

KaiRo-at commented Jan 27, 2020 •

edited

Loading

nventuro commented Jan 27, 2020

KaiRo-at commented Jan 28, 2020

nventuro commented Jan 28, 2020

KaiRo-at commented Jan 28, 2020 •

edited

Loading

nventuro commented Jan 28, 2020

KaiRo-at commented Jan 29, 2020

stale bot commented Feb 13, 2020

nventuro commented Feb 13, 2020

stale bot commented Feb 28, 2020

stale bot commented Mar 14, 2020

KaiRo-at commented Mar 14, 2020

nventuro commented Mar 16, 2020

nventuro left a comment

nventuro May 8, 2020

KaiRo-at May 11, 2020

nventuro May 12, 2020

nventuro May 12, 2020

KaiRo-at May 29, 2020

nventuro May 29, 2020

KaiRo-at commented May 11, 2020

nventuro commented May 12, 2020

KaiRo-at commented May 20, 2020

nventuro commented May 20, 2020

KaiRo-at commented May 29, 2020

nventuro commented May 29, 2020

nventuro commented Jun 2, 2020

nventuro commented Jun 2, 2020

KaiRo-at commented Jun 3, 2020

nventuro commented Jun 3, 2020

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

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

Conversation

KaiRo-at commented Dec 19, 2019

frangio commented Dec 19, 2019

frangio commented Dec 19, 2019 • edited Loading

stale bot commented Jan 3, 2020

stale bot commented Jan 19, 2020

KaiRo-at commented Jan 27, 2020 • edited Loading

nventuro commented Jan 27, 2020

KaiRo-at commented Jan 28, 2020

nventuro commented Jan 28, 2020

KaiRo-at commented Jan 28, 2020 • edited Loading

nventuro commented Jan 28, 2020

KaiRo-at commented Jan 29, 2020

stale bot commented Feb 13, 2020

nventuro commented Feb 13, 2020

stale bot commented Feb 28, 2020

stale bot commented Mar 14, 2020

KaiRo-at commented Mar 14, 2020

nventuro commented Mar 16, 2020

nventuro left a comment

Choose a reason for hiding this comment

nventuro May 8, 2020

Choose a reason for hiding this comment

KaiRo-at May 11, 2020

Choose a reason for hiding this comment

nventuro May 12, 2020

Choose a reason for hiding this comment

nventuro May 12, 2020

Choose a reason for hiding this comment

KaiRo-at May 29, 2020

Choose a reason for hiding this comment

nventuro May 29, 2020

Choose a reason for hiding this comment

KaiRo-at commented May 11, 2020

nventuro commented May 12, 2020

KaiRo-at commented May 20, 2020

nventuro commented May 20, 2020

KaiRo-at commented May 29, 2020

nventuro commented May 29, 2020

nventuro commented Jun 2, 2020

nventuro commented Jun 2, 2020

KaiRo-at commented Jun 3, 2020

nventuro commented Jun 3, 2020

frangio commented Dec 19, 2019 •

edited

Loading

KaiRo-at commented Jan 27, 2020 •

edited

Loading

KaiRo-at commented Jan 28, 2020 •

edited

Loading