OpenZeppelin / openzeppelin-contracts

OpenZeppelin Contracts is a library for secure smart contract development.
https://openzeppelin.com/contracts
MIT License
24.8k stars 11.77k forks source link

Provide implementation of ERC777 #1159

Closed frangio closed 5 years ago

frangio commented 6 years ago

ERC777 is a proposal for a token standard with advanced features that seems to be well received and may gain some adoption.

We would like to provide an implementation of it. There is a reference implementation that is licensed under MPL so we would likely have to write our own.

albertchon commented 6 years ago

I am working on this and will submit a pull request once @jacquesd grants permission to release a modified version of his implementation under the MIT license

catageek commented 5 years ago

I just pushed in my branch an implementation of ERC777 with all unit tests. This is not perfect and the unit tests need some refactorization to be in line with the code style of other tests. This is my first approach of javascript, so be nice with me. Also the licence of the interface is MPL because it's an exact copy of the one written by @jacquesd, so until the licence of the interface change, making a PR is not possible.

Note that the constructor may call an external contract, which may be an issue because the calling contract is not finalized. Maybe a function initialize() should be added to do this external call.

Also I used web3js 1.0 in the tests to get rid of the collision of send() with truffle's own send(), so I can use the interface introspection functionality of web3js 1.0.

cipherzzz commented 5 years ago

@catageek - I tried to implement your branch but was not able to deploy the contract using truffle or my unit tests. I keep getting a revert. Could you pull this truffle project and try it out?

git clone https://github.com/cipherzzz/erc777 && cd erc777
truffle compile
truffle migrate
truffle test
catageek commented 5 years ago

@cipherzzz This contract needs that an ERC820 contract is previously deployed in the test chain. Try to execute truffle test before truffle migrate because there is a test script that deploys an ERC820 registry contract. Disclaimer: I did not update the ERC820 bytecode to the final version of the EIP

utgarda commented 5 years ago

Guys, @catageek, I actually started implementing ERC777 from scratch, following the specs, in order to let it be published under MIT licence, instead of trying to re-licence the reference implementation. I believe, if @jacquesd wanted his code MIT-licenced, he'd do it from the start. For the sake of integrity I didn't even read the reference code thoroughly (maybe that's a mistake), and definitely never copied any of it. Anyway, that code requires a whole bunch of new unit tests to be compliant with OpenZeppelin's repo style.

How do you feel about cooperating on re-implementing the specs following the local guidelines?

catageek commented 5 years ago

Hi @utgarda, my implementation is already from scratch without looking at the reference code, so, according to me, the issue with the license is not in the implementation, but with the interface. This file was copied from the reference implementation because it is the reference interface that we can not modify. It is in the EIP under Creative Commons license, and in the ref implementation under Mozilla Public license. I am not an expert in licensing so I can't say if it's a real issue.

nventuro commented 5 years ago

I'll be working on this to get it to a state in which it can be merged into the repository (as a draft, since the EIP isn't even finalized yet). @utgarda @catageek you both mentioned you've done some work on this, would you mind sharing your progress? Thanks!

catageek commented 5 years ago

@nventuro my implementation is complete with all unit tests. I need to udpate ERC820 registry to use ERC1820. I also need to run linter to conform to OpenZeppelin PR rules, so that's why it's not a PR yet. Would you mind if we don't multiply proposals ? I can finish this in a couple of days.

nventuro commented 5 years ago

@catageek do open a PR! I already have one for ERC1820 (https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1677) which should be merged soon, see if you can integrate that one in.

catageek commented 5 years ago

@nventuro Done! But it's coming with its own ERC1820 deploy script... Feel free to add your modification/suggestion. Any help to break the big test script into small files is appreciated also.