ERC721 (wip) #98
ERC721 (wip) #98
Conversation
contracts/ERC721.sol
Outdated
| } | ||
|
|
||
| //how many badges of a specific release someone owns | ||
| function balanceOf(address _owner) public view returns (uint256 balance) { |
There was a problem hiding this comment.
I propose taking the named var return out.
Changing this: returns (uint256 balance)
To this: returns (uint256)
Since the balance var is not even used
contracts/ERC721Interface.sol
Outdated
| function tokenOfOwnerByIndex(address _owner, uint256 _index) external view returns (uint256 tokenId); | ||
|
|
||
| //links to more metadata | ||
| function tokenMetadata(uint256 _tokenId) public view returns (string infoUrl); |
There was a problem hiding this comment.
Also if we're inheriting the interface to make sure all the functions are specified in the contract then this one is not.
contracts/ERC721Interface.sol
Outdated
| contract ERC721Interface { | ||
|
|
||
| //TODO: should this be done vs https://github.com/ethereum/EIPs/issues/165? | ||
| function implementsERC721() public pure returns (bool); |
There was a problem hiding this comment.
Also another interface definition not implemented in the actual token contract.
test/StandardERC721.js
Outdated
|
|
||
| contract('TestERC721Implementation', function (accounts) { | ||
| beforeEach(async () => { | ||
| // todo: deployment is OOG-ing. |
There was a problem hiding this comment.
Not OOGing. It is actually failing at compile time for not implementing all the function definitions on the interface.
|
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. |
|
Any reason you don't inherit and extend |
|
Also, could you make this PR to |
Not sure what you mean here? ERC721 is substantially functionally different...
Yip! Changed. :) |
|
Will rebase to clean commits when the tests are done. |
|
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.
The easiest thing for you might be to build on top of this branch: https://github.com/ConsenSys/Tokens/tree/Elaniobro-Gitcoin-linter |
|
We can implement solhint at CI level just like Mike did for eslint |
|
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. |
|
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. |
|
This change requires babel btw since I pulled in updated test helpers from OpenZeppelin. |
|
Anyone still keen to take a look? |
|
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. :) |
|
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: ethereum/EIPs#841. I'm going to create a new PR rather and start from scratch since this one will just muddy the conversation. |
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.