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
Initial ERC1155 implementation with some tests #1803
Initial ERC1155 implementation with some tests #1803
Changes from all commits
16bcf7f
a41d23b
2d757b3
2621a8b
b9023d7
ce57e2d
4e6845b
05bb0c6
75cc9e9
85a50b7
d8d848f
eb52901
660090e
ae3725b
77b9627
dc86dba
7f7d9eb
b7125f2
077f8e9
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.
Some of the contracts are missing, such as the one for the metadata and the one for the URI functions. Are those going to be implemented in another stage?
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.
I did not seek to implement those contracts, since we don't know how these features really should work because we don't use them, and didn't plan on using them. These should be added by somebody with more knowledge/use for those features.
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.
There isn't a method to broadcast a new token
id
creation. The EIP states (Minting/creating and burning/destroying rules:): "To broadcast the existence of a token ID with no initial balance, the contract SHOULD emit theTransferSingle
event from0x0
to0x0
, with the token creator as_operator
, and a_value
of 0" but the transfer and_mint
functions don't allow to send tokens to the zero address, therefore it can not be triggered that event with those parameters.Consider adding a new function for creating a new type of token. Take into account that if it is used a
_broadcast
function to let the rest know that a newid
has been created, then the function should check into the storage - either atotalSupply
or aidExists
mapping - to see if thatid
was already created. Otherwise, the creator could trigger severalTransferSingle
events with the 'new-id
-created parameters'.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.
If possible, I'd like to punt on this, because this will require additional storage in the contract, and if implemented, probably should prevent the use of counterfactual IDs (as you've mentioned below via reverting transactions on unminted IDs).
Since we (Gnosis) actually need to use counterfactual IDs in our implementation, we have not implemented this.
I can see how this may be useful for "game item" scenarios though: something that this standard was originally designed for, but because we don't want to exclude our own use case, I think this functionality would fit better in an derived contract.