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

Commit

Permalink
Adapted tests from matchOrders to test matchOrdersWithMaximalFill
Browse files Browse the repository at this point in the history
  • Loading branch information
@jalextowle
jalextowle committed Jul 7, 2019
1 parent 5f47a37 commit 7d2c464
Show file tree
Hide file tree
Showing 2 changed files with 262 additions and 7 deletions.
261 changes: 260 additions & 1 deletion contracts/exchange/test/match_orders.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2187,7 +2187,7 @@ describe('matchOrders', () => {
// Taker
rightMakerAssetReceivedByTakerAmount: Web3Wrapper.toBaseUnitAmount(7, 0),
leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100%
rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(3333, 0), // 3333 rounded down to 3333
rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(3333, 0), // 3333.3 repeating rounded down to 3333
};
// Match signedOrderLeft with signedOrderRight
await matchOrderTester.matchOrdersAndAssertEffectsAsync(
Expand All @@ -2200,6 +2200,265 @@ describe('matchOrders', () => {
true,
);
});

it('Should give left maker and left taker a favorable fee price when rounding', async () => {
// Create orders to match
const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
makerAddress: makerAddressLeft,
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(12, 0),
takerAssetAmount: Web3Wrapper.toBaseUnitAmount(97, 0),
feeRecipientAddress: feeRecipientAddressLeft,
makerFee: Web3Wrapper.toBaseUnitAmount(10000, 0),
takerFee: Web3Wrapper.toBaseUnitAmount(10000, 0),
});
const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
makerAddress: makerAddressRight,
makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(89, 0),
takerAssetAmount: Web3Wrapper.toBaseUnitAmount(1, 0),
feeRecipientAddress: feeRecipientAddressRight,
});
// Note:
// The maker/taker fee percentage paid on the left order differs because
// they received different sale prices. The left maker pays a
// fee slightly lower than the left taker.
const expectedTransferAmounts = {
// Left Maker
leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(11, 0),
leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(9166, 0), // 9166.6 rounded down to 9166
// Right Maker
rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(89, 0),
leftMakerAssetBoughtByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(1, 0),
rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100%
// Taker
leftMakerAssetReceivedByTakerAmount: Web3Wrapper.toBaseUnitAmount(10, 0),
leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(9175, 0), // 9175.2 rounded down to 9175
rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100%
};
// Match signedOrderLeft with signedOrderRight
await matchOrderTester.matchOrdersAndAssertEffectsAsync(
{
leftOrder: signedOrderLeft,
rightOrder: signedOrderRight,
},
takerAddress,
expectedTransferAmounts,
true,
);
});

it('Should give left maker a better sell price when rounding', async () => {
// Create orders to match
const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
makerAddress: makerAddressLeft,
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(12, 0),
takerAssetAmount: Web3Wrapper.toBaseUnitAmount(97, 0),
feeRecipientAddress: feeRecipientAddressLeft,
});
const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
makerAddress: makerAddressRight,
makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(89, 0),
takerAssetAmount: Web3Wrapper.toBaseUnitAmount(1, 0),
feeRecipientAddress: feeRecipientAddressRight,
});
// Note:
// The maker/taker fee percentage paid on the left order differs because
// they received different sale prices. The left maker pays a fee
// slightly lower than the left taker.
const expectedTransferAmounts = {
// Left Maker
leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(11, 0),
leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(
new BigNumber('91.6666666666666666'),
16,
), // 91.6%
// Right Maker
rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(89, 0),
leftMakerAssetBoughtByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(1, 0),
rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100%
// Taker
leftMakerAssetReceivedByTakerAmount: Web3Wrapper.toBaseUnitAmount(10, 0),
leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(
new BigNumber('91.7525773195876288'),
16,
), // 91.75%
rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100%
};
// Match signedOrderLeft with signedOrderRight
await matchOrderTester.matchOrdersAndAssertEffectsAsync(
{
leftOrder: signedOrderLeft,
rightOrder: signedOrderRight,
},
takerAddress,
expectedTransferAmounts,
true,
);
});

it('should transfer the correct amounts when consecutive calls are used to completely fill the left order', async () => {
// Create orders to match
const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(50, 18),
takerAssetAmount: Web3Wrapper.toBaseUnitAmount(100, 18),
});
const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(10, 18),
takerAssetAmount: Web3Wrapper.toBaseUnitAmount(2, 18),
});
// Match orders
const expectedTransferAmounts = {
// Left Maker
leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(5, 18),
leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(10, 16), // 10%
// Right Maker
rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(10, 18),
leftMakerAssetBoughtByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(2, 18),
rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100%
// Taker
leftMakerAssetReceivedByTakerAmount: Web3Wrapper.toBaseUnitAmount(3, 18),
leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(10, 16), // 10%
rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100%
};
// prettier-ignore
const matchResults = await matchOrderTester.matchOrdersAndAssertEffectsAsync(
{
leftOrder: signedOrderLeft,
rightOrder: signedOrderRight,
},
takerAddress,
expectedTransferAmounts,
true,
);
// Construct second right order
// Note: This order needs makerAssetAmount=90/takerAssetAmount=[anything <= 45] to fully fill the right order.
// However, we use 100/50 to ensure a partial fill as we want to go down the "left fill"
// branch in the contract twice for this test.
const signedOrderRight2 = await orderFactoryRight.newSignedOrderAsync({
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(100, 18),
takerAssetAmount: Web3Wrapper.toBaseUnitAmount(50, 18),
});
// Match signedOrderLeft with signedOrderRight2
const expectedTransferAmounts2 = {
// Left Maker
leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(45, 18),
leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(90, 16), // 90% (10% paid earlier)
// Right Maker
rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(90, 18),
rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(90, 16), // 90%
// Taker
leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(90, 16), // 90% (10% paid earlier)
rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(90, 16), // 90%
};

