diff --git a/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol b/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol index 8636737d38..aecb24e202 100644 --- a/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol +++ b/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], diff --git a/contracts/exchange-forwarder/test/forwarder.ts b/contracts/exchange-forwarder/test/forwarder.ts index d446ce09bd..cb2c687b18 100644 --- a/contracts/exchange-forwarder/test/forwarder.ts +++ b/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 diff --git a/contracts/exchange-forwarder/test/utils/forwarder_test_factory.ts b/contracts/exchange-forwarder/test/utils/forwarder_test_factory.ts index afa5a03f19..cec1f411c0 100644 --- a/contracts/exchange-forwarder/test/utils/forwarder_test_factory.ts +++ b/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 { for (const [i, orderStatus] of orderStatusesAfter.entries()) { - const expectedOrderStatus = fractionalNumberOfOrdersToFill.gte(i + 1) - ? OrderStatus.FullyFilled - : orderStatusesBefore[i]; + let expectedOrderStatus = orderStatusesBefore[i]; + if (fractionalNumberOfOrdersToFill.gte(i + 1) && orderStatusesBefore[i] === OrderStatus.Fillable) { + expectedOrderStatus = OrderStatus.FullyFilled; + } + expect(orderStatus).to.equal(expectedOrderStatus); }