Alex/move erc20 wrapper and add specs

.solcover.js

@@ -5,7 +5,6 @@ module.exports = {
     'Migrations.sol',
     'mocks',
     'external',
-    'core/lib/ERC20Wrapper.sol', // TODO: Find a cleaner way to test state mutating functions, we will skip
     'core/exchange-wrappers/ZeroExExchangeWrapper.sol' // TODO: test this
   ],
 };

contracts/core/TransferProxy.sol

@@ -19,7 +19,7 @@ pragma solidity 0.4.24;
 
 import { Authorizable } from "../lib/Authorizable.sol";
 import { SafeMath } from "zeppelin-solidity/contracts/math/SafeMath.sol";
-import { ERC20Wrapper } from "./lib/ERC20Wrapper.sol";
+import { ERC20Wrapper } from "../lib/ERC20Wrapper.sol";
 
 
 /**

contracts/core/Vault.sol

@@ -19,7 +19,7 @@ pragma solidity 0.4.24;
 
 import { Authorizable } from "../lib/Authorizable.sol";
 import { SafeMath } from "zeppelin-solidity/contracts/math/SafeMath.sol";
-import { ERC20Wrapper } from "./lib/ERC20Wrapper.sol";
+import { ERC20Wrapper } from "../lib/ERC20Wrapper.sol";
 
 
 /**

contracts/core/exchange-wrappers/ZeroExExchangeWrapper.sol

@@ -23,7 +23,7 @@ import { LibBytes } from "../../external/0x/LibBytes.sol";
 import { LibOrder as ZeroExOrder } from "../../external/0x/Exchange/libs/LibOrder.sol";
 import { LibFillResults as ZeroExFillResults } from "../../external/0x/Exchange/libs/LibFillResults.sol";
 import { IExchange as ZeroExExchange } from "../../external/0x/Exchange/interfaces/IExchange.sol";
-import { ERC20Wrapper as ERC20 } from "../../core/lib/ERC20Wrapper.sol";
+import { ERC20Wrapper as ERC20 } from "../../lib/ERC20Wrapper.sol";
 
 
 

contracts/core/exchange-wrappers/lib/ZeroExOrderDataHandler.sol

@@ -36,7 +36,7 @@ library ZeroExOrderDataHandler {
 
     bytes4 constant ERC20_SELECTOR = bytes4(keccak256("ERC20Token(address)"));
 
-    string constant INVALID_TOKEN_ADDRESS = 'Address is not for ERC20 asset.';
+    string constant INVALID_TOKEN_ADDRESS = "Address is not for ERC20 asset.";
 
     // ============ Structs ============
 

contracts/core/extensions/CoreInternal.sol

@@ -95,7 +95,7 @@ contract CoreInternal is
         state.validFactories[_factoryAddress] = false;
         for (uint256 i = 0; i < state.factories.length; i++) {
             if (state.factories[i] == _factoryAddress) {
-                state.factories[i] = state.factories[state.factories.length -1];
+                state.factories[i] = state.factories[state.factories.length - 1];
                 state.factories.length -= 1;
                 break;
             }
@@ -117,7 +117,7 @@ contract CoreInternal is
         state.validSets[_setAddress] = false;
         for (uint256 i = 0; i < state.setTokens.length; i++) {
             if (state.setTokens[i] == _setAddress) {
-                state.setTokens[i] = state.setTokens[state.setTokens.length -1];
+                state.setTokens[i] = state.setTokens[state.setTokens.length - 1];
                 state.setTokens.length -= 1;
                 break;
             }

contracts/core/lib/ERC20Wrapper.sol → contracts/lib/ERC20Wrapper.sol

@@ -16,8 +16,8 @@
 
 pragma solidity 0.4.24;
 
-import { IERC20 } from "../../lib/IERC20.sol";
-import { CommonMath } from "../../lib/CommonMath.sol";
+import { CommonMath } from "./CommonMath.sol";
+import { IERC20 } from "./IERC20.sol";
 
 
 /**
@@ -107,23 +107,6 @@ library ERC20Wrapper {
         );
     }
 
-    function ensureAllowance(
-        address _token,
-        address _owner,
-        address _spender,
-        uint256 _quantity
-    )
-        private
-    {
-        if (allowance(_token, _owner, _spender) < _quantity) {
-            approve(
-                _token,
-                _spender,
-                CommonMath.maxUInt256()
-            );
-        }
-    }
-
     // ============ Private Functions ============
 
     /**

contracts/lib/IERC20.sol

@@ -57,5 +57,6 @@ interface IERC20 {
         address _spender,
         uint256 _quantity
     )
-        external;
+        external
+        returns (bool);
 }

contracts/mocks/lib/ERC20WrapperMock.sol

@@ -0,0 +1,28 @@
+pragma solidity 0.4.24;
+pragma experimental "ABIEncoderV2";
+
+import { ERC20Wrapper } from "../../lib/ERC20Wrapper.sol";
+
+// Mock contract implementation of OrderLibrary functions
+contract ERC20WrapperMock {
+    function allowance(
+        address _token,
+        address _owner,
+        address _spender
+    )
+        public
+        returns (uint256)
+    {
+        return ERC20Wrapper.allowance(_token, _owner, _spender);
+    }
+
+    function approve(
+        address _token,
+        address _spender,
+        uint256 _quantity
+    )
+        public
+    {
+        ERC20Wrapper.approve(_token, _spender, _quantity);
+    }
+}

test/contracts/core/transferProxy.spec.ts

@@ -29,12 +29,17 @@ BigNumberSetup.configure();
 ChaiSetup.configure();
 const { expect, assert } = chai;
 
-import { assertTokenBalance, expectRevertError } from "../../utils/tokenAssertions";
+import {
+  assertTokenBalance,
+  expectRevertError
+} from "../../utils/tokenAssertions";
+
 import {
   DEPLOYED_TOKEN_QUANTITY,
   UNLIMITED_ALLOWANCE_IN_BASE_UNITS,
 } from "../../utils/constants";
 
+
 contract("TransferProxy", (accounts) => {
   const [
     ownerAccount,

test/contracts/lib/commonMath.spec.ts

@@ -10,8 +10,8 @@ import { Address } from "../../../types/common.js";
 // Contract types
 import { CommonMathMockContract } from "../../../types/generated/common_math_mock";
 
-// Artifacts
-const CommonMathMock = artifacts.require("CommonMathMock");
+// Wrappers
+import { LibraryMockWrapper } from "../../utils/libraryMockWrapper";
 
 // Testing Set up
 import { BigNumberSetup } from "../../utils/bigNumberSetup";
@@ -20,23 +20,18 @@ BigNumberSetup.configure();
 ChaiSetup.configure();
 const { expect, assert } = chai;
 
+
 contract("CommonMathMock", (accounts) => {
   const [ownerAccount] = accounts;
+  const libraryMockWrapper = new LibraryMockWrapper(ownerAccount);
 
   let commonMathLibrary: CommonMathMockContract;
 
   describe("#testMaxUInt256", async () => {
     let caller: Address = ownerAccount;
 
     beforeEach(async () => {
-      const truffleCommonMathLibrary = await CommonMathMock.new(
-        { from: ownerAccount },
-      );
-
-      commonMathLibrary = new CommonMathMockContract(
-        web3.eth.contract(truffleCommonMathLibrary.abi).at(truffleCommonMathLibrary.address),
-        { from: ownerAccount },
-      );
+      commonMathLibrary = await libraryMockWrapper.deployCommonMathLibraryAsync()
     });
 
     async function subject(): Promise<BigNumber> {

test/contracts/lib/erc20Wrapper.spec.ts

@@ -0,0 +1,124 @@
+import * as chai from "chai";
+import * as _ from "lodash";
+
+import * as ABIDecoder from "abi-decoder";
+import { BigNumber } from "bignumber.js";
+
+// Types
+import { Address } from "../../../types/common.js";
+
+// Contract types
+import { InvalidReturnTokenMockContract } from "../../../types/generated/invalid_return_token_mock";
+import { ERC20WrapperMockContract } from "../../../types/generated/e_r_c20_wrapper_mock";
+import { StandardTokenMockContract } from "../../../types/generated/standard_token_mock";
+
+// Wrappers
+import { LibraryMockWrapper } from "../../utils/libraryMockWrapper";
+import { ERC20Wrapper } from "../../utils/erc20Wrapper";
+
+// Testing Set up
+import { BigNumberSetup } from "../../utils/bigNumberSetup";
+import ChaiSetup from "../../utils/chaiSetup";
+BigNumberSetup.configure();
+ChaiSetup.configure();
+const { expect, assert } = chai;
+
+import {
+  assertTokenBalance,
+  expectRevertError
+} from "../../utils/tokenAssertions";
+
+import {
+  UNLIMITED_ALLOWANCE_IN_BASE_UNITS,
+  ZERO,
+} from "../../utils/constants";
+
+
+contract("ERC20WrapperMock", (accounts) => {
+  const [
+    deployerAccount,
+    ownerAccount,
+    spenderAccount
+  ] = accounts;
+
+  let erc20WrapperLibrary: ERC20WrapperMockContract;
+
+  const erc20Wrapper = new ERC20Wrapper(deployerAccount);
+  const libraryMockWrapper = new LibraryMockWrapper(deployerAccount);
+
+  describe("#approve", async () => {
+    const subjectTransferAllowance: BigNumber = UNLIMITED_ALLOWANCE_IN_BASE_UNITS;
+    let subjectTokenAddress: Address;
+    let token: StandardTokenMockContract;
+    let caller: Address = ownerAccount;
+
+    beforeEach(async () => {
+      token = await erc20Wrapper.deployTokenAsync(ownerAccount);
+      subjectTokenAddress = token.address
+
+      erc20WrapperLibrary = await libraryMockWrapper.deployERC20WrapperLibraryAsync();
+    });
+
+    async function subject(): Promise<string> {
+      return erc20WrapperLibrary.approve.sendTransactionAsync(
+        subjectTokenAddress,
+        spenderAccount,
+        subjectTransferAllowance,
+        { from: ownerAccount },
+      );
+    }
+
+    it("approves the spender on behalf of the calling contract", async () => {
+      const existingAllowance = await token.allowance.callAsync(
+        erc20WrapperLibrary.address,
+        spenderAccount
+      );
+
+      await subject();
+
+      const expectedNewAllowance = existingAllowance.add(subjectTransferAllowance);
+      const newSpenderAllowance = await token.allowance.callAsync(
+        erc20WrapperLibrary.address,
+        spenderAccount
+      );
+      expect(newSpenderAllowance).to.bignumber.equal(subjectTransferAllowance);
+    });
+
+    describe("#when the token has an invalid return", async () => {
+      let invalidReturnToken: InvalidReturnTokenMockContract;
+
+      beforeEach(async () => {
+        invalidReturnToken = await erc20Wrapper.deployTokenInvalidReturnAsync(ownerAccount);
+        subjectTokenAddress = invalidReturnToken.address;
+      });
+
+      it("should revert", async () => {
+        await expectRevertError(subject());
+      });
+    });
+  });
+
+  describe("#allowance", async () => {
+    let token: StandardTokenMockContract;
+    let caller: Address = ownerAccount;
+
+    beforeEach(async () => {
+      token = await erc20Wrapper.deployTokenAsync(ownerAccount);
+      erc20WrapperLibrary = await libraryMockWrapper.deployERC20WrapperLibraryAsync();
+    });
+
+    async function subject(): Promise<BigNumber> {
+      return erc20WrapperLibrary.allowance.callAsync(
+        token.address,
+        ownerAccount,
+        spenderAccount,
+        { from: ownerAccount },
+      );
+    }
+
+    it("approves the spender on behalf of the calling contract", async () => {
+      const allowance = await subject();
+      expect(allowance).to.bignumber.equal(ZERO);
+    });
+  });
+});

test/utils/libraryMockWrapper.ts

@@ -0,0 +1,48 @@
+import * as _ from "lodash";
+
+import { CommonMathMockContract } from "../../types/generated/common_math_mock";
+import { ERC20WrapperMockContract } from "../../types/generated/e_r_c20_wrapper_mock";
+
+import { BigNumber } from "bignumber.js";
+import { Address } from "../../types/common.js";
+import { DEFAULT_GAS } from "../utils/constants";
+
+const ERC20WrapperMock = artifacts.require("ERC20WrapperMock");
+const CommonMathMock = artifacts.require("CommonMathMock");
+
+
+export class LibraryMockWrapper {
+  private _contractOwnerAddress: Address;
+
+  constructor(contractOwnerAddress: Address) {
+    this._contractOwnerAddress = contractOwnerAddress;
+  }
+
+  /* ============ Deployment ============ */
+
+  public async deployCommonMathLibraryAsync(
+    from: Address = this._contractOwnerAddress
+  ): Promise<CommonMathMockContract> {
+    const truffleCommonMathLibrary = await CommonMathMock.new(
+      { from },
+    );
+
+    return new CommonMathMockContract(
+      web3.eth.contract(truffleCommonMathLibrary.abi).at(truffleCommonMathLibrary.address),
+      { from },
+    );
+  }
+
+  public async deployERC20WrapperLibraryAsync(
+    from: Address = this._contractOwnerAddress
+  ): Promise<ERC20WrapperMockContract> {
+    const erc20WrapperMockContract = await ERC20WrapperMock.new(
+      { from },
+    );
+
+    return new ERC20WrapperMockContract(
+      web3.eth.contract(erc20WrapperMockContract.abi).at(erc20WrapperMockContract.address),
+      { from },
+    );
+  }
+}