Add tests and comments

5 files changed, 50 insertions, 31 deletions

diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol
index 97c1f1541..80120bc74 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol
@@ -18,7 +18,6 @@
 
 pragma solidity ^0.4.21;
 pragma experimental ABIEncoderV2;
-pragma experimental "v0.5.0";
 
 import "./mixins/MExchangeCore.sol";
 import "./mixins/MSettlement.sol";
@@ -117,13 +116,13 @@ contract MixinExchangeCore is
             require(isValidSignature(orderHash, order.makerAddress, signature));
         }
         
-        // Validate sender
+        // Validate sender is allowed to fill this order
         if (order.senderAddress != address(0)) {
             require(order.senderAddress == msg.sender);
         }
 
-        // Validate transaction signed by taker
-        address takerAddress = getSignerAddress();
+        // Validate taker is allowed to fill this order
+        address takerAddress = getCurrentContextAddress();
         if (order.takerAddress != address(0)) {
             require(order.takerAddress == takerAddress);
         }
@@ -177,13 +176,13 @@ contract MixinExchangeCore is
         require(order.makerAssetAmount > 0);
         require(order.takerAssetAmount > 0);
 
-        // Validate sender
+        // Validate sender is allowed to cancel this order
         if (order.senderAddress != address(0)) {
             require(order.senderAddress == msg.sender);
         }
         
         // Validate transaction signed by maker
