@0x:contracts-exchange Addressed review comments by completely overhauling transaction_unit_tests.ts

contracts/exchange/contracts/test/TestTransactions.sol

@@ -19,65 +19,26 @@
 pragma solidity ^0.5.9;
 pragma experimental ABIEncoderV2;
 
+import "@0x/contracts-exchange-libs/contracts/src/LibZeroExTransaction.sol";
 import "../src/Exchange.sol";
-import "../src/MixinTransactions.sol";
 
 
 contract TestTransactions is
     Exchange
 {
-    // Indicates whether or not the fallback function should succeed.
+    event ExecutableCalled(bytes data, bytes returnData);
-    bool public shouldSucceedCall;
-
-    // The returndata of the fallback function.
-    bytes public fallbackReturnData;
 
     constructor ()
         public
         Exchange(1337)
     {} // solhint-disable-line no-empty-blocks
 
-    // This fallback function will succeed if the bool `shouldSucceedCall` has been set
-    // to true, and will fail otherwise. It will return returndata `fallbackReturnData`
-    // in either case.
-    function ()
-        external
-    {
-        // Circumvent the compiler to return data through the fallback
-        bool success = shouldSucceedCall;
-        bytes memory returnData = fallbackReturnData;
-        assembly {
-            if or(iszero(success), gt(calldatasize, 0x0)) {
-                revert(add(0x20, returnData), mload(returnData))
-            }
-            return(add(0x20, returnData), mload(returnData))
-        }
-    }
-
     function setCurrentContextAddress(address context)
         external
     {
         currentContextAddress = context;
     }
 
-    function setFallbackReturnData(bytes calldata returnData)
-        external
-    {
-        fallbackReturnData = returnData;
-    }
-
-    function setShouldBeValid(bool isValid)
-        external
-    {
-        shouldBeValid = isValid;
-    }
-
-    function setShouldCallSucceed(bool shouldSucceed)
-        external
-    {
-        shouldSucceedCall = shouldSucceed;
-    }
-
     function setTransactionHash(bytes32 hash)
         external
     {
@@ -92,8 +53,29 @@ contract TestTransactions is
         return _getCurrentContextAddress();
     }
 
+    // This function will execute arbitrary calldata via a delegatecall. This is highly unsafe to use in production, and this
+    // is only meant to be used during testing.
+    function executable(
+        bool shouldSucceed,
+        bytes memory data,
+        bytes memory returnData
+    )
+        public
+        returns (bytes memory)
+    {
+        emit ExecutableCalled(data, returnData);
+        require(shouldSucceed, "EXECUTABLE_FAILED");
+        if (data.length != 0) {
+            (bool didSucceed, bytes memory callResultData) = address(this).delegatecall(data); // This is a delegatecall to preserve the `msg.sender` field
+            if (!didSucceed) {
+                assembly { revert(add(callResultData, 0x20), mload(callResultData)) }
+            }
+        }
+        return returnData;
+    }
+
     function _isValidTransactionWithHashSignature(
-        ZeroExTransaction memory,
+        LibZeroExTransaction.ZeroExTransaction memory,
         bytes32,
         address,
         bytes memory signature

contracts/exchange/test/transactions_unit_tests.ts

@@ -8,8 +8,8 @@ import {
     LogDecoder,
 } from '@0x/contracts-test-utils';
 import { ExchangeRevertErrors, transactionHashUtils } from '@0x/order-utils';
-import { EIP712DomainWithDefaultSchema } from '@0x/types';
+import { EIP712DomainWithDefaultSchema, ZeroExTransaction } from '@0x/types';
-import { BigNumber } from '@0x/utils';
+import { AbiEncoder, BigNumber, StringRevertError } from '@0x/utils';
 import { LogWithDecodedArgs } from 'ethereum-types';
 import * as _ from 'lodash';
 
@@ -56,19 +56,37 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef
         };
     });
 
+    /**
+     * Generates calldata for a call to `executable()` in the `TestTransactions` contract.
+     */
+    function getExecutableCallData(shouldSucceed: boolean, callData: string, returnData: string): string {
+        return (transactionsContract as any).executable.getABIEncodedTransactionData(
+            shouldSucceed,
+            callData,
+            returnData,
+        );
+    }
+
     describe('batchExecuteTransaction', () => {
         it('should revert if the only call to executeTransaction fails', async () => {
+            // Create an expired transaction that will fail when used to call `batchExecuteTransactions()`.
             const currentTimestamp = await getLatestBlockTimestampAsync();
             const transaction = {
                 ...EMPTY_ZERO_EX_TRANSACTION,
                 expirationTimeSeconds: new BigNumber(currentTimestamp).minus(10), // Set the expiration time to before the current timestamp
+                data: getExecutableCallData(false, constants.NULL_BYTES, constants.NULL_BYTES),
                 domain,
             };
             const transactionHash = transactionHashUtils.getTransactionHashHex(transaction);
+
+            // We expect a `TransactionError` to be returned because that is the error that will be triggered in the call to
+            // `executeTransaction`.
             const expectedError = new ExchangeRevertErrors.TransactionError(
                 ExchangeRevertErrors.TransactionErrorCode.Expired,
                 transactionHash,
             );
+
+            // Call the `batchExecuteTransactions()` function and ensure that it reverts with the expected revert error.
             const tx = transactionsContract.batchExecuteTransactions.sendTransactionAsync(
                 [transaction],
                 [randomSignature()],
@@ -77,30 +95,32 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef
         });
 
         it('should revert if the second call to executeTransaction fails', async () => {
-            // Set the contract to accept signatures.
+            // Create a transaction that will succeed when used to call `batchExecuteTransactions()`.
-            await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled('');
-
-            // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability.
-            await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(true)).to.be.fulfilled('');
-
             const currentTimestamp = await getLatestBlockTimestampAsync();
             const transaction1 = {
                 ...EMPTY_ZERO_EX_TRANSACTION,
-                data: '0x', // This call should succeed
+                data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), // This call should succeed
                 expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
                 domain,
             };
+
+            // Create a transaction that will fail when used to call `batchExecuteTransactions()` because the call to executable will fail.
             const transaction2 = {
                 ...EMPTY_ZERO_EX_TRANSACTION,
-                data: '0x32', // This call should fail because the calldata is invalid
+                data: getExecutableCallData(false, constants.NULL_BYTES, constants.NULL_BYTES), // This call should fail
                 expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
                 domain,
             };
             const transactionHash = transactionHashUtils.getTransactionHashHex(transaction2);
+
+            // Create the StringRevertError that reflects the returndata that will be returned by the failed transaction.
+            const executableError = new StringRevertError('EXECUTABLE_FAILED');
             const expectedError = new ExchangeRevertErrors.TransactionExecutionError(
                 transactionHash,
-                constants.NULL_BYTES,
+                executableError.encode(),
             );
+
+            // Call the `batchExecuteTransactions()` function and ensure that it reverts with the expected revert error.
             const tx = transactionsContract.batchExecuteTransactions.sendTransactionAsync(
                 [transaction1, transaction2],
                 [randomSignature(), randomSignature()],
@@ -109,30 +129,32 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef
         });
 
         it('should revert if the first call to executeTransaction fails', async () => {
-            // Set the contract to accept signatures.
+            // Create a transaction that will fail when used to call `batchExecuteTransactions()` because the call to executable will fail.
-            await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled('');
-
-            // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability.
-            await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(true)).to.be.fulfilled('');
-
             const currentTimestamp = await getLatestBlockTimestampAsync();
             const transaction1 = {
                 ...EMPTY_ZERO_EX_TRANSACTION,
-                data: '0x32', // This call should fail because the calldata is invalid
+                data: getExecutableCallData(false, constants.NULL_BYTES, constants.NULL_BYTES), // This call should fail
                 expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
                 domain,
             };
+
+            // Create a transaction that will succeed when used to call `batchExecuteTransactions()`.
             const transaction2 = {
                 ...EMPTY_ZERO_EX_TRANSACTION,
-                data: '0x', // This call should succeed
+                data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES),
                 expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
                 domain,
             };
             const transactionHash = transactionHashUtils.getTransactionHashHex(transaction1);
+
+            // Create the StringRevertError that reflects the returndata that will be returned by the failed transaction.
+            const executableError = new StringRevertError('EXECUTABLE_FAILED');
             const expectedError = new ExchangeRevertErrors.TransactionExecutionError(
                 transactionHash,
-                constants.NULL_BYTES,
+                executableError.encode(),
             );
+
+            // Call the `batchExecuteTransactions()` function and ensure that it reverts with the expected revert error.
             const tx = transactionsContract.batchExecuteTransactions.sendTransactionAsync(
                 [transaction1, transaction2],
                 [randomSignature(), randomSignature()],
@@ -141,34 +163,28 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef
         });
 
         it('should revert if the same transaction is executed twice in a batch', async () => {
-            // Set the contract to accept signatures.
+            // Create a transaction that will succeed when used to call `batchExecuteTransactions()`.
-            await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled('');
-
-            // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability.
-            await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(true)).to.be.fulfilled('');
-
-            // Set the contract fallbackReturnData to the recognizable string of bytes 0xDEADBEEF
-            await expect(transactionsContract.setFallbackReturnData.sendTransactionAsync('0xdeadbeef')).to.be.fulfilled(
-                '',
-            );
-
-            // Set up the necessary data for the transactions and tests
             const currentTimestamp = await getLatestBlockTimestampAsync();
             const transaction1 = {
                 ...EMPTY_ZERO_EX_TRANSACTION,
                 expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
-                signerAddress: accounts[1], // This is different than the account that will be used to send.
+                signerAddress: accounts[1],
+                data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES),
                 domain,
             };
+
+            // Duplicate the first transaction. This should cause the call to `batchExecuteTransactions()` to fail
+            // because this transaction will have the same order hash as transaction1.
             const transaction2 = {
                 ...EMPTY_ZERO_EX_TRANSACTION,
                 expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
-                signerAddress: accounts[1], // This is different than the account that will be used to send.
+                signerAddress: accounts[1],
+                data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES),
                 domain,
             };
             const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2);
 
-            // Verify that the transaction reverts
+            // Call the `batchExecuteTransactions()` function and ensure that it reverts with the expected revert error.
             const expectedError = new ExchangeRevertErrors.TransactionError(
                 ExchangeRevertErrors.TransactionErrorCode.AlreadyExecuted,
                 transactionHash2,
@@ -184,26 +200,18 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef
         });
 
         it('should succeed if the only call to executeTransaction succeeds', async () => {
-            // Set the contract to accept signatures.
-            await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled('');
-
-            // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability.
-            await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(true)).to.be.fulfilled('');
-
-            // Set the contract fallbackReturnData to the recognizable string of bytes 0xDEADBEEF
-            await expect(transactionsContract.setFallbackReturnData.sendTransactionAsync('0xdeadbeef')).to.be.fulfilled(
-                '',
-            );
-
-            // Set up the necessary data for the transactions and tests
             const currentTimestamp = await getLatestBlockTimestampAsync();
+
+            // Create a transaction that will succeed when used to call `batchExecuteTransactions()`.
             const transaction = {
                 ...EMPTY_ZERO_EX_TRANSACTION,
                 expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
-                signerAddress: accounts[1], // This is different than the account that will be used to send.
+                signerAddress: accounts[1],
+                data: getExecutableCallData(true, constants.NULL_BYTES, '0xdeadbeef'),
                 domain,
             };
             const transactionHash = transactionHashUtils.getTransactionHashHex(transaction);
+            const validSignature = randomSignature();
 
             // Verify that the returndata of the transaction is 0xDEADBEEF
             const result = await transactionsContract.batchExecuteTransactions.callAsync(
@@ -213,49 +221,58 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef
                     from: accounts[0],
                 },
             );
