Merge pull request #2139 from 0xProject/feat/3.0/optimize-match-orders

Optimize batchMatchOrders, consolidate TransactionSignatureError rich revert
This commit is contained in:
Amir Bandeali
2019-09-08 21:18:02 -07:00
committed by GitHub
17 changed files with 90 additions and 141 deletions

View File

@@ -50,7 +50,8 @@ library LibExchangeRichErrors {
}
enum SignatureErrorCodes {
BAD_SIGNATURE,
BAD_ORDER_SIGNATURE,
BAD_TRANSACTION_SIGNATURE,
INVALID_LENGTH,
UNSUPPORTED,
ILLEGAL,
@@ -121,10 +122,6 @@ library LibExchangeRichErrors {
bytes4 internal constant TRANSACTION_ERROR_SELECTOR =
0xf5985184;
// bytes4(keccak256("TransactionSignatureError(bytes32,address,bytes)"))
bytes4 internal constant TRANSACTION_SIGNATURE_ERROR_SELECTOR =
0xbfd56ef6;
// bytes4(keccak256("TransactionExecutionError(bytes32,bytes)"))
bytes4 internal constant TRANSACTION_EXECUTION_ERROR_SELECTOR =
0x20d11f61;
@@ -254,14 +251,6 @@ library LibExchangeRichErrors {
return TRANSACTION_ERROR_SELECTOR;
}
function TransactionSignatureErrorSelector()
internal
pure
returns (bytes4)
{
return TRANSACTION_SIGNATURE_ERROR_SELECTOR;
}
function TransactionExecutionErrorSelector()
internal
pure
@@ -538,23 +527,6 @@ library LibExchangeRichErrors {
);
}
function TransactionSignatureError(
bytes32 transactionHash,
address signerAddress,
bytes memory signature
)
internal
pure
returns (bytes memory)
{
return abi.encodeWithSelector(
TRANSACTION_SIGNATURE_ERROR_SELECTOR,
transactionHash,
signerAddress,
signature
);
}
function TransactionExecutionError(
bytes32 transactionHash,
bytes memory errorData

View File

@@ -116,8 +116,7 @@ contract MixinExchangeCore is
}
/// @dev After calling, the order can not be filled anymore.
/// Throws if order is invalid or sender does not have permission to cancel.
/// @param order Order to cancel. Order must be OrderStatus.FILLABLE.
/// @param order Order struct containing order specifications.
function cancelOrder(LibOrder.Order memory order)
public
payable
@@ -136,11 +135,8 @@ contract MixinExchangeCore is
view
returns (LibOrder.OrderInfo memory orderInfo)
{
// Compute the order hash
orderInfo.orderHash = order.getTypedDataHash(EIP712_EXCHANGE_DOMAIN_HASH);
// Fetch filled amount
orderInfo.orderTakerAssetFilledAmount = filled[orderInfo.orderHash];
// Compute the order hash and fetch the amount of takerAsset that has already been filled
(orderInfo.orderHash, orderInfo.orderTakerAssetFilledAmount) = _getOrderHashAndFilledAmount(order);
// If order.makerAssetAmount is zero, we also reject the order.
// While the Exchange contract handles them correctly, they create
@@ -220,7 +216,12 @@ contract MixinExchangeCore is
uint256 takerAssetFilledAmount = LibSafeMath.min256(takerAssetFillAmount, remainingTakerAssetAmount);
// Compute proportional fill amounts
fillResults = LibFillResults.calculateFillResults(order, takerAssetFilledAmount, protocolFeeMultiplier, tx.gasprice);
fillResults = LibFillResults.calculateFillResults(
order,
takerAssetFilledAmount,
protocolFeeMultiplier,
tx.gasprice
);
bytes32 orderHash = orderInfo.orderHash;
@@ -385,7 +386,7 @@ contract MixinExchangeCore is
)
) {
LibRichErrors.rrevert(LibExchangeRichErrors.SignatureError(
LibExchangeRichErrors.SignatureErrorCodes.BAD_SIGNATURE,
LibExchangeRichErrors.SignatureErrorCodes.BAD_ORDER_SIGNATURE,
orderInfo.orderHash,
makerAddress,
signature
@@ -488,4 +489,17 @@ contract MixinExchangeCore is
fillResults.protocolFeePaid = 0;
}
}
/// @dev Gets the order's hash and amount of takerAsset that has already been filled.
/// @param order Order struct containing order specifications.
/// @return The typed data hash and amount filled of the order.
function _getOrderHashAndFilledAmount(LibOrder.Order memory order)
internal
view
returns (bytes32 orderHash, uint256 orderTakerAssetFilledAmount)
{
orderHash = order.getTypedDataHash(EIP712_EXCHANGE_DOMAIN_HASH);
orderTakerAssetFilledAmount = filled[orderHash];
return (orderHash, orderTakerAssetFilledAmount);
}
}

