-
Notifications
You must be signed in to change notification settings - Fork 451
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
ERC-1155 Example #800
ERC-1155 Example #800
Conversation
I'm not sure if the output format is correct, need to read the docs more closely
It wasn't ERC-1155 compliant (no transfer events emitted) and it also leaked the internal structure of how balances were tracked.
Not sure I like some of the decisions though...
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.
The most important thing to deal with in this PR is how we do error handling. Currently the implementation panics a lot which generally is not ideal. Instead Result
should be returned with a predefined set of potential errors (given an enum
or so) so that depending contracts can handle those errors if they want to.
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.
So I think the disconnect with the Err
vs. panic
thing is that the spec explicitly says things like: "MUST revert if _to
is the zero address.", which in my mind translates directly to panic
and don't let the caller handle it.
However, after some thinking I agree, for most of these scenarios it's probably OK to let the caller decide how to proceed, and it also makes the code more idiomatic.
Co-authored-by: Robin Freyler <[email protected]>
Co-authored-by: Robin Freyler <[email protected]>
This way the spellchecker will ignore it and we can avoid adding it to our dictionary.
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.
LGTM
Follow-up for use-ink/ink#800.
This is getting big, so I think it's a good time to get some feedback and also get
some of my questions answered.
This PR aims to inkplement ERC-1155 as specified here.
It implements the "core" APIs, the ERC1155 Receivable interface (even though the just
implementation panics and rejects tokens, lol), and also has some additional
functionality such as a way to easily mint tokens.
Note that this contract implementation is missing some extensions which are mentioned in
the ERC-1155 specification document:
I won't be adding these here since a) this PR is already pretty big and b) they could
make for good first contributor issues.
I also think there are some improvements to be made in terms of how "Rusty" some
things are (having events return
None
vs.0x00
), but I don't want to deviate toofar from the ERC-1155 spec.