Skip to content

ERC721 full implementation #803

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.

Sign up for GitHub

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

Merged
frangio merged 32 commits into from
Mar 23, 2018
Merged

ERC721 full implementation #803

Changes from 1 commit
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
1192e68
Rename current ERC721 implementation to BaseERC721
facuspagnuolo Feb 26, 2018
ca163a8
Implement ERC721 optional & approveAll functionality
facuspagnuolo Feb 26, 2018
71cbc51
Support for new ERC721 interface
spalladino Mar 7, 2018
3745025
Add more tests for ERC721
spalladino Mar 8, 2018
559df81
Implement suggestions by @dekz
spalladino Mar 8, 2018
d726c79
Update comments in ERC721 contracts
spalladino Mar 8, 2018
54a1d2e
Implement tokensByIndex extension
spalladino Mar 9, 2018
851685c
Add default implementation for metadata URI
spalladino Mar 9, 2018
3cef880
Allow operators to call approve on a token
spalladino Mar 9, 2018
6f180a6
Remove gas stipend restriction in call to 721 receiver
spalladino Mar 9, 2018
6fbe771
Remove deprecated implementation
spalladino Mar 9, 2018
626742e
Add notice to isContract helper on constract constructors
spalladino Mar 20, 2018
95a1f9a
Change natspec delimiters for consistency
spalladino Mar 21, 2018
15f9556
Minor linting fixes
spalladino Mar 21, 2018
b332995
Add constant modifier to ERC721_RECEIVED magic value
spalladino Mar 21, 2018
f4748da
Use 4-params safeTransferFrom for implementing the 3-params overload
spalladino Mar 21, 2018
fb4f728
Minor text changes in natspec comments
spalladino Mar 21, 2018
6b98e4e
Use address(0) instead of 0 or 0x0
spalladino Mar 21, 2018
3f2ea8a
Use if-statements instead of boolean one-liners for clarity
spalladino Mar 21, 2018
74db03b
Keep ownedTokensCount state var in sync in full ERC721 implementation
spalladino Mar 21, 2018
981c6f7
Fix incorrect comparison when burning ERC721 tokens with metadata
spalladino Mar 21, 2018
73b77ae
Use address(0) instead of 0 in one more place in ERC721
spalladino Mar 21, 2018
eee5b0e
Throw when querying balance for the zero address
spalladino Mar 21, 2018
9deb637
Update links to approved version of EIP721
spalladino Mar 21, 2018
fe6e4ff
Use explicit size for uint
spalladino Mar 22, 2018
4836279
Remove unneeded internal function in ERC721
spalladino Mar 22, 2018
619ae84
Use underscore instead of 'do' prefix for internal methods in ERC721
spalladino Mar 22, 2018
2e593f2
Fix failing test due to events reordering in ERC721 safe transfer
spalladino Mar 22, 2018
6c09d20
Fix bug introduced in 74db03ba06
spalladino Mar 22, 2018
37929c8
Remove do prefix for internal setTokenUri method
spalladino Mar 22, 2018
3676b55
Allow transfers to self in ERC721
spalladino Mar 23, 2018
7815cc5
Merge branch 'master' into feature/full_erc721
frangio Mar 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Implement suggestions by @dekz
  • Loading branch information
@spalladino
spalladino committed Mar 9, 2018
commit 559df81c4902094e12ed3e5860bea68c6d0166a6
5 changes: 2 additions & 3 deletions contracts/token/ERC721/ERC721BasicToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,13 +195,12 @@ contract ERC721BasicToken is ERC721Basic {
require(_from != address(0));
Copy link
Contributor

@shrugs shrugs Mar 12, 2018

Choose a reason for hiding this comment

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

Can we replace all of the require( addr != address(0) ) checks with an internal function like isValidAddress(addr)? Or is there a gas concern here?

Copy link
Contributor

@frangio frangio Mar 19, 2018

Choose a reason for hiding this comment

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

I think there are many ways for an address to be "invalid" so I would prefer to just be explicit here that we're checking for the null address.

require(_to != address(0));
require(_to != ownerOf(_tokenId));
Copy link
Contributor

@frangio frangio Mar 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Is this precondition in the EIP?

The Nov 9th Draft specifically allowed transfers to oneself as a no-op. Was that explicitly changed along the way before being published?

Copy link
Contributor Author

@spalladino spalladino Mar 21, 2018

Choose a reason for hiding this comment

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

I'm checking the approved EIP and I don't see any mentions to either forbidding them or allowing them.

If we do allow transfers to self, and they should be a no-op, should they clear approvals?

Copy link

@mudgen mudgen Mar 21, 2018

Choose a reason for hiding this comment

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

Also, why is the implementation disallowing approval to self?

Copy link
Contributor Author

@spalladino spalladino Mar 21, 2018

Choose a reason for hiding this comment

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

I can't think of a scenario where it makes sense, so an approval to self is most likely a mistake, so it makes sense to revert on that case.

Copy link
Contributor Author

@spalladino spalladino Mar 21, 2018

Choose a reason for hiding this comment

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

@frangio let me know if you find the no-op requirement on the spec so we can discuss how to handle it

require(ownerOf(_tokenId) == _from);

clearApproval(_from, _tokenId);
removeToken(_from, _tokenId);
addToken(_to, _tokenId);

require(!_safe || checkSafeTransfer(_from, _to, _tokenId, _data));
require(!_safe || checkAndCallSafeTransfer(_from, _to, _tokenId, _data));
Copy link
Contributor

@frangio frangio Mar 19, 2018

Choose a reason for hiding this comment

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

I'd find it clearer if it read: if (_safe) { require(check...); }.


Transfer(_from, _to, _tokenId);
}
Expand Down Expand Up @@ -240,7 +239,7 @@ contract ERC721BasicToken is ERC721Basic {
tokenOwner[_tokenId] = 0;
}

function checkSafeTransfer(address _from, address _to, uint256 _tokenId, bytes _data) internal returns (bool) {
function checkAndCallSafeTransfer(address _from, address _to, uint256 _tokenId, bytes _data) internal returns (bool) {
return !_to.isContract() ||
Copy link
Contributor

@frangio frangio Mar 19, 2018

Choose a reason for hiding this comment

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

I think it would be clearer to break this into an if conditional with a temporary variable for the return value.

Copy link
Contributor Author

@spalladino spalladino Mar 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Hmph I really like to use one-liners with boolean operators (as you have already seen), but I acknowledge that if statements may be easier to follow

(ERC721Receiver(_to).onERC721Received.gas(SAFE_TRANSFER_GAS_STIPEND)(_from, _tokenId, _data) == ERC721_RECEIVED);
Copy link

@dekz dekz Mar 8, 2018

Choose a reason for hiding this comment

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

As mentioned in gitter I think we should discuss this STIPEND more and its pros and cons.

}
Expand Down