Fix/erc721 #993
Fix/erc721 #993
Conversation
There was a problem hiding this comment.
I'm a bit confused by the sequence of commits 61ff1f7, 41bc037, and 878df54. The second reverts the first, and the third one implements the same thing again? 😅
Those lines are there because it was the semantics of Approval specified in the "Second Draft" of ERC721 that is listed in ethereum/EIPs#721. Look for the table in the description of approve. I actually loved that table because it was such a precise specification, but it was removed from the final version 😞 and now the semantics are not clear. I think always emitting Approval in approve is the safest thing.
contracts/token/ERC721/ERC721.sol
Outdated
| _registerInterface(InterfaceId_ERC721Enumerable); | ||
| } | ||
|
|
||
| contract ERC721Enumerable { |
There was a problem hiding this comment.
Was removing the ERC721Basic parent intentional? If so, what is the rationale? (Same question for ERC721Metadata.)
There was a problem hiding this comment.
ah, it was not intentional thanks for catching that. made the mistake of removing both parents instead of just the 165 implementation one
|
your confusion over my commits parallels my own confusion while making them 😂 |
|
|
||
|
|
||
| /** | ||
| * @title ERC721 Non-Fungible Token Standard basic implementation | ||
| * @dev see https://github.com/ethereum/EIPs/blob/master/EIPS/eip-721.md | ||
| */ | ||
| contract ERC721BasicToken is ERC721Basic { | ||
| contract ERC721BasicToken is ERC721Basic, SupportsInterfaceWithLookup { |
There was a problem hiding this comment.
Later on there is
contract ERC721Token is SupportsInterfaceWithLookup, ERC721BasicToken, ERC721 {SupportsInterfaceWithLookup being last in one inheritance list and first in another might bring problems with linearization later on. I would stick to always first or always last. I'm not sure about any of this though. 😣 Damn C3.
|
Ok, I've clarified on ETH-NFT/Lobby that Approval should be emitted on a reaffirmation of 0->0, so this change is accurate. |
|
via: ethereum/EIPs#1151 |
|
Please consider to remove "WIP" from the PR title if a PR is merged. |
fixes #990
Based on my interpretation of
This emits when the approved address for an NFT is changed reaffirmed., I made theapprove()function remove the restriction on not allowing a reaffirmation of theaddress(0)approval.The spec isn't detailed enough for this, it seems, so my interpretation might be wrong. Would appreciate feedback.
🚀 Description
npm run lint:all:fix).