+
             expect(result.length).to.be.eq(1);
-            expect(result[0] === '0xdeadbeef').to.be.true();
+
+            // Create an abiEncoder for bytes. This will be used to decode the result and encode what
+            // is expected.
+            const abiEncoder = AbiEncoder.create('bytes');
+
+            // Ensure that the result contains the abi-encoded bytes "0xdeadbeef"
+            const encodedDeadbeef = abiEncoder.encode('0xdeadbeef');
+            expect(
+                result[0] ===
+                    '0x0000000000000000000000000000000000000000000000000000000000000020'.concat(
+                        encodedDeadbeef.slice(2, encodedDeadbeef.length),
+                    ),
+            ).to.be.true();
 
             // Verify that the logs returned from the call are correct.
             const receipt = await logDecoder.getTxWithDecodedLogsAsync(
                 await transactionsContract.batchExecuteTransactions.sendTransactionAsync(
                     [transaction],
-                    [randomSignature()],
+                    [validSignature],
-                    {
-                        from: accounts[0],
-                    },
                 ),
             );
+
+            // Ensure that the correct number of events were logged.
             const logs = receipt.logs as Array<LogWithDecodedArgs<TestTransactionsTransactionExecutionEventArgs>>;
-            expect(logs.length).to.be.eq(1);
+            expect(logs.length).to.be.eq(2);
-            expect(logs[0].event === 'TransactionExecution').to.be.true();
+
-            expect(logs[0].args.transactionHash).to.eq(transactionHash);
+            // Ensure that the correct events were logged.
+            expect(logs[0].event).to.be.eq('ExecutableCalled');
+            expect(logs[0].args.data).to.be.eq(constants.NULL_BYTES);
+            expect(logs[0].args.returnData).to.be.eq('0xdeadbeef');
+            expect(logs[1].event).to.be.eq('TransactionExecution');
+            expect(logs[1].args.transactionHash).to.eq(transactionHash);
         });
 
         it('should succeed if the both calls to executeTransaction succeed', async () => {
-            // Set the contract to accept signatures.
-            await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled('');
-
-            // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability.
-            await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(true)).to.be.fulfilled('');
-
-            // Set the contract fallbackReturnData to the recognizable string of bytes 0xDEADBEEF
-            await expect(transactionsContract.setFallbackReturnData.sendTransactionAsync('0xdeadbeef')).to.be.fulfilled(
-                '',
-            );
-
-            // Set up the necessary data for the transactions and tests
             const currentTimestamp = await getLatestBlockTimestampAsync();
+
+            // Create two transactions that will succeed when used to call `batchExecuteTransactions()`.
             const transaction1 = {
                 ...EMPTY_ZERO_EX_TRANSACTION,
                 expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
                 signerAddress: accounts[0], // This is different than the account that will be used to send.
+                data: getExecutableCallData(true, constants.NULL_BYTES, '0xdeadbeef'),
                 domain,
             };
             const transaction2 = {
                 ...EMPTY_ZERO_EX_TRANSACTION,
                 expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
-                signerAddress: accounts[1], // This is different than the account that will be used to send.
+                signerAddress: accounts[1], // Different than transaction1's signer address
+                data: getExecutableCallData(true, constants.NULL_BYTES, '0xbeefdead'),
                 domain,
             };
             const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1);
@@ -269,9 +286,30 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef
                     from: accounts[0],
                 },
             );
+
+            // Create an abiEncoder for bytes. This will be used to decode the result and encode what
+            // is expected.
+            const abiEncoder = AbiEncoder.create('bytes');
+
+            // Ensure that the result contains the abi-encoded bytes "0xdeadbeef"
+            const encodedDeadbeef = abiEncoder.encode('0xdeadbeef');
             expect(result.length).to.be.eq(2);
