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

ERC-1155 Example #800

Merged
merged 78 commits into from
Jul 8, 2021
Merged

ERC-1155 Example #800

merged 78 commits into from
Jul 8, 2021

Conversation

HCastano
Copy link
Contributor

@HCastano HCastano commented Jun 1, 2021

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:

  • Missing ERC1155 Metadata URI Interface
  • No differentiation between fungible and non-fungible tokens
  • Tokens are not officially burnable
  • No batch minting

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 too
far from the ERC-1155 spec.

@HCastano HCastano added the A-examples [examples] Work item label Jun 1, 2021
examples/erc1155/lib.rs Outdated Show resolved Hide resolved
@HCastano HCastano requested review from Robbepop and cmichi June 21, 2021 14:59
Copy link
Collaborator

@Robbepop Robbepop left a 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.

examples/erc1155/lib.rs Outdated Show resolved Hide resolved
examples/erc1155/lib.rs Outdated Show resolved Hide resolved
examples/erc1155/lib.rs Show resolved Hide resolved
examples/erc1155/lib.rs Outdated Show resolved Hide resolved
examples/erc1155/lib.rs Outdated Show resolved Hide resolved
examples/erc1155/lib.rs Show resolved Hide resolved
examples/erc1155/lib.rs Show resolved Hide resolved
examples/erc1155/lib.rs Outdated Show resolved Hide resolved
examples/erc1155/lib.rs Outdated Show resolved Hide resolved
examples/erc1155/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@HCastano HCastano left a 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.

examples/erc1155/lib.rs Show resolved Hide resolved
examples/erc1155/lib.rs Outdated Show resolved Hide resolved
examples/erc1155/lib.rs Outdated Show resolved Hide resolved
examples/erc1155/lib.rs Outdated Show resolved Hide resolved
examples/erc1155/lib.rs Outdated Show resolved Hide resolved
examples/erc1155/lib.rs Outdated Show resolved Hide resolved
examples/erc1155/lib.rs Outdated Show resolved Hide resolved
@HCastano HCastano requested a review from Robbepop June 30, 2021 23:20
This way the spellchecker will ignore it and we
can avoid adding it to our dictionary.
Copy link
Collaborator

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

LGTM

@Robbepop Robbepop merged commit afc4871 into master Jul 8, 2021
@Robbepop Robbepop deleted the hc-erc-1155-example branch July 8, 2021 10:49
cmichi added a commit to paritytech/ink-waterfall that referenced this pull request Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-examples [examples] Work item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants