From 073976de10f90ae45ddb0fff1012a2ac39ec695b Mon Sep 17 00:00:00 2001 From: James Towle Date: Mon, 1 Jul 2019 23:19:42 -0500 Subject: [PATCH] Split up TestExchangeInternals into two contracts --- contracts/exchange/compiler.json | 1 + .../contracts/src/MixinMatchOrders.sol | 4 +- .../contracts/src/interfaces/IMatchOrders.sol | 36 +- .../contracts/test/TestExchangeInternals.sol | 104 ------ .../contracts/test/TestExchangeMath.sol | 131 ++++++++ contracts/exchange/package.json | 2 +- contracts/exchange/src/artifacts.ts | 4 +- contracts/exchange/src/wrappers.ts | 1 + contracts/exchange/test/internal.ts | 318 +++++++++++------- contracts/exchange/test/match_orders.ts | 25 +- contracts/exchange/tsconfig.json | 1 + 11 files changed, 369 insertions(+), 258 deletions(-) create mode 100644 contracts/exchange/contracts/test/TestExchangeMath.sol diff --git a/contracts/exchange/compiler.json b/contracts/exchange/compiler.json index d8491dc8ef..167f1f515a 100644 --- a/contracts/exchange/compiler.json +++ b/contracts/exchange/compiler.json @@ -39,6 +39,7 @@ "test/ReentrantERC20Token.sol", "test/TestAssetProxyDispatcher.sol", "test/TestExchangeInternals.sol", + "test/TestExchangeMath.sol", "test/TestLibExchangeRichErrorDecoder.sol", "test/TestSignatureValidator.sol", "test/TestValidatorWallet.sol" diff --git a/contracts/exchange/contracts/src/MixinMatchOrders.sol b/contracts/exchange/contracts/src/MixinMatchOrders.sol index bbd417c2a3..8df27ceaf8 100644 --- a/contracts/exchange/contracts/src/MixinMatchOrders.sol +++ b/contracts/exchange/contracts/src/MixinMatchOrders.sol @@ -172,6 +172,7 @@ contract MixinMatchOrders is /// @dev Match complementary orders that have a profitable spread. /// Each order is filled at their respective price point, and /// the matcher receives a profit denominated in the left maker asset. + /// This is the reentrant version of `batchMatchOrders` and `batchMatchOrdersWithMaximalFill`. /// @param leftOrders Set of orders with the same maker / taker asset. /// @param rightOrders Set of orders to match against `leftOrders` /// @param leftSignatures Proof that left orders were created by the left makers. @@ -671,7 +672,8 @@ contract MixinMatchOrders is /// Each order is filled at their respective price point. However, the calculations are /// carried out as though the orders are both being filled at the right order's price point. /// The profit made by the left order goes to the taker (who matched the two orders). This - /// function is needed to allow for reentrant order matching (used by batchMatchOrders). + /// function is needed to allow for reentrant order matching (used by `batchMatchOrders` and + /// `batchMatchOrdersWithMaximalFill`). /// @param leftOrder First order to match. /// @param rightOrder Second order to match. /// @param leftSignature Proof that order was created by the left maker. diff --git a/contracts/exchange/contracts/src/interfaces/IMatchOrders.sol b/contracts/exchange/contracts/src/interfaces/IMatchOrders.sol index 08808d8be7..630d551dbe 100644 --- a/contracts/exchange/contracts/src/interfaces/IMatchOrders.sol +++ b/contracts/exchange/contracts/src/interfaces/IMatchOrders.sol @@ -42,24 +42,6 @@ contract IMatchOrders { public returns (LibFillResults.BatchMatchedFillResults memory batchMatchedFillResults); - /// @dev Match two complementary orders that have a profitable spread. - /// Each order is filled at their respective price point. However, the calculations are - /// carried out as though the orders are both being filled at the right order's price point. - /// The profit made by the left order goes to the taker (who matched the two orders). - /// @param leftOrder First order to match. - /// @param rightOrder Second order to match. - /// @param leftSignature Proof that order was created by the left maker. - /// @param rightSignature Proof that order was created by the right maker. - /// @return matchedFillResults Amounts filled and fees paid by maker and taker of matched orders. - function matchOrders( - LibOrder.Order memory leftOrder, - LibOrder.Order memory rightOrder, - bytes memory leftSignature, - bytes memory rightSignature - ) - public - returns (LibFillResults.MatchedFillResults memory matchedFillResults); - /// @dev Calculates fill amounts for the matched orders. /// Each order is filled at their respective price point. However, the calculations are /// carried out as though the orders are both being filled at the right order's price point. @@ -79,6 +61,24 @@ contract IMatchOrders { pure returns (LibFillResults.MatchedFillResults memory matchedFillResults); + /// @dev Match two complementary orders that have a profitable spread. + /// Each order is filled at their respective price point. However, the calculations are + /// carried out as though the orders are both being filled at the right order's price point. + /// The profit made by the left order goes to the taker (who matched the two orders). + /// @param leftOrder First order to match. + /// @param rightOrder Second order to match. + /// @param leftSignature Proof that order was created by the left maker. + /// @param rightSignature Proof that order was created by the right maker. + /// @return matchedFillResults Amounts filled and fees paid by maker and taker of matched orders. + function matchOrders( + LibOrder.Order memory leftOrder, + LibOrder.Order memory rightOrder, + bytes memory leftSignature, + bytes memory rightSignature + ) + public + returns (LibFillResults.MatchedFillResults memory matchedFillResults); + /// @dev Match two complementary orders that have a profitable spread. /// Each order is maximally filled at their respective price point, and /// the matcher receives a profit denominated in either the left maker asset, diff --git a/contracts/exchange/contracts/test/TestExchangeInternals.sol b/contracts/exchange/contracts/test/TestExchangeInternals.sol index 08d58e06c9..f40ca21797 100644 --- a/contracts/exchange/contracts/test/TestExchangeInternals.sol +++ b/contracts/exchange/contracts/test/TestExchangeInternals.sol @@ -62,110 +62,6 @@ contract TestExchangeInternals is return _calculateFillResults(order, takerAssetFilledAmount); } - /// @dev Calculates partial value given a numerator and denominator. - /// Reverts if rounding error is >= 0.1% - /// @param numerator Numerator. - /// @param denominator Denominator. - /// @param target Value to calculate partial of. - /// @return Partial value of target. - function safeGetPartialAmountFloor( - uint256 numerator, - uint256 denominator, - uint256 target - ) - public - pure - returns (uint256 partialAmount) - { - return _safeGetPartialAmountFloor(numerator, denominator, target); - } - - /// @dev Calculates partial value given a numerator and denominator. - /// Reverts if rounding error is >= 0.1% - /// @param numerator Numerator. - /// @param denominator Denominator. - /// @param target Value to calculate partial of. - /// @return Partial value of target. - function safeGetPartialAmountCeil( - uint256 numerator, - uint256 denominator, - uint256 target - ) - public - pure - returns (uint256 partialAmount) - { - return _safeGetPartialAmountCeil(numerator, denominator, target); - } - - /// @dev Calculates partial value given a numerator and denominator. - /// @param numerator Numerator. - /// @param denominator Denominator. - /// @param target Value to calculate partial of. - /// @return Partial value of target. - function getPartialAmountFloor( - uint256 numerator, - uint256 denominator, - uint256 target - ) - public - pure - returns (uint256 partialAmount) - { - return _getPartialAmountFloor(numerator, denominator, target); - } - - /// @dev Calculates partial value given a numerator and denominator. - /// @param numerator Numerator. - /// @param denominator Denominator. - /// @param target Value to calculate partial of. - /// @return Partial value of target. - function getPartialAmountCeil( - uint256 numerator, - uint256 denominator, - uint256 target - ) - public - pure - returns (uint256 partialAmount) - { - return _getPartialAmountCeil(numerator, denominator, target); - } - - /// @dev Checks if rounding error >= 0.1%. - /// @param numerator Numerator. - /// @param denominator Denominator. - /// @param target Value to multiply with numerator/denominator. - /// @return Rounding error is present. - function isRoundingErrorFloor( - uint256 numerator, - uint256 denominator, - uint256 target - ) - public - pure - returns (bool isError) - { - return _isRoundingErrorFloor(numerator, denominator, target); - } - - /// @dev Checks if rounding error >= 0.1%. - /// @param numerator Numerator. - /// @param denominator Denominator. - /// @param target Value to multiply with numerator/denominator. - /// @return Rounding error is present. - function isRoundingErrorCeil( - uint256 numerator, - uint256 denominator, - uint256 target - ) - public - pure - returns (bool isError) - { - return _isRoundingErrorCeil(numerator, denominator, target); - } - /// @dev Updates state with results of a fill order. /// @param order that was filled. /// @param takerAddress Address of taker who filled the order. diff --git a/contracts/exchange/contracts/test/TestExchangeMath.sol b/contracts/exchange/contracts/test/TestExchangeMath.sol new file mode 100644 index 0000000000..832e4c5db7 --- /dev/null +++ b/contracts/exchange/contracts/test/TestExchangeMath.sol @@ -0,0 +1,131 @@ +/* + + Copyright 2018 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.9; +pragma experimental ABIEncoderV2; + +import "@0x/contracts-exchange-libs/contracts/src/LibMath.sol"; + + +contract TestExchangeMath is + LibMath +{ + /// @dev Calculates partial value given a numerator and denominator. + /// Reverts if rounding error is >= 0.1% + /// @param numerator Numerator. + /// @param denominator Denominator. + /// @param target Value to calculate partial of. + /// @return Partial value of target. + function safeGetPartialAmountFloor( + uint256 numerator, + uint256 denominator, + uint256 target + ) + public + pure + returns (uint256 partialAmount) + { + return _safeGetPartialAmountFloor(numerator, denominator, target); + } + + /// @dev Calculates partial value given a numerator and denominator. + /// Reverts if rounding error is >= 0.1% + /// @param numerator Numerator. + /// @param denominator Denominator. + /// @param target Value to calculate partial of. + /// @return Partial value of target. + function safeGetPartialAmountCeil( + uint256 numerator, + uint256 denominator, + uint256 target + ) + public + pure + returns (uint256 partialAmount) + { + return _safeGetPartialAmountCeil(numerator, denominator, target); + } + + /// @dev Calculates partial value given a numerator and denominator. + /// @param numerator Numerator. + /// @param denominator Denominator. + /// @param target Value to calculate partial of. + /// @return Partial value of target. + function getPartialAmountFloor( + uint256 numerator, + uint256 denominator, + uint256 target + ) + public + pure + returns (uint256 partialAmount) + { + return _getPartialAmountFloor(numerator, denominator, target); + } + + /// @dev Calculates partial value given a numerator and denominator. + /// @param numerator Numerator. + /// @param denominator Denominator. + /// @param target Value to calculate partial of. + /// @return Partial value of target. + function getPartialAmountCeil( + uint256 numerator, + uint256 denominator, + uint256 target + ) + public + pure + returns (uint256 partialAmount) + { + return _getPartialAmountCeil(numerator, denominator, target); + } + + /// @dev Checks if rounding error >= 0.1%. + /// @param numerator Numerator. + /// @param denominator Denominator. + /// @param target Value to multiply with numerator/denominator. + /// @return Rounding error is present. + function isRoundingErrorFloor( + uint256 numerator, + uint256 denominator, + uint256 target + ) + public + pure + returns (bool isError) + { + return _isRoundingErrorFloor(numerator, denominator, target); + } + + /// @dev Checks if rounding error >= 0.1%. + /// @param numerator Numerator. + /// @param denominator Denominator. + /// @param target Value to multiply with numerator/denominator. + /// @return Rounding error is present. + function isRoundingErrorCeil( + uint256 numerator, + uint256 denominator, + uint256 target + ) + public + pure + returns (bool isError) + { + return _isRoundingErrorCeil(numerator, denominator, target); + } +} diff --git a/contracts/exchange/package.json b/contracts/exchange/package.json index 33f6e505c7..83dc128500 100644 --- a/contracts/exchange/package.json +++ b/contracts/exchange/package.json @@ -34,7 +34,7 @@ "lint-contracts": "solhint -c ../.solhint.json contracts/**/**/**/**/*.sol" }, "config": { - "abis": "./generated-artifacts/@(Exchange|ExchangeWrapper|IAssetProxyDispatcher|IEIP1271Wallet|IExchange|IExchangeCore|IMatchOrders|ISignatureValidator|ITransactions|IWallet|IWrapperFunctions|ReentrantERC20Token|TestAssetProxyDispatcher|TestExchangeInternals|TestLibExchangeRichErrorDecoder|TestSignatureValidator|TestValidatorWallet|Whitelist).json", + "abis": "./generated-artifacts/@(Exchange|ExchangeWrapper|IAssetProxyDispatcher|IEIP1271Wallet|IExchange|IExchangeCore|IMatchOrders|ISignatureValidator|ITransactions|IWallet|IWrapperFunctions|ReentrantERC20Token|TestAssetProxyDispatcher|TestExchangeInternals|TestExchangeMath|TestLibExchangeRichErrorDecoder|TestSignatureValidator|TestValidatorWallet|Whitelist).json", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually." }, "repository": { diff --git a/contracts/exchange/src/artifacts.ts b/contracts/exchange/src/artifacts.ts index 6da04da37f..b0aa72ec35 100644 --- a/contracts/exchange/src/artifacts.ts +++ b/contracts/exchange/src/artifacts.ts @@ -19,6 +19,7 @@ import * as IWrapperFunctions from '../generated-artifacts/IWrapperFunctions.jso import * as ReentrantERC20Token from '../generated-artifacts/ReentrantERC20Token.json'; import * as TestAssetProxyDispatcher from '../generated-artifacts/TestAssetProxyDispatcher.json'; import * as TestExchangeInternals from '../generated-artifacts/TestExchangeInternals.json'; +import * as TestExchangeMath from '../generated-artifacts/TestExchangeMath.json'; import * as TestLibExchangeRichErrorDecoder from '../generated-artifacts/TestLibExchangeRichErrorDecoder.json'; import * as TestSignatureValidator from '../generated-artifacts/TestSignatureValidator.json'; import * as TestValidatorWallet from '../generated-artifacts/TestValidatorWallet.json'; @@ -28,17 +29,18 @@ export const artifacts = { Whitelist: Whitelist as ContractArtifact, Exchange: Exchange as ContractArtifact, IAssetProxyDispatcher: IAssetProxyDispatcher as ContractArtifact, + IEIP1271Wallet: IEIP1271Wallet as ContractArtifact, IExchange: IExchange as ContractArtifact, IExchangeCore: IExchangeCore as ContractArtifact, IMatchOrders: IMatchOrders as ContractArtifact, ISignatureValidator: ISignatureValidator as ContractArtifact, ITransactions: ITransactions as ContractArtifact, IWallet: IWallet as ContractArtifact, - IEIP1271Wallet: IEIP1271Wallet as ContractArtifact, IWrapperFunctions: IWrapperFunctions as ContractArtifact, ReentrantERC20Token: ReentrantERC20Token as ContractArtifact, TestAssetProxyDispatcher: TestAssetProxyDispatcher as ContractArtifact, TestExchangeInternals: TestExchangeInternals as ContractArtifact, + TestExchangeMath: TestExchangeMath as ContractArtifact, TestLibExchangeRichErrorDecoder: TestLibExchangeRichErrorDecoder as ContractArtifact, TestSignatureValidator: TestSignatureValidator as ContractArtifact, TestValidatorWallet: TestValidatorWallet as ContractArtifact, diff --git a/contracts/exchange/src/wrappers.ts b/contracts/exchange/src/wrappers.ts index fc725b0fcf..719df58807 100644 --- a/contracts/exchange/src/wrappers.ts +++ b/contracts/exchange/src/wrappers.ts @@ -17,6 +17,7 @@ export * from '../generated-wrappers/i_wrapper_functions'; export * from '../generated-wrappers/reentrant_erc20_token'; export * from '../generated-wrappers/test_asset_proxy_dispatcher'; export * from '../generated-wrappers/test_exchange_internals'; +export * from '../generated-wrappers/test_exchange_math'; export * from '../generated-wrappers/test_lib_exchange_rich_error_decoder'; export * from '../generated-wrappers/test_signature_validator'; export * from '../generated-wrappers/test_validator_wallet'; diff --git a/contracts/exchange/test/internal.ts b/contracts/exchange/test/internal.ts index 9d2ecd1f3f..48aec23621 100644 --- a/contracts/exchange/test/internal.ts +++ b/contracts/exchange/test/internal.ts @@ -16,7 +16,7 @@ import { BigNumber, providerUtils, SafeMathRevertErrors } from '@0x/utils'; import * as chai from 'chai'; import * as _ from 'lodash'; -import { artifacts, TestExchangeInternalsContract } from '../src'; +import { artifacts, TestExchangeInternalsContract, TestExchangeMathContract } from '../src'; chaiSetup.configure(); const expect = chai.expect; @@ -53,10 +53,9 @@ const emptySignedOrder: SignedOrder = { const safeMathErrorForCall = new SafeMathRevertErrors.SafeMathError(); -describe('Exchange core internal functions', () => { +describe('Exchange math internal functions', () => { let chainId: number; - let testExchange: TestExchangeInternalsContract; - let safeMathErrorForSendTransaction: Error | undefined; + let testExchange: TestExchangeMathContract; let divisionByZeroErrorForCall: Error | undefined; let roundingErrorForCall: Error | undefined; @@ -71,15 +70,13 @@ describe('Exchange core internal functions', () => { emptyOrder.domain.chainId = chainId; emptySignedOrder.domain.chainId = chainId; - testExchange = await TestExchangeInternalsContract.deployFrom0xArtifactAsync( - artifacts.TestExchangeInternals, + testExchange = await TestExchangeMathContract.deployFrom0xArtifactAsync( + artifacts.TestExchangeMath, provider, txDefaults, - new BigNumber(chainId), ); divisionByZeroErrorForCall = new Error(RevertReason.DivisionByZero); roundingErrorForCall = new Error(RevertReason.RoundingError); - safeMathErrorForSendTransaction = safeMathErrorForCall; divisionByZeroErrorForCall = new LibMathRevertErrors.DivisionByZeroError(); roundingErrorForCall = new LibMathRevertErrors.RoundingError(); }); @@ -160,118 +157,6 @@ describe('Exchange core internal functions', () => { return product.dividedToIntegerBy(denominator); } - describe('addFillResults', async () => { - function makeFillResults(value: BigNumber): FillResults { - return { - makerAssetFilledAmount: value, - takerAssetFilledAmount: value, - makerFeePaid: value, - takerFeePaid: value, - }; - } - async function referenceAddFillResultsAsync( - totalValue: BigNumber, - singleValue: BigNumber, - ): Promise { - // Note(albrow): Here, each of totalFillResults and - // singleFillResults will consist of fields with the same values. - // This should be safe because none of the fields in a given - // FillResults are ever used together in a mathemetical operation. - // They are only used with the corresponding field from *the other* - // FillResults, which are different. - const totalFillResults = makeFillResults(totalValue); - const singleFillResults = makeFillResults(singleValue); - // HACK(albrow): _.mergeWith mutates the first argument! To - // workaround this we use _.cloneDeep. - return _.mergeWith( - _.cloneDeep(totalFillResults), - singleFillResults, - (totalVal: BigNumber, singleVal: BigNumber) => { - const newTotal = totalVal.plus(singleVal); - if (newTotal.isGreaterThan(MAX_UINT256)) { - throw safeMathErrorForCall; - } - return newTotal; - }, - ); - } - async function testAddFillResultsAsync(totalValue: BigNumber, singleValue: BigNumber): Promise { - const totalFillResults = makeFillResults(totalValue); - const singleFillResults = makeFillResults(singleValue); - return testExchange.addFillResults.callAsync(totalFillResults, singleFillResults); - } - await testCombinatoriallyWithReferenceFuncAsync( - 'addFillResults', - referenceAddFillResultsAsync, - testAddFillResultsAsync, - [uint256Values, uint256Values], - ); - }); - - describe('calculateFillResults', async () => { - function makeOrder( - makerAssetAmount: BigNumber, - takerAssetAmount: BigNumber, - makerFee: BigNumber, - takerFee: BigNumber, - ): Order { - return { - ...emptyOrder, - makerAssetAmount, - takerAssetAmount, - makerFee, - takerFee, - }; - } - async function referenceCalculateFillResultsAsync( - orderTakerAssetAmount: BigNumber, - takerAssetFilledAmount: BigNumber, - otherAmount: BigNumber, - ): Promise { - // Note(albrow): Here we are re-using the same value (otherAmount) - // for order.makerAssetAmount, order.makerFee, and order.takerFee. - // This should be safe because they are never used with each other - // in any mathematical operation in either the reference TypeScript - // implementation or the Solidity implementation of - // calculateFillResults. - const makerAssetFilledAmount = await referenceSafeGetPartialAmountFloorAsync( - takerAssetFilledAmount, - orderTakerAssetAmount, - otherAmount, - ); - const order = makeOrder(otherAmount, orderTakerAssetAmount, otherAmount, otherAmount); - const orderMakerAssetAmount = order.makerAssetAmount; - return { - makerAssetFilledAmount, - takerAssetFilledAmount, - makerFeePaid: await referenceSafeGetPartialAmountFloorAsync( - makerAssetFilledAmount, - orderMakerAssetAmount, - otherAmount, - ), - takerFeePaid: await referenceSafeGetPartialAmountFloorAsync( - takerAssetFilledAmount, - orderTakerAssetAmount, - otherAmount, - ), - }; - } - async function testCalculateFillResultsAsync( - orderTakerAssetAmount: BigNumber, - takerAssetFilledAmount: BigNumber, - otherAmount: BigNumber, - ): Promise { - const order = makeOrder(otherAmount, orderTakerAssetAmount, otherAmount, otherAmount); - return testExchange.calculateFillResults.callAsync(order, takerAssetFilledAmount); - } - await testCombinatoriallyWithReferenceFuncAsync( - 'calculateFillResults', - referenceCalculateFillResultsAsync, - testCalculateFillResultsAsync, - [uint256Values, uint256Values, uint256Values], - ); - }); - describe('getPartialAmountFloor', async () => { async function referenceGetPartialAmountFloorAsync( numerator: BigNumber, @@ -427,6 +312,198 @@ describe('Exchange core internal functions', () => { [uint256Values, uint256Values, uint256Values], ); }); +}); + +describe('Exchange core internal functions', () => { + let chainId: number; + let testExchange: TestExchangeInternalsContract; + let safeMathErrorForSendTransaction: Error | undefined; + let divisionByZeroErrorForCall: Error | undefined; + let roundingErrorForCall: Error | undefined; + + before(async () => { + await blockchainLifecycle.startAsync(); + }); + after(async () => { + await blockchainLifecycle.revertAsync(); + }); + before(async () => { + chainId = await providerUtils.getChainIdAsync(provider); + emptyOrder.domain.chainId = chainId; + emptySignedOrder.domain.chainId = chainId; + + testExchange = await TestExchangeInternalsContract.deployFrom0xArtifactAsync( + artifacts.TestExchangeInternals, + provider, + txDefaults, + new BigNumber(chainId), + ); + divisionByZeroErrorForCall = new Error(RevertReason.DivisionByZero); + roundingErrorForCall = new Error(RevertReason.RoundingError); + safeMathErrorForSendTransaction = safeMathErrorForCall; + divisionByZeroErrorForCall = new LibMathRevertErrors.DivisionByZeroError(); + roundingErrorForCall = new LibMathRevertErrors.RoundingError(); + }); + // Note(albrow): Don't forget to add beforeEach and afterEach calls to reset + // the blockchain state for any tests which modify it! + + async function referenceIsRoundingErrorFloorAsync( + numerator: BigNumber, + denominator: BigNumber, + target: BigNumber, + ): Promise { + if (denominator.eq(0)) { + throw divisionByZeroErrorForCall; + } + if (numerator.eq(0)) { + return false; + } + if (target.eq(0)) { + return false; + } + const product = numerator.multipliedBy(target); + const remainder = product.mod(denominator); + const remainderTimes1000 = remainder.multipliedBy('1000'); + const isError = remainderTimes1000.gte(product); + if (product.isGreaterThan(MAX_UINT256)) { + throw safeMathErrorForCall; + } + if (remainderTimes1000.isGreaterThan(MAX_UINT256)) { + throw safeMathErrorForCall; + } + return isError; + } + + async function referenceSafeGetPartialAmountFloorAsync( + numerator: BigNumber, + denominator: BigNumber, + target: BigNumber, + ): Promise { + if (denominator.eq(0)) { + throw divisionByZeroErrorForCall; + } + const isRoundingError = await referenceIsRoundingErrorFloorAsync(numerator, denominator, target); + if (isRoundingError) { + throw roundingErrorForCall; + } + const product = numerator.multipliedBy(target); + if (product.isGreaterThan(MAX_UINT256)) { + throw safeMathErrorForCall; + } + return product.dividedToIntegerBy(denominator); + } + + describe('addFillResults', async () => { + function makeFillResults(value: BigNumber): FillResults { + return { + makerAssetFilledAmount: value, + takerAssetFilledAmount: value, + makerFeePaid: value, + takerFeePaid: value, + }; + } + async function referenceAddFillResultsAsync( + totalValue: BigNumber, + singleValue: BigNumber, + ): Promise { + // Note(albrow): Here, each of totalFillResults and + // singleFillResults will consist of fields with the same values. + // This should be safe because none of the fields in a given + // FillResults are ever used together in a mathemetical operation. + // They are only used with the corresponding field from *the other* + // FillResults, which are different. + const totalFillResults = makeFillResults(totalValue); + const singleFillResults = makeFillResults(singleValue); + // HACK(albrow): _.mergeWith mutates the first argument! To + // workaround this we use _.cloneDeep. + return _.mergeWith( + _.cloneDeep(totalFillResults), + singleFillResults, + (totalVal: BigNumber, singleVal: BigNumber) => { + const newTotal = totalVal.plus(singleVal); + if (newTotal.isGreaterThan(MAX_UINT256)) { + throw safeMathErrorForCall; + } + return newTotal; + }, + ); + } + async function testAddFillResultsAsync(totalValue: BigNumber, singleValue: BigNumber): Promise { + const totalFillResults = makeFillResults(totalValue); + const singleFillResults = makeFillResults(singleValue); + return testExchange.addFillResults.callAsync(totalFillResults, singleFillResults); + } + await testCombinatoriallyWithReferenceFuncAsync( + 'addFillResults', + referenceAddFillResultsAsync, + testAddFillResultsAsync, + [uint256Values, uint256Values], + ); + }); + + describe('calculateFillResults', async () => { + function makeOrder( + makerAssetAmount: BigNumber, + takerAssetAmount: BigNumber, + makerFee: BigNumber, + takerFee: BigNumber, + ): Order { + return { + ...emptyOrder, + makerAssetAmount, + takerAssetAmount, + makerFee, + takerFee, + }; + } + async function referenceCalculateFillResultsAsync( + orderTakerAssetAmount: BigNumber, + takerAssetFilledAmount: BigNumber, + otherAmount: BigNumber, + ): Promise { + // Note(albrow): Here we are re-using the same value (otherAmount) + // for order.makerAssetAmount, order.makerFee, and order.takerFee. + // This should be safe because they are never used with each other + // in any mathematical operation in either the reference TypeScript + // implementation or the Solidity implementation of + // calculateFillResults. + const makerAssetFilledAmount = await referenceSafeGetPartialAmountFloorAsync( + takerAssetFilledAmount, + orderTakerAssetAmount, + otherAmount, + ); + const order = makeOrder(otherAmount, orderTakerAssetAmount, otherAmount, otherAmount); + const orderMakerAssetAmount = order.makerAssetAmount; + return { + makerAssetFilledAmount, + takerAssetFilledAmount, + makerFeePaid: await referenceSafeGetPartialAmountFloorAsync( + makerAssetFilledAmount, + orderMakerAssetAmount, + otherAmount, + ), + takerFeePaid: await referenceSafeGetPartialAmountFloorAsync( + takerAssetFilledAmount, + orderTakerAssetAmount, + otherAmount, + ), + }; + } + async function testCalculateFillResultsAsync( + orderTakerAssetAmount: BigNumber, + takerAssetFilledAmount: BigNumber, + otherAmount: BigNumber, + ): Promise { + const order = makeOrder(otherAmount, orderTakerAssetAmount, otherAmount, otherAmount); + return testExchange.calculateFillResults.callAsync(order, takerAssetFilledAmount); + } + await testCombinatoriallyWithReferenceFuncAsync( + 'calculateFillResults', + referenceCalculateFillResultsAsync, + testCalculateFillResultsAsync, + [uint256Values, uint256Values, uint256Values], + ); + }); describe('updateFilledState', async () => { // Note(albrow): Since updateFilledState modifies the state by calling @@ -481,3 +558,4 @@ describe('Exchange core internal functions', () => { ); }); }); +// tslint:disable-line:max-file-line-count diff --git a/contracts/exchange/test/match_orders.ts b/contracts/exchange/test/match_orders.ts index ac85fd6ff9..63e899b5b2 100644 --- a/contracts/exchange/test/match_orders.ts +++ b/contracts/exchange/test/match_orders.ts @@ -26,7 +26,7 @@ import { ExchangeContract, ExchangeWrapper, ReentrantERC20TokenContract, - TestExchangeInternalsContract, + TestExchangeMathContract, } from '../src'; import { MatchOrderTester, TokenBalances } from './utils/match_order_tester'; @@ -80,7 +80,7 @@ describe('matchOrders', () => { let matchOrderTester: MatchOrderTester; - let testExchange: TestExchangeInternalsContract; + let testExchangeMath: TestExchangeMathContract; before(async () => { await blockchainLifecycle.startAsync(); @@ -232,11 +232,10 @@ describe('matchOrders', () => { orderFactoryLeft = new OrderFactory(privateKeyLeft, defaultOrderParamsLeft); const privateKeyRight = constants.TESTRPC_PRIVATE_KEYS[accounts.indexOf(makerAddressRight)]; orderFactoryRight = new OrderFactory(privateKeyRight, defaultOrderParamsRight); - testExchange = await TestExchangeInternalsContract.deployFrom0xArtifactAsync( - artifacts.TestExchangeInternals, + testExchangeMath = await TestExchangeMathContract.deployFrom0xArtifactAsync( + artifacts.TestExchangeMath, provider, txDefaults, - new BigNumber(chainId), ); // Create match order tester matchOrderTester = new MatchOrderTester(exchangeWrapper, erc20Wrapper, erc721Wrapper, erc1155ProxyWrapper); @@ -269,13 +268,13 @@ describe('matchOrders', () => { const numerator = signedOrderLeft.makerAssetAmount; const denominator = signedOrderLeft.takerAssetAmount; const target = signedOrderRight.makerAssetAmount; - const isRoundingErrorCeil = await testExchange.isRoundingErrorCeil.callAsync( + const isRoundingErrorCeil = await testExchangeMath.isRoundingErrorCeil.callAsync( numerator, denominator, target, ); expect(isRoundingErrorCeil).to.be.true(); - const isRoundingErrorFloor = await testExchange.isRoundingErrorFloor.callAsync( + const isRoundingErrorFloor = await testExchangeMath.isRoundingErrorFloor.callAsync( numerator, denominator, target, @@ -335,13 +334,13 @@ describe('matchOrders', () => { const numerator = signedOrderRight.takerAssetAmount; const denominator = signedOrderRight.makerAssetAmount; const target = signedOrderLeft.takerAssetAmount; - const isRoundingErrorFloor = await testExchange.isRoundingErrorFloor.callAsync( + const isRoundingErrorFloor = await testExchangeMath.isRoundingErrorFloor.callAsync( numerator, denominator, target, ); expect(isRoundingErrorFloor).to.be.true(); - const isRoundingErrorCeil = await testExchange.isRoundingErrorCeil.callAsync( + const isRoundingErrorCeil = await testExchangeMath.isRoundingErrorCeil.callAsync( numerator, denominator, target, @@ -1897,13 +1896,13 @@ describe('matchOrders', () => { const numerator = signedOrderLeft.makerAssetAmount; const denominator = signedOrderLeft.takerAssetAmount; const target = signedOrderRight.makerAssetAmount; - const isRoundingErrorCeil = await testExchange.isRoundingErrorCeil.callAsync( + const isRoundingErrorCeil = await testExchangeMath.isRoundingErrorCeil.callAsync( numerator, denominator, target, ); expect(isRoundingErrorCeil).to.be.true(); - const isRoundingErrorFloor = await testExchange.isRoundingErrorFloor.callAsync( + const isRoundingErrorFloor = await testExchangeMath.isRoundingErrorFloor.callAsync( numerator, denominator, target, @@ -1963,13 +1962,13 @@ describe('matchOrders', () => { const numerator = signedOrderRight.makerAssetAmount; const denominator = signedOrderRight.takerAssetAmount; const target = signedOrderLeft.makerAssetAmount; - const isRoundingErrorCeil = await testExchange.isRoundingErrorCeil.callAsync( + const isRoundingErrorCeil = await testExchangeMath.isRoundingErrorCeil.callAsync( numerator, denominator, target, ); expect(isRoundingErrorCeil).to.be.false(); - const isRoundingErrorFloor = await testExchange.isRoundingErrorFloor.callAsync( + const isRoundingErrorFloor = await testExchangeMath.isRoundingErrorFloor.callAsync( numerator, denominator, target, diff --git a/contracts/exchange/tsconfig.json b/contracts/exchange/tsconfig.json index 6f878d304c..6d8ad9c993 100644 --- a/contracts/exchange/tsconfig.json +++ b/contracts/exchange/tsconfig.json @@ -17,6 +17,7 @@ "generated-artifacts/ReentrantERC20Token.json", "generated-artifacts/TestAssetProxyDispatcher.json", "generated-artifacts/TestExchangeInternals.json", + "generated-artifacts/TestExchangeMath.json", "generated-artifacts/TestLibExchangeRichErrorDecoder.json", "generated-artifacts/TestSignatureValidator.json", "generated-artifacts/TestValidatorWallet.json",