await matchOrderTester.matchOrdersAndAssertEffectsAsync(
{
leftOrder: signedOrderLeft,
rightOrder: signedOrderRight2,
leftOrderTakerAssetFilledAmount: matchResults.orders.leftOrderTakerAssetFilledAmount,
},
takerAddress,
expectedTransferAmounts2,
true,
await matchOrderTester.getBalancesAsync(),
);
});

it('Should transfer correct amounts when right order fill amount deviates from amount derived by `Exchange.fillOrder`', async () => {
// Create orders to match
const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
makerAddress: makerAddressLeft,
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(1000, 0),
takerAssetAmount: Web3Wrapper.toBaseUnitAmount(1005, 0),
feeRecipientAddress: feeRecipientAddressLeft,
});
const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
makerAddress: makerAddressRight,
makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(2126, 0),
takerAssetAmount: Web3Wrapper.toBaseUnitAmount(1063, 0),
feeRecipientAddress: feeRecipientAddressRight,
});
const expectedTransferAmounts = {
// Left Maker
leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(1000, 0),
rightMakerAssetBoughtByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(1005, 0),
leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100%
// Right Maker
// Notes:
// i.
// The left order is fully filled by the right order, so the right maker must sell 1005 units of their asset to the left maker.
// By selling 1005 units, the right maker should theoretically receive 502.5 units of the left maker's asset.
// Since the transfer amount must be an integer, this value must be rounded down to 502 or up to 503.
// ii.
// If the right order were filled via `Exchange.fillOrder` the respective fill amounts would be [1004, 502] or [1006, 503].
// It follows that we cannot trigger a sale of 1005 units of the right maker's asset through `Exchange.fillOrder`.
// iii.
// For an optimal match, the algorithm must choose either [1005, 502] or [1005, 503] as fill amounts for the right order.
// The algorithm favors the right maker when the exchange rate must be rounded, so the final fill for the right order is [1005, 503].
// iv.
// The right maker fee differs from the right taker fee because their exchange rate differs.
// The right maker always receives the better exchange and fee price.
rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(2000, 0),
rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(
new BigNumber('94.0733772342427093'),
16,
), // 94.07%
// Taker
rightMakerAssetReceivedByTakerAmount: Web3Wrapper.toBaseUnitAmount(995, 0),
leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100%
rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(
new BigNumber('94.0733772342427093'),
16,
), // 94.07%
};
// Match signedOrderLeft with signedOrderRight
await matchOrderTester.matchOrdersAndAssertEffectsAsync(
{
leftOrder: signedOrderLeft,
rightOrder: signedOrderRight,
},
takerAddress,
expectedTransferAmounts,
true,
);
});

const reentrancyTest = (functionNames: string[]) => {
_.forEach(functionNames, async (functionName: string, functionId: number) => {
const description = `should not allow matchOrdersWithMaximalFill to reenter the Exchange contract via ${functionName}`;
it(description, async () => {
const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address),
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(5, 18),
takerAssetAmount: Web3Wrapper.toBaseUnitAmount(10, 18),
});
const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
makerAddress: makerAddressRight,
takerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address),
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(10, 18),
takerAssetAmount: Web3Wrapper.toBaseUnitAmount(2, 18),
feeRecipientAddress: feeRecipientAddressRight,
});
await web3Wrapper.awaitTransactionSuccessAsync(
await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId),
constants.AWAIT_TRANSACTION_MINED_MS,
);
const tx = exchangeWrapper.matchOrdersWithMaximalFillAsync(
signedOrderLeft,
signedOrderRight,
takerAddress,
);
return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal);
});
});
};
describe('matchOrdersWithMaximalFill reentrancy tests', () =>
reentrancyTest(exchangeConstants.FUNCTIONS_WITH_MUTEX));
});

describe('batchMatchOrders and batchMatchOrdersWithMaximalFill rich errors', async () => {
Expand Down
8 changes: 2 additions & 6 deletions contracts/exchange/test/utils/match_order_tester.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1112,17 +1112,13 @@ function convertToBatchMatchResults(results: BatchMatchResults): BatchMatchedFil
*/
function convertToMatchResults(result: MatchResults): MatchedFillResults {
// If the left spread is negative, set it to zero
let profitInLeftMakerAsset = result.fills[0].makerAssetFilledAmount.minus(
result.fills[1].takerAssetFilledAmount,
);
let profitInLeftMakerAsset = result.fills[0].makerAssetFilledAmount.minus(result.fills[1].takerAssetFilledAmount);
if (profitInLeftMakerAsset.isLessThanOrEqualTo(ZERO)) {
profitInLeftMakerAsset = ZERO;
}

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

0 comments on commit 7d2c464

Please sign in to comment.