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

Commit

Permalink
Updated batchMatchOrders to fix an edge case and added tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@jalextowle
jalextowle committed Jul 7, 2019
1 parent 64c5e7c commit 5f47a37
Show file tree
Hide file tree
Showing 5 changed files with 205 additions and 26 deletions.
8 changes: 4 additions & 4 deletions contracts/exchange-libs/contracts/src/LibFillResults.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ contract LibFillResults is
}

struct MatchedFillResults {
FillResults left; // Amounts filled and fees paid of left order.
FillResults right; // Amounts filled and fees paid of right order.
uint256 leftMakerAssetSpreadAmount; // Spread between price of left and right order, denominated in the left order's makerAsset, paid to taker.
uint256 rightMakerAssetSpreadAmount; // Spread between price of right and left order, denominated in the right order's makerAsset, paid to taker.
FillResults left; // Amounts filled and fees paid of left order.
FillResults right; // Amounts filled and fees paid of right order.
uint256 profitInLeftMakerAsset; // Profit taken from the left maker
uint256 profitInRightMakerAsset; // Profit taken from the right maker
}

/// @dev Adds properties of both FillResults instances.
Expand Down
25 changes: 13 additions & 12 deletions contracts/exchange/contracts/src/MixinMatchOrders.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,15 +188,15 @@ contract MixinMatchOrders is

