Merge pull request #1162 from 0xProject/fix/contracts/forwarderRounding

[WIP] Round up for Market Buys in Forwarding Contract.

3 files changed, 278 insertions, 8 deletions

diff --git a/packages/contracts/contracts/extensions/Forwarder/MixinExchangeWrapper.sol b/packages/contracts/contracts/extensions/Forwarder/MixinExchangeWrapper.sol
index fea9a53c2..4991c0ea5 100644
--- a/packages/contracts/contracts/extensions/Forwarder/MixinExchangeWrapper.sol
+++ b/packages/contracts/contracts/extensions/Forwarder/MixinExchangeWrapper.sol
@@ -155,8 +155,10 @@ contract MixinExchangeWrapper is
             uint256 remainingMakerAssetFillAmount = safeSub(makerAssetFillAmount, totalFillResults.makerAssetFilledAmount);
 
             // Convert the remaining amount of makerAsset to buy into remaining amount
-            // of takerAsset to sell, assuming entire amount can be sold in the current order
-            uint256 remainingTakerAssetFillAmount = getPartialAmountFloor(
+            // of takerAsset to sell, assuming entire amount can be sold in the current order.
+            // We round up because the exchange rate computed by fillOrder rounds in favor
+            // of the Maker. In this case we want to overestimate the amount of takerAsset.
+            uint256 remainingTakerAssetFillAmount = getPartialAmountCeil(
                 orders[i].takerAssetAmount,
                 orders[i].makerAssetAmount,
                 remainingMakerAssetFillAmount
@@ -224,7 +226,9 @@ contract MixinExchangeWrapper is
 
             // Convert the remaining amount of ZRX to buy into remaining amount
             // of WETH to sell, assuming entire amount can be sold in the current order.
-            uint256 remainingWethSellAmount = getPartialAmountFloor(
+            // We round up because the exchange rate computed by fillOrder rounds in favor
+            // of the Maker. In this case we want to overestimate the amount of takerAsset.
+            uint256 remainingWethSellAmount = getPartialAmountCeil(
                 orders[i].takerAssetAmount,
                 safeSub(orders[i].makerAssetAmount, orders[i].takerFee),  // our exchange rate after fees 
                 remainingZrxBuyAmount
@@ -233,7 +237,7 @@ contract MixinExchangeWrapper is
             // Attempt to sell the remaining amount of WETH.
             FillResults memory singleFillResult = fillOrderNoThrow(
                 orders[i],
-                safeAdd(remainingWethSellAmount, 1),  // we add 1 wei to the fill amount to make up for rounding errors
+                remainingWethSellAmount,
                 signatures[i]
             );
 
diff --git a/packages/contracts/test/extensions/forwarder.ts b/packages/contracts/test/extensions/forwarder.ts
index b76624fa9..c006be0fe 100644
--- a/packages/contracts/test/extensions/forwarder.ts
+++ b/packages/contracts/test/extensions/forwarder.ts
@@ -45,6 +45,7 @@ describe(ContractName.Forwarder, () => {
 
     let weth: DummyERC20TokenContract;
     let zrxToken: DummyERC20TokenContract;
+    let erc20TokenA: DummyERC20TokenContract;
     let erc721Token: DummyERC721TokenContract;
     let forwarderContract: ForwarderContract;
     let wethContract: WETH9Contract;
@@ -77,7 +78,6 @@ describe(ContractName.Forwarder, () => {
         erc20Wrapper = new ERC20Wrapper(provider, usedAddresses, owner);
 
         const numDummyErc20ToDeploy = 3;
-        let erc20TokenA;
         [erc20TokenA, zrxToken] = await erc20Wrapper.deployDummyTokensAsync(
             numDummyErc20ToDeploy,
             constants.DUMMY_TOKEN_DECIMALS,
@@ -902,6 +902,269 @@ describe(ContractName.Forwarder, () => {
             );
             expect(forwarderEthBalance).to.be.bignumber.equal(constants.ZERO_AMOUNT);
         });
+        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
+            // in turn increases the amount of MakerAsset being purchased.
+            // Example:
+            //  The taker wants to buy 5 units of the MakerAsset at a rate of 3M/2T.
+            //  For every 2 units of TakerAsset, the taker will receive 3 units of MakerAsset.
+            //  To purchase 5 units, the taker must spend 10/3 = 3.33 units of TakerAssset.
+            //  However, the Taker can only spend whole units.
+            //  Spending floor(10/3) = 3 units will yield a profit of Floor(3*3/2) = Floor(4.5) = 4 units of MakerAsset.
+            //  Spending ceil(10/3) = 4 units will yield a profit of Floor(4*3/2) = 6 units of MakerAsset.
+            //
+            //  The forwarding contract will opt for the second option, which overbuys, to ensure the taker
+            //  receives at least the amount of MakerAsset they requested.
+            //
+            // Construct test case using values from example above
+            orderWithoutFee = await orderFactory.newSignedOrderAsync({
+                makerAssetAmount: new BigNumber('30'),
+                takerAssetAmount: new BigNumber('20'),
+                makerAssetData: assetDataUtils.encodeERC20AssetData(erc20TokenA.address),
+                takerAssetData: assetDataUtils.encodeERC20AssetData(weth.address),
+                makerFee: new BigNumber(0),
+                takerFee: new BigNumber(0),
+            });
+            const ordersWithoutFee = [orderWithoutFee];
+            const feeOrders: SignedOrder[] = [];
+            const desiredMakerAssetFillAmount = new BigNumber('5');
+            const makerAssetFillAmount = new BigNumber('6');
+            const ethValue = new BigNumber('4');
+            // Execute test case
+            tx = await forwarderWrapper.marketBuyOrdersWithEthAsync(
+                ordersWithoutFee,
+                feeOrders,
+                desiredMakerAssetFillAmount,
+                {
+                    value: ethValue,
+                    from: takerAddress,
+                },
+            );
+            // Fetch end balances and construct expected outputs
+            const takerEthBalanceAfter = await web3Wrapper.getBalanceInWeiAsync(takerAddress);
+            const forwarderEthBalance = await web3Wrapper.getBalanceInWeiAsync(forwarderContract.address);
+            const newBalances = await erc20Wrapper.getBalancesAsync();
+            const primaryTakerAssetFillAmount = ethValue;
+            const totalEthSpent = primaryTakerAssetFillAmount.plus(gasPrice.times(tx.gasUsed));
+            // Validate test case
+            expect(makerAssetFillAmount).to.be.bignumber.greaterThan(desiredMakerAssetFillAmount);
+            expect(takerEthBalanceAfter).to.be.bignumber.equal(takerEthBalanceBefore.minus(totalEthSpent));
+            expect(newBalances[makerAddress][defaultMakerAssetAddress]).to.be.bignumber.equal(
+                erc20Balances[makerAddress][defaultMakerAssetAddress].minus(makerAssetFillAmount),
+            );
+            expect(newBalances[takerAddress][defaultMakerAssetAddress]).to.be.bignumber.equal(
+                erc20Balances[takerAddress][defaultMakerAssetAddress].plus(makerAssetFillAmount),
+            );
+            expect(newBalances[makerAddress][weth.address]).to.be.bignumber.equal(
+                erc20Balances[makerAddress][weth.address].plus(primaryTakerAssetFillAmount),
+            );
+            expect(newBalances[forwarderContract.address][weth.address]).to.be.bignumber.equal(constants.ZERO_AMOUNT);
+            expect(newBalances[forwarderContract.address][defaultMakerAssetAddress]).to.be.bignumber.equal(
+                constants.ZERO_AMOUNT,
+            );
+            expect(forwarderEthBalance).to.be.bignumber.equal(constants.ZERO_AMOUNT);
+        });
+        it('Should buy slightly greater MakerAsset when exchange rate is rounded, and MakerAsset is ZRX', async () => {
+            // See the test case above for a detailed description of this case.
+            // The difference here is that the MakerAsset is ZRX. We expect the same result as above,
+            // but this tests a different code path.
+            //
+            // Construct test case using values from example above
+            orderWithoutFee = await orderFactory.newSignedOrderAsync({
+                makerAssetAmount: new BigNumber('30'),
+                takerAssetAmount: new BigNumber('20'),
+                makerAssetData: zrxAssetData,
+                takerAssetData: assetDataUtils.encodeERC20AssetData(weth.address),
+                makerFee: new BigNumber(0),
+                takerFee: new BigNumber(0),
+            });
+            const ordersWithoutFee = [orderWithoutFee];
+            const feeOrders: SignedOrder[] = [];
+            const desiredMakerAssetFillAmount = new BigNumber('5');
+            const makerAssetFillAmount = new BigNumber('6');
+            const ethValue = new BigNumber('4');
+            // Execute test case
+            tx = await forwarderWrapper.marketBuyOrdersWithEthAsync(
+                ordersWithoutFee,
+                feeOrders,
+                desiredMakerAssetFillAmount,
+                {
+                    value: ethValue,
+                    from: takerAddress,
+                },
+            );
+            // Fetch end balances and construct expected outputs
+            const takerEthBalanceAfter = await web3Wrapper.getBalanceInWeiAsync(takerAddress);
+            const forwarderEthBalance = await web3Wrapper.getBalanceInWeiAsync(forwarderContract.address);
+            const newBalances = await erc20Wrapper.getBalancesAsync();
+            const primaryTakerAssetFillAmount = ethValue;
+            const totalEthSpent = primaryTakerAssetFillAmount.plus(gasPrice.times(tx.gasUsed));
+            // Validate test case
+            expect(makerAssetFillAmount).to.be.bignumber.greaterThan(desiredMakerAssetFillAmount);
+            expect(takerEthBalanceAfter).to.be.bignumber.equal(takerEthBalanceBefore.minus(totalEthSpent));
+            expect(newBalances[makerAddress][zrxToken.address]).to.be.bignumber.equal(
+                erc20Balances[makerAddress][zrxToken.address].minus(makerAssetFillAmount),
+            );
+            expect(newBalances[takerAddress][zrxToken.address]).to.be.bignumber.equal(
+                erc20Balances[takerAddress][zrxToken.address].plus(makerAssetFillAmount),
+            );
+            expect(newBalances[makerAddress][weth.address]).to.be.bignumber.equal(
+                erc20Balances[makerAddress][weth.address].plus(primaryTakerAssetFillAmount),
+            );
+            expect(newBalances[forwarderContract.address][weth.address]).to.be.bignumber.equal(constants.ZERO_AMOUNT);
+            expect(forwarderEthBalance).to.be.bignumber.equal(constants.ZERO_AMOUNT);
+        });
+        it('Should buy slightly greater MakerAsset when exchange rate is rounded (Regression Test)', async () => {
+            // Order taken from a transaction on mainnet that failed due to a rounding error.
+            orderWithoutFee = await orderFactory.newSignedOrderAsync({
+                makerAssetAmount: new BigNumber('268166666666666666666'),
+                takerAssetAmount: new BigNumber('219090625878836371'),
+                makerAssetData: assetDataUtils.encodeERC20AssetData(erc20TokenA.address),
+                takerAssetData: assetDataUtils.encodeERC20AssetData(weth.address),
+                makerFee: new BigNumber(0),
+                takerFee: new BigNumber(0),
+            });
+            const ordersWithoutFee = [orderWithoutFee];
+            const feeOrders: SignedOrder[] = [];
+            // The taker will receive more than the desired amount of makerAsset due to rounding
+            const desiredMakerAssetFillAmount = new BigNumber('5000000000000000000');
+            const ethValue = new BigNumber('4084971271824171');
+            const makerAssetFillAmount = ethValue
+                .times(orderWithoutFee.makerAssetAmount)
+                .dividedToIntegerBy(orderWithoutFee.takerAssetAmount);
+            // Execute test case
+            tx = await forwarderWrapper.marketBuyOrdersWithEthAsync(
+                ordersWithoutFee,
+                feeOrders,
+                desiredMakerAssetFillAmount,
+                {
+                    value: ethValue,
+                    from: takerAddress,
+                },
+            );
+            // Fetch end balances and construct expected outputs
+            const takerEthBalanceAfter = await web3Wrapper.getBalanceInWeiAsync(takerAddress);
+            const forwarderEthBalance = await web3Wrapper.getBalanceInWeiAsync(forwarderContract.address);
+            const newBalances = await erc20Wrapper.getBalancesAsync();
+            const primaryTakerAssetFillAmount = ethValue;
+            const totalEthSpent = primaryTakerAssetFillAmount.plus(gasPrice.times(tx.gasUsed));
+            // Validate test case
+            expect(makerAssetFillAmount).to.be.bignumber.greaterThan(desiredMakerAssetFillAmount);
+            expect(takerEthBalanceAfter).to.be.bignumber.equal(takerEthBalanceBefore.minus(totalEthSpent));
+            expect(newBalances[makerAddress][defaultMakerAssetAddress]).to.be.bignumber.equal(
+                erc20Balances[makerAddress][defaultMakerAssetAddress].minus(makerAssetFillAmount),
+            );
+            expect(newBalances[takerAddress][defaultMakerAssetAddress]).to.be.bignumber.equal(
+                erc20Balances[takerAddress][defaultMakerAssetAddress].plus(makerAssetFillAmount),
+            );
+            expect(newBalances[makerAddress][weth.address]).to.be.bignumber.equal(
+                erc20Balances[makerAddress][weth.address].plus(primaryTakerAssetFillAmount),
+            );
+            expect(newBalances[forwarderContract.address][weth.address]).to.be.bignumber.equal(constants.ZERO_AMOUNT);
+            expect(newBalances[forwarderContract.address][defaultMakerAssetAddress]).to.be.bignumber.equal(
+                constants.ZERO_AMOUNT,
+            );
+            expect(forwarderEthBalance).to.be.bignumber.equal(constants.ZERO_AMOUNT);
+        });
+        it('Should buy slightly greater MakerAsset when exchange rate is rounded, and MakerAsset is ZRX (Regression Test)', async () => {
+            // Order taken from a transaction on mainnet that failed due to a rounding error.
+            orderWithoutFee = await orderFactory.newSignedOrderAsync({
+                makerAssetAmount: new BigNumber('268166666666666666666'),
+                takerAssetAmount: new BigNumber('219090625878836371'),
+                makerAssetData: zrxAssetData,
+                takerAssetData: assetDataUtils.encodeERC20AssetData(weth.address),
+                makerFee: new BigNumber(0),
+                takerFee: new BigNumber(0),
+            });
+            const ordersWithoutFee = [orderWithoutFee];
+            const feeOrders: SignedOrder[] = [];
+            // The taker will receive more than the desired amount of makerAsset due to rounding
+            const desiredMakerAssetFillAmount = new BigNumber('5000000000000000000');
+            const ethValue = new BigNumber('4084971271824171');
+            const makerAssetFillAmount = ethValue
+                .times(orderWithoutFee.makerAssetAmount)
+                .dividedToIntegerBy(orderWithoutFee.takerAssetAmount);
+            // Execute test case
+            tx = await forwarderWrapper.marketBuyOrdersWithEthAsync(
+                ordersWithoutFee,
+                feeOrders,
+                desiredMakerAssetFillAmount,
+                {
+                    value: ethValue,
+                    from: takerAddress,
+                },
+            );
+            // Fetch end balances and construct expected outputs
+            const takerEthBalanceAfter = await web3Wrapper.getBalanceInWeiAsync(takerAddress);
+            const forwarderEthBalance = await web3Wrapper.getBalanceInWeiAsync(forwarderContract.address);
+            const newBalances = await erc20Wrapper.getBalancesAsync();
+            const primaryTakerAssetFillAmount = ethValue;
+            const totalEthSpent = primaryTakerAssetFillAmount.plus(gasPrice.times(tx.gasUsed));
+            // Validate test case
+            expect(makerAssetFillAmount).to.be.bignumber.greaterThan(desiredMakerAssetFillAmount);
+            expect(takerEthBalanceAfter).to.be.bignumber.equal(takerEthBalanceBefore.minus(totalEthSpent));
+            expect(newBalances[makerAddress][zrxToken.address]).to.be.bignumber.equal(
+                erc20Balances[makerAddress][zrxToken.address].minus(makerAssetFillAmount),
+            );
+            expect(newBalances[takerAddress][zrxToken.address]).to.be.bignumber.equal(
+                erc20Balances[takerAddress][zrxToken.address].plus(makerAssetFillAmount),
+            );
+            expect(newBalances[makerAddress][weth.address]).to.be.bignumber.equal(
+                erc20Balances[makerAddress][weth.address].plus(primaryTakerAssetFillAmount),
+            );
+            expect(newBalances[forwarderContract.address][weth.address]).to.be.bignumber.equal(constants.ZERO_AMOUNT);
+            expect(forwarderEthBalance).to.be.bignumber.equal(constants.ZERO_AMOUNT);
+        });
+        it('Should buy correct MakerAsset when exchange rate is NOT rounded, and MakerAsset is ZRX (Regression Test)', async () => {
+            // An extra unit of TakerAsset was sent to the exchange contract to account for rounding errors, in Forwarder v1.
+            // Specifically, the takerFillAmount was calculated using Floor(desiredMakerAmount * exchangeRate) + 1
+            // We have since changed this to be Ceil(desiredMakerAmount * exchangeRate)
+            // These calculations produce different results when `desiredMakerAmount * exchangeRate` is an integer.
+            //
+            // This test verifies that `ceil` is sufficient:
+            //  Let TakerAssetAmount = MakerAssetAmount * 2
+            //  -> exchangeRate = TakerAssetAmount / MakerAssetAmount = (2*MakerAssetAmount)/MakerAssetAmount = 2
+            //  .: desiredMakerAmount * exchangeRate is an integer.
+            //
+            // Construct test case using values from example above
+            orderWithoutFee = await orderFactory.newSignedOrderAsync({
+                makerAssetAmount: new BigNumber('30'),
+                takerAssetAmount: new BigNumber('60'),
+                makerAssetData: zrxAssetData,
+                takerAssetData: assetDataUtils.encodeERC20AssetData(weth.address),
+                makerFee: new BigNumber(0),
+                takerFee: new BigNumber(0),
+            });
+            const ordersWithoutFee = [orderWithoutFee];
+            const feeOrders: SignedOrder[] = [];
+            const makerAssetFillAmount = new BigNumber('5');
+            const ethValue = new BigNumber('10');
+            // Execute test case
+            tx = await forwarderWrapper.marketBuyOrdersWithEthAsync(ordersWithoutFee, feeOrders, makerAssetFillAmount, {
+                value: ethValue,
+                from: takerAddress,
+            });
+            // Fetch end balances and construct expected outputs
+            const takerEthBalanceAfter = await web3Wrapper.getBalanceInWeiAsync(takerAddress);
+            const forwarderEthBalance = await web3Wrapper.getBalanceInWeiAsync(forwarderContract.address);
+            const newBalances = await erc20Wrapper.getBalancesAsync();
+            const primaryTakerAssetFillAmount = ethValue;
+            const totalEthSpent = primaryTakerAssetFillAmount.plus(gasPrice.times(tx.gasUsed));
+            // Validate test case
+            expect(takerEthBalanceAfter).to.be.bignumber.equal(takerEthBalanceBefore.minus(totalEthSpent));
+            expect(newBalances[makerAddress][zrxToken.address]).to.be.bignumber.equal(
+                erc20Balances[makerAddress][zrxToken.address].minus(makerAssetFillAmount),
+            );
+            expect(newBalances[takerAddress][zrxToken.address]).to.be.bignumber.equal(
+                erc20Balances[takerAddress][zrxToken.address].plus(makerAssetFillAmount),
+            );
+            expect(newBalances[makerAddress][weth.address]).to.be.bignumber.equal(
+                erc20Balances[makerAddress][weth.address].plus(primaryTakerAssetFillAmount),
+            );
+            expect(newBalances[forwarderContract.address][weth.address]).to.be.bignumber.equal(constants.ZERO_AMOUNT);
+            expect(forwarderEthBalance).to.be.bignumber.equal(constants.ZERO_AMOUNT);
+        });
     });
     describe('marketBuyOrdersWithEth with extra fees', () => {
         it('should buy an asset and send fee to feeRecipient', async () => {
diff --git a/packages/contracts/test/utils/forwarder_wrapper.ts b/packages/contracts/test/utils/forwarder_wrapper.ts
index f1a64d47d..a0bfcfe1d 100644
--- a/packages/contracts/test/utils/forwarder_wrapper.ts
+++ b/packages/contracts/test/utils/forwarder_wrapper.ts
@@ -26,9 +26,12 @@ export class ForwarderWrapper {
         _.forEach(feeOrders, feeOrder => {
             const feeAvailable = feeOrder.makerAssetAmount.minus(feeOrder.takerFee);
             if (!remainingFeeAmount.isZero() && feeAvailable.gt(remainingFeeAmount)) {
-                wethAmount = wethAmount
-                    .plus(feeOrder.takerAssetAmount.times(remainingFeeAmount).dividedToIntegerBy(feeAvailable))
-                    .plus(1);
+                wethAmount = wethAmount.plus(
+                    feeOrder.takerAssetAmount
+                        .times(remainingFeeAmount)
+                        .dividedBy(feeAvailable)
+                        .ceil(),
+                );
                 remainingFeeAmount = new BigNumber(0);
             } else if (!remainingFeeAmount.isZero()) {
                 wethAmount = wethAmount.plus(feeOrder.takerAssetAmount);