View File

@@ -33,6 +33,7 @@ contract MixinMatchOrders is
{
using LibBytes for bytes;
using LibSafeMath for uint256;
using LibOrder for LibOrder.Order;
/// @dev Match complementary orders that have a profitable spread.
/// Each order is filled at their respective price point, and
@@ -165,7 +166,7 @@ contract MixinMatchOrders is
bytes32 rightOrderHash
)
internal
view
pure
{
// 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
@@ -236,11 +237,11 @@ contract MixinMatchOrders is
uint256 leftIdx = 0;
uint256 rightIdx = 0;
// Keep local variables for orders, order info, and signatures for efficiency.
// Keep local variables for orders, order filled amounts, and signatures for efficiency.
LibOrder.Order memory leftOrder = leftOrders[0];
LibOrder.Order memory rightOrder = rightOrders[0];
LibOrder.OrderInfo memory leftOrderInfo = getOrderInfo(leftOrder);
LibOrder.OrderInfo memory rightOrderInfo = getOrderInfo(rightOrder);
(, uint256 leftOrderTakerAssetFilledAmount) = _getOrderHashAndFilledAmount(leftOrder);
(, uint256 rightOrderTakerAssetFilledAmount) = _getOrderHashAndFilledAmount(rightOrder);
LibFillResults.FillResults memory leftFillResults;
LibFillResults.FillResults memory rightFillResults;
@@ -256,13 +257,9 @@ contract MixinMatchOrders is
shouldMaximallyFillOrders
);
// Update the orderInfo structs with the updated takerAssetFilledAmount
leftOrderInfo.orderTakerAssetFilledAmount = leftOrderInfo.orderTakerAssetFilledAmount.safeAdd(
matchResults.left.takerAssetFilledAmount
);
rightOrderInfo.orderTakerAssetFilledAmount = rightOrderInfo.orderTakerAssetFilledAmount.safeAdd(
matchResults.right.takerAssetFilledAmount
);
// Update the order filled amounts with the updated takerAssetFilledAmount
leftOrderTakerAssetFilledAmount = leftOrderTakerAssetFilledAmount.safeAdd(matchResults.left.takerAssetFilledAmount);
rightOrderTakerAssetFilledAmount = rightOrderTakerAssetFilledAmount.safeAdd(matchResults.right.takerAssetFilledAmount);
// Aggregate the new fill results with the previous fill results for the current orders.
leftFillResults = LibFillResults.addFillResults(
@@ -285,7 +282,7 @@ contract MixinMatchOrders is
// 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 (leftOrderTakerAssetFilledAmount >= leftOrder.takerAssetAmount) {
// Update the batched fill results once the leftIdx is updated.
batchMatchedFillResults.left[leftIdx++] = leftFillResults;
// Clear the intermediate fill results value.
@@ -299,13 +296,13 @@ contract MixinMatchOrders is
break;
} else {
leftOrder = leftOrders[leftIdx];
leftOrderInfo = getOrderInfo(leftOrder);
(, leftOrderTakerAssetFilledAmount) = _getOrderHashAndFilledAmount(leftOrder);
}
}
// If the rightOrder is filled, update the rightIdx, rightOrder, and rightSignature,
// or break out of the loop if there are no more rightOrders to match.
if (rightOrderInfo.orderTakerAssetFilledAmount >= rightOrder.takerAssetAmount) {
if (rightOrderTakerAssetFilledAmount >= rightOrder.takerAssetAmount) {
// Update the batched fill results once the rightIdx is updated.
batchMatchedFillResults.right[rightIdx++] = rightFillResults;
// Clear the intermediate fill results value.
@@ -319,7 +316,7 @@ contract MixinMatchOrders is
break;
} else {
rightOrder = rightOrders[rightIdx];
rightOrderInfo = getOrderInfo(rightOrder);
(, rightOrderTakerAssetFilledAmount) = _getOrderHashAndFilledAmount(rightOrder);
}
}
}
@@ -440,6 +437,8 @@ contract MixinMatchOrders is
)
internal
{
address leftMakerAddress = leftOrder.makerAddress;
address rightMakerAddress = rightOrder.makerAddress;
address leftFeeRecipientAddress = leftOrder.feeRecipientAddress;
address rightFeeRecipientAddress = rightOrder.feeRecipientAddress;
@@ -447,8 +446,8 @@ contract MixinMatchOrders is
_dispatchTransferFrom(
rightOrderHash,
rightOrder.makerAssetData,
rightOrder.makerAddress,
leftOrder.makerAddress,
rightMakerAddress,
leftMakerAddress,
matchedFillResults.left.takerAssetFilledAmount
);
@@ -456,8 +455,8 @@ contract MixinMatchOrders is
_dispatchTransferFrom(
leftOrderHash,
leftOrder.makerAssetData,
leftOrder.makerAddress,
rightOrder.makerAddress,
leftMakerAddress,
rightMakerAddress,
matchedFillResults.right.takerAssetFilledAmount
);
@@ -465,7 +464,7 @@ contract MixinMatchOrders is
_dispatchTransferFrom(
rightOrderHash,
rightOrder.makerFeeAssetData,
rightOrder.makerAddress,
rightMakerAddress,
rightFeeRecipientAddress,
matchedFillResults.right.makerFeePaid
);
@@ -474,7 +473,7 @@ contract MixinMatchOrders is
_dispatchTransferFrom(
leftOrderHash,
leftOrder.makerFeeAssetData,
leftOrder.makerAddress,
leftMakerAddress,
leftFeeRecipientAddress,
matchedFillResults.left.makerFeePaid
);
@@ -483,14 +482,14 @@ contract MixinMatchOrders is
_dispatchTransferFrom(
leftOrderHash,
leftOrder.makerAssetData,
leftOrder.makerAddress,
leftMakerAddress,
takerAddress,
matchedFillResults.profitInLeftMakerAsset
);
_dispatchTransferFrom(
rightOrderHash,
rightOrder.makerAssetData,
rightOrder.makerAddress,
rightMakerAddress,
takerAddress,
matchedFillResults.profitInRightMakerAsset
);
@@ -500,8 +499,8 @@ contract MixinMatchOrders is
leftOrderHash,
rightOrderHash,
matchedFillResults.left.protocolFeePaid,
leftOrder.makerAddress,
rightOrder.makerAddress,
leftMakerAddress,
rightMakerAddress,
takerAddress
);

