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

Feature/erc777 #1159 #1684

Merged
merged 68 commits into from
Apr 16, 2019
Merged

Conversation

catageek
Copy link
Contributor

Fixes #1159

This PR adds a ERC777 compatible token contract in the drafts folder.

A few points:

  • The interfaces IERC777TokenSender.sol and IERC777TokenReceiver.sol are under MPL license since it's the license used by the reference implementation. I didn't take the responsibility to remove the license. For IERC777.sol, the license is not obvious also.
  • This implementation uses a local version of test/helpers/ExpectEvent.js because the implementation provided by the openzeppelin-test-helpers package does not work for events triggered during contract creation. A PR to the repository of the helpers package is expected, and this local version will be discarded when it's merged.
  • test/introspection/ERC1820Deploy.js is the script that deploys an ERC1820 registry before executing the tests.
  • The test file is a big monolithic file that should be dispatched in several files. I am not a Javascript developper, so forgive me to release it "as is".
  • The security of the code has not been reviewed.

@nventuro nventuro added this to the v2.3 milestone Mar 18, 2019
@catageek
Copy link
Contributor Author

OK, I drop trying to make the unit tests pass. For an unknown reason, I can't deploy an ERC1820 registry contract because it has already been deployed (nonce = 1), but I can't access the existing one and all the tests fail.

@nventuro nventuro self-assigned this Mar 25, 2019
@nventuro nventuro added improvement contracts Smart contract code. labels Mar 25, 2019
@nventuro
Copy link
Contributor

Hello @catageek! Sorry for taking so long to review this, I was out of town for a couple days.

One small comment before I review the code proper:

The interfaces IERC777TokenSender.sol and IERC777TokenReceiver.sol are under MPL license since it's the license used by the reference implementation. I didn't take the responsibility to remove the license. For IERC777.sol, the license is not obvious also.

From my understanding of the EIP copyright, all rights are waved via CC0. This includes the contract interfaces, so we should be free to use them and re-license them under MIT (OpenZeppelin's license). IANAL though.

contracts/drafts/ERC777/IERC777TokensRecipient.sol Outdated Show resolved Hide resolved
contracts/drafts/ERC777/ERC777Base.sol Outdated Show resolved Hide resolved
contracts/drafts/ERC777/ERC777.sol Outdated Show resolved Hide resolved
contracts/drafts/ERC777/ERC777Base.sol Outdated Show resolved Hide resolved
@nventuro
Copy link
Contributor

@catageek if you don't mind, I'll start working directly on top of your commits in your branch, since there are multiple extremely minor things to polish. The overall PR looks absolutely great though, thanks a bunch! This is probably one of the best contributions to the repo I've seen: most issues come from the sheer size of it.

I'll also look into the tests and see if I can figure out what's going on there. I'll try to keep the commits to the point so that this can be more easily reviewed.

Once again, thanks!

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

I did a first pass review through the ERC777 contract. The documentation needs a lot of work here, but we'll do that in a separate PR.

contracts/drafts/ERC777/ERC777.sol Outdated Show resolved Hide resolved
contracts/drafts/ERC777/ERC777.sol Outdated Show resolved Hide resolved
contracts/drafts/ERC777/ERC777.sol Outdated Show resolved Hide resolved
contracts/drafts/ERC777/ERC777.sol Outdated Show resolved Hide resolved
contracts/drafts/ERC777/ERC777.sol Outdated Show resolved Hide resolved
contracts/drafts/ERC777/ERC777.sol Outdated Show resolved Hide resolved
contracts/drafts/ERC777/ERC777.sol Show resolved Hide resolved
@nventuro
Copy link
Contributor

All that's missing now are the _mint tests (which should be easy to add to the send/burn blocks).

Co-Authored-By: nventuro <nicolas.venturo@gmail.com>
@frangio
Copy link
Contributor

frangio commented Apr 15, 2019

For reference, I measured the gas cost of a send to an account (EOA) with no hooks registered.

  • No token balance in the receiver: 60695 gas
  • Some token balance in the receiver: 45695 gas

frangio
frangio previously approved these changes Apr 15, 2019
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

This is OK to merge IMO.

We should do a second review of the Solidity code during the release candidate period.

Thank you @catageek and @nventuro!

@nventuro nventuro merged commit 5a2b349 into OpenZeppelin:master Apr 16, 2019
@nventuro nventuro deleted the feature/ERC777-#1159 branch April 16, 2019 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contracts Smart contract code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide implementation of ERC777
5 participants