From f373aae08f7532709fcfcc799770168ea1677b64 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 | 159 ++++++++---------- .../contracts/src/interfaces/IMatchOrders.sol | 2 +- contracts/exchange/test/match_orders.ts | 29 +--- .../exchange/test/utils/match_order_tester.ts | 6 +- 4 files changed, 78 insertions(+), 118 deletions(-) diff --git a/contracts/exchange/contracts/src/MixinMatchOrders.sol b/contracts/exchange/contracts/src/MixinMatchOrders.sol index fb1e06eb12..6f3e4a4511 100644 --- a/contracts/exchange/contracts/src/MixinMatchOrders.sol +++ b/contracts/exchange/contracts/src/MixinMatchOrders.sol @@ -75,6 +75,28 @@ contract MixinMatchOrders is return _batchMatchOrders(leftOrders, rightOrders, leftSignatures, rightSignatures, true); } + /// @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. + /// The profit made by the leftOrder order goes to the taker (who matched the two orders). + /// @param leftOrder First order to match. + /// @param rightOrder Second order to match. + /// @param leftOrderTakerAssetFilledAmount Amount of left order already filled. + /// @param rightOrderTakerAssetFilledAmount Amount of right order already filled. + /// @param matchedFillResults Amounts to fill and fees to pay by maker and taker of matched orders. + function calculateMatchedFillResults( + LibOrder.Order memory leftOrder, + LibOrder.Order memory rightOrder, + uint256 leftOrderTakerAssetFilledAmount, + uint256 rightOrderTakerAssetFilledAmount + ) + public + pure + returns (LibFillResults.MatchedFillResults memory matchedFillResults) + { + return _calculateMatchedFillResults(leftOrder, rightOrder, leftOrderTakerAssetFilledAmount, rightOrderTakerAssetFilledAmount, false); + } + /// @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. @@ -146,7 +168,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 +218,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 ); } @@ -245,50 +278,6 @@ contract MixinMatchOrders is return 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 - nonReentrant - returns (LibFillResults.MatchedFillResults memory matchedFillResults) - { - return _matchOrders(leftOrder, rightOrder, leftSignature, rightSignature); - } - - /// @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. - /// The profit made by the leftOrder order goes to the taker (who matched the two orders). - /// @param leftOrder First order to match. - /// @param rightOrder Second order to match. - /// @param leftOrderTakerAssetFilledAmount Amount of left order already filled. - /// @param rightOrderTakerAssetFilledAmount Amount of right order already filled. - /// @param matchedFillResults Amounts to fill and fees to pay by maker and taker of matched orders. - function calculateMatchedFillResults( - LibOrder.Order memory leftOrder, - LibOrder.Order memory rightOrder, - uint256 leftOrderTakerAssetFilledAmount, - uint256 rightOrderTakerAssetFilledAmount, - ) - public - pure - returns (LibFillResults.MatchedFillResults memory matchedFillResults) - { - returns calculateMatchedFillResults(leftOrder, rightOrder, leftOrderTakerAssetFilledAmount, rightOrderTakerAssetFilledAmount, false); - } - /// @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. @@ -325,33 +314,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 +371,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 +384,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, @@ -450,33 +462,6 @@ contract MixinMatchOrders is return matchedFillResults; } - /// @dev Validates context for matchOrders. Succeeds or throws. - /// @param leftOrder First order to match. - /// @param rightOrder Second order to match. - function _assertValidMatch( - LibOrder.Order memory leftOrder, - LibOrder.Order memory rightOrder - ) - internal - view - { - // Make sure there is a profitable spread. - // There is a profitable spread iff the cost per unit bought (OrderA.MakerAmount/OrderA.TakerAmount) for each order is greater - // than the profit per unit sold of the matched order (OrderB.TakerAmount/OrderB.MakerAmount). - // This is satisfied by the equations below: - // / >= / - // AND - // / >= / - // These equations can be combined to get the following: - if (_safeMul(leftOrder.makerAssetAmount, rightOrder.makerAssetAmount) < - _safeMul(leftOrder.takerAssetAmount, rightOrder.takerAssetAmount)) { - _rrevert(NegativeSpreadError( - getOrderHash(leftOrder), - getOrderHash(rightOrder) - )); - } - } - /// @dev Determines whether or not an order has been fully filled and returns the result. /// @param order The order that should be checked. /// @return A boolean reflecting whether or not the order was fully filled @@ -501,7 +486,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/contracts/src/interfaces/IMatchOrders.sol b/contracts/exchange/contracts/src/interfaces/IMatchOrders.sol index 60bc4c7f7f..08808d8be7 100644 --- a/contracts/exchange/contracts/src/interfaces/IMatchOrders.sol +++ b/contracts/exchange/contracts/src/interfaces/IMatchOrders.sol @@ -77,6 +77,7 @@ contract IMatchOrders { ) public pure + 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 @@ -94,6 +95,5 @@ contract IMatchOrders { bytes memory rightSignature ) public ->>>>>>> Implemented batchMatchOrdersWithMaximalFill returns (LibFillResults.MatchedFillResults memory matchedFillResults); } 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(