diff --git a/contracts/exchange/CHANGELOG.json b/contracts/exchange/CHANGELOG.json index abdb2c003a..741bd115f7 100644 --- a/contracts/exchange/CHANGELOG.json +++ b/contracts/exchange/CHANGELOG.json @@ -133,6 +133,10 @@ { "note": "Remove functions from `TestExchangeInternals.sol` that are no longer tested in this package.", "pr": "TODO" + }, + { + "note": "Remove `_assertValidFill()`", + "pr": "TODO" } ] }, diff --git a/contracts/exchange/contracts/src/MixinExchangeCore.sol b/contracts/exchange/contracts/src/MixinExchangeCore.sol index d0b082e213..844419ec70 100644 --- a/contracts/exchange/contracts/src/MixinExchangeCore.sol +++ b/contracts/exchange/contracts/src/MixinExchangeCore.sol @@ -210,15 +210,6 @@ contract MixinExchangeCore is uint256 remainingTakerAssetAmount = _safeSub(order.takerAssetAmount, orderInfo.orderTakerAssetFilledAmount); uint256 takerAssetFilledAmount = _min256(takerAssetFillAmount, remainingTakerAssetAmount); - // Validate context - _assertValidFill( - order, - orderInfo, - takerAssetFillAmount, - takerAssetFilledAmount, - fillResults.makerAssetFilledAmount - ); - // Compute proportional fill amounts fillResults = _calculateFillResults(order, takerAssetFilledAmount); @@ -389,78 +380,6 @@ contract MixinExchangeCore is } } - /// @dev Validates context for fillOrder. Succeeds or throws. - /// @param order to be filled. - /// @param orderInfo OrderStatus, orderHash, and amount already filled of order. - /// @param takerAssetFillAmount Desired amount of order to fill by taker. - /// @param takerAssetFilledAmount Amount of takerAsset that will be filled. - /// @param makerAssetFilledAmount Amount of makerAsset that will be transfered. - function _assertValidFill( - Order memory order, - OrderInfo memory orderInfo, - uint256 takerAssetFillAmount, // TODO: use FillResults - uint256 takerAssetFilledAmount, - uint256 makerAssetFilledAmount - ) - internal - pure - { - // Revert if fill amount is invalid - // TODO: reconsider necessity for v2.1 - if (takerAssetFillAmount == 0) { - LibRichErrors._rrevert(LibExchangeRichErrors.FillError( - FillErrorCodes.INVALID_TAKER_AMOUNT, - orderInfo.orderHash - )); - } - - // Make sure taker does not pay more than desired amount - // NOTE: This assertion should never fail, it is here - // as an extra defence against potential bugs. - if (takerAssetFilledAmount > takerAssetFillAmount) { - LibRichErrors._rrevert(LibExchangeRichErrors.FillError( - FillErrorCodes.TAKER_OVERPAY, - orderInfo.orderHash - )); - } - - // Make sure order is not overfilled - // NOTE: This assertion should never fail, it is here - // as an extra defence against potential bugs. - if (_safeAdd(orderInfo.orderTakerAssetFilledAmount, takerAssetFilledAmount) - > order.takerAssetAmount) { - LibRichErrors._rrevert(LibExchangeRichErrors.FillError( - FillErrorCodes.OVERFILL, - orderInfo.orderHash - )); - } - - // Make sure order is filled at acceptable price. - // The order has an implied price from the makers perspective: - // order price = order.makerAssetAmount / order.takerAssetAmount - // i.e. the number of makerAsset maker is paying per takerAsset. The - // maker is guaranteed to get this price or a better (lower) one. The - // actual price maker is getting in this fill is: - // fill price = makerAssetFilledAmount / takerAssetFilledAmount - // We need `fill price <= order price` for the fill to be fair to maker. - // This amounts to: - // makerAssetFilledAmount order.makerAssetAmount - // ------------------------ <= ----------------------- - // takerAssetFilledAmount order.takerAssetAmount - // or, equivalently: - // makerAssetFilledAmount * order.takerAssetAmount <= - // order.makerAssetAmount * takerAssetFilledAmount - // NOTE: This assertion should never fail, it is here - // as an extra defence against potential bugs. - if (_safeMul(makerAssetFilledAmount, order.takerAssetAmount) - > _safeMul(order.makerAssetAmount, takerAssetFilledAmount)) { - LibRichErrors._rrevert(LibExchangeRichErrors.FillError( - FillErrorCodes.INVALID_FILL_PRICE, - orderInfo.orderHash - )); - } - } - /// @dev Validates context for cancelOrder. Succeeds or throws. /// @param order to be cancelled. /// @param orderInfo OrderStatus, orderHash, and amount already filled of order. diff --git a/contracts/exchange/contracts/src/MixinMatchOrders.sol b/contracts/exchange/contracts/src/MixinMatchOrders.sol index 04ddb08a9b..17eeeaf453 100644 --- a/contracts/exchange/contracts/src/MixinMatchOrders.sol +++ b/contracts/exchange/contracts/src/MixinMatchOrders.sol @@ -680,22 +680,6 @@ contract MixinMatchOrders is shouldMaximallyFillOrders ); - // Validate fill contexts - _assertValidFill( - leftOrder, - leftOrderInfo, - matchedFillResults.left.takerAssetFilledAmount, - matchedFillResults.left.takerAssetFilledAmount, - matchedFillResults.left.makerAssetFilledAmount - ); - _assertValidFill( - rightOrder, - rightOrderInfo, - matchedFillResults.right.takerAssetFilledAmount, - matchedFillResults.right.takerAssetFilledAmount, - matchedFillResults.right.makerAssetFilledAmount - ); - // Update exchange state _updateFilledState( leftOrder, diff --git a/contracts/exchange/contracts/test/TestExchangeInternals.sol b/contracts/exchange/contracts/test/TestExchangeInternals.sol index 2ae6467300..151a2858de 100644 --- a/contracts/exchange/contracts/test/TestExchangeInternals.sol +++ b/contracts/exchange/contracts/test/TestExchangeInternals.sol @@ -26,6 +26,14 @@ import "../src/Exchange.sol"; contract TestExchangeInternals is Exchange { + event DispatchTransferFromCalled( + bytes32 orderHash, + bytes assetData, + address from, + address to, + uint256 amount + ); + constructor (uint256 chainId) public Exchange(chainId) @@ -62,4 +70,34 @@ contract TestExchangeInternals is fillResults ); } + + function settleOrder( + bytes32 orderHash, + LibOrder.Order memory order, + address takerAddress, + LibFillResults.FillResults memory fillResults + ) + public + { + _settleOrder(orderHash, order, takerAddress, fillResults); + } + + /// @dev Overidden to only log arguments so we can test `_settleOrder()`. + function _dispatchTransferFrom( + bytes32 orderHash, + bytes memory assetData, + address from, + address to, + uint256 amount + ) + internal + { + emit DispatchTransferFromCalled( + orderHash, + assetData, + from, + to, + amount + ); + } } diff --git a/contracts/exchange/test/fill_order.ts b/contracts/exchange/test/fill_order.ts index 34ddafdff2..342b4014f9 100644 --- a/contracts/exchange/test/fill_order.ts +++ b/contracts/exchange/test/fill_order.ts @@ -189,15 +189,7 @@ blockchainTests.resets('FillOrder Tests', ({ web3Wrapper, txDefaults }) => { await fillOrderCombinatorialUtils.testFillOrderScenarioFailureAsync(fillScenario); }); - it('should revert if takerAssetFillAmount is 0', async () => { - const fillScenario = { - ...defaultFillScenario, - takerAssetFillAmountScenario: TakerAssetFillAmountScenario.Zero, - }; - await fillOrderCombinatorialUtils.testFillOrderScenarioFailureAsync(fillScenario); - }); - - it('should revert if an order is expired', async () => { + it('should throw if an order is expired', async () => { const fillScenario = { ...defaultFillScenario, orderScenario: { diff --git a/contracts/exchange/test/internal.ts b/contracts/exchange/test/internal.ts index a39a73f62e..9798ea53e2 100644 --- a/contracts/exchange/test/internal.ts +++ b/contracts/exchange/test/internal.ts @@ -1,3 +1,4 @@ +import { ReferenceFunctions as LibReferenceFunctions } from '@0x/contracts-exchange-libs'; import { blockchainTests, constants, @@ -8,6 +9,7 @@ import { testCombinatoriallyWithReferenceFunc, uint256Values, } from '@0x/contracts-test-utils'; +import { LibMathRevertErrors } from '@0x/order-utils'; import { FillResults, OrderWithoutDomain as Order } from '@0x/types'; import { BigNumber, SafeMathRevertErrors } from '@0x/utils'; import { LogWithDecodedArgs } from 'ethereum-types'; @@ -18,11 +20,12 @@ import { ReferenceFunctions, TestExchangeInternalsContract, TestExchangeInternalsFillEventArgs, + TestExchangeInternalsDispatchTransferFromCalledEventArgs, } from '../src'; -// TODO(dorothy-zbornak): Add _settleOrder -blockchainTests.only('Exchange core internal functions', env => { +blockchainTests('Exchange core internal functions', env => { const CHAIN_ID = 1337; + const ONE_ETHER = constants.ONE_ETHER; const EMPTY_ORDER: Order = { senderAddress: constants.NULL_ADDRESS, makerAddress: constants.NULL_ADDRESS, @@ -39,6 +42,9 @@ blockchainTests.only('Exchange core internal functions', env => { feeRecipientAddress: constants.NULL_ADDRESS, expirationTimeSeconds: constants.ZERO_AMOUNT, }; + const randomAddress = () => hexRandom(constants.ADDRESS_LENGTH); + const randomHash = () => hexRandom(constants.WORD_LENGTH); + const randomAssetData = () => hexRandom(36); let testExchange: TestExchangeInternalsContract; let logDecoder: LogDecoder; let senderAddress: string; @@ -54,49 +60,49 @@ blockchainTests.only('Exchange core internal functions', env => { logDecoder = new LogDecoder(env.web3Wrapper, artifacts); }); - blockchainTests('calculateFillResults', async () => { - function makeOrder( - makerAssetAmount: BigNumber, - takerAssetAmount: BigNumber, - makerFee: BigNumber, - takerFee: BigNumber, - ): Order { - return { - ...EMPTY_ORDER, - 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. - return ReferenceFunctions.calculateFillResults( - makeOrder(otherAmount, orderTakerAssetAmount, otherAmount, otherAmount), - takerAssetFilledAmount, - ); - } - - async function testCalculateFillResultsAsync( - orderTakerAssetAmount: BigNumber, - takerAssetFilledAmount: BigNumber, - otherAmount: BigNumber, - ): Promise { - const order = makeOrder(otherAmount, orderTakerAssetAmount, otherAmount, otherAmount); - return testExchange.calculateFillResults.callAsync(order, takerAssetFilledAmount); - } - + blockchainTests('calculateFillResults', () => { describe.optional('combinatorial tests', () => { + function makeOrder( + makerAssetAmount: BigNumber, + takerAssetAmount: BigNumber, + makerFee: BigNumber, + takerFee: BigNumber, + ): Order { + return { + ...EMPTY_ORDER, + 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. + return ReferenceFunctions.calculateFillResults( + makeOrder(otherAmount, orderTakerAssetAmount, otherAmount, otherAmount), + takerAssetFilledAmount, + ); + } + + async function testCalculateFillResultsAsync( + orderTakerAssetAmount: BigNumber, + takerAssetFilledAmount: BigNumber, + otherAmount: BigNumber, + ): Promise { + const order = makeOrder(otherAmount, orderTakerAssetAmount, otherAmount, otherAmount); + return testExchange.calculateFillResults.callAsync(order, takerAssetFilledAmount); + } + testCombinatoriallyWithReferenceFunc( 'calculateFillResults', referenceCalculateFillResultsAsync, @@ -104,13 +110,167 @@ blockchainTests.only('Exchange core internal functions', env => { [uint256Values, uint256Values, uint256Values], ); }); + + describe('explicit tests', () => { + const MAX_UINT256_ROOT = constants.MAX_UINT256_ROOT; + function makeOrder(details?: Partial): Order { + return _.assign( + {}, + EMPTY_ORDER, + details, + ); + } + + it('matches the output of the reference function', async () => { + const order = makeOrder({ + makerAssetAmount: ONE_ETHER, + takerAssetAmount: ONE_ETHER.times(2), + makerFee: ONE_ETHER.times(0.0023), + takerFee: ONE_ETHER.times(0.0025), + }); + const takerAssetFilledAmount = ONE_ETHER.dividedToIntegerBy(3); + const expected = ReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount); + const actual = await testExchange.calculateFillResults.callAsync(order, takerAssetFilledAmount); + expect(actual).to.deep.eq(expected); + }); + + it('reverts if computing `fillResults.makerAssetFilledAmount` overflows', async () => { + // All values need to be large to ensure we don't trigger a RoundingError. + const order = makeOrder({ + makerAssetAmount: MAX_UINT256_ROOT.times(2), + takerAssetAmount: MAX_UINT256_ROOT, + }); + const takerAssetFilledAmount = MAX_UINT256_ROOT; + const expectedError = new SafeMathRevertErrors.SafeMathError( + SafeMathRevertErrors.SafeMathErrorCodes.Uint256MultiplicationOverflow, + takerAssetFilledAmount, + order.makerAssetAmount, + ); + return expect(testExchange.calculateFillResults.callAsync(order, takerAssetFilledAmount)) + .to.revertWith(expectedError); + }); + + it('reverts if computing `fillResults.makerFeePaid` overflows', async () => { + // All values need to be large to ensure we don't trigger a RoundingError. + const order = makeOrder({ + makerAssetAmount: MAX_UINT256_ROOT, + takerAssetAmount: MAX_UINT256_ROOT, + makerFee: MAX_UINT256_ROOT.times(11), + }); + const takerAssetFilledAmount = MAX_UINT256_ROOT.dividedToIntegerBy(10); + const makerAssetFilledAmount = LibReferenceFunctions.getPartialAmountFloor( + takerAssetFilledAmount, + order.takerAssetAmount, + order.makerAssetAmount, + ); + const expectedError = new SafeMathRevertErrors.SafeMathError( + SafeMathRevertErrors.SafeMathErrorCodes.Uint256MultiplicationOverflow, + makerAssetFilledAmount, + order.makerFee, + ); + return expect(testExchange.calculateFillResults.callAsync(order, takerAssetFilledAmount)) + .to.revertWith(expectedError); + }); + + it('reverts if computing `fillResults.takerFeePaid` overflows', async () => { + // All values need to be large to ensure we don't trigger a RoundingError. + const order = makeOrder({ + makerAssetAmount: MAX_UINT256_ROOT, + takerAssetAmount: MAX_UINT256_ROOT, + takerFee: MAX_UINT256_ROOT.times(11), + }); + const takerAssetFilledAmount = MAX_UINT256_ROOT.dividedToIntegerBy(10); + const expectedError = new SafeMathRevertErrors.SafeMathError( + SafeMathRevertErrors.SafeMathErrorCodes.Uint256MultiplicationOverflow, + takerAssetFilledAmount, + order.takerFee, + ); + return expect(testExchange.calculateFillResults.callAsync(order, takerAssetFilledAmount)) + .to.revertWith(expectedError); + }); + + it('reverts if `order.makerAssetAmount` is 0', async () => { + const order = makeOrder({ + makerAssetAmount: constants.ZERO_AMOUNT, + takerAssetAmount: ONE_ETHER, + }); + const takerAssetFilledAmount = ONE_ETHER; + const expectedError = new LibMathRevertErrors.DivisionByZeroError(); + return expect(testExchange.calculateFillResults.callAsync(order, takerAssetFilledAmount)) + .to.revertWith(expectedError); + }); + + it('reverts if `order.takerAssetAmount` is 0', async () => { + const order = makeOrder({ + makerAssetAmount: ONE_ETHER, + takerAssetAmount: constants.ZERO_AMOUNT, + }); + const takerAssetFilledAmount = ONE_ETHER; + const expectedError = new LibMathRevertErrors.DivisionByZeroError(); + return expect(testExchange.calculateFillResults.callAsync(order, takerAssetFilledAmount)) + .to.revertWith(expectedError); + }); + + it('reverts if there is a rounding error computing `makerAsssetFilledAmount`', async () => { + const order = makeOrder({ + makerAssetAmount: new BigNumber(100), + takerAssetAmount: ONE_ETHER, + }); + const takerAssetFilledAmount = order.takerAssetAmount.dividedToIntegerBy(3); + const expectedError = new LibMathRevertErrors.RoundingError( + takerAssetFilledAmount, + order.takerAssetAmount, + order.makerAssetAmount, + ); + return expect(testExchange.calculateFillResults.callAsync(order, takerAssetFilledAmount)) + .to.revertWith(expectedError); + }); + + it('reverts if there is a rounding error computing `makerFeePaid`', async () => { + const order = makeOrder({ + makerAssetAmount: ONE_ETHER, + takerAssetAmount: ONE_ETHER, + makerFee: new BigNumber(100), + }); + const takerAssetFilledAmount = order.takerAssetAmount.dividedToIntegerBy(3); + const makerAssetFilledAmount = LibReferenceFunctions.getPartialAmountFloor( + takerAssetFilledAmount, + order.takerAssetAmount, + order.makerAssetAmount, + ); + const expectedError = new LibMathRevertErrors.RoundingError( + makerAssetFilledAmount, + order.makerAssetAmount, + order.makerFee, + ); + return expect(testExchange.calculateFillResults.callAsync(order, takerAssetFilledAmount)) + .to.revertWith(expectedError); + }); + + it('reverts if there is a rounding error computing `takerFeePaid`', async () => { + const order = makeOrder({ + makerAssetAmount: ONE_ETHER, + takerAssetAmount: ONE_ETHER, + takerFee: new BigNumber(100), + }); + const takerAssetFilledAmount = order.takerAssetAmount.dividedToIntegerBy(3); + const makerAssetFilledAmount = LibReferenceFunctions.getPartialAmountFloor( + takerAssetFilledAmount, + order.takerAssetAmount, + order.makerAssetAmount, + ); + const expectedError = new LibMathRevertErrors.RoundingError( + makerAssetFilledAmount, + order.makerAssetAmount, + order.takerFee, + ); + return expect(testExchange.calculateFillResults.callAsync(order, takerAssetFilledAmount)) + .to.revertWith(expectedError); + }); + }); }); blockchainTests.resets('updateFilledState', async () => { - const ONE_ETHER = new BigNumber(1e18); - const randomAddress = () => hexRandom(constants.ADDRESS_LENGTH); - const randomHash = () => hexRandom(constants.WORD_LENGTH); - const randomAssetData = () => hexRandom(36); const ORDER_DEFAULTS = { senderAddress: randomAddress(), makerAddress: randomAddress(), @@ -129,11 +289,7 @@ blockchainTests.only('Exchange core internal functions', env => { }; function makeOrder(details?: Partial): Order { - return _.assign( - {}, - ORDER_DEFAULTS, - details, - ); + return _.assign({}, ORDER_DEFAULTS, details); } async function testUpdateFilledStateAsync( @@ -192,7 +348,7 @@ blockchainTests.only('Exchange core internal functions', env => { ); }); - it('throws if `leftOrderTakerAssetFilledAmount + fillResults.takerAssetFilledAmount` overflows', async () => { + it('reverts if `leftOrderTakerAssetFilledAmount + fillResults.takerAssetFilledAmount` overflows', async () => { const order = makeOrder(); const orderTakerAssetFilledAmount = constants.MAX_UINT256.dividedToIntegerBy(2); const takerAssetFillAmount = constants.MAX_UINT256.dividedToIntegerBy(2).plus(2); @@ -216,5 +372,71 @@ blockchainTests.only('Exchange core internal functions', env => { )).to.revertWith(expectedError); }); }); + + blockchainTests('settleOrder', () => { + const DEFAULT_ORDER = { + senderAddress: randomAddress(), + makerAddress: randomAddress(), + takerAddress: randomAddress(), + makerFee: ONE_ETHER.times(0.001), + takerFee: ONE_ETHER.times(0.003), + makerAssetAmount: ONE_ETHER, + takerAssetAmount: ONE_ETHER.times(0.5), + makerAssetData: randomAssetData(), + takerAssetData: randomAssetData(), + makerFeeAssetData: randomAssetData(), + takerFeeAssetData: randomAssetData(), + salt: new BigNumber(_.random(0, 1e8)), + feeRecipientAddress: randomAddress(), + expirationTimeSeconds: new BigNumber(_.random(0, 1e8)), + }; + + it('calls `_dispatchTransferFrom()` in the right order with the correct arguments', async () => { + const order = DEFAULT_ORDER; + const orderHash = randomHash(); + const takerAddress = randomAddress(); + const fillResults = { + makerAssetFilledAmount: ONE_ETHER.times(2), + takerAssetFilledAmount: ONE_ETHER.times(10), + makerFeePaid: ONE_ETHER.times(0.01), + takerFeePaid: ONE_ETHER.times(0.025), + }; + const receipt = await logDecoder.getTxWithDecodedLogsAsync( + await testExchange.settleOrder.sendTransactionAsync( + orderHash, + order, + takerAddress, + fillResults, + ), + ); + const logs = receipt.logs as Array>; + expect(logs.length === 4); + expect(_.every(logs, log => log.event === 'DispatchTransferFromCalled')).to.be.true(); + // taker -> maker + expect(logs[0].args.orderHash).to.eq(orderHash); + expect(logs[0].args.assetData).to.eq(order.takerAssetData); + expect(logs[0].args.from).to.eq(takerAddress); + expect(logs[0].args.to).to.eq(order.makerAddress); + expect(logs[0].args.amount).to.bignumber.eq(fillResults.takerAssetFilledAmount); + // maker -> taker + expect(logs[1].args.orderHash).to.eq(orderHash); + expect(logs[1].args.assetData).to.eq(order.makerAssetData); + expect(logs[1].args.from).to.eq(order.makerAddress); + expect(logs[1].args.to).to.eq(takerAddress); + expect(logs[1].args.amount).to.bignumber.eq(fillResults.makerAssetFilledAmount); + // taker fee -> feeRecipient + expect(logs[2].args.orderHash).to.eq(orderHash); + expect(logs[2].args.assetData).to.eq(order.takerFeeAssetData); + expect(logs[2].args.from).to.eq(takerAddress); + expect(logs[2].args.to).to.eq(order.feeRecipientAddress); + expect(logs[2].args.amount).to.bignumber.eq(fillResults.takerFeePaid); + // maker fee -> feeRecipient + expect(logs[3].args.orderHash).to.eq(orderHash); + expect(logs[3].args.assetData).to.eq(order.makerFeeAssetData); + expect(logs[3].args.from).to.eq(order.makerAddress); + expect(logs[3].args.to).to.eq(order.feeRecipientAddress); + expect(logs[3].args.amount).to.bignumber.eq(fillResults.makerFeePaid); + }); + }); }); // tslint:disable-line:max-file-line-count diff --git a/contracts/exchange/test/isolated_fill_order.ts b/contracts/exchange/test/isolated_fill_order.ts index 71b28343f3..3513373155 100644 --- a/contracts/exchange/test/isolated_fill_order.ts +++ b/contracts/exchange/test/isolated_fill_order.ts @@ -1,3 +1,4 @@ +import { ReferenceFunctions as LibReferenceFunctions } from '@0x/contracts-exchange-libs'; import { blockchainTests, constants, @@ -6,7 +7,7 @@ import { } from '@0x/contracts-test-utils'; import { ExchangeRevertErrors, LibMathRevertErrors } from '@0x/order-utils'; import { FillResults, OrderInfo, OrderStatus, SignatureType } from '@0x/types'; -import { BigNumber } from '@0x/utils'; +import { BigNumber, SafeMathRevertErrors } from '@0x/utils'; import * as _ from 'lodash'; import { calculateFillResults } from '../src/reference_functions'; @@ -24,8 +25,7 @@ import { blockchainTests('Isolated fillOrder() tests', env => { const randomAddress = () => hexRandom(constants.ADDRESS_LENGTH); const getCurrentTime = () => Math.floor(_.now() / 1000); - const { ZERO_AMOUNT } = constants; - const ONE_ETHER = new BigNumber(10).pow(18); + const { ZERO_AMOUNT, ONE_ETHER, MAX_UINT256_ROOT } = constants; const ONE_DAY = 60 * 60 * 24; const TOMORROW = getCurrentTime() + ONE_DAY; const DEFAULT_ORDER: Order = { @@ -464,6 +464,61 @@ blockchainTests('Isolated fillOrder() tests', env => { .to.revertWith(expectedError); }); + it('can\'t fill an order that results in a `makerAssetFilledAmount` overflow.', async () => { + // All values need to be large to ensure we don't trigger a Rounding. + const order = createOrder({ + makerAssetAmount: MAX_UINT256_ROOT.times(2), + takerAssetAmount: MAX_UINT256_ROOT, + }); + const takerAssetFillAmount = MAX_UINT256_ROOT; + const expectedError = new SafeMathRevertErrors.SafeMathError( + SafeMathRevertErrors.SafeMathErrorCodes.Uint256MultiplicationOverflow, + takerAssetFillAmount, + order.makerAssetAmount, + ); + return expect(exchange.fillOrderAsync(order, takerAssetFillAmount)) + .to.revertWith(expectedError); + }); + + it('can\'t fill an order that results in a `makerFeePaid` overflow.', async () => { + // All values need to be large to ensure we don't trigger a Rounding. + const order = createOrder({ + makerAssetAmount: MAX_UINT256_ROOT, + takerAssetAmount: MAX_UINT256_ROOT, + makerFee: MAX_UINT256_ROOT.times(11), + }); + const takerAssetFillAmount = MAX_UINT256_ROOT.dividedToIntegerBy(10); + const makerAssetFilledAmount = LibReferenceFunctions.getPartialAmountFloor( + takerAssetFillAmount, + order.takerAssetAmount, + order.makerAssetAmount, + ); + const expectedError = new SafeMathRevertErrors.SafeMathError( + SafeMathRevertErrors.SafeMathErrorCodes.Uint256MultiplicationOverflow, + makerAssetFilledAmount, + order.makerFee, + ); + return expect(exchange.fillOrderAsync(order, takerAssetFillAmount)) + .to.revertWith(expectedError); + }); + + it('can\'t fill an order that results in a `takerFeePaid` overflow.', async () => { + // All values need to be large to ensure we don't trigger a Rounding. + const order = createOrder({ + makerAssetAmount: MAX_UINT256_ROOT, + takerAssetAmount: MAX_UINT256_ROOT, + takerFee: MAX_UINT256_ROOT.times(11), + }); + const takerAssetFillAmount = MAX_UINT256_ROOT.dividedToIntegerBy(10); + const expectedError = new SafeMathRevertErrors.SafeMathError( + SafeMathRevertErrors.SafeMathErrorCodes.Uint256MultiplicationOverflow, + takerAssetFillAmount, + order.takerFee, + ); + return expect(exchange.fillOrderAsync(order, takerAssetFillAmount)) + .to.revertWith(expectedError); + }); + it('can\'t fill an order with a bad signature', async () => { const order = createOrder(); const signature = createBadSignature(); @@ -529,6 +584,11 @@ blockchainTests('Isolated fillOrder() tests', env => { }); describe('permitted fills', () => { + it('should allow takerAssetFillAmount to be zero', async () => { + const order = createOrder(); + return fillOrderAndAssertResultsAsync(order, constants.ZERO_AMOUNT); + }); + it('can fill an order if taker is `takerAddress`', async () => { const order = createOrder({ takerAddress, diff --git a/contracts/exchange/test/reference_functions.ts b/contracts/exchange/test/reference_functions.ts new file mode 100644 index 0000000000..b2dd6dae23 --- /dev/null +++ b/contracts/exchange/test/reference_functions.ts @@ -0,0 +1,178 @@ +import { ReferenceFunctions as LibReferenceFunctions } from '@0x/contracts-exchange-libs'; +import { + constants, + describe, + expect, +} from '@0x/contracts-test-utils'; +import { LibMathRevertErrors } from '@0x/order-utils'; +import { OrderWithoutDomain as Order } from '@0x/types'; +import { BigNumber, SafeMathRevertErrors } from '@0x/utils'; +import * as _ from 'lodash'; + +import { calculateFillResults } from '../src/reference_functions'; + +describe('Reference functions', () => { + const ONE_ETHER = constants.ONE_ETHER; + const EMPTY_ORDER: Order = { + senderAddress: constants.NULL_ADDRESS, + makerAddress: constants.NULL_ADDRESS, + takerAddress: constants.NULL_ADDRESS, + makerFee: constants.ZERO_AMOUNT, + takerFee: constants.ZERO_AMOUNT, + makerAssetAmount: constants.ZERO_AMOUNT, + takerAssetAmount: constants.ZERO_AMOUNT, + makerAssetData: constants.NULL_BYTES, + takerAssetData: constants.NULL_BYTES, + makerFeeAssetData: constants.NULL_BYTES, + takerFeeAssetData: constants.NULL_BYTES, + salt: constants.ZERO_AMOUNT, + feeRecipientAddress: constants.NULL_ADDRESS, + expirationTimeSeconds: constants.ZERO_AMOUNT, + }; + + describe('calculateFillResults', () => { + const MAX_UINT256_ROOT = constants.MAX_UINT256_ROOT; + function makeOrder(details?: Partial): Order { + return _.assign( + {}, + EMPTY_ORDER, + details, + ); + } + + it('reverts if computing `fillResults.makerAssetFilledAmount` overflows', () => { + // All values need to be large to ensure we don't trigger a RondingError. + const order = makeOrder({ + makerAssetAmount: MAX_UINT256_ROOT.times(2), + takerAssetAmount: MAX_UINT256_ROOT, + }); + const takerAssetFilledAmount = MAX_UINT256_ROOT; + const expectedError = new SafeMathRevertErrors.SafeMathError( + SafeMathRevertErrors.SafeMathErrorCodes.Uint256MultiplicationOverflow, + takerAssetFilledAmount, + order.makerAssetAmount, + ); + return expect(() => calculateFillResults(order, takerAssetFilledAmount)) + .to.throw(expectedError.message); + }); + + it('reverts if computing `fillResults.makerFeePaid` overflows', () => { + // All values need to be large to ensure we don't trigger a RondingError. + const order = makeOrder({ + makerAssetAmount: MAX_UINT256_ROOT, + takerAssetAmount: MAX_UINT256_ROOT, + makerFee: MAX_UINT256_ROOT.times(11), + }); + const takerAssetFilledAmount = MAX_UINT256_ROOT.dividedToIntegerBy(10); + const makerAssetFilledAmount = LibReferenceFunctions.getPartialAmountFloor( + takerAssetFilledAmount, + order.takerAssetAmount, + order.makerAssetAmount, + ); + const expectedError = new SafeMathRevertErrors.SafeMathError( + SafeMathRevertErrors.SafeMathErrorCodes.Uint256MultiplicationOverflow, + makerAssetFilledAmount, + order.makerFee, + ); + return expect(() => calculateFillResults(order, takerAssetFilledAmount)) + .to.throw(expectedError.message); + }); + + it('reverts if computing `fillResults.takerFeePaid` overflows', () => { + // All values need to be large to ensure we don't trigger a RondingError. + const order = makeOrder({ + makerAssetAmount: MAX_UINT256_ROOT, + takerAssetAmount: MAX_UINT256_ROOT, + takerFee: MAX_UINT256_ROOT.times(11), + }); + const takerAssetFilledAmount = MAX_UINT256_ROOT.dividedToIntegerBy(10); + const expectedError = new SafeMathRevertErrors.SafeMathError( + SafeMathRevertErrors.SafeMathErrorCodes.Uint256MultiplicationOverflow, + takerAssetFilledAmount, + order.takerFee, + ); + return expect(() => calculateFillResults(order, takerAssetFilledAmount)) + .to.throw(expectedError.message); + }); + + it('reverts if `order.makerAssetAmount` is 0', () => { + const order = makeOrder({ + makerAssetAmount: constants.ZERO_AMOUNT, + takerAssetAmount: ONE_ETHER, + }); + const takerAssetFilledAmount = ONE_ETHER; + const expectedError = new LibMathRevertErrors.DivisionByZeroError(); + return expect(() => calculateFillResults(order, takerAssetFilledAmount)) + .to.throw(expectedError.message); + }); + + it('reverts if `order.takerAssetAmount` is 0', () => { + const order = makeOrder({ + makerAssetAmount: ONE_ETHER, + takerAssetAmount: constants.ZERO_AMOUNT, + }); + const takerAssetFilledAmount = ONE_ETHER; + const expectedError = new LibMathRevertErrors.DivisionByZeroError(); + return expect(() => calculateFillResults(order, takerAssetFilledAmount)) + .to.throw(expectedError.message); + }); + + it('reverts if there is a rounding error computing `makerAsssetFilledAmount`', () => { + const order = makeOrder({ + makerAssetAmount: new BigNumber(100), + takerAssetAmount: ONE_ETHER, + }); + const takerAssetFilledAmount = order.takerAssetAmount.dividedToIntegerBy(3); + const expectedError = new LibMathRevertErrors.RoundingError( + takerAssetFilledAmount, + order.takerAssetAmount, + order.makerAssetAmount, + ); + return expect(() => calculateFillResults(order, takerAssetFilledAmount)) + .to.throw(expectedError.message); + }); + + it('reverts if there is a rounding error computing `makerFeePaid`', () => { + const order = makeOrder({ + makerAssetAmount: ONE_ETHER, + takerAssetAmount: ONE_ETHER, + makerFee: new BigNumber(100), + }); + const takerAssetFilledAmount = order.takerAssetAmount.dividedToIntegerBy(3); + const makerAssetFilledAmount = LibReferenceFunctions.getPartialAmountFloor( + takerAssetFilledAmount, + order.takerAssetAmount, + order.makerAssetAmount, + ); + const expectedError = new LibMathRevertErrors.RoundingError( + makerAssetFilledAmount, + order.makerAssetAmount, + order.makerFee, + ); + return expect(() => calculateFillResults(order, takerAssetFilledAmount)) + .to.throw(expectedError.message); + }); + + it('reverts if there is a rounding error computing `takerFeePaid`', () => { + const order = makeOrder({ + makerAssetAmount: ONE_ETHER, + takerAssetAmount: ONE_ETHER, + takerFee: new BigNumber(100), + }); + const takerAssetFilledAmount = order.takerAssetAmount.dividedToIntegerBy(3); + const makerAssetFilledAmount = LibReferenceFunctions.getPartialAmountFloor( + takerAssetFilledAmount, + order.takerAssetAmount, + order.makerAssetAmount, + ); + const expectedError = new LibMathRevertErrors.RoundingError( + makerAssetFilledAmount, + order.makerAssetAmount, + order.takerFee, + ); + return expect(() => calculateFillResults(order, takerAssetFilledAmount)) + .to.throw(expectedError.message); + }); + }); +}); +// tslint:disable-line:max-file-line-count