// Calculate amount given to taker in the left order's maker asset if the left spread will be part of the profit.
if (doesLeftMakerAssetProfitExist) {
matchedFillResults.leftMakerAssetSpreadAmount = _safeSub(
matchedFillResults.profitInLeftMakerAsset = _safeSub(
matchedFillResults.left.makerAssetFilledAmount,
matchedFillResults.right.takerAssetFilledAmount
);
}

// Calculate amount given to taker in the right order's maker asset if the right spread will be part of the profit.
if (doesRightMakerAssetProfitExist) {
matchedFillResults.rightMakerAssetSpreadAmount = _safeSub(
matchedFillResults.profitInRightMakerAsset = _safeSub(
matchedFillResults.right.makerAssetFilledAmount,
matchedFillResults.left.takerAssetFilledAmount
);
Expand Down Expand Up @@ -238,7 +238,7 @@ contract MixinMatchOrders is
}

// Calculate amount given to taker
matchedFillResults.leftMakerAssetSpreadAmount = _safeSub(
matchedFillResults.profitInLeftMakerAsset = _safeSub(
matchedFillResults.left.makerAssetFilledAmount,
matchedFillResults.right.takerAssetFilledAmount
);
Expand Down Expand Up @@ -395,8 +395,8 @@ contract MixinMatchOrders is
// Without simulating all of the order matching, this program cannot know how many
// matches there will be. To ensure that batchMatchedFillResults has enough memory
// allocated for the left and the right side, we will allocate enough space for the
// maximum amount of matches (the maximum of the left and the right sides).
uint256 maxLength = _max256(leftOrders.length, rightOrders.length);
// maximum amount of matches (the left side length added to the right side length).
uint256 maxLength = leftOrders.length + rightOrders.length;
batchMatchedFillResults.left = new LibFillResults.FillResults[](maxLength);
batchMatchedFillResults.right = new LibFillResults.FillResults[](maxLength);

Expand All @@ -413,7 +413,7 @@ contract MixinMatchOrders is

// Loop infinitely (until broken inside of the loop), but keep a counter of how
// many orders have been matched.
for (matchCount = 0;; matchCount++) {
for (matchCount = 0;;) {
// Match the two orders that are pointed to by the left and right indices
LibFillResults.MatchedFillResults memory matchResults = _matchOrders(
leftOrder,
Expand All @@ -439,18 +439,20 @@ contract MixinMatchOrders is
batchMatchedFillResults.right[matchCount] = matchResults.right;
batchMatchedFillResults.profitInLeftMakerAsset = _safeAdd(
batchMatchedFillResults.profitInLeftMakerAsset,
matchResults.leftMakerAssetSpreadAmount
matchResults.profitInLeftMakerAsset
);
batchMatchedFillResults.profitInRightMakerAsset = _safeAdd(
batchMatchedFillResults.profitInRightMakerAsset,
matchResults.rightMakerAssetSpreadAmount
matchResults.profitInRightMakerAsset
);

// Increment the number of matches
matchCount++;

// If the leftOrder is filled, update the leftIdx, leftOrder, and leftSignature,
// or break out of the loop if there are no more leftOrders to match.
if (leftOrderInfo.orderTakerAssetFilledAmount >= leftOrder.takerAssetAmount) {
if (++leftIdx == leftOrders.length) {
matchCount++;
break;
} else {
leftOrder = leftOrders[leftIdx];
Expand All @@ -462,7 +464,6 @@ contract MixinMatchOrders is
// or break out of the loop if there are no more rightOrders to match.
if (rightOrderInfo.orderTakerAssetFilledAmount >= rightOrder.takerAssetAmount) {
if (++rightIdx == rightOrders.length) {
matchCount++;
break;
} else {
rightOrder = rightOrders[rightIdx];
Expand Down Expand Up @@ -647,15 +648,15 @@ contract MixinMatchOrders is
leftOrder.makerAssetData,
leftOrder.makerAddress,
takerAddress,
matchedFillResults.leftMakerAssetSpreadAmount
matchedFillResults.profitInLeftMakerAsset
);

_dispatchTransferFrom(
rightOrderHash,
rightOrder.makerAssetData,
rightOrder.makerAddress,
takerAddress,
matchedFillResults.rightMakerAssetSpreadAmount
matchedFillResults.profitInRightMakerAsset
);

// Settle taker fees.
Expand Down
178 changes: 178 additions & 0 deletions contracts/exchange/test/match_orders.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2576,6 +2576,83 @@ describe('matchOrders', () => {
false,
);
});
it('should have three order matchings with only two left orders and two right orders', async () => {
const leftOrders = [
await orderFactoryLeft.newSignedOrderAsync({
makerAddress: makerAddressLeft,
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(4, 0),
takerAssetAmount: Web3Wrapper.toBaseUnitAmount(2, 0),
feeRecipientAddress: feeRecipientAddressLeft,
}),
await orderFactoryLeft.newSignedOrderAsync({
makerAddress: makerAddressLeft,
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(2, 0),
takerAssetAmount: Web3Wrapper.toBaseUnitAmount(1, 0),
feeRecipientAddress: feeRecipientAddressLeft,
}),
];
const rightOrders = [
await orderFactoryRight.newSignedOrderAsync({
makerAddress: makerAddressRight,
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(1, 0),
takerAssetAmount: Web3Wrapper.toBaseUnitAmount(2, 0),
feeRecipientAddress: feeRecipientAddressRight,
}),
await orderFactoryRight.newSignedOrderAsync({
makerAddress: makerAddressRight,
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(2, 0),
takerAssetAmount: Web3Wrapper.toBaseUnitAmount(4, 0),
feeRecipientAddress: feeRecipientAddressRight,
}),
];
const expectedTransferAmounts = [
{
// Left Maker
leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(2, 0),
leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(50, 16), // 50%
// Right Maker
rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(1, 0),
rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100%
// Taker
leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(50, 16), // 50%
rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100%
},
{
// Left Maker
leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(2, 0),
leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(50, 16), // 50%
// Right Maker
rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(1, 0),
rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(50, 16), // 50%
// Taker
leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(50, 16), // 50%
rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(50, 16), // 50%
},
{
// Left Maker
leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(2, 0),
leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100%
// Right Maker
rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(1, 0),
rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(50, 16), // 50%
// Taker
leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100%
rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(50, 16), // 50%
},
];
await matchOrderTester.batchMatchOrdersAndAssertEffectsAsync(
{
leftOrders,
rightOrders,
leftOrdersTakerAssetFilledAmounts: [ZERO, ZERO],
rightOrdersTakerAssetFilledAmounts: [ZERO, ZERO],
},
takerAddress,
[[0, 0], [0, 1], [1, 1]],
expectedTransferAmounts,
false,
);
});
});
describe('batchMatchOrdersWithMaximalFill', () => {
it('should fully fill the the right order and pay the profit denominated in the left maker asset', async () => {
Expand Down Expand Up @@ -2736,6 +2813,107 @@ describe('matchOrders', () => {
true,
);
});
it('should correctly fill all four orders in three matches', async () => {
const leftOrders = [
await orderFactoryLeft.newSignedOrderAsync({
makerAddress: makerAddressLeft,
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(2, 0),
takerAssetAmount: Web3Wrapper.toBaseUnitAmount(1, 0),
feeRecipientAddress: feeRecipientAddressLeft,
}),
await orderFactoryLeft.newSignedOrderAsync({
makerAddress: makerAddressLeft,
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(72, 0),
takerAssetAmount: Web3Wrapper.toBaseUnitAmount(36, 0),
feeRecipientAddress: feeRecipientAddressLeft,
}),
];
const rightOrders = [
await orderFactoryRight.newSignedOrderAsync({
makerAddress: makerAddressRight,
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(15, 0),
takerAssetAmount: Web3Wrapper.toBaseUnitAmount(30, 0),
feeRecipientAddress: feeRecipientAddressRight,
}),
await orderFactoryRight.newSignedOrderAsync({
makerAddress: makerAddressRight,
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(22, 0),
takerAssetAmount: Web3Wrapper.toBaseUnitAmount(44, 0),
feeRecipientAddress: feeRecipientAddressRight,
}),
];
const expectedTransferAmounts = [
{
// Left Maker
leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(2, 0),
leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100%
// Right Maker
rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(1, 0),
rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(
new BigNumber('6.6666666666666666'),
16,
), // 6.66%
// Taker
leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100%
rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(
new BigNumber('6.6666666666666666'),
16,
), // 6.66%
},
{
// Left Maker
leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(28, 0),
leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(
new BigNumber('38.8888888888888888'),
16,
), // 38.88%
// Right Maker
rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(14, 0),
rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(
new BigNumber('93.3333333333333333'),
16,
), // 93.33%
// Taker
leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(
new BigNumber('38.8888888888888888'),
16,
), // 38.88%
rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(
new BigNumber('93.3333333333333333'),
16,
), // 93.33%
},
{
// Left Maker
leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(44, 0),
leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(
new BigNumber('61.1111111111111111'),
16,
), // 61.11%
// Right Maker
rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(22, 0),
rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100%
// Taker
leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(
new BigNumber('61.1111111111111111'),
16,
), // 61.11%
rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100%
},
];
await matchOrderTester.batchMatchOrdersAndAssertEffectsAsync(
{
leftOrders,
rightOrders,
leftOrdersTakerAssetFilledAmounts: [ZERO, ZERO],
rightOrdersTakerAssetFilledAmounts: [ZERO, ZERO],
},
takerAddress,
[[0, 0], [1, 0], [1, 1]],
expectedTransferAmounts,
true,
);
});
});
});
// tslint:disable-line:max-file-line-count
16 changes: 8 additions & 8 deletions contracts/exchange/test/utils/match_order_tester.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1112,19 +1112,19 @@ function convertToBatchMatchResults(results: BatchMatchResults): BatchMatchedFil
*/
function convertToMatchResults(result: MatchResults): MatchedFillResults {
// If the left spread is negative, set it to zero
let leftMakerAssetSpreadAmount = result.fills[0].makerAssetFilledAmount.minus(
let profitInLeftMakerAsset = result.fills[0].makerAssetFilledAmount.minus(
result.fills[1].takerAssetFilledAmount,
);
if (leftMakerAssetSpreadAmount.isLessThanOrEqualTo(ZERO)) {
leftMakerAssetSpreadAmount = ZERO;
if (profitInLeftMakerAsset.isLessThanOrEqualTo(ZERO)) {
profitInLeftMakerAsset = ZERO;
}

// If the right spread is negative, set it to zero
let rightMakerAssetSpreadAmount = result.fills[1].makerAssetFilledAmount.minus(
let profitInRightMakerAsset = result.fills[1].makerAssetFilledAmount.minus(
result.fills[0].takerAssetFilledAmount,
);
if (rightMakerAssetSpreadAmount.isLessThanOrEqualTo(ZERO)) {
rightMakerAssetSpreadAmount = ZERO;
if (profitInRightMakerAsset.isLessThanOrEqualTo(ZERO)) {
profitInRightMakerAsset = ZERO;
}

const matchedFillResults: MatchedFillResults = {
Expand All @@ -1140,8 +1140,8 @@ function convertToMatchResults(result: MatchResults): MatchedFillResults {
makerFeePaid: result.fills[1].makerFeePaid,
takerFeePaid: result.fills[1].takerFeePaid,
},
leftMakerAssetSpreadAmount,
rightMakerAssetSpreadAmount,
profitInLeftMakerAsset,
profitInRightMakerAsset,
};
return matchedFillResults;
}
Expand Down
4 changes: 2 additions & 2 deletions contracts/test-utils/src/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,8 @@ export interface FillResults {
export interface MatchedFillResults {
left: FillResults;
right: FillResults;
leftMakerAssetSpreadAmount: BigNumber;
rightMakerAssetSpreadAmount: BigNumber;
profitInLeftMakerAsset: BigNumber;
profitInRightMakerAsset: BigNumber;
}

export interface BatchMatchedFillResults {
Expand Down

0 comments on commit 5f47a37

Please sign in to comment.