-            expect(result[0] === '0xdeadbeef').to.be.true();
+            expect(
-            expect(result[1] === '0xdeadbeef').to.be.true();
+                result[0] ===
+                    '0x0000000000000000000000000000000000000000000000000000000000000020'.concat(
+                        encodedDeadbeef.slice(2, encodedDeadbeef.length),
+                    ),
+            ).to.be.true();
+
+            // Ensure that the result contains the abi-encoded bytes "0xdeadbeef"
+            const encodedBeefdead = abiEncoder.encode('0xbeefdead');
+            expect(result.length).to.be.eq(2);
+            expect(
+                result[1] ===
+                    '0x0000000000000000000000000000000000000000000000000000000000000020'.concat(
+                        encodedBeefdead.slice(2, encodedBeefdead.length),
+                    ),
+            ).to.be.true();
 
             // Verify that the logs returned from the call are correct.
             const receipt = await logDecoder.getTxWithDecodedLogsAsync(
@@ -284,15 +322,32 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef
                 ),
             );
 
+            // Verify that the correct number of events were logged.
             const logs = receipt.logs as Array<LogWithDecodedArgs<TestTransactionsTransactionExecutionEventArgs>>;
-            expect(logs.length).to.be.eq(2);
+            expect(logs.length).to.be.eq(4);
-            logs.map(log => expect(log.event === 'TransactionExecution').to.be.true());
+
-            expect(logs[0].args.transactionHash).to.eq(transactionHash1);
+            // Ensure that the correct events were logged.
-            expect(logs[1].args.transactionHash).to.eq(transactionHash2);
+            expect(logs[0].event).to.be.eq('ExecutableCalled');
+            expect(logs[0].args.data).to.be.eq(constants.NULL_BYTES);
+            expect(logs[0].args.returnData).to.be.eq('0xdeadbeef');
+            expect(logs[1].event).to.be.eq('TransactionExecution');
+            expect(logs[1].args.transactionHash).to.eq(transactionHash1);
+            expect(logs[2].event).to.be.eq('ExecutableCalled');
+            expect(logs[2].args.data).to.be.eq(constants.NULL_BYTES);
+            expect(logs[2].args.returnData).to.be.eq('0xbeefdead');
+            expect(logs[3].event).to.be.eq('TransactionExecution');
+            expect(logs[3].args.transactionHash).to.eq(transactionHash2);
         });
     });
 
     describe('executeTransaction', () => {
+        function getExecuteTransactionCallData(transaction: ZeroExTransaction, signature: string): string {
+            return (transactionsContract as any).executeTransaction.getABIEncodedTransactionData(
+                transaction,
+                signature,
+            );
+        }
+
         it('should revert if the current time is past the expiration time', async () => {
             const currentTimestamp = await getLatestBlockTimestampAsync();
             const transaction = {
@@ -309,47 +364,190 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef
             return expect(tx).to.revertWith(expectedError);
         });
 
-        it('should revert if the current context address is not address zero', async () => {
+        // FIXME - This should be unskipped when the contracts have been updated to fix this problem.
-            // Set the current context address to a nonzero address before the call to `executeTransaction()`
+        it.skip('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender == signer for both calls', async () => {
-            expect(transactionsContract.setCurrentContextAddress.sendTransactionAsync(accounts[0])).to.be.fulfilled('');
+            const validSignature = randomSignature();
 
-            // Run the transaction with an updated current context address
             const currentTimestamp = await getLatestBlockTimestampAsync();
-            const transaction = {
+            const transaction1 = {
                 ...EMPTY_ZERO_EX_TRANSACTION,
                 expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
+                data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), // This should never get called
+                signerAddress: accounts[0],
                 domain,
             };
-            const transactionHash = transactionHashUtils.getTransactionHashHex(transaction);
+            const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1);
-            const expectedError = new ExchangeRevertErrors.TransactionError(
+
-                ExchangeRevertErrors.TransactionErrorCode.NoReentrancy,
+            const callData = getExecutableCallData(
-                transactionHash,
+                true,
+                getExecuteTransactionCallData(transaction1, validSignature),
+                '0xdeadbeef',
             );
-            const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature());
+
+            const transaction2 = {
+                ...EMPTY_ZERO_EX_TRANSACTION,
+                expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
+                data: callData,
+                signerAddress: accounts[0],
+                domain,
+            };
+            const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2);
+            const abiEncoder = AbiEncoder.createMethod('TransactionError', ['uint8', 'bytes32']);
+            const errorData = abiEncoder.encode([
+                ExchangeRevertErrors.TransactionErrorCode.NoReentrancy,
+                transactionHash1,
+            ]);
+
+            const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash2, errorData);
+            const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction2, validSignature, {
+                from: accounts[0],
+            });
+            return expect(tx).to.revertWith(expectedError);
+        });
+
+        it('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender != signer for both calls', async () => {
+            const validSignature = randomSignature();
+
+            const currentTimestamp = await getLatestBlockTimestampAsync();
+            const transaction1 = {
+                ...EMPTY_ZERO_EX_TRANSACTION,
+                expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
+                data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), // This should never get called
+                signerAddress: accounts[0],
+                domain,
+            };
+            const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1);
+
+            const callData = getExecutableCallData(
+                true,
+                getExecuteTransactionCallData(transaction1, validSignature),
+                '0xdeadbeef',
+            );
+
+            const transaction2 = {
+                ...EMPTY_ZERO_EX_TRANSACTION,
+                expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
+                data: callData,
+                signerAddress: accounts[0],
+                domain,
+            };
+            const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2);
+            const abiEncoder = AbiEncoder.createMethod('TransactionError', ['uint8', 'bytes32']);
+            const errorData = abiEncoder.encode([
+                ExchangeRevertErrors.TransactionErrorCode.NoReentrancy,
+                transactionHash1,
+            ]);
+
+            const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash2, errorData);
+            const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction2, validSignature, {
+                from: accounts[1], // Different then the signing addresses
+            });
+            return expect(tx).to.revertWith(expectedError);
+        });
+
+        it('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender != signer and then msg.sender == sender', async () => {
+            const validSignature = randomSignature();
+
+            const currentTimestamp = await getLatestBlockTimestampAsync();
+            const transaction1 = {
+                ...EMPTY_ZERO_EX_TRANSACTION,
+                expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
+                data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), // This should never get called
+                signerAddress: accounts[1],
+                domain,
+            };
+            const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1);
+
+            const callData = getExecutableCallData(
+                true,
+                getExecuteTransactionCallData(transaction1, validSignature),
+                '0xdeadbeef',
+            );
+
+            const transaction2 = {
+                ...EMPTY_ZERO_EX_TRANSACTION,
+                expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
+                data: callData,
+                signerAddress: accounts[0],
+                domain,
+            };
+            const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2);
+            const abiEncoder = AbiEncoder.createMethod('TransactionError', ['uint8', 'bytes32']);
+            const errorData = abiEncoder.encode([
+                ExchangeRevertErrors.TransactionErrorCode.NoReentrancy,
+                transactionHash1,
+            ]);
+
+            const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash2, errorData);
+            const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction2, validSignature, {
+                from: accounts[1], // Different then the signing addresses
+            });
+            return expect(tx).to.revertWith(expectedError);
+        });
+
+        // FIXME - This should be unskipped when the contracts have been updated to fix this problem.
+        it.skip('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender == signer and then msg.sender != sender', async () => {
+            const validSignature = randomSignature();
+
+            const currentTimestamp = await getLatestBlockTimestampAsync();
+            const transaction1 = {
+                ...EMPTY_ZERO_EX_TRANSACTION,
+                expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
+                data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), // This should never get called
+                signerAddress: accounts[0],
+                domain,
+            };
+            const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1);
+
+            const callData = getExecutableCallData(
+                true,
+                getExecuteTransactionCallData(transaction1, validSignature),
+                '0xdeadbeef',
+            );
+
+            const transaction2 = {
+                ...EMPTY_ZERO_EX_TRANSACTION,
+                expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
+                data: callData,
+                signerAddress: accounts[1],
+                domain,
+            };
+            const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2);
+            const abiEncoder = AbiEncoder.createMethod('TransactionError', ['uint8', 'bytes32']);
+            const errorData = abiEncoder.encode([
+                ExchangeRevertErrors.TransactionErrorCode.NoReentrancy,
+                transactionHash1,
+            ]);
+
+            const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash2, errorData);
+            const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction2, validSignature, {
+                from: accounts[1], // Different then the signing addresses
+            });
             return expect(tx).to.revertWith(expectedError);
         });
 
         it('should revert if the transaction has been executed previously', async () => {
+            const validSignature = randomSignature();
             const currentTimestamp = await getLatestBlockTimestampAsync();
             const transaction = {
                 ...EMPTY_ZERO_EX_TRANSACTION,
                 expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
+                data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES),
                 domain,
             };
             const transactionHash = transactionHashUtils.getTransactionHashHex(transaction);
 
-            // Make it seem like the transaction has already been executed by setting it's value in the transactionsExecuted
+            // Use the transaction in execute transaction.
-            // mapping to true.
+            await expect(
-            await expect(transactionsContract.setTransactionHash.sendTransactionAsync(transactionHash)).to.be.fulfilled(
+                transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature),
-                '',
+            ).to.be.fulfilled('');
-            );
 
