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 1, 2019
1 parent 1e2d43c commit a000574
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 46 deletions.
66 changes: 51 additions & 15 deletions contracts/exchange/contracts/src/MixinMatchOrders.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
);
}

Expand Down Expand Up @@ -325,54 +336,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 @@ -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,
Expand Down
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 a000574

Please sign in to comment.