search

Consensys / Tokens

Ethereum Token Contracts
MIT License
2.06k stars 1.2k forks source link

ERC721 (wip) #98

Closed simondlr closed 6 years ago

simondlr commented 6 years ago

This PR contains an implementation of ERC721 as well a test contract that extends it. It contains various tests.

I want some eyes & ACKs on this before we rebase & merge.

simondlr commented 6 years ago

Thanks @GNSPS for inspecting. Tests work. Getting to writing more tests. Like ERC20 interface, I removed the functions that are implemented as public vars in the implementation.

skmgoldin commented 6 years ago

Any reason you don't inherit and extend StandardToken.sol in ERC721Interface.sol?

skmgoldin commented 6 years ago

Also, could you make this PR to staging? 😇

simondlr commented 6 years ago

Any reason you don't inherit and extend StandardToken.sol in ERC721Interface.sol?

Not sure what you mean here? ERC721 is substantially functionally different...

Also, could you make this PR to staging?

Yip! Changed. :)

simondlr commented 6 years ago

Will rebase to clean commits when the tests are done.

maurelian commented 6 years ago

This is looking great @simondlr. There are a few changes (mostly structural) that would help this to conform with the direction we're hoping to take this repo. Apologies for not communicating it more clearly yet before now.

  1. Move the solidity folder into /contracts/erc721 (as has been done in this branch: https://github.com/ConsenSys/Tokens/tree/staging)
  2. Split up the tests similarly into /tests/erc721
  3. Even better, break tests out into per-function files to conform with #102
  4. Lint the solidity files with solhint (https://protofire.github.io/solhint/)

The easiest thing for you might be to build on top of this branch: https://github.com/ConsenSys/Tokens/tree/Elaniobro-Gitcoin-linter

GNSPS commented 6 years ago

We can implement solhint at CI level just like Mike did for eslint

simondlr commented 6 years ago

Some more tests. Found a bug that took a while to dissect. Yay for tests! Last bunch is the approves. Will continue tomorrow on that.

simondlr commented 6 years ago

Okay. There might be some extra tests I want to write wrt juggling tokens in various stages of approval & transferred & multiples of them, but it seems good enough for now so that it can get more eyes on them.

Once there's ACKs, I'll rebase and clean up merge conflicts.

simondlr commented 6 years ago

This change requires babel btw since I pulled in updated test helpers from OpenZeppelin.

simondlr commented 6 years ago

Anyone still keen to take a look?

simondlr commented 6 years ago

Just to add to above.

Reviewing this is not urgent for us at Ujo. We are still in testnet for the foreseeable future. Will be undergoing a proper audit in due time, which would include an audit of this contract code.

With the current conversations around ERC 777 (new ERC20) & ERC821 (similar style to ERC777 for ERC721), it might be better to just update to these standards instead. So don't have to do double work, if we just anyway implement ERC777 & ERC821 instead into the future. :)

simondlr commented 6 years ago

I'm going to close this for now. ERC721 after 3 months of back & forth finished discussions and implements a new standard (adding in changes from ERC821).

Seen here: https://github.com/ethereum/EIPs/pull/841.

I'm going to create a new PR rather and start from scratch since this one will just muddy the conversation.