Fix typos and add tests for decodeOrdersFromFillData

contracts/coordinator/contracts/src/libs/LibCoordinatorApproval.sol

@@ -38,9 +38,9 @@ contract LibCoordinatorApproval is
 
     struct CoordinatorApproval {
         address txOrigin;                       // Required signer of Ethereum transaction that is submitting approval.
-        bytes32 transactionHash;                // EIP712 hash of the transaction, using the domain separator of this contract.
+        bytes32 transactionHash;                // EIP712 hash of the transaction.
         bytes transactionSignature;             // Signature of the 0x transaction.
-        uint256 approvalExpirationTimeSeconds;  // Timestamp in seconds for which the signature expires.
+        uint256 approvalExpirationTimeSeconds;  // Timestamp in seconds for which the approval expires.
     }
 
     /// @dev Calculated the EIP712 hash of the Coordinator approval mesasage using the domain separator of this contract.
@@ -72,9 +72,10 @@ contract LibCoordinatorApproval is
         // Assembly for more efficiently computing:
         // keccak256(abi.encodePacked(
         //     EIP712_COORDINATOR_APPROVAL_SCHEMA_HASH,
+        //     approval.txOrigin,
         //     approval.transactionHash,
         //     keccak256(approval.transactionSignature)
-        //     approval.expiration,
+        //     approval.approvalExpirationTimeSeconds,
         // ));
 
         assembly {

contracts/coordinator/contracts/src/libs/LibZeroExTransaction.sol

@@ -41,7 +41,7 @@ contract LibZeroExTransaction is
         bytes data;             // AbiV2 encoded calldata.
     }
 
-    /// @dev Calculates the EIP712 hash of a 0x transaction using the domain separator of this contract.
+    /// @dev Calculates the EIP712 hash of a 0x transaction using the domain separator of the Exchange contract.
     /// @param transaction 0x transaction containing salt, signerAddress, and data.
     /// @return EIP712 hash of the transaction with the domain separator of this contract.
     function getTransactionHash(ZeroExTransaction memory transaction)

contracts/coordinator/test/coordinator.ts

