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

EIP721 #130

Open
wants to merge 4 commits into
base: staging
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
311 changes: 311 additions & 0 deletions contracts/eip721/EIP721.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,311 @@
pragma solidity ^0.4.21;
import "./EIP721Interface.sol";
import "./EIP721MetadataInterface.sol";
import "./EIP721EnumerableInterface.sol";
import "./EIP721TokenReceiverInterface.sol";

/*
This is a full implementation of all ERC721's features.
Influenced by OpenZeppelin's implementation with some stylistic changes.
https://github.com/OpenZeppelin/zeppelin-solidity/tree/master/contracts/token/ERC721
*/

contract EIP721 is EIP721Interface, EIP721MetadataInterface, EIP721EnumerableInterface, ERC165Interface {
string public name;
string public symbol;

// all tokens
uint256[] internal allTokens;
mapping(uint256 => uint256) internal allTokensIndex;
Copy link

Choose a reason for hiding this comment

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

Mapping of tokenID => position in allTokens. Maybe a comment here?

// Array of tokens owned by a specific owner
mapping(address => uint256[]) internal ownedTokens;
// Mapping from token ID to owner
mapping(uint256 => address) internal ownerOfToken;
// Mapping of the token ID to where it is in the owner's array.
mapping(uint256 => uint256) internal ownedTokensIndex;

// Mapping of a token to a specifically approved owner.
mapping(uint256 => address) internal approvedOwnerOfToken;

// An operator is allowed to manage all assets of another owner.
mapping(address => mapping (address => bool)) internal approvedOperators;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this just be called operators? It technically contains all operators, just with their boolean value flag set to false. Like, if I have some operator who I approve, and then I revoke the approval, we make the revocation assignment to the _approved_Operators mapping, which seems semantically questionable.


mapping(uint256 => string) internal tokenURIs;

// base ERC721 interface = 0x80ac58cd
// metadata interface = 0x5b5e139f
// enumerable interface = 0x780e9d63
bytes4 internal constant ERC721_BASE_INTERFACE_SIGNATURE = 0x80ac58cd;
bytes4 internal constant ERC721_METADATA_INTERFACE_SIGNATURE = 0x5b5e139f;
bytes4 internal constant ERC721_ENUMERABLE_INTERFACE_SIGNATURE = 0x780e9d63;

/* Modifiers */
modifier tokenExists(uint256 _tokenId) {
require(ownerOfToken[_tokenId] != 0);
_;
}

// checks: is the owner of the token == msg.sender?
// OR has the owner of the token granted msg.sender access to operate?
modifier allowedToOperate(uint256 _tokenId) {
require(checkIfAllowedToOperate(_tokenId));
_;
}

modifier allowedToTransfer(address _from, address _to, uint256 _tokenId) {
require(checkIfAllowedToOperate(_tokenId) || approvedOwnerOfToken[_tokenId] == msg.sender);
require(ownerOfToken[_tokenId] == _from);
require(_to != 0); //not allowed to burn in transfer method
_;
}

/// @notice Transfer ownership of an NFT -- THE CALLER IS RESPONSIBLE
/// TO CONFIRM THAT `_to` IS CAPABLE OF RECEIVING NFTS OR ELSE
/// THEY MAY BE PERMANENTLY LOST
/// @dev Throws unless `msg.sender` is the current owner, an authorized
/// operator, or the approved address for this NFT. Throws if `_from` is
/// not the current owner. Throws if `_to` is the zero address. Throws if
/// `_tokenId` is not a valid NFT.
/// @param _from The current owner of the NFT
/// @param _to The new owner
/// @param _tokenId The NFT to transfer
function transferFrom(address _from, address _to, uint256 _tokenId) external payable
tokenExists(_tokenId)
allowedToTransfer(_from, _to, _tokenId) {
//transfer token
settleTransfer(_from, _to, _tokenId);
}

/// @notice Transfers the ownership of an NFT from one address to another address
/// @dev Throws unless `msg.sender` is the current owner, an authorized
/// operator, or the approved address for this NFT. Throws if `_from` is
/// not the current owner. Throws if `_to` is the zero address. Throws if
/// `_tokenId` is not a valid NFT. When transfer is complete, this function
/// checks if `_to` is a smart contract (code size > 0). If so, it calls
/// `onERC721Received` on `_to` and throws if the return value is not
/// `bytes4(keccak256("onERC721Received(address,uint256,bytes)"))`.
/// @param _from The current owner of the NFT
/// @param _to The new owner
/// @param _tokenId The NFT to transfer
/// @param data Additional data with no specified format, sent in call to `_to`
function safeTransferFrom(address _from, address _to, uint256 _tokenId, bytes data) external payable
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be payable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As stated in the PR, I wondered about this too.

EIP721 by default sets transfer functions to add payable to transfer & approve functions. This does not seem the most secure to me. Should we perhaps think of forcing it to not have this by default?

I'm okay with following the standard and keeping payable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, sorry. Started the review on the plane, skipped the PR notes. Will review before I dig in again tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

The standard is actually quite flexible on this item.

So payable is unnecessary to stick with the standard.

Copy link

Choose a reason for hiding this comment

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

Regarding naming: I believe data can be _data (or the other parameters can have no underscore), since the function ID only uses the types of the parameters, not their Solidity names.

tokenExists(_tokenId)
allowedToTransfer(_from, _to, _tokenId) {
settleTransfer(_from, _to, _tokenId);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's a big deal, but since you have a possible revert later in this function, I think settleTransfer() would be best called at the end.

This would be in keeping with the "Checks, effects, interactions" pattern.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that's the case here since if you call the callback before settling the transfer the callback would have no different state to handle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only problem is that it's a call, which shouldn't that be usually after internal state handling? either/or, not a big problem, but wondering what is the best practice here.


// check if a smart contract
uint256 size;
assembly { size := extcodesize(_to) } // solhint-disable-line no-inline-assembly
if (size > 0) {
// call on onERC721Received.
require(EIP721TokenReceiverInterface(_to).onERC721Received(_from, _tokenId, data) == 0xf0b9e5ba);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add a constant for this signature like you have for others towards the top of the file?

}
}

/// @notice Transfers the ownership of an NFT from one address to another address
/// @dev This works identically to the other function with an extra data parameter,
/// except this function just sets data to ""
/// @param _from The current owner of the NFT
/// @param _to The new owner
/// @param _tokenId The NFT to transfer
function safeTransferFrom(address _from, address _to, uint256 _tokenId) external payable
tokenExists(_tokenId)
allowedToTransfer(_from, _to, _tokenId) {
settleTransfer(_from, _to, _tokenId);

// check if a smart contract
uint256 size;
assembly { size := extcodesize(_to) } // solhint-disable-line no-inline-assembly
if (size > 0) {
// call on onERC721Received.
require(EIP721TokenReceiverInterface(_to).onERC721Received(_from, _tokenId, "") == 0xf0b9e5ba);
}
}

/// @notice Set or reaffirm the approved address for an NFT
/// @dev The zero address indicates there is no approved address.
/// @dev Throws unless `msg.sender` is the current NFT owner, or an authorized
/// operator of the current owner.
/// @param _approved The new approved NFT controller
/// @param _tokenId The NFT to approve
function approve(address _approved, uint256 _tokenId) external payable
tokenExists(_tokenId)
allowedToOperate(_tokenId) {
address owner = ownerOfToken[_tokenId];
internalApprove(owner, _approved, _tokenId);
}

/// @notice Enable or disable approval for a third party ("operator") to manage
/// all of `msg.sender`'s assets.
/// @dev Emits the ApprovalForAll event. The contract MUST allow
/// multiple operators per owner.
/// @param _operator Address to add to the set of authorized operators.
/// @param _approved True if the operator is approved, false to revoke approval
function setApprovalForAll(address _operator, bool _approved) external {
require(_operator != msg.sender); // can't make oneself an operator
approvedOperators[msg.sender][_operator] = _approved;
emit ApprovalForAll(msg.sender, _operator, _approved);
}

/* public View Functions */
/// @notice Count NFTs tracked by this contract
/// @return A count of valid NFTs tracked by this contract, where each one of
/// them has an assigned and queryable owner not equal to the zero address
function totalSupply() external view returns (uint256) {
return allTokens.length;
}

/// @notice Find the owner of an NFT
/// @param _tokenId The identifier for an NFT
/// @dev NFTs assigned to zero address are considered invalid, and queries
/// about them do throw.
/// @return The address of the owner of the NFT
function ownerOf(uint256 _tokenId) external view
tokenExists(_tokenId) returns (address) {
return ownerOfToken[_tokenId];
}

/// @notice Enumerate valid NFTs
/// @dev Throws if `_index` >= `totalSupply()`.
/// @param _index A counter less than `totalSupply()`
/// @return The token identifier for the `_index`th NFT,
/// (sort order not specified)
function tokenByIndex(uint256 _index) external view returns (uint256) {
require(_index < allTokens.length);
return allTokens[_index];
}

/// @notice Enumerate NFTs assigned to an owner
/// @dev Throws if `_index` >= `balanceOf(_owner)` or if
/// `_owner` is the zero address, representing invalid NFTs.
/// @param _owner An address where we are interested in NFTs owned by them
/// @param _index A counter less than `balanceOf(_owner)`
/// @return The token identifier for the `_index`th NFT assigned to `_owner`,
/// (sort order not specified)
function tokenOfOwnerByIndex(address _owner, uint256 _index) external view
tokenExists(_tokenId) returns (uint256 _tokenId) {
require(_index < ownedTokens[_owner].length);
return ownedTokens[_owner][_index];
}

/// @notice Count all NFTs assigned to an owner
/// @dev NFTs assigned to the zero address are considered invalid, and this
/// function throws for queries about the zero address.
/// @param _owner An address for whom to query the balance
/// @return The number of NFTs owned by `_owner`, possibly zero
function balanceOf(address _owner) external view returns (uint256) {
require(_owner != 0);
return ownedTokens[_owner].length;
}

/// @notice Get the approved address for a single NFT
/// @dev Throws if `_tokenId` is not a valid NFT
/// @param _tokenId The NFT to find the approved address for
// todo: The approved address for this NFT, or the zero address if there is none
function getApproved(uint256 _tokenId) external view
tokenExists(_tokenId) returns (address) {
return approvedOwnerOfToken[_tokenId];
}

/// @notice Query if an address is an authorized operator for another address
/// @param _owner The address that owns the NFTs
/// @param _operator The address that acts on behalf of the owner
/// @return True if `_operator` is an approved operator for `_owner`, false otherwise
function isApprovedForAll(address _owner, address _operator) external view returns (bool) {
return approvedOperators[_owner][_operator];
}

/// @notice A distinct Uniform Resource Identifier (URI) for a given asset.
/// @dev Throws if `_tokenId` is not a valid NFT. URIs are defined in RFC
/// 3986. The URI may point to a JSON file that conforms to the "ERC721
/// Metadata JSON Schema".
function tokenURI(uint256 _tokenId) external view returns (string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't find a setter to actually populate the URI mapping?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It's not in the standard.

Copy link
Contributor

Choose a reason for hiding this comment

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

The standard also says this function is optional. I would remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Part of the goal for me was to implement all functionality in one contract only (subjective choice for readability). Removing this means the metadata interface is only partly implemented, which will also break the interface signature.

It's a subjective choice though.

return tokenURIs[_tokenId];
}

/// @notice Query if a contract implements an interface
/// @param interfaceID The interface identifier, as specified in ERC-165
/// @dev Interface identification is specified in ERC-165. This function
/// uses less than 30,000 gas.
/// @return `true` if the contract implements `interfaceID` and
/// `interfaceID` is not 0xffffffff, `false` otherwise
function supportsInterface(bytes4 interfaceID) external view returns (bool) {

if (interfaceID == ERC721_BASE_INTERFACE_SIGNATURE ||
interfaceID == ERC721_METADATA_INTERFACE_SIGNATURE ||
interfaceID == ERC721_ENUMERABLE_INTERFACE_SIGNATURE) {
return true;
} else { return false; }
}

/* -- Internal Functions -- */
function checkIfAllowedToOperate(uint256 _tokenId) internal view returns (bool) {
return ownerOfToken[_tokenId] == msg.sender || approvedOperators[ownerOfToken[_tokenId]][msg.sender];
}

function internalApprove(address _owner, address _approved, uint256 _tokenId) internal {
require(_approved != _owner); //can't approve to owner to itself

// Note: the following code is equivalent to: require(getApproved(_tokenId) != 0) || _approved != 0);
// However: I found the following easier to read & understand.
if (approvedOwnerOfToken[_tokenId] == 0 && _approved == 0) {
revert(); // add reason for revert?
Copy link

Choose a reason for hiding this comment

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

Add a comment such as "Revert on NOOP" for readability.

} else {
approvedOwnerOfToken[_tokenId] = _approved;
emit Approval(_owner, _approved, _tokenId);
}
}

function settleTransfer(address _from, address _to, uint256 _tokenId) internal {
//clear pending approvals if there are any
if (approvedOwnerOfToken[_tokenId] != 0) {
internalApprove(_from, 0, _tokenId);
}

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

emit Transfer(_from, _to, _tokenId);
}

function addToken(address _to, uint256 _tokenId) internal {
Copy link

Choose a reason for hiding this comment

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

_to should probably be _owner.

allTokens.push(_tokenId);
allTokensIndex[_tokenId] = allTokens.length-1;

// set token to be owned by address _to
ownerOfToken[_tokenId] = _to;
// add that token to an array keeping track of tokens owned by that address
ownedTokens[_to].push(_tokenId);
// shorten length
Copy link

Choose a reason for hiding this comment

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

This comment does not seem to correspond to what is happening. It's actually filling in the position of the just-added token in the owner's token list.

ownedTokensIndex[_tokenId] = ownedTokens[_to].length-1;
}

function removeToken(address _from, uint256 _tokenId) internal {
Copy link

Choose a reason for hiding this comment

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

_owner would be a better name for this parameter, since it is used to get the owner of the token.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both remove/add can be used in context's where it's not the owner specifically that's removing it.


// remove token from allTokens array.
uint256 allIndex = allTokensIndex[_tokenId];
uint256 allTokensLength = allTokens.length;
//1) Put last item into index of token to be removed.
allTokens[allIndex] = allTokens[allTokensLength - 1];
allTokensIndex[allTokensLength - 1] = allIndex;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this line doing? Is it necessary? Did you mean to do something else? Here is a mental code execution.

At time zero, we have these data structures:
allTokens = [A, B, C]
allTokensIndex = { A : 0, B : 1, C: 2 }

Say we want to get rid of NFT B. We will correspond line numbers in this code to our mental execution:

286: allIndex = 1
287: allTokensArrayLength = 3

289: allTokens = [A, C, C] // Replace B with the content of the last array item
290: allTokensIndex = { A : 0, B : 1, C: 2, 2 : 1 } // ????

Execution continues and successfully truncates the array:
292: allTokens = [A, C, -]
293: allTokens = [A, C]

But what is happening, or what was supposed to happen on line 290? In a mapping of tokenIds to array indexes, you added an array index as a key with an array index as a value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same question on line 303

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you found a bug @skmgoldin. Here's what should happen.

Here's a mental exercise.

allTokens = [100, 123, 142, 161, 170] // using larger ids for example
allTokensIndex = { 100: 0, 123: 1, 142: 2, 161: 3, 170: 4 }

Remove Token 123.

286: allIndex = 1
287: allTokensArrayLength = 5

289: allTokens = [100, 170, 142, 161, 170]

Now, in line 290, allTokensIndex of id 170, should be changed to 1 (replacing 123's index). So the line should be:

allTokensIndex[allTokens[allTokensLength-1]] = allIndex;

This takes the id of the last token (170) and changes its index to the index of the token just removes (123).

Can we got a confirm on this @GNSPS @maurelian?

[variable naming is hard]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This crept through because the tests have the same indexes as its IDs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should basically change the TestImplementation to start ids at 10 [so that it doesn't match the indexes]. Fix incoming.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Confirmed! ❤️ Eternal love to @skmgoldin & crew for doing this bug hunting!

//2) delete last item (since it's now a duplicate)
delete allTokens[allTokensLength-1];
//3) reduce length of array
allTokens.length -= 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a possible underflow here?


// remove token from owner array.
// get the index of where this token is in the owner's array
uint256 ownerIndex = ownedTokensIndex[_tokenId];
uint256 ownerLength = ownedTokens[_from].length;
/* Remove Token From Index */
//1) Put last item into index of token to be removed.
ownedTokens[_from][ownerIndex] = ownedTokens[_from][ownerLength-1];
ownedTokensIndex[ownerLength-1] = ownerIndex;
//2) delete last item (since it's now a duplicate)
delete ownedTokens[_from][ownerLength-1];
//3) reduce length of array
ownedTokens[_from].length -= 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a possible underflow here?


delete ownerOfToken[_tokenId];
}
}
28 changes: 28 additions & 0 deletions contracts/eip721/EIP721EnumerableInterface.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
pragma solidity ^0.4.21;


/// @title ERC-721 Non-Fungible Token Standard, optional enumeration extension
/// @dev See https://github.com/ethereum/EIPs/blob/master/EIPS/eip-721.md
/// Note: the ERC-165 identifier for this interface is 0x780e9d63
interface EIP721EnumerableInterface {
/// @notice Count NFTs tracked by this contract
/// @return A count of valid NFTs tracked by this contract, where each one of
/// them has an assigned and queryable owner not equal to the zero address
function totalSupply() external view returns (uint256);

/// @notice Enumerate valid NFTs
/// @dev Throws if `_index` >= `totalSupply()`.
/// @param _index A counter less than `totalSupply()`
/// @return The token identifier for the `_index`th NFT,
/// (sort order not specified)
function tokenByIndex(uint256 _index) external view returns (uint256);

/// @notice Enumerate NFTs assigned to an owner
/// @dev Throws if `_index` >= `balanceOf(_owner)` or if
/// `_owner` is the zero address, representing invalid NFTs.
/// @param _owner An address where we are interested in NFTs owned by them
/// @param _index A counter less than `balanceOf(_owner)`
/// @return The token identifier for the `_index`th NFT assigned to `_owner`,
/// (sort order not specified)
function tokenOfOwnerByIndex(address _owner, uint256 _index) external view returns (uint256 _tokenId);
}
Loading