merge development

7 files changed, 364 insertions, 24 deletions

diff --git a/packages/contracts/test/exchange/internal.ts b/packages/contracts/test/exchange/internal.ts
index dc2c5fbe0..156e086af 100644
--- a/packages/contracts/test/exchange/internal.ts
+++ b/packages/contracts/test/exchange/internal.ts
@@ -6,10 +6,7 @@ import * as _ from 'lodash';
 
 import { TestExchangeInternalsContract } from '../../generated_contract_wrappers/test_exchange_internals';
 import { artifacts } from '../utils/artifacts';
-import {
-    getInvalidOpcodeErrorMessageForCallAsync,
-    getRevertReasonOrErrorMessageForSendTransactionAsync,
-} from '../utils/assertions';
+import { getRevertReasonOrErrorMessageForSendTransactionAsync } from '../utils/assertions';
 import { chaiSetup } from '../utils/chai_setup';
 import { bytes32Values, testCombinatoriallyWithReferenceFuncAsync, uint256Values } from '../utils/combinatorial_utils';
 import { constants } from '../utils/constants';
diff --git a/packages/contracts/test/exchange/match_orders.ts b/packages/contracts/test/exchange/match_orders.ts
index 554c456cc..c6e7b494f 100644
--- a/packages/contracts/test/exchange/match_orders.ts
+++ b/packages/contracts/test/exchange/match_orders.ts
@@ -406,6 +406,100 @@ describe('matchOrders', () => {
             );
         });
 
+        it('Should give right maker and right taker a favorable fee price when rounding', async () => {
+            // Create orders to match
+            const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
+                makerAddress: makerAddressLeft,
+                makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(16), 0),
+                takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(22), 0),
+                feeRecipientAddress: feeRecipientAddressLeft,
+            });
+            const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
+                makerAddress: makerAddressRight,
+                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
+                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
+                makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(83), 0),
+                takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(49), 0),
+                feeRecipientAddress: feeRecipientAddressRight,
+                makerFee: Web3Wrapper.toBaseUnitAmount(new BigNumber(10000), 0),
+                takerFee: Web3Wrapper.toBaseUnitAmount(new BigNumber(10000), 0),
+            });
+            // Note:
+            //  The maker/taker fee percentage paid on the right order differs because
+            //  they received different sale prices. The right maker pays a
+            //  fee slightly lower than the right taker.
+            const expectedTransferAmounts = {
+                // Left Maker
+                amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(16), 0),
+                amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(22), 0),
+                feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100%
+                // Right Maker
+                amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(22), 0),
+                amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(13), 0),
+                feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(2650), 0), // 2650.6 rounded down tro 2650
+                // Taker
+                amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(3), 0),
+                feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100%
+                feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(2653), 0), // 2653.1 rounded down to 2653
+            };
+            // Match signedOrderLeft with signedOrderRight
+            await matchOrderTester.matchOrdersAndAssertEffectsAsync(
+                signedOrderLeft,
+                signedOrderRight,
+                takerAddress,
+                erc20BalancesByOwner,
+                erc721TokenIdsByOwner,
+                expectedTransferAmounts,
+            );
+        });
+
+        it('Should give left maker and left taker a favorable fee price when rounding', async () => {
+            // Create orders to match
+            const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
+                makerAddress: makerAddressLeft,
+                makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(12), 0),
+                takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(97), 0),
+                feeRecipientAddress: feeRecipientAddressLeft,
+                makerFee: Web3Wrapper.toBaseUnitAmount(new BigNumber(10000), 0),
+                takerFee: Web3Wrapper.toBaseUnitAmount(new BigNumber(10000), 0),
+            });
+            const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
+                makerAddress: makerAddressRight,
+                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
+                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
+                makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(89), 0),
+                takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(1), 0),
+                feeRecipientAddress: feeRecipientAddressRight,
+            });
+            // Note:
+            //  The maker/taker fee percentage paid on the left order differs because
+            //  they received different sale prices. The left maker pays a
+            //  fee slightly lower than the left taker.
+            const expectedTransferAmounts = {
+                // Left Maker
+                amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(11), 0),
+                amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(89), 0),
+                feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(9166), 0), // 9166.6 rounded down to 9166
+                // Right Maker
+                amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(89), 0),
+                amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(1), 0),
+                feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100%
+                // Taker
+                amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 0),
+                feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(9175), 0), // 9175.2 rounded down to 9175
+                feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100%
+            };
+            // Match signedOrderLeft with signedOrderRight
+            await matchOrderTester.matchOrdersAndAssertEffectsAsync(
+                signedOrderLeft,
+                signedOrderRight,
+                takerAddress,
+                erc20BalancesByOwner,
+                erc721TokenIdsByOwner,
+                expectedTransferAmounts,
+            );
+        });
+
         it('Should transfer correct amounts when right order fill amount deviates from amount derived by `Exchange.fillOrder`', async () => {
             // Create orders to match
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
diff --git a/packages/contracts/test/exchange/signature_validator.ts b/packages/contracts/test/exchange/signature_validator.ts
index b25483c4b..5cc62e777 100644
--- a/packages/contracts/test/exchange/signature_validator.ts
+++ b/packages/contracts/test/exchange/signature_validator.ts
@@ -14,7 +14,7 @@ import { ValidatorContract } from '../../generated_contract_wrappers/validator';
 import { WalletContract } from '../../generated_contract_wrappers/wallet';
 import { addressUtils } from '../utils/address_utils';
 import { artifacts } from '../utils/artifacts';
-import { expectContractCallFailedAsync, expectContractCallFailedWithoutReasonAsync } from '../utils/assertions';
+import { expectContractCallFailedAsync } from '../utils/assertions';
 import { chaiSetup } from '../utils/chai_setup';
 import { constants } from '../utils/constants';
 import { LogDecoder } from '../utils/log_decoder';
diff --git a/packages/contracts/test/extensions/forwarder.ts b/packages/contracts/test/extensions/forwarder.ts
index 18101d684..8424d01fd 100644
--- a/packages/contracts/test/extensions/forwarder.ts
+++ b/packages/contracts/test/extensions/forwarder.ts
@@ -12,7 +12,11 @@ import { ExchangeContract } from '../../generated_contract_wrappers/exchange';
 import { ForwarderContract } from '../../generated_contract_wrappers/forwarder';
 import { WETH9Contract } from '../../generated_contract_wrappers/weth9';
 import { artifacts } from '../utils/artifacts';
-import { expectTransactionFailedAsync } from '../utils/assertions';
+import {
+    expectContractCreationFailedAsync,
+    expectTransactionFailedAsync,
+    sendTransactionResult,
+} from '../utils/assertions';
 import { chaiSetup } from '../utils/chai_setup';
 import { constants } from '../utils/constants';
 import { ERC20Wrapper } from '../utils/erc20_wrapper';
@@ -37,6 +41,7 @@ describe(ContractName.Forwarder, () => {
     let otherAddress: string;
     let defaultMakerAssetAddress: string;
     let zrxAssetData: string;
+    let wethAssetData: string;
 
     let weth: DummyERC20TokenContract;
     let zrxToken: DummyERC20TokenContract;
@@ -90,7 +95,7 @@ describe(ContractName.Forwarder, () => {
         weth = new DummyERC20TokenContract(wethContract.abi, wethContract.address, provider);
         erc20Wrapper.addDummyTokenContract(weth);
 
-        const wethAssetData = assetDataUtils.encodeERC20AssetData(wethContract.address);
+        wethAssetData = assetDataUtils.encodeERC20AssetData(wethContract.address);
         zrxAssetData = assetDataUtils.encodeERC20AssetData(zrxToken.address);
         const exchangeInstance = await ExchangeContract.deployFrom0xArtifactAsync(
             artifacts.Exchange,
@@ -98,8 +103,7 @@ describe(ContractName.Forwarder, () => {
             txDefaults,
             zrxAssetData,
         );
-        const exchangeContract = new ExchangeContract(exchangeInstance.abi, exchangeInstance.address, provider);
-        exchangeWrapper = new ExchangeWrapper(exchangeContract, provider);
+        exchangeWrapper = new ExchangeWrapper(exchangeInstance, provider);
         await exchangeWrapper.registerAssetProxyAsync(erc20Proxy.address, owner);
         await exchangeWrapper.registerAssetProxyAsync(erc721Proxy.address, owner);
 
@@ -162,6 +166,27 @@ describe(ContractName.Forwarder, () => {
         await blockchainLifecycle.revertAsync();
     });
 
+    describe('constructor', () => {
+        it('should revert if assetProxy is unregistered', async () => {
+            const exchangeInstance = await ExchangeContract.deployFrom0xArtifactAsync(
+                artifacts.Exchange,
+                provider,
+                txDefaults,
+                zrxAssetData,
+            );
+            return expectContractCreationFailedAsync(
+                (ForwarderContract.deployFrom0xArtifactAsync(
+                    artifacts.Forwarder,
+                    provider,
+                    txDefaults,
+                    exchangeInstance.address,
+                    zrxAssetData,
+                    wethAssetData,
+                ) as any) as sendTransactionResult,
+                RevertReason.UnregisteredAssetProxy,
+            );
+        });
+    });
     describe('marketSellOrdersWithEth without extra fees', () => {
         it('should fill a single order', async () => {
             const ordersWithoutFee = [orderWithoutFee];
diff --git a/packages/contracts/test/multisig/asset_proxy_owner.ts b/packages/contracts/test/multisig/asset_proxy_owner.ts
index bb2b3b1a3..299707512 100644
--- a/packages/contracts/test/multisig/asset_proxy_owner.ts
+++ b/packages/contracts/test/multisig/asset_proxy_owner.ts
@@ -34,6 +34,7 @@ const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper);
 describe('AssetProxyOwner', () => {
     let owners: string[];
     let authorized: string;
+    let notOwner: string;
     const REQUIRED_APPROVALS = new BigNumber(2);
     const SECONDS_TIME_LOCKED = new BigNumber(1000000);
 
@@ -51,7 +52,9 @@ describe('AssetProxyOwner', () => {
     before(async () => {
         const accounts = await web3Wrapper.getAvailableAddressesAsync();
         owners = [accounts[0], accounts[1]];
-        const initialOwner = (authorized = accounts[0]);
+        authorized = accounts[2];
+        notOwner = accounts[3];
+        const initialOwner = accounts[0];
         erc20Proxy = await MixinAuthorizableContract.deployFrom0xArtifactAsync(
             artifacts.MixinAuthorizable,
             provider,
@@ -269,8 +272,12 @@ describe('AssetProxyOwner', () => {
             await multiSigWrapper.confirmTransactionAsync(erc721AddAuthorizedAddressTxId, owners[1]);
             await increaseTimeAndMineBlockAsync(SECONDS_TIME_LOCKED.toNumber());
             await multiSigWrapper.executeTransactionAsync(registerAssetProxyTxId, owners[0]);
-            await multiSigWrapper.executeTransactionAsync(erc20AddAuthorizedAddressTxId, owners[0]);
-            await multiSigWrapper.executeTransactionAsync(erc721AddAuthorizedAddressTxId, owners[0]);
+            await multiSigWrapper.executeTransactionAsync(erc20AddAuthorizedAddressTxId, owners[0], {
+                gas: constants.MAX_EXECUTE_TRANSACTION_GAS,
+            });
+            await multiSigWrapper.executeTransactionAsync(erc721AddAuthorizedAddressTxId, owners[0], {
+                gas: constants.MAX_EXECUTE_TRANSACTION_GAS,
+            });
         });
 
         describe('validRemoveAuthorizedAddressAtIndexTx', () => {
@@ -342,10 +349,11 @@ describe('AssetProxyOwner', () => {
                 const log = res.logs[0] as LogWithDecodedArgs<AssetProxyOwnerSubmissionEventArgs>;
                 const txId = log.args.transactionId;
 
-                return expectTransactionFailedWithoutReasonAsync(
+                return expectTransactionFailedAsync(
                     testAssetProxyOwner.executeRemoveAuthorizedAddressAtIndex.sendTransactionAsync(txId, {
                         from: owners[1],
                     }),
+                    RevertReason.TxNotFullyConfirmed,
                 );
             });
 
@@ -395,7 +403,10 @@ describe('AssetProxyOwner', () => {
                 );
             });
 
-            it('should execute removeAuthorizedAddressAtIndex for registered address if fully confirmed', async () => {
+            it('should execute removeAuthorizedAddressAtIndex for registered address if fully confirmed and called by owner', async () => {
+                const isAuthorizedBefore = await erc20Proxy.authorized.callAsync(authorized);
+                expect(isAuthorizedBefore).to.equal(true);
+
                 const removeAuthorizedAddressAtIndexData = erc20Proxy.removeAuthorizedAddressAtIndex.getABIEncodedTransactionData(
                     authorized,
                     erc20Index,
@@ -418,8 +429,38 @@ describe('AssetProxyOwner', () => {
                 const isExecuted = tx[3];
                 expect(isExecuted).to.equal(true);
 
-                const isAuthorized = await erc20Proxy.authorized.callAsync(authorized);
-                expect(isAuthorized).to.equal(false);
+                const isAuthorizedAfter = await erc20Proxy.authorized.callAsync(authorized);
+                expect(isAuthorizedAfter).to.equal(false);
+            });
+
+            it('should execute removeAuthorizedAddressAtIndex for registered address if fully confirmed and called by non-owner', async () => {
+                const isAuthorizedBefore = await erc20Proxy.authorized.callAsync(authorized);
+                expect(isAuthorizedBefore).to.equal(true);
+
+                const removeAuthorizedAddressAtIndexData = erc20Proxy.removeAuthorizedAddressAtIndex.getABIEncodedTransactionData(
+                    authorized,
+                    erc20Index,
+                );
+                const submitRes = await multiSigWrapper.submitTransactionAsync(
+                    erc20Proxy.address,
+                    removeAuthorizedAddressAtIndexData,
+                    owners[0],
+                );
+                const submitLog = submitRes.logs[0] as LogWithDecodedArgs<AssetProxyOwnerSubmissionEventArgs>;
+                const txId = submitLog.args.transactionId;
+
+                await multiSigWrapper.confirmTransactionAsync(txId, owners[1]);
+
+                const execRes = await multiSigWrapper.executeRemoveAuthorizedAddressAtIndexAsync(txId, notOwner);
+                const execLog = execRes.logs[1] as LogWithDecodedArgs<AssetProxyOwnerExecutionEventArgs>;
+                expect(execLog.args.transactionId).to.be.bignumber.equal(txId);
+
+                const tx = await testAssetProxyOwner.transactions.callAsync(txId);
+                const isExecuted = tx[3];
+                expect(isExecuted).to.equal(true);
+
+                const isAuthorizedAfter = await erc20Proxy.authorized.callAsync(authorized);
+                expect(isAuthorizedAfter).to.equal(false);
             });
 
             it('should throw if already executed', async () => {
diff --git a/packages/contracts/test/multisig/multi_sig_with_time_lock.ts b/packages/contracts/test/multisig/multi_sig_with_time_lock.ts
index 8eeeeca6b..bc1de7ed4 100644
--- a/packages/contracts/test/multisig/multi_sig_with_time_lock.ts
+++ b/packages/contracts/test/multisig/multi_sig_with_time_lock.ts
@@ -1,14 +1,21 @@
 import { BlockchainLifecycle } from '@0xproject/dev-utils';
+import { RevertReason } from '@0xproject/types';
 import { BigNumber } from '@0xproject/utils';
 import * as chai from 'chai';
 import { LogWithDecodedArgs } from 'ethereum-types';
+import * as _ from 'lodash';
 
+import { DummyERC20TokenContract } from '../../generated_contract_wrappers/dummy_erc20_token';
 import {
+    MultiSigWalletWithTimeLockConfirmationEventArgs,
+    MultiSigWalletWithTimeLockConfirmationTimeSetEventArgs,
     MultiSigWalletWithTimeLockContract,
+    MultiSigWalletWithTimeLockExecutionEventArgs,
+    MultiSigWalletWithTimeLockExecutionFailureEventArgs,
     MultiSigWalletWithTimeLockSubmissionEventArgs,
 } from '../../generated_contract_wrappers/multi_sig_wallet_with_time_lock';
 import { artifacts } from '../utils/artifacts';
-import { expectTransactionFailedWithoutReasonAsync } from '../utils/assertions';
+import { expectTransactionFailedAsync, expectTransactionFailedWithoutReasonAsync } from '../utils/assertions';
 import { increaseTimeAndMineBlockAsync } from '../utils/block_timestamp';
 import { chaiSetup } from '../utils/chai_setup';
 import { constants } from '../utils/constants';
@@ -21,6 +28,7 @@ const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper);
 // tslint:disable:no-unnecessary-type-assertion
 describe('MultiSigWalletWithTimeLock', () => {
     let owners: string[];
+    let notOwner: string;
     const REQUIRED_APPROVALS = new BigNumber(2);
     const SECONDS_TIME_LOCKED = new BigNumber(1000000);
 
@@ -32,7 +40,8 @@ describe('MultiSigWalletWithTimeLock', () => {
     });
     before(async () => {
         const accounts = await web3Wrapper.getAvailableAddressesAsync();
-        owners = [accounts[0], accounts[1]];
+        owners = [accounts[0], accounts[1], accounts[2]];
+        notOwner = accounts[3];
     });
 
     let multiSig: MultiSigWalletWithTimeLockContract;
@@ -45,6 +54,171 @@ describe('MultiSigWalletWithTimeLock', () => {
         await blockchainLifecycle.revertAsync();
     });
 
+    describe('external_call', () => {
+        it('should be internal', async () => {
+            const secondsTimeLocked = new BigNumber(0);
+            multiSig = await MultiSigWalletWithTimeLockContract.deployFrom0xArtifactAsync(
+                artifacts.MultiSigWalletWithTimeLock,
+                provider,
+                txDefaults,
+                owners,
+                REQUIRED_APPROVALS,
+                secondsTimeLocked,
+            );
+            expect(_.isUndefined((multiSig as any).external_call)).to.be.equal(true);
+        });
+    });
+    describe('confirmTransaction', () => {
+        let txId: BigNumber;
+        beforeEach(async () => {
+            const secondsTimeLocked = new BigNumber(0);
+            multiSig = await MultiSigWalletWithTimeLockContract.deployFrom0xArtifactAsync(
+                artifacts.MultiSigWalletWithTimeLock,
+                provider,
+                txDefaults,
+                owners,
+                REQUIRED_APPROVALS,
+                secondsTimeLocked,
+            );
+            multiSigWrapper = new MultiSigWrapper(multiSig, provider);
+            const destination = notOwner;
+            const data = constants.NULL_BYTES;
+            const txReceipt = await multiSigWrapper.submitTransactionAsync(destination, data, owners[0]);
+            txId = (txReceipt.logs[0] as LogWithDecodedArgs<MultiSigWalletWithTimeLockSubmissionEventArgs>).args
+                .transactionId;
+        });
+        it('should revert if called by a non-owner', async () => {
+            await expectTransactionFailedWithoutReasonAsync(multiSigWrapper.confirmTransactionAsync(txId, notOwner));
+        });
+        it('should revert if transaction does not exist', async () => {
+            const nonexistentTxId = new BigNumber(123456789);
+            await expectTransactionFailedWithoutReasonAsync(
+                multiSigWrapper.confirmTransactionAsync(nonexistentTxId, owners[1]),
+            );
+        });
+        it('should revert if transaction is already confirmed by caller', async () => {
+            await expectTransactionFailedWithoutReasonAsync(multiSigWrapper.confirmTransactionAsync(txId, owners[0]));
+        });
+        it('should confirm transaction for caller and log a Confirmation event', async () => {
+            const txReceipt = await multiSigWrapper.confirmTransactionAsync(txId, owners[1]);
+            const log = txReceipt.logs[0] as LogWithDecodedArgs<MultiSigWalletWithTimeLockConfirmationEventArgs>;
+            expect(log.event).to.be.equal('Confirmation');
+            expect(log.args.sender).to.be.equal(owners[1]);
+            expect(log.args.transactionId).to.be.bignumber.equal(txId);
+        });
+        it('should revert if fully confirmed', async () => {
+            await multiSigWrapper.confirmTransactionAsync(txId, owners[1]);
+            await expectTransactionFailedAsync(
+                multiSigWrapper.confirmTransactionAsync(txId, owners[2]),
+                RevertReason.TxFullyConfirmed,
+            );
+        });
+        it('should set the confirmation time of the transaction if it becomes fully confirmed', async () => {
+            const blockNum = await web3Wrapper.getBlockNumberAsync();
+            const timestamp = new BigNumber(await web3Wrapper.getBlockTimestampAsync(blockNum));
+            const txReceipt = await multiSigWrapper.confirmTransactionAsync(txId, owners[1]);
+            const log = txReceipt.logs[1] as LogWithDecodedArgs<MultiSigWalletWithTimeLockConfirmationTimeSetEventArgs>;
+            expect(log.args.confirmationTime).to.be.bignumber.equal(timestamp);
+            expect(log.args.transactionId).to.be.bignumber.equal(txId);
+        });
+    });
+    describe('executeTransaction', () => {
+        let txId: BigNumber;
+        const secondsTimeLocked = new BigNumber(1000000);
+        beforeEach(async () => {
+            multiSig = await MultiSigWalletWithTimeLockContract.deployFrom0xArtifactAsync(
+                artifacts.MultiSigWalletWithTimeLock,
+                provider,
+                txDefaults,
+                owners,
+                REQUIRED_APPROVALS,
+                secondsTimeLocked,
+            );
+            multiSigWrapper = new MultiSigWrapper(multiSig, provider);
+            const destination = notOwner;
+            const data = constants.NULL_BYTES;
+            const txReceipt = await multiSigWrapper.submitTransactionAsync(destination, data, owners[0]);
+            txId = (txReceipt.logs[0] as LogWithDecodedArgs<MultiSigWalletWithTimeLockSubmissionEventArgs>).args
+                .transactionId;
+        });
+        it('should revert if transaction has not been fully confirmed', async () => {
+            await increaseTimeAndMineBlockAsync(secondsTimeLocked.toNumber());
+            await expectTransactionFailedAsync(
+                multiSigWrapper.executeTransactionAsync(txId, owners[1]),
+                RevertReason.TxNotFullyConfirmed,
+            );
+        });
+        it('should revert if time lock has not passed', async () => {
+            await multiSigWrapper.confirmTransactionAsync(txId, owners[1]);
+            await expectTransactionFailedAsync(
+                multiSigWrapper.executeTransactionAsync(txId, owners[1]),
+                RevertReason.TimeLockIncomplete,
+            );
+        });
+        it('should execute a transaction and log an Execution event if successful and called by owner', async () => {
+            await multiSigWrapper.confirmTransactionAsync(txId, owners[1]);
+            await increaseTimeAndMineBlockAsync(secondsTimeLocked.toNumber());
+            const txReceipt = await multiSigWrapper.executeTransactionAsync(txId, owners[1]);
+            const log = txReceipt.logs[0] as LogWithDecodedArgs<MultiSigWalletWithTimeLockExecutionEventArgs>;
+            expect(log.event).to.be.equal('Execution');
+            expect(log.args.transactionId).to.be.bignumber.equal(txId);
+        });
+        it('should execute a transaction and log an Execution event if successful and called by non-owner', async () => {
+            await multiSigWrapper.confirmTransactionAsync(txId, owners[1]);
+            await increaseTimeAndMineBlockAsync(secondsTimeLocked.toNumber());
+            const txReceipt = await multiSigWrapper.executeTransactionAsync(txId, notOwner);
+            const log = txReceipt.logs[0] as LogWithDecodedArgs<MultiSigWalletWithTimeLockExecutionEventArgs>;
+            expect(log.event).to.be.equal('Execution');
+            expect(log.args.transactionId).to.be.bignumber.equal(txId);
+        });
+        it('should revert if a required confirmation is revoked before executeTransaction is called', async () => {
+            await multiSigWrapper.confirmTransactionAsync(txId, owners[1]);
+            await increaseTimeAndMineBlockAsync(secondsTimeLocked.toNumber());
+            await multiSigWrapper.revokeConfirmationAsync(txId, owners[0]);
+            await expectTransactionFailedAsync(
+                multiSigWrapper.executeTransactionAsync(txId, owners[1]),
+                RevertReason.TxNotFullyConfirmed,
+            );
+        });
+        it('should revert if transaction has been executed', async () => {
+            await multiSigWrapper.confirmTransactionAsync(txId, owners[1]);
+            await increaseTimeAndMineBlockAsync(secondsTimeLocked.toNumber());
+            const txReceipt = await multiSigWrapper.executeTransactionAsync(txId, owners[1]);
+            const log = txReceipt.logs[0] as LogWithDecodedArgs<MultiSigWalletWithTimeLockExecutionEventArgs>;
+            expect(log.args.transactionId).to.be.bignumber.equal(txId);
+            await expectTransactionFailedWithoutReasonAsync(multiSigWrapper.executeTransactionAsync(txId, owners[1]));
+        });
+        it("should log an ExecutionFailure event and not update the transaction's execution state if unsuccessful", async () => {
+            const contractWithoutFallback = await DummyERC20TokenContract.deployFrom0xArtifactAsync(
+                artifacts.DummyERC20Token,
+                provider,
+                txDefaults,
+                constants.DUMMY_TOKEN_NAME,
+                constants.DUMMY_TOKEN_SYMBOL,
+                constants.DUMMY_TOKEN_DECIMALS,
+                constants.DUMMY_TOKEN_TOTAL_SUPPLY,
+            );
+            const data = constants.NULL_BYTES;
+            const value = new BigNumber(10);
+            const submissionTxReceipt = await multiSigWrapper.submitTransactionAsync(
+                contractWithoutFallback.address,
+                data,
+                owners[0],
+                { value },
+            );
+            const newTxId = (submissionTxReceipt.logs[0] as LogWithDecodedArgs<
+                MultiSigWalletWithTimeLockSubmissionEventArgs
+            >).args.transactionId;
+            await multiSigWrapper.confirmTransactionAsync(newTxId, owners[1]);
+            await increaseTimeAndMineBlockAsync(secondsTimeLocked.toNumber());
+            const txReceipt = await multiSigWrapper.executeTransactionAsync(newTxId, owners[1]);
+            const executionFailureLog = txReceipt.logs[0] as LogWithDecodedArgs<
+                MultiSigWalletWithTimeLockExecutionFailureEventArgs
+            >;
+            expect(executionFailureLog.event).to.be.equal('ExecutionFailure');
+            expect(executionFailureLog.args.transactionId).to.be.bignumber.equal(newTxId);
+        });
+    });
     describe('changeTimeLock', () => {
         describe('initially non-time-locked', async () => {
             before(async () => {
@@ -78,8 +252,9 @@ describe('MultiSigWalletWithTimeLock', () => {
                 const res = await multiSigWrapper.submitTransactionAsync(destination, changeTimeLockData, owners[0]);
                 const log = res.logs[0] as LogWithDecodedArgs<MultiSigWalletWithTimeLockSubmissionEventArgs>;
                 const txId = log.args.transactionId;
-                return expectTransactionFailedWithoutReasonAsync(
+                return expectTransactionFailedAsync(
                     multiSig.executeTransaction.sendTransactionAsync(txId, { from: owners[0] }),
+                    RevertReason.TxNotFullyConfirmed,
                 );
             });
 
@@ -147,8 +322,9 @@ describe('MultiSigWalletWithTimeLock', () => {
             });
 
             it('should throw if it has enough confirmations but is not past the time lock', async () => {
-                return expectTransactionFailedWithoutReasonAsync(
+                return expectTransactionFailedAsync(
                     multiSig.executeTransaction.sendTransactionAsync(txId, { from: owners[0] }),
+                    RevertReason.TimeLockIncomplete,
                 );
             });
 
diff --git a/packages/contracts/test/utils/multi_sig_wrapper.ts b/packages/contracts/test/utils/multi_sig_wrapper.ts
index e0c27b839..e12a58695 100644
--- a/packages/contracts/test/utils/multi_sig_wrapper.ts
+++ b/packages/contracts/test/utils/multi_sig_wrapper.ts
@@ -6,7 +6,6 @@ import * as _ from 'lodash';
 import { AssetProxyOwnerContract } from '../../generated_contract_wrappers/asset_proxy_owner';
 import { MultiSigWalletContract } from '../../generated_contract_wrappers/multi_sig_wallet';
 
-import { constants } from './constants';
 import { LogDecoder } from './log_decoder';
 
 export class MultiSigWrapper {
@@ -36,10 +35,19 @@ export class MultiSigWrapper {
         const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash);
         return tx;
     }
-    public async executeTransactionAsync(txId: BigNumber, from: string): Promise<TransactionReceiptWithDecodedLogs> {
+    public async revokeConfirmationAsync(txId: BigNumber, from: string): Promise<TransactionReceiptWithDecodedLogs> {
+        const txHash = await this._multiSig.revokeConfirmation.sendTransactionAsync(txId, { from });
+        const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash);
+        return tx;
+    }
+    public async executeTransactionAsync(
+        txId: BigNumber,
+        from: string,
+        opts: { gas?: number } = {},
+    ): Promise<TransactionReceiptWithDecodedLogs> {
         const txHash = await this._multiSig.executeTransaction.sendTransactionAsync(txId, {
             from,
-            gas: constants.MAX_EXECUTE_TRANSACTION_GAS,
+            gas: opts.gas,
         });
         const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash);
         return tx;
@@ -52,7 +60,6 @@ export class MultiSigWrapper {
         const txHash = await (this
             ._multiSig as AssetProxyOwnerContract).executeRemoveAuthorizedAddressAtIndex.sendTransactionAsync(txId, {
             from,
-            gas: constants.MAX_EXECUTE_TRANSACTION_GAS,
         });
         const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash);
         return tx;