Add senderAddress to Fill and Cancel logs, add comments to events and types

5 files changed, 47 insertions, 44 deletions

diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol
index 8539342b7..901e71936 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol
@@ -225,8 +225,9 @@ contract MixinExchangeCore is
         // Log order
         emit Fill(
             order.makerAddress,
-            takerAddress,
             order.feeRecipientAddress,
+            takerAddress,
+            msg.sender,
             fillResults.makerAssetFilledAmount,
             fillResults.takerAssetFilledAmount,
             fillResults.makerFeePaid,
@@ -255,6 +256,7 @@ contract MixinExchangeCore is
         emit Cancel(
             order.makerAddress,
             order.feeRecipientAddress,
+            msg.sender,
             orderHash,
             order.makerAssetData,
             order.takerAssetData
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibFillResults.sol b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibFillResults.sol
index b7550d6d2..63f1b8c87 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibFillResults.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibFillResults.sol
@@ -25,16 +25,16 @@ contract LibFillResults is
 {
 
     struct FillResults {
-        uint256 makerAssetFilledAmount;
-        uint256 takerAssetFilledAmount;
-        uint256 makerFeePaid;
-        uint256 takerFeePaid;
+        uint256 makerAssetFilledAmount;  // Total amount of makerAsset(s) filled.
+        uint256 takerAssetFilledAmount;  // Total amount of takerAsset(s) filled.
+        uint256 makerFeePaid;            // Total amount of ZRX paid by maker(s) to feeRecipient(s).
+        uint256 takerFeePaid;            // Total amount of ZRX paid by taker to feeRecipients(s).
     }
 
     struct MatchedFillResults {
-        FillResults left;
-        FillResults right;
-        uint256 leftMakerAssetSpreadAmount;
+        FillResults left;                    // Amounts filled and fees paid of left order.
+        FillResults right;                   // Amounts filled and fees paid of right order.
+        uint256 leftMakerAssetSpreadAmount;  // Spread between price of left and right order, denominated in the left order's makerAsset, paid to taker.
     }
 
     /// @dev Adds properties of both FillResults instances.
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibOrder.sol b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibOrder.sol
index bfc7aaae0..f3f1e9277 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibOrder.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibOrder.sol
@@ -55,27 +55,24 @@ contract LibOrder is
     }
 
     struct Order {
-        address makerAddress;
-        address takerAddress;
-        address feeRecipientAddress;
-        address senderAddress;
-        uint256 makerAssetAmount;
-        uint256 takerAssetAmount;
-        uint256 makerFee;
-        uint256 takerFee;
-        uint256 expirationTimeSeconds;
-        uint256 salt;
-        bytes makerAssetData;
-        bytes takerAssetData;
+        address makerAddress;           // Address that created the order.      
+        address takerAddress;           // Address that is allowed to fill the order. If set to 0, any address is allowed to fill the order.          
+        address feeRecipientAddress;    // Address that will recieve fees when order is filled.      
+        address senderAddress;          // Address that is allowed to call Exchange contract methods that affect this order. If set to 0, any address is allowed to call these methods.
+        uint256 makerAssetAmount;       // Amount of makerAsset being offered by maker. Must be greater than 0.        
+        uint256 takerAssetAmount;       // Amount of takerAsset being bid on by maker. Must be greater than 0.        
+        uint256 makerFee;               // Amount of ZRX paid to feeRecipient by maker when order is filled. If set to 0, no transfer of ZRX from maker to feeRecipient will be attempted.
+        uint256 takerFee;               // Amount of ZRX paid to feeRecipient by taker when order is filled. If set to 0, no transfer of ZRX from taker to feeRecipient will be attempted.
+        uint256 expirationTimeSeconds;  // Timestamp in seconds at which order expires.          
+        uint256 salt;                   // Arbitrary number to facilitate uniqueness of the order's hash.     
+        bytes makerAssetData;           // Encoded data that can be decoded by a specified proxy contract when transferring makerAsset. The last byte references the id of this proxy.
+        bytes takerAssetData;           // Encoded data that can be decoded by a specified proxy contract when transferring takerAsset. The last byte references the id of this proxy.
     }
 
     struct OrderInfo {
-        // See LibStatus for a complete description of order statuses
-        uint8 orderStatus;
-        // Keccak-256 EIP712 hash of the order
-        bytes32 orderHash;
-        // Amount of order that has been filled
-        uint256 orderTakerAssetFilledAmount;
+        uint8 orderStatus;                    // Status that describes order's validity and fillability.
+        bytes32 orderHash;                    // EIP712 hash of the order (see LibOrder.getOrderHash).
+        uint256 orderTakerAssetFilledAmount;  // Amount of order that has already been filled.
     }
 
     /// @dev Calculates Keccak-256 hash of the order.
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol
index 1737f5baf..b0dbceff6 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol
@@ -28,32 +28,34 @@ contract MExchangeCore is
 {
     // Fill event is emitted whenever an order is filled.
     event Fill(
-        address indexed makerAddress,
-        address takerAddress,
-        address indexed feeRecipientAddress,
-        uint256 makerAssetFilledAmount,
-        uint256 takerAssetFilledAmount,
-        uint256 makerFeePaid,
-        uint256 takerFeePaid,
-        bytes32 indexed orderHash,
-        bytes makerAssetData,
-        bytes takerAssetData
+        address indexed makerAddress,         // Address that created the order.      
+        address indexed feeRecipientAddress,  // Address that received fees.
+        address takerAddress,                 // Address that filled the order.
+        address senderAddress,                // Address that called the Exchange contract (msg.sender).
+        uint256 makerAssetFilledAmount,       // Amount of makerAsset sold by maker and bought by taker. 
+        uint256 takerAssetFilledAmount,       // Amount of takerAsset sold by taker and bought by maker.
+        uint256 makerFeePaid,                 // Amount of ZRX paid to feeRecipient by maker.
+        uint256 takerFeePaid,                 // Amount of ZRX paid to feeRecipient by taker.
+        bytes32 indexed orderHash,            // EIP712 hash of order (see LibOrder.getOrderHash)
+        bytes makerAssetData,                 // Encoded data specific to makerAsset. 
+        bytes takerAssetData                  // Encoded data specific to takerAsset.
     );
 
     // Cancel event is emitted whenever an individual order is cancelled.
     event Cancel(
-        address indexed makerAddress,
-        address indexed feeRecipientAddress,
-        bytes32 indexed orderHash,
-        bytes makerAssetData,
-        bytes takerAssetData
+        address indexed makerAddress,         // Address that created the order.      
+        address indexed feeRecipientAddress,  // Address that would have recieved fees if order was filled.   
+        address senderAddress,                // Address that called the Exchange contract (msg.sender).
+        bytes32 indexed orderHash,            // EIP712 hash of order (see LibOrder.getOrderHash)
+        bytes makerAssetData,                 // Encoded data specific to makerAsset. 
+        bytes takerAssetData                  // Encoded data specific to takerAsset.
     );
 
     // CancelUpTo event is emitted whenever `cancelOrdersUpTo` is executed succesfully.
     event CancelUpTo(
-        address indexed makerAddress,
-        address indexed senderAddress,
-        uint256 orderEpoch
+        address indexed makerAddress,         // Orders cancelled must have been created by this address.
+        address indexed senderAddress,        // Orders cancelled must have a `senderAddress` equal to this address.
+        uint256 orderEpoch                    // Orders specified makerAddress and senderAddress with a salt <= this value are considered cancelled.
     );
 
     /// @dev Updates state with results of a fill order.
diff --git a/packages/contracts/test/exchange/core.ts b/packages/contracts/test/exchange/core.ts
index ea37a1e99..89a1c6492 100644
--- a/packages/contracts/test/exchange/core.ts
+++ b/packages/contracts/test/exchange/core.ts
@@ -399,6 +399,7 @@ describe('Exchange core', () => {
 
             expect(signedOrder.makerAddress).to.be.equal(logArgs.makerAddress);
             expect(takerAddress).to.be.equal(logArgs.takerAddress);
+            expect(takerAddress).to.be.equal(logArgs.senderAddress);
             expect(signedOrder.feeRecipientAddress).to.be.equal(logArgs.feeRecipientAddress);
             expect(signedOrder.makerAssetData).to.be.equal(logArgs.makerAssetData);
             expect(signedOrder.takerAssetData).to.be.equal(logArgs.takerAssetData);
@@ -577,6 +578,7 @@ describe('Exchange core', () => {
             const logArgs = log.args;
 
             expect(signedOrder.makerAddress).to.be.equal(logArgs.makerAddress);
+            expect(signedOrder.makerAddress).to.be.equal(logArgs.senderAddress);
             expect(signedOrder.feeRecipientAddress).to.be.equal(logArgs.feeRecipientAddress);
             expect(signedOrder.makerAssetData).to.be.equal(logArgs.makerAssetData);
             expect(signedOrder.takerAssetData).to.be.equal(logArgs.takerAssetData);