diff options
Diffstat (limited to 'contracts')
| -rw-r--r-- | contracts/extensions/contracts/OrderMatcher/MixinMatchOrders.sol | 322 | ||||
| -rw-r--r-- | contracts/extensions/test/extensions/order_matcher.ts | 44 |
2 files changed, 242 insertions, 124 deletions
diff --git a/contracts/extensions/contracts/OrderMatcher/MixinMatchOrders.sol b/contracts/extensions/contracts/OrderMatcher/MixinMatchOrders.sol index c691b732f..b6bc83af9 100644 --- a/contracts/extensions/contracts/OrderMatcher/MixinMatchOrders.sol +++ b/contracts/extensions/contracts/OrderMatcher/MixinMatchOrders.sol @@ -25,7 +25,7 @@ import "@0x/contracts-utils/contracts/utils/Ownable/Ownable.sol"; contract MixinMatchOrders is Ownable, LibConstants -{ +{ // The below assembly in the fallback function is functionaly equivalent to the following Solidity code: /* /// @dev Match two complementary orders that have a profitable spread. @@ -54,21 +54,33 @@ contract MixinMatchOrders is rightSignature ); - // If a spread was taken, use the spread to fill remaining amount of `rightOrder` - // Only attempt to fill `rightOrder` if a spread was taken and if not already completely filled uint256 leftMakerAssetSpreadAmount = matchedFillResults.leftMakerAssetSpreadAmount; - if (leftMakerAssetSpreadAmount > 0 && matchedFillResults.right.takerAssetFilledAmount < rightOrder.takerAssetAmount) { - // The `assetData` fields of the `rightOrder` could have been null for the `matchOrders` call. We reassign them before calling `fillOrder`. - rightOrder.makerAssetData = leftOrder.takerAssetData; - rightOrder.takerAssetData = leftOrder.makerAssetData; - - // We do not need to pass in a signature since it was already validated in the `matchOrders` call - EXCHANGE.fillOrder( - rightOrder, - leftMakerAssetSpreadAmount, - "" - ); + uint256 rightOrderTakerAssetAmount = rightOrder.takerAssetAmount; + + // Do not attempt to call `fillOrder` if no spread was taken or `rightOrder` has been completely filled + if (leftMakerAssetSpreadAmount == 0 || matchedFillResults.right.takerAssetFilledAmount == rightOrderTakerAssetAmount) { + return; + } + + // The `assetData` fields of the `rightOrder` could have been null for the `matchOrders` call. We reassign them before calling `fillOrder`. + rightOrder.makerAssetData = leftOrder.takerAssetData; + rightOrder.takerAssetData = leftOrder.makerAssetData; + + // Query `rightOrder` info to check if it has been completely filled + // We need to make this check in case the `rightOrder` was partially filled before the `matchOrders` call + OrderInfo memory orderInfo = EXCHANGE.getOrderInfo(rightOrder); + + // Do not attempt to call `fillOrder` if order has been completely filled + if (orderInfo.orderTakerAssetFilledAmount == rightOrderTakerAssetAmount) { + return; } + + // We do not need to pass in a signature since it was already validated in the `matchOrders` call + EXCHANGE.fillOrder( + rightOrder, + leftMakerAssetSpreadAmount, + "" + ); } */ // solhint-disable-next-line payable-fallback @@ -101,7 +113,11 @@ contract MixinMatchOrders is // Copy calldata to memory // The calldata should be identical to the the ABIv2 encoded data required to call `EXCHANGE.matchOrders` - calldatacopy(0, 0, calldatasize()) + calldatacopy( + 0, // copy to memory at 0 + 0, // copy from calldata at 0 + calldatasize() // copy all calldata + ) // At this point, calldata and memory have the following layout: @@ -153,7 +169,7 @@ contract MixinMatchOrders is // | 1092 + a + b + c + d + e | f | rightSignature Contents | // Call `EXCHANGE.matchOrders` - let matchOrdersSuccess := call( + let success := call( gas, // forward all gas exchange, // call address of Exchange contract 0, // transfer 0 wei @@ -163,12 +179,17 @@ contract MixinMatchOrders is 288 // length of output is 288 bytes ) - if iszero(matchOrdersSuccess) { - // Revert with reason if `matchOrders` call was unsuccessful + if iszero(success) { + // Revert with reason if `EXCHANGE.matchOrders` call was unsuccessful + returndatacopy( + 0, // copy to memory at 0 + 0, // copy from return data at 0 + returndatasize() // copy all return data + ) revert(0, returndatasize()) } - // After calling `matchOrders`, the relevant parts of memory are: + // After calling `matchOrders`, the return data is layed out in memory as: // | Offset | Length | Contents | // |---------------------------|---------|-------------------------------------------- | @@ -182,6 +203,67 @@ contract MixinMatchOrders is // | 192 | | 7. right.makerFeePaid | // | 224 | | 8. right.takerFeePaid | // | 256 | | 9. leftMakerAssetSpreadAmount | + + let rightOrderStart := add(calldataload(36), 4) + + // If no spread was taken or if the entire `rightOrderTakerAssetAmount` has been filled, there is no need to call `EXCHANGE.fillOrder`. + if or( + iszero(mload(256)), // iszero(leftMakerAssetSpreadAmount) + eq(mload(160), calldataload(add(rightOrderStart, 160))) // eq(rightOrderTakerAssetFilledAmount, rightOrderTakerAssetAmount) + ) { + return(0, 0) + } + + // We assume that `leftOrder.makerAssetData == rightOrder.takerAssetData` and `leftOrder.takerAssetData == rightOrder.makerAssetData` + // `EXCHANGE.matchOrders` already makes this assumption, so it is likely + // that the `rightMakerAssetData` and `rightTakerAssetData` in calldata are empty + + let leftOrderStart := add(calldataload(4), 4) + + // Calculate locations of `leftMakerAssetData` and `leftTakerAssetData` in calldata + let leftMakerAssetDataStart := add( + leftOrderStart, + calldataload(add(leftOrderStart, 320)) // offset to `leftMakerAssetData` + ) + let leftTakerAssetDataStart := add( + leftOrderStart, + calldataload(add(leftOrderStart, 352)) // offset to `leftTakerAssetData` + ) + + // Load lengths of `leftMakerAssetData` and `leftTakerAssetData` + let leftMakerAssetDataLen := calldataload(leftMakerAssetDataStart) + let leftTakerAssetDataLen := calldataload(leftTakerAssetDataStart) + + // Write offset to `rightMakerAssetData` + mstore(add(rightOrderStart, 320), 384) + + // Write offset to `rightTakerAssetData` + let rightTakerAssetDataOffset := add(416, leftTakerAssetDataLen) + mstore(add(rightOrderStart, 352), rightTakerAssetDataOffset) + + // Copy `leftTakerAssetData` from calldata onto `rightMakerAssetData` in memory + calldatacopy( + add(rightOrderStart, 384), // `rightMakerAssetDataStart` + leftTakerAssetDataStart, + add(leftTakerAssetDataLen, 32) + ) + + // Copy `leftMakerAssetData` from calldata onto `rightTakerAssetData` in memory + calldatacopy( + add(rightOrderStart, rightTakerAssetDataOffset), // `rightTakerAssetDataStart` + leftMakerAssetDataStart, + add(leftMakerAssetDataLen, 32) + ) + + // We must call `EXCHANGE.getOrderInfo(rightOrder)` before calling `EXCHANGE.fillOrder` to ensure that the order + // has not already been entirely filled (which is possible if an order was partially filled before the `matchOrders` call). + // We want the following layout in memory before calling `getOrderInfo`: + + // | Offset | Length | Contents | + // |---------------------------|---------|-------------------------------------------- | + // | 544 + a + b | 4 | `getOrderInfo` function selector | + // | | 3 * 32 | function parameters: | + // | 548 + a + b | | 1. offset to rightOrder (*) | // | | 12 * 32 | rightOrder: | // | 580 + a + b | | 1. senderAddress | // | 612 + a + b | | 2. makerAddress | @@ -195,126 +277,124 @@ contract MixinMatchOrders is // | 868 + a + b | | 10. salt | // | 900 + a + b | | 11. offset to rightMakerAssetData (*) | // | 932 + a + b | | 12. offset to rightTakerAssetData (*) | + // | 964 + a + b | 32 | rightMakerAssetData Length | + // | 996 + a + b | c | rightMakerAssetData Contents | + // | 996 + a + b + c | 32 | rightTakerAssetData Length | + // | 1028 + a + b + c | d | rightTakerAssetData Contents | - let rightOrderStart := add(calldataload(36), 4) + // function selector (4 bytes) + 1 param (32 bytes) must be stored before `rightOrderStart` + let cdStart := sub(rightOrderStart, 36) - // Only call `fillOrder` if a spread was taken and `rightOrder` has not been completely filled - if and( - gt(mload(256), 0), // gt(leftMakerAssetSpreadAmount, 0) - lt(mload(160), calldataload(add(rightOrderStart, 160))) // lt(rightOrderTakerAssetFilledAmount, rightOrderTakerAssetAmount) - ) { - - // We want the following layout in memory before calling `fillOrder`: - - // | Offset | Length | Contents | - // |---------------------------|---------|-------------------------------------------- | - // | 480 + a + b | 4 | `fillOrder` function selector | - // | | 3 * 32 | function parameters: | - // | 484 + a + b | | 1. offset to rightOrder (*) | - // | 516 + a + b | | 2. takerAssetFillAmount | - // | 548 + a + b | | 3. offset to rightSignature (*) | - // | | 12 * 32 | rightOrder: | - // | 580 + a + b | | 1. senderAddress | - // | 612 + a + b | | 2. makerAddress | - // | 644 + a + b | | 3. takerAddress | - // | 676 + a + b | | 4. feeRecipientAddress | - // | 708 + a + b | | 5. makerAssetAmount | - // | 740 + a + b | | 6. takerAssetAmount | - // | 772 + a + b | | 7. makerFeeAmount | - // | 804 + a + b | | 8. takerFeeAmount | - // | 836 + a + b | | 9. expirationTimeSeconds | - // | 868 + a + b | | 10. salt | - // | 900 + a + b | | 11. offset to rightMakerAssetData (*) | - // | 932 + a + b | | 12. offset to rightTakerAssetData (*) | - // | 964 + a + b | 32 | rightMakerAssetData Length | - // | 996 + a + b | c | rightMakerAssetData Contents | - // | 996 + a + b + c | 32 | rightTakerAssetData Length | - // | 1028 + a + b + c | d | rightTakerAssetData Contents | - // | 1028 + a + b + c + d | 32 | rightSignature Length (always 0) | - - // We assume that `leftOrder.makerAssetData == rightOrder.takerAssetData` and `leftOrder.takerAssetData == rightOrder.makerAssetData` - // `EXCHANGE.matchOrders` already makes this assumption, so it is likely - // that the `rightMakerAssetData` and `rightTakerAssetData` in calldata are empty - - let leftOrderStart := add(calldataload(4), 4) - - // Calculate locations of `leftMakerAssetData` and `leftTakerAssetData` in calldata - let leftMakerAssetDataStart := add( - leftOrderStart, - calldataload(add(leftOrderStart, 320)) // offset to `leftMakerAssetData` - ) - let leftTakerAssetDataStart := add( - leftOrderStart, - calldataload(add(leftOrderStart, 352)) // offset to `leftTakerAssetData` - ) + // `getOrderInfo` selector = 0xc75e0a81 + mstore(cdStart, 0xc75e0a8100000000000000000000000000000000000000000000000000000000) - // Load lengths of `leftMakerAssetData` and `leftTakerAssetData` - let leftMakerAssetDataLen := calldataload(leftMakerAssetDataStart) - let leftTakerAssetDataLen := calldataload(leftTakerAssetDataStart) + // Write offset to `rightOrder` + mstore(add(cdStart, 4), 32) - // Write offset to `rightMakerAssetData` - mstore(add(rightOrderStart, 320), 384) + let rightOrderEnd := add( + add(rightOrderStart, 484), + add(leftMakerAssetDataLen, leftTakerAssetDataLen) + ) - // Write offset to `rightTakerAssetData` - let rightTakerAssetDataOffset := add(416, leftTakerAssetDataLen) - mstore(add(rightOrderStart, 352), rightTakerAssetDataOffset) + // Call `EXCHANGE.getOrderInfo(rightOrder)` + success := staticcall( + gas, // forward all gas + exchange, // call address of Exchange contract + cdStart, // start of input + sub(rightOrderEnd, cdStart), // length of input + 0, // write output over old output + 96 // output is 96 bytes + ) - // Copy `leftTakerAssetData` from calldata onto `rightMakerAssetData` in memory - calldatacopy( - add(rightOrderStart, 384), // `rightMakerAssetDataStart` - leftTakerAssetDataStart, - add(leftTakerAssetDataLen, 32) - ) + // After calling `EXCHANGE.getOrderInfo`, the return data is layed out in memory as: - // Copy `leftMakerAssetData` from calldata onto `rightTakerAssetData` in memory - calldatacopy( - add(rightOrderStart, rightTakerAssetDataOffset), // `rightTakerAssetDataStart` - leftMakerAssetDataStart, - add(leftMakerAssetDataLen, 32) - ) + // | Offset | Length | Contents | + // |---------------------------|---------|-------------------------------------------- | + // | | 3 * 32 | orderInfo | + // | 0 | | 1. orderStatus | + // | 32 | | 2. orderHash | + // | 64 | | 3. orderTakerAssetFilledAmount | + + // We do not need to check if the `getOrderInfo` call was successful because it has no possibility of reverting + // If order has been entirely filled, there is no need to call `EXCHANGE.fillOrder` + // eq(orderTakerAssetFilledAmount, rightOrderTakerAssetAmount) + if eq(mload(64), calldataload(add(rightOrderStart, 160))) { + return(0, 0) + } - // Write length of signature (always 0 since signature was previously validated) - let rightSignatureStart := add( - add(rightOrderStart, rightTakerAssetDataOffset), // `rightTakerAssetDataStart` - add(leftMakerAssetDataLen, 32) - ) - mstore(rightSignatureStart, 0) + // We want the following layout in memory before calling `EXCHANGE.fillOrder`: - // function selector (4 bytes) + 3 params (3 * 32 bytes) must be stored before `rightOrderStart` - let cdStart := sub(rightOrderStart, 100) + // | Offset | Length | Contents | + // |---------------------------|---------|-------------------------------------------- | + // | 480 + a + b | 4 | `fillOrder` function selector | + // | | 3 * 32 | function parameters: | + // | 484 + a + b | | 1. offset to rightOrder (*) | + // | 516 + a + b | | 2. takerAssetFillAmount | + // | 548 + a + b | | 3. offset to rightSignature (*) | + // | | 12 * 32 | rightOrder: | + // | 580 + a + b | | 1. senderAddress | + // | 612 + a + b | | 2. makerAddress | + // | 644 + a + b | | 3. takerAddress | + // | 676 + a + b | | 4. feeRecipientAddress | + // | 708 + a + b | | 5. makerAssetAmount | + // | 740 + a + b | | 6. takerAssetAmount | + // | 772 + a + b | | 7. makerFeeAmount | + // | 804 + a + b | | 8. takerFeeAmount | + // | 836 + a + b | | 9. expirationTimeSeconds | + // | 868 + a + b | | 10. salt | + // | 900 + a + b | | 11. offset to rightMakerAssetData (*) | + // | 932 + a + b | | 12. offset to rightTakerAssetData (*) | + // | 964 + a + b | 32 | rightMakerAssetData Length | + // | 996 + a + b | c | rightMakerAssetData Contents | + // | 996 + a + b + c | 32 | rightTakerAssetData Length | + // | 1028 + a + b + c | d | rightTakerAssetData Contents | + // | 1028 + a + b + c + d | 32 | rightSignature Length (always 0) | - // `fillOrder` selector = 0xb4be83d5 - mstore(cdStart, 0xb4be83d500000000000000000000000000000000000000000000000000000000) + // Write length of signature (always 0 since signature was previously validated) + mstore(rightOrderEnd, 0) - // Write offset to `rightOrder` - mstore(add(cdStart, 4), 96) + // function selector (4 bytes) + 3 params (3 * 32 bytes) must be stored before `rightOrderStart` + cdStart := sub(rightOrderStart, 100) - // Write `takerAssetFillAmount`, which will be the `leftMakerAssetSpreadAmount` received from the `matchOrders` call - mstore(add(cdStart, 36), mload(256)) + // `fillOrder` selector = 0xb4be83d5 + mstore(cdStart, 0xb4be83d500000000000000000000000000000000000000000000000000000000) - // Write offset to `rightSignature` - mstore(add(cdStart, 68), sub(rightSignatureStart, add(cdStart, 4))) + // Write offset to `rightOrder` + mstore(add(cdStart, 4), 96) - let fillOrderSuccess := call( - gas, // forward all gas - exchange, // call address of Exchange contract - 0, // transfer 0 wei - cdStart, // start of input - sub(add(rightSignatureStart, 32), cdStart), // length of input is end - start - 0, // write output over input - 128 // length of output is 128 bytes - ) + // Write `takerAssetFillAmount`, which will be the `leftMakerAssetSpreadAmount` received from the `matchOrders` call + mstore(add(cdStart, 36), mload(256)) - if fillOrderSuccess { - return(0, 0) - } - - // Revert with reason if `fillOrder` call was unsuccessful + // Write offset to `rightSignature` + mstore( + add(cdStart, 68), + sub(rightOrderEnd, add(cdStart, 4)) + ) + + // Call `EXCHANGE.fillOrder(rightOrder, leftMakerAssetSpreadAmount, "")` + success := call( + gas, // forward all gas + exchange, // call address of Exchange contract + 0, // transfer 0 wei + cdStart, // start of input + add(sub(rightOrderEnd, cdStart), 32), // length of input is end - start + 0, // write output over input + 0 // do not write output to memory + ) + + if iszero(success) { + // Revert with reason if `EXCHANGE.fillOrder` call was unsuccessful + returndatacopy( + 0, // copy to memory at 0 + 0, // copy from return data at 0 + returndatasize() // copy all return data + ) revert(0, returndatasize()) } - - // Return if `matchOrders` call successful and `fillOrder` was not called + + // Return if `EXCHANGE.fillOrder` did not revert return(0, 0) + } // Revert if undefined function is called diff --git a/contracts/extensions/test/extensions/order_matcher.ts b/contracts/extensions/test/extensions/order_matcher.ts index 45002b324..acb46ced4 100644 --- a/contracts/extensions/test/extensions/order_matcher.ts +++ b/contracts/extensions/test/extensions/order_matcher.ts @@ -435,7 +435,7 @@ describe('OrderMatcher', () => { initialLeftTakerAssetTakerBalance.plus(expectedTransferAmounts.leftTakerAssetSpreadAmount), ); }); - it('should not call fillOrder when rightOrder is completely filled after matchOrders call', async () => { + it('should not call fillOrder when rightOrder is completely filled after matchOrders call and orders were never partially filled', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), @@ -443,7 +443,45 @@ describe('OrderMatcher', () => { }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + }); + const data = exchange.matchOrders.getABIEncodedTransactionData( + signedOrderLeft, + signedOrderRight, + signedOrderLeft.signature, + signedOrderRight.signature, + ); + const logDecoder = new LogDecoder(web3Wrapper, { ...artifacts, ...tokenArtifacts, ...protocolArtifacts }); + const txReceipt = await logDecoder.getTxWithDecodedLogsAsync( + await web3Wrapper.sendTransactionAsync({ + data, + to: orderMatcher.address, + from: owner, + gas: constants.MAX_MATCH_ORDERS_GAS, + }), + ); + const fillLogs = _.filter( + txReceipt.logs, + log => (log as LogWithDecodedArgs<ExchangeFillEventArgs>).event === 'Fill', + ); + // Only 2 Fill logs should exist for `matchOrders` call. `fillOrder` should not have been called and should not have emitted a Fill event. + expect(fillLogs.length).to.be.equal(2); + }); + it('should not call fillOrder when rightOrder is completely filled after matchOrders call and orders were initially partially filled', async () => { + // Create orders to match + const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + }); + const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + }); + await exchangeWrapper.fillOrderAsync(signedOrderLeft, takerAddress, { + takerAssetFillAmount: signedOrderLeft.takerAssetAmount.dividedToIntegerBy(5), + }); + await exchangeWrapper.fillOrderAsync(signedOrderRight, takerAddress, { + takerAssetFillAmount: signedOrderRight.takerAssetAmount.dividedToIntegerBy(5), }); const data = exchange.matchOrders.getABIEncodedTransactionData( signedOrderLeft, @@ -451,7 +489,7 @@ describe('OrderMatcher', () => { signedOrderLeft.signature, signedOrderRight.signature, ); - const logDecoder = new LogDecoder(web3Wrapper, { ...artifacts, ...tokenArtifacts }); + const logDecoder = new LogDecoder(web3Wrapper, { ...artifacts, ...tokenArtifacts, ...protocolArtifacts }); const txReceipt = await logDecoder.getTxWithDecodedLogsAsync( await web3Wrapper.sendTransactionAsync({ data, |