Address feedback

3 files changed, 29 insertions, 16 deletions

diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts
index 94de45dd8..5caa1da2d 100644
--- a/src/contract_wrappers/exchange_wrapper.ts
+++ b/src/contract_wrappers/exchange_wrapper.ts
@@ -127,7 +127,7 @@ export class ExchangeWrapper extends ContractWrapper {
         return cancelledAmountInBaseUnits;
     }
     /**
-     * Fills a signed order with a takerTokenFillAmount denominated in baseUnits of the taker token.
+     * Fills a signed order with an amount denominated in baseUnits of the taker token.
      * Since the order in which transactions are included in the next block is indeterminate, race-conditions
      * could arise where a users balance or allowance changes before the fillOrder executes. Because of this,
      * we allow you to specify `shouldCheckTransfer`. If true, the smart contract will not throw if while
@@ -176,25 +176,27 @@ export class ExchangeWrapper extends ContractWrapper {
         this.throwErrorLogsAsErrors(response.logs);
     }
     /**
-     * Batched version of fillOrderAsync. Executes fills atomically in a single transaction.
+     * Batch version of fillOrderAsync.
+     * Executes multiple fills atomically in a single transaction.
+     * If shouldCheckTransfer is set to true, it will continue filling subsequent orders even when earlier ones fail.
+     * When shouldCheckTransfer is set to false, if any fill fails, the entire batch fails.
      */
     public async batchFillOrderAsync(orderFillRequests: OrderFillRequest[],
                                      shouldCheckTransfer: boolean, takerAddress: string): Promise<void> {
-        if (_.isEmpty(orderFillRequests)) {
-            return; // no-op
-        }
         assert.isBoolean('shouldCheckTransfer', shouldCheckTransfer);
         await assert.isSenderAddressAsync('takerAddress', takerAddress, this.web3Wrapper);
         _.forEach(orderFillRequests,
-            async (orderFillRequest: OrderFillRequest) => {
-            assert.doesConformToSchema('signedOrder',
+            async (orderFillRequest: OrderFillRequest, i: number) => {
+            assert.doesConformToSchema(`orderFillRequests[${i}].signedOrder`,
                 SchemaValidator.convertToJSONSchemaCompatibleObject(orderFillRequest.signedOrder as object),
                 signedOrderSchema);
-            assert.isBigNumber('takerTokenFillAmount', orderFillRequest.takerTokenFillAmount);
+            assert.isBigNumber(`orderFillRequests[${i}].takerTokenFillAmount`, orderFillRequest.takerTokenFillAmount);
             await this.validateFillOrderAndThrowIfInvalidAsync(
                 orderFillRequest.signedOrder, orderFillRequest.takerTokenFillAmount, takerAddress);
         });
-        const exchangeInstance = await this.getExchangeContractAsync();
+        if (_.isEmpty(orderFillRequests)) {
+            return; // no-op
+        }
 
         const orderAddressesValuesAmountsAndSignatureArray = _.map(orderFillRequests, orderFillRequest => {
             return [
@@ -210,6 +212,7 @@ export class ExchangeWrapper extends ContractWrapper {
             orderAddressesValuesAmountsAndSignatureArray,
         );
 
+        const exchangeInstance = await this.getExchangeContractAsync();
         const gas = await exchangeInstance.batchFill.estimateGas(
             orderAddressesArray,
             orderValuesArray,
diff --git a/test/exchange_wrapper_test.ts b/test/exchange_wrapper_test.ts
index 2d7810bcc..5cc864d12 100644
--- a/test/exchange_wrapper_test.ts
+++ b/test/exchange_wrapper_test.ts
@@ -229,9 +229,9 @@ describe('ExchangeWrapper', () => {
                     const signedOrder = await fillScenarios.createFillableSignedOrderAsync(
                         makerTokenAddress, takerTokenAddress, makerAddress, takerAddress, fillableAmount,
                     );
-                    const nonExistentSenderAddress = userAddresses[6];
+                    const nonTakerAddress = userAddresses[6];
                     return expect(zeroEx.exchange.fillOrderAsync(
-                        signedOrder, fillTakerAmount, shouldCheckTransfer, nonExistentSenderAddress,
+                        signedOrder, fillTakerAmount, shouldCheckTransfer, nonTakerAddress,
                     )).to.be.rejectedWith(ExchangeContractErrs.TRANSACTION_SENDER_IS_NOT_FILL_ORDER_TAKER);
                 });
                 it('should throw when order is expired', async () => {
@@ -520,7 +520,7 @@ describe('ExchangeWrapper', () => {
                             order: signedOrderWithDifferentMaker,
                             takerTokenCancelAmount: cancelAmount,
                         },
-                    ])).to.be.rejectedWith('Can not cancel orders from multiple makers in a single batch');
+                    ])).to.be.rejectedWith(ExchangeContractErrs.MULTIPLE_MAKERS_IN_SINGLE_CANCEL_BATCH);
                 });
             });
             describe('successful batch cancels', () => {
diff --git a/test/utils/fill_scenarios.ts b/test/utils/fill_scenarios.ts
index 8ff27ceb1..539333b9b 100644
--- a/test/utils/fill_scenarios.ts
+++ b/test/utils/fill_scenarios.ts
@@ -72,19 +72,29 @@ export class FillScenarios {
         await this.zeroEx.token.transferAsync(makerTokenAddress, this.coinbase, makerAddress, makerFillableAmount);
         const oldMakerAllowance = await this.zeroEx.token.getProxyAllowanceAsync(makerTokenAddress, makerAddress);
         await this.zeroEx.token.setProxyAllowanceAsync(
-            makerTokenAddress, makerAddress, oldMakerAllowance.plus(makerFillableAmount));
+            makerTokenAddress, makerAddress, oldMakerAllowance.plus(makerFillableAmount),
+        );
         await this.zeroEx.token.transferAsync(takerTokenAddress, this.coinbase, takerAddress, takerFillableAmount);
         const oldTakerAllowance = await this.zeroEx.token.getProxyAllowanceAsync(takerTokenAddress, takerAddress);
         await this.zeroEx.token.setProxyAllowanceAsync(
-            takerTokenAddress, takerAddress, oldTakerAllowance.plus(takerFillableAmount));
+            takerTokenAddress, takerAddress, oldTakerAllowance.plus(takerFillableAmount),
+        );
 
         if (!makerFee.isZero()) {
             await this.zeroEx.token.transferAsync(this.zrxTokenAddress, this.coinbase, makerAddress, makerFee);
-            await this.zeroEx.token.setProxyAllowanceAsync(this.zrxTokenAddress, makerAddress, makerFee);
+            const oldMakerFeeAllowance =
+                await this.zeroEx.token.getProxyAllowanceAsync(this.zrxTokenAddress, makerAddress);
+            await this.zeroEx.token.setProxyAllowanceAsync(
+                this.zrxTokenAddress, makerAddress, oldMakerFeeAllowance.plus(makerFee),
+            );
         }
         if (!takerFee.isZero()) {
             await this.zeroEx.token.transferAsync(this.zrxTokenAddress, this.coinbase, takerAddress, takerFee);
-            await this.zeroEx.token.setProxyAllowanceAsync(this.zrxTokenAddress, takerAddress, takerFee);
+            const oldTakerFeeAllowance =
+                await this.zeroEx.token.getProxyAllowanceAsync(this.zrxTokenAddress, takerAddress);
+            await this.zeroEx.token.setProxyAllowanceAsync(
+                this.zrxTokenAddress, takerAddress, oldTakerFeeAllowance.plus(takerFee),
+            );
         }
 
         const signedOrder = await orderFactory.createSignedOrderAsync(this.zeroEx,