View File

@@ -422,7 +422,7 @@ contract MixinSignatureValidator is
returns (SignatureType signatureType)
{
// Read the signatureType from the signature
SignatureType signatureType = _readSignatureType(
signatureType = _readSignatureType(
hash,
signerAddress,
signature

View File

@@ -117,7 +117,7 @@ contract MixinTransactions is
_setCurrentContextAddressIfRequired(signerAddress, address(0));
emit TransactionExecution(transactionHash);
return returnData;
}
@@ -178,7 +178,8 @@ contract MixinTransactions is
signature
)
) {
LibRichErrors.rrevert(LibExchangeRichErrors.TransactionSignatureError(
LibRichErrors.rrevert(LibExchangeRichErrors.SignatureError(
LibExchangeRichErrors.SignatureErrorCodes.BAD_TRANSACTION_SIGNATURE,
transactionHash,
signerAddress,
signature

View File

@@ -302,27 +302,6 @@ contract LibExchangeRichErrorDecoder {
errorCode = LibExchangeRichErrors.TransactionErrorCodes(_errorCode);
}
/// @dev Decompose an ABI-encoded TransactionSignatureError.
/// @param encoded ABI-encoded revert error.
/// @return transactionHash Hash of the transaction.
/// @return signerAddress Signer of the transaction.
/// @return signature Full signature for the transaction.
function decodeTransactionSignatureError(bytes memory encoded)
public
pure
returns (
bytes32 transactionHash,
address signerAddress,
bytes memory signature
)
{
_assertSelectorBytes(encoded, LibExchangeRichErrors.TransactionSignatureErrorSelector());
(transactionHash, signerAddress, signature) = abi.decode(
encoded.sliceDestructive(4, encoded.length),
(bytes32, address, bytes)
);
}
/// @dev Decompose an ABI-encoded TransactionExecutionError.
/// @param encoded ABI-encoded revert error.
/// @return transactionHash Hash of the transaction.

View File

@@ -72,8 +72,8 @@ contract IsolatedExchange is
/// @dev Overridden to simplify signature validation.
/// Unfortunately, this is `view`, so it can't log arguments.
function _isValidOrderWithHashSignature(
LibOrder.Order memory order,
bytes32 orderHash,
LibOrder.Order memory,
bytes32,
bytes memory signature
)
internal

View File

@@ -50,16 +50,16 @@ contract ReentrancyTester is
function isReentrant(bytes calldata fnCallData)
external
nonReentrant
returns (bool isReentrant)
returns (bool allowsReentrancy)
{
(bool didSucceed, bytes memory resultData) = address(this).delegatecall(fnCallData);
if (didSucceed) {
isReentrant = true;
allowsReentrancy = true;
} else {
if (resultData.equals(LibReentrancyGuardRichErrors.IllegalReentrancyError())) {
isReentrant = false;
allowsReentrancy = false;
} else {
isReentrant = true;
allowsReentrancy = true;
}
}
}
@@ -96,8 +96,8 @@ contract ReentrancyTester is
/// @dev Overridden to always succeed.
function _fillOrder(
LibOrder.Order memory order,
uint256 takerAssetFillAmount,
bytes memory signature
uint256,
bytes memory
)
internal
returns (LibFillResults.FillResults memory fillResults)
@@ -111,8 +111,8 @@ contract ReentrancyTester is
/// @dev Overridden to always succeed.
function _fillOrKillOrder(
LibOrder.Order memory order,
uint256 takerAssetFillAmount,
bytes memory signature
uint256,
bytes memory
)
internal
returns (LibFillResults.FillResults memory fillResults)
@@ -125,8 +125,8 @@ contract ReentrancyTester is
/// @dev Overridden to always succeed.
function _executeTransaction(
LibZeroExTransaction.ZeroExTransaction memory transaction,
bytes memory signature
LibZeroExTransaction.ZeroExTransaction memory,
bytes memory
)
internal
returns (bytes memory resultData)
@@ -139,9 +139,9 @@ contract ReentrancyTester is
function _batchMatchOrders(
LibOrder.Order[] memory leftOrders,
LibOrder.Order[] memory rightOrders,
bytes[] memory leftSignatures,
bytes[] memory rightSignatures,
bool shouldMaximallyFillOrders
bytes[] memory,
bytes[] memory,
bool
)
internal
returns (LibFillResults.BatchMatchedFillResults memory batchMatchedFillResults)
@@ -171,9 +171,9 @@ contract ReentrancyTester is
function _matchOrders(
LibOrder.Order memory leftOrder,
LibOrder.Order memory rightOrder,
bytes memory leftSignature,
bytes memory rightSignature,
bool shouldMaximallyFillOrders
bytes memory,
bytes memory,
bool
)
internal
returns (LibFillResults.MatchedFillResults memory matchedFillResults)

View File

@@ -253,6 +253,7 @@ contract TestProtocolFeesReceiver {
uint256 expectedProtocolFeePaid
)
internal
pure
{
// If the expected available balance was sufficient to pay the protocol fee, the protocol fee
// should have been paid in ether. Otherwise, no ether should be sent to pay the protocol fee.

View File

@@ -281,7 +281,7 @@ blockchainTests.resets('Exchange core', () => {
takerAssetFillAmount: fillAmount,
});
const expectedError = new ExchangeRevertErrors.SignatureError(
ExchangeRevertErrors.SignatureErrorCode.BadSignature,
ExchangeRevertErrors.SignatureErrorCode.BadOrderSignature,
orderHashHex,
signedOrder.makerAddress,
signedOrder.signature,
@@ -312,7 +312,7 @@ blockchainTests.resets('Exchange core', () => {
takerAssetFillAmount: fillAmount,
});
const expectedError = new ExchangeRevertErrors.SignatureError(
ExchangeRevertErrors.SignatureErrorCode.BadSignature,
ExchangeRevertErrors.SignatureErrorCode.BadOrderSignature,
orderHashHex,
signedOrder.makerAddress,
signedOrder.signature,
@@ -343,7 +343,7 @@ blockchainTests.resets('Exchange core', () => {
takerAssetFillAmount: fillAmount,
});
const expectedError = new ExchangeRevertErrors.SignatureError(
ExchangeRevertErrors.SignatureErrorCode.BadSignature,
ExchangeRevertErrors.SignatureErrorCode.BadOrderSignature,
orderHashHex,
signedOrder.makerAddress,
signedOrder.signature,

View File

@@ -507,7 +507,7 @@ blockchainTests('Isolated fillOrder() tests', env => {
const order = createOrder();
const signature = createBadSignature();
const expectedError = new ExchangeRevertErrors.SignatureError(
ExchangeRevertErrors.SignatureErrorCode.BadSignature,
ExchangeRevertErrors.SignatureErrorCode.BadOrderSignature,
exchange.getOrderHash(order),
order.makerAddress,
signature,
@@ -523,7 +523,7 @@ blockchainTests('Isolated fillOrder() tests', env => {
const goodSignature = createGoodSignature(SignatureType.Wallet);
const badSignature = createBadSignature(SignatureType.Wallet);
const expectedError = new ExchangeRevertErrors.SignatureError(
ExchangeRevertErrors.SignatureErrorCode.BadSignature,
ExchangeRevertErrors.SignatureErrorCode.BadOrderSignature,
exchange.getOrderHash(order),
order.makerAddress,
badSignature,

View File

@@ -143,13 +143,6 @@ blockchainTests.resets('LibExchangeRichErrorDecoder', ({ provider, txDefaults })
createDecodeTest(ExchangeRevertErrors.TransactionError, [errorCode, transactionHash]);
})();
(() => {
const transactionHash = orderUtils.generatePseudoRandomOrderHash();
const signer = addressUtils.generatePseudoRandomAddress();
const signature = hexRandom(SIGNATURE_LENGTH);
createDecodeTest(ExchangeRevertErrors.TransactionSignatureError, [transactionHash, signer, signature]);
})();
(() => {
const transactionHash = orderUtils.generatePseudoRandomOrderHash();
const errorData = hexRandom(ERROR_DATA_LENGTH);

View File

@@ -1300,7 +1300,7 @@ describe('matchOrders', () => {
};
const orderHashHex = orderHashUtils.getOrderHashHex(reconstructedOrderRight);
const expectedError = new ExchangeRevertErrors.SignatureError(
ExchangeRevertErrors.SignatureErrorCode.BadSignature,
ExchangeRevertErrors.SignatureErrorCode.BadOrderSignature,
orderHashHex,
signedOrderRight.makerAddress,
signedOrderRight.signature,
@@ -1327,7 +1327,7 @@ describe('matchOrders', () => {
};
const orderHashHex = orderHashUtils.getOrderHashHex(reconstructedOrderRight);
const expectedError = new ExchangeRevertErrors.SignatureError(
ExchangeRevertErrors.SignatureErrorCode.BadSignature,
ExchangeRevertErrors.SignatureErrorCode.BadOrderSignature,
orderHashHex,
signedOrderRight.makerAddress,
signedOrderRight.signature,
@@ -2381,7 +2381,7 @@ describe('matchOrders', () => {
};
const orderHashHex = orderHashUtils.getOrderHashHex(reconstructedOrderRight);
const expectedError = new ExchangeRevertErrors.SignatureError(
ExchangeRevertErrors.SignatureErrorCode.BadSignature,
ExchangeRevertErrors.SignatureErrorCode.BadOrderSignature,
orderHashHex,
signedOrderRight.makerAddress,
signedOrderRight.signature,
@@ -2408,7 +2408,7 @@ describe('matchOrders', () => {
};
const orderHashHex = orderHashUtils.getOrderHashHex(reconstructedOrderRight);
const expectedError = new ExchangeRevertErrors.SignatureError(
ExchangeRevertErrors.SignatureErrorCode.BadSignature,
ExchangeRevertErrors.SignatureErrorCode.BadOrderSignature,
orderHashHex,
signedOrderRight.makerAddress,
signedOrderRight.signature,

View File

@@ -229,7 +229,8 @@ blockchainTests.resets('Exchange transactions', env => {
const invalidSigHex = `0x${invalidSigBuff.toString('hex')}`;
transaction.signature = invalidSigHex;
const transactionHashHex = transactionHashUtils.getTransactionHashHex(transaction);
const expectedError = new ExchangeRevertErrors.TransactionSignatureError(
const expectedError = new ExchangeRevertErrors.SignatureError(
ExchangeRevertErrors.SignatureErrorCode.BadTransactionSignature,
transactionHashHex,
transaction.signerAddress,
transaction.signature,

View File

@@ -466,7 +466,8 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef
it('should revert if the signer != msg.sender and the signature is not valid', async () => {
const transaction = await generateZeroExTransactionAsync({ signerAddress: accounts[1] });
const transactionHash = transactionHashUtils.getTransactionHashHex(transaction);
const expectedError = new ExchangeRevertErrors.TransactionSignatureError(
const expectedError = new ExchangeRevertErrors.SignatureError(
ExchangeRevertErrors.SignatureErrorCode.BadTransactionSignature,
transactionHash,
accounts[1],
INVALID_SIGNATURE,
@@ -655,7 +656,8 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef
it('should revert if signer != msg.sender and the signature is invalid', async () => {
const transaction = await generateZeroExTransactionAsync({ signerAddress: accounts[0] });
const transactionHash = transactionHashUtils.getTransactionHashHex(transaction);
const expectedError = new ExchangeRevertErrors.TransactionSignatureError(
const expectedError = new ExchangeRevertErrors.SignatureError(
ExchangeRevertErrors.SignatureErrorCode.BadTransactionSignature,
transactionHash,
accounts[0],
INVALID_SIGNATURE,

View File

@@ -9,6 +9,7 @@ contract TestOwnable is
function externalOnlyOwner()
external
onlyOwner
view
returns (bool)
{
return true;

View File

@@ -25,7 +25,8 @@ export enum FillErrorCode {
}
export enum SignatureErrorCode {
BadSignature,
BadOrderSignature,
BadTransactionSignature,
InvalidLength,
Unsupported,
Illegal,
@@ -179,20 +180,6 @@ export class TransactionError extends RevertError {
}
}
export class TransactionSignatureError extends RevertError {
constructor(transactionHash?: string, signer?: string, signature?: string) {
super(
'TransactionSignatureError',
'TransactionSignatureError(bytes32 transactionHash, address signer, bytes signature)',
{
transactionHash,
signer,
signature,
},
);
}
}
export class TransactionExecutionError extends RevertError {
constructor(transactionHash?: string, errorData?: string) {
super('TransactionExecutionError', 'TransactionExecutionError(bytes32 transactionHash, bytes errorData)', {
@@ -292,7 +279,6 @@ const types = [
TransactionExecutionError,
TransactionGasPriceError,
TransactionInvalidContextError,
TransactionSignatureError,
];
// Register the types we've defined.