-        address makerAddress = getSignerAddress();
+        address makerAddress = getCurrentContextAddress();
         require(order.makerAddress == makerAddress);
         
         if (block.timestamp >= order.expirationTimeSeconds) {
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol
index e4fc4767b..9edb1694f 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol
@@ -27,10 +27,11 @@ contract MixinTransactions is
 {
 
     // Mapping of transaction hash => executed
+    // This prevents transactions from being executed more than once.
     mapping (bytes32 => bool) public transactions;
 
     // Address of current transaction signer
-    address currentSigner;
+    address public currentContextAddress;
 
     /// @dev Executes an exchange method call in the context of signer.
     /// @param salt Arbitrary number to ensure uniqueness of transaction hash.
@@ -45,7 +46,7 @@ contract MixinTransactions is
         external
     {
         // Prevent reentrancy
-        require(currentSigner == address(0));
+        require(currentContextAddress == address(0));
 
         // Calculate transaction hash
         bytes32 transactionHash = keccak256(
@@ -63,7 +64,7 @@ contract MixinTransactions is
             require(isValidSignature(transactionHash, signer, signature));
 
             // Set the current transaction signer
-            currentSigner = signer;
+            currentContextAddress = signer;
         }
 
         // Execute transaction
@@ -71,15 +72,21 @@ contract MixinTransactions is
         require(address(this).delegatecall(data));
 
         // Reset current transaction signer
-        currentSigner = address(0);
+        // TODO: Check if gas is paid when currentContextAddress is already 0.
+        currentContextAddress = address(0);
     }
 
-    function getSignerAddress()
+    /// @dev The current function will be called in the context of this address (either 0x transaction signer or `msg.sender`).
+    ///      If calling a fill function, this address will represent the taker.
+    ///      If calling a cancel function, this address will represent the maker.
+    /// @return Signer of 0x transaction if entry point is `executeTransaction`.
+    ///         `msg.sender` if entry point is any other function.
+    function getCurrentContextAddress()
         internal
         view
         returns (address)
     {
-        address signerAddress = currentSigner == address(0) ? msg.sender : currentSigner;
-        return signerAddress;
+        address contextAddress = currentContextAddress == address(0) ? msg.sender : currentContextAddress;
+        return contextAddress;
     }
 }
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MTransactions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MTransactions.sol
index 31209de02..10bfcb035 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MTransactions.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MTransactions.sol
@@ -34,7 +34,12 @@ contract MTransactions is MSignatureValidator {
         bytes signature)
         external;
 
-    function getSignerAddress()
+    /// @dev The current function will be called in the context of this address (either 0x transaction signer or `msg.sender`).
+    ///      If calling a fill function, this address will represent the taker.
+    ///      If calling a cancel function, this address will represent the maker.
+    /// @return Signer of 0x transaction if entry point is `executeTransaction`.
+    ///         `msg.sender` if entry point is any other function.
+    function getCurrentContextAddress()
         internal
         view
         returns (address);
diff --git a/packages/contracts/src/utils/crypto.ts b/packages/contracts/src/utils/crypto.ts
index 85a37122a..222cb7cca 100644
--- a/packages/contracts/src/utils/crypto.ts
+++ b/packages/contracts/src/utils/crypto.ts
@@ -29,8 +29,6 @@ export const crypto = {
                 args[i] = new BN(arg.toString(10), 10);
             } else if (ethUtil.isValidAddress(arg)) {
                 argTypes.push('address');
-            } else if (arg instanceof Buffer) {
-                argTypes.push('bytes');
             } else if (_.isString(arg)) {
                 argTypes.push('string');
             } else if (_.isBuffer(arg)) {
diff --git a/packages/contracts/test/exchange/transactions.ts b/packages/contracts/test/exchange/transactions.ts
index 62bba0ab8..2bc078153 100644
--- a/packages/contracts/test/exchange/transactions.ts
+++ b/packages/contracts/test/exchange/transactions.ts
@@ -49,6 +49,7 @@ describe('Exchange transactions', () => {
 
     let erc20Balances: ERC20BalancesByOwner;
     let signedOrder: SignedOrder;
+    let signedTx: SignedTransaction;
     let order: OrderStruct;
     let orderFactory: OrderFactory;
     let makerTransactionFactory: TransactionFactory;
@@ -111,33 +112,28 @@ describe('Exchange transactions', () => {
 
     describe('executeTransaction', () => {
         describe('fillOrder', () => {
+            let takerAssetFillAmount: BigNumber;
             beforeEach(async () => {
                 erc20Balances = await erc20Wrapper.getBalancesAsync();
                 signedOrder = orderFactory.newSignedOrder();
                 order = orderUtils.getOrderStruct(signedOrder);
-            });
 
-            it('should throw if not called by specified sender', async () => {
-                const takerAssetFillAmount = signedOrder.takerAssetAmount.div(2);
+                takerAssetFillAmount = signedOrder.takerAssetAmount.div(2);
                 const data = exchange.fillOrder.getABIEncodedTransactionData(
                     order,
                     takerAssetFillAmount,
                     signedOrder.signature,
                 );
-                const signedTx = takerTransactionFactory.newSignedTransaction(data);
+                signedTx = takerTransactionFactory.newSignedTransaction(data);
+            });
+
+            it('should throw if not called by specified sender', async () => {
                 return expect(exchangeWrapper.executeTransactionAsync(signedTx, takerAddress)).to.be.rejectedWith(
                     constants.REVERT,
                 );
             });
 
             it('should transfer the correct amounts when signed by taker and called by sender', async () => {
-                const takerAssetFillAmount = signedOrder.takerAssetAmount.div(2);
-                const data = exchange.fillOrder.getABIEncodedTransactionData(
-                    order,
-                    takerAssetFillAmount,
-                    signedOrder.signature,
-                );
-                const signedTx = takerTransactionFactory.newSignedTransaction(data);
                 await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress);
                 const newBalances = await erc20Wrapper.getBalancesAsync();
                 const makerAssetFillAmount = takerAssetFillAmount
@@ -171,20 +167,34 @@ describe('Exchange transactions', () => {
                     erc20Balances[feeRecipientAddress][zrxToken.address].add(makerFeePaid.add(takerFeePaid)),
                 );
             });
+
+            it('should throw if the a 0x transaction with the same transactionHash has already been executed', async () => {
+                await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress);
+                return expect(exchangeWrapper.executeTransactionAsync(signedTx, senderAddress)).to.be.rejectedWith(
+                    constants.REVERT,
+                );
+            });
+
+            it('should reset the currentContextAddress', async () => {
+                await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress);
+                const currentContextAddress = await exchange.currentContextAddress.callAsync();
+                expect(currentContextAddress).to.equal(ZeroEx.NULL_ADDRESS);
+            });
         });
 
         describe('cancelOrder', () => {
-            it('should throw if not called by specified sender', async () => {
+            beforeEach(async () => {
                 const data = exchange.cancelOrder.getABIEncodedTransactionData(order);
-                const signedTx = makerTransactionFactory.newSignedTransaction(data);
+                signedTx = makerTransactionFactory.newSignedTransaction(data);
+            });
+
+            it('should throw if not called by specified sender', async () => {
                 return expect(exchangeWrapper.executeTransactionAsync(signedTx, makerAddress)).to.be.rejectedWith(
                     constants.REVERT,
                 );
             });
 
             it('should cancel the order when signed by maker and called by sender', async () => {
-                const data = exchange.cancelOrder.getABIEncodedTransactionData(order);
-                const signedTx = makerTransactionFactory.newSignedTransaction(data);
                 await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress);
                 const res = await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress);
                 const newBalances = await erc20Wrapper.getBalancesAsync();