@@ -435,7 +435,7 @@ describe('Coordinator tests', () => {
     describe('cancels', () => {
         it('cancelOrder call should be successful without an approval', async () => {
             const orders = [await orderFactory.newSignedOrderAsync()];
-            const data = exchangeDataEncoder.encodeOrdersToExchangeData(constants.CANCEL_ORDERS, orders);
+            const data = exchangeDataEncoder.encodeOrdersToExchangeData(constants.CANCEL_ORDER, orders);
             const transaction = makerTransactionFactory.newSignedTransaction(data);
             const transactionReceipt = await web3Wrapper.awaitTransactionSuccessAsync(
                 await coordinatorContract.executeTransaction.sendTransactionAsync(

contracts/coordinator/test/mixins.ts

@@ -135,6 +135,70 @@ describe('Mixins tests', () => {
         });
     });
 
+    describe('decodeOrdersFromFillData', () => {
+        for (const fnName of constants.SINGLE_FILL_FN_NAMES) {
+            it(`should correctly decode the orders for ${fnName} data`, async () => {
+                const orders = [defaultOrder];
+                const data = exchangeDataEncoder.encodeOrdersToExchangeData(fnName, orders);
+                const decodedOrders = await mixins.decodeOrdersFromFillData.callAsync(data);
+                const decodedSignedOrders = decodedOrders.map(order => ({
+                    ...order,
+                    exchangeAddress: devConstants.NULL_ADDRESS,
+                    signature: devConstants.NULL_BYTES,
+                }));
+                expect(orders).to.deep.eq(decodedSignedOrders);
+            });
+        }
+        for (const fnName of constants.BATCH_FILL_FN_NAMES) {
+            it(`should correctly decode the orders for ${fnName} data`, async () => {
+                const orders = [defaultOrder, defaultOrder];
+                const data = exchangeDataEncoder.encodeOrdersToExchangeData(fnName, orders);
+                const decodedOrders = await mixins.decodeOrdersFromFillData.callAsync(data);
+                const decodedSignedOrders = decodedOrders.map(order => ({
+                    ...order,
+                    exchangeAddress: devConstants.NULL_ADDRESS,
+                    signature: devConstants.NULL_BYTES,
+                }));
+                expect(orders).to.deep.eq(decodedSignedOrders);
+            });
+        }
+        for (const fnName of constants.MARKET_FILL_FN_NAMES) {
+            it(`should correctly decode the orders for ${fnName} data`, async () => {
+                const orders = [defaultOrder, defaultOrder];
+                const data = exchangeDataEncoder.encodeOrdersToExchangeData(fnName, orders);
+                const decodedOrders = await mixins.decodeOrdersFromFillData.callAsync(data);
+                const decodedSignedOrders = decodedOrders.map(order => ({
+                    ...order,
+                    exchangeAddress: devConstants.NULL_ADDRESS,
+                    signature: devConstants.NULL_BYTES,
+                }));
+                expect(orders).to.deep.eq(decodedSignedOrders);
+            });
+        }
+        for (const fnName of [constants.CANCEL_ORDER, constants.BATCH_CANCEL_ORDERS, constants.CANCEL_ORDERS_UP_TO]) {
+            it(`should correctly decode the orders for ${fnName} data`, async () => {
+                const orders = [defaultOrder, defaultOrder];
+                const data = exchangeDataEncoder.encodeOrdersToExchangeData(fnName, orders);
+                const decodedOrders = await mixins.decodeOrdersFromFillData.callAsync(data);
+                const emptyArray: any[] = [];
+                expect(emptyArray).to.deep.eq(decodedOrders);
+            });
+        }
+        it('should decode an empty array for invalid data', async () => {
+            const data = '0x0123456789';
+            const decodedOrders = await mixins.decodeOrdersFromFillData.callAsync(data);
+            const emptyArray: any[] = [];
+            expect(emptyArray).to.deep.eq(decodedOrders);
+        });
+        it('should revert if data is less than 4 bytes long', async () => {
+            const data = '0x010203';
+            await expectContractCallFailedAsync(
+                mixins.decodeOrdersFromFillData.callAsync(data),
+                RevertReason.LibBytesGreaterOrEqualTo4LengthRequired,
+            );
+        });
+    });
+
     describe('Single order approvals', () => {
         for (const fnName of constants.SINGLE_FILL_FN_NAMES) {
             it(`Should be successful: function=${fnName}, caller=tx_signer, senderAddress=[verifier], approval_sig=[approver1], expiration=[valid]`, async () => {
@@ -620,9 +684,9 @@ describe('Mixins tests', () => {
         }
     });
     describe('cancels', () => {
-        it('should allow the tx signer to call `cancelOrders` without approval', async () => {
+        it('should allow the tx signer to call `cancelOrder` without approval', async () => {
             const orders = [defaultOrder];
-            const data = exchangeDataEncoder.encodeOrdersToExchangeData(constants.CANCEL_ORDERS, orders);
+            const data = exchangeDataEncoder.encodeOrdersToExchangeData(constants.CANCEL_ORDER, orders);
             const transaction = transactionFactory.newSignedTransaction(data);
             await mixins.assertValidCoordinatorApprovals.callAsync(
                 transaction,

contracts/coordinator/test/utils/constants.ts

@@ -5,7 +5,7 @@ export const constants = {
     BATCH_FILL_FN_NAMES: ['batchFillOrders', 'batchFillOrKillOrders', 'batchFillOrdersNoThrow'],
     MARKET_FILL_FN_NAMES: ['marketBuyOrders', 'marketBuyOrdersNoThrow', 'marketSellOrders', 'marketSellOrdersNoThrow'],
     MATCH_ORDERS: 'matchOrders',
-    CANCEL_ORDERS: 'cancelOrders',
+    CANCEL_ORDER: 'cancelOrder',
     BATCH_CANCEL_ORDERS: 'batchCancelOrders',
     CANCEL_ORDERS_UP_TO: 'cancelOrdersUpTo',
     TIME_BUFFER: new BigNumber(1000),

contracts/coordinator/test/utils/exchange_data_encoder.ts

@@ -37,7 +37,7 @@ export const exchangeDataEncoder = {
                 orders[0].signature,
                 orders[1].signature,
             );
-        } else if (fnName === constants.CANCEL_ORDERS) {
+        } else if (fnName === constants.CANCEL_ORDER) {
             data = exchangeInstance.cancelOrder.getABIEncodedTransactionData(orders[0]);
         } else if (fnName === constants.BATCH_CANCEL_ORDERS) {
             data = exchangeInstance.batchCancelOrders.getABIEncodedTransactionData(orders);