Remove unnecessary SafeMath call

CHANGELOG.md

@@ -2,6 +2,12 @@
 
 ## 2.2.0 (unreleased)
 
+### New features:
+ * `Counter`'s API has been stabilized, and therefore moved out of the `drafts` directory into `utils`.
+
+### Improvements::
+ * `ERC721`'s transfers are now more gas efficient due to removal of unnecessary `SafeMath` calls.
+
 ## 2.1.1 (2019-04-01)
  * Version bump to avoid conflict in the npm registry.
 

contracts/mocks/CounterImpl.sol

@@ -1,17 +1,21 @@
 pragma solidity ^0.5.0;
 
-import "../drafts/Counter.sol";
+import "../utils/Counter.sol";
 
 contract CounterImpl {
     using Counter for Counter.Counter;
 
-    uint256 public theId;
+    Counter.Counter private _counter;
 
-    // use whatever key you want to track your counters
-    mapping(string => Counter.Counter) private _counters;
+    function current() public view returns (uint256) {
+        return _counter.current();
+    }
+
+    function increment() public {
+        _counter.increment();
+    }
 
-    function doThing(string memory key) public returns (uint256) {
-        theId = _counters[key].next();
-        return theId;
+    function decrement() public {
+        _counter.decrement();
     }
 }

contracts/token/ERC721/ERC721.sol

@@ -4,6 +4,7 @@ import "./IERC721.sol";
 import "./IERC721Receiver.sol";
 import "../../math/SafeMath.sol";
 import "../../utils/Address.sol";
+import "../../utils/Counter.sol";
 import "../../introspection/ERC165.sol";
 
 /**
@@ -13,6 +14,7 @@ import "../../introspection/ERC165.sol";
 contract ERC721 is ERC165, IERC721 {
     using SafeMath for uint256;
     using Address for address;
+    using Counter for Counter.Counter;
 
     // Equals to `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`
     // which can be also obtained as `IERC721Receiver(0).onERC721Received.selector`
@@ -25,7 +27,7 @@ contract ERC721 is ERC165, IERC721 {
     mapping (uint256 => address) private _tokenApprovals;
 
     // Mapping from owner to number of owned token
-    mapping (address => uint256) private _ownedTokensCount;
+    mapping (address => Counter.Counter) private _ownedTokensCount;
 
     // Mapping from owner to operator approvals
     mapping (address => mapping (address => bool)) private _operatorApprovals;
@@ -56,7 +58,7 @@ contract ERC721 is ERC165, IERC721 {
      */
     function balanceOf(address owner) public view returns (uint256) {
         require(owner != address(0));
-        return _ownedTokensCount[owner];
+        return _ownedTokensCount[owner].current();
     }
 
     /**
@@ -200,7 +202,7 @@ contract ERC721 is ERC165, IERC721 {
         require(!_exists(tokenId));
 
         _tokenOwner[tokenId] = to;
-        _ownedTokensCount[to] = _ownedTokensCount[to].add(1);
+        _ownedTokensCount[to].increment();
 
         emit Transfer(address(0), to, tokenId);
     }
@@ -217,7 +219,7 @@ contract ERC721 is ERC165, IERC721 {
 
         _clearApproval(tokenId);
 
-        _ownedTokensCount[owner] = _ownedTokensCount[owner].sub(1);
+        _ownedTokensCount[owner].decrement();
         _tokenOwner[tokenId] = address(0);
 
         emit Transfer(owner, address(0), tokenId);
@@ -245,8 +247,8 @@ contract ERC721 is ERC165, IERC721 {
 
         _clearApproval(tokenId);
 
-        _ownedTokensCount[from] = _ownedTokensCount[from].sub(1);
-        _ownedTokensCount[to] = _ownedTokensCount[to].add(1);
+        _ownedTokensCount[from].decrement();
+        _ownedTokensCount[to].increment();
 
         _tokenOwner[tokenId] = to;
 

contracts/drafts/Counter.sol → contracts/utils/Counter.sol

@@ -1,5 +1,7 @@
 pragma solidity ^0.5.0;
 
+import "../math/SafeMath.sol";
+
 /**
  * @title Counter
  * @author Matt Condon (@shrugs)
@@ -13,12 +15,21 @@ pragma solidity ^0.5.0;
  * so it's not something you have to worry about.)
  */
 library Counter {
+    using SafeMath for uint256;
+
     struct Counter {
-        uint256 current; // default: 0
+        uint256 _value; // default: 0
+    }
+
+    function current(Counter storage counter) internal view returns (uint256) {
+        return counter._value;
+    }
+
+    function increment(Counter storage counter) internal {
+        counter._value += 1;
     }
 
-    function next(Counter storage index) internal returns (uint256) {
-        index.current += 1;
-        return index.current;
+    function decrement(Counter storage counter) internal {
+        counter._value = counter._value.sub(1);
     }
 }

test/drafts/Counter.test.js

@@ -1,37 +1,58 @@
-const { BN } = require('openzeppelin-test-helpers');
+const { shouldFail } = require('openzeppelin-test-helpers');
 
 const CounterImpl = artifacts.require('CounterImpl');
 
-const EXPECTED = [new BN(1), new BN(2), new BN(3), new BN(4)];
-const KEY1 = web3.utils.sha3('key1');
-const KEY2 = web3.utils.sha3('key2');
-
-contract('Counter', function ([_, owner]) {
+contract('Counter', function () {
   beforeEach(async function () {
-    this.mock = await CounterImpl.new({ from: owner });
+    this.counter = await CounterImpl.new();
+  });
+
+  it('starts at zero', async function () {
+    (await this.counter.current()).should.be.bignumber.equal('0');
   });
 
-  context('custom key', async function () {
-    it('should return expected values', async function () {
-      for (const expectedId of EXPECTED) {
-        await this.mock.doThing(KEY1, { from: owner });
-        const actualId = await this.mock.theId();
-        actualId.should.be.bignumber.equal(expectedId);
-      }
+  describe('increment', function () {
+    it('increments the current value by one', async function () {
+      await this.counter.increment();
+      (await this.counter.current()).should.be.bignumber.equal('1');
+    });
+
+    it('can be called multiple times', async function () {
+      await this.counter.increment();
+      await this.counter.increment();
+      await this.counter.increment();
+
+      (await this.counter.current()).should.be.bignumber.equal('3');
     });
   });
 
-  context('parallel keys', async function () {
-    it('should return expected values for each counter', async function () {
-      for (const expectedId of EXPECTED) {
-        await this.mock.doThing(KEY1, { from: owner });
-        let actualId = await this.mock.theId();
-        actualId.should.be.bignumber.equal(expectedId);
-
-        await this.mock.doThing(KEY2, { from: owner });
-        actualId = await this.mock.theId();
-        actualId.should.be.bignumber.equal(expectedId);
-      }
+  describe('decrement', function () {
+    beforeEach(async function () {
+      await this.counter.increment();
+      (await this.counter.current()).should.be.bignumber.equal('1');
+    });
+
+    it('decrements the current value by one', async function () {
+      await this.counter.decrement();
+      (await this.counter.current()).should.be.bignumber.equal('0');
+    });
+
+    it('reverts if the current value is 0', async function () {
+      await this.counter.decrement();
+      await shouldFail.reverting(this.counter.decrement());
+    });
+
+    it('can be called multiple times', async function () {
+      await this.counter.increment();
+      await this.counter.increment();
+
+      (await this.counter.current()).should.be.bignumber.equal('3');
+
+      await this.counter.decrement();
+      await this.counter.decrement();
+      await this.counter.decrement();
+
+      (await this.counter.current()).should.be.bignumber.equal('0');
     });
   });
 });