@0x/contracts-exchange: Round up in marketBuyOrdersNoThrow() so `…

…marketBuyOrdersFillOrKill()` doesn't throw up.

contracts/exchange/CHANGELOG.json

@@ -9,6 +9,10 @@
             {
                 "note": "Introduced new export ExchangeRevertErrors",
                 "pr": 2321
+            },
+            {
+                "note": "Round up in `marketBuyOrdersNoThrow()` so `marketBuyOrdersFillOrKill()` doesn't throw up.",
+                "pr": 2338
             }
         ]
     },

contracts/exchange/contracts/src/MixinWrapperFunctions.sol

@@ -205,7 +205,7 @@ contract MixinWrapperFunctions is
 
             // 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 = LibMath.getPartialAmountFloor(
+            uint256 remainingTakerAssetFillAmount = LibMath.getPartialAmountCeil(
                 orders[i].takerAssetAmount,
                 orders[i].makerAssetAmount,
                 remainingMakerAssetFillAmount

contracts/exchange/contracts/test/TestFillRounding.sol

@@ -0,0 +1,61 @@
+/*
+
+  Copyright 2019 ZeroEx Intl.
+
+  Licensed under the Apache License, Version 2.0 (the "License");
+  you may not use this file except in compliance with the License.
+  You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+
+*/
+
+pragma solidity ^0.5.5;
+pragma experimental ABIEncoderV2;
+
+import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol";
+import "@0x/contracts-exchange-libs/contracts/src/LibFillResults.sol";
+import "../src/Exchange.sol";
+
+
+// Exchange contract with settlement disabled so we can just check `_fillOrder()``
+// calculations.
+contract TestFillRounding is
+    Exchange
+{
+    // solhint-disable no-empty-blocks
+    constructor ()
+        public
+        // Initialize the exchange with a fixed chainId ("test" in hex).
+        Exchange(0x74657374)
+    {}
+
+    function _settleOrder(
+        bytes32 orderHash,
+        LibOrder.Order memory order,
+        address takerAddress,
+        LibFillResults.FillResults memory fillResults
+    )
+        internal
+    {
+        // No-op.
+    }
+
+    function _assertFillableOrder(
+        LibOrder.Order memory order,
+        LibOrder.OrderInfo memory orderInfo,
+        address takerAddress,
+        bytes memory signature
+    )
+        internal
+        view
+    {
+        // No-op.
+    }
+}

contracts/exchange/package.json

@@ -38,7 +38,7 @@
     "config": {
         "publicInterfaceContracts": "Exchange,IExchange",
         "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually.",
-        "abis": "./test/generated-artifacts/@(Exchange|IAssetProxy|IAssetProxyDispatcher|IEIP1271Data|IEIP1271Wallet|IExchange|IExchangeCore|IMatchOrders|IProtocolFees|ISignatureValidator|ITransactions|ITransferSimulator|IWallet|IWrapperFunctions|IsolatedExchange|LibExchangeRichErrorDecoder|MixinAssetProxyDispatcher|MixinExchangeCore|MixinMatchOrders|MixinProtocolFees|MixinSignatureValidator|MixinTransactions|MixinTransferSimulator|MixinWrapperFunctions|ReentrancyTester|TestAssetProxyDispatcher|TestExchangeInternals|TestLibExchangeRichErrorDecoder|TestProtocolFeeCollector|TestProtocolFees|TestProtocolFeesReceiver|TestSignatureValidator|TestTransactions|TestValidatorWallet|TestWrapperFunctions).json"
+        "abis": "./test/generated-artifacts/@(Exchange|IAssetProxy|IAssetProxyDispatcher|IEIP1271Data|IEIP1271Wallet|IExchange|IExchangeCore|IMatchOrders|IProtocolFees|ISignatureValidator|ITransactions|ITransferSimulator|IWallet|IWrapperFunctions|IsolatedExchange|LibExchangeRichErrorDecoder|MixinAssetProxyDispatcher|MixinExchangeCore|MixinMatchOrders|MixinProtocolFees|MixinSignatureValidator|MixinTransactions|MixinTransferSimulator|MixinWrapperFunctions|ReentrancyTester|TestAssetProxyDispatcher|TestExchangeInternals|TestFillRounding|TestLibExchangeRichErrorDecoder|TestProtocolFeeCollector|TestProtocolFees|TestProtocolFeesReceiver|TestSignatureValidator|TestTransactions|TestValidatorWallet|TestWrapperFunctions).json"
     },
     "repository": {
         "type": "git",

contracts/exchange/test/artifacts.ts

@@ -32,6 +32,7 @@ import * as MixinWrapperFunctions from '../test/generated-artifacts/MixinWrapper
 import * as ReentrancyTester from '../test/generated-artifacts/ReentrancyTester.json';
 import * as TestAssetProxyDispatcher from '../test/generated-artifacts/TestAssetProxyDispatcher.json';
 import * as TestExchangeInternals from '../test/generated-artifacts/TestExchangeInternals.json';
+import * as TestFillRounding from '../test/generated-artifacts/TestFillRounding.json';
 import * as TestLibExchangeRichErrorDecoder from '../test/generated-artifacts/TestLibExchangeRichErrorDecoder.json';
 import * as TestProtocolFeeCollector from '../test/generated-artifacts/TestProtocolFeeCollector.json';
 import * as TestProtocolFees from '../test/generated-artifacts/TestProtocolFees.json';
@@ -68,6 +69,7 @@ export const artifacts = {
     ReentrancyTester: ReentrancyTester as ContractArtifact,
     TestAssetProxyDispatcher: TestAssetProxyDispatcher as ContractArtifact,
     TestExchangeInternals: TestExchangeInternals as ContractArtifact,
+    TestFillRounding: TestFillRounding as ContractArtifact,
     TestLibExchangeRichErrorDecoder: TestLibExchangeRichErrorDecoder as ContractArtifact,
     TestProtocolFeeCollector: TestProtocolFeeCollector as ContractArtifact,
     TestProtocolFees: TestProtocolFees as ContractArtifact,

contracts/exchange/test/lib_exchange_rich_error_decoder.ts

@@ -16,7 +16,7 @@ import ExchangeRevertErrors = require('../src/revert_errors');
 import { artifacts } from './artifacts';
 import { TestLibExchangeRichErrorDecoderContract } from './wrappers';
 
-blockchainTests.resets.only('LibExchangeRichErrorDecoder', ({ provider, txDefaults }) => {
+blockchainTests.resets('LibExchangeRichErrorDecoder', ({ provider, txDefaults }) => {
     const ASSET_PROXY_ID_LENGTH = 4;
     const SIGNATURE_LENGTH = 66;
     const ASSET_DATA_LENGTH = 36;

contracts/exchange/test/wrapper_unit_tests.ts

@@ -1,6 +1,14 @@
 import { ContractTxFunctionObj } from '@0x/base-contract';
 import { ReferenceFunctions as LibReferenceFunctions } from '@0x/contracts-exchange-libs';
-import { blockchainTests, constants, describe, expect, hexRandom, orderHashUtils } from '@0x/contracts-test-utils';
+import {
+    blockchainTests,
+    constants,
+    describe,
+    expect,
+    getRandomPortion,
+    hexRandom,
+    orderHashUtils,
+} from '@0x/contracts-test-utils';
 import { ReferenceFunctions as UtilReferenceFunctions, SafeMathRevertErrors } from '@0x/contracts-utils';
 import { FillResults, Order } from '@0x/types';
 import { AnyRevertError, BigNumber, StringRevertError } from '@0x/utils';
@@ -12,6 +20,7 @@ import ExchangeRevertErrors = require('../src/revert_errors');
 
 import { artifacts } from './artifacts';
 import {
+    TestFillRoundingContract,
     TestWrapperFunctionsCancelOrderCalledEventArgs as CancelOrderCalledEventArgs,
     TestWrapperFunctionsContract,
     TestWrapperFunctionsFillOrderCalledEventArgs as FillOrderCalledEventArgs,
@@ -20,7 +29,7 @@ import {
 blockchainTests('Exchange wrapper functions unit tests.', env => {
     const CHAIN_ID = 0x74657374;
     const { ONE_ETHER, MAX_UINT256 } = constants;
-    const { addFillResults, getPartialAmountFloor } = LibReferenceFunctions;
+    const { addFillResults, getPartialAmountCeil } = LibReferenceFunctions;
     const { safeSub } = UtilReferenceFunctions;
     const protocolFeeMultiplier = new BigNumber(150000);
     const randomAddress = () => hexRandom(constants.ADDRESS_LENGTH);
@@ -939,9 +948,9 @@ blockchainTests('Exchange wrapper functions unit tests.', env => {
                 const signatures = _.times(orders.length, i => createOrderSignature(orders[i]));
                 const makerAssetFillAmount = ONE_ETHER;
                 const expectedError = new SafeMathRevertErrors.Uint256BinOpError(
-                    SafeMathRevertErrors.BinOpErrorCodes.DivisionByZero,
-                    orders[0].takerAssetAmount.times(makerAssetFillAmount),
-                    orders[0].makerAssetAmount,
+                    SafeMathRevertErrors.BinOpErrorCodes.SubtractionUnderflow,
+                    constants.ZERO_AMOUNT,
+                    new BigNumber(1),
                 );
                 const tx = getContractFn()(orders, makerAssetFillAmount, signatures).awaitTransactionSuccessAsync();
                 return expect(tx).to.revertWith(expectedError);
@@ -989,7 +998,7 @@ blockchainTests('Exchange wrapper functions unit tests.', env => {
             let fillResults = _.cloneDeep(EMPTY_FILL_RESULTS);
             for (const [order, signature] of _.zip(orders, signatures) as [[Order, string]]) {
                 const remainingMakerAssetFillAmount = safeSub(makerAssetFillAmount, fillResults.makerAssetFilledAmount);
-                const remainingTakerAssetFillAmount = getPartialAmountFloor(
+                const remainingTakerAssetFillAmount = getPartialAmountCeil(
                     order.takerAssetAmount,
                     order.makerAssetAmount,
                     remainingMakerAssetFillAmount,
@@ -1059,6 +1068,93 @@ blockchainTests('Exchange wrapper functions unit tests.', env => {
                 expect(actualResult).to.deep.eq(expectedResult);
                 assertFillOrderCallsFromLogs(receipt.logs, expectedCalls);
             });
+
+            describe('partial fills', () => {
+                let roundingTestContract: TestFillRoundingContract;
+                // Use another test contract with `_fillOrder()` preserved so the
+                // correct fill results are returned and we can test partial fills.
+                // TODO(dorothy-zbornak): Consolidate these contracts into one.
+                before(async () => {
+                    roundingTestContract = await TestFillRoundingContract.deployFrom0xArtifactAsync(
+                        artifacts.TestFillRounding,
+                        env.provider,
+                        {
+                            ...env.txDefaults,
+                        },
+                        {},
+                    );
+                });
+
+                it('small quantities', async () => {
+                    const orders = [
+                        randomOrder({
+                            makerAssetAmount: new BigNumber(30000),
+                            takerAssetAmount: new BigNumber(20000),
+                        }),
+                    ];
+                    const signatures = [hexRandom()];
+                    const fillAmount = new BigNumber(10000);
+                    const fillResults = await roundingTestContract.marketBuyOrdersNoThrow(
+                        orders,
+                        fillAmount,
+                        signatures,
+                    ).callAsync();
+                    expect(fillResults.makerAssetFilledAmount).to.bignumber.eq(10000);
+                });
+
+                it('large quantities', async () => {
+                    const orders = [
+                        randomOrder({
+                            makerAssetAmount: new BigNumber('21400000000000000000'),
+                            takerAssetAmount: new BigNumber('6300000000000000000'),
+                        }),
+                    ];
+                    const signatures = [hexRandom()];
+                    const fillAmount = new BigNumber('2400000000000000000');
+                    const fillResults = await roundingTestContract.marketBuyOrdersNoThrow(
+                        orders,
+                        fillAmount,
+                        signatures,
+                    ).callAsync();
+                    expect(fillResults.makerAssetFilledAmount).to.bignumber.eq('2400000000000000002');
+                });
+
+                it('large full precision quantities', async () => {
+                    const orders = [
+                        randomOrder({
+                            makerAssetAmount: new BigNumber('942848588381848588533'),
+                            takerAssetAmount: new BigNumber('103048102885858024121'),
+                        }),
+                    ];
+                    const signatures = [hexRandom()];
+                    const fillAmount = new BigNumber('84819838457312347759');
+                    const fillResults = await roundingTestContract.marketBuyOrdersNoThrow(
+                        orders,
+                        fillAmount,
+                        signatures,
+                    ).callAsync();
+                    expect(fillResults.makerAssetFilledAmount).to.bignumber.eq('84819838457312347760');
+                });
+
+                describe.optional('full precision fuzzing', () => {
+                    const FUZZ_COUNT = 256;
+                    for (const i of _.times(FUZZ_COUNT)) {
+                        it(`${i + 1}/${FUZZ_COUNT}`, async () => {
+                            const ordersCount = _.random(1, 10);
+                            const orders = _.times(ordersCount, () => randomOrder());
+                            const signatures = orders.map(() => hexRandom());
+                            const totalMakerAssetAmount = BigNumber.sum(...orders.map(o => o.makerAssetAmount));
+                            const fillAmount = getRandomPortion(totalMakerAssetAmount);
+                            const fillResults = await roundingTestContract.marketBuyOrdersNoThrow(
+                                orders,
+                                fillAmount,
+                                signatures,
+                            ).callAsync();
+                            expect(fillResults.makerAssetFilledAmount).to.bignumber.gte(fillAmount);
+                        });
+                    }
+                });
+            });
         });
 
         describe('marketBuyOrdersFillOrKill', () => {

contracts/exchange/test/wrappers.ts

@@ -30,6 +30,7 @@ export * from '../test/generated-wrappers/mixin_wrapper_functions';
 export * from '../test/generated-wrappers/reentrancy_tester';
 export * from '../test/generated-wrappers/test_asset_proxy_dispatcher';
 export * from '../test/generated-wrappers/test_exchange_internals';
+export * from '../test/generated-wrappers/test_fill_rounding';
 export * from '../test/generated-wrappers/test_lib_exchange_rich_error_decoder';
 export * from '../test/generated-wrappers/test_protocol_fee_collector';
 export * from '../test/generated-wrappers/test_protocol_fees';

contracts/exchange/tsconfig.json

@@ -32,6 +32,7 @@
         "test/generated-artifacts/ReentrancyTester.json",
         "test/generated-artifacts/TestAssetProxyDispatcher.json",
         "test/generated-artifacts/TestExchangeInternals.json",
+        "test/generated-artifacts/TestFillRounding.json",
         "test/generated-artifacts/TestLibExchangeRichErrorDecoder.json",
         "test/generated-artifacts/TestProtocolFeeCollector.json",
         "test/generated-artifacts/TestProtocolFees.json",