-            // Run the transaction with an updated current context address
+            // Use the same transaction to make another call
             const expectedError = new ExchangeRevertErrors.TransactionError(
                 ExchangeRevertErrors.TransactionErrorCode.AlreadyExecuted,
                 transactionHash,
             );
-            const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature());
+            const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature);
             return expect(tx).to.revertWith(expectedError);
         });
 
@@ -375,17 +573,21 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef
         });
 
         it('should revert if the signer == msg.sender but the delegatecall fails', async () => {
+            // This calldata is encoded to fail when it hits the executable function.
+            const callData = getExecutableCallData(false, constants.NULL_BYTES, constants.NULL_BYTES);
             const currentTimestamp = await getLatestBlockTimestampAsync();
             const transaction = {
                 ...EMPTY_ZERO_EX_TRANSACTION,
                 expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
                 signerAddress: accounts[1],
+                data: callData,
                 domain,
             };
             const transactionHash = transactionHashUtils.getTransactionHashHex(transaction);
+            const executableError = new StringRevertError('EXECUTABLE_FAILED');
             const expectedError = new ExchangeRevertErrors.TransactionExecutionError(
                 transactionHash,
-                constants.NULL_BYTES,
+                executableError.encode(),
             );
             const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature(), {
                 from: accounts[1],
@@ -394,21 +596,22 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef
         });
 
         it('should revert if the signer != msg.sender and the signature is valid but the delegatecall fails', async () => {
-            // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability.
+            // This calldata is encoded to fail when it hits the executable function.
-            // await expect(transactionsContract.setShouldSucceedCall.sendTransactionAsync(false)).to.be.fulfilled('');
+            const callData = getExecutableCallData(false, constants.NULL_BYTES, constants.NULL_BYTES);
-
             const currentTimestamp = await getLatestBlockTimestampAsync();
             const transaction = {
                 ...EMPTY_ZERO_EX_TRANSACTION,
                 expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
                 signerAddress: accounts[1], // This is different than the account that will be used to send.
+                data: callData,
                 domain,
             };
             const validSignature = randomSignature(); // Valid because length != 2
             const transactionHash = transactionHashUtils.getTransactionHashHex(transaction);
+            const executableError = new StringRevertError('EXECUTABLE_FAILED');
             const expectedError = new ExchangeRevertErrors.TransactionExecutionError(
                 transactionHash,
-                constants.NULL_BYTES,
+                executableError.encode(),
             );
             const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature, {
                 from: accounts[0],
@@ -416,47 +619,16 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef
             return expect(tx).to.revertWith(expectedError);
         });
 
-        it('should revert with the correct return data if the signer != msg.sender and the signature is valid but the delegatecall fails', async () => {
-            // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability.
-            // await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(false)).to.be.fulfilled('');
-
-            // Set the contract fallbackReturnData to the recognizable string of bytes 0xDEADBEEF
-            await expect(transactionsContract.setFallbackReturnData.sendTransactionAsync('0xdeadbeef')).to.be.fulfilled(
-                '',
-            );
-
-            const currentTimestamp = await getLatestBlockTimestampAsync();
-            const validSignature = randomSignature(); // Valid because length != 2
-            const transaction = {
-                ...EMPTY_ZERO_EX_TRANSACTION,
-                expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
-                signerAddress: accounts[1], // This is different than the account that will be used to send.
-                domain,
-            };
-            const transactionHash = transactionHashUtils.getTransactionHashHex(transaction);
-            const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash, '0xdeadbeef');
-            const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature, {
-                from: accounts[0],
-            });
-            return expect(tx).to.revertWith(expectedError);
-        });
-
         it('should succeed with the correct return hash and event emitted', async () => {
-            // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability.
+            // This calldata is encoded to succeed when it hits the executable function.
-            await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(true)).to.be.fulfilled('');
+            const callData = getExecutableCallData(true, constants.NULL_BYTES, '0xdeadbeef');
-
-            // Set the contract fallbackReturnData to the recognizable string of bytes 0xDEADBEEF
-            await expect(transactionsContract.setFallbackReturnData.sendTransactionAsync('0xdeadbeef')).to.be.fulfilled(
-                '',
-            );
-
-            // Set up the necessary data for the transactions and tests
             const currentTimestamp = await getLatestBlockTimestampAsync();
             const validSignature = randomSignature(); // Valid because length != 2
             const transaction = {
                 ...EMPTY_ZERO_EX_TRANSACTION,
                 expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10),
                 signerAddress: accounts[1], // This is different than the account that will be used to send.
+                data: callData,
                 domain,
             };
             const transactionHash = transactionHashUtils.getTransactionHashHex(transaction);
