add tests for partially filled, unfillable orders

contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol

@@ -154,6 +154,11 @@ contract MixinExchangeWrapper is
                 ));
             }
 
+            // Preemptively skip to avoid division by zero in _marketSellSingleOrder
+            if (orders[i].makerAssetAmount == 0 || orders[i].takerAssetAmount == 0) {
+                continue;
+            }
+
             LibFillResults.FillResults memory singleFillResults = _marketSellSingleOrder(
                 orders[i],
                 signatures[i],
@@ -268,6 +273,11 @@ contract MixinExchangeWrapper is
                 ));
             }
 
+            // Preemptively skip to avoid division by zero in _marketBuySingleOrder
+            if (orders[i].makerAssetAmount == 0 || orders[i].takerAssetAmount == 0) {
+                continue;
+            }
+
             LibFillResults.FillResults memory singleFillResults = _marketBuySingleOrder(
                 orders[i],
                 signatures[i],

contracts/exchange-forwarder/test/forwarder.ts

@@ -6,6 +6,7 @@ import {
     chaiSetup,
     constants,
     ContractName,
+    getLatestBlockTimestampAsync,
     OrderFactory,
     provider,
     sendTransactionResult,
@@ -333,6 +334,70 @@ describe(ContractName.Forwarder, () => {
                 revertError,
             });
         });
