Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

Commit

Permalink
Polished MixinMatchOrders and removed unimplemented test
Browse files Browse the repository at this point in the history
  • Loading branch information
@jalextowle
jalextowle committed Jul 2, 2019
1 parent 1e2d43c commit f373aae
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 118 deletions.
159 changes: 73 additions & 86 deletions contracts/exchange/contracts/src/MixinMatchOrders.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
);
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -325,54 +314,77 @@ 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
matchedFillResults.right.makerAssetFilledAmount = rightMakerAssetAmountRemaining;
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,
matchedFillResults.right.takerAssetFilledAmount
);
}

// 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,
Expand Down Expand Up @@ -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:
// <leftOrder.makerAssetAmount> / <leftOrder.takerAssetAmount> >= <rightOrder.takerAssetAmount> / <rightOrder.makerAssetAmount>
// AND
// <rightOrder.makerAssetAmount> / <rightOrder.takerAssetAmount> >= <leftOrder.takerAssetAmount> / <leftOrder.makerAssetAmount>
// 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
Expand All @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion contracts/exchange/contracts/src/interfaces/IMatchOrders.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -94,6 +95,5 @@ contract IMatchOrders {
bytes memory rightSignature
)
public
>>>>>>> Implemented batchMatchOrdersWithMaximalFill
returns (LibFillResults.MatchedFillResults memory matchedFillResults);
}
29 changes: 3 additions & 26 deletions contracts/exchange/test/match_orders.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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),
Expand All @@ -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
};
Expand Down
6 changes: 1 addition & 5 deletions contracts/exchange/test/utils/match_order_tester.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down

0 comments on commit f373aae

Please sign in to comment.