search

mattlockyer / composables-998

An implementation and documentation repo for developing the ERC-998 standard for Ethereum.
MIT License
103 stars 64 forks source link

ERC998PossessERC721.sol changes #4

Closed mudgen closed 6 years ago

mudgen commented 6 years ago

This is actually the first time I have done a pull request so excuse me if I do something wrong. And let me know how to do it better next time.

I made some interesting changes to ERC998PossessERC721.sol. Let me describe why I made the changes.

  1. I replaced the pseudo-addresses with nested maps because the pseudo-addresses don't help with gas.
  2. I got rid of mapping(address => bool) childOwned. I did this by indexing childTokenIndex starting with 1 instead of 0 so that a 0 value means that the child does not exist. This change reduces the gas in the onERC721Received function by about 20,000 gas because a separate owned value does not need to be stored.
  3. I replaced the childReceived function with onERC721Received.
  4. The onERC721Received function expects the _data argument to contain an actual integer value instead of an ascii string that represents a number. Up to 32 bytes of the _data argument is converted to an uint256. I made this change for these reasons:
    1. The bit of assembly that converts bytes to an uint256 uses much less gas than converting a string number into a uint256.
    2. Up to the first 32 bytes of _data are converted to uint256. This allows bytes after the first 32 to be contract specific and customizable by contracts for their purposes.
  5. The onERC721Received function only adds a new contract when it doesn't already exist and the removeChild function only removes a contract when the number of tokens for it becomes 0.
  6. The code that calls safeTransferFrom is moved after bookkeeping to prevent re-entry attacks. If this was executed before bookkeeping then it would be possible to mess up the bookeeping by re-entrying the contract when safeTransferFrom is called.
  7. The transferChild(address _to, uint256 _tokenId, address _childContract, uint256 _childTokenId, bytes data) function can be used to transfer child tokens to user accounts and to transfer tokens to consumable tokens.
  8. I created the interfaces ERC998NFT and ERC998NFTEnumerable and implemented them.
  9. I added the childOwnerOf functions. This is like the ownerOf function from ERC721 except it gives the token owner of a child token.
  10. I added the childTokenOwner mapping which is used by the childOwnerOf function.

I would love some feedback on all these changes and additions.

mattlockyer commented 6 years ago

Nick! WOW 😄

Thank you so much for all the work you put into this commit! I can't wait to try it out myself and dig in deeper today.

We may discuss some of the finer points but this is a tremendous leap forward so thank you for bringing new energy here!

mudgen commented 6 years ago

Great! Let me know how it goes and I am interested in discussing things. More to come!