From 8204f6a71fcb1597e3c92117290600f4e752ca19 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Thu, 18 Oct 2018 19:07:34 -0300
Subject: [PATCH] Improved some ERC721 internal shenanigans (#1450)

* Made _clearApproval private, added clarifying comments in _addTokenTo and _removeTokenFrom.

* Added approval information.
---
 contracts/token/ERC721/ERC721.sol           | 31 ++++++++++++---------
 contracts/token/ERC721/ERC721Enumerable.sol |  5 ++++
 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol
index 3dc243874..ec0763b96 100644
--- a/contracts/token/ERC721/ERC721.sol
+++ b/contracts/token/ERC721/ERC721.sol
@@ -262,21 +262,10 @@ contract ERC721 is ERC165, IERC721 {
     emit Transfer(owner, address(0), tokenId);
   }
 
-  /**
-   * @dev Internal function to clear current approval of a given token ID
-   * Reverts if the given address is not indeed the owner of the token
-   * @param owner owner of the token
-   * @param tokenId uint256 ID of the token to be transferred
-   */
-  function _clearApproval(address owner, uint256 tokenId) internal {
-    require(ownerOf(tokenId) == owner);
-    if (_tokenApprovals[tokenId] != address(0)) {
-      _tokenApprovals[tokenId] = address(0);
-    }
-  }
-
   /**
    * @dev Internal function to add a token ID to the list of a given address
+   * Note that this function is left internal to make ERC721Enumerable possible, but is not
+   * intended to be called by custom derived contracts: in particular, it emits no Transfer event.
    * @param to address representing the new owner of the given token ID
    * @param tokenId uint256 ID of the token to be added to the tokens list of the given address
    */
@@ -288,6 +277,9 @@ contract ERC721 is ERC165, IERC721 {
 
   /**
    * @dev Internal function to remove a token ID from the list of a given address
+   * Note that this function is left internal to make ERC721Enumerable possible, but is not
+   * intended to be called by custom derived contracts: in particular, it emits no Transfer event,
+   * and doesn't clear approvals.
    * @param from address representing the previous owner of the given token ID
    * @param tokenId uint256 ID of the token to be removed from the tokens list of the given address
    */
@@ -322,4 +314,17 @@ contract ERC721 is ERC165, IERC721 {
       msg.sender, from, tokenId, _data);
     return (retval == _ERC721_RECEIVED);
   }
+
+  /**
+   * @dev Private function to clear current approval of a given token ID
+   * Reverts if the given address is not indeed the owner of the token
+   * @param owner owner of the token
+   * @param tokenId uint256 ID of the token to be transferred
+   */
+  function _clearApproval(address owner, uint256 tokenId) private {
+    require(ownerOf(tokenId) == owner);
+    if (_tokenApprovals[tokenId] != address(0)) {
+      _tokenApprovals[tokenId] = address(0);
+    }
+  }
 }
diff --git a/contracts/token/ERC721/ERC721Enumerable.sol b/contracts/token/ERC721/ERC721Enumerable.sol
index 0a2797eed..c85514720 100644
--- a/contracts/token/ERC721/ERC721Enumerable.sol
+++ b/contracts/token/ERC721/ERC721Enumerable.sol
@@ -72,6 +72,8 @@ contract ERC721Enumerable is ERC165, ERC721, IERC721Enumerable {
 
   /**
    * @dev Internal function to add a token ID to the list of a given address
+   * This function is internal due to language limitations, see the note in ERC721.sol.
+   * It is not intended to be called by custom derived contracts: in particular, it emits no Transfer event.
    * @param to address representing the new owner of the given token ID
    * @param tokenId uint256 ID of the token to be added to the tokens list of the given address
    */
@@ -84,6 +86,9 @@ contract ERC721Enumerable is ERC165, ERC721, IERC721Enumerable {
 
   /**
    * @dev Internal function to remove a token ID from the list of a given address
+   * This function is internal due to language limitations, see the note in ERC721.sol.
+   * It is not intended to be called by custom derived contracts: in particular, it emits no Transfer event,
+   * and doesn't clear approvals.
    * @param from address representing the previous owner of the given token ID
    * @param tokenId uint256 ID of the token to be removed from the tokens list of the given address
    */