ERC721 unnecessarily emits an Approval event on transfers

[abandeali1]

🎉 Description

• 🐛 This is a bug report.
• 📈 This is a feature request.

Currently, an Approval event is unnecessarily emitted when a transfer that resets an approval occurs. See comments towards the end of ethereum/EIPs#841 clarifying that an Approval event should not be emitted in this scenario.

[shrugs]

The 841 thread can't be used for identifying the standard's requirements, since they've been outdated by various updates to the primary spec at http://eips.ethereum.org/EIPS/eip-721

The text of 721:

    /// @dev This emits when ownership of any NFT changes by any mechanism.
    ///  This event emits when NFTs are created (`from` == 0) and destroyed
    ///  (`to` == 0). Exception: during contract creation, any number of NFTs
    ///  may be created and assigned without emitting Transfer. At the time of
    ///  any transfer, the approved address for that NFT (if any) is reset to none.
    event Transfer(address indexed _from, address indexed _to, uint256 indexed _tokenId);

    /// @dev This emits when the approved address for an NFT is changed or
    ///  reaffirmed. The zero address indicates there is no approved address.
    ///  When a Transfer event emits, this also indicates that the approved
    ///  address for that NFT (if any) is reset to none.
    event Approval(address indexed _owner, address indexed _approved, uint256 indexed _tokenId);

Specifically, At the time of any transfer, the approved address for that NFT (if any) is reset to none. followed by This emits when the approved address for an NFT is changed or reaffirmed. doesn't provide any special case for approval changes as a side effect of transfers.

Let me know if that should be interpreted differently

[abandeali1]

For the record, the relevant comment is currently the last comment in the EIP, and I haven't seen any more recent comments or updates to the spec that would challenge this assertion (by @fulldecent: The Approval event is not emitted on transfer.) . I understand that the spec should be what we base the actual implementation off of, though.

I guess the line At the time of any transfer, the approved address for that NFT (if any) is reset to none. seems a bit out of place to me. If an Approval event is also emitted, why is there even a need to mention the Transfer event?

Overall, I think your interpretation of the spec is probably correct. I'm just not sure if this is the intent of the spec though.

[shrugs]

@abandeali1 ah, sorry for not checking the thread directly. didn't realize it was fulldecent that had commented on the distinction. I do think my interpretation makes much more sense than the special case for transfer-initiated approval resets, but I'm not sure how flexible the standard is, now that it's been finalized. What are the pros towards not emitting the Approval event? An indexer trying to ascertain the full approval log of a token must also understand the implicit side effects of a Transfer event if they are to build an accurate history.

Anyway, the spec is confusing in this case, and the language should be updated to very clearly state the intention. I've reached out to @fulldecent in the ETH-NFt chat to get his understanding.

[abandeali1]

I just find it to be redundant - you're not gaining any extra information that you don't already get from the Transfer event (and of course, this costs gas). ERC20 doesn't emit an Approval event every time transferFrom is called either.

[shrugs]

I can understand that, yeah. Looks like fulldecent has already approved and pushed the oxcert change, so I'll take that as the intent of the spec. It would be nice to get the wording clearer in the future.

[fulldecent]

Hello all, I'm dropping by from a note in NFT/Lobby. I can see that a specific word in the spec is ambiguous and has lead to two different interpretations:

/// @dev This emits when the approved address for an NFT is changed or
///  reaffirmed. The zero address indicates there is no approved address.
///  When a Transfer event emits, this also indicates that the approved
///  address for that NFT (if any) is reset to none.
event Approval(address indexed _owner, address indexed _approved, uint256 indexed _tokenId);

It would have been more clear to say "It is unnecessary to emit Approval when a transfer occurs," and that is the intention of these words in the standard.

I can see the problem is my poor choice of "this also indicates" -- where "this" is the Transfer.

To be clear, an implementation that emits a redundant Approval event is compliant with the standard.

---

If OZ will accept this interpretation of the standard and make the change, then I would appreciate if anybody can propose an errata for 721 to clarify this.