Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ERC1155: Add a simple catch-all implementation of the metadata URI interface #2029
Changes from 17 commits
20642cc
f57f16e
727512e
c3f4ae3
a7493e9
067d278
267700f
7910e8c
e574206
6acec05
132728c
85283ab
7e0f247
82c7bd7
5fd36e7
7c2052f
fa4e185
77db2a1
4457b39
23cb821
bc8887b
473d0ce
d55b138
4a5108c
56b50f3
22c4cd3
fd50e27
6653018
fa92e6a
4763213
d52171a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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 anid
value of 0 is valid, perhaps we shouldn't emit the event at all? The spec seems to allow this:There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. ;-)
There was a problem hiding this comment.
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: