diff --git a/.solcover.js b/.solcover.js index 94ce572e0..bf2f0fd06 100644 --- a/.solcover.js +++ b/.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 ], }; diff --git a/.soliumignore b/.soliumignore index b73203abe..5945ea697 100644 --- a/.soliumignore +++ b/.soliumignore @@ -1,4 +1,4 @@ node_modules -contracts/lib contracts/test -contracts/Migrations.sol \ No newline at end of file +contracts/Migrations.sol +contracts/external \ No newline at end of file diff --git a/contracts/core/TransferProxy.sol b/contracts/core/TransferProxy.sol index b01dc8ce6..28c7c89f3 100644 --- a/contracts/core/TransferProxy.sol +++ b/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)); } } diff --git a/contracts/core/exchange-wrappers/ZeroExExchangeWrapper.sol b/contracts/core/exchange-wrappers/ZeroExExchangeWrapper.sol index 494640ba6..b5cb0ef09 100644 --- a/contracts/core/exchange-wrappers/ZeroExExchangeWrapper.sol +++ b/contracts/core/exchange-wrappers/ZeroExExchangeWrapper.sol @@ -82,6 +82,7 @@ contract ZeroExExchangeWrapper /* ============ Getters ============ */ /* ============ Private ============ */ + function fillZeroExOrder( bytes _zeroExOrderData ) diff --git a/contracts/core/exchange-wrappers/lib/ZeroExOrderDataHandler.sol b/contracts/core/exchange-wrappers/lib/ZeroExOrderDataHandler.sol index e6318d843..5ce318392 100644 --- a/contracts/core/exchange-wrappers/lib/ZeroExOrderDataHandler.sol +++ b/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)); diff --git a/contracts/core/extensions/CoreAccounting.sol b/contracts/core/extensions/CoreAccounting.sol index ab50565c0..c396e6782 100644 --- a/contracts/core/extensions/CoreAccounting.sol +++ b/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 diff --git a/contracts/core/extensions/CoreIssuance.sol b/contracts/core/extensions/CoreIssuance.sol index 45bb0ae26..6dbe69c48 100644 --- a/contracts/core/extensions/CoreIssuance.sol +++ b/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 diff --git a/contracts/core/interfaces/ITransferProxy.sol b/contracts/core/interfaces/ITransferProxy.sol index 26d3f7b85..453606317 100644 --- a/contracts/core/interfaces/ITransferProxy.sol +++ b/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; } diff --git a/test/core/extensions/coreAccounting.spec.ts b/test/core/extensions/coreAccounting.spec.ts index d785d573c..3f8ec5e07 100644 --- a/test/core/extensions/coreAccounting.spec.ts +++ b/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); }); diff --git a/test/core/extensions/coreInternal.spec.ts b/test/core/extensions/coreInternal.spec.ts index 9110eeda7..23accdd7e 100644 --- a/test/core/extensions/coreInternal.spec.ts +++ b/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); diff --git a/test/core/extensions/coreIssuance.spec.ts b/test/core/extensions/coreIssuance.spec.ts index f59b23004..8af30371a 100644 --- a/test/core/extensions/coreIssuance.spec.ts +++ b/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); }); diff --git a/test/core/extensions/coreIssuanceOrder.spec.ts b/test/core/extensions/coreIssuanceOrder.spec.ts index afe7b84be..b7da3a080 100644 --- a/test/core/extensions/coreIssuanceOrder.spec.ts +++ b/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); }); diff --git a/test/core/external/lib/mockZeroExOrderDataHandlerLibrary.spec.ts b/test/core/external/lib/mockZeroExOrderDataHandlerLibrary.spec.ts index 13fe906d4..a4408377b 100644 --- a/test/core/external/lib/mockZeroExOrderDataHandlerLibrary.spec.ts +++ b/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,14 +216,14 @@ contract("MockZeroExOrderDataHandlerLibrary", (accounts) => { }); describe("#parseERC20TokenAddress", async () => { - let subjectOrderData: Bytes32; + let subjectAssetData: Bytes32; beforeEach(async () => { - subjectOrderData = makerAssetData; + subjectAssetData = makerAssetData; }); async function subject(): Promise { - return zeroExExchangeWrapper.parseERC20TokenAddress.callAsync(subjectOrderData); + return zeroExExchangeWrapper.parseERC20TokenAddress.callAsync(subjectAssetData); } it("should correctly parse the maker token address", async () => { @@ -228,5 +231,14 @@ contract("MockZeroExOrderDataHandlerLibrary", (accounts) => { 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()); + }); + }); }); }); diff --git a/test/core/transferProxy.spec.ts b/test/core/transferProxy.spec.ts index ca15792db..b124a0b8f 100644 --- a/test/core/transferProxy.spec.ts +++ b/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 { - 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 }, ); } diff --git a/test/utils/coreWrapper.ts b/test/utils/coreWrapper.ts index ae57810ea..700aba940 100644 --- a/test/utils/coreWrapper.ts +++ b/test/utils/coreWrapper.ts @@ -36,7 +36,6 @@ export class CoreWrapper { /* ============ Deployment ============ */ public async deployTransferProxyAsync( - vaultAddress: Address, from: Address = this._tokenOwnerAddress ): Promise { const truffleTransferProxy = await TransferProxy.new( @@ -48,12 +47,6 @@ export class CoreWrapper { { from, gas: DEFAULT_GAS }, ); - // Set TransferProxy dependencies - await transferProxy.setVaultAddress.sendTransactionAsync( - vaultAddress, - { from }, - ); - return transferProxy; }