diff --git a/contracts/exchange/CHANGELOG.json b/contracts/exchange/CHANGELOG.json index 2582ec0ebb..deba02193e 100644 --- a/contracts/exchange/CHANGELOG.json +++ b/contracts/exchange/CHANGELOG.json @@ -17,6 +17,10 @@ { "note": "Refactor example contracts that use `executeTransaction`", "pr": 1753 + }, + { + "note": "Upgrade all string reverts to rich reverts", + "pr": 1761 } ] }, diff --git a/contracts/exchange/compiler.json b/contracts/exchange/compiler.json index c1f8663d37..32c356b673 100644 --- a/contracts/exchange/compiler.json +++ b/contracts/exchange/compiler.json @@ -40,6 +40,7 @@ "test/ReentrantERC20Token.sol", "test/TestAssetProxyDispatcher.sol", "test/TestExchangeInternals.sol", + "test/TestRevertReceiver.sol", "test/TestSignatureValidator.sol", "test/TestStaticCallReceiver.sol" ] diff --git a/contracts/exchange/contracts/src/Exchange.sol b/contracts/exchange/contracts/src/Exchange.sol index 34a2b30bf0..67f501f6b1 100644 --- a/contracts/exchange/contracts/src/Exchange.sol +++ b/contracts/exchange/contracts/src/Exchange.sol @@ -26,6 +26,7 @@ import "./MixinWrapperFunctions.sol"; import "./MixinAssetProxyDispatcher.sol"; import "./MixinTransactions.sol"; import "./MixinMatchOrders.sol"; +import "./MixinExchangeRichErrors.sol"; // solhint-disable no-empty-blocks @@ -35,7 +36,8 @@ contract Exchange is MixinSignatureValidator, MixinTransactions, MixinAssetProxyDispatcher, - MixinWrapperFunctions + MixinWrapperFunctions, + MixinExchangeRichErrors { string constant public VERSION = "3.0.0"; diff --git a/contracts/exchange/contracts/src/MixinAssetProxyDispatcher.sol b/contracts/exchange/contracts/src/MixinAssetProxyDispatcher.sol index e9e6805737..ce1eb119cc 100644 --- a/contracts/exchange/contracts/src/MixinAssetProxyDispatcher.sol +++ b/contracts/exchange/contracts/src/MixinAssetProxyDispatcher.sol @@ -20,12 +20,14 @@ pragma solidity ^0.5.5; import "@0x/contracts-utils/contracts/src/Ownable.sol"; import "./mixins/MAssetProxyDispatcher.sol"; +import "./mixins/MExchangeRichErrors.sol"; import "./interfaces/IAssetProxy.sol"; contract MixinAssetProxyDispatcher is Ownable, - MAssetProxyDispatcher + MAssetProxyDispatcher, + MExchangeRichErrors { // Mapping from Asset Proxy Id's to their respective Asset Proxy mapping (bytes4 => address) public assetProxies; @@ -40,10 +42,9 @@ contract MixinAssetProxyDispatcher is // Ensure that no asset proxy exists with current id. bytes4 assetProxyId = IAssetProxy(assetProxy).getProxyId(); address currentAssetProxy = assetProxies[assetProxyId]; - require( - currentAssetProxy == address(0), - "ASSET_PROXY_ALREADY_EXISTS" - ); + if (currentAssetProxy != address(0)) { + rrevert(AssetProxyExistsError(currentAssetProxy)); + } // Add asset proxy and log registration. assetProxies[assetProxyId] = assetProxy; @@ -65,11 +66,13 @@ contract MixinAssetProxyDispatcher is } /// @dev Forwards arguments to assetProxy and calls `transferFrom`. Either succeeds or throws. + /// @param orderHash Hash of the order associated with this transfer. /// @param assetData Byte array encoded for the asset. /// @param from Address to transfer token from. /// @param to Address to transfer token to. /// @param amount Amount of token to transfer. function dispatchTransferFrom( + bytes32 orderHash, bytes memory assetData, address from, address to, @@ -80,11 +83,14 @@ contract MixinAssetProxyDispatcher is // Do nothing if no amount should be transferred. if (amount > 0 && from != to) { // Ensure assetData length is valid - require( - assetData.length > 3, - "LENGTH_GREATER_THAN_3_REQUIRED" - ); - + if (assetData.length <= 3) { + rrevert(AssetProxyDispatchError( + AssetProxyDispatchErrorCodes.INVALID_ASSET_DATA_LENGTH, + orderHash, + assetData + )); + } + // Lookup assetProxy. We do not use `LibBytes.readBytes4` for gas efficiency reasons. bytes4 assetProxyId; assembly { @@ -96,14 +102,22 @@ contract MixinAssetProxyDispatcher is address assetProxy = assetProxies[assetProxyId]; // Ensure that assetProxy exists - require( - assetProxy != address(0), - "ASSET_PROXY_DOES_NOT_EXIST" - ); - + if (assetProxy == address(0)) { + rrevert(AssetProxyDispatchError( + AssetProxyDispatchErrorCodes.UNKNOWN_ASSET_PROXY, + orderHash, + assetData + )); + } + + // Whether the AssetProxy transfer succeeded. + bool didSucceed; + // On failure, the revert data thrown by the asset proxy. + bytes memory revertData; + // We construct calldata for the `assetProxy.transferFrom` ABI. // The layout of this calldata is in the table below. - // + // // | Area | Offset | Length | Contents | // | -------- |--------|---------|-------------------------------------------- | // | Header | 0 | 4 | function selector | @@ -127,12 +141,11 @@ contract MixinAssetProxyDispatcher is // `cdEnd` is the end of the calldata for `assetProxy.transferFrom`. let cdEnd := add(cdStart, add(132, dataAreaLength)) - /////// Setup Header Area /////// // This area holds the 4-byte `transferFromSelector`. // bytes4(keccak256("transferFrom(bytes,address,address,uint256)")) = 0xa85e59e4 mstore(cdStart, 0xa85e59e400000000000000000000000000000000000000000000000000000000) - + /////// Setup Params Area /////// // Each parameter is padded to 32-bytes. The entire Params Area is 128 bytes. // Notes: @@ -142,31 +155,51 @@ contract MixinAssetProxyDispatcher is mstore(add(cdStart, 36), and(from, 0xffffffffffffffffffffffffffffffffffffffff)) mstore(add(cdStart, 68), and(to, 0xffffffffffffffffffffffffffffffffffffffff)) mstore(add(cdStart, 100), amount) - + /////// Setup Data Area /////// // This area holds `assetData`. let dataArea := add(cdStart, 132) + let assetDataArea := assetData // solhint-disable-next-line no-empty-blocks for {} lt(dataArea, cdEnd) {} { - mstore(dataArea, mload(assetData)) + mstore(dataArea, mload(assetDataArea)) dataArea := add(dataArea, 32) - assetData := add(assetData, 32) + assetDataArea := add(assetDataArea, 32) } /////// Call `assetProxy.transferFrom` using the constructed calldata /////// - let success := call( + didSucceed := call( gas, // forward all gas assetProxy, // call address of asset proxy 0, // don't send any ETH cdStart, // pointer to start of input - sub(cdEnd, cdStart), // length of input - cdStart, // write output over input - 512 // reserve 512 bytes for output + sub(cdEnd, cdStart), // length of input + 0, // don't store the returndata + 0 // don't store the returndata ) - if iszero(success) { - revert(cdStart, returndatasize()) + + if iszero(didSucceed) { // Call reverted. + let revertDataSize := returndatasize() + // Construct a `bytes memory` type to hold the revert data + // at `cdStart`. + // The first 32 bytes are the length of the data. + mstore(cdStart, revertDataSize) + // Copy the revert data immediately after the length. + returndatacopy(add(cdStart, 32), 0, revertDataSize) + // We need to move the free memory pointer because we + // still have solidity logic that executes after this assembly. + mstore(64, add(cdStart, add(revertDataSize, 32))) + revertData := cdStart } } + + if (!didSucceed) { + rrevert(AssetProxyTransferError( + orderHash, + assetData, + revertData + )); + } } } } diff --git a/contracts/exchange/contracts/src/MixinExchangeCore.sol b/contracts/exchange/contracts/src/MixinExchangeCore.sol index b95d547839..9a1117dae5 100644 --- a/contracts/exchange/contracts/src/MixinExchangeCore.sol +++ b/contracts/exchange/contracts/src/MixinExchangeCore.sol @@ -28,6 +28,7 @@ import "./mixins/MExchangeCore.sol"; import "./mixins/MSignatureValidator.sol"; import "./mixins/MTransactions.sol"; import "./mixins/MAssetProxyDispatcher.sol"; +import "./mixins/MExchangeRichErrors.sol"; contract MixinExchangeCore is @@ -39,7 +40,8 @@ contract MixinExchangeCore is MAssetProxyDispatcher, MExchangeCore, MSignatureValidator, - MTransactions + MTransactions, + MExchangeRichErrors { // Mapping of orderHash => amount of takerAsset already bought by maker mapping (bytes32 => uint256) public filled; @@ -64,14 +66,13 @@ contract MixinExchangeCore is address senderAddress = makerAddress == msg.sender ? address(0) : msg.sender; // orderEpoch is initialized to 0, so to cancelUpTo we need salt + 1 - uint256 newOrderEpoch = targetOrderEpoch + 1; + uint256 newOrderEpoch = targetOrderEpoch + 1; uint256 oldOrderEpoch = orderEpoch[makerAddress][senderAddress]; // Ensure orderEpoch is monotonically increasing - require( - newOrderEpoch > oldOrderEpoch, - "INVALID_NEW_ORDER_EPOCH" - ); + if (newOrderEpoch <= oldOrderEpoch) { + rrevert(OrderEpochError(makerAddress, senderAddress, oldOrderEpoch)); + } // Update orderEpoch orderEpoch[makerAddress][senderAddress] = newOrderEpoch; @@ -193,7 +194,7 @@ contract MixinExchangeCore is // Fetch taker address address takerAddress = getCurrentContextAddress(); - + // Assert that the order is fillable by taker assertFillableOrder( order, @@ -201,7 +202,7 @@ contract MixinExchangeCore is takerAddress, signature ); - + // Get amount of takerAsset to fill uint256 remainingTakerAssetAmount = safeSub(order.takerAssetAmount, orderInfo.orderTakerAssetFilledAmount); uint256 takerAssetFilledAmount = min256(takerAssetFillAmount, remainingTakerAssetAmount); @@ -218,17 +219,20 @@ contract MixinExchangeCore is // Compute proportional fill amounts fillResults = calculateFillResults(order, takerAssetFilledAmount); + bytes32 orderHash = orderInfo.orderHash; + // Update exchange internal state updateFilledState( order, takerAddress, - orderInfo.orderHash, + orderHash, orderInfo.orderTakerAssetFilledAmount, fillResults ); - + // Settle order settleOrder( + orderHash, order, takerAddress, fillResults @@ -309,7 +313,7 @@ contract MixinExchangeCore is order.takerAssetData ); } - + /// @dev Validates context for fillOrder. Succeeds or throws. /// @param order to be filled. /// @param orderInfo OrderStatus, orderHash, and amount already filled of order. @@ -325,40 +329,44 @@ contract MixinExchangeCore is view { // An order can only be filled if its status is FILLABLE. - require( - orderInfo.orderStatus == uint8(OrderStatus.FILLABLE), - "ORDER_UNFILLABLE" - ); - + if (orderInfo.orderStatus != uint8(OrderStatus.FILLABLE)) { + rrevert(OrderStatusError( + OrderStatus(orderInfo.orderStatus), + orderInfo.orderHash + )); + } + // Validate sender is allowed to fill this order if (order.senderAddress != address(0)) { - require( - order.senderAddress == msg.sender, - "INVALID_SENDER" - ); + if (order.senderAddress != msg.sender) { + rrevert(InvalidSenderError(orderInfo.orderHash, msg.sender)); + } } - + // Validate taker is allowed to fill this order if (order.takerAddress != address(0)) { - require( - order.takerAddress == takerAddress, - "INVALID_TAKER" - ); + if (order.takerAddress != takerAddress) { + rrevert(InvalidTakerError(orderInfo.orderHash, takerAddress)); + } } - + // Validate Maker signature (check only if first time seen) if (orderInfo.orderTakerAssetFilledAmount == 0) { - require( - isValidSignature( + address makerAddress = order.makerAddress; + if (!isValidSignature( orderInfo.orderHash, - order.makerAddress, + makerAddress, + signature)) { + rrevert(SignatureError( + SignatureErrorCodes.BAD_SIGNATURE, + orderInfo.orderHash, + makerAddress, signature - ), - "INVALID_ORDER_SIGNATURE" - ); + )); + } } } - + /// @dev Validates context for fillOrder. Succeeds or throws. /// @param order to be filled. /// @param orderInfo OrderStatus, orderHash, and amount already filled of order. @@ -377,27 +385,25 @@ contract MixinExchangeCore is { // Revert if fill amount is invalid // TODO: reconsider necessity for v2.1 - require( - takerAssetFillAmount != 0, - "INVALID_TAKER_AMOUNT" - ); - + if (takerAssetFillAmount == 0) { + rrevert(FillError(FillErrorCodes.INVALID_TAKER_AMOUNT, orderInfo.orderHash)); + } + // Make sure taker does not pay more than desired amount // NOTE: This assertion should never fail, it is here // as an extra defence against potential bugs. - require( - takerAssetFilledAmount <= takerAssetFillAmount, - "TAKER_OVERPAY" - ); - + if (takerAssetFilledAmount > takerAssetFillAmount) { + rrevert(FillError(FillErrorCodes.TAKER_OVERPAY, orderInfo.orderHash)); + } + // Make sure order is not overfilled // NOTE: This assertion should never fail, it is here // as an extra defence against potential bugs. - require( - safeAdd(orderInfo.orderTakerAssetFilledAmount, takerAssetFilledAmount) <= order.takerAssetAmount, - "ORDER_OVERFILL" - ); - + if (safeAdd(orderInfo.orderTakerAssetFilledAmount, takerAssetFilledAmount) + > order.takerAssetAmount) { + rrevert(FillError(FillErrorCodes.OVERFILL, orderInfo.orderHash)); + } + // Make sure order is filled at acceptable price. // The order has an implied price from the makers perspective: // order price = order.makerAssetAmount / order.takerAssetAmount @@ -415,12 +421,10 @@ contract MixinExchangeCore is // order.makerAssetAmount * takerAssetFilledAmount // NOTE: This assertion should never fail, it is here // as an extra defence against potential bugs. - require( - safeMul(makerAssetFilledAmount, order.takerAssetAmount) - <= - safeMul(order.makerAssetAmount, takerAssetFilledAmount), - "INVALID_FILL_PRICE" - ); + if (safeMul(makerAssetFilledAmount, order.takerAssetAmount) + > safeMul(order.makerAssetAmount, takerAssetFilledAmount)) { + rrevert(FillError(FillErrorCodes.INVALID_FILL_PRICE, orderInfo.orderHash)); + } } /// @dev Validates context for cancelOrder. Succeeds or throws. @@ -435,25 +439,22 @@ contract MixinExchangeCore is { // Ensure order is valid // An order can only be cancelled if its status is FILLABLE. - require( - orderInfo.orderStatus == uint8(OrderStatus.FILLABLE), - "ORDER_UNFILLABLE" - ); + if (orderInfo.orderStatus != uint8(OrderStatus.FILLABLE)) { + rrevert(OrderStatusError(OrderStatus(orderInfo.orderStatus), orderInfo.orderHash)); + } // Validate sender is allowed to cancel this order if (order.senderAddress != address(0)) { - require( - order.senderAddress == msg.sender, - "INVALID_SENDER" - ); + if (order.senderAddress != msg.sender) { + rrevert(InvalidSenderError(orderInfo.orderHash, msg.sender)); + } } // Validate transaction signed by maker address makerAddress = getCurrentContextAddress(); - require( - order.makerAddress == makerAddress, - "INVALID_MAKER" - ); + if (order.makerAddress != makerAddress) { + rrevert(InvalidMakerError(orderInfo.orderHash, makerAddress)); + } } /// @dev Calculates amounts filled and fees paid by maker and taker. @@ -490,10 +491,12 @@ contract MixinExchangeCore is } /// @dev Settles an order by transferring assets between counterparties. + /// @param orderHash The order hash. /// @param order Order struct containing order specifications. /// @param takerAddress Address selling takerAsset and buying makerAsset. /// @param fillResults Amounts to be filled and fees paid by maker and taker. function settleOrder( + bytes32 orderHash, LibOrder.Order memory order, address takerAddress, LibFillResults.FillResults memory fillResults @@ -502,24 +505,28 @@ contract MixinExchangeCore is { bytes memory zrxAssetData = ZRX_ASSET_DATA; dispatchTransferFrom( + orderHash, order.makerAssetData, order.makerAddress, takerAddress, fillResults.makerAssetFilledAmount ); dispatchTransferFrom( + orderHash, order.takerAssetData, takerAddress, order.makerAddress, fillResults.takerAssetFilledAmount ); dispatchTransferFrom( + orderHash, zrxAssetData, order.makerAddress, order.feeRecipientAddress, fillResults.makerFeePaid ); dispatchTransferFrom( + orderHash, zrxAssetData, takerAddress, order.feeRecipientAddress, diff --git a/contracts/exchange/contracts/src/MixinExchangeRichErrors.sol b/contracts/exchange/contracts/src/MixinExchangeRichErrors.sol new file mode 100644 index 0000000000..a0693c8979 --- /dev/null +++ b/contracts/exchange/contracts/src/MixinExchangeRichErrors.sol @@ -0,0 +1,301 @@ +/* + + Copyright 2018 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.5.5; + +import "@0x/contracts-utils/contracts/src/RichErrors.sol"; +import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol"; +import "./mixins/MExchangeRichErrors.sol"; + + +contract MixinExchangeRichErrors is + RichErrors, + MExchangeRichErrors +{ + // solhint-disable func-name-mixedcase + function SignatureError( + SignatureErrorCodes error, + bytes32 orderHash, + address signer, + bytes memory signature + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + SIGNATURE_ERROR_SELECTOR, + error, + orderHash, + signer, + signature + ); + } + + function SignatureValidatorError( + bytes32 orderHash, + address signer, + bytes memory signature, + bytes memory errorData + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + SIGNATURE_VALIDATOR_ERROR_SELECTOR, + orderHash, + signer, + signature, + errorData + ); + } + + function SignatureWalletError( + bytes32 orderHash, + address signer, + bytes memory signature, + bytes memory errorData + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + SIGNATURE_WALLET_ERROR_SELECTOR, + orderHash, + signer, + signature, + errorData + ); + } + + function OrderStatusError( + LibOrder.OrderStatus orderStatus, + bytes32 orderHash + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + ORDER_STATUS_ERROR_SELECTOR, + orderStatus, + orderHash + ); + } + + function InvalidSenderError( + bytes32 orderHash, + address sender + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + INVALID_SENDER_ERROR_SELECTOR, + orderHash, + sender + ); + } + + function InvalidMakerError( + bytes32 orderHash, + address maker + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + INVALID_MAKER_ERROR_SELECTOR, + orderHash, + maker + ); + } + + function FillError( + FillErrorCodes error, + bytes32 orderHash + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + FILL_ERROR_SELECTOR, + error, + orderHash + ); + } + + function InvalidTakerError( + bytes32 orderHash, + address taker + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + INVALID_TAKER_ERROR_SELECTOR, + orderHash, + taker + ); + } + + function OrderEpochError( + address maker, + address sender, + uint256 currentEpoch + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + ORDER_EPOCH_ERROR_SELECTOR, + maker, + sender, + currentEpoch + ); + } + + function AssetProxyExistsError( + address proxyAddress + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + ASSET_PROXY_EXISTS_ERROR_SELECTOR, + proxyAddress + ); + } + + function AssetProxyDispatchError( + AssetProxyDispatchErrorCodes error, + bytes32 orderHash, + bytes memory assetData + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + ASSET_PROXY_DISPATCH_ERROR_SELECTOR, + error, + orderHash, + assetData + ); + } + + function AssetProxyTransferError( + bytes32 orderHash, + bytes memory assetData, + bytes memory errorData + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + ASSET_PROXY_TRANSFER_ERROR_SELECTOR, + orderHash, + assetData, + errorData + ); + } + + function NegativeSpreadError( + bytes32 leftOrderHash, + bytes32 rightOrderHash + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + NEGATIVE_SPREAD_ERROR_SELECTOR, + leftOrderHash, + rightOrderHash + ); + } + + function TransactionError( + TransactionErrorCodes error, + bytes32 transactionHash + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + TRANSACTION_ERROR_SELECTOR, + error, + transactionHash + ); + } + + function TransactionSignatureError( + bytes32 transactionHash, + address signer, + bytes memory signature + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + TRANSACTION_SIGNATURE_ERROR_SELECTOR, + transactionHash, + signer, + signature + ); + } + + function TransactionExecutionError( + bytes32 transactionHash, + bytes memory errorData + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + TRANSACTION_EXECUTION_ERROR_SELECTOR, + transactionHash, + errorData + ); + } + + function IncompleteFillError( + bytes32 orderHash + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + INCOMPLETE_FILL_ERROR_SELECTOR, + orderHash + ); + } +} diff --git a/contracts/exchange/contracts/src/MixinMatchOrders.sol b/contracts/exchange/contracts/src/MixinMatchOrders.sol index c389399141..8087025b62 100644 --- a/contracts/exchange/contracts/src/MixinMatchOrders.sol +++ b/contracts/exchange/contracts/src/MixinMatchOrders.sol @@ -23,16 +23,19 @@ import "./mixins/MExchangeCore.sol"; import "./mixins/MMatchOrders.sol"; import "./mixins/MTransactions.sol"; import "./mixins/MAssetProxyDispatcher.sol"; +import "./mixins/MExchangeRichErrors.sol"; contract MixinMatchOrders is ReentrancyGuard, LibConstants, LibMath, + LibOrder, MAssetProxyDispatcher, MExchangeCore, MMatchOrders, - MTransactions + MTransactions, + MExchangeRichErrors { /// @dev Match two complementary orders that have a profitable spread. /// Each order is filled at their respective price point. However, the calculations are @@ -64,7 +67,7 @@ contract MixinMatchOrders is // Fetch taker address address takerAddress = getCurrentContextAddress(); - + // Either our context is valid or we revert assertFillableOrder( leftOrder, @@ -103,7 +106,7 @@ contract MixinMatchOrders is matchedFillResults.right.takerAssetFilledAmount, matchedFillResults.right.makerAssetFilledAmount ); - + // Update exchange state updateFilledState( leftOrder, @@ -122,6 +125,8 @@ contract MixinMatchOrders is // Settle matched orders. Succeeds or throws. settleMatchedOrders( + leftOrderInfo.orderHash, + rightOrderInfo.orderHash, leftOrder, rightOrder, takerAddress, @@ -139,7 +144,7 @@ contract MixinMatchOrders is LibOrder.Order memory rightOrder ) internal - pure + view { // 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 @@ -149,11 +154,13 @@ contract MixinMatchOrders is // AND // / >= / // These equations can be combined to get the following: - require( - safeMul(leftOrder.makerAssetAmount, rightOrder.makerAssetAmount) >= - safeMul(leftOrder.takerAssetAmount, rightOrder.takerAssetAmount), - "NEGATIVE_SPREAD_REQUIRED" - ); + if (safeMul(leftOrder.makerAssetAmount, rightOrder.makerAssetAmount) < + safeMul(leftOrder.takerAssetAmount, rightOrder.takerAssetAmount)) { + rrevert(NegativeSpreadError( + getOrderHash(leftOrder), + getOrderHash(rightOrder) + )); + } } /// @dev Calculates fill amounts for the matched orders. @@ -203,7 +210,7 @@ contract MixinMatchOrders is matchedFillResults.right.makerAssetFilledAmount = rightMakerAssetAmountRemaining; matchedFillResults.right.takerAssetFilledAmount = rightTakerAssetAmountRemaining; matchedFillResults.left.takerAssetFilledAmount = matchedFillResults.right.makerAssetFilledAmount; - // Round down to ensure the maker's exchange rate does not exceed the price specified by the order. + // Round down to ensure the maker's exchange rate does not exceed the price specified by the order. // We favor the maker when the exchange rate must be rounded. matchedFillResults.left.makerAssetFilledAmount = safeGetPartialAmountFloor( leftOrder.makerAssetAmount, @@ -259,11 +266,15 @@ contract MixinMatchOrders is } /// @dev Settles matched order by transferring appropriate funds between order makers, taker, and fee recipient. + /// @param leftOrderHash First matched order hash. + /// @param rightOrderHash Second matched order hash. /// @param leftOrder First matched order. /// @param rightOrder Second matched order. /// @param takerAddress Address that matched the orders. The taker receives the spread between orders as profit. /// @param matchedFillResults Struct holding amounts to transfer between makers, taker, and fee recipients. function settleMatchedOrders( + bytes32 leftOrderHash, + bytes32 rightOrderHash, LibOrder.Order memory leftOrder, LibOrder.Order memory rightOrder, address takerAddress, @@ -274,18 +285,21 @@ contract MixinMatchOrders is bytes memory zrxAssetData = ZRX_ASSET_DATA; // Order makers and taker dispatchTransferFrom( + leftOrderHash, leftOrder.makerAssetData, leftOrder.makerAddress, rightOrder.makerAddress, matchedFillResults.right.takerAssetFilledAmount ); dispatchTransferFrom( + rightOrderHash, rightOrder.makerAssetData, rightOrder.makerAddress, leftOrder.makerAddress, matchedFillResults.left.takerAssetFilledAmount ); dispatchTransferFrom( + leftOrderHash, leftOrder.makerAssetData, leftOrder.makerAddress, takerAddress, @@ -294,12 +308,14 @@ contract MixinMatchOrders is // Maker fees dispatchTransferFrom( + leftOrderHash, zrxAssetData, leftOrder.makerAddress, leftOrder.feeRecipientAddress, matchedFillResults.left.makerFeePaid ); dispatchTransferFrom( + rightOrderHash, zrxAssetData, rightOrder.makerAddress, rightOrder.feeRecipientAddress, @@ -309,6 +325,7 @@ contract MixinMatchOrders is // Taker fees if (leftOrder.feeRecipientAddress == rightOrder.feeRecipientAddress) { dispatchTransferFrom( + leftOrderHash, zrxAssetData, takerAddress, leftOrder.feeRecipientAddress, @@ -319,12 +336,14 @@ contract MixinMatchOrders is ); } else { dispatchTransferFrom( + leftOrderHash, zrxAssetData, takerAddress, leftOrder.feeRecipientAddress, matchedFillResults.left.takerFeePaid ); dispatchTransferFrom( + rightOrderHash, zrxAssetData, takerAddress, rightOrder.feeRecipientAddress, diff --git a/contracts/exchange/contracts/src/MixinSignatureValidator.sol b/contracts/exchange/contracts/src/MixinSignatureValidator.sol index 66bbdd43fb..a34ad2ed08 100644 --- a/contracts/exchange/contracts/src/MixinSignatureValidator.sol +++ b/contracts/exchange/contracts/src/MixinSignatureValidator.sol @@ -23,6 +23,7 @@ import "@0x/contracts-utils/contracts/src/LibBytes.sol"; import "@0x/contracts-utils/contracts/src/ReentrancyGuard.sol"; import "./mixins/MSignatureValidator.sol"; import "./mixins/MTransactions.sol"; +import "./mixins/MExchangeRichErrors.sol"; import "./interfaces/IWallet.sol"; import "./interfaces/IValidator.sol"; @@ -30,10 +31,11 @@ import "./interfaces/IValidator.sol"; contract MixinSignatureValidator is ReentrancyGuard, MSignatureValidator, - MTransactions + MTransactions, + MExchangeRichErrors { using LibBytes for bytes; - + // Mapping of hash => signer => signed mapping (bytes32 => mapping (address => bool)) public preSigned; @@ -52,14 +54,17 @@ contract MixinSignatureValidator is external { if (signerAddress != msg.sender) { - require( - isValidSignature( + if (!isValidSignature( + hash, + signerAddress, + signature)) { + rrevert(SignatureError( + SignatureErrorCodes.BAD_SIGNATURE, hash, signerAddress, signature - ), - "INVALID_SIGNATURE" - ); + )); + } } preSigned[hash][signerAddress] = true; } @@ -97,19 +102,27 @@ contract MixinSignatureValidator is view returns (bool isValid) { - require( - signature.length > 0, - "LENGTH_GREATER_THAN_0_REQUIRED" - ); + if (signature.length == 0) { + rrevert(SignatureError( + SignatureErrorCodes.INVALID_LENGTH, + hash, + signerAddress, + signature + )); + } - // Pop last byte off of signature byte array. - uint8 signatureTypeRaw = uint8(signature.popLastByte()); + // Read the last byte off of signature byte array. + uint8 signatureTypeRaw = uint8(signature[signature.length - 1]); // Ensure signature is supported - require( - signatureTypeRaw < uint8(SignatureType.NSignatureTypes), - "SIGNATURE_UNSUPPORTED" - ); + if (signatureTypeRaw >= uint8(SignatureType.NSignatureTypes)) { + rrevert(SignatureError( + SignatureErrorCodes.UNSUPPORTED, + hash, + signerAddress, + signature + )); + } SignatureType signatureType = SignatureType(signatureTypeRaw); @@ -125,26 +138,39 @@ contract MixinSignatureValidator is // it an explicit option. This aids testing and analysis. It is // also the initialization value for the enum type. if (signatureType == SignatureType.Illegal) { - revert("SIGNATURE_ILLEGAL"); + rrevert(SignatureError( + SignatureErrorCodes.ILLEGAL, + hash, + signerAddress, + signature + )); // Always invalid signature. // Like Illegal, this is always implicitly available and therefore // offered explicitly. It can be implicitly created by providing // a correctly formatted but incorrect signature. } else if (signatureType == SignatureType.Invalid) { - require( - signature.length == 0, - "LENGTH_0_REQUIRED" - ); + if (signature.length != 1) { + rrevert(SignatureError( + SignatureErrorCodes.INVALID_LENGTH, + hash, + signerAddress, + signature + )); + } isValid = false; return isValid; // Signature using EIP712 } else if (signatureType == SignatureType.EIP712) { - require( - signature.length == 65, - "LENGTH_65_REQUIRED" - ); + if (signature.length != 66) { + rrevert(SignatureError( + SignatureErrorCodes.INVALID_LENGTH, + hash, + signerAddress, + signature + )); + } v = uint8(signature[0]); r = signature.readBytes32(1); s = signature.readBytes32(33); @@ -159,10 +185,14 @@ contract MixinSignatureValidator is // Signed using web3.eth_sign } else if (signatureType == SignatureType.EthSign) { - require( - signature.length == 65, - "LENGTH_65_REQUIRED" - ); + if (signature.length != 66) { + rrevert(SignatureError( + SignatureErrorCodes.INVALID_LENGTH, + hash, + signerAddress, + signature + )); + } v = uint8(signature[0]); r = signature.readBytes32(1); s = signature.readBytes32(33); @@ -189,22 +219,8 @@ contract MixinSignatureValidator is return isValid; // Signature verified by validator contract. - // If used with an order, the maker of the order can still be an EOA. - // A signature using this type should be encoded as: - // | Offset | Length | Contents | - // | 0x00 | x | Signature to validate | - // | 0x00 + x | 20 | Address of validator contract | - // | 0x14 + x | 1 | Signature type is always "\x06" | } else if (signatureType == SignatureType.Validator) { - // Pop last 20 bytes off of signature byte array. - address validatorAddress = signature.popLast20Bytes(); - - // Ensure signer has approved validator. - if (!allowedValidators[signerAddress][validatorAddress]) { - return false; - } isValid = isValidValidatorSignature( - validatorAddress, hash, signerAddress, signature @@ -222,7 +238,12 @@ contract MixinSignatureValidator is // that we currently support. In this case returning false // may lead the caller to incorrectly believe that the // signature was invalid.) - revert("SIGNATURE_UNSUPPORTED"); + rrevert(SignatureError( + SignatureErrorCodes.UNSUPPORTED, + hash, + signerAddress, + signature + )); } /// @dev Verifies signature using logic defined by Wallet contract. @@ -236,90 +257,95 @@ contract MixinSignatureValidator is address walletAddress, bytes memory signature ) - internal + private view returns (bool isValid) { + uint256 signatureLength = signature.length; + // Shave the signature type off the signature. + assembly { + mstore(signature, sub(signatureLength, 1)) + } + // Encode the call data. bytes memory callData = abi.encodeWithSelector( IWallet(walletAddress).isValidSignature.selector, hash, signature ); + // Restore the full signature. assembly { - let cdStart := add(callData, 32) - let success := staticcall( - gas, // forward all gas - walletAddress, // address of Wallet contract - cdStart, // pointer to start of input - mload(callData), // length of input - cdStart, // write output over input - 32 // output size is 32 bytes - ) - - switch success - case 0 { - // Revert with `Error("WALLET_ERROR")` - mstore(0, 0x08c379a000000000000000000000000000000000000000000000000000000000) - mstore(32, 0x0000002000000000000000000000000000000000000000000000000000000000) - mstore(64, 0x0000000c57414c4c45545f4552524f5200000000000000000000000000000000) - mstore(96, 0) - revert(0, 100) - } - case 1 { - // Signature is valid if call did not revert and returned true - isValid := mload(cdStart) - } + mstore(signature, signatureLength) + } + // Static call the verification function. + (bool didSucceed, bytes memory returnData) = walletAddress.staticcall(callData); + // Return data should be a single bool. + if (didSucceed && returnData.length == 32) { + return returnData.readUint256(0) == 1; } - return isValid; + // Static call to verifier failed. + rrevert(SignatureWalletError( + hash, + walletAddress, + signature, + returnData + )); } /// @dev Verifies signature using logic defined by Validator contract. - /// @param validatorAddress Address of validator contract. /// @param hash Any 32 byte hash. /// @param signerAddress Address that should have signed the given hash. /// @param signature Proof that the hash has been signed by signer. /// @return True if the address recovered from the provided signature matches the input signer address. function isValidValidatorSignature( - address validatorAddress, bytes32 hash, address signerAddress, bytes memory signature ) - internal + private view returns (bool isValid) { + // If used with an order, the maker of the order can still be an EOA. + // A signature using this type should be encoded as: + // | Offset | Length | Contents | + // | 0x00 | x | Signature to validate | + // | 0x00 + x | 20 | Address of validator contract | + // | 0x14 + x | 1 | Signature type is always "\x06" | + + uint256 signatureLength = signature.length; + // Read the validator address from the signature. + address validatorAddress = signature.readAddress(signatureLength - 21); + // Ensure signer has approved validator. + if (!allowedValidators[signerAddress][validatorAddress]) { + return false; + } + // Shave the validator address and signature type from the signature. + assembly { + mstore(signature, sub(signatureLength, 21)) + } + // Encode the call data. bytes memory callData = abi.encodeWithSelector( - IValidator(signerAddress).isValidSignature.selector, + IValidator(validatorAddress).isValidSignature.selector, hash, signerAddress, signature ); + // Restore the full signature. assembly { - let cdStart := add(callData, 32) - let success := staticcall( - gas, // forward all gas - validatorAddress, // address of Validator contract - cdStart, // pointer to start of input - mload(callData), // length of input - cdStart, // write output over input - 32 // output size is 32 bytes - ) - - switch success - case 0 { - // Revert with `Error("VALIDATOR_ERROR")` - mstore(0, 0x08c379a000000000000000000000000000000000000000000000000000000000) - mstore(32, 0x0000002000000000000000000000000000000000000000000000000000000000) - mstore(64, 0x0000000f56414c494441544f525f4552524f5200000000000000000000000000) - mstore(96, 0) - revert(0, 100) - } - case 1 { - // Signature is valid if call did not revert and returned true - isValid := mload(cdStart) - } + mstore(signature, signatureLength) + } + // Static call the verification function. + (bool didSucceed, bytes memory returnData) = validatorAddress.staticcall(callData); + // Return data should be a single bool. + if (didSucceed && returnData.length == 32) { + return returnData.readUint256(0) == 1; } - return isValid; + // Static call to verifier failed. + rrevert(SignatureValidatorError( + hash, + signerAddress, + signature, + returnData + )); } } diff --git a/contracts/exchange/contracts/src/MixinTransactions.sol b/contracts/exchange/contracts/src/MixinTransactions.sol index 9995170332..460cde5563 100644 --- a/contracts/exchange/contracts/src/MixinTransactions.sol +++ b/contracts/exchange/contracts/src/MixinTransactions.sol @@ -22,12 +22,14 @@ pragma experimental ABIEncoderV2; import "@0x/contracts-exchange-libs/contracts/src/LibZeroExTransaction.sol"; import "./mixins/MSignatureValidator.sol"; import "./mixins/MTransactions.sol"; +import "./mixins/MExchangeRichErrors.sol"; contract MixinTransactions is LibZeroExTransaction, MSignatureValidator, - MTransactions + MTransactions, + MExchangeRichErrors { // Mapping of transaction hash => executed @@ -46,32 +48,38 @@ contract MixinTransactions is ) public { - // Prevent reentrancy - require( - currentContextAddress == address(0), - "REENTRANCY_ILLEGAL" - ); - bytes32 transactionHash = getTransactionHash(transaction); + // Prevent reentrancy + if (currentContextAddress != address(0)) { + rrevert(TransactionError( + TransactionErrorCodes.NO_REENTRANCY, + transactionHash + )); + } + // Validate transaction has not been executed - require( - !transactions[transactionHash], - "INVALID_TX_HASH" - ); + if (transactions[transactionHash]) { + rrevert(TransactionError( + TransactionErrorCodes.ALREADY_EXECUTED, + transactionHash + )); + } // Transaction always valid if signer is sender of transaction address signerAddress = transaction.signerAddress; if (signerAddress != msg.sender) { // Validate signature - require( - isValidSignature( + if (!isValidSignature( + transactionHash, + signerAddress, + signature)) { + rrevert(TransactionSignatureError( transactionHash, signerAddress, signature - ), - "INVALID_TX_SIGNATURE" - ); + )); + } // Set the current transaction signer currentContextAddress = signerAddress; @@ -79,11 +87,13 @@ contract MixinTransactions is // Execute transaction transactions[transactionHash] = true; - (bool success,) = address(this).delegatecall(transaction.data); - require( - success, - "FAILED_EXECUTION" - ); + (bool didSucceed, bytes memory callReturnData) = address(this).delegatecall(transaction.data); + if (!didSucceed) { + rrevert(TransactionExecutionError( + transactionHash, + callReturnData + )); + } // Reset current transaction signer if it was previously updated if (signerAddress != msg.sender) { diff --git a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol index 9c8cf3f2e6..da8be1c4bb 100644 --- a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol +++ b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol @@ -26,6 +26,7 @@ import "@0x/contracts-exchange-libs/contracts/src/LibFillResults.sol"; import "@0x/contracts-exchange-libs/contracts/src/LibAbiEncoder.sol"; import "./mixins/MExchangeCore.sol"; import "./mixins/MWrapperFunctions.sol"; +import "./mixins/MExchangeRichErrors.sol"; contract MixinWrapperFunctions is @@ -34,7 +35,8 @@ contract MixinWrapperFunctions is LibFillResults, LibAbiEncoder, MExchangeCore, - MWrapperFunctions + MWrapperFunctions, + MExchangeRichErrors { /// @dev Fills the input order. Reverts if exact takerAssetFillAmount not filled. /// @param order Order struct containing order specifications. @@ -195,7 +197,7 @@ contract MixinWrapperFunctions is returns (FillResults memory totalFillResults) { bytes memory takerAssetData = orders[0].takerAssetData; - + uint256 ordersLength = orders.length; for (uint256 i = 0; i != ordersLength; i++) { @@ -417,10 +419,9 @@ contract MixinWrapperFunctions is takerAssetFillAmount, signature ); - require( - fillResults.takerAssetFilledAmount == takerAssetFillAmount, - "COMPLETE_FILL_FAILED" - ); + if (fillResults.takerAssetFilledAmount != takerAssetFillAmount) { + rrevert(IncompleteFillError(getOrderInfo(order).orderHash)); + } return fillResults; } } diff --git a/contracts/exchange/contracts/src/mixins/MAssetProxyDispatcher.sol b/contracts/exchange/contracts/src/mixins/MAssetProxyDispatcher.sol index b53e7166e6..336c174602 100644 --- a/contracts/exchange/contracts/src/mixins/MAssetProxyDispatcher.sol +++ b/contracts/exchange/contracts/src/mixins/MAssetProxyDispatcher.sol @@ -31,11 +31,13 @@ contract MAssetProxyDispatcher is ); /// @dev Forwards arguments to assetProxy and calls `transferFrom`. Either succeeds or throws. + /// @param orderHash Hash of the order associated with this transfer. /// @param assetData Byte array encoded for the asset. /// @param from Address to transfer token from. /// @param to Address to transfer token to. /// @param amount Amount of token to transfer. function dispatchTransferFrom( + bytes32 orderHash, bytes memory assetData, address from, address to, diff --git a/contracts/exchange/contracts/src/mixins/MExchangeRichErrors.sol b/contracts/exchange/contracts/src/mixins/MExchangeRichErrors.sol new file mode 100644 index 0000000000..6b28cb6162 --- /dev/null +++ b/contracts/exchange/contracts/src/mixins/MExchangeRichErrors.sol @@ -0,0 +1,247 @@ +/* + + Copyright 2018 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.5.5; + +import "@0x/contracts-utils/contracts/src/mixins/MRichErrors.sol"; +import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol"; + + +contract MExchangeRichErrors is + MRichErrors +{ + enum FillErrorCodes { + INVALID_TAKER_AMOUNT, + TAKER_OVERPAY, + OVERFILL, + INVALID_FILL_PRICE + } + + enum SignatureErrorCodes { + BAD_SIGNATURE, + INVALID_LENGTH, + UNSUPPORTED, + ILLEGAL + } + + enum AssetProxyDispatchErrorCodes { + INVALID_ASSET_DATA_LENGTH, + UNKNOWN_ASSET_PROXY + } + + enum TransactionErrorCodes { + NO_REENTRANCY, + ALREADY_EXECUTED + } + + // solhint-disable func-name-mixedcase + bytes4 internal constant SIGNATURE_ERROR_SELECTOR = + bytes4(keccak256("SignatureError(uint8,bytes32,address,bytes)")); + + function SignatureError( + SignatureErrorCodes error, + bytes32 orderHash, + address signer, + bytes memory signature + ) + internal + pure + returns (bytes memory); + + bytes4 internal constant SIGNATURE_VALIDATOR_ERROR_SELECTOR = + bytes4(keccak256("SignatureValidatorError(bytes32,address,bytes,bytes)")); + + function SignatureValidatorError( + bytes32 orderHash, + address signer, + bytes memory signature, + bytes memory errorData + ) + internal + pure + returns (bytes memory); + + bytes4 internal constant SIGNATURE_WALLET_ERROR_SELECTOR = + bytes4(keccak256("SignatureWalletError(bytes32,address,bytes,bytes)")); + + function SignatureWalletError( + bytes32 orderHash, + address signer, + bytes memory signature, + bytes memory errorData + ) + internal + pure + returns (bytes memory); + + bytes4 internal constant ORDER_STATUS_ERROR_SELECTOR = + bytes4(keccak256("OrderStatusError(uint8,bytes32)")); + + function OrderStatusError( + LibOrder.OrderStatus orderStatus, + bytes32 orderHash + ) + internal + pure + returns (bytes memory); + + bytes4 internal constant INVALID_SENDER_ERROR_SELECTOR = + bytes4(keccak256("InvalidSenderError(bytes32,address)")); + + function InvalidSenderError( + bytes32 orderHash, + address sender + ) + internal + pure + returns (bytes memory); + + bytes4 internal constant INVALID_MAKER_ERROR_SELECTOR = + bytes4(keccak256("InvalidMakerError(bytes32,address)")); + + function InvalidMakerError( + bytes32 orderHash, + address maker + ) + internal + pure + returns (bytes memory); + + bytes4 internal constant FILL_ERROR_SELECTOR = + bytes4(keccak256("FillError(uint8,bytes32)")); + + function FillError( + FillErrorCodes error, + bytes32 orderHash + ) + internal + pure + returns (bytes memory); + + bytes4 internal constant INVALID_TAKER_ERROR_SELECTOR = + bytes4(keccak256("InvalidTakerError(bytes32,address)")); + + function InvalidTakerError( + bytes32 orderHash, + address taker + ) + internal + pure + returns (bytes memory); + + bytes4 internal constant ORDER_EPOCH_ERROR_SELECTOR = + bytes4(keccak256("OrderEpochError(address,address,uint256)")); + + function OrderEpochError( + address maker, + address sender, + uint256 currentEpoch + ) + internal + pure + returns (bytes memory); + + bytes4 internal constant ASSET_PROXY_EXISTS_ERROR_SELECTOR = + bytes4(keccak256("AssetProxyExistsError(address)")); + + function AssetProxyExistsError( + address proxyAddress + ) + internal + pure + returns (bytes memory); + + bytes4 internal constant ASSET_PROXY_DISPATCH_ERROR_SELECTOR = + bytes4(keccak256("AssetProxyDispatchError(uint8,bytes32,bytes)")); + + function AssetProxyDispatchError( + AssetProxyDispatchErrorCodes error, + bytes32 orderHash, + bytes memory assetData + ) + internal + pure + returns (bytes memory); + + bytes4 internal constant ASSET_PROXY_TRANSFER_ERROR_SELECTOR = + bytes4(keccak256("AssetProxyTransferError(bytes32,bytes,bytes)")); + + function AssetProxyTransferError( + bytes32 orderHash, + bytes memory assetData, + bytes memory errorData + ) + internal + pure + returns (bytes memory); + + bytes4 internal constant NEGATIVE_SPREAD_ERROR_SELECTOR = + bytes4(keccak256("NegativeSpreadError(bytes32,bytes32)")); + + function NegativeSpreadError( + bytes32 leftOrderHash, + bytes32 rightOrderHash + ) + internal + pure + returns (bytes memory); + + bytes4 internal constant TRANSACTION_ERROR_SELECTOR = + bytes4(keccak256("TransactionError(uint8,bytes32)")); + + function TransactionError( + TransactionErrorCodes error, + bytes32 transactionHash + ) + internal + pure + returns (bytes memory); + + bytes4 internal constant TRANSACTION_SIGNATURE_ERROR_SELECTOR = + bytes4(keccak256("TransactionSignatureError(bytes32,address,bytes)")); + + function TransactionSignatureError( + bytes32 transactionHash, + address signer, + bytes memory signature + ) + internal + pure + returns (bytes memory); + + bytes4 internal constant TRANSACTION_EXECUTION_ERROR_SELECTOR = + bytes4(keccak256("TransactionExecutionError(bytes32,bytes)")); + + function TransactionExecutionError( + bytes32 transactionHash, + bytes memory errorData + ) + internal + pure + returns (bytes memory); + + bytes4 internal constant INCOMPLETE_FILL_ERROR_SELECTOR = + bytes4(keccak256("IncompleteFillError(bytes32)")); + + function IncompleteFillError( + bytes32 orderHash + ) + internal + pure + returns (bytes memory); +} diff --git a/contracts/exchange/contracts/src/mixins/MMatchOrders.sol b/contracts/exchange/contracts/src/mixins/MMatchOrders.sol index bce93614a3..a6c1ba7c06 100644 --- a/contracts/exchange/contracts/src/mixins/MMatchOrders.sol +++ b/contracts/exchange/contracts/src/mixins/MMatchOrders.sol @@ -35,7 +35,7 @@ contract MMatchOrders is LibOrder.Order memory rightOrder ) internal - pure; + view; /// @dev Calculates fill amounts for the matched orders. /// Each order is filled at their respective price point. However, the calculations are diff --git a/contracts/exchange/contracts/src/mixins/MSignatureValidator.sol b/contracts/exchange/contracts/src/mixins/MSignatureValidator.sol index 705c795ca7..b637b18d6e 100644 --- a/contracts/exchange/contracts/src/mixins/MSignatureValidator.sol +++ b/contracts/exchange/contracts/src/mixins/MSignatureValidator.sol @@ -42,35 +42,4 @@ contract MSignatureValidator is PreSigned, // 0x06 NSignatureTypes // 0x07, number of signature types. Always leave at end. } - - /// @dev Verifies signature using logic defined by Wallet contract. - /// @param hash Any 32 byte hash. - /// @param walletAddress Address that should have signed the given hash - /// and defines its own signature verification method. - /// @param signature Proof that the hash has been signed by signer. - /// @return True if the address recovered from the provided signature matches the input signer address. - function isValidWalletSignature( - bytes32 hash, - address walletAddress, - bytes memory signature - ) - internal - view - returns (bool isValid); - - /// @dev Verifies signature using logic defined by Validator contract. - /// @param validatorAddress Address of validator contract. - /// @param hash Any 32 byte hash. - /// @param signerAddress Address that should have signed the given hash. - /// @param signature Proof that the hash has been signed by signer. - /// @return True if the address recovered from the provided signature matches the input signer address. - function isValidValidatorSignature( - address validatorAddress, - bytes32 hash, - address signerAddress, - bytes memory signature - ) - internal - view - returns (bool isValid); } diff --git a/contracts/exchange/contracts/test/TestAssetProxyDispatcher.sol b/contracts/exchange/contracts/test/TestAssetProxyDispatcher.sol index f010334d4a..e2d64fc00f 100644 --- a/contracts/exchange/contracts/test/TestAssetProxyDispatcher.sol +++ b/contracts/exchange/contracts/test/TestAssetProxyDispatcher.sol @@ -19,12 +19,15 @@ pragma solidity ^0.5.5; import "../src/MixinAssetProxyDispatcher.sol"; +import "../src/MixinExchangeRichErrors.sol"; -contract TestAssetProxyDispatcher is - MixinAssetProxyDispatcher +contract TestAssetProxyDispatcher is + MixinAssetProxyDispatcher, + MixinExchangeRichErrors { function publicDispatchTransferFrom( + bytes32 orderHash, bytes memory assetData, address from, address to, @@ -32,6 +35,6 @@ contract TestAssetProxyDispatcher is ) public { - dispatchTransferFrom(assetData, from, to, amount); + dispatchTransferFrom(orderHash, assetData, from, to, amount); } } diff --git a/contracts/exchange/contracts/test/TestRevertReceiver.sol b/contracts/exchange/contracts/test/TestRevertReceiver.sol new file mode 100644 index 0000000000..9a01048c5f --- /dev/null +++ b/contracts/exchange/contracts/test/TestRevertReceiver.sol @@ -0,0 +1,58 @@ +/* + + Copyright 2018 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.5.5; + +import "@0x/contracts-erc20/contracts/src/interfaces/IERC20Token.sol"; + + +// solhint-disable no-unused-vars +contract TestRevertReceiver { + + string constant public REVERT_REASON = "you shall not pass"; + + /// @dev Reverts with `REVERT_REASON`. Intended to be used with `Validator` signature type. + /// @param hash Message hash that is signed. + /// @param signerAddress Address that should have signed the given hash. + /// @param signature Proof of signing. + /// @return Validity of order signature. + function isValidSignature( + bytes32 hash, + address signerAddress, + bytes calldata signature + ) + external + returns (bool isValid) + { + revert(REVERT_REASON); + } + + /// @dev Reverts with `REVERT_REASON`. Intended to be used with `Wallet` signature type. + /// @param hash Message hash that is signed. + /// @param signature Proof of signing. + /// @return Validity of order signature. + function isValidSignature( + bytes32 hash, + bytes calldata signature + ) + external + returns (bool isValid) + { + revert(REVERT_REASON); + } +} diff --git a/contracts/exchange/contracts/test/TestSignatureValidator.sol b/contracts/exchange/contracts/test/TestSignatureValidator.sol index 108974bdaa..4f68928ac4 100644 --- a/contracts/exchange/contracts/test/TestSignatureValidator.sol +++ b/contracts/exchange/contracts/test/TestSignatureValidator.sol @@ -22,12 +22,14 @@ pragma experimental ABIEncoderV2; import "@0x/contracts-exchange-libs/contracts/src/LibEIP712ExchangeDomain.sol"; import "../src/MixinSignatureValidator.sol"; import "../src/MixinTransactions.sol"; +import "../src/MixinExchangeRichErrors.sol"; contract TestSignatureValidator is LibEIP712ExchangeDomain, MixinSignatureValidator, - MixinTransactions + MixinTransactions, + MixinExchangeRichErrors { // solhint-disable no-empty-blocks diff --git a/contracts/exchange/package.json b/contracts/exchange/package.json index 6f4f51bae6..569db4abe4 100644 --- a/contracts/exchange/package.json +++ b/contracts/exchange/package.json @@ -33,7 +33,7 @@ "lint-contracts": "solhint -c ../.solhint.json contracts/**/**/**/**/*.sol" }, "config": { - "abis": "./generated-artifacts/@(Exchange|ExchangeWrapper|IAssetProxyDispatcher|IExchange|IExchangeCore|IMatchOrders|ISignatureValidator|ITransactions|IValidator|IWallet|IWrapperFunctions|ReentrantERC20Token|TestAssetProxyDispatcher|TestExchangeInternals|TestSignatureValidator|TestStaticCallReceiver|Validator|Wallet|Whitelist).json", + "abis": "./generated-artifacts/@(Exchange|ExchangeWrapper|IAssetProxyDispatcher|IExchange|IExchangeCore|IMatchOrders|ISignatureValidator|ITransactions|IValidator|IWallet|IWrapperFunctions|ReentrantERC20Token|TestAssetProxyDispatcher|TestExchangeInternals|TestRevertReceiver|TestSignatureValidator|TestStaticCallReceiver|Validator|Wallet|Whitelist).json", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually." }, "repository": { diff --git a/contracts/exchange/src/artifacts.ts b/contracts/exchange/src/artifacts.ts index 5928b8a43e..19c1a66f43 100644 --- a/contracts/exchange/src/artifacts.ts +++ b/contracts/exchange/src/artifacts.ts @@ -19,6 +19,7 @@ import * as IWrapperFunctions from '../generated-artifacts/IWrapperFunctions.jso import * as ReentrantERC20Token from '../generated-artifacts/ReentrantERC20Token.json'; import * as TestAssetProxyDispatcher from '../generated-artifacts/TestAssetProxyDispatcher.json'; import * as TestExchangeInternals from '../generated-artifacts/TestExchangeInternals.json'; +import * as TestRevertReceiver from '../generated-artifacts/TestRevertReceiver.json'; import * as TestSignatureValidator from '../generated-artifacts/TestSignatureValidator.json'; import * as TestStaticCallReceiver from '../generated-artifacts/TestStaticCallReceiver.json'; import * as Validator from '../generated-artifacts/Validator.json'; @@ -39,9 +40,10 @@ export const artifacts = { IValidator: IValidator as ContractArtifact, IWallet: IWallet as ContractArtifact, IWrapperFunctions: IWrapperFunctions as ContractArtifact, + ReentrantERC20Token: ReentrantERC20Token as ContractArtifact, TestAssetProxyDispatcher: TestAssetProxyDispatcher as ContractArtifact, TestExchangeInternals: TestExchangeInternals as ContractArtifact, TestSignatureValidator: TestSignatureValidator as ContractArtifact, TestStaticCallReceiver: TestStaticCallReceiver as ContractArtifact, - ReentrantERC20Token: ReentrantERC20Token as ContractArtifact, + TestRevertReceiver: TestRevertReceiver as ContractArtifact, }; diff --git a/contracts/exchange/src/wrappers.ts b/contracts/exchange/src/wrappers.ts index 76928a0abd..3a355d08c3 100644 --- a/contracts/exchange/src/wrappers.ts +++ b/contracts/exchange/src/wrappers.ts @@ -17,6 +17,7 @@ export * from '../generated-wrappers/i_wrapper_functions'; export * from '../generated-wrappers/reentrant_erc20_token'; export * from '../generated-wrappers/test_asset_proxy_dispatcher'; export * from '../generated-wrappers/test_exchange_internals'; +export * from '../generated-wrappers/test_revert_receiver'; export * from '../generated-wrappers/test_signature_validator'; export * from '../generated-wrappers/test_static_call_receiver'; export * from '../generated-wrappers/validator'; diff --git a/contracts/exchange/test/core.ts b/contracts/exchange/test/core.ts index 484ebb6d79..6138b8c83c 100644 --- a/contracts/exchange/test/core.ts +++ b/contracts/exchange/test/core.ts @@ -19,7 +19,6 @@ import { chaiSetup, constants, ERC20BalancesByOwner, - expectTransactionFailedAsync, getLatestBlockTimestampAsync, increaseTimeAndMineBlockAsync, OrderFactory, @@ -29,9 +28,9 @@ import { web3Wrapper, } from '@0x/contracts-test-utils'; import { BlockchainLifecycle } from '@0x/dev-utils'; -import { assetDataUtils, orderHashUtils } from '@0x/order-utils'; +import { assetDataUtils, ExchangeRevertErrors, orderHashUtils } from '@0x/order-utils'; import { RevertReason, SignatureType, SignedOrder } from '@0x/types'; -import { BigNumber, providerUtils } from '@0x/utils'; +import { BigNumber, providerUtils, StringRevertError } from '@0x/utils'; import { Web3Wrapper } from '@0x/web3-wrapper'; import * as chai from 'chai'; import { LogWithDecodedArgs } from 'ethereum-types'; @@ -51,6 +50,7 @@ import { chaiSetup.configure(); const expect = chai.expect; const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); + // tslint:disable:no-unnecessary-type-assertion describe('Exchange core', () => { let chainId: number; @@ -276,10 +276,8 @@ describe('Exchange core', () => { makerAssetData: await assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), }); await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId); - await expectTransactionFailedAsync( - exchangeWrapper.fillOrderAsync(signedOrder, takerAddress), - RevertReason.ReentrancyIllegal, - ); + const tx = exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); + return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); }); }); }; @@ -289,6 +287,7 @@ describe('Exchange core', () => { signedOrder = await orderFactory.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), }); + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); const v = ethUtil.toBuffer(signedOrder.signature.slice(0, 4)); const invalidR = ethUtil.sha3('invalidR'); @@ -297,19 +296,23 @@ describe('Exchange core', () => { const invalidSigBuff = Buffer.concat([v, invalidR, invalidS, signatureType]); const invalidSigHex = `0x${invalidSigBuff.toString('hex')}`; signedOrder.signature = invalidSigHex; - return expectTransactionFailedAsync( - exchangeWrapper.fillOrderAsync(signedOrder, takerAddress), - RevertReason.InvalidOrderSignature, - ); + const expectedError = new ExchangeRevertErrors.SignatureError( + ExchangeRevertErrors.SignatureErrorCode.BadSignature, + orderHashHex, + signedOrder.makerAddress, + invalidSigHex, + ); + const tx = exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); + return expect(tx).to.revertWith(expectedError); }); - it('should throw if no value is filled', async () => { + it('should throw if fully filled', async () => { signedOrder = await orderFactory.newSignedOrderAsync(); + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); - return expectTransactionFailedAsync( - exchangeWrapper.fillOrderAsync(signedOrder, takerAddress), - RevertReason.OrderUnfillable, - ); + const expectedError = new ExchangeRevertErrors.OrderStatusError(OrderStatus.FullyFilled, orderHashHex); + const tx = exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); + return expect(tx).to.revertWith(expectedError); }); it('should revert if `isValidSignature` tries to update state when SignatureType=Wallet', async () => { @@ -333,11 +336,16 @@ describe('Exchange core', () => { makerAddress: maliciousMakerAddress, makerFee: constants.ZERO_AMOUNT, }); + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); signedOrder.signature = `0x0${SignatureType.Wallet}`; - await expectTransactionFailedAsync( - exchangeWrapper.fillOrderAsync(signedOrder, takerAddress), - RevertReason.WalletError, - ); + const expectedError = new ExchangeRevertErrors.SignatureWalletError( + orderHashHex, + signedOrder.makerAddress, + signedOrder.signature, + constants.NULL_BYTES, + ); + const tx = exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); + return expect(tx).to.revertWith(expectedError); }); it('should revert if `isValidSignature` tries to update state when SignatureType=Validator', async () => { @@ -351,10 +359,15 @@ describe('Exchange core', () => { constants.AWAIT_TRANSACTION_MINED_MS, ); signedOrder.signature = `${maliciousValidator.address}0${SignatureType.Validator}`; - await expectTransactionFailedAsync( - exchangeWrapper.fillOrderAsync(signedOrder, takerAddress), - RevertReason.ValidatorError, - ); + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const expectedError = new ExchangeRevertErrors.SignatureValidatorError( + orderHashHex, + signedOrder.makerAddress, + signedOrder.signature, + constants.NULL_BYTES, + ); + const tx = exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); + return expect(tx).to.revertWith(expectedError); }); it('should not emit transfer events for transfers where from == to', async () => { @@ -530,42 +543,46 @@ describe('Exchange core', () => { }); it('should throw if not sent by maker', async () => { - return expectTransactionFailedAsync( - exchangeWrapper.cancelOrderAsync(signedOrder, takerAddress), - RevertReason.InvalidMaker, - ); + const orderHash = orderHashUtils.getOrderHashHex(signedOrder); + const expectedError = new ExchangeRevertErrors.InvalidMakerError(orderHash, takerAddress); + const tx = exchangeWrapper.cancelOrderAsync(signedOrder, takerAddress); + return expect(tx).to.revertWith(expectedError); }); it('should throw if makerAssetAmount is 0', async () => { signedOrder = await orderFactory.newSignedOrderAsync({ makerAssetAmount: new BigNumber(0), }); - - return expectTransactionFailedAsync( - exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress), - RevertReason.OrderUnfillable, + const orderHash = orderHashUtils.getOrderHashHex(signedOrder); + const expectedError = new ExchangeRevertErrors.OrderStatusError( + OrderStatus.InvalidMakerAssetAmount, + orderHash, ); + const tx = exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress); + return expect(tx).to.revertWith(expectedError); }); it('should throw if takerAssetAmount is 0', async () => { signedOrder = await orderFactory.newSignedOrderAsync({ takerAssetAmount: new BigNumber(0), }); - - return expectTransactionFailedAsync( - exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress), - RevertReason.OrderUnfillable, + const orderHash = orderHashUtils.getOrderHashHex(signedOrder); + const expectedError = new ExchangeRevertErrors.OrderStatusError( + OrderStatus.InvalidTakerAssetAmount, + orderHash, ); + const tx = exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress); + return expect(tx).to.revertWith(expectedError); }); - it('should be able to cancel a full order', async () => { + it('should be able to cancel an order', async () => { await exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress); - return expectTransactionFailedAsync( - exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { - takerAssetFillAmount: signedOrder.takerAssetAmount.div(2), - }), - RevertReason.OrderUnfillable, - ); + const orderHash = orderHashUtils.getOrderHashHex(signedOrder); + const expectedError = new ExchangeRevertErrors.OrderStatusError(OrderStatus.Cancelled, orderHash); + const tx = exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { + takerAssetFillAmount: signedOrder.takerAssetAmount.div(2), + }); + return expect(tx).to.revertWith(expectedError); }); it('should log 1 event with correct arguments', async () => { @@ -585,10 +602,10 @@ describe('Exchange core', () => { it('should throw if already cancelled', async () => { await exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress); - return expectTransactionFailedAsync( - exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress), - RevertReason.OrderUnfillable, - ); + const orderHash = orderHashUtils.getOrderHashHex(signedOrder); + const expectedError = new ExchangeRevertErrors.OrderStatusError(OrderStatus.Cancelled, orderHash); + const tx = exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress); + return expect(tx).to.revertWith(expectedError); }); it('should throw if order is expired', async () => { @@ -596,10 +613,10 @@ describe('Exchange core', () => { signedOrder = await orderFactory.newSignedOrderAsync({ expirationTimeSeconds: new BigNumber(currentTimestamp).minus(10), }); - return expectTransactionFailedAsync( - exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress), - RevertReason.OrderUnfillable, - ); + const orderHash = orderHashUtils.getOrderHashHex(signedOrder); + const expectedError = new ExchangeRevertErrors.OrderStatusError(OrderStatus.Expired, orderHash); + const tx = exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress); + return expect(tx).to.revertWith(expectedError); }); it('should throw if rounding error is greater than 0.1%', async () => { @@ -614,12 +631,10 @@ describe('Exchange core', () => { }); const fillTakerAssetAmount2 = new BigNumber(1); - return expectTransactionFailedAsync( - exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { - takerAssetFillAmount: fillTakerAssetAmount2, - }), - RevertReason.RoundingError, - ); + const tx = exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { + takerAssetFillAmount: fillTakerAssetAmount2, + }); + return expect(tx).to.revertWith(RevertReason.RoundingError); }); }); @@ -628,19 +643,25 @@ describe('Exchange core', () => { const orderEpoch = new BigNumber(1); await exchangeWrapper.cancelOrdersUpToAsync(orderEpoch, makerAddress); const lesserOrderEpoch = new BigNumber(0); - return expectTransactionFailedAsync( - exchangeWrapper.cancelOrdersUpToAsync(lesserOrderEpoch, makerAddress), - RevertReason.InvalidNewOrderEpoch, + const expectedError = new ExchangeRevertErrors.OrderEpochError( + makerAddress, + constants.NULL_ADDRESS, + orderEpoch.plus(1), ); + const tx = exchangeWrapper.cancelOrdersUpToAsync(lesserOrderEpoch, makerAddress); + return expect(tx).to.revertWith(expectedError); }); it('should fail to set orderEpoch equal to existing orderEpoch', async () => { const orderEpoch = new BigNumber(1); await exchangeWrapper.cancelOrdersUpToAsync(orderEpoch, makerAddress); - return expectTransactionFailedAsync( - exchangeWrapper.cancelOrdersUpToAsync(orderEpoch, makerAddress), - RevertReason.InvalidNewOrderEpoch, + const expectedError = new ExchangeRevertErrors.OrderEpochError( + makerAddress, + constants.NULL_ADDRESS, + orderEpoch.plus(1), ); + const tx = exchangeWrapper.cancelOrdersUpToAsync(orderEpoch, makerAddress); + return expect(tx).to.revertWith(expectedError); }); it('should cancel only orders with a orderEpoch less than existing orderEpoch', async () => { @@ -720,6 +741,7 @@ describe('Exchange core', () => { makerAssetData: assetDataUtils.encodeERC721AssetData(erc721Token.address, makerAssetId), takerAssetData: assetDataUtils.encodeERC721AssetData(erc721Token.address, takerAssetId), }); + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); // Verify pre-conditions const initialOwnerMakerAsset = await erc721Token.ownerOf.callAsync(makerAssetId); expect(initialOwnerMakerAsset).to.be.bignumber.not.equal(makerAddress); @@ -727,10 +749,13 @@ describe('Exchange core', () => { expect(initialOwnerTakerAsset).to.be.bignumber.equal(takerAddress); // Call Exchange const takerAssetFillAmount = signedOrder.takerAssetAmount; - return expectTransactionFailedAsync( - exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { takerAssetFillAmount }), - RevertReason.TransferFailed, + const expectedError = new ExchangeRevertErrors.AssetProxyTransferError( + orderHashHex, + signedOrder.makerAssetData, + new StringRevertError(RevertReason.TransferFailed).encode(), ); + const tx = exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { takerAssetFillAmount }); + return expect(tx).to.revertWith(expectedError); }); it('should throw when taker does not own the token with id takerAssetId', async () => { @@ -743,6 +768,7 @@ describe('Exchange core', () => { makerAssetData: assetDataUtils.encodeERC721AssetData(erc721Token.address, makerAssetId), takerAssetData: assetDataUtils.encodeERC721AssetData(erc721Token.address, takerAssetId), }); + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); // Verify pre-conditions const initialOwnerMakerAsset = await erc721Token.ownerOf.callAsync(makerAssetId); expect(initialOwnerMakerAsset).to.be.bignumber.equal(makerAddress); @@ -750,10 +776,13 @@ describe('Exchange core', () => { expect(initialOwnerTakerAsset).to.be.bignumber.not.equal(takerAddress); // Call Exchange const takerAssetFillAmount = signedOrder.takerAssetAmount; - return expectTransactionFailedAsync( - exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { takerAssetFillAmount }), - RevertReason.TransferFailed, + const expectedError = new ExchangeRevertErrors.AssetProxyTransferError( + orderHashHex, + signedOrder.takerAssetData, + new StringRevertError(RevertReason.TransferFailed).encode(), ); + const tx = exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { takerAssetFillAmount }); + return expect(tx).to.revertWith(expectedError); }); it('should throw when makerAssetAmount is greater than 1', async () => { @@ -766,6 +795,7 @@ describe('Exchange core', () => { makerAssetData: assetDataUtils.encodeERC721AssetData(erc721Token.address, makerAssetId), takerAssetData: assetDataUtils.encodeERC721AssetData(erc721Token.address, takerAssetId), }); + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); // Verify pre-conditions const initialOwnerMakerAsset = await erc721Token.ownerOf.callAsync(makerAssetId); expect(initialOwnerMakerAsset).to.be.bignumber.equal(makerAddress); @@ -773,10 +803,13 @@ describe('Exchange core', () => { expect(initialOwnerTakerAsset).to.be.bignumber.equal(takerAddress); // Call Exchange const takerAssetFillAmount = signedOrder.takerAssetAmount; - return expectTransactionFailedAsync( - exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { takerAssetFillAmount }), - RevertReason.InvalidAmount, + const expectedError = new ExchangeRevertErrors.AssetProxyTransferError( + orderHashHex, + signedOrder.makerAssetData, + new StringRevertError(RevertReason.InvalidAmount).encode(), ); + const tx = exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { takerAssetFillAmount }); + return expect(tx).to.revertWith(expectedError); }); it('should throw when takerAssetAmount is greater than 1', async () => { @@ -789,6 +822,7 @@ describe('Exchange core', () => { makerAssetData: assetDataUtils.encodeERC721AssetData(erc721Token.address, makerAssetId), takerAssetData: assetDataUtils.encodeERC721AssetData(erc721Token.address, takerAssetId), }); + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); // Verify pre-conditions const initialOwnerMakerAsset = await erc721Token.ownerOf.callAsync(makerAssetId); expect(initialOwnerMakerAsset).to.be.bignumber.equal(makerAddress); @@ -796,10 +830,13 @@ describe('Exchange core', () => { expect(initialOwnerTakerAsset).to.be.bignumber.equal(takerAddress); // Call Exchange const takerAssetFillAmount = signedOrder.takerAssetAmount; - return expectTransactionFailedAsync( - exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { takerAssetFillAmount }), - RevertReason.InvalidAmount, + const expectedError = new ExchangeRevertErrors.AssetProxyTransferError( + orderHashHex, + signedOrder.takerAssetData, + new StringRevertError(RevertReason.InvalidAmount).encode(), ); + const tx = exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { takerAssetFillAmount }); + return expect(tx).to.revertWith(expectedError); }); it('should throw on partial fill', async () => { @@ -813,10 +850,8 @@ describe('Exchange core', () => { }); // Call Exchange const takerAssetFillAmount = signedOrder.takerAssetAmount.div(2); - return expectTransactionFailedAsync( - exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { takerAssetFillAmount }), - RevertReason.RoundingError, - ); + const tx = exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { takerAssetFillAmount }); + return expect(tx).to.revertWith(RevertReason.RoundingError); }); }); diff --git a/contracts/exchange/test/dispatcher.ts b/contracts/exchange/test/dispatcher.ts index a257559365..7227583bd7 100644 --- a/contracts/exchange/test/dispatcher.ts +++ b/contracts/exchange/test/dispatcher.ts @@ -9,14 +9,14 @@ import { DummyERC20TokenContract } from '@0x/contracts-erc20'; import { chaiSetup, constants, - expectTransactionFailedAsync, LogDecoder, + orderUtils, provider, txDefaults, web3Wrapper, } from '@0x/contracts-test-utils'; import { BlockchainLifecycle } from '@0x/dev-utils'; -import { assetDataUtils } from '@0x/order-utils'; +import { assetDataUtils, ExchangeRevertErrors } from '@0x/order-utils'; import { AssetProxyId, RevertReason } from '@0x/types'; import { BigNumber } from '@0x/utils'; import * as chai from 'chai'; @@ -135,20 +135,18 @@ describe('AssetProxyDispatcher', () => { provider, txDefaults, ); - // Register new ERC20 Transfer Proxy contract - return expectTransactionFailedAsync( - assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(newErc20TransferProxy.address, { - from: owner, - }), - RevertReason.AssetProxyAlreadyExists, - ); + const expectedError = new ExchangeRevertErrors.AssetProxyExistsError(proxyAddress); + const tx = assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(newErc20TransferProxy.address, { + from: owner, + }); + return expect(tx).to.revertWith(expectedError); }); it('should throw if requesting address is not owner', async () => { - return expectTransactionFailedAsync( - assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { from: notOwner }), - RevertReason.OnlyContractOwner, - ); + const tx = assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { + from: notOwner, + }); + return expect(tx).to.revertWith(RevertReason.OnlyContractOwner); }); it('should log an event with correct arguments when an asset proxy is registered', async () => { @@ -180,6 +178,7 @@ describe('AssetProxyDispatcher', () => { }); describe('dispatchTransferFrom', () => { + const orderHash = orderUtils.generatePseudoRandomOrderHash(); it('should dispatch transfer to registered proxy', async () => { // Register ERC20 proxy await web3Wrapper.awaitTransactionSuccessAsync( @@ -194,6 +193,7 @@ describe('AssetProxyDispatcher', () => { const amount = new BigNumber(10); await web3Wrapper.awaitTransactionSuccessAsync( await assetProxyDispatcher.publicDispatchTransferFrom.sendTransactionAsync( + orderHash, encodedAssetData, makerAddress, takerAddress, @@ -226,6 +226,7 @@ describe('AssetProxyDispatcher', () => { const amount = constants.ZERO_AMOUNT; const txReceipt = await web3Wrapper.awaitTransactionSuccessAsync( await assetProxyDispatcher.publicDispatchTransferFrom.sendTransactionAsync( + orderHash, encodedAssetData, makerAddress, takerAddress, @@ -253,6 +254,7 @@ describe('AssetProxyDispatcher', () => { const amount = new BigNumber(10); const txReceipt = await web3Wrapper.awaitTransactionSuccessAsync( await assetProxyDispatcher.publicDispatchTransferFrom.sendTransactionAsync( + orderHash, encodedAssetData, makerAddress, makerAddress, @@ -271,16 +273,20 @@ describe('AssetProxyDispatcher', () => { const encodedAssetData = assetDataUtils.encodeERC20AssetData(zrxToken.address); // Perform a transfer from makerAddress to takerAddress const amount = new BigNumber(10); - return expectTransactionFailedAsync( - assetProxyDispatcher.publicDispatchTransferFrom.sendTransactionAsync( - encodedAssetData, - makerAddress, - takerAddress, - amount, - { from: owner }, - ), - RevertReason.AssetProxyDoesNotExist, + const expectedError = new ExchangeRevertErrors.AssetProxyDispatchError( + ExchangeRevertErrors.AssetProxyDispatchErrorCode.UnknownAssetProxy, + orderHash, + encodedAssetData, + ); + const tx = assetProxyDispatcher.publicDispatchTransferFrom.sendTransactionAsync( + orderHash, + encodedAssetData, + makerAddress, + takerAddress, + amount, + { from: owner }, ); + return expect(tx).to.revertWith(expectedError); }); }); }); diff --git a/contracts/exchange/test/match_orders.ts b/contracts/exchange/test/match_orders.ts index b247e791dd..75859dc646 100644 --- a/contracts/exchange/test/match_orders.ts +++ b/contracts/exchange/test/match_orders.ts @@ -6,15 +6,14 @@ import { constants, ERC20BalancesByOwner, ERC721TokenIdsByOwner, - expectTransactionFailedAsync, OrderFactory, provider, txDefaults, web3Wrapper, } from '@0x/contracts-test-utils'; import { BlockchainLifecycle } from '@0x/dev-utils'; -import { assetDataUtils } from '@0x/order-utils'; -import { RevertReason } from '@0x/types'; +import { assetDataUtils, ExchangeRevertErrors, orderHashUtils } from '@0x/order-utils'; +import { OrderStatus, RevertReason } from '@0x/types'; import { BigNumber, providerUtils } from '@0x/utils'; import { Web3Wrapper } from '@0x/web3-wrapper'; import * as chai from 'chai'; @@ -588,10 +587,8 @@ describe('matchOrders', () => { await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId), constants.AWAIT_TRANSACTION_MINED_MS, ); - await expectTransactionFailedAsync( - exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress), - RevertReason.ReentrancyIllegal, - ); + const tx = exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress); + return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); }); }); }; @@ -1125,13 +1122,13 @@ describe('matchOrders', () => { makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), }); + const orderHashHexLeft = orderHashUtils.getOrderHashHex(signedOrderLeft); // Cancel left order await exchangeWrapper.cancelOrderAsync(signedOrderLeft, signedOrderLeft.makerAddress); // Match orders - return expectTransactionFailedAsync( - exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress), - RevertReason.OrderUnfillable, - ); + const expectedError = new ExchangeRevertErrors.OrderStatusError(OrderStatus.Cancelled, orderHashHexLeft); + const tx = exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress); + return expect(tx).to.revertWith(expectedError); }); it('Should throw if right order is not fillable', async () => { @@ -1144,13 +1141,13 @@ describe('matchOrders', () => { makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), }); + const orderHashHexRight = orderHashUtils.getOrderHashHex(signedOrderRight); // Cancel right order await exchangeWrapper.cancelOrderAsync(signedOrderRight, signedOrderRight.makerAddress); // Match orders - return expectTransactionFailedAsync( - exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress), - RevertReason.OrderUnfillable, - ); + const expectedError = new ExchangeRevertErrors.OrderStatusError(OrderStatus.Cancelled, orderHashHexRight); + const tx = exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress); + return expect(tx).to.revertWith(expectedError); }); it('should throw if there is not a positive spread', async () => { @@ -1163,11 +1160,12 @@ describe('matchOrders', () => { makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(1), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(200), 18), }); + const orderHashHexLeft = orderHashUtils.getOrderHashHex(signedOrderLeft); + const orderHashHexRight = orderHashUtils.getOrderHashHex(signedOrderRight); // Match orders - return expectTransactionFailedAsync( - exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress), - RevertReason.NegativeSpreadRequired, - ); + const expectedError = new ExchangeRevertErrors.NegativeSpreadError(orderHashHexLeft, orderHashHexRight); + const tx = exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress); + return expect(tx).to.revertWith(expectedError); }); it('should throw if the left maker asset is not equal to the right taker asset ', async () => { @@ -1181,15 +1179,24 @@ describe('matchOrders', () => { makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), }); - // Match orders - return expectTransactionFailedAsync( - exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress), - // We are assuming assetData fields of the right order are the - // reverse of the left order, rather than checking equality. This - // saves a bunch of gas, but as a result if the assetData fields are - // off then the failure ends up happening at signature validation - RevertReason.InvalidOrderSignature, + // We are assuming assetData fields of the right order are the + // reverse of the left order, rather than checking equality. This + // saves a bunch of gas, but as a result if the assetData fields are + // off then the failure ends up happening at signature validation + const reconstructedOrderRight = { + ...signedOrderRight, + takerAssetData: signedOrderLeft.makerAssetData, + }; + const orderHashHex = orderHashUtils.getOrderHashHex(reconstructedOrderRight); + const expectedError = new ExchangeRevertErrors.SignatureError( + ExchangeRevertErrors.SignatureErrorCode.BadSignature, + orderHashHex, + signedOrderRight.makerAddress, + signedOrderRight.signature, ); + // Match orders + const tx = exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress); + return expect(tx).to.revertWith(expectedError); }); it('should throw if the right maker asset is not equal to the left taker asset', async () => { @@ -1203,11 +1210,20 @@ describe('matchOrders', () => { makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), }); - // Match orders - return expectTransactionFailedAsync( - exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress), - RevertReason.InvalidOrderSignature, + const reconstructedOrderRight = { + ...signedOrderRight, + makerAssetData: signedOrderLeft.takerAssetData, + }; + const orderHashHex = orderHashUtils.getOrderHashHex(reconstructedOrderRight); + const expectedError = new ExchangeRevertErrors.SignatureError( + ExchangeRevertErrors.SignatureErrorCode.BadSignature, + orderHashHex, + signedOrderRight.makerAddress, + signedOrderRight.signature, ); + // Match orders + const tx = exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress); + return expect(tx).to.revertWith(expectedError); }); it('should transfer correct amounts when left order maker asset is an ERC721 token', async () => { diff --git a/contracts/exchange/test/signature_validator.ts b/contracts/exchange/test/signature_validator.ts index ef820d3e6c..3c9d0677fb 100644 --- a/contracts/exchange/test/signature_validator.ts +++ b/contracts/exchange/test/signature_validator.ts @@ -2,7 +2,6 @@ import { addressUtils, chaiSetup, constants, - expectContractCallFailedAsync, LogDecoder, OrderFactory, provider, @@ -10,15 +9,16 @@ import { web3Wrapper, } from '@0x/contracts-test-utils'; import { BlockchainLifecycle } from '@0x/dev-utils'; -import { assetDataUtils, orderHashUtils, signatureUtils } from '@0x/order-utils'; -import { RevertReason, SignatureType, SignedOrder } from '@0x/types'; -import { BigNumber, providerUtils } from '@0x/utils'; +import { assetDataUtils, ExchangeRevertErrors, orderHashUtils, signatureUtils } from '@0x/order-utils'; +import { SignatureType, SignedOrder } from '@0x/types'; +import { BigNumber, providerUtils, StringRevertError } from '@0x/utils'; import * as chai from 'chai'; import { LogWithDecodedArgs } from 'ethereum-types'; import ethUtil = require('ethereumjs-util'); import { artifacts, + TestRevertReceiverContract, TestSignatureValidatorContract, TestSignatureValidatorSignatureValidatorApprovalEventArgs, TestStaticCallReceiverContract, @@ -40,6 +40,8 @@ describe('MixinSignatureValidator', () => { let testValidator: ValidatorContract; let maliciousWallet: TestStaticCallReceiverContract; let maliciousValidator: TestStaticCallReceiverContract; + let revertingWallet: TestRevertReceiverContract; + let revertingValidator: TestRevertReceiverContract; let signerAddress: string; let signerPrivateKey: Buffer; let notSignerAddress: string; @@ -81,6 +83,11 @@ describe('MixinSignatureValidator', () => { provider, txDefaults, ); + revertingWallet = revertingValidator = await TestRevertReceiverContract.deployFrom0xArtifactAsync( + artifacts.TestRevertReceiver, + provider, + txDefaults, + ); signatureValidatorLogDecoder = new LogDecoder(web3Wrapper, artifacts); await web3Wrapper.awaitTransactionSuccessAsync( await signatureValidator.setSignatureValidatorApproval.sendTransactionAsync(testValidator.address, true, { @@ -98,6 +105,16 @@ describe('MixinSignatureValidator', () => { ), constants.AWAIT_TRANSACTION_MINED_MS, ); + await web3Wrapper.awaitTransactionSuccessAsync( + await signatureValidator.setSignatureValidatorApproval.sendTransactionAsync( + revertingValidator.address, + true, + { + from: signerAddress, + }, + ), + constants.AWAIT_TRANSACTION_MINED_MS, + ); const defaultOrderParams = { ...constants.STATIC_ORDER_PARAMS, @@ -123,48 +140,62 @@ describe('MixinSignatureValidator', () => { }); describe('isValidSignature', () => { + const REVERT_REASON = 'you shall not pass'; + beforeEach(async () => { signedOrder = await orderFactory.newSignedOrderAsync(); }); it('should revert when signature is empty', async () => { - const emptySignature = '0x'; + const emptySignature = constants.NULL_BYTES; const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - return expectContractCallFailedAsync( - signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signedOrder.makerAddress, - emptySignature, - ), - RevertReason.LengthGreaterThan0Required, + const expectedError = new ExchangeRevertErrors.SignatureError( + ExchangeRevertErrors.SignatureErrorCode.InvalidLength, + orderHashHex, + signedOrder.makerAddress, + emptySignature, ); + const tx = signatureValidator.publicIsValidSignature.callAsync( + orderHashHex, + signedOrder.makerAddress, + emptySignature, + ); + return expect(tx).to.revertWith(expectedError); }); it('should revert when signature type is unsupported', async () => { const unsupportedSignatureType = SignatureType.NSignatureTypes; const unsupportedSignatureHex = `0x${Buffer.from([unsupportedSignatureType]).toString('hex')}`; const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - return expectContractCallFailedAsync( - signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signedOrder.makerAddress, - unsupportedSignatureHex, - ), - RevertReason.SignatureUnsupported, + const expectedError = new ExchangeRevertErrors.SignatureError( + ExchangeRevertErrors.SignatureErrorCode.Unsupported, + orderHashHex, + signedOrder.makerAddress, + unsupportedSignatureHex, ); + const tx = signatureValidator.publicIsValidSignature.callAsync( + orderHashHex, + signedOrder.makerAddress, + unsupportedSignatureHex, + ); + return expect(tx).to.revertWith(expectedError); }); it('should revert when SignatureType=Illegal', async () => { - const unsupportedSignatureHex = `0x${Buffer.from([SignatureType.Illegal]).toString('hex')}`; + const illegalSignatureHex = `0x${Buffer.from([SignatureType.Illegal]).toString('hex')}`; const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - return expectContractCallFailedAsync( - signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signedOrder.makerAddress, - unsupportedSignatureHex, - ), - RevertReason.SignatureIllegal, + const expectedError = new ExchangeRevertErrors.SignatureError( + ExchangeRevertErrors.SignatureErrorCode.Illegal, + orderHashHex, + signedOrder.makerAddress, + illegalSignatureHex, + ); + const tx = signatureValidator.publicIsValidSignature.callAsync( + orderHashHex, + signedOrder.makerAddress, + illegalSignatureHex, ); + return expect(tx).to.revertWith(expectedError); }); it('should return false when SignatureType=Invalid and signature has a length of zero', async () => { @@ -184,14 +215,18 @@ describe('MixinSignatureValidator', () => { const signatureBuffer = Buffer.concat([fillerData, signatureType]); const signatureHex = ethUtil.bufferToHex(signatureBuffer); const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - return expectContractCallFailedAsync( - signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signedOrder.makerAddress, - signatureHex, - ), - RevertReason.Length0Required, + const expectedError = new ExchangeRevertErrors.SignatureError( + ExchangeRevertErrors.SignatureErrorCode.InvalidLength, + orderHashHex, + signedOrder.makerAddress, + signatureHex, + ); + const tx = signatureValidator.publicIsValidSignature.callAsync( + orderHashHex, + signedOrder.makerAddress, + signatureHex, ); + return expect(tx).to.revertWith(expectedError); }); it('should return true when SignatureType=EIP712 and signature is valid', async () => { @@ -344,14 +379,45 @@ describe('MixinSignatureValidator', () => { ethUtil.toBuffer(`0x${SignatureType.Wallet}`), ]); const signatureHex = ethUtil.bufferToHex(signature); - await expectContractCallFailedAsync( - signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - maliciousWallet.address, - signatureHex, - ), - RevertReason.WalletError, + const expectedError = new ExchangeRevertErrors.SignatureWalletError( + orderHashHex, + maliciousWallet.address, + signatureHex, + constants.NULL_BYTES, + ); + const tx = signatureValidator.publicIsValidSignature.callAsync( + orderHashHex, + maliciousWallet.address, + signatureHex, + ); + return expect(tx).to.revertWith(expectedError); + }); + + it('should revert when `isValidSignature` reverts and SignatureType=Wallet', async () => { + // Create EIP712 signature + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const orderHashBuffer = ethUtil.toBuffer(orderHashHex); + const ecSignature = ethUtil.ecsign(orderHashBuffer, signerPrivateKey); + // Create 0x signature from EIP712 signature + const signature = Buffer.concat([ + ethUtil.toBuffer(ecSignature.v), + ecSignature.r, + ecSignature.s, + ethUtil.toBuffer(`0x${SignatureType.Wallet}`), + ]); + const signatureHex = ethUtil.bufferToHex(signature); + const expectedError = new ExchangeRevertErrors.SignatureWalletError( + orderHashHex, + revertingWallet.address, + signatureHex, + new StringRevertError(REVERT_REASON).encode(), ); + const tx = signatureValidator.publicIsValidSignature.callAsync( + orderHashHex, + revertingWallet.address, + signatureHex, + ); + return expect(tx).to.revertWith(expectedError); }); it('should return true when SignatureType=Validator, signature is valid and validator is approved', async () => { @@ -390,11 +456,32 @@ describe('MixinSignatureValidator', () => { const signature = Buffer.concat([validatorAddress, signatureType]); const signatureHex = ethUtil.bufferToHex(signature); const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - await expectContractCallFailedAsync( - signatureValidator.publicIsValidSignature.callAsync(orderHashHex, signerAddress, signatureHex), - RevertReason.ValidatorError, + const expectedError = new ExchangeRevertErrors.SignatureValidatorError( + orderHashHex, + signedOrder.makerAddress, + signatureHex, + constants.NULL_BYTES, + ); + const tx = signatureValidator.publicIsValidSignature.callAsync(orderHashHex, signerAddress, signatureHex); + return expect(tx).to.revertWith(expectedError); + }); + + it('should revert when `isValidSignature` reverts and SignatureType=Validator', async () => { + const validatorAddress = ethUtil.toBuffer(`${revertingValidator.address}`); + const signatureType = ethUtil.toBuffer(`0x${SignatureType.Validator}`); + const signature = Buffer.concat([validatorAddress, signatureType]); + const signatureHex = ethUtil.bufferToHex(signature); + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const expectedError = new ExchangeRevertErrors.SignatureValidatorError( + orderHashHex, + signedOrder.makerAddress, + signatureHex, + new StringRevertError(REVERT_REASON).encode(), ); + const tx = signatureValidator.publicIsValidSignature.callAsync(orderHashHex, signerAddress, signatureHex); + return expect(tx).to.revertWith(expectedError); }); + it('should return false when SignatureType=Validator, signature is valid and validator is not approved', async () => { // Set approval of signature validator to false await web3Wrapper.awaitTransactionSuccessAsync( diff --git a/contracts/exchange/test/transactions.ts b/contracts/exchange/test/transactions.ts index 862e0aa207..e3e645874f 100644 --- a/contracts/exchange/test/transactions.ts +++ b/contracts/exchange/test/transactions.ts @@ -1,10 +1,10 @@ +// tslint:disable: max-file-line-count import { ERC20ProxyContract, ERC20Wrapper } from '@0x/contracts-asset-proxy'; import { DummyERC20TokenContract } from '@0x/contracts-erc20'; import { chaiSetup, constants, ERC20BalancesByOwner, - expectTransactionFailedAsync, OrderFactory, orderUtils, provider, @@ -13,9 +13,16 @@ import { web3Wrapper, } from '@0x/contracts-test-utils'; import { BlockchainLifecycle } from '@0x/dev-utils'; -import { assetDataUtils, generatePseudoRandomSalt } from '@0x/order-utils'; +import { + assetDataUtils, + ExchangeRevertErrors, + generatePseudoRandomSalt, + orderHashUtils, + transactionHashUtils, +} from '@0x/order-utils'; import { EIP712DomainWithDefaultSchema, + OrderStatus, OrderWithoutDomain, RevertReason, SignedOrder, @@ -23,6 +30,7 @@ import { } from '@0x/types'; import { BigNumber, providerUtils } from '@0x/utils'; import * as chai from 'chai'; +import * as ethUtil from 'ethereumjs-util'; import * as _ from 'lodash'; import { artifacts, ExchangeContract, ExchangeWrapper, ExchangeWrapperContract, WhitelistContract } from '../src/'; @@ -147,11 +155,33 @@ describe('Exchange transactions', () => { signedTx = takerTransactionFactory.newSignedTransaction(data); }); + it('should throw if signature is invalid', async () => { + const v = ethUtil.toBuffer(signedTx.signature.slice(0, 4)); + const invalidR = ethUtil.sha3('invalidR'); + const invalidS = ethUtil.sha3('invalidS'); + const signatureType = ethUtil.toBuffer(`0x${signedTx.signature.slice(-2)}`); + const invalidSigBuff = Buffer.concat([v, invalidR, invalidS, signatureType]); + const invalidSigHex = `0x${invalidSigBuff.toString('hex')}`; + signedTx.signature = invalidSigHex; + const transactionHashHex = transactionHashUtils.getTransactionHashHex(signedTx); + const expectedError = new ExchangeRevertErrors.TransactionSignatureError( + transactionHashHex, + signedTx.signerAddress, + signedTx.signature, + ); + const tx = exchangeWrapper.executeTransactionAsync(signedTx, senderAddress); + return expect(tx).to.revertWith(expectedError); + }); + it('should throw if not called by specified sender', async () => { - return expectTransactionFailedAsync( - exchangeWrapper.executeTransactionAsync(signedTx, takerAddress), - RevertReason.FailedExecution, + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const transactionHashHex = transactionHashUtils.getTransactionHashHex(signedTx); + const expectedError = new ExchangeRevertErrors.TransactionExecutionError( + transactionHashHex, + new ExchangeRevertErrors.InvalidSenderError(orderHashHex, takerAddress).encode(), ); + const tx = exchangeWrapper.executeTransactionAsync(signedTx, takerAddress); + return expect(tx).to.revertWith(expectedError); }); it('should transfer the correct amounts when signed by taker and called by sender', async () => { @@ -191,10 +221,13 @@ describe('Exchange transactions', () => { it('should throw if the a 0x transaction with the same transactionHash has already been executed', async () => { await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress); - return expectTransactionFailedAsync( - exchangeWrapper.executeTransactionAsync(signedTx, senderAddress), - RevertReason.InvalidTxHash, + const transactionHashHex = transactionHashUtils.getTransactionHashHex(signedTx); + const expectedError = new ExchangeRevertErrors.TransactionError( + ExchangeRevertErrors.TransactionErrorCode.AlreadyExecuted, + transactionHashHex, ); + const tx = exchangeWrapper.executeTransactionAsync(signedTx, senderAddress); + return expect(tx).to.revertWith(expectedError); }); it('should reset the currentContextAddress', async () => { @@ -211,18 +244,22 @@ describe('Exchange transactions', () => { }); it('should throw if not called by specified sender', async () => { - return expectTransactionFailedAsync( - exchangeWrapper.executeTransactionAsync(signedTx, makerAddress), - RevertReason.FailedExecution, + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const transactionHashHex = transactionHashUtils.getTransactionHashHex(signedTx); + const expectedError = new ExchangeRevertErrors.TransactionExecutionError( + transactionHashHex, + new ExchangeRevertErrors.InvalidSenderError(orderHashHex, makerAddress).encode(), ); + const tx = exchangeWrapper.executeTransactionAsync(signedTx, makerAddress); + return expect(tx).to.revertWith(expectedError); }); it('should cancel the order when signed by maker and called by sender', async () => { await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress); - return expectTransactionFailedAsync( - exchangeWrapper.fillOrderAsync(signedOrder, senderAddress), - RevertReason.OrderUnfillable, - ); + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const expectedError = new ExchangeRevertErrors.OrderStatusError(OrderStatus.Cancelled, orderHashHex); + const tx = exchangeWrapper.fillOrderAsync(signedOrder, senderAddress); + return expect(tx).to.revertWith(expectedError); }); }); @@ -264,17 +301,21 @@ describe('Exchange transactions', () => { signedOrder.signature, ); const signedFillTx = takerTransactionFactory.newSignedTransaction(fillData); - return expectTransactionFailedAsync( - exchangeWrapperContract.fillOrder.sendTransactionAsync( - orderWithoutDomain, - takerAssetFillAmount, - signedFillTx.salt, - signedOrder.signature, - signedFillTx.signature, - { from: takerAddress }, - ), - RevertReason.FailedExecution, + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const transactionHashHex = transactionHashUtils.getTransactionHashHex(signedFillTx); + const expectedError = new ExchangeRevertErrors.TransactionExecutionError( + transactionHashHex, + new ExchangeRevertErrors.OrderStatusError(OrderStatus.Cancelled, orderHashHex).encode(), ); + const tx = exchangeWrapperContract.fillOrder.sendTransactionAsync( + orderWithoutDomain, + takerAssetFillAmount, + signedFillTx.salt, + signedOrder.signature, + signedFillTx.signature, + { from: takerAddress }, + ); + return expect(tx).to.revertWith(expectedError); }); it("should not cancel an order if not called from the order's sender", async () => { @@ -384,16 +425,14 @@ describe('Exchange transactions', () => { orderWithoutDomain = orderUtils.getOrderWithoutDomain(signedOrder); const takerAssetFillAmount = signedOrder.takerAssetAmount; const salt = generatePseudoRandomSalt(); - return expectTransactionFailedAsync( - whitelist.fillOrderIfWhitelisted.sendTransactionAsync( - orderWithoutDomain, - takerAssetFillAmount, - salt, - signedOrder.signature, - { from: takerAddress }, - ), - RevertReason.MakerNotWhitelisted, + const tx = whitelist.fillOrderIfWhitelisted.sendTransactionAsync( + orderWithoutDomain, + takerAssetFillAmount, + salt, + signedOrder.signature, + { from: takerAddress }, ); + return expect(tx).to.revertWith(RevertReason.MakerNotWhitelisted); }); it('should revert if taker has not been whitelisted', async () => { @@ -406,16 +445,14 @@ describe('Exchange transactions', () => { orderWithoutDomain = orderUtils.getOrderWithoutDomain(signedOrder); const takerAssetFillAmount = signedOrder.takerAssetAmount; const salt = generatePseudoRandomSalt(); - return expectTransactionFailedAsync( - whitelist.fillOrderIfWhitelisted.sendTransactionAsync( - orderWithoutDomain, - takerAssetFillAmount, - salt, - signedOrder.signature, - { from: takerAddress }, - ), - RevertReason.TakerNotWhitelisted, + const tx = whitelist.fillOrderIfWhitelisted.sendTransactionAsync( + orderWithoutDomain, + takerAssetFillAmount, + salt, + signedOrder.signature, + { from: takerAddress }, ); + return expect(tx).to.revertWith(RevertReason.TakerNotWhitelisted); }); it('should fill the order if maker and taker have been whitelisted', async () => { diff --git a/contracts/exchange/test/utils/fill_order_combinatorial_utils.ts b/contracts/exchange/test/utils/fill_order_combinatorial_utils.ts index b07e1c4b1e..3fcae8edfd 100644 --- a/contracts/exchange/test/utils/fill_order_combinatorial_utils.ts +++ b/contracts/exchange/test/utils/fill_order_combinatorial_utils.ts @@ -6,7 +6,6 @@ import { BalanceAmountScenario, chaiSetup, constants, - expectTransactionFailedAsync, ExpirationTimeSecondsScenario, FeeRecipientAddressScenario, FillScenario, @@ -21,13 +20,14 @@ import { import { assetDataUtils, BalanceAndProxyAllowanceLazyStore, + ExchangeRevertErrors, ExchangeTransferSimulator, orderHashUtils, OrderStateUtils, OrderValidationUtils, } from '@0x/order-utils'; -import { AssetProxyId, RevertReason, SignatureType, SignedOrder } from '@0x/types'; -import { BigNumber, errorUtils, logUtils, providerUtils } from '@0x/utils'; +import { AssetProxyId, Order, RevertReason, SignatureType, SignedOrder } from '@0x/types'; +import { BigNumber, errorUtils, logUtils, providerUtils, RevertError, StringRevertError } from '@0x/utils'; import { Web3Wrapper } from '@0x/web3-wrapper'; import * as chai from 'chai'; import { LogWithDecodedArgs, TxData } from 'ethereum-types'; @@ -417,7 +417,7 @@ export class FillOrderCombinatorialUtils { logUtils.log(`Expecting fillOrder to succeed.`); } } catch (err) { - fillRevertReasonIfExists = err.message; + fillRevertReasonIfExists = validationErrorToRevertError(order, err.message); if (isVerbose) { logUtils.log(`Expecting fillOrder to fail with:`); logUtils.log(err); @@ -438,13 +438,11 @@ export class FillOrderCombinatorialUtils { signedOrder: SignedOrder, takerAssetFillAmount: BigNumber, lazyStore: BalanceAndProxyAllowanceLazyStore, - fillRevertReasonIfExists: RevertReason | undefined, + fillRevertReasonIfExists: RevertReason | RevertError | undefined, ): Promise { if (!_.isUndefined(fillRevertReasonIfExists)) { - return expectTransactionFailedAsync( - this.exchangeWrapper.fillOrderAsync(signedOrder, this.takerAddress, { takerAssetFillAmount }), - fillRevertReasonIfExists, - ); + const tx = this.exchangeWrapper.fillOrderAsync(signedOrder, this.takerAddress, { takerAssetFillAmount }); + return expect(tx).to.revertWith(fillRevertReasonIfExists); } const makerAddress = signedOrder.makerAddress; @@ -928,4 +926,38 @@ export class FillOrderCombinatorialUtils { ); } } -} // tslint:disable:max-file-line-count +} + +// HACK(dorothy-zbnornak): OrderValidationUtils errors do not map perfectly to rich revert errors. +// This only covers the errors that are raised by these combinatorial tests. +// At some point it may be worthwhile to update OrderValidationUtils to throw +// rich revert errors. +function validationErrorToRevertError(order: Order, reason: RevertReason): RevertError { + const orderHash = orderHashUtils.getOrderHashHex(order); + switch (reason) { + case RevertReason.InvalidMaker: + return new ExchangeRevertErrors.InvalidMakerError(orderHash); + case RevertReason.InvalidTaker: + return new ExchangeRevertErrors.InvalidTakerError(orderHash); + case RevertReason.OrderUnfillable: + return new ExchangeRevertErrors.OrderStatusError(undefined, orderHash); + case RevertReason.InvalidTakerAmount: + return new ExchangeRevertErrors.FillError(ExchangeRevertErrors.FillErrorCode.InvalidTakerAmount, orderHash); + case RevertReason.TakerOverpay: + return new ExchangeRevertErrors.FillError(ExchangeRevertErrors.FillErrorCode.TakerOverpay, orderHash); + case RevertReason.OrderOverfill: + return new ExchangeRevertErrors.FillError(ExchangeRevertErrors.FillErrorCode.Overfill, orderHash); + case RevertReason.InvalidFillPrice: + return new ExchangeRevertErrors.FillError(ExchangeRevertErrors.FillErrorCode.InvalidFillPrice, orderHash); + case RevertReason.TransferFailed: + return new ExchangeRevertErrors.AssetProxyTransferError( + orderHash, + undefined, + new StringRevertError(RevertReason.TransferFailed).encode(), + ); + default: + return new StringRevertError(reason); + } +} + +// tslint:disable:max-file-line-count diff --git a/contracts/exchange/test/wrapper.ts b/contracts/exchange/test/wrapper.ts index 6287416c0c..5fa7b0460d 100644 --- a/contracts/exchange/test/wrapper.ts +++ b/contracts/exchange/test/wrapper.ts @@ -5,18 +5,16 @@ import { chaiSetup, constants, ERC20BalancesByOwner, - expectTransactionFailedAsync, getLatestBlockTimestampAsync, increaseTimeAndMineBlockAsync, OrderFactory, - OrderStatus, provider, txDefaults, web3Wrapper, } from '@0x/contracts-test-utils'; import { BlockchainLifecycle } from '@0x/dev-utils'; -import { assetDataUtils, orderHashUtils } from '@0x/order-utils'; -import { RevertReason, SignedOrder } from '@0x/types'; +import { assetDataUtils, ExchangeRevertErrors, orderHashUtils } from '@0x/order-utils'; +import { OrderStatus, RevertReason, SignedOrder } from '@0x/types'; import { BigNumber, providerUtils } from '@0x/utils'; import { Web3Wrapper } from '@0x/web3-wrapper'; import * as chai from 'chai'; @@ -152,10 +150,8 @@ describe('Exchange wrappers', () => { await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId), constants.AWAIT_TRANSACTION_MINED_MS, ); - await expectTransactionFailedAsync( - exchangeWrapper.fillOrKillOrderAsync(signedOrder, takerAddress), - RevertReason.ReentrancyIllegal, - ); + const tx = exchangeWrapper.fillOrKillOrderAsync(signedOrder, takerAddress); + return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); }); }); }; @@ -210,11 +206,10 @@ describe('Exchange wrappers', () => { const signedOrder = await orderFactory.newSignedOrderAsync({ expirationTimeSeconds: new BigNumber(currentTimestamp).minus(10), }); - - return expectTransactionFailedAsync( - exchangeWrapper.fillOrKillOrderAsync(signedOrder, takerAddress), - RevertReason.OrderUnfillable, - ); + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const expectedError = new ExchangeRevertErrors.OrderStatusError(OrderStatus.Expired, orderHashHex); + const tx = exchangeWrapper.fillOrKillOrderAsync(signedOrder, takerAddress); + return expect(tx).to.revertWith(expectedError); }); it('should throw if entire takerAssetFillAmount not filled', async () => { @@ -224,10 +219,10 @@ describe('Exchange wrappers', () => { takerAssetFillAmount: signedOrder.takerAssetAmount.div(2), }); - return expectTransactionFailedAsync( - exchangeWrapper.fillOrKillOrderAsync(signedOrder, takerAddress), - RevertReason.CompleteFillFailed, - ); + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const expectedError = new ExchangeRevertErrors.IncompleteFillError(orderHashHex); + const tx = exchangeWrapper.fillOrKillOrderAsync(signedOrder, takerAddress); + return expect(tx).to.revertWith(expectedError); }); }); @@ -462,10 +457,8 @@ describe('Exchange wrappers', () => { await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId), constants.AWAIT_TRANSACTION_MINED_MS, ); - await expectTransactionFailedAsync( - exchangeWrapper.batchFillOrdersAsync([signedOrder], takerAddress), - RevertReason.ReentrancyIllegal, - ); + const tx = exchangeWrapper.batchFillOrdersAsync([signedOrder], takerAddress); + return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); }); }); }; @@ -531,10 +524,8 @@ describe('Exchange wrappers', () => { await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId), constants.AWAIT_TRANSACTION_MINED_MS, ); - await expectTransactionFailedAsync( - exchangeWrapper.batchFillOrKillOrdersAsync([signedOrder], takerAddress), - RevertReason.ReentrancyIllegal, - ); + const tx = exchangeWrapper.batchFillOrKillOrdersAsync([signedOrder], takerAddress); + return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); }); }); }; @@ -595,13 +586,12 @@ describe('Exchange wrappers', () => { }); await exchangeWrapper.fillOrKillOrderAsync(signedOrders[0], takerAddress); - - return expectTransactionFailedAsync( - exchangeWrapper.batchFillOrKillOrdersAsync(signedOrders, takerAddress, { - takerAssetFillAmounts, - }), - RevertReason.OrderUnfillable, - ); + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrders[0]); + const expectedError = new ExchangeRevertErrors.OrderStatusError(OrderStatus.FullyFilled, orderHashHex); + const tx = exchangeWrapper.batchFillOrKillOrdersAsync(signedOrders, takerAddress, { + takerAssetFillAmounts, + }); + return expect(tx).to.revertWith(expectedError); }); }); @@ -749,12 +739,10 @@ describe('Exchange wrappers', () => { await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId), constants.AWAIT_TRANSACTION_MINED_MS, ); - await expectTransactionFailedAsync( - exchangeWrapper.marketSellOrdersAsync([signedOrder], takerAddress, { - takerAssetFillAmount: signedOrder.takerAssetAmount, - }), - RevertReason.ReentrancyIllegal, - ); + const tx = exchangeWrapper.marketSellOrdersAsync([signedOrder], takerAddress, { + takerAssetFillAmount: signedOrder.takerAssetAmount, + }); + return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); }); }); }; @@ -839,15 +827,21 @@ describe('Exchange wrappers', () => { }), await orderFactory.newSignedOrderAsync(), ]; - - return expectTransactionFailedAsync( - exchangeWrapper.marketSellOrdersAsync(signedOrders, takerAddress, { - takerAssetFillAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(1000), 18), - }), - // We simply use the takerAssetData from the first order for all orders. - // If they are not the same, the contract throws when validating the order signature - RevertReason.InvalidOrderSignature, + const reconstructedOrder = { + ...signedOrders[1], + takerAssetData: signedOrders[0].takerAssetData, + }; + const orderHashHex = orderHashUtils.getOrderHashHex(reconstructedOrder); + const expectedError = new ExchangeRevertErrors.SignatureError( + ExchangeRevertErrors.SignatureErrorCode.BadSignature, + orderHashHex, + signedOrders[1].makerAddress, + signedOrders[1].signature, ); + const tx = exchangeWrapper.marketSellOrdersAsync(signedOrders, takerAddress, { + takerAssetFillAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(1000), 18), + }); + return expect(tx).to.revertWith(expectedError); }); }); @@ -1010,12 +1004,10 @@ describe('Exchange wrappers', () => { await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId), constants.AWAIT_TRANSACTION_MINED_MS, ); - await expectTransactionFailedAsync( - exchangeWrapper.marketBuyOrdersAsync([signedOrder], takerAddress, { - makerAssetFillAmount: signedOrder.makerAssetAmount, - }), - RevertReason.ReentrancyIllegal, - ); + const tx = exchangeWrapper.marketBuyOrdersAsync([signedOrder], takerAddress, { + makerAssetFillAmount: signedOrder.makerAssetAmount, + }); + return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); }); }); }; @@ -1100,13 +1092,21 @@ describe('Exchange wrappers', () => { }), await orderFactory.newSignedOrderAsync(), ]; - - return expectTransactionFailedAsync( - exchangeWrapper.marketBuyOrdersAsync(signedOrders, takerAddress, { - makerAssetFillAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(1000), 18), - }), - RevertReason.InvalidOrderSignature, + const reconstructedOrder = { + ...signedOrders[1], + makerAssetData: signedOrders[0].makerAssetData, + }; + const orderHashHex = orderHashUtils.getOrderHashHex(reconstructedOrder); + const expectedError = new ExchangeRevertErrors.SignatureError( + ExchangeRevertErrors.SignatureErrorCode.BadSignature, + orderHashHex, + signedOrders[1].makerAddress, + signedOrders[1].signature, ); + const tx = exchangeWrapper.marketBuyOrdersAsync(signedOrders, takerAddress, { + makerAssetFillAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(1000), 18), + }); + return expect(tx).to.revertWith(expectedError); }); }); diff --git a/contracts/exchange/tsconfig.json b/contracts/exchange/tsconfig.json index 4158ece939..46bfc600bb 100644 --- a/contracts/exchange/tsconfig.json +++ b/contracts/exchange/tsconfig.json @@ -17,6 +17,7 @@ "generated-artifacts/ReentrantERC20Token.json", "generated-artifacts/TestAssetProxyDispatcher.json", "generated-artifacts/TestExchangeInternals.json", + "generated-artifacts/TestRevertReceiver.json", "generated-artifacts/TestSignatureValidator.json", "generated-artifacts/TestStaticCallReceiver.json", "generated-artifacts/Validator.json", diff --git a/contracts/extensions/CHANGELOG.json b/contracts/extensions/CHANGELOG.json index dd71967b35..8b311f2512 100644 --- a/contracts/extensions/CHANGELOG.json +++ b/contracts/extensions/CHANGELOG.json @@ -9,6 +9,10 @@ { "note": "Refactor BalanceThresholdFilter to use new ITransactions interface", "pr": 1753 + }, + { + "note": "Update tests for rich reverts", + "pr": 1761 } ] }, diff --git a/contracts/extensions/test/balance_threshold_filter.ts b/contracts/extensions/test/balance_threshold_filter.ts index 79f550a7c6..e6db69926c 100644 --- a/contracts/extensions/test/balance_threshold_filter.ts +++ b/contracts/extensions/test/balance_threshold_filter.ts @@ -1,6 +1,6 @@ import { ExchangeContract, ExchangeWrapper } from '@0x/contracts-exchange'; import { BlockchainLifecycle } from '@0x/dev-utils'; -import { assetDataUtils } from '@0x/order-utils'; +import { assetDataUtils, ExchangeRevertErrors } from '@0x/order-utils'; import { Order, RevertReason, SignedOrder } from '@0x/types'; import { BigNumber, providerUtils } from '@0x/utils'; import { Web3Wrapper } from '@0x/web3-wrapper'; @@ -15,7 +15,6 @@ import { constants, ContractName, ERC20BalancesByOwner, - expectTransactionFailedAsync, OrderFactory, OrderStatus, provider, @@ -293,15 +292,13 @@ describe(ContractName.BalanceThresholdFilter, () => { const badSelectorHex = '0x00000000'; const signatureHex = '0x'; // Call valid forwarder - return expectTransactionFailedAsync( - erc721BalanceThresholdFilterInstance.executeTransaction.sendTransactionAsync( - salt, - validTakerAddress, - badSelectorHex, - signatureHex, - ), - RevertReason.InvalidOrBlockedExchangeSelector, + const tx = erc721BalanceThresholdFilterInstance.executeTransaction.sendTransactionAsync( + salt, + validTakerAddress, + badSelectorHex, + signatureHex, ); + return expect(tx).to.revertWith(RevertReason.InvalidOrBlockedExchangeSelector); }); it('should revert if senderAddress is not set to the valid forwarding contract', async () => { // Create signed order with incorrect senderAddress @@ -309,13 +306,14 @@ describe(ContractName.BalanceThresholdFilter, () => { const signedOrderWithBadSenderAddress = await orderFactory.newSignedOrderAsync({ senderAddress: notBalanceThresholdFilterAddress, }); + const expectedError = new ExchangeRevertErrors.TransactionExecutionError(); // Call valid forwarder - return expectTransactionFailedAsync( - erc721TakerBalanceThresholdWrapper.fillOrderAsync(signedOrderWithBadSenderAddress, validTakerAddress, { - takerAssetFillAmount, - }), - RevertReason.FailedExecution, + const tx = erc721TakerBalanceThresholdWrapper.fillOrderAsync( + signedOrderWithBadSenderAddress, + validTakerAddress, + { takerAssetFillAmount }, ); + return expect(tx).to.revertWith(expectedError); }); }); @@ -396,22 +394,18 @@ describe(ContractName.BalanceThresholdFilter, () => { }); const orders = [validSignedOrder, signedOrderWithBadMakerAddress]; // Execute transaction - return expectTransactionFailedAsync( - erc721TakerBalanceThresholdWrapper.batchFillOrdersAsync(orders, validTakerAddress, { - takerAssetFillAmounts, - }), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, - ); + const tx = erc721TakerBalanceThresholdWrapper.batchFillOrdersAsync(orders, validTakerAddress, { + takerAssetFillAmounts, + }); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); it('should revert if taker does not meet the balance threshold', async () => { const orders = [validSignedOrder, validSignedOrder2]; const takerAssetFillAmounts = [takerAssetFillAmount, takerAssetFillAmount]; - return expectTransactionFailedAsync( - erc721NonValidBalanceThresholdWrapper.batchFillOrdersAsync(orders, invalidAddress, { - takerAssetFillAmounts, - }), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, - ); + const tx = erc721NonValidBalanceThresholdWrapper.batchFillOrdersAsync(orders, invalidAddress, { + takerAssetFillAmounts, + }); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); }); @@ -500,22 +494,18 @@ describe(ContractName.BalanceThresholdFilter, () => { }); const orders = [validSignedOrder, signedOrderWithBadMakerAddress]; // Execute transaction - return expectTransactionFailedAsync( - erc721TakerBalanceThresholdWrapper.batchFillOrdersNoThrowAsync(orders, validTakerAddress, { - takerAssetFillAmounts, - }), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, - ); + const tx = erc721TakerBalanceThresholdWrapper.batchFillOrdersNoThrowAsync(orders, validTakerAddress, { + takerAssetFillAmounts, + }); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); it('should revert if taker does not meet the balance threshold', async () => { const orders = [validSignedOrder, validSignedOrder2]; const takerAssetFillAmounts = [takerAssetFillAmount, takerAssetFillAmount]; - return expectTransactionFailedAsync( - erc721NonValidBalanceThresholdWrapper.batchFillOrdersNoThrowAsync(orders, invalidAddress, { - takerAssetFillAmounts, - }), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, - ); + const tx = erc721NonValidBalanceThresholdWrapper.batchFillOrdersNoThrowAsync(orders, invalidAddress, { + takerAssetFillAmounts, + }); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); }); @@ -598,33 +588,29 @@ describe(ContractName.BalanceThresholdFilter, () => { }); const orders = [validSignedOrder, signedOrderWithBadMakerAddress]; // Execute transaction - return expectTransactionFailedAsync( - erc721TakerBalanceThresholdWrapper.batchFillOrKillOrdersAsync(orders, validTakerAddress, { - takerAssetFillAmounts, - }), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, - ); + const tx = erc721TakerBalanceThresholdWrapper.batchFillOrKillOrdersAsync(orders, validTakerAddress, { + takerAssetFillAmounts, + }); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); it('should revert if taker does not meet the balance threshold', async () => { const orders = [validSignedOrder, validSignedOrder2]; const takerAssetFillAmounts = [takerAssetFillAmount, takerAssetFillAmount]; - return expectTransactionFailedAsync( - erc721NonValidBalanceThresholdWrapper.batchFillOrKillOrdersAsync(orders, invalidAddress, { - takerAssetFillAmounts, - }), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, - ); + const tx = erc721NonValidBalanceThresholdWrapper.batchFillOrKillOrdersAsync(orders, invalidAddress, { + takerAssetFillAmounts, + }); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); it('should revert if one takerAssetFillAmount is not fully filled', async () => { const tooBigTakerAssetFillAmount = validSignedOrder.takerAssetAmount.times(2); const orders = [validSignedOrder, validSignedOrder2]; const takerAssetFillAmounts = [takerAssetFillAmount, tooBigTakerAssetFillAmount]; - return expectTransactionFailedAsync( - erc721TakerBalanceThresholdWrapper.batchFillOrKillOrdersAsync(orders, validTakerAddress, { - takerAssetFillAmounts, - }), - RevertReason.FailedExecution, - ); + const expectedError = new ExchangeRevertErrors.TransactionExecutionError(); + // Call valid forwarder + const tx = erc721TakerBalanceThresholdWrapper.batchFillOrKillOrdersAsync(orders, validTakerAddress, { + takerAssetFillAmounts, + }); + return expect(tx).to.revertWith(expectedError); }); }); @@ -683,20 +669,18 @@ describe(ContractName.BalanceThresholdFilter, () => { makerAddress: invalidAddress, }); // Execute transaction - return expectTransactionFailedAsync( - erc721TakerBalanceThresholdWrapper.fillOrderAsync(signedOrderWithBadMakerAddress, validTakerAddress, { - takerAssetFillAmount, - }), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, + const tx = erc721TakerBalanceThresholdWrapper.fillOrderAsync( + signedOrderWithBadMakerAddress, + validTakerAddress, + { takerAssetFillAmount }, ); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); it('should revert if taker does not meet the balance threshold', async () => { - return expectTransactionFailedAsync( - erc721NonValidBalanceThresholdWrapper.fillOrderAsync(validSignedOrder, invalidAddress, { - takerAssetFillAmount, - }), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, - ); + const tx = erc721NonValidBalanceThresholdWrapper.fillOrderAsync(validSignedOrder, invalidAddress, { + takerAssetFillAmount, + }); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); }); @@ -761,22 +745,18 @@ describe(ContractName.BalanceThresholdFilter, () => { makerAddress: invalidAddress, }); // Execute transaction - return expectTransactionFailedAsync( - erc721TakerBalanceThresholdWrapper.fillOrderNoThrowAsync( - signedOrderWithBadMakerAddress, - validTakerAddress, - { takerAssetFillAmount }, - ), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, + const tx = erc721TakerBalanceThresholdWrapper.fillOrderNoThrowAsync( + signedOrderWithBadMakerAddress, + validTakerAddress, + { takerAssetFillAmount }, ); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); it('should revert if taker does not meet the balance threshold', async () => { - return expectTransactionFailedAsync( - erc721NonValidBalanceThresholdWrapper.fillOrderNoThrowAsync(validSignedOrder, invalidAddress, { - takerAssetFillAmount, - }), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, - ); + const tx = erc721NonValidBalanceThresholdWrapper.fillOrderNoThrowAsync(validSignedOrder, invalidAddress, { + takerAssetFillAmount, + }); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); }); @@ -836,31 +816,26 @@ describe(ContractName.BalanceThresholdFilter, () => { makerAddress: invalidAddress, }); // Execute transaction - return expectTransactionFailedAsync( - erc721TakerBalanceThresholdWrapper.fillOrKillOrderAsync( - signedOrderWithBadMakerAddress, - validTakerAddress, - { takerAssetFillAmount }, - ), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, + const tx = erc721TakerBalanceThresholdWrapper.fillOrKillOrderAsync( + signedOrderWithBadMakerAddress, + validTakerAddress, + { takerAssetFillAmount }, ); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); it('should revert if taker does not meet the balance threshold', async () => { - return expectTransactionFailedAsync( - erc721NonValidBalanceThresholdWrapper.fillOrKillOrderAsync(validSignedOrder, invalidAddress, { - takerAssetFillAmount, - }), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, - ); + const tx = erc721NonValidBalanceThresholdWrapper.fillOrKillOrderAsync(validSignedOrder, invalidAddress, { + takerAssetFillAmount, + }); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); it('should revert if takerAssetFillAmount is not fully filled', async () => { const tooBigTakerAssetFillAmount = validSignedOrder.takerAssetAmount.times(2); - return expectTransactionFailedAsync( - erc721TakerBalanceThresholdWrapper.fillOrKillOrderAsync(validSignedOrder, validTakerAddress, { - takerAssetFillAmount: tooBigTakerAssetFillAmount, - }), - RevertReason.FailedExecution, - ); + const expectedError = new ExchangeRevertErrors.TransactionExecutionError(); + const tx = erc721TakerBalanceThresholdWrapper.fillOrKillOrderAsync(validSignedOrder, validTakerAddress, { + takerAssetFillAmount: tooBigTakerAssetFillAmount, + }); + return expect(tx).to.revertWith(expectedError); }); }); @@ -944,21 +919,17 @@ describe(ContractName.BalanceThresholdFilter, () => { }); const orders = [validSignedOrder, signedOrderWithBadMakerAddress]; // Execute transaction - return expectTransactionFailedAsync( - erc721TakerBalanceThresholdWrapper.marketSellOrdersAsync(orders, validTakerAddress, { - takerAssetFillAmount, - }), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, - ); + const tx = erc721TakerBalanceThresholdWrapper.marketSellOrdersAsync(orders, validTakerAddress, { + takerAssetFillAmount, + }); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); it('should revert if taker does not meet the balance threshold', async () => { const orders = [validSignedOrder, validSignedOrder2]; - return expectTransactionFailedAsync( - erc721NonValidBalanceThresholdWrapper.marketSellOrdersAsync(orders, invalidAddress, { - takerAssetFillAmount, - }), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, - ); + const tx = erc721NonValidBalanceThresholdWrapper.marketSellOrdersAsync(orders, invalidAddress, { + takerAssetFillAmount, + }); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); }); @@ -1048,21 +1019,17 @@ describe(ContractName.BalanceThresholdFilter, () => { }); const orders = [validSignedOrder, signedOrderWithBadMakerAddress]; // Execute transaction - return expectTransactionFailedAsync( - erc721TakerBalanceThresholdWrapper.marketSellOrdersNoThrowAsync(orders, validTakerAddress, { - takerAssetFillAmount, - }), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, - ); + const tx = erc721TakerBalanceThresholdWrapper.marketSellOrdersNoThrowAsync(orders, validTakerAddress, { + takerAssetFillAmount, + }); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); it('should revert if taker does not meet the balance threshold', async () => { const orders = [validSignedOrder, validSignedOrder2]; - return expectTransactionFailedAsync( - erc721NonValidBalanceThresholdWrapper.marketSellOrdersNoThrowAsync(orders, invalidAddress, { - takerAssetFillAmount, - }), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, - ); + const tx = erc721NonValidBalanceThresholdWrapper.marketSellOrdersNoThrowAsync(orders, invalidAddress, { + takerAssetFillAmount, + }); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); }); @@ -1145,22 +1112,18 @@ describe(ContractName.BalanceThresholdFilter, () => { const orders = [validSignedOrder, signedOrderWithBadMakerAddress]; // Execute transaction const dummyMakerAssetFillAmount = new BigNumber(0); - return expectTransactionFailedAsync( - erc721TakerBalanceThresholdWrapper.marketBuyOrdersAsync(orders, validTakerAddress, { - makerAssetFillAmount: dummyMakerAssetFillAmount, - }), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, - ); + const tx = erc721TakerBalanceThresholdWrapper.marketBuyOrdersAsync(orders, validTakerAddress, { + makerAssetFillAmount: dummyMakerAssetFillAmount, + }); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); it('should revert if taker does not meet the balance threshold', async () => { const orders = [validSignedOrder, validSignedOrder2]; const dummyMakerAssetFillAmount = new BigNumber(0); - return expectTransactionFailedAsync( - erc721NonValidBalanceThresholdWrapper.marketBuyOrdersAsync(orders, invalidAddress, { - makerAssetFillAmount: dummyMakerAssetFillAmount, - }), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, - ); + const tx = erc721NonValidBalanceThresholdWrapper.marketBuyOrdersAsync(orders, invalidAddress, { + makerAssetFillAmount: dummyMakerAssetFillAmount, + }); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); }); @@ -1251,22 +1214,18 @@ describe(ContractName.BalanceThresholdFilter, () => { const orders = [validSignedOrder, signedOrderWithBadMakerAddress]; // Execute transaction const dummyMakerAssetFillAmount = new BigNumber(0); - return expectTransactionFailedAsync( - erc721TakerBalanceThresholdWrapper.marketBuyOrdersNoThrowAsync(orders, validTakerAddress, { - makerAssetFillAmount: dummyMakerAssetFillAmount, - }), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, - ); + const tx = erc721TakerBalanceThresholdWrapper.marketBuyOrdersNoThrowAsync(orders, validTakerAddress, { + makerAssetFillAmount: dummyMakerAssetFillAmount, + }); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); it('should revert if taker does not meet the balance threshold', async () => { const orders = [validSignedOrder, validSignedOrder2]; const dummyMakerAssetFillAmount = new BigNumber(0); - return expectTransactionFailedAsync( - erc721NonValidBalanceThresholdWrapper.marketBuyOrdersNoThrowAsync(orders, invalidAddress, { - makerAssetFillAmount: dummyMakerAssetFillAmount, - }), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, - ); + const tx = erc721NonValidBalanceThresholdWrapper.marketBuyOrdersNoThrowAsync(orders, invalidAddress, { + makerAssetFillAmount: dummyMakerAssetFillAmount, + }); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); }); @@ -1409,14 +1368,12 @@ describe(ContractName.BalanceThresholdFilter, () => { makerAddress: invalidAddress, }); // Execute transaction - return expectTransactionFailedAsync( - erc721TakerBalanceThresholdWrapper.matchOrdersAsync( - validSignedOrder, - signedOrderWithBadMakerAddress, - validTakerAddress, - ), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, + const tx = erc721TakerBalanceThresholdWrapper.matchOrdersAsync( + validSignedOrder, + signedOrderWithBadMakerAddress, + validTakerAddress, ); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); it('should revert if right maker does not meet the balance threshold', async () => { // Create signed order with non-valid maker address @@ -1425,24 +1382,20 @@ describe(ContractName.BalanceThresholdFilter, () => { makerAddress: invalidAddress, }); // Execute transaction - return expectTransactionFailedAsync( - erc721TakerBalanceThresholdWrapper.matchOrdersAsync( - signedOrderWithBadMakerAddress, - validSignedOrder, - validTakerAddress, - ), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, + const tx = erc721TakerBalanceThresholdWrapper.matchOrdersAsync( + signedOrderWithBadMakerAddress, + validSignedOrder, + validTakerAddress, ); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); it('should revert if taker does not meet the balance threshold', async () => { - return expectTransactionFailedAsync( - erc721NonValidBalanceThresholdWrapper.matchOrdersAsync( - validSignedOrder, - validSignedOrder, - invalidAddress, - ), - RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold, + const tx = erc721NonValidBalanceThresholdWrapper.matchOrdersAsync( + validSignedOrder, + validSignedOrder, + invalidAddress, ); + return expect(tx).to.revertWith(RevertReason.AtLeastOneAddressDoesNotMeetBalanceThreshold); }); }); diff --git a/contracts/extensions/test/dutch_auction.ts b/contracts/extensions/test/dutch_auction.ts index 488fde40b9..c0fa437abb 100644 --- a/contracts/extensions/test/dutch_auction.ts +++ b/contracts/extensions/test/dutch_auction.ts @@ -8,7 +8,6 @@ import { constants, ContractName, ERC20BalancesByOwner, - expectTransactionFailedAsync, getLatestBlockTimestampAsync, OrderFactory, provider, @@ -291,28 +290,22 @@ describe(ContractName.DutchAuction, () => { expirationTimeSeconds: auctionEndTimeSeconds, makerAssetData, }); - return expectTransactionFailedAsync( - dutchAuctionTestWrapper.matchOrdersAsync(buyOrder, sellOrder, takerAddress), - RevertReason.AuctionExpired, - ); + const tx = dutchAuctionTestWrapper.matchOrdersAsync(buyOrder, sellOrder, takerAddress); + return expect(tx).to.revertWith(RevertReason.AuctionExpired); }); it('cannot be filled for less than the current price', async () => { buyOrder = await buyerOrderFactory.newSignedOrderAsync({ makerAssetAmount: sellOrder.takerAssetAmount, }); - return expectTransactionFailedAsync( - dutchAuctionTestWrapper.matchOrdersAsync(buyOrder, sellOrder, takerAddress), - RevertReason.AuctionInvalidAmount, - ); + const tx = dutchAuctionTestWrapper.matchOrdersAsync(buyOrder, sellOrder, takerAddress); + return expect(tx).to.revertWith(RevertReason.AuctionInvalidAmount); }); it('auction begin amount must be higher than final amount ', async () => { sellOrder = await sellerOrderFactory.newSignedOrderAsync({ takerAssetAmount: auctionBeginAmount.plus(1), }); - return expectTransactionFailedAsync( - dutchAuctionTestWrapper.matchOrdersAsync(buyOrder, sellOrder, takerAddress), - RevertReason.AuctionInvalidAmount, - ); + const tx = dutchAuctionTestWrapper.matchOrdersAsync(buyOrder, sellOrder, takerAddress); + return expect(tx).to.revertWith(RevertReason.AuctionInvalidAmount); }); it('begin time is less than end time', async () => { auctionBeginTimeSeconds = new BigNumber(auctionEndTimeSeconds).plus(tenMinutesInSeconds); @@ -325,19 +318,15 @@ describe(ContractName.DutchAuction, () => { expirationTimeSeconds: auctionEndTimeSeconds, makerAssetData, }); - return expectTransactionFailedAsync( - dutchAuctionTestWrapper.matchOrdersAsync(buyOrder, sellOrder, takerAddress), - RevertReason.AuctionInvalidBeginTime, - ); + const tx = dutchAuctionTestWrapper.matchOrdersAsync(buyOrder, sellOrder, takerAddress); + return expect(tx).to.revertWith(RevertReason.AuctionInvalidBeginTime); }); it('asset data contains auction parameters', async () => { sellOrder = await sellerOrderFactory.newSignedOrderAsync({ makerAssetData: assetDataUtils.encodeERC20AssetData(defaultMakerAssetAddress), }); - return expectTransactionFailedAsync( - dutchAuctionTestWrapper.matchOrdersAsync(buyOrder, sellOrder, takerAddress), - RevertReason.InvalidAssetData, - ); + const tx = dutchAuctionTestWrapper.matchOrdersAsync(buyOrder, sellOrder, takerAddress); + return expect(tx).to.revertWith(RevertReason.InvalidAssetData); }); describe('ERC721', () => { diff --git a/contracts/extensions/test/order_matcher.ts b/contracts/extensions/test/order_matcher.ts index c1f727b3d8..e6c9dc0e6c 100644 --- a/contracts/extensions/test/order_matcher.ts +++ b/contracts/extensions/test/order_matcher.ts @@ -12,7 +12,6 @@ import { constants, ERC20BalancesByOwner, expectContractCreationFailedAsync, - expectTransactionFailedAsync, LogDecoder, OrderFactory, provider, @@ -21,7 +20,7 @@ import { web3Wrapper, } from '@0x/contracts-test-utils'; import { BlockchainLifecycle } from '@0x/dev-utils'; -import { assetDataUtils } from '@0x/order-utils'; +import { assetDataUtils, ExchangeRevertErrors } from '@0x/order-utils'; import { RevertReason } from '@0x/types'; import { BigNumber, providerUtils } from '@0x/utils'; import { Web3Wrapper } from '@0x/web3-wrapper'; @@ -241,15 +240,13 @@ describe('OrderMatcher', () => { signedOrderLeft.signature, signedOrderRight.signature, ); - await expectTransactionFailedAsync( - web3Wrapper.sendTransactionAsync({ - data, - to: orderMatcher.address, - from: takerAddress, - gas: constants.MAX_MATCH_ORDERS_GAS, - }), - RevertReason.OnlyContractOwner, - ); + const tx = web3Wrapper.sendTransactionAsync({ + data, + to: orderMatcher.address, + from: takerAddress, + gas: constants.MAX_MATCH_ORDERS_GAS, + }); + return expect(tx).to.revertWith(RevertReason.OnlyContractOwner); }); it('should transfer the correct amounts when orders completely fill each other', async () => { // Create orders to match @@ -676,15 +673,14 @@ describe('OrderMatcher', () => { signedOrderLeft.signature, signedOrderRight.signature, ); - await expectTransactionFailedAsync( - web3Wrapper.sendTransactionAsync({ - data, - to: orderMatcher.address, - from: owner, - gas: constants.MAX_MATCH_ORDERS_GAS, - }), - RevertReason.InvalidOrderSignature, - ); + const expectedError = new ExchangeRevertErrors.SignatureError(); + const tx = web3Wrapper.sendTransactionAsync({ + data, + to: orderMatcher.address, + from: owner, + gas: constants.MAX_MATCH_ORDERS_GAS, + }); + return expect(tx).to.revertWith(expectedError); }); it('should revert with the correct reason if fillOrder call reverts', async () => { // Create orders to match @@ -709,15 +705,14 @@ describe('OrderMatcher', () => { signedOrderLeft.signature, signedOrderRight.signature, ); - await expectTransactionFailedAsync( - web3Wrapper.sendTransactionAsync({ - data, - to: orderMatcher.address, - from: owner, - gas: constants.MAX_MATCH_ORDERS_GAS, - }), - RevertReason.TransferFailed, - ); + const expectedError = new ExchangeRevertErrors.AssetProxyTransferError(); + const tx = web3Wrapper.sendTransactionAsync({ + data, + to: orderMatcher.address, + from: owner, + gas: constants.MAX_MATCH_ORDERS_GAS, + }); + return expect(tx).to.revertWith(expectedError); }); }); describe('withdrawAsset', () => { @@ -757,12 +752,10 @@ describe('OrderMatcher', () => { it('should revert if not called by owner', async () => { const erc20AWithdrawAmount = await erc20TokenA.balanceOf.callAsync(orderMatcher.address); expect(erc20AWithdrawAmount).to.be.bignumber.gt(constants.ZERO_AMOUNT); - await expectTransactionFailedAsync( - orderMatcher.withdrawAsset.sendTransactionAsync(leftMakerAssetData, erc20AWithdrawAmount, { - from: takerAddress, - }), - RevertReason.OnlyContractOwner, - ); + const tx = orderMatcher.withdrawAsset.sendTransactionAsync(leftMakerAssetData, erc20AWithdrawAmount, { + from: takerAddress, + }); + return expect(tx).to.revertWith(RevertReason.OnlyContractOwner); }); }); describe('approveAssetProxy', () => { @@ -813,12 +806,10 @@ describe('OrderMatcher', () => { }); it('should revert if not called by owner', async () => { const approval = new BigNumber(1); - await expectTransactionFailedAsync( - orderMatcher.approveAssetProxy.sendTransactionAsync(leftMakerAssetData, approval, { - from: takerAddress, - }), - RevertReason.OnlyContractOwner, - ); + const tx = orderMatcher.approveAssetProxy.sendTransactionAsync(leftMakerAssetData, approval, { + from: takerAddress, + }); + return expect(tx).to.revertWith(RevertReason.OnlyContractOwner); }); }); }); diff --git a/contracts/test-utils/CHANGELOG.json b/contracts/test-utils/CHANGELOG.json index 5a8ccc69d2..b1ad55570c 100644 --- a/contracts/test-utils/CHANGELOG.json +++ b/contracts/test-utils/CHANGELOG.json @@ -9,6 +9,18 @@ { "note": "Use new `Order` structure with `domain` field", "pr": 1742 + }, + { + "note": "Inherit `chaiSetup` from `@0x/dev-utils`", + "pr": 1761 + }, + { + "note": "Add `generatePseudoRandomOrderHash()` to `orderUtils`", + "pr": 1761 + }, + { + "note": "Inherit `OrderStatus` from `@0x/types`", + "pr": 1761 } ] }, diff --git a/contracts/test-utils/package.json b/contracts/test-utils/package.json index c79e2f20ee..fbf8a3feaa 100644 --- a/contracts/test-utils/package.json +++ b/contracts/test-utils/package.json @@ -59,9 +59,6 @@ "@types/node": "*", "bn.js": "^4.11.8", "chai": "^4.0.1", - "chai-as-promised": "^7.1.0", - "chai-bignumber": "^3.0.0", - "dirty-chai": "^2.0.1", "ethereum-types": "^2.1.1", "ethereumjs-util": "^5.1.1", "ethers": "~4.0.4", diff --git a/contracts/test-utils/src/chai_setup.ts b/contracts/test-utils/src/chai_setup.ts index 1a87330932..e721397b28 100644 --- a/contracts/test-utils/src/chai_setup.ts +++ b/contracts/test-utils/src/chai_setup.ts @@ -1,13 +1 @@ -import * as chai from 'chai'; -import chaiAsPromised = require('chai-as-promised'); -import ChaiBigNumber = require('chai-bignumber'); -import * as dirtyChai from 'dirty-chai'; - -export const chaiSetup = { - configure(): void { - chai.config.includeStack = true; - chai.use(ChaiBigNumber()); - chai.use(dirtyChai); - chai.use(chaiAsPromised); - }, -}; +export { chaiSetup } from '@0x/dev-utils'; diff --git a/contracts/test-utils/src/order_utils.ts b/contracts/test-utils/src/order_utils.ts index 15d9dafff4..7272b6f2d0 100644 --- a/contracts/test-utils/src/order_utils.ts +++ b/contracts/test-utils/src/order_utils.ts @@ -1,3 +1,5 @@ +import { generatePseudoRandomSalt } from '@0x/order-utils'; +import { crypto } from '@0x/order-utils/lib/src/crypto'; import { OrderWithoutDomain, SignedOrder } from '@0x/types'; import { BigNumber } from '@0x/utils'; import * as _ from 'lodash'; @@ -42,4 +44,10 @@ export const orderUtils = { fill.right.takerAssetData = constants.NULL_BYTES; return fill; }, + generatePseudoRandomOrderHash(): string { + const randomBigNum = generatePseudoRandomSalt(); + const randomBuff = crypto.solSHA3([randomBigNum]); + const randomHash = `0x${randomBuff.toString('hex')}`; + return randomHash; + }, }; diff --git a/contracts/test-utils/src/types.ts b/contracts/test-utils/src/types.ts index 8ba632cf33..acdcea433d 100644 --- a/contracts/test-utils/src/types.ts +++ b/contracts/test-utils/src/types.ts @@ -2,6 +2,8 @@ import { OrderWithoutDomain } from '@0x/types'; import { BigNumber } from '@0x/utils'; import { AbiDefinition } from 'ethereum-types'; +export { OrderStatus } from '@0x/types'; + export interface ERC20BalancesByOwner { [ownerAddress: string]: { [tokenAddress: string]: BigNumber; @@ -90,16 +92,6 @@ export interface Token { swarmHash: string; } -export enum OrderStatus { - Invalid, - InvalidMakerAssetAmount, - InvalidTakerAssetAmount, - Fillable, - Expired, - FullyFilled, - Cancelled, -} - export enum ContractName { TokenRegistry = 'TokenRegistry', MultiSigWalletWithTimeLock = 'MultiSigWalletWithTimeLock', diff --git a/contracts/utils/CHANGELOG.json b/contracts/utils/CHANGELOG.json index 1eae4ad541..8ff70684e7 100644 --- a/contracts/utils/CHANGELOG.json +++ b/contracts/utils/CHANGELOG.json @@ -9,6 +9,10 @@ { "note": "Add LibEIP712 contract", "pr": 1753 + }, + { + "note": "Add `RichErrors` and `mixins/MRichErrors`", + "pr": 1761 } ] }, diff --git a/contracts/utils/compiler.json b/contracts/utils/compiler.json index 6dec4be463..642872c89c 100644 --- a/contracts/utils/compiler.json +++ b/contracts/utils/compiler.json @@ -28,6 +28,7 @@ "src/LibEIP712.sol", "src/Ownable.sol", "src/ReentrancyGuard.sol", + "src/RichErrors.sol", "src/SafeMath.sol", "src/interfaces/IOwnable.sol", "test/TestConstants.sol", diff --git a/contracts/utils/contracts/src/RichErrors.sol b/contracts/utils/contracts/src/RichErrors.sol new file mode 100644 index 0000000000..45730fa6d7 --- /dev/null +++ b/contracts/utils/contracts/src/RichErrors.sol @@ -0,0 +1,57 @@ +/* + + Copyright 2019 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.5.5; + +import "./mixins/MRichErrors.sol"; + + +contract RichErrors is + MRichErrors +{ + // solhint-disable func-name-mixedcase + /// @dev ABI encode a standard, string revert error payload. + /// This is the same payload that would be included by a `revert(string)` + /// solidity statement. It has the function signature `Error(string)`. + /// @param message The error string. + /// @return The ABI encoded error. + function StandardError( + string memory message + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + STANDARD_ERROR_SELECTOR, + bytes(message) + ); + } + // solhint-enable func-name-mixedcase + + /// @dev Reverts an encoded rich revert reason `errorData`. + /// @param errorData ABI encoded error data. + function rrevert(bytes memory errorData) + internal + pure + { + assembly { + revert(add(errorData, 0x20), mload(errorData)) + } + } +} diff --git a/contracts/utils/contracts/src/mixins/MRichErrors.sol b/contracts/utils/contracts/src/mixins/MRichErrors.sol new file mode 100644 index 0000000000..e4f00d443e --- /dev/null +++ b/contracts/utils/contracts/src/mixins/MRichErrors.sol @@ -0,0 +1,40 @@ +/* + + Copyright 2019 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.5.5; + + +contract MRichErrors { + // solhint-disable func-name-mixedcase + + bytes4 internal constant STANDARD_ERROR_SELECTOR = + bytes4(keccak256("Error(string)")); + + function StandardError( + string memory message + ) + internal + pure + returns (bytes memory); + + /// @dev Reverts an encoded rich revert reason `errorData`. + /// @param errorData ABI encoded error data. + function rrevert(bytes memory errorData) + internal + pure; +} diff --git a/contracts/utils/package.json b/contracts/utils/package.json index 027c651a08..9dc2d84f45 100644 --- a/contracts/utils/package.json +++ b/contracts/utils/package.json @@ -33,7 +33,7 @@ "lint-contracts": "solhint -c ../.solhint.json contracts/**/**/**/**/*.sol" }, "config": { - "abis": "./generated-artifacts/@(Address|IOwnable|LibBytes|LibEIP712|Ownable|ReentrancyGuard|SafeMath|TestConstants|TestLibAddressArray|TestLibBytes).json", + "abis": "./generated-artifacts/@(Address|IOwnable|LibBytes|LibEIP712|Ownable|ReentrancyGuard|RichErrors|SafeMath|TestConstants|TestLibAddressArray|TestLibBytes).json", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually." }, "repository": { diff --git a/contracts/utils/src/artifacts.ts b/contracts/utils/src/artifacts.ts index e7650e53e4..9f1008b75a 100644 --- a/contracts/utils/src/artifacts.ts +++ b/contracts/utils/src/artifacts.ts @@ -11,6 +11,7 @@ import * as LibBytes from '../generated-artifacts/LibBytes.json'; import * as LibEIP712 from '../generated-artifacts/LibEIP712.json'; import * as Ownable from '../generated-artifacts/Ownable.json'; import * as ReentrancyGuard from '../generated-artifacts/ReentrancyGuard.json'; +import * as RichErrors from '../generated-artifacts/RichErrors.json'; import * as SafeMath from '../generated-artifacts/SafeMath.json'; import * as TestConstants from '../generated-artifacts/TestConstants.json'; import * as TestLibAddressArray from '../generated-artifacts/TestLibAddressArray.json'; @@ -22,6 +23,7 @@ export const artifacts = { ReentrancyGuard: ReentrancyGuard as ContractArtifact, SafeMath: SafeMath as ContractArtifact, LibEIP712: LibEIP712 as ContractArtifact, + RichErrors: RichErrors as ContractArtifact, IOwnable: IOwnable as ContractArtifact, TestConstants: TestConstants as ContractArtifact, TestLibAddressArray: TestLibAddressArray as ContractArtifact, diff --git a/contracts/utils/src/wrappers.ts b/contracts/utils/src/wrappers.ts index a5b4954ad3..b97fc95c2c 100644 --- a/contracts/utils/src/wrappers.ts +++ b/contracts/utils/src/wrappers.ts @@ -9,6 +9,7 @@ export * from '../generated-wrappers/lib_bytes'; export * from '../generated-wrappers/lib_e_i_p712'; export * from '../generated-wrappers/ownable'; export * from '../generated-wrappers/reentrancy_guard'; +export * from '../generated-wrappers/rich_errors'; export * from '../generated-wrappers/safe_math'; export * from '../generated-wrappers/test_constants'; export * from '../generated-wrappers/test_lib_address_array'; diff --git a/contracts/utils/tsconfig.json b/contracts/utils/tsconfig.json index d4f0363c91..c37381d5fa 100644 --- a/contracts/utils/tsconfig.json +++ b/contracts/utils/tsconfig.json @@ -9,6 +9,7 @@ "generated-artifacts/LibEIP712.json", "generated-artifacts/Ownable.json", "generated-artifacts/ReentrancyGuard.json", + "generated-artifacts/RichErrors.json", "generated-artifacts/SafeMath.json", "generated-artifacts/TestConstants.json", "generated-artifacts/TestLibAddressArray.json", diff --git a/packages/base-contract/CHANGELOG.json b/packages/base-contract/CHANGELOG.json index cdebb7a06a..d1a6828bfb 100644 --- a/packages/base-contract/CHANGELOG.json +++ b/packages/base-contract/CHANGELOG.json @@ -1,4 +1,17 @@ [ + { + "version": "5.1.0", + "changes": [ + { + "note": "Automatically decode and throw rich reverts in `_throwIfRevertWithReasonCallResult`", + "pr": 1761 + }, + { + "note": "Remove dependency on ether.js", + "pr": 1761 + } + ] + }, { "timestamp": 1553183790, "version": "5.0.4", diff --git a/packages/base-contract/package.json b/packages/base-contract/package.json index 9d54c8faec..48ef00f720 100644 --- a/packages/base-contract/package.json +++ b/packages/base-contract/package.json @@ -44,7 +44,6 @@ "@0x/utils": "^4.3.0", "@0x/web3-wrapper": "^6.0.4", "ethereum-types": "^2.1.1", - "ethers": "~4.0.4", "lodash": "^4.17.11" }, "publishConfig": { diff --git a/packages/base-contract/src/index.ts b/packages/base-contract/src/index.ts index a8bdaa4078..7133e86796 100644 --- a/packages/base-contract/src/index.ts +++ b/packages/base-contract/src/index.ts @@ -1,4 +1,4 @@ -import { AbiEncoder, abiUtils, BigNumber } from '@0x/utils'; +import { AbiEncoder, abiUtils, BigNumber, RevertError } from '@0x/utils'; import { Web3Wrapper } from '@0x/web3-wrapper'; import { AbiDefinition, @@ -11,7 +11,6 @@ import { TxData, TxDataPayable, } from 'ethereum-types'; -import * as ethers from 'ethers'; import * as _ from 'lodash'; import { formatABIDataItem } from './utils'; @@ -20,11 +19,6 @@ export interface AbiEncoderByFunctionSignature { [key: string]: AbiEncoder.Method; } -const REVERT_ERROR_SELECTOR = '08c379a0'; -const REVERT_ERROR_SELECTOR_OFFSET = 2; -const REVERT_ERROR_SELECTOR_BYTES_LENGTH = 4; -const REVERT_ERROR_SELECTOR_END = REVERT_ERROR_SELECTOR_OFFSET + REVERT_ERROR_SELECTOR_BYTES_LENGTH * 2; - export class BaseContract { protected _abiEncoderByFunctionSignature: AbiEncoderByFunctionSignature; protected _web3Wrapper: Web3Wrapper; @@ -85,18 +79,15 @@ export class BaseContract { return txDataWithDefaults; } protected static _throwIfRevertWithReasonCallResult(rawCallResult: string): void { - if (rawCallResult.slice(REVERT_ERROR_SELECTOR_OFFSET, REVERT_ERROR_SELECTOR_END) === REVERT_ERROR_SELECTOR) { - const revertReasonArray = AbiEncoder.create('(string)').decodeAsArray( - ethers.utils.hexDataSlice(rawCallResult, REVERT_ERROR_SELECTOR_BYTES_LENGTH), - ); - if (revertReasonArray.length !== 1) { - throw new Error( - `Cannot safely decode revert reason: Expected an array with one element, got ${revertReasonArray}`, - ); - } - const revertReason = revertReasonArray[0]; - throw new Error(revertReason); + // Try to decode the call result as a revert error. + let revert: RevertError; + try { + revert = RevertError.decode(rawCallResult); + } catch (err) { + // Can't decode it as a revert error, so assume it didn't revert. + return; } + throw revert; } // Throws if the given arguments cannot be safely/correctly encoded based on // the given inputAbi. An argument may not be considered safely encodeable diff --git a/packages/contract-wrappers/CHANGELOG.json b/packages/contract-wrappers/CHANGELOG.json index f7825156c8..16a3a17054 100644 --- a/packages/contract-wrappers/CHANGELOG.json +++ b/packages/contract-wrappers/CHANGELOG.json @@ -1,4 +1,13 @@ [ + { + "version": "8.1.1", + "changes": [ + { + "note": "Update tests for rich reverts", + "pr": 1761 + } + ] + }, { "version": "8.1.0", "changes": [ diff --git a/packages/contract-wrappers/package.json b/packages/contract-wrappers/package.json index 88ceb51658..334dbfe884 100644 --- a/packages/contract-wrappers/package.json +++ b/packages/contract-wrappers/package.json @@ -71,13 +71,13 @@ "@0x/assert": "^2.0.8", "@0x/contract-addresses": "^2.3.0", "@0x/contract-artifacts": "^1.4.0", + "ethereum-types": "^2.1.1", "@0x/json-schemas": "^3.0.8", "@0x/order-utils": "^7.1.1", "@0x/types": "^2.2.1", "@0x/typescript-typings": "^4.2.1", "@0x/utils": "^4.3.0", "@0x/web3-wrapper": "^6.0.4", - "ethereum-types": "^2.1.1", "ethereumjs-abi": "0.6.5", "ethereumjs-blockstream": "6.0.0", "ethereumjs-util": "^5.1.1", diff --git a/packages/contract-wrappers/test/revert_validation_test.ts b/packages/contract-wrappers/test/revert_validation_test.ts index efd5dd61f1..2660e33dc0 100644 --- a/packages/contract-wrappers/test/revert_validation_test.ts +++ b/packages/contract-wrappers/test/revert_validation_test.ts @@ -2,7 +2,7 @@ import { BlockchainLifecycle, devConstants, web3Factory } from '@0x/dev-utils'; import { FillScenarios } from '@0x/fill-scenarios'; import { runMigrationsAsync } from '@0x/migrations'; import { assetDataUtils } from '@0x/order-utils'; -import { SignedOrder } from '@0x/types'; +import { RevertReason, SignedOrder } from '@0x/types'; import { BigNumber } from '@0x/utils'; import { Web3Wrapper } from '@0x/web3-wrapper'; import * as chai from 'chai'; @@ -105,11 +105,10 @@ describe('Revert Validation ExchangeWrapper', () => { makerTokenBalance, ); await web3Wrapper.awaitTransactionSuccessAsync(txHash, constants.AWAIT_TRANSACTION_MINED_MS); - return expect( - contractWrappers.exchange.fillOrderAsync(signedOrder, takerTokenFillAmount, takerAddress, { - shouldValidate: true, - }), - ).to.be.rejectedWith('TRANSFER_FAILED'); + const tx = contractWrappers.exchange.fillOrderAsync(signedOrder, takerTokenFillAmount, takerAddress, { + shouldValidate: true, + }); + return expect(tx).to.revertWith(RevertReason.TransferFailed); }); }); }); diff --git a/packages/contract-wrappers/test/utils/chai_setup.ts b/packages/contract-wrappers/test/utils/chai_setup.ts index 1a87330932..e721397b28 100644 --- a/packages/contract-wrappers/test/utils/chai_setup.ts +++ b/packages/contract-wrappers/test/utils/chai_setup.ts @@ -1,13 +1 @@ -import * as chai from 'chai'; -import chaiAsPromised = require('chai-as-promised'); -import ChaiBigNumber = require('chai-bignumber'); -import * as dirtyChai from 'dirty-chai'; - -export const chaiSetup = { - configure(): void { - chai.config.includeStack = true; - chai.use(ChaiBigNumber()); - chai.use(dirtyChai); - chai.use(chaiAsPromised); - }, -}; +export { chaiSetup } from '@0x/dev-utils'; diff --git a/packages/dev-utils/CHANGELOG.json b/packages/dev-utils/CHANGELOG.json index e6ec03c134..0b8f9fd9c4 100644 --- a/packages/dev-utils/CHANGELOG.json +++ b/packages/dev-utils/CHANGELOG.json @@ -1,4 +1,13 @@ [ + { + "version": "2.3.0", + "changes": [ + { + "note": "Add `chaiSetup` function with `RevertError` testing support", + "pr": 1761 + } + ] + }, { "version": "2.2.0", "changes": [ diff --git a/packages/dev-utils/package.json b/packages/dev-utils/package.json index b61ab4b58c..a3d58dab28 100644 --- a/packages/dev-utils/package.json +++ b/packages/dev-utils/package.json @@ -48,6 +48,9 @@ "@0x/web3-wrapper": "^6.0.4", "@types/web3-provider-engine": "^14.0.0", "chai": "^4.0.1", + "chai-as-promised": "^7.1.0", + "chai-bignumber": "^3.0.0", + "dirty-chai": "^2.0.1", "ethereum-types": "^2.1.1", "lodash": "^4.17.11" }, diff --git a/packages/dev-utils/src/chai_revert_error.ts b/packages/dev-utils/src/chai_revert_error.ts new file mode 100644 index 0000000000..5b870e57cd --- /dev/null +++ b/packages/dev-utils/src/chai_revert_error.ts @@ -0,0 +1,198 @@ +import { AnyRevertError, RevertError, StringRevertError } from '@0x/utils'; + +// tslint:disable only-arrow-functions prefer-conditional-expression + +type ChaiPromiseHandler = (x: any, ...rest: any[]) => Promise; +type ChaiAssertHandler = (x: any, ...rest: any[]) => void; + +interface Chai { + Assertion: ChaiAssertionType; +} + +interface ChaiAssertionInstance { + assert: ChaiAssert; + _obj: any; + __flags: any; +} + +interface ChaiAssertionType { + overwriteMethod: (name: string, _super: (expected: any) => any) => void; + new (): ChaiAssertionInstance; +} + +type ChaiAssert = ( + condition: boolean, + failMessage?: string, + negatedFailMessage?: string, + expected?: any, + actual?: any, +) => void; + +interface GanacheTransactionRevertResult { + error: 'revert'; + program_counter: number; + return?: string; + reason?: string; +} + +interface GanacheTransactionRevertError extends Error { + results: { [hash: string]: GanacheTransactionRevertResult }; + hashes: string[]; +} + +export function revertErrorHelper(_chai: Chai): void { + const proto = _chai.Assertion; + proto.overwriteMethod('revertWith', function(_super: ChaiPromiseHandler): ChaiPromiseHandler { + return async function(this: ChaiAssertionInstance, expected: any, ...rest: any[]): Promise { + const maybePromise = this._obj; + // Make sure we're working with a promise. + chaiAssert(_chai, maybePromise instanceof Promise, `Expected ${maybePromise} to be a promise`); + // Wait for the promise to reject. + let resolveValue; + let rejectValue: any; + let didReject = false; + try { + resolveValue = await maybePromise; + } catch (err) { + rejectValue = err; + didReject = true; + } + if (!didReject) { + chaiFail(_chai, `Expected promise to reject but instead resolved with: ${resolveValue}`); + } + if (!compareRevertErrors.call(this, _chai, rejectValue, expected, true)) { + // Wasn't handled by the comparison function so call the previous handler. + _super.call(this, expected, ...rest); + } + }; + }); + proto.overwriteMethod('become', function(_super: ChaiPromiseHandler): ChaiPromiseHandler { + return async function(this: ChaiAssertionInstance, expected: any, ...rest: any[]): Promise { + const maybePromise = this._obj; + // Make sure we're working with a promise. + chaiAssert(_chai, maybePromise instanceof Promise, `Expected ${maybePromise} to be a promise`); + // Wait for the promise to resolve. + if (!compareRevertErrors.call(this, _chai, await maybePromise, expected)) { + // Wasn't handled by the comparison function so call the previous handler. + _super.call(this, expected, ...rest); + } + }; + }); + proto.overwriteMethod('equal', function(_super: ChaiAssertHandler): ChaiAssertHandler { + return function(this: ChaiAssertionInstance, expected: any, ...rest: any[]): void { + if (!compareRevertErrors.call(this, _chai, this._obj, expected)) { + // Wasn't handled by the comparison function so call the previous handler. + _super.call(this, expected, ...rest); + } + }; + }); +} + +/** + * Compare two values as compatible RevertError types. + * @return `true` if the comparison was fully evaluated. `false` indicates that + * it should be deferred to another handler. + */ +function compareRevertErrors( + this: ChaiAssertionInstance, + _chai: Chai, + _actual: any, + _expected: any, + force?: boolean, +): boolean { + let actual = _actual; + let expected = _expected; + // If either subject is a RevertError, or the `force` is `true`, + // try to coerce the subjects into a RevertError. + // Some of this is for convenience, some is for backwards-compatibility. + if (force || expected instanceof RevertError || actual instanceof RevertError) { + // `actual` can be a RevertError, string, or an Error type. + if (!(actual instanceof RevertError)) { + if (typeof actual === 'string') { + actual = new StringRevertError(actual); + } else if (actual instanceof Error) { + actual = coerceErrorToRevertError(actual); + } else { + chaiAssert(_chai, false, `Result is not of type RevertError: ${actual}`); + } + } + // `expected` can be a RevertError or string. + if (typeof expected === 'string') { + expected = new StringRevertError(expected); + } + } + if (expected instanceof RevertError && actual instanceof RevertError) { + // Check for equality. + this.assert( + expected.equals(actual), + `${actual.toString()} != ${expected.toString()}`, + `${actual.toString()} == ${expected.toString()}`, + expected.toString(), + actual.toString(), + ); + // Return true to signal we handled it. + return true; + } + return false; +} + +const GANACHE_TRANSACTION_REVERT_ERROR_MESSAGE = /^VM Exception while processing transaction: revert/; +const GETH_TRANSACTION_REVERT_ERROR_MESSAGE = /always failing transaction$/; + +function coerceErrorToRevertError(error: Error | GanacheTransactionRevertError): RevertError { + // Handle ganache transaction reverts. + if (isGanacheTransactionRevertError(error)) { + // Grab the first result attached. + const result = error.results[error.hashes[0]]; + // If a reason is provided, just wrap it in a StringRevertError + if (result.reason !== undefined) { + return new StringRevertError(result.reason); + } + // If there is return data, try to decode it. + if (result.return !== undefined && result.return !== '0x') { + return RevertError.decode(result.return); + } + // Otherwise, return an AnyRevertError type. + return new AnyRevertError(); + } + + // Handle geth transaction reverts. + if (isGethTransactionRevertError(error)) { + // Geth transaction reverts are opaque, meaning no useful data is returned, + // so we just return an AnyRevertError type. + return new AnyRevertError(); + } + + // Handle call reverts. + // `BaseContract` will throw a plain `Error` type for `StringRevertErrors` + // in callAsync functions for backwards compatibility, and a proper RevertError + // for all others. + if (error instanceof RevertError) { + return error; + } + // Coerce plain errors into a StringRevertError. + return new StringRevertError(error.message); +} + +function isGanacheTransactionRevertError( + error: Error | GanacheTransactionRevertError, +): error is GanacheTransactionRevertError { + if (GANACHE_TRANSACTION_REVERT_ERROR_MESSAGE.test(error.message) && 'hashes' in error && 'results' in error) { + return true; + } + return false; +} + +function isGethTransactionRevertError(error: Error | GanacheTransactionRevertError): boolean { + return GETH_TRANSACTION_REVERT_ERROR_MESSAGE.test(error.message); +} + +function chaiAssert(_chai: Chai, condition: boolean, failMessage?: string, expected?: any, actual?: any): void { + const assert = new _chai.Assertion(); + assert.assert(condition, failMessage, undefined, expected, actual); +} + +function chaiFail(_chai: Chai, failMessage?: string, expected?: any, actual?: any): void { + const assert = new _chai.Assertion(); + assert.assert(false, failMessage, undefined, expected, actual); +} diff --git a/packages/dev-utils/src/chai_setup.ts b/packages/dev-utils/src/chai_setup.ts new file mode 100644 index 0000000000..71d8649dfd --- /dev/null +++ b/packages/dev-utils/src/chai_setup.ts @@ -0,0 +1,17 @@ +import * as chai from 'chai'; +import chaiAsPromised = require('chai-as-promised'); +import ChaiBigNumber = require('chai-bignumber'); +import * as dirtyChai from 'dirty-chai'; + +import { revertErrorHelper } from './chai_revert_error'; + +export const chaiSetup = { + configure(): void { + chai.config.includeStack = true; + // Order matters. + chai.use(ChaiBigNumber()); + chai.use(chaiAsPromised); + chai.use(revertErrorHelper); + chai.use(dirtyChai); + }, +}; diff --git a/packages/dev-utils/src/index.ts b/packages/dev-utils/src/index.ts index d4c19f1bf2..3872bc5a30 100644 --- a/packages/dev-utils/src/index.ts +++ b/packages/dev-utils/src/index.ts @@ -3,3 +3,4 @@ export { web3Factory } from './web3_factory'; export { constants as devConstants } from './constants'; export { env, EnvVars } from './env'; export { callbackErrorReporter } from './callback_error_reporter'; +export { chaiSetup } from './chai_setup'; diff --git a/packages/dev-utils/test/chai_test.ts b/packages/dev-utils/test/chai_test.ts new file mode 100644 index 0000000000..aa8f6cebf4 --- /dev/null +++ b/packages/dev-utils/test/chai_test.ts @@ -0,0 +1,200 @@ +import { StringRevertError } from '@0x/utils'; +import * as chai from 'chai'; + +import { chaiSetup } from '../src'; + +chaiSetup.configure(); +const expect = chai.expect; + +class DescendantRevertError extends StringRevertError { + constructor(msg: string) { + super(msg); + } +} + +describe('Chai tests', () => { + describe('RevertErrors', () => { + describe('#equal', () => { + it('should equate two identical RevertErrors', () => { + const message = 'foo'; + const revert1 = new StringRevertError(message); + const revert2 = new StringRevertError(message); + expect(revert1).is.equal(revert2); + }); + it('should equate two RevertErrors where one has missing fields', () => { + const revert1 = new StringRevertError('foo'); + const revert2 = new StringRevertError(); + expect(revert1).is.equal(revert2); + }); + it('should not equate two RevertErrors with diferent fields', () => { + const revert1 = new StringRevertError('foo1'); + const revert2 = new StringRevertError('foo2'); + expect(revert1).is.not.equal(revert2); + }); + it('should not equate two RevertErrors with diferent types', () => { + const message = 'foo'; + const revert1 = new StringRevertError(message); + const revert2 = new DescendantRevertError(message); + expect(revert1).is.not.equal(revert2); + }); + it('should equate a StringRevertError to a string equal to message', () => { + const message = 'foo'; + const revert = new StringRevertError(message); + expect(message).is.equal(revert); + }); + it('should equate an Error to a StringRevertError with an equal message', () => { + const message = 'foo'; + const revert = new StringRevertError(message); + const error = new Error(message); + expect(error).is.equal(revert); + }); + it('should equate a ganache transaction revert error with reason to a StringRevertError with an equal message', () => { + const message = 'foo'; + const error: any = new Error(`VM Exception while processing transaction: revert ${message}`); + error.hashes = ['0x1']; + error.results = { '0x1': { error: 'revert', program_counter: 1, return: '0x', reason: message } }; + const revert = new StringRevertError(message); + expect(error).is.equal(revert); + }); + it('should equate a ganache transaction revert error with return data to a StringRevertError with an equal message', () => { + const error: any = new Error(`VM Exception while processing transaction: revert`); + error.hashes = ['0x1']; + // Encoding for `Error(string message='foo')` + const returnData = + '0x08c379a000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000003666f6f0000000000000000000000000000000000000000000000000000000000'; + error.results = { + '0x1': { error: 'revert', program_counter: 1, return: returnData, reason: undefined }, + }; + const revert = new StringRevertError('foo'); + expect(error).is.equal(revert); + }); + it('should equate an empty ganache transaction revert error to any RevertError', () => { + const error: any = new Error(`VM Exception while processing transaction: revert`); + error.hashes = ['0x1']; + error.results = { '0x1': { error: 'revert', program_counter: 1, return: '0x', reason: undefined } }; + const revert = new StringRevertError('foo'); + expect(error).is.equal(revert); + }); + it('should not equate a ganache transaction revert error with reason to a StringRevertError with a different message', () => { + const message = 'foo'; + const error: any = new Error(`VM Exception while processing transaction: revert ${message}`); + error.hashes = ['0x1']; + error.results = { '0x1': { error: 'revert', program_counter: 1, return: '0x', reason: message } }; + const revert = new StringRevertError('boo'); + expect(error).is.not.equal(revert); + }); + it('should not equate a ganache transaction revert error with return data to a StringRevertError with a different message', () => { + const error: any = new Error(`VM Exception while processing transaction: revert`); + error.hashes = ['0x1']; + // Encoding for `Error(string message='foo')` + const returnData = + '0x08c379a000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000003666f6f0000000000000000000000000000000000000000000000000000000000'; + error.results = { + '0x1': { error: 'revert', program_counter: 1, return: returnData, reason: undefined }, + }; + const revert = new StringRevertError('boo'); + expect(error).is.not.equal(revert); + }); + it('should equate an opaque geth transaction revert error to any RevertError', () => { + const error = new Error(`always failing transaction`); + const revert = new StringRevertError('foo'); + expect(error).is.equal(revert); + }); + it('should equate a string to a StringRevertError with the same message', () => { + const message = 'foo'; + const revert = new StringRevertError(message); + expect(revert).is.equal(message); + }); + it('should not equate a StringRevertError to a string not equal to message', () => { + const revert = new StringRevertError('foo1'); + expect('foo2').is.not.equal(revert); + }); + it('should not equate a string to a StringRevertError with a different message', () => { + const revert = new StringRevertError('foo1'); + expect(revert).is.not.equal('foo2'); + }); + it('should not equate an Error to a StringRevertError with a different message', () => { + const revert = new StringRevertError('foo1'); + const error = new Error('foo2'); + expect(error).is.not.equal(revert); + }); + }); + describe('#revertWith', () => { + it('should equate a promise that rejects to identical RevertErrors', async () => { + const message = 'foo'; + const revert1 = new StringRevertError(message); + const revert2 = new StringRevertError(message); + const promise = (async () => { + throw revert1; + })(); + return expect(promise).to.revertWith(revert2); + }); + it('should not equate a promise that rejects to a StringRevertError with a different messages', async () => { + const revert1 = new StringRevertError('foo1'); + const revert2 = new StringRevertError('foo2'); + const promise = (async () => { + throw revert1; + })(); + return expect(promise).to.not.revertWith(revert2); + }); + it('should not equate a promise that rejects to different RevertError types', async () => { + const message = 'foo'; + const revert1 = new StringRevertError(message); + const revert2 = new DescendantRevertError(message); + const promise = (async () => { + throw revert1; + })(); + return expect(promise).to.not.revertWith(revert2); + }); + }); + describe('#become', () => { + it('should equate a promise that resolves to an identical RevertErrors', async () => { + const message = 'foo'; + const revert1 = new StringRevertError(message); + const revert2 = new StringRevertError(message); + const promise = (async () => revert1)(); + return expect(promise).to.become(revert2); + }); + it('should not equate a promise that resolves to a StringRevertError with a different messages', async () => { + const revert1 = new StringRevertError('foo1'); + const revert2 = new StringRevertError('foo2'); + const promise = (async () => revert1)(); + return expect(promise).to.not.become(revert2); + }); + it('should not equate a promise that resolves to different RevertError types', async () => { + const message = 'foo'; + const revert1 = new StringRevertError(message); + const revert2 = new DescendantRevertError(message); + const promise = (async () => revert1)(); + return expect(promise).to.not.become(revert2); + }); + }); + // TODO: Remove these tests when we no longer coerce `Error` types to `StringRevertError` types + // for backwards compatibility. + describe('#rejectedWith (backwards compatibility)', () => { + it('should equate a promise that rejects with an Error to a string of the same message', async () => { + const message = 'foo'; + const revert1 = new Error(message); + const promise = (async () => { + throw revert1; + })(); + return expect(promise).to.rejectedWith(message); + }); + it('should equate a promise that rejects with an StringRevertErrors to a string of the same message', async () => { + const message = 'foo'; + const revert = new StringRevertError(message); + const promise = (async () => { + throw revert; + })(); + return expect(promise).to.rejectedWith(message); + }); + it('should not equate a promise that rejects with an StringRevertErrors to a string with different messages', async () => { + const revert = new StringRevertError('foo1'); + const promise = (async () => { + throw revert; + })(); + return expect(promise).to.not.be.rejectedWith('foo2'); + }); + }); + }); +}); diff --git a/packages/ethereum-types/CHANGELOG.json b/packages/ethereum-types/CHANGELOG.json index 5654d582c8..2e00b2a8f0 100644 --- a/packages/ethereum-types/CHANGELOG.json +++ b/packages/ethereum-types/CHANGELOG.json @@ -1,4 +1,13 @@ [ + { + "version": "2.2.0", + "changes": [ + { + "note": "Add `RevertErrorAbi` interface as part of `AbiDefinition` types", + "pr": 1761 + } + ] + }, { "timestamp": 1553091633, "version": "2.1.1", diff --git a/packages/ethereum-types/src/index.ts b/packages/ethereum-types/src/index.ts index 2d0efb5dfc..70e650fe70 100644 --- a/packages/ethereum-types/src/index.ts +++ b/packages/ethereum-types/src/index.ts @@ -77,7 +77,7 @@ export interface EIP1193Provider { export type ContractAbi = AbiDefinition[]; -export type AbiDefinition = FunctionAbi | EventAbi; +export type AbiDefinition = FunctionAbi | EventAbi | RevertErrorAbi; export type FunctionAbi = MethodAbi | ConstructorAbi | FallbackAbi; @@ -116,6 +116,12 @@ export interface EventParameter extends DataItem { indexed: boolean; } +export interface RevertErrorAbi { + type: 'error'; + name: string; + arguments?: DataItem[]; +} + export interface EventAbi { // Ideally this would be set to: `'event'` but then TS complains when artifacts are loaded // from JSON files, and this value has type `string` not type `'event'` diff --git a/packages/order-utils/CHANGELOG.json b/packages/order-utils/CHANGELOG.json index b0f5ce1e7c..01420f223b 100644 --- a/packages/order-utils/CHANGELOG.json +++ b/packages/order-utils/CHANGELOG.json @@ -9,6 +9,10 @@ { "note": "Update domain schema for Exchange and Coordinator", "pr": 1742 + }, + { + "note": "Add Exchange `RevertError` types to `ExchangeRevertErrors`", + "pr": 1761 } ] }, diff --git a/packages/order-utils/package.json b/packages/order-utils/package.json index 5452ea7a3a..bf08cc5a6b 100644 --- a/packages/order-utils/package.json +++ b/packages/order-utils/package.json @@ -41,9 +41,6 @@ "@types/lodash": "4.14.104", "@types/web3-provider-engine": "^14.0.0", "chai": "^4.0.1", - "chai-as-promised": "^7.1.0", - "chai-bignumber": "^3.0.0", - "dirty-chai": "^2.0.1", "make-promises-safe": "^1.1.0", "mocha": "^4.1.0", "npm-run-all": "^4.1.2", diff --git a/packages/order-utils/src/exchange_revert_errors.ts b/packages/order-utils/src/exchange_revert_errors.ts new file mode 100644 index 0000000000..a9e82fefb2 --- /dev/null +++ b/packages/order-utils/src/exchange_revert_errors.ts @@ -0,0 +1,189 @@ +import { OrderStatus } from '@0x/types'; +import { BigNumber, RevertError } from '@0x/utils'; +import * as _ from 'lodash'; + +// tslint:disable:max-classes-per-file + +export enum FillErrorCode { + InvalidTakerAmount, + TakerOverpay, + Overfill, + InvalidFillPrice, +} + +export enum SignatureErrorCode { + BadSignature, + InvalidLength, + Unsupported, + Illegal, + WalletError, + ValidatorError, +} + +export enum AssetProxyDispatchErrorCode { + InvalidAssetDataLength, + UnknownAssetProxy, +} + +export enum TransactionErrorCode { + NoReentrancy, + AlreadyExecuted, +} + +export class SignatureError extends RevertError { + constructor(error?: SignatureErrorCode, hash?: string, signer?: string, signature?: string) { + super('SignatureError(uint8 error, bytes32 hash, address signer, bytes signature)', { + error, + hash, + signer, + signature, + }); + } +} + +export class SignatureValidatorError extends RevertError { + constructor(hash?: string, signer?: string, signature?: string, errorData?: string) { + super('SignatureValidatorError(bytes32 hash, address signer, bytes signature, bytes errorData)', { + hash, + signer, + signature, + errorData, + }); + } +} + +export class SignatureWalletError extends RevertError { + constructor(hash?: string, signer?: string, signature?: string, errorData?: string) { + super('SignatureWalletError(bytes32 hash, address signer, bytes signature, bytes errorData)', { + hash, + signer, + signature, + errorData, + }); + } +} + +export class OrderStatusError extends RevertError { + constructor(status?: OrderStatus, orderHash?: string) { + super('OrderStatusError(uint8 status, bytes32 orderHash)', { status, orderHash }); + } +} + +export class InvalidSenderError extends RevertError { + constructor(orderHash?: string, sender?: string) { + super('InvalidSenderError(bytes32 orderHash, address sender)', { orderHash, sender }); + } +} + +export class InvalidTakerError extends RevertError { + constructor(orderHash?: string, taker?: string) { + super('InvalidTakerError(bytes32 orderHash, address taker)', { orderHash, taker }); + } +} + +export class InvalidMakerError extends RevertError { + constructor(orderHash?: string, maker?: string) { + super('InvalidMakerError(bytes32 orderHash, address maker)', { orderHash, maker }); + } +} + +export class FillError extends RevertError { + constructor(error?: FillErrorCode, orderHash?: string) { + super('FillError(uint8 error, bytes32 orderHash)', { error, orderHash }); + } +} + +export class OrderEpochError extends RevertError { + constructor(maker?: string, sender?: string, currentEpoch?: BigNumber | number | string) { + super('OrderEpochError(address maker, address sender, uint256 currentEpoch)', { + maker, + sender, + currentEpoch, + }); + } +} + +export class AssetProxyExistsError extends RevertError { + constructor(proxy?: string) { + super('AssetProxyExistsError(address proxy)', { proxy }); + } +} + +export class AssetProxyDispatchError extends RevertError { + constructor(error?: AssetProxyDispatchErrorCode, orderHash?: string, assetData?: string) { + super('AssetProxyDispatchError(uint8 error, bytes32 orderHash, bytes assetData)', { + error, + orderHash, + assetData, + }); + } +} + +export class AssetProxyTransferError extends RevertError { + constructor(orderHash?: string, assetData?: string, errorData?: string) { + super('AssetProxyTransferError(bytes32 orderHash, bytes assetData, bytes errorData)', { + orderHash, + assetData, + errorData, + }); + } +} + +export class NegativeSpreadError extends RevertError { + constructor(leftOrderHash?: string, rightOrderHash?: string) { + super('NegativeSpreadError(bytes32 leftOrderHash, bytes32 rightOrderHash)', { leftOrderHash, rightOrderHash }); + } +} + +export class TransactionError extends RevertError { + constructor(error?: TransactionErrorCode, transactionHash?: string) { + super('TransactionError(uint8 error, bytes32 transactionHash)', { error, transactionHash }); + } +} + +export class TransactionSignatureError extends RevertError { + constructor(transactionHash?: string, signer?: string, signature?: string) { + super('TransactionSignatureError(bytes32 transactionHash, address signer, bytes signature)', { + transactionHash, + signer, + signature, + }); + } +} + +export class TransactionExecutionError extends RevertError { + constructor(transactionHash?: string, errorData?: string) { + super('TransactionExecutionError(bytes32 transactionHash, bytes errorData)', { transactionHash, errorData }); + } +} + +export class IncompleteFillError extends RevertError { + constructor(orderHash?: string) { + super('IncompleteFillError(bytes32 orderHash)', { orderHash }); + } +} + +const types = [ + OrderStatusError, + SignatureError, + SignatureWalletError, + SignatureValidatorError, + InvalidSenderError, + InvalidTakerError, + InvalidMakerError, + FillError, + OrderEpochError, + AssetProxyExistsError, + AssetProxyDispatchError, + AssetProxyTransferError, + NegativeSpreadError, + TransactionError, + TransactionSignatureError, + TransactionExecutionError, + IncompleteFillError, +]; + +// Register the types we've defined. +for (const type of types) { + RevertError.registerType(type); +} diff --git a/packages/order-utils/src/index.ts b/packages/order-utils/src/index.ts index 8bb1bd671e..a77dff5a02 100644 --- a/packages/order-utils/src/index.ts +++ b/packages/order-utils/src/index.ts @@ -1,3 +1,5 @@ +import * as ExchangeRevertErrors from './exchange_revert_errors'; + export { orderHashUtils } from './order_hash'; export { signatureUtils } from './signature_utils'; export { generatePseudoRandomSalt } from './salt'; @@ -75,3 +77,5 @@ export { FeeOrdersAndRemainingFeeAmount, OrdersAndRemainingFillAmount, } from './types'; + +export { ExchangeRevertErrors }; diff --git a/packages/order-utils/test/utils/chai_setup.ts b/packages/order-utils/test/utils/chai_setup.ts index 1a87330932..e721397b28 100644 --- a/packages/order-utils/test/utils/chai_setup.ts +++ b/packages/order-utils/test/utils/chai_setup.ts @@ -1,13 +1 @@ -import * as chai from 'chai'; -import chaiAsPromised = require('chai-as-promised'); -import ChaiBigNumber = require('chai-bignumber'); -import * as dirtyChai from 'dirty-chai'; - -export const chaiSetup = { - configure(): void { - chai.config.includeStack = true; - chai.use(ChaiBigNumber()); - chai.use(dirtyChai); - chai.use(chaiAsPromised); - }, -}; +export { chaiSetup } from '@0x/dev-utils'; diff --git a/packages/types/CHANGELOG.json b/packages/types/CHANGELOG.json index a392bfba16..0a3d1a829f 100644 --- a/packages/types/CHANGELOG.json +++ b/packages/types/CHANGELOG.json @@ -1,4 +1,13 @@ [ + { + "version": "2.4.0", + "changes": [ + { + "note": "Add `OrderStatus` type", + "pr": 1761 + } + ] + }, { "version": "2.3.0", "changes": [ diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 4e52682e1d..398c7d2ea8 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -764,3 +764,13 @@ export interface EIP712DomainWithDefaultSchema { chainId: number; verifyingContractAddress: string; } + +export enum OrderStatus { + Invalid, + InvalidMakerAssetAmount, + InvalidTakerAssetAmount, + Fillable, + Expired, + FullyFilled, + Cancelled, +} diff --git a/packages/typescript-typings/CHANGELOG.json b/packages/typescript-typings/CHANGELOG.json index 657041d2b3..888c124f72 100644 --- a/packages/typescript-typings/CHANGELOG.json +++ b/packages/typescript-typings/CHANGELOG.json @@ -1,4 +1,13 @@ [ + { + "version": "4.3.0", + "changes": [ + { + "note": "Add types for `@0x/dev-utils` chai helpers in `types/@0x`", + "pr": 1761 + } + ] + }, { "timestamp": 1553183790, "version": "4.2.1", diff --git a/packages/typescript-typings/types/@0x/index.d.ts b/packages/typescript-typings/types/@0x/index.d.ts new file mode 100644 index 0000000000..123fe211f9 --- /dev/null +++ b/packages/typescript-typings/types/@0x/index.d.ts @@ -0,0 +1,6 @@ +// tslint:disable: no-namespace +declare namespace Chai { + interface Assertion { + revertWith: (expected: string | RevertError) => Promise; + } +} diff --git a/packages/utils/CHANGELOG.json b/packages/utils/CHANGELOG.json index 7797d16ff5..c94f0ff9f9 100644 --- a/packages/utils/CHANGELOG.json +++ b/packages/utils/CHANGELOG.json @@ -9,6 +9,10 @@ { "note": "More robust normalization of `uint256` types in `sign_typed_data_utils`", "pr": 1742 + }, + { + "note": "Add `RevertError`, `StringRevertError`, `AnyRevertError` types and associated utilities", + "pr": 1761 } ] }, diff --git a/packages/utils/src/index.ts b/packages/utils/src/index.ts index 02d4c545e3..ecbc76e52e 100644 --- a/packages/utils/src/index.ts +++ b/packages/utils/src/index.ts @@ -14,3 +14,10 @@ export { signTypedDataUtils } from './sign_typed_data_utils'; export import AbiEncoder = require('./abi_encoder'); export * from './types'; export { generatePseudoRandom256BitNumber } from './random'; +export { + decodeRevertError, + registerRevertErrorType, + RevertError, + StringRevertError, + AnyRevertError, +} from './revert_error'; diff --git a/packages/utils/src/revert_error.ts b/packages/utils/src/revert_error.ts new file mode 100644 index 0000000000..2676ce3a17 --- /dev/null +++ b/packages/utils/src/revert_error.ts @@ -0,0 +1,352 @@ +import { ObjectMap } from '@0x/types'; +import { DataItem, RevertErrorAbi } from 'ethereum-types'; +import * as ethUtil from 'ethereumjs-util'; +import * as _ from 'lodash'; +import { inspect } from 'util'; + +import * as AbiEncoder from './abi_encoder'; +import { BigNumber } from './configured_bignumber'; + +// tslint:disable: max-classes-per-file + +type ArgTypes = string | BigNumber | number | boolean; +type ValueMap = ObjectMap; +type RevertErrorDecoder = (hex: string) => ValueMap; + +interface RevertErrorType { + new (): RevertError; +} + +interface RevertErrorRegistryItem { + type: RevertErrorType; + decoder: RevertErrorDecoder; +} + +/** + * Register a RevertError type so that it can be decoded by + * `decodeRevertError`. + * @param revertClass A class that inherits from RevertError. + */ +export function registerRevertErrorType(revertClass: RevertErrorType): void { + RevertError.registerType(revertClass); +} + +/** + * Decode an ABI encoded revert error. + * Throws if the data cannot be decoded as a known RevertError type. + * @param bytes The ABI encoded revert error. Either a hex string or a Buffer. + * @return A RevertError object. + */ +export function decodeRevertError(bytes: string | Buffer): RevertError { + return RevertError.decode(bytes); +} + +/** + * Base type for revert errors. + */ +export abstract class RevertError extends Error { + // Map of types registered via `registerType`. + private static readonly _typeRegistry: ObjectMap = {}; + public abi?: RevertErrorAbi; + public values: ValueMap = {}; + + /** + * Decode an ABI encoded revert error. + * Throws if the data cannot be decoded as a known RevertError type. + * @param bytes The ABI encoded revert error. Either a hex string or a Buffer. + * @return A RevertError object. + */ + public static decode(bytes: string | Buffer): RevertError { + const _bytes = bytes instanceof Buffer ? ethUtil.bufferToHex(bytes) : ethUtil.addHexPrefix(bytes); + // tslint:disable-next-line: custom-no-magic-numbers + const selector = _bytes.slice(2, 10); + const { decoder, type } = this._lookupType(selector); + const instance = new type(); + try { + const values = decoder(_bytes); + return _.assign(instance, { values }); + } catch (err) { + throw new Error( + `Bytes ${_bytes} cannot be decoded as a revert error of type ${instance.signature}: ${err.message}`, + ); + } + } + + /** + * Register a RevertError type so that it can be decoded by + * `RevertError.decode`. + * @param revertClass A class that inherits from RevertError. + */ + public static registerType(revertClass: RevertErrorType): void { + const instance = new revertClass(); + if (instance.selector in RevertError._typeRegistry) { + throw new Error(`RevertError type with signature "${instance.signature}" is already registered`); + } + if (_.isNil(instance.abi)) { + throw new Error(`Attempting to register a RevertError class with no ABI`); + } + RevertError._typeRegistry[instance.selector] = { + type: revertClass, + decoder: createDecoder(instance.abi), + }; + } + + // Ge tthe registry info given a selector. + private static _lookupType(selector: string): RevertErrorRegistryItem { + if (selector in RevertError._typeRegistry) { + return RevertError._typeRegistry[selector]; + } + throw new Error(`Unknown revert error selector "${selector}"`); + } + + /** + * Create a RevertError instance with optional parameter values. + * Parameters that are left undefined will not be tested in equality checks. + * @param declaration Function-style declaration of the revert (e.g., Error(string message)) + * @param values Optional mapping of parameters to values. + */ + protected constructor(declaration?: string, values?: ValueMap) { + super(); + if (declaration !== undefined) { + this.abi = declarationToAbi(declaration); + if (values !== undefined) { + _.assign(this.values, _.cloneDeep(values)); + } + } + // Extending Error is tricky; we need to explicitly set the prototype. + Object.setPrototypeOf(this, new.target.prototype); + this.message = this.toString(); + } + + /** + * Get the ABI name for this revert. + */ + get name(): string { + if (!_.isNil(this.abi)) { + return this.abi.name; + } + return `<${this.typeName}>`; + } + + /** + * Get the class name of this type. + */ + get typeName(): string { + // tslint:disable-next-line: no-string-literal + return this.constructor.name; + } + + /** + * Get the hex selector for this revert (without leading '0x'). + */ + get selector(): string { + if (!_.isNil(this.abi)) { + return toSelector(this.abi); + } + return ''; + } + + /** + * Get the signature for this revert: e.g., 'Error(string)'. + */ + get signature(): string { + if (!_.isNil(this.abi)) { + return toSignature(this.abi); + } + return ''; + } + + /** + * Get the ABI arguments for this revert. + */ + get arguments(): DataItem[] { + if (!_.isNil(this.abi)) { + return this.abi.arguments || []; + } + return []; + } + + /** + * Compares this instance with another. + * Fails if instances are not of the same type. + * Only fields/values defined in both instances are compared. + * @param other Either another RevertError instance, hex-encoded bytes, or a Buffer of the ABI encoded revert. + * @return True if both instances match. + */ + public equals(other: RevertError | Buffer | string): boolean { + let _other = other; + if (_other instanceof Buffer) { + _other = ethUtil.bufferToHex(_other); + } + if (typeof _other === 'string') { + _other = RevertError.decode(_other); + } + if (!(_other instanceof RevertError)) { + return false; + } + // If either is of the `AnyRevertError` type, always succeed. + if (this._isAnyType || _other._isAnyType) { + return true; + } + // Must be of same type. + if (this.constructor !== _other.constructor) { + return false; + } + // Must share the same parameter values if defined in both instances. + for (const name of Object.keys(this.values)) { + const a = this.values[name]; + const b = _other.values[name]; + if (a === b) { + continue; + } + if (!_.isNil(a) && !_.isNil(b)) { + const { type } = this._getArgumentByName(name); + if (!checkArgEquality(type, a, b)) { + return false; + } + } + } + return true; + } + + public encode(): string { + if (!this._hasAllArgumentValues) { + throw new Error(`Instance of ${this.typeName} does not have all its parameter values set.`); + } + const encoder = createEncoder(this.abi as RevertErrorAbi); + return encoder(this.values); + } + + public toString(): string { + const values = _.omitBy(this.values, (v: any) => _.isNil(v)); + const inner = _.isEmpty(values) ? '' : inspect(values); + return `${this.constructor.name}(${inner})`; + } + + private _getArgumentByName(name: string): DataItem { + const arg = _.find(this.arguments, (a: DataItem) => a.name === name); + if (_.isNil(arg)) { + throw new Error(`RevertError ${this.signature} has no argument named ${name}`); + } + return arg; + } + + private get _isAnyType(): boolean { + return _.isNil(this.abi); + } + + private get _hasAllArgumentValues(): boolean { + if (_.isNil(this.abi) || _.isNil(this.abi.arguments)) { + return false; + } + for (const arg of this.abi.arguments) { + if (_.isNil(this.values[arg.name])) { + return false; + } + } + return true; + } +} + +/** + * RevertError type for standard string reverts. + */ +export class StringRevertError extends RevertError { + constructor(message?: string) { + super('Error(string message)', { message }); + } +} + +/** + * Special RevertError type that matches with any other RevertError instance. + */ +export class AnyRevertError extends RevertError { + constructor() { + super(); + } +} + +/** + * Parse a solidity function declaration into a RevertErrorAbi object. + * @param declaration Function declaration (e.g., 'foo(uint256 bar)'). + * @return A RevertErrorAbi object. + */ +function declarationToAbi(declaration: string): RevertErrorAbi { + let m = /^\s*([_a-z][a-z0-9_]*)\((.*)\)\s*$/i.exec(declaration); + if (!m) { + throw new Error(`Invalid Revert Error signature: "${declaration}"`); + } + const [name, args] = m.slice(1); + const argList: string[] = _.filter(args.split(',')); + const argData: DataItem[] = _.map(argList, (a: string) => { + m = /^\s*([_a-z][a-z0-9_]*)\s+([_a-z][a-z0-9_]*)\s*$/i.exec(a); + if (!m) { + throw new Error(`Invalid Revert Error signature: "${declaration}"`); + } + return { + name: m[2], + type: m[1], + }; + }); + const r: RevertErrorAbi = { + type: 'error', + name, + arguments: _.isEmpty(argData) ? [] : argData, + }; + return r; +} + +function checkArgEquality(type: string, lhs: ArgTypes, rhs: ArgTypes): boolean { + if (type === 'address') { + return normalizeAddress(lhs as string) === normalizeAddress(rhs as string); + } else if (type.startsWith('bytes')) { + return normalizeBytes(lhs as string) === normalizeBytes(rhs as string); + } else if (type === 'string') { + return lhs === rhs; + } + // tslint:disable-next-line + return new BigNumber((lhs as any) || 0).eq(rhs as any); +} + +function normalizeAddress(addr: string): string { + const ADDRESS_SIZE = 20; + return ethUtil.bufferToHex(ethUtil.setLengthLeft(ethUtil.toBuffer(ethUtil.addHexPrefix(addr)), ADDRESS_SIZE)); +} + +function normalizeBytes(bytes: string): string { + return ethUtil.addHexPrefix(bytes).toLowerCase(); +} + +function createEncoder(abi: RevertErrorAbi): (values: ObjectMap) => string { + const encoder = AbiEncoder.createMethod(abi.name, abi.arguments || []); + return (values: ObjectMap): string => { + const valuesArray = _.map(abi.arguments, (arg: DataItem) => values[arg.name]); + return encoder.encode(valuesArray); + }; +} + +function createDecoder(abi: RevertErrorAbi): (hex: string) => ValueMap { + const encoder = AbiEncoder.createMethod(abi.name, abi.arguments || []); + return (hex: string): ValueMap => { + return encoder.decode(hex) as ValueMap; + }; +} + +function toSignature(abi: RevertErrorAbi): string { + const argTypes = _.map(abi.arguments, (a: DataItem) => a.type); + const args = argTypes.join(','); + return `${abi.name}(${args})`; +} + +function toSelector(abi: RevertErrorAbi): string { + return ( + ethUtil + .sha3(Buffer.from(toSignature(abi))) + // tslint:disable-next-line: custom-no-magic-numbers + .slice(0, 4) + .toString('hex') + ); +} + +// Register StringRevertError +RevertError.registerType(StringRevertError); diff --git a/packages/utils/test/revert_error_test.ts b/packages/utils/test/revert_error_test.ts new file mode 100644 index 0000000000..a09dcadb35 --- /dev/null +++ b/packages/utils/test/revert_error_test.ts @@ -0,0 +1,108 @@ +import * as chai from 'chai'; +import * as _ from 'lodash'; + +import { AnyRevertError, RevertError, StringRevertError } from '../src/revert_error'; + +import { chaiSetup } from './utils/chai_setup'; + +chaiSetup.configure(); +const expect = chai.expect; + +// tslint:disable: max-classes-per-file +class DescendantRevertError extends StringRevertError { + public constructor(message: string) { + super(message); + } +} + +class CustomRevertError extends RevertError { + public constructor(message?: string) { + super('CustomRevertError(string message)', { message }); + } +} + +RevertError.registerType(CustomRevertError); + +describe('RevertError', () => { + describe('equality', () => { + const message = 'foo'; + it('should equate two identical RevertErrors', () => { + const revert1 = new StringRevertError(message); + const revert2 = new StringRevertError(message); + expect(revert1.equals(revert2)).to.be.true(); + }); + it('should equate two RevertErrors with missing fields', () => { + const revert1 = new StringRevertError(message); + const revert2 = new StringRevertError(); + expect(revert1.equals(revert2)).to.be.true(); + }); + it('should equate AnyRevertError with a real RevertError', () => { + const revert1 = new StringRevertError(message); + const revert2 = new AnyRevertError(); + expect(revert1.equals(revert2)).to.be.true(); + }); + it('should not equate a the same RevertError type with different values', () => { + const revert1 = new StringRevertError(message); + const revert2 = new StringRevertError(`${message}1`); + expect(revert1.equals(revert2)).to.be.false(); + }); + it('should not equate different RevertError types', () => { + const revert1 = new StringRevertError(message); + const revert2 = new DescendantRevertError(message); + expect(revert1.equals(revert2)).to.be.false(); + }); + }); + describe('registering', () => { + it('should throw when registering an already registered signature', () => { + class CustomRevertError2 extends RevertError { + public constructor() { + super(new CustomRevertError().signature, {}); + } + } + expect(() => RevertError.registerType(CustomRevertError2)).to.throw(); + }); + }); + describe('decoding', () => { + // tslint:disable: prefer-template custom-no-magic-numbers + const message = 'foobar'; + const encoded = + '0x08c379a0' + + '0000000000000000000000000000000000000000000000000000000000000020' + + '0000000000000000000000000000000000000000000000000000000000000006' + + Buffer.from(message).toString('hex') + + _.repeat('00', 32 - 6); + + it('should decode an ABI encoded revert error', () => { + const expected = new StringRevertError(message); + const decoded = RevertError.decode(encoded); + expect(decoded.equals(expected)).to.be.true(); + }); + it('should fail to decode an ABI encoded revert error with an unknown selector', () => { + const _encoded = encoded.substr(0, 2) + '00' + encoded.substr(4); + const decode = () => RevertError.decode(_encoded); + expect(decode).to.be.throw(); + }); + it('should fail to decode a malformed ABI encoded revert error', () => { + const _encoded = encoded.substr(0, encoded.substr.length - 1); + const decode = () => RevertError.decode(_encoded); + expect(decode).to.be.throw(); + }); + }); + describe('encoding', () => { + const message = 'foobar'; + it('should be able to encode', () => { + const expected = + '0x08c379a0' + + '0000000000000000000000000000000000000000000000000000000000000020' + + '0000000000000000000000000000000000000000000000000000000000000006' + + Buffer.from(message).toString('hex') + + _.repeat('00', 32 - 6); + const revert = new StringRevertError(message); + expect(revert.encode()).to.equal(expected); + }); + it('should throw if missing parameter values', () => { + const revert = new StringRevertError(); + expect(() => revert.encode()).to.throw(); + }); + }); +});