@@ -465,18 +637,37 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef
             const result = await transactionsContract.executeTransaction.callAsync(transaction, validSignature, {
                 from: accounts[0],
             });
-            expect(result === '0xdeadbeef').to.be.true();
+
+            // Create an abiEncoder for bytes. This will be used to decode the result and encode what
+            // is expected.
+            const abiEncoder = AbiEncoder.create('bytes');
+
+            // Ensure that the result contains the abi-encoded bytes "0xdeadbeef"
+            const encodedDeadbeef = abiEncoder.encode('0xdeadbeef');
+            expect(
+                result ===
+                    '0x0000000000000000000000000000000000000000000000000000000000000020'.concat(
+                        encodedDeadbeef.slice(2, encodedDeadbeef.length),
+                    ),
+            ).to.be.true();
 
             // Verify that the logs returned from the call are correct.
             const receipt = await logDecoder.getTxWithDecodedLogsAsync(
-                await transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature(), {
+                await transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature, {
                     from: accounts[0],
                 }),
             );
+
+            // Ensure that the correct number of events were logged.
             const logs = receipt.logs as Array<LogWithDecodedArgs<TestTransactionsTransactionExecutionEventArgs>>;
-            expect(logs.length).to.be.eq(1);
+            expect(logs.length).to.be.eq(2);
-            expect(logs[0].event === 'TransactionExecution').to.be.true();
+
-            expect(logs[0].args.transactionHash).to.eq(transactionHash);
+            // Ensure that the correct events were logged.
+            expect(logs[0].event).to.be.eq('ExecutableCalled');
+            expect(logs[0].args.data).to.be.eq(constants.NULL_BYTES);
+            expect(logs[0].args.returnData).to.be.eq('0xdeadbeef');
+            expect(logs[1].event).to.be.eq('TransactionExecution');
+            expect(logs[1].args.transactionHash).to.eq(transactionHash);
         });
     });