From a000574dab5cce89dd555f6e425b7559b9f5a117 Mon Sep 17 00:00:00 2001 From: James Towle Date: Mon, 1 Jul 2019 16:36:47 -0500 Subject: [PATCH] Polished MixinMatchOrders and removed unimplemented test --- .../contracts/src/MixinMatchOrders.sol | 66 ++++++++++++++----- contracts/exchange/test/match_orders.ts | 29 +------- .../exchange/test/utils/match_order_tester.ts | 6 +- 3 files changed, 55 insertions(+), 46 deletions(-) diff --git a/contracts/exchange/contracts/src/MixinMatchOrders.sol b/contracts/exchange/contracts/src/MixinMatchOrders.sol index fb1e06eb12..e38a363002 100644 --- a/contracts/exchange/contracts/src/MixinMatchOrders.sol +++ b/contracts/exchange/contracts/src/MixinMatchOrders.sol @@ -146,7 +146,16 @@ contract MixinMatchOrders is } } - // FIXME + /// @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. + /// @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. + /// @param rightSignatures Proof that right orders were created by the right makers. + /// @param withMaximalFill A value that indicates whether or not the order matching + /// should be done with maximal fill. + /// @return batchMatchedFillResults Amounts filled and profit generated. function _batchMatchOrders( LibOrder.Order[] memory leftOrders, LibOrder.Order[] memory rightOrders, @@ -187,18 +196,20 @@ contract MixinMatchOrders is // matchOrdersWithMaximalFill function. if (withMaximalFill) { // Match the two orders that are pointed to by the left and right indices - matchResults = matchOrdersWithMaximalFill( + matchResults = _matchOrders( leftOrder, rightOrder, leftSignature, - rightSignature + rightSignature, + true ); } else { - matchResults = matchOrders( + matchResults = _matchOrders( leftOrder, rightOrder, leftSignature, - rightSignature + rightSignature, + false ); } @@ -325,33 +336,56 @@ contract MixinMatchOrders is rightTakerAssetAmountRemaining ); + // Maximally fill the orders and pay out profits to the matcher in one or both of the maker assets. if (withMaximalFill) { bool leftSpread; bool rightSpread; - // FIXME -- Add good comments + + // Calculate the maximum fill results for the maker and taker assets. At least one of the orders will be fully filled. + // + // The maximum that the left maker can possibly buy is the amount that the right order can sell. + // The maximum that the right maker can possibly buy is the amount that the left order can sell. + // + // If the left order is fully filled, profit will be paid out in the left maker asset. If the right order is fully filled, + // the profit will be out in the right maker asset. + // + // There are three cases to consider: + // Case 1. + // If the left maker can buy more than the right maker can sell, then only the right order is fully filled. + // Case 2. + // If the right maker can buy more than the left maker can sell, then only the right order is fully filled. + // Case 3. + // If the right maker can sell the max of what the left maker can buy and the left maker can sell the max of + // what the right maker can buy, then both orders are fully filled. if (leftTakerAssetAmountRemaining > rightMakerAssetAmountRemaining) { // Case 1: Right order is fully filled with the profit paid in the left makerAsset matchedFillResults.right.makerAssetFilledAmount = rightMakerAssetAmountRemaining; matchedFillResults.right.takerAssetFilledAmount = rightTakerAssetAmountRemaining; matchedFillResults.left.takerAssetFilledAmount = rightMakerAssetAmountRemaining; + // Round down to ensure the left maker's exchange rate does not exceed the price specified by the order. + // We favor the left maker when the exchange rate must be rounded and the profit is being paid in the + // left maker asset. matchedFillResults.left.makerAssetFilledAmount = _safeGetPartialAmountFloor( leftMakerAssetAmountRemaining, leftTakerAssetAmountRemaining, rightMakerAssetAmountRemaining ); - // FIXME(Add comment?) + // Indicate that the profit should be set to the spread denominated in the left maker asset. leftSpread = true; } else if (rightTakerAssetAmountRemaining > leftMakerAssetAmountRemaining) { - // Case 2: Left order is fully filled with the profit paid in the right makerAsset + // Case 2: Left order is fully filled with the profit paid in the right makerAsset. + matchedFillResults.left.makerAssetFilledAmount = leftMakerAssetAmountRemaining; + matchedFillResults.left.takerAssetFilledAmount = leftTakerAssetAmountRemaining; + // Round down to ensure the right maker's exchange rate does not exceed the price specified by the order. + // We favor the right maker when the exchange rate must be rounded and the profit is being paid in the + // right maker asset. matchedFillResults.right.makerAssetFilledAmount = _safeGetPartialAmountFloor( rightMakerAssetAmountRemaining, rightTakerAssetAmountRemaining, leftMakerAssetAmountRemaining ); matchedFillResults.right.takerAssetFilledAmount = leftMakerAssetAmountRemaining; - matchedFillResults.left.makerAssetFilledAmount = leftMakerAssetAmountRemaining; - matchedFillResults.left.takerAssetFilledAmount = leftTakerAssetAmountRemaining; - // FIXME(Add comment?) + // Indicate that the profit should be set to the spread denominated in the right maker asset. rightSpread = true; } else { // Case 3: The right and left orders are fully filled @@ -359,12 +393,12 @@ contract MixinMatchOrders is matchedFillResults.right.takerAssetFilledAmount = rightTakerAssetAmountRemaining; matchedFillResults.left.makerAssetFilledAmount = leftMakerAssetAmountRemaining; matchedFillResults.left.takerAssetFilledAmount = leftTakerAssetAmountRemaining; - // FIXME(Add comment?) + // Indicate that the profit should be set to the spread denominated in the left and the right maker assets. leftSpread = true; rightSpread = true; } - // Calculate amount given to taker in the left order's maker asset + // Calculate amount given to taker in the left order's maker asset if the left spread will be part of the profit. if (leftSpread) { matchedFillResults.leftMakerAssetSpreadAmount = _safeSub( matchedFillResults.left.makerAssetFilledAmount, @@ -372,7 +406,7 @@ contract MixinMatchOrders is ); } - // Calculate amount given to taker in the right order's maker asset + // Calculate amount given to taker in the right order's maker asset if the right spread will be part of the profit. if (rightSpread) { matchedFillResults.rightMakerAssetSpreadAmount = _safeSub( matchedFillResults.right.makerAssetFilledAmount, @@ -501,7 +535,9 @@ contract MixinMatchOrders is /// @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. - /// @param withMaximalFill TODO + /// @param withMaximalFill A value indicating whether or not the orders should be maximally filled + /// with the profit being paid in the right maker asset, the left maker asset, + /// or both. /// @return matchedFillResults Amounts filled and fees paid by maker and taker of matched orders. function _matchOrders( LibOrder.Order memory leftOrder, diff --git a/contracts/exchange/test/match_orders.ts b/contracts/exchange/test/match_orders.ts index 373e520b3b..fffe48c078 100644 --- a/contracts/exchange/test/match_orders.ts +++ b/contracts/exchange/test/match_orders.ts @@ -1942,7 +1942,7 @@ describe('matchOrders', () => { ); }); - it('Should transfer correct amounts when left order is fully filled and values pass isRoundingErrorCeil but fail isRoundingErrorFloor', async () => { + it('Should transfer correct amounts when left order is fully filled and values pass isRoundingErrorCeil and isRoundingErrorFloor', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ makerAddress: makerAddressLeft, @@ -1967,7 +1967,7 @@ describe('matchOrders', () => { denominator, target, ); - expect(isRoundingErrorCeil).to.be.true(); + expect(isRoundingErrorCeil).to.be.false(); const isRoundingErrorFloor = await testExchange.isRoundingErrorFloor.callAsync( numerator, denominator, @@ -2128,10 +2128,6 @@ describe('matchOrders', () => { makerFee: Web3Wrapper.toBaseUnitAmount(10000, 0), takerFee: Web3Wrapper.toBaseUnitAmount(10000, 0), }); - // Note: - // The maker/taker fee percentage paid on the right order differs because - // they received different sale prices. The right maker pays a - // fee slightly lower than the right taker. const expectedTransferAmounts = { // Left Maker leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(16, 0), @@ -2140,28 +2136,9 @@ describe('matchOrders', () => { // Right Maker rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(29, 0), leftMakerAssetBoughtByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(16, 0), - rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount( - new BigNumber('33.3333333333333333'), - 16, - ), // 33.33% - // Taker - rightMakerAssetReceivedByTakerAmount: Web3Wrapper.toBaseUnitAmount(7, 0), - leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% - rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount( - new BigNumber('33.3333333333333333'), - 16, - ), // 33.33% - }; - const expectedTransferAmounts = { - // Left Maker - leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(16, 0), - leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% - rightMakerAssetBoughtByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(22, 0), - // Right Maker - leftMakerAssetBoughtByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(13, 0), rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(3333, 0), // 3333.3 repeating rounded down to 3333 // Taker - leftMakerAssetReceivedByTakerAmount: Web3Wrapper.toBaseUnitAmount(3, 0), + rightMakerAssetReceivedByTakerAmount: Web3Wrapper.toBaseUnitAmount(7, 0), leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(3333, 0), // 3333 rounded down to 3333 }; diff --git a/contracts/exchange/test/utils/match_order_tester.ts b/contracts/exchange/test/utils/match_order_tester.ts index ccab99a1d6..4d56b24be2 100644 --- a/contracts/exchange/test/utils/match_order_tester.ts +++ b/contracts/exchange/test/utils/match_order_tester.ts @@ -236,11 +236,7 @@ export class MatchOrderTester { takerAddress, ); } else { - transactionReceipt = await this._executeMatchOrdersAsync( - orders.leftOrder, - orders.rightOrder, - takerAddress, - ); + transactionReceipt = await this._executeMatchOrdersAsync(orders.leftOrder, orders.rightOrder, takerAddress); } // Simulate the fill. const matchResults = simulateMatchOrders(