From f58e28d1bea2d4594d2557250477298d7194eeb9 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Mon, 5 Aug 2019 16:24:58 -0700 Subject: [PATCH] Implemented reference functions and added tests for _calculateCompleteRightFill --- contracts/exchange/src/reference_functions.ts | 203 +++++++++++++++++- .../exchange/test/match_orders_unit_tests.ts | 148 +++++++++---- 2 files changed, 306 insertions(+), 45 deletions(-) diff --git a/contracts/exchange/src/reference_functions.ts b/contracts/exchange/src/reference_functions.ts index 91a96999ad..dcb0e10883 100644 --- a/contracts/exchange/src/reference_functions.ts +++ b/contracts/exchange/src/reference_functions.ts @@ -1,8 +1,11 @@ import { ReferenceFunctions as ExchangeLibsReferenceFunctions } from '@0x/contracts-exchange-libs'; -import { FillResults, OrderWithoutDomain } from '@0x/types'; +import { constants } from '@0x/contracts-test-utils'; +import { ReferenceFunctions as UtilsReferenceFunctions } from '@0x/contracts-utils'; +import { FillResults, MatchedFillResults, OrderWithoutDomain } from '@0x/types'; import { BigNumber } from '@0x/utils'; -const { safeGetPartialAmountFloor } = ExchangeLibsReferenceFunctions; +const { safeGetPartialAmountCeil, safeGetPartialAmountFloor } = ExchangeLibsReferenceFunctions; +const { safeSub } = UtilsReferenceFunctions; /** * Calculates amounts filled and fees paid by maker and taker. @@ -22,3 +25,199 @@ export function calculateFillResults(order: OrderWithoutDomain, takerAssetFilled takerFeePaid, }; } + +/** + * Calculates amounts filled and fees paid by maker and taker. + */ +export function calculateMatchedFillResults( + leftOrder: OrderWithoutDomain, + rightOrder: OrderWithoutDomain, + leftOrderTakerAssetFilledAmount: BigNumber, + rightOrderTakerAssetFilledAmount: BigNumber, +): MatchedFillResults { + // Get an empty matched fill results object to fill with this data. + const matchedFillResults = emptyMatchedFillResults(); + + // Calculate the remaining amounts of maker and taker assets for both orders. + const [ + leftMakerAssetAmountRemaining, + leftTakerAssetAmountRemaining, + rightMakerAssetAmountRemaining, + rightTakerAssetAmountRemaining, + ] = getRemainingFillAmounts( + leftOrder, + rightOrder, + leftOrderTakerAssetFilledAmount, + rightOrderTakerAssetFilledAmount, + ); + + // Calculate the profit from the matching + matchedFillResults.profitInLeftMakerAsset = safeSub( + matchedFillResults.left.makerAssetFilledAmount, + matchedFillResults.right.takerAssetFilledAmount, + ); + + if (leftTakerAssetAmountRemaining.isGreaterThan(rightMakerAssetAmountRemaining)) { + // Case 1: Right order is fully filled + calculateCompleteRightFill( + leftOrder, + rightMakerAssetAmountRemaining, + rightTakerAssetAmountRemaining, + matchedFillResults, + ); + } else if (leftTakerAssetAmountRemaining.isLessThan(rightMakerAssetAmountRemaining)) { + // Case 2: Left order is fully filled + matchedFillResults.left.makerAssetFilledAmount = leftMakerAssetAmountRemaining; + matchedFillResults.left.takerAssetFilledAmount = leftTakerAssetAmountRemaining; + matchedFillResults.right.makerAssetFilledAmount = leftTakerAssetAmountRemaining; + matchedFillResults.right.takerAssetFilledAmount = safeGetPartialAmountCeil( + rightOrder.takerAssetAmount, + rightOrder.makerAssetAmount, + leftTakerAssetAmountRemaining, + ); + } else { + // Case 3: Both orders are fully filled. Technically, this could be captured by the above cases, but + // this calculation will be more precise since it does not include rounding. + calculateCompleteFillBoth( + leftMakerAssetAmountRemaining, + leftTakerAssetAmountRemaining, + rightMakerAssetAmountRemaining, + rightTakerAssetAmountRemaining, + matchedFillResults, + ); + } + + // Compute the fees from the order matching + calculateFees( + leftOrder, + rightOrder, + matchedFillResults, + ); + + return matchedFillResults; +} + +/** + * Calculates the complete fill of both orders and updates the provided MatchedFillResults object. + */ +export function calculateCompleteFillBoth( + leftMakerAssetAmountRemaining: BigNumber, + leftTakerAssetAmountRemaining: BigNumber, + rightMakerAssetAmountRemaining: BigNumber, + rightTakerAssetAmountRemaining: BigNumber, + matchedFillResults?: MatchedFillResults, +): MatchedFillResults { + if (matchedFillResults === undefined) { + matchedFillResults = emptyMatchedFillResults(); // tslint:disable-line:no-parameter-reassignment + } + matchedFillResults.left.makerAssetFilledAmount = leftMakerAssetAmountRemaining; + matchedFillResults.left.takerAssetFilledAmount = leftTakerAssetAmountRemaining; + matchedFillResults.right.makerAssetFilledAmount = rightMakerAssetAmountRemaining; + matchedFillResults.right.takerAssetFilledAmount = rightTakerAssetAmountRemaining; + return matchedFillResults; +} + +/** + * Calculates the complete fill of the right order and updates the provided MatchedFillResults object. + */ +export function calculateCompleteRightFill( + leftOrder: OrderWithoutDomain, + rightMakerAssetAmountRemaining: BigNumber, + rightTakerAssetAmountRemaining: BigNumber, + matchedFillResults?: MatchedFillResults, +): MatchedFillResults { + if (matchedFillResults === undefined) { + matchedFillResults = emptyMatchedFillResults(); // tslint:disable-line:no-parameter-reassignment + } + matchedFillResults.left.makerAssetFilledAmount = safeGetPartialAmountFloor( + leftOrder.makerAssetAmount, + leftOrder.takerAssetAmount, + rightMakerAssetAmountRemaining, + ); + matchedFillResults.left.takerAssetFilledAmount = rightMakerAssetAmountRemaining; + matchedFillResults.right.makerAssetFilledAmount = rightMakerAssetAmountRemaining; + matchedFillResults.right.takerAssetFilledAmount = rightTakerAssetAmountRemaining; + return matchedFillResults; +} + +/** + * Compute the fees for the left and right orders. + */ +function calculateFees( + leftOrder: OrderWithoutDomain, + rightOrder: OrderWithoutDomain, + matchedFillResults: MatchedFillResults, +): void { + // Compute fees for left order + matchedFillResults.left.makerFeePaid = safeGetPartialAmountFloor( + matchedFillResults.left.makerAssetFilledAmount, + leftOrder.makerAssetAmount, + leftOrder.makerFee, + ); + matchedFillResults.left.takerFeePaid = safeGetPartialAmountFloor( + matchedFillResults.left.takerAssetFilledAmount, + leftOrder.takerAssetAmount, + leftOrder.takerFee, + ); + + // Compute fees for right order + matchedFillResults.right.makerFeePaid = safeGetPartialAmountFloor( + matchedFillResults.right.makerAssetFilledAmount, + rightOrder.makerAssetAmount, + rightOrder.makerFee, + ); + matchedFillResults.right.takerFeePaid = safeGetPartialAmountFloor( + matchedFillResults.right.takerAssetFilledAmount, + rightOrder.takerAssetAmount, + rightOrder.takerFee, + ); +} + +/** + * Returns an empty fill results object. + */ +function emptyFillResults(): FillResults { + return { + makerAssetFilledAmount: constants.ZERO_AMOUNT, + takerAssetFilledAmount: constants.ZERO_AMOUNT, + makerFeePaid: constants.ZERO_AMOUNT, + takerFeePaid: constants.ZERO_AMOUNT, + }; +} + +/** + * Returns an empty matched fill results object. + */ +function emptyMatchedFillResults(): MatchedFillResults { + return { + left: emptyFillResults(), + right: emptyFillResults(), + profitInLeftMakerAsset: constants.ZERO_AMOUNT, + profitInRightMakerAsset: constants.ZERO_AMOUNT, + }; +} + +/** + * Returns the token amounts that are remaining to be filled. + */ +function getRemainingFillAmounts( + leftOrder: OrderWithoutDomain, + rightOrder: OrderWithoutDomain, + leftOrderTakerAssetFilledAmount: BigNumber, + rightOrderTakerAssetFilledAmount: BigNumber, +): [ BigNumber, BigNumber, BigNumber, BigNumber ] { + return [ + safeGetPartialAmountFloor( + leftOrder.makerAssetAmount, + leftOrder.takerAssetAmount, + leftOrderTakerAssetFilledAmount + ), + safeSub(leftOrder.takerAssetAmount, leftOrderTakerAssetFilledAmount), + safeGetPartialAmountFloor( + rightOrder.makerAssetAmount, + rightOrder.takerAssetAmount, + rightOrderTakerAssetFilledAmount + ), + safeSub(rightOrder.takerAssetAmount, rightOrderTakerAssetFilledAmount), + ]; +} diff --git a/contracts/exchange/test/match_orders_unit_tests.ts b/contracts/exchange/test/match_orders_unit_tests.ts index ce7fb8a231..61c647362a 100644 --- a/contracts/exchange/test/match_orders_unit_tests.ts +++ b/contracts/exchange/test/match_orders_unit_tests.ts @@ -25,7 +25,7 @@ import { } from '@0x/contracts-test-utils'; import { BlockchainLifecycle } from '@0x/dev-utils'; import { assetDataUtils, orderHashUtils } from '@0x/order-utils'; -import { Order, OrderStatus, SignedOrder } from '@0x/types'; +import { OrderWithoutDomain, OrderStatus, SignedOrder } from '@0x/types'; import { BigNumber, providerUtils } from '@0x/utils'; import { Web3Wrapper } from '@0x/web3-wrapper'; import * as chai from 'chai'; @@ -40,6 +40,7 @@ import { ReferenceFunctions, TestMatchOrdersContract, } from '../src'; +import { calculateCompleteFillBoth, calculateCompleteRightFill } from '../src/reference_functions'; import { MatchOrderTester, TokenBalances } from './utils/match_order_tester'; @@ -48,30 +49,6 @@ import { MatchOrderTester, TokenBalances } from './utils/match_order_tester'; constants.NUM_DUMMY_ERC721_TO_DEPLOY = 1; constants.NUM_DUMMY_ERC1155_CONTRACTS_TO_DEPLOY = 1; -/** - * Converts a SignedOrder object into an Order object by removing the signature field. - * - */ -//function toOrder(order: SignedOrder): Order { -// return { -// domain: order.domain, -// makerAddress: order.makerAddress, -// takerAddress: order.takerAddress, -// feeRecipientAddress: order.feeRecipientAddress, -// senderAddress: order.senderAddress, -// makerAssetAmount: order.makerAssetAmount, -// takerAssetAmount: order.takerAssetAmount, -// makerFee: order.makerFee, -// takerFee: order.takerFee, -// expirationTimeSeconds: order.expirationTimeSeconds, -// salt: order.salt, -// makerAssetData: order.makerAssetData, -// takerAssetData: order.takerAssetData, -// makerFeeAssetData: order.makerFeeAssetData, -// takerFeeAssetData: order.takerFeeAssetData, -// } -//} - /** * Tests the _calculateCompleteFillBoth function with the provided inputs by making a call * to the provided matchOrders contract's externalCalculateCompleteFillBoth function with the @@ -90,19 +67,55 @@ async function testCalculateCompleteFillBothAsync( // Ensure that the correct number of arguments were provided. expect(args.length).to.be.eq(4); - // Get the resultant matched fill results from the call to _calculateCompleteFillBoth. - const matchedFillResults = await matchOrders.externalCalculateCompleteFillBoth.callAsync( + // Get the expected matched fill results from calling the reference function. + const expectedMatchedFillResults = calculateCompleteFillBoth( args[0], args[1], args[2], args[3], ); - // Ensure that the matched fill results are correct. - expect(matchedFillResults.left.makerAssetFilledAmount).bignumber.to.be.eq(args[0]); - expect(matchedFillResults.left.takerAssetFilledAmount).bignumber.to.be.eq(args[1]); - expect(matchedFillResults.right.makerAssetFilledAmount).bignumber.to.be.eq(args[2]); - expect(matchedFillResults.right.takerAssetFilledAmount).bignumber.to.be.eq(args[3]); + // Get the resultant matched fill results from the call to _calculateCompleteFillBoth. + const actualMatchedFillResults = await matchOrders.externalCalculateCompleteFillBoth.callAsync( + args[0], + args[1], + args[2], + args[3], + ); + + expect(actualMatchedFillResults).to.be.deep.eq(expectedMatchedFillResults); +} + +/** + * Tests the _calculateCompleteRightFill function with the provided inputs by making a call + * to the provided matchOrders contract's externalCalculateCompleteRightFill function with the + * provided inputs and asserting that the resultant struct is correct. + * @param matchOrders The TestMatchOrders contract object that should be used to make the call to + * the smart contract. + */ +async function testCalculateCompleteRightFillAsync( + matchOrders: TestMatchOrdersContract, + leftOrder: OrderWithoutDomain, + args: BigNumber[], +): Promise { + // Ensure that the correct number of arguments were provided. + expect(args.length).to.be.eq(2); + + // Get the expected matched fill results from calling the reference function. + const expectedMatchedFillResults = calculateCompleteRightFill( + leftOrder, + args[0], + args[1], + ); + + // Get the resultant matched fill results from the call to _calculateCompleteRightFill. + const actualMatchedFillResults = await matchOrders.publicCalculateCompleteRightFill.callAsync( + leftOrder, + args[0], + args[1], + ); + + expect(actualMatchedFillResults).to.be.deep.eq(expectedMatchedFillResults); } /** @@ -138,6 +151,9 @@ async function testCalculateCompleteFillBothAsync( // expect(matchedFillResults.right.takerAssetFilledAmount).bignumber.to.be.eq(args[3]); //} +chaiSetup.configure(); +const expect = chai.expect; + blockchainTests.resets.only('MatchOrders Tests', ({ web3Wrapper, txDefaults }) => { let chainId: number; let makerAddressLeft: string; @@ -327,14 +343,6 @@ blockchainTests.resets.only('MatchOrders Tests', ({ web3Wrapper, txDefaults }) = ); }); - beforeEach(async () => { - await blockchainLifecycle.startAsync(); - }); - - afterEach(async () => { - await blockchainLifecycle.revertAsync(); - }); - describe('_assertValidMatch', () => { }); @@ -400,9 +408,63 @@ blockchainTests.resets.only('MatchOrders Tests', ({ web3Wrapper, txDefaults }) = }); describe('_calculateCompleteRightFill', () => { - // FIXME - Test a few different situations. - // FIXME - Verify that rounding fails when it should. Add a comment that says that it - // can possibly be removed after more rigorous unit testing + /** + * NOTE(jalextowle): These test cases actually cover all code branches of _calculateCompleteRightFill (in + * fact any one of these test cases provide 100% coverage), but they do not verify that _safeGetPartialAmountCeil + * and _safeGetPartialAmountFloor revert appropriately. Keeping in the spirit of unit testing, these functions + * are unit tested to ensure that they exhibit the correct behavior in a more isolated setting. + */ + + it('should correctly calculate the complete right fill', async () => { + const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ + makerAddress: makerAddressLeft, + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(17, 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(98, 0), + feeRecipientAddress: feeRecipientAddressLeft, + }); + await testCalculateCompleteRightFillAsync( + matchOrders, + signedOrderLeft, + [ + Web3Wrapper.toBaseUnitAmount(75, 0), + Web3Wrapper.toBaseUnitAmount(13, 0), + ], + ); + }); + + it('should correctly calculate the complete right fill', async () => { + const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ + makerAddress: makerAddressLeft, + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(12, 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(97, 0), + feeRecipientAddress: feeRecipientAddressLeft, + }); + await testCalculateCompleteRightFillAsync( + matchOrders, + signedOrderLeft, + [ + Web3Wrapper.toBaseUnitAmount(89, 0), + Web3Wrapper.toBaseUnitAmount(1, 0), + ], + ); + }); + + it('should correctly calculate the complete right fill', async () => { + const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ + makerAddress: makerAddressLeft, + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(50, 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(100, 18), + feeRecipientAddress: feeRecipientAddressLeft, + }); + await testCalculateCompleteRightFillAsync( + matchOrders, + signedOrderLeft, + [ + Web3Wrapper.toBaseUnitAmount(10, 18), + Web3Wrapper.toBaseUnitAmount(2, 18), + ], + ); + }); }); describe('_settleMatchedOrders', () => {