diff --git a/contracts/exchange/CHANGELOG.json b/contracts/exchange/CHANGELOG.json index 87ab529067..414be2dc11 100644 --- a/contracts/exchange/CHANGELOG.json +++ b/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 } ] }, diff --git a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol index f01e9c084f..66ce4bd65f 100644 --- a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol +++ b/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 diff --git a/contracts/exchange/contracts/test/TestFillRounding.sol b/contracts/exchange/contracts/test/TestFillRounding.sol new file mode 100644 index 0000000000..4e2876b396 --- /dev/null +++ b/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. + } +} diff --git a/contracts/exchange/package.json b/contracts/exchange/package.json index 18aeaac665..3d5b4d7574 100644 --- a/contracts/exchange/package.json +++ b/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", diff --git a/contracts/exchange/test/artifacts.ts b/contracts/exchange/test/artifacts.ts index 8eb424b93b..89075160f7 100644 --- a/contracts/exchange/test/artifacts.ts +++ b/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, diff --git a/contracts/exchange/test/lib_exchange_rich_error_decoder.ts b/contracts/exchange/test/lib_exchange_rich_error_decoder.ts index 61c9ad1f72..b6821c571d 100644 --- a/contracts/exchange/test/lib_exchange_rich_error_decoder.ts +++ b/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; diff --git a/contracts/exchange/test/wrapper_unit_tests.ts b/contracts/exchange/test/wrapper_unit_tests.ts index bcd9b6e06f..96d0327744 100644 --- a/contracts/exchange/test/wrapper_unit_tests.ts +++ b/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,85 @@ 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', () => { diff --git a/contracts/exchange/test/wrappers.ts b/contracts/exchange/test/wrappers.ts index 7daa712abe..d38276dee5 100644 --- a/contracts/exchange/test/wrappers.ts +++ b/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'; diff --git a/contracts/exchange/tsconfig.json b/contracts/exchange/tsconfig.json index 06fbc962d6..b5598e3af5 100644 --- a/contracts/exchange/tsconfig.json +++ b/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",