Alex/generalize transfer proxy

.solcover.js

@@ -5,6 +5,7 @@ module.exports = {
     'Migrations.sol',
     'test',
     'external',
-    'core/lib/ERC20Wrapper.sol' // TODO: Find a cleaner way to test state mutating functions, we will skip
+    '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
   ],
 };

.soliumignore

@@ -1,4 +1,4 @@
 node_modules
-contracts/lib
 contracts/test
-contracts/Migrations.sol
+contracts/Migrations.sol
+contracts/external

contracts/core/TransferProxy.sol

@@ -26,8 +26,8 @@ import { ERC20Wrapper } from "./lib/ERC20Wrapper.sol";
  * @title TransferProxy
  * @author Set Protocol
  *
- * The proxy contract is responsible for transferring funds from the user to the vault during Set issuance.
- * The contract is separated to allow for upgrades, particularly if new token standards emerge or upgrades are required.
+ * The proxy contract is responsible for updating token balances to assist with issuance
+ * and filling issuance orders.
  */
 
 contract TransferProxy is
@@ -36,69 +36,49 @@ contract TransferProxy is
     // Use SafeMath library for all uint256 arithmetic
     using SafeMath for uint256;
 
-    /* ============ State Variables ============ */
-
-    // Address of the Vault contract
-    address public vaultAddress;
-
     /* ============ No Constructor ============ */
 
-    /* ============ Setter Functions ============ */
-
-    /**
-     * Set vaultAddress. Can only be set by owner of TransferProxy.
-     *
-     * @param  _vaultAddress   The address of the Vault
-     */
-
-    function setVaultAddress(
-        address _vaultAddress
-    )
-        external
-        onlyOwner
-    {
-        // Commit passed address to vaultAddress state variable
-        vaultAddress = _vaultAddress;
-    }
-
-    /* ============ Public Functions ============ */
+    /* ============ External Functions ============ */
 
     /**
-     * Transfers tokens from an address (that has set allowance on the proxy) to the vault.
+     * Transfers tokens from an address (that has set allowance on the proxy).
      * Can only be called by authorized core contracts.
      *
-     * @param  _from           The address to transfer tokens from
      * @param  _tokenAddress   The address of the ERC20 token
      * @param  _quantity       The number of tokens to transfer
+     * @param  _from           The address to transfer from
+     * @param  _to             The address to transfer to
      */
