From fa7570352ce65dc58df6969c5a24cf3f487e738f Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Fri, 11 May 2018 11:32:12 -0700 Subject: Added require reasons to MixinMatchOrders and cleaned up some comments. --- .../protocol/Exchange/MixinExchangeCore.sol | 18 +++++---- .../current/protocol/Exchange/MixinMatchOrders.sol | 44 ++++++++++++++++------ .../current/protocol/Exchange/MixinSettlement.sol | 8 +++- .../protocol/Exchange/libs/LibExchangeErrors.sol | 8 ++++ .../protocol/Exchange/mixins/MExchangeCore.sol | 2 +- packages/contracts/src/utils/types.ts | 35 ++++++++--------- 6 files changed, 75 insertions(+), 40 deletions(-) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol index 58b5fa6b1..981c5984e 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol @@ -53,7 +53,7 @@ contract MixinExchangeCore is ////// Core exchange functions ////// - /// @dev Gets information about an order. + /// @dev Gets information about an order: status, hash, and amount filled. /// @param order Order to gather information on. /// @return status Status of order. Statuses are defined in the LibStatus.Status struct. /// @return orderHash Keccak-256 EIP712 hash of the order. @@ -105,14 +105,12 @@ contract MixinExchangeCore is orderStatus = uint8(Status.ORDER_CANCELLED); return (orderStatus, orderHash, takerAssetFilledAmount); } - - // Validate order is not cancelled if (makerEpoch[order.makerAddress] > order.salt) { orderStatus = uint8(Status.ORDER_CANCELLED); return (orderStatus, orderHash, takerAssetFilledAmount); } - // Order is Fillable + // All other statuses are ruled out: order is Fillable orderStatus = uint8(Status.ORDER_FILLABLE); return (orderStatus, orderHash, takerAssetFilledAmount); } @@ -136,6 +134,8 @@ contract MixinExchangeCore is internal { // Ensure order is valid + // An order can only be filled if it is Status.FILLABLE; + // however, only invalid statuses result in a throw. require( orderStatus != uint8(Status.ORDER_INVALID_MAKER_ASSET_AMOUNT), INVALID_ORDER_MAKER_ASSET_AMOUNT @@ -269,7 +269,7 @@ contract MixinExchangeCore is public returns (FillResults memory fillResults) { - // Fetch current order status + // Fetch order info bytes32 orderHash; uint8 orderStatus; uint256 takerAssetFilledAmount; @@ -309,6 +309,8 @@ contract MixinExchangeCore is internal { // Ensure order is valid + // An order can only be cancelled if it is Status.FILLABLE; + // however, only invalid statuses result in a throw. require( orderStatus != uint8(Status.ORDER_INVALID_MAKER_ASSET_AMOUNT), INVALID_ORDER_MAKER_ASSET_AMOUNT @@ -372,9 +374,11 @@ contract MixinExchangeCore is } /// @dev After calling, the order can not be filled anymore. - /// @param order Order struct containing order specifications. + /// Throws if order is invalid or sender does not have permission to cancel. + /// @param order Order to cancel. Order must be Status.FILLABLE. /// @return True if the order state changed to cancelled. - /// False if the transaction was already cancelled or expired. + /// False if the order was order was in a valid, but + /// unfillable state (see LibStatus.STATUS for order states) function cancelOrder(Order memory order) public returns (bool) diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol index 76b021919..a333b8221 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol @@ -23,6 +23,7 @@ import "./libs/LibMath.sol"; import "./libs/LibOrder.sol"; import "./libs/LibStatus.sol"; import "../../utils/LibBytes/LibBytes.sol"; +import "./libs/LibExchangeErrors.sol"; contract MixinMatchOrders is SafeMath, @@ -30,6 +31,7 @@ contract MixinMatchOrders is LibMath, LibStatus, LibOrder, + LibExchangeErrors, MExchangeCore, MMatchOrders, MSettlement, @@ -45,22 +47,29 @@ contract MixinMatchOrders is internal { // The leftOrder maker asset must be the same as the rightOrder taker asset. - require(areBytesEqual(leftOrder.makerAssetData, rightOrder.takerAssetData)); + require( + areBytesEqual(leftOrder.makerAssetData, rightOrder.takerAssetData), + ASSET_MISMATCH_MAKER_TAKER + ); // The leftOrder taker asset must be the same as the rightOrder maker asset. - require(areBytesEqual(leftOrder.takerAssetData, rightOrder.makerAssetData)); + require( + areBytesEqual(leftOrder.takerAssetData, rightOrder.makerAssetData), + ASSET_MISMATCH_TAKER_MAKER + ); // Make sure there is a positive spread. // There is a positive spread iff the cost per unit bought (OrderA.MakerAmount/OrderA.TakerAmount) for each order is greater // than the profit per unit sold of the matched order (OrderB.TakerAmount/OrderB.MakerAmount). // This is satisfied by the equations below: - // / >= / + // / = / // AND // / >= / // These equations can be combined to get the following: require( safeMul(leftOrder.makerAssetAmount, rightOrder.makerAssetAmount) >= - safeMul(leftOrder.takerAssetAmount, rightOrder.takerAssetAmount) + safeMul(leftOrder.takerAssetAmount, rightOrder.takerAssetAmount), + NEGATIVE_SPREAD ); } @@ -69,11 +78,21 @@ contract MixinMatchOrders is function validateMatchedOrderFillResultsOrThrow(MatchedFillResults memory matchedFillResults) internal { - // The right order must spend at least as much as we're transferring to the left order's maker. - // If the amount transferred from the right order is greater than what is transferred, it is a rounding error amount. + // The right order must spend at least as much as we're transferring to the left order. + require( + matchedFillResults.right.makerAssetFilledAmount >= + matchedFillResults.left.takerAssetFilledAmount, + MISCALCULATED_TRANSFER_AMOUNTS + ); + // If the amount transferred from the right order is different than what is transferred, it is a rounding error amount. // Ensure this difference is negligible by dividing the values with each other. The result should equal to ~1. - require(matchedFillResults.right.makerAssetFilledAmount >= matchedFillResults.left.takerAssetFilledAmount); - require(!isRoundingError(matchedFillResults.right.makerAssetFilledAmount, matchedFillResults.left.takerAssetFilledAmount, 1)); + require( + !isRoundingError( + matchedFillResults.right.makerAssetFilledAmount, + matchedFillResults.left.takerAssetFilledAmount, + 1), + ROUNDING_ERROR_TRANSFER_AMOUNTS + ); } /// @dev Calculates partial value given a numerator and denominator. @@ -89,7 +108,10 @@ contract MixinMatchOrders is internal pure returns (uint256 partialAmount) { - require(!isRoundingError(numerator, denominator, target)); + require( + !isRoundingError(numerator, denominator, target), + ROUNDING_ERROR_ON_PARTIAL_AMOUNT + ); return getPartialAmount(numerator, denominator, target); } @@ -136,7 +158,7 @@ contract MixinMatchOrders is leftOrderAmountToFill = leftTakerAssetAmountRemaining; // The right order receives an amount proportional to how much was spent. - // TODO: Ensure rounding error is in the correct direction. + // TODO: Can we ensure rounding error is in the correct direction? rightOrderAmountToFill = safeGetPartialAmount( rightOrder.takerAssetAmount, rightOrder.makerAssetAmount, @@ -146,7 +168,7 @@ contract MixinMatchOrders is rightOrderAmountToFill = rightTakerAssetAmountRemaining; // The left order receives an amount proportional to how much was spent. - // TODO: Ensure rounding error is in the correct direction. + // TODO: Can we ensure rounding error is in the correct direction? leftOrderAmountToFill = safeGetPartialAmount( rightOrder.makerAssetAmount, rightOrder.takerAssetAmount, diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSettlement.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSettlement.sol index 3b67051d2..cde64ba74 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSettlement.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSettlement.sol @@ -23,9 +23,11 @@ import "./mixins/MAssetProxyDispatcher.sol"; import "./libs/LibOrder.sol"; import "./libs/LibMath.sol"; import "./mixins/MMatchOrders.sol"; +import "./mixins/LibExchangeErrors.sol"; contract MixinSettlement is LibMath, + LibExchangeErrors, MMatchOrders, MSettlement, MAssetProxyDispatcher @@ -134,7 +136,11 @@ contract MixinSettlement is // rightOrder.MakerAsset == leftOrder.TakerAsset // leftOrder.takerAssetFilledAmount ~ rightOrder.makerAssetFilledAmount // The change goes to right, not to taker. - assert(matchedFillResults.right.makerAssetFilledAmount >= matchedFillResults.left.takerAssetFilledAmount); + require( + matchedFillResults.right.makerAssetFilledAmount >= + matchedFillResults.left.takerAssetFilledAmount, + MISCALCULATED_TRANSFER_AMOUNTS + ); dispatchTransferFrom( rightOrder.makerAssetData, rightOrder.makerAddress, diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol index bf048e815..ba055dd8e 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol @@ -48,4 +48,12 @@ contract LibExchangeErrors { string constant INVALID_SIGNATURE_LENGTH = "Invalid signature length."; string constant ILLEGAL_SIGNATURE_TYPE = "Illegal signature type."; string constant UNSUPPORTED_SIGNATURE_TYPE = "Unsupported signature type."; + + // Order matching revert reasons + string constant ASSET_MISMATCH_MAKER_TAKER = "Left order maker asset is different from right order taker asset."; + string constant ASSET_MISMATCH_TAKER_MAKER = "Left order taker asset is different from right order maker asset."; + string constant NEGATIVE_SPREAD = "Matched orders must have a positive spread."; + string constant MISCALCULATED_TRANSFER_AMOUNTS = "A miscalculation occurred: the left maker would receive more than the right maker would spend."; + string constant ROUNDING_ERROR_TRANSFER_AMOUNTS = "A rounding error occurred when calculating transfer amounts for matched orders."; + string constant ROUNDING_ERROR_ON_PARTIAL_AMOUNT = "A rounding error occurred when calculating partial transfer amounts."; } diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol index 416cf426d..cfd2678ba 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol @@ -65,7 +65,7 @@ contract MExchangeCore is LibFillResults.FillResults memory fillResults) internal; - /// @dev Gets information about an order. + /// @dev Gets information about an order: status, hash, and amount filled. /// @param order Order to gather information on. /// @return status Status of order. Statuses are defined in the LibStatus.Status struct. /// @return orderHash Keccak-256 EIP712 hash of the order. diff --git a/packages/contracts/src/utils/types.ts b/packages/contracts/src/utils/types.ts index ce7fbcbe1..0e3b2c9a8 100644 --- a/packages/contracts/src/utils/types.ts +++ b/packages/contracts/src/utils/types.ts @@ -75,26 +75,21 @@ export interface Token { } export enum ExchangeStatus { - /// Default Status /// - INVALID, // General invalid status - - /// General Exchange Statuses /// - SUCCESS, // Indicates a successful operation - ROUNDING_ERROR_TOO_LARGE, // Rounding error too large - INSUFFICIENT_BALANCE_OR_ALLOWANCE, // Insufficient balance or allowance for token transfer - TAKER_ASSET_FILL_AMOUNT_TOO_LOW, // takerAssetFillAmount is <= 0 - INVALID_SIGNATURE, // Invalid signature - INVALID_SENDER, // Invalid sender - INVALID_TAKER, // Invalid taker - INVALID_MAKER, // Invalid maker - - /// Order State Statuses /// - ORDER_INVALID_MAKER_ASSET_AMOUNT, // Order does not have a valid maker asset amount - ORDER_INVALID_TAKER_ASSET_AMOUNT, // Order does not have a valid taker asset amount - ORDER_FILLABLE, // Order is fillable - ORDER_EXPIRED, // Order has already expired - ORDER_FULLY_FILLED, // Order is fully filled - ORDER_CANCELLED, // Order has been cancelled + INVALID, + SUCCESS, + ROUNDING_ERROR_TOO_LARGE, + INSUFFICIENT_BALANCE_OR_ALLOWANCE, + TAKER_ASSET_FILL_AMOUNT_TOO_LOW, + INVALID_SIGNATURE, + INVALID_SENDER, + INVALID_TAKER, + INVALID_MAKER, + ORDER_INVALID_MAKER_ASSET_AMOUNT, + ORDER_INVALID_TAKER_ASSET_AMOUNT, + ORDER_FILLABLE, + ORDER_EXPIRED, + ORDER_FULLY_FILLED, + ORDER_CANCELLED, } export enum ContractName { -- cgit v1.2.3