+        it('should fill a partially-filled order without a taker fee', async () => {
+            const order = await orderFactory.newSignedOrderAsync();
+            await forwarderTestFactory.marketSellTestAsync([order], new BigNumber(0.3), erc20Token);
+            await forwarderTestFactory.marketSellTestAsync([order], new BigNumber(0.8), erc20Token);
+        });
+        it('should skip over an order with an invalid maker asset amount', async () => {
+            const unfillableOrder = await orderFactory.newSignedOrderAsync({
+                makerAssetAmount: constants.ZERO_AMOUNT,
+            });
+            const fillableOrder = await orderFactory.newSignedOrderAsync();
+
+            await forwarderTestFactory.marketSellTestAsync(
+                [unfillableOrder, fillableOrder],
+                new BigNumber(1.5),
+                erc20Token,
+            );
+        });
+        it('should skip over an order with an invalid taker asset amount', async () => {
+            const unfillableOrder = await orderFactory.newSignedOrderAsync({
+                takerAssetAmount: constants.ZERO_AMOUNT,
+            });
+            const fillableOrder = await orderFactory.newSignedOrderAsync();
+
+            await forwarderTestFactory.marketSellTestAsync(
+                [unfillableOrder, fillableOrder],
+                new BigNumber(1.5),
+                erc20Token,
+            );
+        });
+        it('should skip over an expired order', async () => {
+            const currentTimestamp = await getLatestBlockTimestampAsync();
+            const expiredOrder = await orderFactory.newSignedOrderAsync({
+                expirationTimeSeconds: new BigNumber(currentTimestamp).minus(10),
+            });
+            const fillableOrder = await orderFactory.newSignedOrderAsync();
+
+            await forwarderTestFactory.marketSellTestAsync(
+                [expiredOrder, fillableOrder],
+                new BigNumber(1.5),
+                erc20Token,
+            );
+        });
+        it('should skip over a fully filled order', async () => {
+            const fullyFilledOrder = await orderFactory.newSignedOrderAsync();
+            await forwarderTestFactory.marketSellTestAsync([fullyFilledOrder], new BigNumber(1), erc20Token);
+
+            const fillableOrder = await orderFactory.newSignedOrderAsync();
+            await forwarderTestFactory.marketSellTestAsync(
+                [fullyFilledOrder, fillableOrder],
+                new BigNumber(1.5),
+                erc20Token,
+            );
+        });
+        it('should skip over a cancelled order', async () => {
+            const cancelledOrder = await orderFactory.newSignedOrderAsync();
+            await exchangeWrapper.cancelOrderAsync(cancelledOrder, makerAddress);
+
+            const fillableOrder = await orderFactory.newSignedOrderAsync();
+            await forwarderTestFactory.marketSellTestAsync(
+                [cancelledOrder, fillableOrder],
+                new BigNumber(1.5),
+                erc20Token,
+            );
+        });
     });
     describe('marketSellOrdersWithEth with extra fees', () => {
         it('should fill the order and send fee to feeRecipient', async () => {
@@ -447,6 +512,70 @@ describe(ContractName.Forwarder, () => {
                 revertError,
             });
         });
+        it('should fill a partially-filled order without a taker fee', async () => {
+            const order = await orderFactory.newSignedOrderAsync();
+            await forwarderTestFactory.marketBuyTestAsync([order], new BigNumber(0.3), erc20Token);
+            await forwarderTestFactory.marketBuyTestAsync([order], new BigNumber(0.8), erc20Token);
+        });
+        it('should skip over an order with an invalid maker asset amount', async () => {
+            const unfillableOrder = await orderFactory.newSignedOrderAsync({
+                makerAssetAmount: constants.ZERO_AMOUNT,
+            });
+            const fillableOrder = await orderFactory.newSignedOrderAsync();
+
+            await forwarderTestFactory.marketBuyTestAsync(
+                [unfillableOrder, fillableOrder],
+                new BigNumber(1.5),
+                erc20Token,
+            );
+        });
+        it('should skip over an order with an invalid taker asset amount', async () => {
+            const unfillableOrder = await orderFactory.newSignedOrderAsync({
+                takerAssetAmount: constants.ZERO_AMOUNT,
+            });
+            const fillableOrder = await orderFactory.newSignedOrderAsync();
+
+            await forwarderTestFactory.marketBuyTestAsync(
+                [unfillableOrder, fillableOrder],
+                new BigNumber(1.5),
+                erc20Token,
+            );
+        });
+        it('should skip over an expired order', async () => {
+            const currentTimestamp = await getLatestBlockTimestampAsync();
+            const expiredOrder = await orderFactory.newSignedOrderAsync({
+                expirationTimeSeconds: new BigNumber(currentTimestamp).minus(10),
+            });
+            const fillableOrder = await orderFactory.newSignedOrderAsync();
+
+            await forwarderTestFactory.marketBuyTestAsync(
+                [expiredOrder, fillableOrder],
+                new BigNumber(1.5),
+                erc20Token,
+            );
+        });
+        it('should skip over a fully filled order', async () => {
+            const fullyFilledOrder = await orderFactory.newSignedOrderAsync();
+            await forwarderTestFactory.marketBuyTestAsync([fullyFilledOrder], new BigNumber(1), erc20Token);
+
+            const fillableOrder = await orderFactory.newSignedOrderAsync();
+            await forwarderTestFactory.marketBuyTestAsync(
+                [fullyFilledOrder, fillableOrder],
+                new BigNumber(1.5),
+                erc20Token,
+            );
+        });
+        it('should skip over a cancelled order', async () => {
+            const cancelledOrder = await orderFactory.newSignedOrderAsync();
+            await exchangeWrapper.cancelOrderAsync(cancelledOrder, makerAddress);
+
+            const fillableOrder = await orderFactory.newSignedOrderAsync();
+            await forwarderTestFactory.marketBuyTestAsync(
+                [cancelledOrder, fillableOrder],
+                new BigNumber(1.5),
+                erc20Token,
+            );
+        });
         it('Should buy slightly greater MakerAsset when exchange rate is rounded', async () => {
             // The 0x Protocol contracts round the exchange rate in favor of the Maker.
             // In this case, the taker must round up how much they're going to spend, which

contracts/exchange-forwarder/test/utils/forwarder_test_factory.ts

@@ -26,7 +26,11 @@ interface ForwarderFillState {
 // Simulates filling some orders via the Forwarder contract. For example, if
 // orders = [A, B, C, D] and fractionalNumberOfOrdersToFill = 2.3, then
 // we simulate A and B being completely filled, and 0.3 * C being filled.
-function computeExpectedResults(orders: SignedOrder[], fractionalNumberOfOrdersToFill: BigNumber): ForwarderFillState {
+function computeExpectedResults(
+    orders: SignedOrder[],
+    ordersInfoBefore: OrderInfo[],
+    fractionalNumberOfOrdersToFill: BigNumber,
+): ForwarderFillState {
     const currentState = {
         takerAssetFillAmount: constants.ZERO_AMOUNT,
         makerAssetFillAmount: constants.ZERO_AMOUNT,
@@ -37,11 +41,17 @@ function computeExpectedResults(orders: SignedOrder[], fractionalNumberOfOrdersT
     };
     let remainingOrdersToFill = fractionalNumberOfOrdersToFill;
 
-    for (const order of orders) {
+    for (const [i, order] of orders.entries()) {
         if (remainingOrdersToFill.isEqualTo(constants.ZERO_AMOUNT)) {
             break;
         }
 
+        if (ordersInfoBefore[i].orderStatus !== OrderStatus.Fillable) {
+            // If the order is not fillable, skip over it but still count it towards fractionalNumberOfOrdersToFill
+            remainingOrdersToFill = BigNumber.max(remainingOrdersToFill.minus(new BigNumber(1)), constants.ZERO_AMOUNT);
+            continue;
+        }
+
         let makerAssetAmount;
         let takerAssetAmount;
         let takerFee;
@@ -57,7 +67,7 @@ function computeExpectedResults(orders: SignedOrder[], fractionalNumberOfOrdersT
 
             // Up to 1 wei worth of WETH will be oversold on the last order due to rounding
             currentState.maxOversoldWeth = new BigNumber(1);
-            // Equivalently, up to 1 wei worth of maker asset will be overbought per order
+            // Equivalently, up to 1 wei worth of maker asset will be overbought
             currentState.maxOverboughtMakerAsset = currentState.maxOversoldWeth
                 .times(order.makerAssetAmount)
                 .dividedToIntegerBy(order.takerAssetAmount);
@@ -67,6 +77,15 @@ function computeExpectedResults(orders: SignedOrder[], fractionalNumberOfOrdersT
             takerFee = order.takerFee;
         }
 
+        // Accounting for partially filled orders
+        // As with unfillable orders, these still count as 1 towards fractionalNumberOfOrdersToFill
+        const takerAssetFilled = ordersInfoBefore[i].orderTakerAssetFilledAmount;
+        const makerAssetFilled = takerAssetFilled
+            .times(order.makerAssetAmount)
+            .dividedToIntegerBy(order.takerAssetAmount);
+        takerAssetAmount = BigNumber.max(takerAssetAmount.minus(takerAssetFilled), constants.ZERO_AMOUNT);
+        makerAssetAmount = BigNumber.max(makerAssetAmount.minus(makerAssetFilled), constants.ZERO_AMOUNT);
+
         currentState.takerAssetFillAmount = currentState.takerAssetFillAmount.plus(takerAssetAmount);
         currentState.makerAssetFillAmount = currentState.makerAssetFillAmount.plus(makerAssetAmount);
 
@@ -150,7 +169,10 @@ export class ForwarderTestFactory {
             this._forwarderFeeRecipientAddress,
         );
 
-        const expectedResults = computeExpectedResults(orders, fractionalNumberOfOrdersToFill);
+        const ordersInfoBefore = await this._exchangeWrapper.getOrdersInfoAsync(orders);
+        const orderStatusesBefore = ordersInfoBefore.map(orderInfo => orderInfo.orderStatus);
+
+        const expectedResults = computeExpectedResults(orders, ordersInfoBefore, fractionalNumberOfOrdersToFill);
         const ethSpentOnForwarderFee = ForwarderTestFactory.getPercentageOfValue(
             expectedResults.takerAssetFillAmount,
             forwarderFeePercentage,
@@ -166,9 +188,6 @@ export class ForwarderTestFactory {
             .plus(ethSpentOnForwarderFee)
             .plus(ethValueAdjustment);
 
-        const ordersInfoBefore = await this._exchangeWrapper.getOrdersInfoAsync(orders);
-        const orderStatusesBefore = ordersInfoBefore.map(orderInfo => orderInfo.orderStatus);
-
         const tx = this._forwarderWrapper.marketBuyOrdersWithEthAsync(
             orders,
             expectedResults.makerAssetFillAmount.minus(expectedResults.percentageFees),
@@ -221,7 +240,10 @@ export class ForwarderTestFactory {
             this._forwarderFeeRecipientAddress,
         );
 
-        const expectedResults = computeExpectedResults(orders, fractionalNumberOfOrdersToFill);
+        const ordersInfoBefore = await this._exchangeWrapper.getOrdersInfoAsync(orders);
+        const orderStatusesBefore = ordersInfoBefore.map(orderInfo => orderInfo.orderStatus);
+
+        const expectedResults = computeExpectedResults(orders, ordersInfoBefore, fractionalNumberOfOrdersToFill);
         const ethSpentOnForwarderFee = ForwarderTestFactory.getPercentageOfValue(
             expectedResults.takerAssetFillAmount,
             forwarderFeePercentage,
@@ -236,9 +258,6 @@ export class ForwarderTestFactory {
             .plus(expectedResults.maxOversoldWeth)
             .plus(ethSpentOnForwarderFee);
 
-        const ordersInfoBefore = await this._exchangeWrapper.getOrdersInfoAsync(orders);
-        const orderStatusesBefore = ordersInfoBefore.map(orderInfo => orderInfo.orderStatus);
-
         const tx = this._forwarderWrapper.marketSellOrdersWithEthAsync(
             orders,
             {
@@ -318,9 +337,11 @@ export class ForwarderTestFactory {
         } = {},
     ): Promise<void> {
         for (const [i, orderStatus] of orderStatusesAfter.entries()) {
-            const expectedOrderStatus = fractionalNumberOfOrdersToFill.gte(i + 1)
+            let expectedOrderStatus = orderStatusesBefore[i];
-                ? OrderStatus.FullyFilled
+            if (fractionalNumberOfOrdersToFill.gte(i + 1) && orderStatusesBefore[i] === OrderStatus.Fillable) {
-                : orderStatusesBefore[i];
+                expectedOrderStatus = OrderStatus.FullyFilled;
+            }
+
             expect(orderStatus).to.equal(expectedOrderStatus);
         }