-    function transferToVault(
-        address _from,
+    function transfer(
         address _tokenAddress,
-        uint _quantity
+        uint _quantity,
+        address _from,
+        address _to
     )
         external
         onlyAuthorized
     {
-        // Retrieve current balance of token for the vault
-        uint existingVaultBalance = ERC20Wrapper.balanceOf(
+        // Retrieve current balance of token for the receiver
+        uint existingBalance = ERC20Wrapper.balanceOf(
             _tokenAddress,
-            vaultAddress
+            _to
         );
 
-        // Call specified ERC20 contract to transfer tokens from user to Vault (via proxy).
-
+        // Call specified ERC20 contract to transfer tokens (via proxy).
         ERC20Wrapper.transferFrom(
             _tokenAddress,
             _from,
-            vaultAddress,
+            _to,
             _quantity
         );
 
-        // Verify transfer quantity is reflected in balance
-        uint newVaultBalance = ERC20Wrapper.balanceOf(
+        // Get new balance of transferred token for receiver
+        uint newBalance = ERC20Wrapper.balanceOf(
             _tokenAddress,
-            vaultAddress
+            _to
         );
-        require(newVaultBalance == existingVaultBalance.add(_quantity));
+
+        // Verify transfer quantity is reflected in balance
+        require(newBalance == existingBalance.add(_quantity));
     }
 }

contracts/core/exchange-wrappers/ZeroExExchangeWrapper.sol

@@ -82,6 +82,7 @@ contract ZeroExExchangeWrapper
     /* ============ Getters ============ */
 
     /* ============ Private ============ */
+
     function fillZeroExOrder(
         bytes _zeroExOrderData
     )

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

@@ -32,6 +32,12 @@ library ZeroExOrderDataHandler {
     using SafeMath for uint256;
     using LibBytes for bytes;
 
+    // ============ Constants ============
+
+    bytes4 constant ERC20_SELECTOR = bytes4(keccak256("ERC20Token(address)"));
+
+    string constant INVALID_TOKEN_ADDRESS = 'Address is not for ERC20 asset.';
+
     // ============ Structs ============
 
     struct ZeroExHeader {
@@ -160,11 +166,11 @@ library ZeroExOrderDataHandler {
         // ** - Maker Asset Data Length
         // *** - Taker Asset Data Length
         assembly {
-            mstore(order,           mload(orderDataAddr))  // maker
+            mstore(order,           mload(orderDataAddr))           // maker
             mstore(add(order, 32),  mload(add(orderDataAddr, 32)))  // taker
             mstore(add(order, 64),  mload(add(orderDataAddr, 64)))  // feeRecipient
             mstore(add(order, 96),  mload(add(orderDataAddr, 96)))  // senderAddress
-            mstore(add(order, 128),  mload(add(orderDataAddr, 128))) // makerAssetAmount
+            mstore(add(order, 128), mload(add(orderDataAddr, 128))) // makerAssetAmount
             mstore(add(order, 160), mload(add(orderDataAddr, 160))) // takerAssetAmount
             mstore(add(order, 192), mload(add(orderDataAddr, 192))) // makerFee
             mstore(add(order, 224), mload(add(orderDataAddr, 224))) // takerFee
@@ -202,11 +208,12 @@ library ZeroExOrderDataHandler {
         pure
         returns(address)
     {
-        bytes4 ERC20_SELECTOR = bytes4(keccak256("ERC20Token(address)"));
         // Ensure that the asset is ERC20
-        bytes4 orderProxyId = _assetData.readBytes4(0);
-
-        require(ERC20_SELECTOR == orderProxyId);
+        bytes4 assetType = _assetData.readBytes4(0);
+        require(
+            ERC20_SELECTOR == assetType,
+            INVALID_TOKEN_ADDRESS
+        );
 
         address tokenAddress = address(_assetData.readBytes32(4));
 

contracts/core/extensions/CoreAccounting.sol

@@ -130,10 +130,11 @@ contract CoreAccounting is
         isPositiveQuantity(_quantity)
     {
         // Call TransferProxy contract to transfer user tokens to Vault
-        ITransferProxy(state.transferProxyAddress).transferToVault(
-            msg.sender,
+        ITransferProxy(state.transferProxyAddress).transfer(
             _tokenAddress,
-            _quantity
+            _quantity,
+            msg.sender,
+            state.vaultAddress
         );
 
         // Call Vault contract to attribute deposited tokens to user

contracts/core/extensions/CoreIssuance.sol

@@ -259,10 +259,11 @@ contract CoreIssuance is
                 uint amountToDeposit = requiredComponentQuantity.sub(vaultBalance);
 
                 // Transfer the remainder component quantity required to vault
-                ITransferProxy(state.transferProxyAddress).transferToVault(
-                    _owner,
+                ITransferProxy(state.transferProxyAddress).transfer(
                     component,
-                    requiredComponentQuantity.sub(vaultBalance)
+                    requiredComponentQuantity.sub(vaultBalance),
+                    _owner,
+                    state.vaultAddress
                 );
 
                 // Log transfer of component from issuer waller

contracts/core/interfaces/ITransferProxy.sol

@@ -26,17 +26,19 @@ pragma solidity 0.4.24;
 interface ITransferProxy {
 
     /**
-     * Transfers tokens from an address (that has set allowance on the proxy) to the vault.
+     * Transfers tokens from an address (that has set allowance on the proxy).
      * Can only be called by authorized core contracts.
      *
-     * @param  _from           The address to transfer tokens from
      * @param  _tokenAddress   The address of the ERC20 token
      * @param  _quantity       The number of tokens to transfer
+     * @param  _from           The address to transfer from
+     * @param  _to             The address to transfer to
      */
-    function transferToVault(
-        address _from,
+    function transfer(
         address _tokenAddress,
-        uint _quantity
+        uint _quantity,
+        address _from,
+        address _to
     )
         external;
 }

test/core/extensions/coreAccounting.spec.ts

@@ -57,7 +57,7 @@ contract("CoreAccounting", (accounts) => {
   beforeEach(async () => {
     core = await coreWrapper.deployCoreAsync();
     vault = await coreWrapper.deployVaultAsync();
-    transferProxy = await coreWrapper.deployTransferProxyAsync(vault.address);
+    transferProxy = await coreWrapper.deployTransferProxyAsync();
     setTokenFactory = await coreWrapper.deploySetTokenFactoryAsync();
     await coreWrapper.setDefaultStateAndAuthorizationsAsync(core, vault, transferProxy, setTokenFactory);
   });

test/core/extensions/coreInternal.spec.ts

@@ -104,7 +104,7 @@ contract("CoreInternal", (accounts) => {
 
     beforeEach(async () => {
       vault = await coreWrapper.deployVaultAsync();
-      transferProxy = await coreWrapper.deployTransferProxyAsync(vault.address);
+      transferProxy = await coreWrapper.deployTransferProxyAsync();
 
       subjectCaller = ownerAccount;
     });
@@ -209,7 +209,7 @@ contract("CoreInternal", (accounts) => {
 
     beforeEach(async () => {
       vault = await coreWrapper.deployVaultAsync();
-      transferProxy = await coreWrapper.deployTransferProxyAsync(vault.address);
+      transferProxy = await coreWrapper.deployTransferProxyAsync();
       setTokenFactory = await coreWrapper.deploySetTokenFactoryAsync();
       await coreWrapper.setDefaultStateAndAuthorizationsAsync(core, vault, transferProxy, setTokenFactory);
 

test/core/extensions/coreIssuance.spec.ts

@@ -78,7 +78,7 @@ contract("CoreIssuance", (accounts) => {
   beforeEach(async () => {
     core = await coreWrapper.deployCoreAsync();
     vault = await coreWrapper.deployVaultAsync();
-    transferProxy = await coreWrapper.deployTransferProxyAsync(vault.address);
+    transferProxy = await coreWrapper.deployTransferProxyAsync();
     setTokenFactory = await coreWrapper.deploySetTokenFactoryAsync();
     await coreWrapper.setDefaultStateAndAuthorizationsAsync(core, vault, transferProxy, setTokenFactory);
   });

test/core/extensions/coreIssuanceOrder.spec.ts

@@ -80,7 +80,7 @@ contract("CoreIssuanceOrder", (accounts) => {
   beforeEach(async () => {
     core = await coreWrapper.deployCoreAsync();
     vault = await coreWrapper.deployVaultAsync();
-    transferProxy = await coreWrapper.deployTransferProxyAsync(vault.address);
+    transferProxy = await coreWrapper.deployTransferProxyAsync();
     setTokenFactory = await coreWrapper.deploySetTokenFactoryAsync();
     await coreWrapper.setDefaultStateAndAuthorizationsAsync(core, vault, transferProxy, setTokenFactory);
   });

test/core/external/lib/mockZeroExOrderDataHandlerLibrary.spec.ts

@@ -31,6 +31,10 @@ BigNumberSetup.configure();
 ChaiSetup.configure();
 const { expect, assert } = chai;
 
+import {
+  expectRevertError,
+} from "../../../utils/tokenAssertions";
+
 import {
   DEFAULT_GAS,
 } from "../../../utils/constants";
@@ -95,7 +99,6 @@ contract("MockZeroExOrderDataHandlerLibrary", (accounts) => {
     let makerAssetDataLength: BigNumber;
     let takerAssetDataLength: BigNumber;
 
-
     let subjectOrderData: Bytes32;
 
     beforeEach(async () => {
@@ -213,20 +216,29 @@ contract("MockZeroExOrderDataHandlerLibrary", (accounts) => {
   });
 
   describe("#parseERC20TokenAddress", async () => {
-    let subjectOrderData: Bytes32;
+    let subjectAssetData: Bytes32;
 
     beforeEach(async () => {
-      subjectOrderData = makerAssetData;
+      subjectAssetData = makerAssetData;
     });
 
     async function subject(): Promise<any> {
-      return zeroExExchangeWrapper.parseERC20TokenAddress.callAsync(subjectOrderData);
+      return zeroExExchangeWrapper.parseERC20TokenAddress.callAsync(subjectAssetData);
     }
 
     it("should correctly parse the maker token address", async () => {
       const makerTokenAddressResult = await subject();
       expect(makerTokenAddressResult).to.equal(makerTokenAddress);
     });
 
+    describe("when the asset type for the token is not ERC20", async () => {
+      beforeEach(async () => {
+        subjectAssetData = '0xInvalidAssetSelector';
+      });
+
+      it("should revert", async () => {
+        await expectRevertError(subject());
+      });
+    });
   });
 });

test/core/transferProxy.spec.ts

@@ -58,41 +58,7 @@ contract("TransferProxy", (accounts) => {
     ABIDecoder.removeABI(TransferProxy.abi);
   });
 
-  describe("#setVaultAddress", async () => {
-    // Setup
-    beforeEach(async () => {
-      transferProxy = await coreWrapper.deployTransferProxyAsync(vaultAccount);
-    });
-
-    // Subject
-    let caller: Address = ownerAccount;
-
-    async function subject(): Promise<string> {
-      return transferProxy.setVaultAddress.sendTransactionAsync(
-        vaultAccount,
-        { from: caller },
-      );
-    }
-
-    it("sets vault address correctly", async () => {
-      await subject();
-
-      const storedVaultAddress = await transferProxy.vaultAddress.callAsync();
-      expect(storedVaultAddress).to.eql(vaultAccount);
-    });
-
-    describe("when the caller is not the owner of the contract", async () => {
-      before(async () => {
-        caller = otherAccount;
-      });
-
-      it("should revert", async () => {
-        await expectRevertError(subject());
-      });
-    });
-  });
-
-  describe("#transferToVault", async () => {
+  describe("#transfer", async () => {
     // Setup
     let approver: Address = ownerAccount;
     let authorizedContract: Address = authorizedAccount;
@@ -101,7 +67,7 @@ contract("TransferProxy", (accounts) => {
     let tokenAddress: Address;
 
     beforeEach(async () => {
-      transferProxy = await coreWrapper.deployTransferProxyAsync(vaultAccount);
+      transferProxy = await coreWrapper.deployTransferProxyAsync();
       await coreWrapper.addAuthorizationAsync(transferProxy, authorizedContract);
       mockToken = await erc20Wrapper.deployTokenAsync(ownerAccount);
       await erc20Wrapper.approveTransferAsync(mockToken, transferProxy.address, approver);
@@ -118,10 +84,11 @@ contract("TransferProxy", (accounts) => {
       // Initialize tokenToTransfer to deployed token's address unless tokenAddress is overwritten in test cases
       const tokenToTransfer = tokenAddress || mockToken.address;
 
-      return transferProxy.transferToVault.sendTransactionAsync(
-        subjectCaller,
+      return transferProxy.transfer.sendTransactionAsync(
         tokenToTransfer,
         amountToTransfer,
+        subjectCaller,
+        vaultAccount,
         { from: authorizedContract },
       );
     }