From 342432dc76d1bfc2531fb2edef77afc523eacefe Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 29 May 2018 22:09:57 -0700 Subject: Update Exchange statuses, revert instead of emmitting event on fill/cancel failures, and remove redundant logic in matchOrders --- packages/contracts/compiler.json | 2 +- .../current/protocol/AssetProxy/ERC20Proxy.sol | 15 +- .../current/protocol/AssetProxy/ERC721Proxy.sol | 14 +- .../protocol/AssetProxy/MixinAssetProxy.sol | 4 +- .../protocol/AssetProxy/MixinAuthorizable.sol | 13 +- .../AssetProxy/libs/LibAssetProxyErrors.sol | 37 +++ .../Exchange/MixinAssetProxyDispatcher.sol | 13 +- .../protocol/Exchange/MixinExchangeCore.sol | 349 +++++++++------------ .../current/protocol/Exchange/MixinMatchOrders.sol | 160 +++------- .../current/protocol/Exchange/MixinSettlement.sol | 15 +- .../protocol/Exchange/MixinSignatureValidator.sol | 31 +- .../protocol/Exchange/MixinTransactions.sol | 16 +- .../protocol/Exchange/MixinWrapperFunctions.sol | 24 +- .../protocol/Exchange/interfaces/IExchangeCore.sol | 28 +- .../protocol/Exchange/libs/LibExchangeErrors.sol | 79 ++--- .../protocol/Exchange/libs/LibFillResults.sol | 21 +- .../current/protocol/Exchange/libs/LibMath.sol | 20 -- .../current/protocol/Exchange/libs/LibOrder.sol | 34 +- .../current/protocol/Exchange/libs/LibStatus.sol | 51 --- .../protocol/Exchange/mixins/MExchangeCore.sol | 76 ++--- .../protocol/Exchange/mixins/MMatchOrders.sol | 22 +- .../protocol/Exchange/mixins/MSettlement.sol | 1 - .../contracts/current/test/TestLibs/TestLibs.sol | 4 +- packages/contracts/src/utils/match_order_tester.ts | 1 - packages/contracts/src/utils/types.ts | 22 +- .../contracts/test/asset_proxy/authorizable.ts | 1 + packages/contracts/test/exchange/core.ts | 89 +++--- packages/contracts/test/exchange/match_orders.ts | 43 ++- packages/contracts/test/exchange/transactions.ts | 6 +- packages/contracts/test/exchange/wrapper.ts | 2 +- 30 files changed, 491 insertions(+), 702 deletions(-) create mode 100644 packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibAssetProxyErrors.sol delete mode 100644 packages/contracts/src/contracts/current/protocol/Exchange/libs/LibStatus.sol diff --git a/packages/contracts/compiler.json b/packages/contracts/compiler.json index ed59069fe..48ba4ffcd 100644 --- a/packages/contracts/compiler.json +++ b/packages/contracts/compiler.json @@ -4,7 +4,7 @@ "compilerSettings": { "optimizer": { "enabled": true, - "runs": 0 + "runs": 200 }, "outputSelection": { "*": { diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol index 79824fbbb..2c321e134 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol @@ -20,9 +20,9 @@ pragma solidity ^0.4.24; pragma experimental ABIEncoderV2; import "../../utils/LibBytes/LibBytes.sol"; -import "../../tokens/ERC20Token/IERC20Token.sol"; import "./MixinAssetProxy.sol"; import "./MixinAuthorizable.sol"; +import "../../tokens/ERC20Token/IERC20Token.sol"; contract ERC20Proxy is LibBytes, @@ -33,11 +33,6 @@ contract ERC20Proxy is // Id of this proxy. uint8 constant PROXY_ID = 1; - // Revert reasons - string constant INVALID_METADATA_LENGTH = "Metadata must have a length of 21."; - string constant TRANSFER_FAILED = "Transfer failed."; - string constant PROXY_ID_MISMATCH = "Proxy id in metadata does not match this proxy id."; - /// @dev Internal version of `transferFrom`. /// @param assetMetadata Encoded byte array. /// @param from Address to transfer asset from. @@ -56,12 +51,12 @@ contract ERC20Proxy is require( length == 21, - INVALID_METADATA_LENGTH + LENGTH_21_REQUIRED ); - + // TODO: Is this too inflexible in the future? require( uint8(assetMetadata[length - 1]) == PROXY_ID, - PROXY_ID_MISMATCH + ASSET_PROXY_ID_MISMATCH ); // Decode metadata. @@ -70,7 +65,7 @@ contract ERC20Proxy is // Transfer tokens. bool success = IERC20Token(token).transferFrom(from, to, amount); require( - success == true, + success, TRANSFER_FAILED ); } diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol index ace3570bc..07e01c774 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol @@ -20,9 +20,9 @@ pragma solidity ^0.4.24; pragma experimental ABIEncoderV2; import "../../utils/LibBytes/LibBytes.sol"; -import "../../tokens/ERC721Token/ERC721Token.sol"; import "./MixinAssetProxy.sol"; import "./MixinAuthorizable.sol"; +import "../../tokens/ERC721Token/ERC721Token.sol"; contract ERC721Proxy is LibBytes, @@ -33,11 +33,6 @@ contract ERC721Proxy is // Id of this proxy. uint8 constant PROXY_ID = 2; - // Revert reasons - string constant INVALID_TRANSFER_AMOUNT = "Transfer amount must equal 1."; - string constant INVALID_METADATA_LENGTH = "Metadata must have a length of 53."; - string constant PROXY_ID_MISMATCH = "Proxy id in metadata does not match this proxy id."; - /// @dev Internal version of `transferFrom`. /// @param assetMetadata Encoded byte array. /// @param from Address to transfer asset from. @@ -56,18 +51,19 @@ contract ERC721Proxy is require( length == 53, - INVALID_METADATA_LENGTH + LENGTH_53_REQUIRED ); + // TODO: Is this too inflexible in the future? require( uint8(assetMetadata[length - 1]) == PROXY_ID, - PROXY_ID_MISMATCH + ASSET_PROXY_ID_MISMATCH ); // There exists only 1 of each token. require( amount == 1, - INVALID_TRANSFER_AMOUNT + INVALID_AMOUNT ); // Decode metadata diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAssetProxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAssetProxy.sol index 4bd533148..5fa33cbef 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAssetProxy.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAssetProxy.sol @@ -19,10 +19,10 @@ pragma solidity ^0.4.24; pragma experimental ABIEncoderV2; -import "./mixins/MAssetProxy.sol"; import "./mixins/MAuthorizable.sol"; +import "./mixins/MAssetProxy.sol"; -contract MixinAssetProxy is +contract MixinAssetProxy is MAuthorizable, MAssetProxy { diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAuthorizable.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAuthorizable.sol index 4aaeb66d1..8cb4254c5 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAuthorizable.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAuthorizable.sol @@ -19,21 +19,16 @@ pragma solidity ^0.4.24; pragma experimental ABIEncoderV2; -import "./mixins/MAuthorizable.sol"; +import "./libs/LibAssetProxyErrors.sol"; import "../../utils/Ownable/Ownable.sol"; +import "./mixins/MAuthorizable.sol"; contract MixinAuthorizable is + LibAssetProxyErrors, Ownable, MAuthorizable { - // Revert reasons - string constant SENDER_NOT_AUTHORIZED = "Sender not authorized to call this method."; - string constant TARGET_NOT_AUTHORIZED = "Target address must be authorized."; - string constant TARGET_ALREADY_AUTHORIZED = "Target must not already be authorized."; - string constant INDEX_OUT_OF_BOUNDS = "Specified array index is out of bounds."; - string constant INDEX_ADDRESS_MISMATCH = "Address found at index does not match target address."; - /// @dev Only authorized addresses can invoke functions with this modifier. modifier onlyAuthorized { require( @@ -99,7 +94,7 @@ contract MixinAuthorizable is ); require( authorities[index] == target, - INDEX_ADDRESS_MISMATCH + AUTHORIZED_ADDRESS_MISMATCH ); delete authorized[target]; diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibAssetProxyErrors.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibAssetProxyErrors.sol new file mode 100644 index 000000000..e0c7fc796 --- /dev/null +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibAssetProxyErrors.sol @@ -0,0 +1,37 @@ +/* + + 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.4.24; + +contract LibAssetProxyErrors { + /// Authorizable errors /// + string constant SENDER_NOT_AUTHORIZED = "SENDER_NOT_AUTHORIZED"; // Sender not authorized to call this method. + string constant TARGET_NOT_AUTHORIZED = "TARGET_NOT_AUTHORIZED"; // Target address not authorized to call this method. + string constant TARGET_ALREADY_AUTHORIZED = "TARGET_ALREADY_AUTHORIZED"; // Target address must not already be authorized. + string constant INDEX_OUT_OF_BOUNDS = "INDEX_OUT_OF_BOUNDS"; // Specified array index is out of bounds. + string constant AUTHORIZED_ADDRESS_MISMATCH = "AUTHORIZED_ADDRESS_MISMATCH"; // Address at index does not match given target address. + + /// AssetProxy errors /// + string constant ASSET_PROXY_ID_MISMATCH = "ASSET_PROXY_ID_MISMATCH"; // Proxy id in metadata does not match this proxy id. + string constant INVALID_AMOUNT = "INVALID_AMOUNT"; // Transfer amount must equal 1. + string constant TRANSFER_FAILED = "TRANSFER_FAILED"; // Transfer failed. + + /// Length validation errors /// + string constant LENGTH_21_REQUIRED = "LENGTH_21_REQUIRED"; // Byte array must have a length of 21. + string constant LENGTH_53_REQUIRED = "LENGTH_53_REQUIRED"; // Byte array must have a length of 53. +} diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol index d996f933c..397ca89d2 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol @@ -19,13 +19,13 @@ pragma solidity ^0.4.24; import "../../utils/Ownable/Ownable.sol"; -import "../AssetProxy/interfaces/IAssetProxy.sol"; import "./libs/LibExchangeErrors.sol"; import "./mixins/MAssetProxyDispatcher.sol"; +import "../AssetProxy/interfaces/IAssetProxy.sol"; contract MixinAssetProxyDispatcher is - LibExchangeErrors, Ownable, + LibExchangeErrors, MAssetProxyDispatcher { // Mapping from Asset Proxy Id's to their respective Asset Proxy @@ -45,9 +45,10 @@ contract MixinAssetProxyDispatcher is onlyOwner { // Ensure the existing asset proxy is not unintentionally overwritten + address currentAssetProxy = address(assetProxies[assetProxyId]); require( - oldAssetProxy == address(assetProxies[assetProxyId]), - OLD_ASSET_PROXY_MISMATCH + oldAssetProxy == currentAssetProxy, + ASSET_PROXY_MISMATCH ); IAssetProxy assetProxy = IAssetProxy(newAssetProxy); @@ -57,7 +58,7 @@ contract MixinAssetProxyDispatcher is uint8 newAssetProxyId = assetProxy.getProxyId(); require( newAssetProxyId == assetProxyId, - NEW_ASSET_PROXY_MISMATCH + ASSET_PROXY_ID_MISMATCH ); } @@ -98,7 +99,7 @@ contract MixinAssetProxyDispatcher is uint256 length = assetMetadata.length; require( length > 0, - GT_ZERO_LENGTH_REQUIRED + LENGTH_GT_0_REQUIRED ); uint8 assetProxyId = uint8(assetMetadata[length - 1]); IAssetProxy assetProxy = assetProxies[assetProxyId]; diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol index 1c2420374..42128c92a 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol @@ -22,7 +22,6 @@ pragma experimental ABIEncoderV2; import "./libs/LibFillResults.sol"; import "./libs/LibOrder.sol"; import "./libs/LibMath.sol"; -import "./libs/LibStatus.sol"; import "./libs/LibExchangeErrors.sol"; import "./mixins/MExchangeCore.sol"; import "./mixins/MSettlement.sol"; @@ -30,9 +29,7 @@ import "./mixins/MSignatureValidator.sol"; import "./mixins/MTransactions.sol"; contract MixinExchangeCore is - SafeMath, LibMath, - LibStatus, LibOrder, LibFillResults, LibExchangeErrors, @@ -58,13 +55,21 @@ contract MixinExchangeCore is function cancelOrdersUpTo(uint256 salt) external { - uint256 newMakerEpoch = salt + 1; // makerEpoch is initialized to 0, so to cancelUpTo we need salt + 1 + address makerAddress = getCurrentContextAddress(); + + // makerEpoch is initialized to 0, so to cancelUpTo we need salt + 1 + uint256 newMakerEpoch = salt + 1; + uint256 oldMakerEpoch = makerEpoch[makerAddress]; + + // Ensure makerEpoch is monotonically increasing require( - newMakerEpoch > makerEpoch[msg.sender], // epoch must be monotonically increasing + newMakerEpoch > oldMakerEpoch, INVALID_NEW_MAKER_EPOCH ); - makerEpoch[msg.sender] = newMakerEpoch; - emit CancelUpTo(msg.sender, newMakerEpoch); + + // Update makerEpoch + makerEpoch[makerAddress] = newMakerEpoch; + emit CancelUpTo(makerAddress, newMakerEpoch); } /// @dev Fills the input order. @@ -86,29 +91,22 @@ contract MixinExchangeCore is // Fetch taker address address takerAddress = getCurrentContextAddress(); - // Either our context is valid or we revert + // Get amount of takerAsset to fill + uint256 remainingTakerAssetAmount = safeSub(order.takerAssetAmount, orderInfo.orderTakerAssetFilledAmount); + uint256 takerAssetFilledAmount = min256(takerAssetFillAmount, remainingTakerAssetAmount); + + // Validate context assertValidFill( order, - orderInfo.orderStatus, - orderInfo.orderHash, + orderInfo, takerAddress, - orderInfo.orderTakerAssetFilledAmount, takerAssetFillAmount, + takerAssetFilledAmount, signature ); // Compute proportional fill amounts - uint8 status; - (status, fillResults) = calculateFillResults( - order, - orderInfo.orderStatus, - orderInfo.orderTakerAssetFilledAmount, - takerAssetFillAmount - ); - if (status != uint8(Status.SUCCESS)) { - emit ExchangeStatus(uint8(status), orderInfo.orderHash); - return getNullFillResults(); - } + fillResults = calculateFillResults(order, takerAssetFilledAmount); // Settle order settleOrder(order, takerAddress, fillResults); @@ -126,32 +124,28 @@ contract MixinExchangeCore is /// @dev After calling, the order can not be filled anymore. /// Throws if order is invalid or sender does not have permission to cancel. - /// @param order Order to cancel. Order must be Status.FILLABLE. - /// @return True if the order state changed to cancelled. - /// False if the order was valid, but in an - /// unfillable state (see LibStatus.STATUS for order states) + /// @param order Order to cancel. Order must be OrderStatus.FILLABLE. function cancelOrder(Order memory order) public - returns (bool) { // Fetch current order status OrderInfo memory orderInfo = getOrderInfo(order); // Validate context - assertValidCancel(order, orderInfo.orderStatus, orderInfo.orderHash); + assertValidCancel(order, orderInfo); // Perform cancel - return updateCancelledState(order, orderInfo.orderStatus, orderInfo.orderHash); + updateCancelledState(order, orderInfo.orderHash); } /// @dev Gets information about an order: status, hash, and amount filled. /// @param order Order to gather information on. /// @return OrderInfo Information about the order and its state. - /// See LibOrder.OrderInfo for a complete description. + /// See LibOrder.OrderInfo for a complete description. function getOrderInfo(Order memory order) public view - returns (LibOrder.OrderInfo memory orderInfo) + returns (OrderInfo memory orderInfo) { // Compute the order hash orderInfo.orderHash = getOrderHash(order); @@ -161,7 +155,7 @@ contract MixinExchangeCore is // edge cases in the supporting infrastructure because they have // an 'infinite' price when computed by a simple division. if (order.makerAssetAmount == 0) { - orderInfo.orderStatus = uint8(Status.ORDER_INVALID_MAKER_ASSET_AMOUNT); + orderInfo.orderStatus = uint8(OrderStatus.INVALID_MAKER_ASSET_AMOUNT); return orderInfo; } @@ -170,148 +164,124 @@ contract MixinExchangeCore is // Instead of distinguishing between unfilled and filled zero taker // amount orders, we choose not to support them. if (order.takerAssetAmount == 0) { - orderInfo.orderStatus = uint8(Status.ORDER_INVALID_TAKER_ASSET_AMOUNT); + orderInfo.orderStatus = uint8(OrderStatus.INVALID_TAKER_ASSET_AMOUNT); return orderInfo; } // Validate order expiration if (block.timestamp >= order.expirationTimeSeconds) { - orderInfo.orderStatus = uint8(Status.ORDER_EXPIRED); + orderInfo.orderStatus = uint8(OrderStatus.EXPIRED); return orderInfo; } // Check if order has been cancelled if (cancelled[orderInfo.orderHash]) { - orderInfo.orderStatus = uint8(Status.ORDER_CANCELLED); + orderInfo.orderStatus = uint8(OrderStatus.CANCELLED); return orderInfo; } if (makerEpoch[order.makerAddress] > order.salt) { - orderInfo.orderStatus = uint8(Status.ORDER_CANCELLED); + orderInfo.orderStatus = uint8(OrderStatus.CANCELLED); return orderInfo; } // Fetch filled amount and validate order availability orderInfo.orderTakerAssetFilledAmount = filled[orderInfo.orderHash]; if (orderInfo.orderTakerAssetFilledAmount >= order.takerAssetAmount) { - orderInfo.orderStatus = uint8(Status.ORDER_FULLY_FILLED); + orderInfo.orderStatus = uint8(OrderStatus.FULLY_FILLED); return orderInfo; } // All other statuses are ruled out: order is Fillable - orderInfo.orderStatus = uint8(Status.ORDER_FILLABLE); + orderInfo.orderStatus = uint8(OrderStatus.FILLABLE); return orderInfo; } - /// @dev Calculates amounts filled and fees paid by maker and taker. - /// @param order to be filled. - /// @param orderStatus Status of order to be filled. + /// @dev Updates state with results of a fill order. + /// @param order that was filled. + /// @param takerAddress Address of taker who filled the order. /// @param orderTakerAssetFilledAmount Amount of order already filled. - /// @param takerAssetFillAmount Desired amount of order to fill by taker. - /// @return status Return status of calculating fill amounts. Returns Status.SUCCESS on success. /// @return fillResults Amounts filled and fees paid by maker and taker. - function calculateFillResults( + function updateFilledState( Order memory order, - uint8 orderStatus, + address takerAddress, + bytes32 orderHash, uint256 orderTakerAssetFilledAmount, - uint256 takerAssetFillAmount + FillResults memory fillResults ) - public - pure - returns ( - uint8 status, - FillResults memory fillResults - ) + internal { - // Fill amount must be greater than 0 - if (takerAssetFillAmount == 0) { - status = uint8(Status.TAKER_ASSET_FILL_AMOUNT_TOO_LOW); - return (status, fillResults); - } - - // Ensure the order is fillable - if (orderStatus != uint8(Status.ORDER_FILLABLE)) { - status = orderStatus; - return (status, fillResults); - } - - // Compute takerAssetFilledAmount - uint256 remainingTakerAssetAmount = safeSub(order.takerAssetAmount, orderTakerAssetFilledAmount); - uint256 takerAssetFilledAmount = min256(takerAssetFillAmount, remainingTakerAssetAmount); - - // Validate fill order rounding - if (isRoundingError( - takerAssetFilledAmount, - order.takerAssetAmount, - order.makerAssetAmount)) - { - status = uint8(Status.ROUNDING_ERROR_TOO_LARGE); - return (status, fillResults); - } + // Update state + filled[orderHash] = safeAdd(orderTakerAssetFilledAmount, fillResults.takerAssetFilledAmount); - // Compute proportional transfer amounts - // TODO: All three are multiplied by the same fraction. This can - // potentially be optimized. - fillResults.takerAssetFilledAmount = takerAssetFilledAmount; - fillResults.makerAssetFilledAmount = getPartialAmount( - fillResults.takerAssetFilledAmount, - order.takerAssetAmount, - order.makerAssetAmount - ); - fillResults.makerFeePaid = getPartialAmount( - fillResults.takerAssetFilledAmount, - order.takerAssetAmount, - order.makerFee - ); - fillResults.takerFeePaid = getPartialAmount( + // Log order + emit Fill( + order.makerAddress, + takerAddress, + order.feeRecipientAddress, + fillResults.makerAssetFilledAmount, fillResults.takerAssetFilledAmount, - order.takerAssetAmount, - order.takerFee + fillResults.makerFeePaid, + fillResults.takerFeePaid, + orderHash, + order.makerAssetData, + order.takerAssetData ); + } + + /// @dev Updates state with results of cancelling an order. + /// State is only updated if the order is currently fillable. + /// Otherwise, updating state would have no effect. + /// @param order that was cancelled. + /// @param orderHash Hash of order that was cancelled. + function updateCancelledState( + Order memory order, + bytes32 orderHash + ) + internal + { + // Perform cancel + cancelled[orderHash] = true; - status = uint8(Status.SUCCESS); - return (status, fillResults); + // Log cancel + emit Cancel( + order.makerAddress, + order.feeRecipientAddress, + orderHash, + order.makerAssetData, + order.takerAssetData + ); } /// @dev Validates context for fillOrder. Succeeds or throws. /// @param order to be filled. - /// @param orderStatus Status of order to be filled. - /// @param orderHash Hash of order to be filled. + /// @param orderInfo OrderStatus, orderHash, and amount already filled of order. /// @param takerAddress Address of order taker. - /// @param orderTakerAssetFilledAmount Amount of order already filled. /// @param takerAssetFillAmount Desired amount of order to fill by taker. + /// @param takerAssetFilledAmount Amount of takerAsset that will be filled. /// @param signature Proof that the orders was created by its maker. function assertValidFill( Order memory order, - uint8 orderStatus, - bytes32 orderHash, + OrderInfo memory orderInfo, address takerAddress, - uint256 orderTakerAssetFilledAmount, uint256 takerAssetFillAmount, + uint256 takerAssetFilledAmount, bytes memory signature ) internal + view { - // Ensure order is valid - // An order can only be filled if its status is FILLABLE; - // however, only invalid statuses result in a throw. - // See LibStatus for a complete description of order statuses. + // An order can only be filled if its status is FILLABLE. require( - orderStatus != uint8(Status.ORDER_INVALID_MAKER_ASSET_AMOUNT), - INVALID_ORDER_MAKER_ASSET_AMOUNT + orderInfo.orderStatus == uint8(OrderStatus.FILLABLE), + ORDER_UNFILLABLE ); + + // Revert if fill amount is invalid require( - orderStatus != uint8(Status.ORDER_INVALID_TAKER_ASSET_AMOUNT), - INVALID_ORDER_TAKER_ASSET_AMOUNT + takerAssetFillAmount != 0, + INVALID_TAKER_AMOUNT ); - // Validate Maker signature (check only if first time seen) - if (orderTakerAssetFilledAmount == 0) { - require( - isValidSignature(orderHash, order.makerAddress, signature), - SIGNATURE_VALIDATION_FAILED - ); - } - // Validate sender is allowed to fill this order if (order.senderAddress != address(0)) { require( @@ -324,76 +294,44 @@ contract MixinExchangeCore is if (order.takerAddress != address(0)) { require( order.takerAddress == takerAddress, - INVALID_CONTEXT + INVALID_TAKER ); } - require( - takerAssetFillAmount > 0, - GT_ZERO_AMOUNT_REQUIRED - ); - } - /// @dev Updates state with results of a fill order. - /// @param order that was filled. - /// @param takerAddress Address of taker who filled the order. - /// @param orderTakerAssetFilledAmount Amount of order already filled. - /// @return fillResults Amounts filled and fees paid by maker and taker. - function updateFilledState( - Order memory order, - address takerAddress, - bytes32 orderHash, - uint256 orderTakerAssetFilledAmount, - FillResults memory fillResults - ) - internal - { - // Update state - filled[orderHash] = safeAdd(orderTakerAssetFilledAmount, fillResults.takerAssetFilledAmount); + // Validate Maker signature (check only if first time seen) + if (orderInfo.orderTakerAssetFilledAmount == 0) { + require( + isValidSignature(orderInfo.orderHash, order.makerAddress, signature), + INVALID_ORDER_SIGNATURE + ); + } - // Log order - emit Fill( - order.makerAddress, - takerAddress, - order.feeRecipientAddress, - fillResults.makerAssetFilledAmount, - fillResults.takerAssetFilledAmount, - fillResults.makerFeePaid, - fillResults.takerFeePaid, - orderHash, - order.makerAssetData, - order.takerAssetData + // Validate fill order rounding + require( + !isRoundingError( + takerAssetFilledAmount, + order.takerAssetAmount, + order.makerAssetAmount + ), + ROUNDING_ERROR ); } /// @dev Validates context for cancelOrder. Succeeds or throws. - /// @param order that was cancelled. - /// @param orderStatus Status of order that was cancelled. - /// @param orderHash Hash of order that was cancelled. + /// @param order to be cancelled. + /// @param orderInfo OrderStatus, orderHash, and amount already filled of order. function assertValidCancel( Order memory order, - uint8 orderStatus, - bytes32 orderHash + OrderInfo memory orderInfo ) internal + view { // Ensure order is valid - // An order can only be cancelled if its status is FILLABLE; - // however, only invalid statuses result in a throw. - // See LibStatus for a complete description of order statuses. - require( - orderStatus != uint8(Status.ORDER_INVALID_MAKER_ASSET_AMOUNT), - INVALID_ORDER_MAKER_ASSET_AMOUNT - ); + // An order can only be cancelled if its status is FILLABLE. require( - orderStatus != uint8(Status.ORDER_INVALID_TAKER_ASSET_AMOUNT), - INVALID_ORDER_TAKER_ASSET_AMOUNT - ); - - // Validate transaction signed by maker - address makerAddress = getCurrentContextAddress(); - require( - order.makerAddress == makerAddress, - INVALID_CONTEXT + orderInfo.orderStatus == uint8(OrderStatus.FILLABLE), + ORDER_UNFILLABLE ); // Validate sender is allowed to cancel this order @@ -403,44 +341,47 @@ contract MixinExchangeCore is INVALID_SENDER ); } + + // Validate transaction signed by maker + address makerAddress = getCurrentContextAddress(); + require( + order.makerAddress == makerAddress, + INVALID_MAKER + ); } - /// @dev Updates state with results of cancelling an order. - /// State is only updated if the order is currently fillable. - /// Otherwise, updating state would have no effect. - /// @param order that was cancelled. - /// @param orderStatus Status of order that was cancelled. - /// @param orderHash Hash of order that was cancelled. - /// @return stateUpdated Returns true only if state was updated. - function updateCancelledState( + /// @dev Calculates amounts filled and fees paid by maker and taker. + /// @param order to be filled. + /// @param takerAssetFilledAmount Amount of takerAsset that will be filled. + /// @return fillResults Amounts filled and fees paid by maker and taker. + function calculateFillResults( Order memory order, - uint8 orderStatus, - bytes32 orderHash + uint256 takerAssetFilledAmount ) internal - returns (bool stateUpdated) + pure + returns (FillResults memory fillResults) { - // Ensure order is fillable (otherwise cancelling does nothing) - // See LibStatus for a complete description of order statuses. - if (orderStatus != uint8(Status.ORDER_FILLABLE)) { - emit ExchangeStatus(uint8(orderStatus), orderHash); - stateUpdated = false; - return stateUpdated; - } - - // Perform cancel - cancelled[orderHash] = true; - stateUpdated = true; - - // Log cancel - emit Cancel( - order.makerAddress, - order.feeRecipientAddress, - orderHash, - order.makerAssetData, - order.takerAssetData + // Compute proportional transfer amounts + // TODO: All three are multiplied by the same fraction. This can + // potentially be optimized. + fillResults.takerAssetFilledAmount = takerAssetFilledAmount; + fillResults.makerAssetFilledAmount = getPartialAmount( + fillResults.takerAssetFilledAmount, + order.takerAssetAmount, + order.makerAssetAmount + ); + fillResults.makerFeePaid = getPartialAmount( + fillResults.takerAssetFilledAmount, + order.takerAssetAmount, + order.makerFee + ); + fillResults.takerFeePaid = getPartialAmount( + fillResults.takerAssetFilledAmount, + order.takerAssetAmount, + order.takerFee ); - return stateUpdated; + return fillResults; } } diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol index 9d8b521c0..ed76287e0 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol @@ -14,24 +14,19 @@ pragma solidity ^0.4.24; pragma experimental ABIEncoderV2; +import "../../utils/LibBytes/LibBytes.sol"; +import "./libs/LibMath.sol"; +import "./libs/LibOrder.sol"; +import "./libs/LibFillResults.sol"; +import "./libs/LibExchangeErrors.sol"; import "./mixins/MExchangeCore.sol"; import "./mixins/MMatchOrders.sol"; import "./mixins/MSettlement.sol"; import "./mixins/MTransactions.sol"; -import "../../utils/SafeMath/SafeMath.sol"; -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, LibBytes, LibMath, - LibStatus, - LibOrder, - LibFillResults, LibExchangeErrors, MExchangeCore, MMatchOrders, @@ -50,17 +45,22 @@ contract MixinMatchOrders is /// @return matchedFillResults Amounts filled and fees paid by maker and taker of matched orders. /// TODO: Make this function external once supported by Solidity (See Solidity Issues #3199, #1603) function matchOrders( - Order memory leftOrder, - Order memory rightOrder, + LibOrder.Order memory leftOrder, + LibOrder.Order memory rightOrder, bytes memory leftSignature, bytes memory rightSignature ) public - returns (MatchedFillResults memory matchedFillResults) + returns (LibFillResults.MatchedFillResults memory matchedFillResults) { + // We assume that rightOrder.takerAssetData == leftOrder.makerAssetData and rightOrder.makerAssetData == leftOrder.takerAssetData. + // If this assumption isn't true, the match will fail at signature validation. + rightOrder.makerAssetData = leftOrder.takerAssetData; + rightOrder.takerAssetData = leftOrder.makerAssetData; + // Get left & right order info - OrderInfo memory leftOrderInfo = getOrderInfo(leftOrder); - OrderInfo memory rightOrderInfo = getOrderInfo(rightOrder); + LibOrder.OrderInfo memory leftOrderInfo = getOrderInfo(leftOrder); + LibOrder.OrderInfo memory rightOrderInfo = getOrderInfo(rightOrder); // Fetch taker address address takerAddress = getCurrentContextAddress(); @@ -72,8 +72,6 @@ contract MixinMatchOrders is matchedFillResults = calculateMatchedFillResults( leftOrder, rightOrder, - leftOrderInfo.orderStatus, - rightOrderInfo.orderStatus, leftOrderInfo.orderTakerAssetFilledAmount, rightOrderInfo.orderTakerAssetFilledAmount ); @@ -81,19 +79,17 @@ contract MixinMatchOrders is // Validate fill contexts assertValidFill( leftOrder, - leftOrderInfo.orderStatus, - leftOrderInfo.orderHash, + leftOrderInfo, takerAddress, - leftOrderInfo.orderTakerAssetFilledAmount, + matchedFillResults.left.takerAssetFilledAmount, matchedFillResults.left.takerAssetFilledAmount, leftSignature ); assertValidFill( rightOrder, - rightOrderInfo.orderStatus, - rightOrderInfo.orderHash, + rightOrderInfo, takerAddress, - rightOrderInfo.orderTakerAssetFilledAmount, + matchedFillResults.right.takerAssetFilledAmount, matchedFillResults.right.takerAssetFilledAmount, rightSignature ); @@ -129,25 +125,12 @@ contract MixinMatchOrders is /// @param leftOrder First order to match. /// @param rightOrder Second order to match. function assertValidMatch( - Order memory leftOrder, - Order memory rightOrder + LibOrder.Order memory leftOrder, + LibOrder.Order memory rightOrder ) internal + pure { - // The leftOrder maker asset must be the same as the rightOrder taker asset. - // TODO: Can we safely assume equality and expect a later failure otherwise? - require( - areBytesEqual(leftOrder.makerAssetData, rightOrder.takerAssetData), - ASSET_MISMATCH_MAKER_TAKER - ); - - // The leftOrder taker asset must be the same as the rightOrder maker asset. - // TODO: Can we safely assume equality and expect a later failure otherwise? - require( - areBytesEqual(leftOrder.takerAssetData, rightOrder.makerAssetData), - ASSET_MISMATCH_TAKER_MAKER - ); - // 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 // than the profit per unit sold of the matched order (OrderB.TakerAmount/OrderB.MakerAmount). @@ -159,39 +142,7 @@ contract MixinMatchOrders is require( safeMul(leftOrder.makerAssetAmount, rightOrder.makerAssetAmount) >= safeMul(leftOrder.takerAssetAmount, rightOrder.takerAssetAmount), - NEGATIVE_SPREAD - ); - } - - /// @dev Validates matched fill results. Succeeds or throws. - /// @param matchedFillResults Amounts to fill and fees to pay by maker and taker of matched orders. - function assertValidMatchResults(MatchedFillResults memory matchedFillResults) - internal - { - // If the amount transferred from the left 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. - uint256 amountSpentByLeft = safeAdd( - matchedFillResults.right.takerAssetFilledAmount, - matchedFillResults.takerFillAmount - ); - require( - !isRoundingError( - matchedFillResults.left.makerAssetFilledAmount, - amountSpentByLeft, - 1 - ), - ROUNDING_ERROR_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( - !isRoundingError( - matchedFillResults.right.makerAssetFilledAmount, - matchedFillResults.left.takerAssetFilledAmount, - 1 - ), - ROUNDING_ERROR_TRANSFER_AMOUNTS + NEGATIVE_SPREAD_REQUIRED ); } @@ -201,21 +152,18 @@ contract MixinMatchOrders is /// The profit made by the leftOrder order goes to the taker (who matched the two orders). /// @param leftOrder First order to match. /// @param rightOrder Second order to match. - /// @param leftOrderStatus Order status of left order. - /// @param rightOrderStatus Order status of right order. - /// @param leftOrderFilledAmount Amount of left order already filled. - /// @param rightOrderFilledAmount Amount of right order already filled. + /// @param leftOrderTakerAssetFilledAmount Amount of left order already filled. + /// @param rightOrderTakerAssetFilledAmount Amount of right order already filled. /// @param matchedFillResults Amounts to fill and fees to pay by maker and taker of matched orders. function calculateMatchedFillResults( - Order memory leftOrder, - Order memory rightOrder, - uint8 leftOrderStatus, - uint8 rightOrderStatus, - uint256 leftOrderFilledAmount, - uint256 rightOrderFilledAmount + LibOrder.Order memory leftOrder, + LibOrder.Order memory rightOrder, + uint256 leftOrderTakerAssetFilledAmount, + uint256 rightOrderTakerAssetFilledAmount ) internal - returns (MatchedFillResults memory matchedFillResults) + pure + returns (LibFillResults.MatchedFillResults memory matchedFillResults) { // We settle orders at the exchange rate of the right order. // The amount saved by the left maker goes to the taker. @@ -226,71 +174,55 @@ contract MixinMatchOrders is // <= * // <= * / // * <= * - uint256 rightTakerAssetAmountRemaining = safeSub(rightOrder.takerAssetAmount, rightOrderFilledAmount); - uint256 leftTakerAssetAmountRemaining = safeSub(leftOrder.takerAssetAmount, leftOrderFilledAmount); - uint256 leftOrderAmountToFill; - uint256 rightOrderAmountToFill; + uint256 leftTakerAssetAmountRemaining = safeSub(leftOrder.takerAssetAmount, leftOrderTakerAssetFilledAmount); + uint256 rightTakerAssetAmountRemaining = safeSub(rightOrder.takerAssetAmount, rightOrderTakerAssetFilledAmount); + uint256 leftTakerAssetFilledAmount; + uint256 rightTakerAssetFilledAmount; if ( safeMul(leftTakerAssetAmountRemaining, rightOrder.takerAssetAmount) <= safeMul(rightTakerAssetAmountRemaining, rightOrder.makerAssetAmount) ) { // Left order will be fully filled: maximally fill left - leftOrderAmountToFill = leftTakerAssetAmountRemaining; + leftTakerAssetFilledAmount = leftTakerAssetAmountRemaining; // The right order receives an amount proportional to how much was spent. // TODO: Can we ensure rounding error is in the correct direction? - rightOrderAmountToFill = safeGetPartialAmount( + rightTakerAssetFilledAmount = getPartialAmount( rightOrder.takerAssetAmount, rightOrder.makerAssetAmount, - leftOrderAmountToFill + leftTakerAssetFilledAmount ); } else { // Right order will be fully filled: maximally fill right - rightOrderAmountToFill = rightTakerAssetAmountRemaining; + rightTakerAssetFilledAmount = rightTakerAssetAmountRemaining; // The left order receives an amount proportional to how much was spent. // TODO: Can we ensure rounding error is in the correct direction? - leftOrderAmountToFill = safeGetPartialAmount( + leftTakerAssetFilledAmount = getPartialAmount( rightOrder.makerAssetAmount, rightOrder.takerAssetAmount, - rightOrderAmountToFill + rightTakerAssetFilledAmount ); } // Calculate fill results for left order - uint8 status; - (status, matchedFillResults.left) = calculateFillResults( + matchedFillResults.left = calculateFillResults( leftOrder, - leftOrderStatus, - leftOrderFilledAmount, - leftOrderAmountToFill - ); - require( - status == uint8(Status.SUCCESS), - FAILED_TO_CALCULATE_FILL_RESULTS_FOR_LEFT_ORDER + leftTakerAssetFilledAmount ); // Calculate fill results for right order - (status, matchedFillResults.right) = calculateFillResults( + matchedFillResults.right = calculateFillResults( rightOrder, - rightOrderStatus, - rightOrderFilledAmount, - rightOrderAmountToFill - ); - require( - status == uint8(Status.SUCCESS), - FAILED_TO_CALCULATE_FILL_RESULTS_FOR_RIGHT_ORDER + rightTakerAssetFilledAmount ); // Calculate amount given to taker - matchedFillResults.takerFillAmount = safeSub( + matchedFillResults.leftMakerAssetSpreadAmount = safeSub( matchedFillResults.left.makerAssetFilledAmount, matchedFillResults.right.takerAssetFilledAmount ); - // Validate the fill results - assertValidMatchResults(matchedFillResults); - // Return fill results return matchedFillResults; } diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSettlement.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSettlement.sol index 7c03bde75..646d3ed58 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSettlement.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSettlement.sol @@ -19,17 +19,16 @@ pragma solidity ^0.4.24; pragma experimental ABIEncoderV2; -import "./mixins/MSettlement.sol"; -import "./mixins/MAssetProxyDispatcher.sol"; -import "./libs/LibOrder.sol"; import "./libs/LibMath.sol"; -import "./libs/LibExchangeErrors.sol"; import "./libs/LibFillResults.sol"; +import "./libs/LibOrder.sol"; +import "./libs/LibExchangeErrors.sol"; import "./mixins/MMatchOrders.sol"; +import "./mixins/MSettlement.sol"; +import "./mixins/MAssetProxyDispatcher.sol"; contract MixinSettlement is LibMath, - LibFillResults, LibExchangeErrors, MMatchOrders, MSettlement, @@ -65,7 +64,7 @@ contract MixinSettlement is function settleOrder( LibOrder.Order memory order, address takerAddress, - FillResults memory fillResults + LibFillResults.FillResults memory fillResults ) internal { @@ -104,7 +103,7 @@ contract MixinSettlement is LibOrder.Order memory leftOrder, LibOrder.Order memory rightOrder, address takerAddress, - MatchedFillResults memory matchedFillResults + LibFillResults.MatchedFillResults memory matchedFillResults ) internal { @@ -125,7 +124,7 @@ contract MixinSettlement is leftOrder.makerAssetData, leftOrder.makerAddress, takerAddress, - matchedFillResults.takerFillAmount + matchedFillResults.leftMakerAssetSpreadAmount ); // Maker fees diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol index 79dc87af0..ae7d053fd 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol @@ -18,12 +18,12 @@ pragma solidity ^0.4.24; +import "../../utils/LibBytes/LibBytes.sol"; +import "./libs/LibExchangeErrors.sol"; import "./mixins/MSignatureValidator.sol"; import "./mixins/MTransactions.sol"; import "./interfaces/IWallet.sol"; import "./interfaces/IValidator.sol"; -import "./libs/LibExchangeErrors.sol"; -import "../../utils/LibBytes/LibBytes.sol"; contract MixinSignatureValidator is LibBytes, @@ -31,6 +31,9 @@ contract MixinSignatureValidator is MSignatureValidator, MTransactions { + // Personal message headers + string constant ETH_PERSONAL_MESSAGE = "\x19Ethereum Signed Message:\n32"; + string constant TREZOR_PERSONAL_MESSAGE = "\x19Ethereum Signed Message:\n\x41"; // Mapping of hash => signer => signed mapping (bytes32 => mapping (address => bool)) public preSigned; @@ -51,7 +54,7 @@ contract MixinSignatureValidator is { require( isValidSignature(hash, signer, signature), - SIGNATURE_VALIDATION_FAILED + INVALID_SIGNATURE ); preSigned[hash][signer] = true; } @@ -85,8 +88,8 @@ contract MixinSignatureValidator is { // TODO: Domain separation: make hash depend on role. (Taker sig should not be valid as maker sig, etc.) require( - signature.length >= 1, - INVALID_SIGNATURE_LENGTH + signature.length > 0, + LENGTH_GT_0_REQUIRED ); // Pop last byte off of signature byte array. @@ -104,7 +107,7 @@ 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(ILLEGAL_SIGNATURE_TYPE); + revert(SIGNATURE_ILLEGAL); // Always invalid signature. // Like Illegal, this is always implicitly available and therefore @@ -113,7 +116,7 @@ contract MixinSignatureValidator is } else if (signatureType == SignatureType.Invalid) { require( signature.length == 0, - INVALID_SIGNATURE_LENGTH + LENGTH_0_REQUIRED ); isValid = false; return isValid; @@ -122,7 +125,7 @@ contract MixinSignatureValidator is } else if (signatureType == SignatureType.EIP712) { require( signature.length == 65, - INVALID_SIGNATURE_LENGTH + LENGTH_65_REQUIRED ); v = uint8(signature[0]); r = readBytes32(signature, 1); @@ -135,13 +138,13 @@ contract MixinSignatureValidator is } else if (signatureType == SignatureType.EthSign) { require( signature.length == 65, - INVALID_SIGNATURE_LENGTH + LENGTH_65_REQUIRED ); v = uint8(signature[0]); r = readBytes32(signature, 1); s = readBytes32(signature, 33); recovered = ecrecover( - keccak256("\x19Ethereum Signed Message:\n32", hash), + keccak256(abi.encodePacked(ETH_PERSONAL_MESSAGE, hash)), v, r, s @@ -160,7 +163,7 @@ contract MixinSignatureValidator is } else if (signatureType == SignatureType.Caller) { require( signature.length == 0, - INVALID_SIGNATURE_LENGTH + LENGTH_0_REQUIRED ); isValid = signer == msg.sender; return isValid; @@ -208,13 +211,13 @@ contract MixinSignatureValidator is } else if (signatureType == SignatureType.Trezor) { require( signature.length == 65, - INVALID_SIGNATURE_LENGTH + LENGTH_65_REQUIRED ); v = uint8(signature[0]); r = readBytes32(signature, 1); s = readBytes32(signature, 33); recovered = ecrecover( - keccak256("\x19Ethereum Signed Message:\n\x41", hash), + keccak256(abi.encodePacked(TREZOR_PERSONAL_MESSAGE, hash)), v, r, s @@ -233,6 +236,6 @@ 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(UNSUPPORTED_SIGNATURE_TYPE); + revert(SIGNATURE_UNSUPPORTED); } } diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol index d153dfa5c..30b0102fd 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol @@ -17,9 +17,9 @@ */ pragma solidity ^0.4.24; +import "./libs/LibExchangeErrors.sol"; import "./mixins/MSignatureValidator.sol"; import "./mixins/MTransactions.sol"; -import "./libs/LibExchangeErrors.sol"; contract MixinTransactions is LibExchangeErrors, @@ -50,29 +50,29 @@ contract MixinTransactions is // Prevent reentrancy require( currentContextAddress == address(0), - REENTRANCY_NOT_ALLOWED + REENTRANCY_ILLEGAL ); // Calculate transaction hash - bytes32 transactionHash = keccak256( + bytes32 transactionHash = keccak256(abi.encodePacked( address(this), signer, salt, data - ); + )); // Validate transaction has not been executed require( !transactions[transactionHash], - DUPLICATE_TRANSACTION_HASH + INVALID_TX_HASH ); - // TODO: is SignatureType.Caller necessary if we make this check? + // Transaction always valid if signer is sender of transaction if (signer != msg.sender) { // Validate signature require( isValidSignature(transactionHash, signer, signature), - SIGNATURE_VALIDATION_FAILED + INVALID_TX_SIGNATURE ); // Set the current transaction signer @@ -83,7 +83,7 @@ contract MixinTransactions is transactions[transactionHash] = true; require( address(this).delegatecall(data), - TRANSACTION_EXECUTION_FAILED + FAILED_EXECUTION ); // Reset current transaction signer diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol index 15f1a2e0b..0ad0710ce 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol @@ -20,17 +20,15 @@ pragma solidity ^0.4.24; pragma experimental ABIEncoderV2; import "../../utils/LibBytes/LibBytes.sol"; -import "./mixins/MExchangeCore.sol"; import "./libs/LibMath.sol"; import "./libs/LibOrder.sol"; import "./libs/LibFillResults.sol"; import "./libs/LibExchangeErrors.sol"; +import "./mixins/MExchangeCore.sol"; contract MixinWrapperFunctions is - SafeMath, LibBytes, LibMath, - LibOrder, LibFillResults, LibExchangeErrors, MExchangeCore @@ -40,7 +38,7 @@ contract MixinWrapperFunctions is /// @param takerAssetFillAmount Desired amount of takerAsset to sell. /// @param signature Proof that order has been created by maker. function fillOrKillOrder( - Order memory order, + LibOrder.Order memory order, uint256 takerAssetFillAmount, bytes memory signature) public @@ -65,7 +63,7 @@ contract MixinWrapperFunctions is /// @param signature Proof that order has been created by maker. /// @return Amounts filled and fees paid by maker and taker. function fillOrderNoThrow( - Order memory order, + LibOrder.Order memory order, uint256 takerAssetFillAmount, bytes memory signature) public @@ -264,7 +262,7 @@ contract MixinWrapperFunctions is /// @param takerAssetFillAmounts Array of desired amounts of takerAsset to sell in orders. /// @param signatures Proofs that orders have been created by makers. function batchFillOrders( - Order[] memory orders, + LibOrder.Order[] memory orders, uint256[] memory takerAssetFillAmounts, bytes[] memory signatures) public @@ -283,7 +281,7 @@ contract MixinWrapperFunctions is /// @param takerAssetFillAmounts Array of desired amounts of takerAsset to sell in orders. /// @param signatures Proofs that orders have been created by makers. function batchFillOrKillOrders( - Order[] memory orders, + LibOrder.Order[] memory orders, uint256[] memory takerAssetFillAmounts, bytes[] memory signatures) public @@ -303,7 +301,7 @@ contract MixinWrapperFunctions is /// @param takerAssetFillAmounts Array of desired amounts of takerAsset to sell in orders. /// @param signatures Proofs that orders have been created by makers. function batchFillOrdersNoThrow( - Order[] memory orders, + LibOrder.Order[] memory orders, uint256[] memory takerAssetFillAmounts, bytes[] memory signatures) public @@ -323,7 +321,7 @@ contract MixinWrapperFunctions is /// @param signatures Proofs that orders have been created by makers. /// @return Amounts filled and fees paid by makers and taker. function marketSellOrders( - Order[] memory orders, + LibOrder.Order[] memory orders, uint256 takerAssetFillAmount, bytes[] memory signatures) public @@ -366,7 +364,7 @@ contract MixinWrapperFunctions is /// @param signatures Proofs that orders have been signed by makers. /// @return Amounts filled and fees paid by makers and taker. function marketSellOrdersNoThrow( - Order[] memory orders, + LibOrder.Order[] memory orders, uint256 takerAssetFillAmount, bytes[] memory signatures) public @@ -408,7 +406,7 @@ contract MixinWrapperFunctions is /// @param signatures Proofs that orders have been signed by makers. /// @return Amounts filled and fees paid by makers and taker. function marketBuyOrders( - Order[] memory orders, + LibOrder.Order[] memory orders, uint256 makerAssetFillAmount, bytes[] memory signatures) public @@ -459,7 +457,7 @@ contract MixinWrapperFunctions is /// @param signatures Proofs that orders have been signed by makers. /// @return Amounts filled and fees paid by makers and taker. function marketBuyOrdersNoThrow( - Order[] memory orders, + LibOrder.Order[] memory orders, uint256 makerAssetFillAmount, bytes[] memory signatures) public @@ -505,7 +503,7 @@ contract MixinWrapperFunctions is /// @dev Synchronously cancels multiple orders in a single transaction. /// @param orders Array of order specifications. - function batchCancelOrders(Order[] memory orders) + function batchCancelOrders(LibOrder.Order[] memory orders) public { for (uint256 i = 0; i < orders.length; i++) { diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IExchangeCore.sol b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IExchangeCore.sol index fc0157d75..7ca2dd052 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IExchangeCore.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IExchangeCore.sol @@ -37,17 +37,15 @@ contract IExchangeCore { function fillOrder( LibOrder.Order memory order, uint256 takerAssetFillAmount, - bytes memory signature) + bytes memory signature + ) public returns (LibFillResults.FillResults memory fillResults); /// @dev After calling, the order can not be filled anymore. /// @param order Order struct containing order specifications. - /// @return True if the order state changed to cancelled. - /// False if the transaction was already cancelled or expired. function cancelOrder(LibOrder.Order memory order) - public - returns (bool); + public; /// @dev Gets information about an order: status, hash, and amount filled. /// @param order Order to gather information on. @@ -57,24 +55,4 @@ contract IExchangeCore { public view returns (LibOrder.OrderInfo memory orderInfo); - - /// @dev Calculates amounts filled and fees paid by maker and taker. - /// @param order to be filled. - /// @param orderStatus Status of order to be filled. - /// @param orderTakerAssetFilledAmount Amount of order already filled. - /// @param takerAssetFillAmount Desired amount of order to fill by taker. - /// @return status Return status of calculating fill amounts. Returns Status.SUCCESS on success. - /// @return fillResults Amounts filled and fees paid by maker and taker. - function calculateFillResults( - LibOrder.Order memory order, - uint8 orderStatus, - uint256 orderTakerAssetFilledAmount, - uint256 takerAssetFillAmount - ) - public - pure - returns ( - uint8 status, - LibFillResults.FillResults memory fillResults - ); } 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 7d67e5080..8b2e3bb35 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol @@ -19,43 +19,44 @@ pragma solidity ^0.4.24; contract LibExchangeErrors { - - // Core revert reasons - string constant GT_ZERO_AMOUNT_REQUIRED = "Amount must be greater than 0."; - string constant SIGNATURE_VALIDATION_FAILED = "Signature validation failed."; - string constant INVALID_SENDER = "Invalid `msg.sender`."; - string constant INVALID_CONTEXT = "Function called in an invalid context."; - string constant INVALID_NEW_MAKER_EPOCH = "Specified salt must be greater than or equal to existing makerEpoch."; - - // Order revert reasons - string constant INVALID_ORDER_TAKER_ASSET_AMOUNT = "Invalid order taker asset amount: expected a non-zero value."; - string constant INVALID_ORDER_MAKER_ASSET_AMOUNT = "Invalid order maker asset amount: expected a non-zero value."; - - // Transaction revert reasons - string constant REENTRANCY_NOT_ALLOWED = "`executeTransaction` is not allowed to call itself recursively."; - string constant DUPLICATE_TRANSACTION_HASH = "Transaction has already been executed."; - string constant TRANSACTION_EXECUTION_FAILED = "Transaction execution failed."; - - // Wrapper revert reasons - string constant COMPLETE_FILL_FAILED = "Desired fill amount could not be completely filled."; - string constant ASSET_DATA_MISMATCH = "Asset data must be the same for each order."; - - // Asset proxy dispatcher revert reasons - string constant GT_ZERO_LENGTH_REQUIRED = "Length must be greater than 0."; - string constant OLD_ASSET_PROXY_MISMATCH = "Old asset proxy does not match asset proxy at given id."; - string constant NEW_ASSET_PROXY_MISMATCH = "New asset proxy id does not match given id."; - - // Signature validator revert reasons - 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 FAILED_TO_CALCULATE_FILL_RESULTS_FOR_LEFT_ORDER = "Failed to calculate fill results for left order."; - string constant FAILED_TO_CALCULATE_FILL_RESULTS_FOR_RIGHT_ORDER = "Failed to calculate fill results for right order."; + /// Order validation errors /// + string constant ORDER_UNFILLABLE = "ORDER_UNFILLABLE"; // Order cannot be filled. + string constant INVALID_MAKER = "INVALID_MAKER"; // Invalid makerAddress. + string constant INVALID_TAKER = "INVALID_TAKER"; // Invalid takerAddress. + string constant INVALID_SENDER = "INVALID_SENDER"; // Invalid `msg.sender`. + string constant INVALID_ORDER_SIGNATURE = "INVALID_ORDER_SIGNATURE"; // Signature validation failed. + string constant ASSET_DATA_MISMATCH = "ASSET_DATA_MISMATCH"; // Asset data must be the same for each order. + + /// fillOrder validation errors /// + string constant INVALID_TAKER_AMOUNT = "INVALID_TAKER_AMOUNT"; // takerAssetFillAmount cannot equal 0. + string constant ROUNDING_ERROR = "ROUNDING_ERROR"; // Rounding error greater than 0.1% of takerAssetFillAmount. + + /// Signature validation errors /// + string constant INVALID_SIGNATURE = "INVALID_SIGNATURE"; // Signature validation failed. + string constant SIGNATURE_ILLEGAL = "SIGNATURE_ILLEGAL"; // Signature type is illegal. + string constant SIGNATURE_UNSUPPORTED = "SIGNATURE_UNSUPPORTED"; // Signature type unsupported. + + /// cancelOrdersUptTo errors /// + string constant INVALID_NEW_MAKER_EPOCH = "INVALID_NEW_MAKER_EPOCH"; // Specified salt must be greater than or equal to existing makerEpoch. + + /// fillOrKillOrder errors /// + string constant COMPLETE_FILL_FAILED = "COMPLETE_FILL_FAILED"; // Desired takerAssetFillAmount could not be completely filled. + + /// matchOrders errors /// + string constant NEGATIVE_SPREAD_REQUIRED = "NEGATIVE_SPREAD_REQUIRED"; // Matched orders must have a negative spread. + + /// Transaction errors /// + string constant REENTRANCY_ILLEGAL = "REENTRANCY_ILLEGAL"; // Recursive reentrancy is not allowed. + string constant INVALID_TX_HASH = "INVALID_TX_HASH"; // Transaction has already been executed. + string constant INVALID_TX_SIGNATURE = "INVALID_TX_SIGNATURE"; // Signature validation failed. + string constant FAILED_EXECUTION = "FAILED_EXECUTION"; // Transaction execution failed. + + /// registerAssetProxy errors /// + string constant ASSET_PROXY_MISMATCH = "ASSET_PROXY_MISMATCH"; // oldAssetProxy proxy does not match currentAssetProxy. + string constant ASSET_PROXY_ID_MISMATCH = "ASSET_PROXY_ID_MISMATCH"; // newAssetProxyId does not match given assetProxyId. + + /// Length validation errors /// + string constant LENGTH_GT_0_REQUIRED = "LENGTH_GT_0_REQUIRED"; // Byte array must have a length greater than 0. + string constant LENGTH_0_REQUIRED = "LENGTH_1_REQUIRED"; // Byte array must have a length of 1. + string constant LENGTH_65_REQUIRED = "LENGTH_66_REQUIRED"; // Byte array must have a length of 66. } diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibFillResults.sol b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibFillResults.sol index aa54598fa..b7550d6d2 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibFillResults.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibFillResults.sol @@ -32,9 +32,9 @@ contract LibFillResults is } struct MatchedFillResults { - LibFillResults.FillResults left; - LibFillResults.FillResults right; - uint256 takerFillAmount; + FillResults left; + FillResults right; + uint256 leftMakerAssetSpreadAmount; } /// @dev Adds properties of both FillResults instances. @@ -50,19 +50,4 @@ contract LibFillResults is totalFillResults.makerFeePaid = safeAdd(totalFillResults.makerFeePaid, singleFillResults.makerFeePaid); totalFillResults.takerFeePaid = safeAdd(totalFillResults.takerFeePaid, singleFillResults.takerFeePaid); } - - /// @dev Returns a null fill results struct - function getNullFillResults() - internal - pure - returns (FillResults memory) - { - // returns zeroed out FillResults instance - return FillResults({ - makerAssetFilledAmount: 0, - takerAssetFilledAmount: 0, - makerFeePaid: 0, - takerFeePaid: 0 - }); - } } diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibMath.sol b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibMath.sol index ea8c138d6..233547b9f 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibMath.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibMath.sol @@ -45,26 +45,6 @@ contract LibMath is return partialAmount; } - /// @dev Calculates partial value given a numerator and denominator. - /// Throws if there is a rounding error. - /// @param numerator Numerator. - /// @param denominator Denominator. - /// @param target Value to calculate partial of. - /// @return Partial value of target. - function safeGetPartialAmount( - uint256 numerator, - uint256 denominator, - uint256 target) - internal pure - returns (uint256 partialAmount) - { - require( - !isRoundingError(numerator, denominator, target), - ROUNDING_ERROR_ON_PARTIAL_AMOUNT - ); - return getPartialAmount(numerator, denominator, target); - } - /// @dev Checks if rounding error > 0.1%. /// @param numerator Numerator. /// @param denominator Denominator. diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibOrder.sol b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibOrder.sol index ed7f9391d..05ea27ffc 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibOrder.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibOrder.sol @@ -20,11 +20,11 @@ pragma solidity ^0.4.24; contract LibOrder { - bytes32 constant DOMAIN_SEPARATOR_SCHEMA_HASH = keccak256( + bytes32 constant DOMAIN_SEPARATOR_SCHEMA_HASH = keccak256(abi.encodePacked( "DomainSeparator(address contract)" - ); + )); - bytes32 constant ORDER_SCHEMA_HASH = keccak256( + bytes32 constant ORDER_SCHEMA_HASH = keccak256(abi.encodePacked( "Order(", "address makerAddress,", "address takerAddress,", @@ -39,7 +39,19 @@ contract LibOrder { "bytes makerAssetData,", "bytes takerAssetData,", ")" - ); + )); + + // A valid order remains fillable until it is expired, fully filled, or cancelled. + // An order's state is unaffected by external factors, like account balances. + enum OrderStatus { + INVALID, // Default value + INVALID_MAKER_ASSET_AMOUNT, // Order does not have a valid maker asset amount + INVALID_TAKER_ASSET_AMOUNT, // Order does not have a valid taker asset amount + FILLABLE, // Order is fillable + EXPIRED, // Order has already expired + FULLY_FILLED, // Order is fully filled + CANCELLED // Order has been cancelled + } struct Order { address makerAddress; @@ -75,11 +87,11 @@ contract LibOrder { { // TODO: EIP712 is not finalized yet // Source: https://github.com/ethereum/EIPs/pull/712 - orderHash = keccak256( + orderHash = keccak256(abi.encodePacked( DOMAIN_SEPARATOR_SCHEMA_HASH, - keccak256(address(this)), + keccak256(abi.encodePacked(address(this))), ORDER_SCHEMA_HASH, - keccak256( + keccak256(abi.encodePacked( order.makerAddress, order.takerAddress, order.feeRecipientAddress, @@ -90,10 +102,10 @@ contract LibOrder { order.takerFee, order.expirationTimeSeconds, order.salt, - keccak256(order.makerAssetData), - keccak256(order.takerAssetData) - ) - ); + keccak256(abi.encodePacked(order.makerAssetData)), + keccak256(abi.encodePacked(order.takerAssetData)) + )) + )); return orderHash; } } diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibStatus.sol b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibStatus.sol deleted file mode 100644 index f72b7d65f..000000000 --- a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibStatus.sol +++ /dev/null @@ -1,51 +0,0 @@ -/* - - 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.4.24; -pragma experimental ABIEncoderV2; - -contract LibStatus { - - // Exchange Status Codes - enum Status { - /// 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 /// - // A valid order remains fillable until it is expired, fully filled, or cancelled. - // An order's state is unaffected by external factors, like account balances. - 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 - } - - event ExchangeStatus(uint8 indexed statusId, bytes32 indexed orderHash); -} 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 ae1e50637..de7c4d3af 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol @@ -26,7 +26,6 @@ import "../interfaces/IExchangeCore.sol"; contract MExchangeCore is IExchangeCore { - // Fill event is emitted whenever an order is filled. event Fill( address indexed makerAddress, @@ -56,25 +55,6 @@ contract MExchangeCore is uint256 makerEpoch ); - /// @dev Validates context for fillOrder. Succeeds or throws. - /// @param order to be filled. - /// @param orderStatus Status of order to be filled. - /// @param orderHash Hash of order to be filled. - /// @param takerAddress Address of order taker. - /// @param orderTakerAssetFilledAmount Amount of order already filled. - /// @param takerAssetFillAmount Desired amount of order to fill by taker. - /// @param signature Proof that the orders was created by its maker. - function assertValidFill( - LibOrder.Order memory order, - uint8 orderStatus, - bytes32 orderHash, - address takerAddress, - uint256 orderTakerAssetFilledAmount, - uint256 takerAssetFillAmount, - bytes memory signature - ) - internal; - /// @dev Updates state with results of a fill order. /// @param order that was filled. /// @param takerAddress Address of taker who filled the order. @@ -89,29 +69,55 @@ contract MExchangeCore is ) internal; - /// @dev Validates context for cancelOrder. Succeeds or throws. - /// @param order that was cancelled. - /// @param orderStatus Status of order that was cancelled. - /// @param orderHash Hash of order that was cancelled. - function assertValidCancel( - LibOrder.Order memory order, - uint8 orderStatus, - bytes32 orderHash - ) - internal; - /// @dev Updates state with results of cancelling an order. /// State is only updated if the order is currently fillable. /// Otherwise, updating state would have no effect. /// @param order that was cancelled. - /// @param orderStatus Status of order that was cancelled. /// @param orderHash Hash of order that was cancelled. - /// @return stateUpdated Returns true only if state was updated. function updateCancelledState( LibOrder.Order memory order, - uint8 orderStatus, bytes32 orderHash + ) + internal; + + /// @dev Validates context for fillOrder. Succeeds or throws. + /// @param order to be filled. + /// @param orderInfo Status, orderHash, and amount already filled of order. + /// @param takerAddress Address of order taker. + /// @param takerAssetFillAmount Desired amount of order to fill by taker. + /// @param takerAssetFilledAmount Amount of takerAsset that will be filled. + /// @param signature Proof that the orders was created by its maker. + function assertValidFill( + LibOrder.Order memory order, + LibOrder.OrderInfo memory orderInfo, + address takerAddress, + uint256 takerAssetFillAmount, + uint256 takerAssetFilledAmount, + bytes memory signature + ) + internal + view; + + + /// @dev Validates context for cancelOrder. Succeeds or throws. + /// @param order to be cancelled. + /// @param orderInfo OrderStatus, orderHash, and amount already filled of order. + function assertValidCancel( + LibOrder.Order memory order, + LibOrder.OrderInfo memory orderInfo + ) + internal + view; + + /// @dev Calculates amounts filled and fees paid by maker and taker. + /// @param order to be filled. + /// @param takerAssetFilledAmount Amount of takerAsset that will be filled. + /// @return fillResults Amounts filled and fees paid by maker and taker. + function calculateFillResults( + LibOrder.Order memory order, + uint256 takerAssetFilledAmount ) internal - returns (bool stateUpdated); + pure + returns (LibFillResults.FillResults memory fillResults); } diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MMatchOrders.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MMatchOrders.sol index 7dd608cf2..e52963a26 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MMatchOrders.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MMatchOrders.sol @@ -20,7 +20,6 @@ pragma experimental ABIEncoderV2; import "../libs/LibOrder.sol"; import "../libs/LibFillResults.sol"; -import "./MExchangeCore.sol"; import "../interfaces/IMatchOrders.sol"; contract MMatchOrders is @@ -34,12 +33,8 @@ contract MMatchOrders is LibOrder.Order memory leftOrder, LibOrder.Order memory rightOrder ) - internal; - - /// @dev Validates matched fill results. Succeeds or throws. - /// @param matchedFillResults Amounts to fill and fees to pay by maker and taker of matched orders. - function assertValidMatchResults(LibFillResults.MatchedFillResults memory matchedFillResults) - internal; + internal + pure; /// @dev Calculates fill amounts for the matched orders. /// Each order is filled at their respective price point. However, the calculations are @@ -47,19 +42,16 @@ contract MMatchOrders is /// The profit made by the leftOrder order goes to the taker (who matched the two orders). /// @param leftOrder First order to match. /// @param rightOrder Second order to match. - /// @param leftOrderStatus Order status of left order. - /// @param rightOrderStatus Order status of right order. - /// @param leftOrderFilledAmount Amount of left order already filled. - /// @param rightOrderFilledAmount Amount of right order already filled. + /// @param leftOrderTakerAssetFilledAmount Amount of left order already filled. + /// @param rightOrderTakerAssetFilledAmount Amount of right order already filled. /// @param matchedFillResults Amounts to fill and fees to pay by maker and taker of matched orders. function calculateMatchedFillResults( LibOrder.Order memory leftOrder, LibOrder.Order memory rightOrder, - uint8 leftOrderStatus, - uint8 rightOrderStatus, - uint256 leftOrderFilledAmount, - uint256 rightOrderFilledAmount + uint256 leftOrderTakerAssetFilledAmount, + uint256 rightOrderTakerAssetFilledAmount ) internal + pure returns (LibFillResults.MatchedFillResults memory matchedFillResults); } diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSettlement.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSettlement.sol index 50b62e79f..2c403162f 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSettlement.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSettlement.sol @@ -19,7 +19,6 @@ pragma solidity ^0.4.24; import "../libs/LibOrder.sol"; -import "./MMatchOrders.sol"; import "../libs/LibFillResults.sol"; contract MSettlement { diff --git a/packages/contracts/src/contracts/current/test/TestLibs/TestLibs.sol b/packages/contracts/src/contracts/current/test/TestLibs/TestLibs.sol index bd01277dd..47ce0dcf3 100644 --- a/packages/contracts/src/contracts/current/test/TestLibs/TestLibs.sol +++ b/packages/contracts/src/contracts/current/test/TestLibs/TestLibs.sol @@ -71,7 +71,7 @@ contract TestLibs is function getOrderSchemaHash() public - view + pure returns (bytes32) { return ORDER_SCHEMA_HASH; @@ -79,7 +79,7 @@ contract TestLibs is function getDomainSeparatorSchemaHash() public - view + pure returns (bytes32) { return DOMAIN_SEPARATOR_SCHEMA_HASH; diff --git a/packages/contracts/src/utils/match_order_tester.ts b/packages/contracts/src/utils/match_order_tester.ts index 09c0d8083..6170188bc 100644 --- a/packages/contracts/src/utils/match_order_tester.ts +++ b/packages/contracts/src/utils/match_order_tester.ts @@ -26,7 +26,6 @@ import { ContractName, ERC20BalancesByOwner, ERC721TokenIdsByOwner, - ExchangeStatus, TransferAmountsByMatchOrders as TransferAmounts, } from '../utils/types'; import { provider, web3Wrapper } from '../utils/web3_wrapper'; diff --git a/packages/contracts/src/utils/types.ts b/packages/contracts/src/utils/types.ts index 6340c4a51..360e1fdbc 100644 --- a/packages/contracts/src/utils/types.ts +++ b/packages/contracts/src/utils/types.ts @@ -69,22 +69,14 @@ export interface Token { swarmHash: string; } -export enum ExchangeStatus { +export enum OrderStatus { 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, + INVALID_MAKER_ASSET_AMOUNT, + INVALID_TAKER_ASSET_AMOUNT, + FILLABLE, + EXPIRED, + FULLY_FILLED, + CANCELLED, } export enum ContractName { diff --git a/packages/contracts/test/asset_proxy/authorizable.ts b/packages/contracts/test/asset_proxy/authorizable.ts index 87f89c513..e8274acb1 100644 --- a/packages/contracts/test/asset_proxy/authorizable.ts +++ b/packages/contracts/test/asset_proxy/authorizable.ts @@ -19,6 +19,7 @@ describe('Authorizable', () => { let notOwner: string; let address: string; let authorizable: MixinAuthorizableContract; + before(async () => { await blockchainLifecycle.startAsync(); }); diff --git a/packages/contracts/test/exchange/core.ts b/packages/contracts/test/exchange/core.ts index 8320e97d6..16492b8ac 100644 --- a/packages/contracts/test/exchange/core.ts +++ b/packages/contracts/test/exchange/core.ts @@ -15,7 +15,6 @@ import { ERC721ProxyContract } from '../../src/contract_wrappers/generated/e_r_c import { CancelContractEventArgs, ExchangeContract, - ExchangeStatusContractEventArgs, FillContractEventArgs, } from '../../src/contract_wrappers/generated/exchange'; import { artifacts } from '../../src/utils/artifacts'; @@ -25,6 +24,7 @@ import { ERC20Wrapper } from '../../src/utils/erc20_wrapper'; import { ERC721Wrapper } from '../../src/utils/erc721_wrapper'; import { ExchangeWrapper } from '../../src/utils/exchange_wrapper'; import { OrderFactory } from '../../src/utils/order_factory'; +import { orderUtils } from '../../src/utils/order_utils'; import { ContractName, ERC20BalancesByOwner, ExchangeStatus } from '../../src/utils/types'; import { provider, txDefaults, web3Wrapper } from '../../src/utils/web3_wrapper'; @@ -132,6 +132,7 @@ describe('Exchange core', () => { signedOrder = orderFactory.newSignedOrder(); }); +<<<<<<< HEAD it('should create an unfillable order', async () => { signedOrder = orderFactory.newSignedOrder({ makerAssetAmount: new BigNumber(1001), @@ -164,6 +165,8 @@ describe('Exchange core', () => { expect(takerAssetFilledAmountAfter2).to.be.bignumber.equal(takerAssetFilledAmountAfter1); }); +======= +>>>>>>> Update Exchange statuses, revert instead of emmitting event on fill/cancel failures, and remove redundant logic in matchOrders it('should transfer the correct amounts when makerAssetAmount === takerAssetAmount', async () => { signedOrder = orderFactory.newSignedOrder({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), @@ -550,37 +553,21 @@ describe('Exchange core', () => { ); }); - it('should not change erc20Balances if an order is expired', async () => { + it('should throw if an order is expired', async () => { signedOrder = orderFactory.newSignedOrder({ expirationTimeSeconds: new BigNumber(Math.floor((Date.now() - 10000) / 1000)), }); - await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); - - const newBalances = await erc20Wrapper.getBalancesAsync(); - expect(newBalances).to.be.deep.equal(erc20Balances); - }); - - it('should log an error event if an order is expired', async () => { - signedOrder = orderFactory.newSignedOrder({ - expirationTimeSeconds: new BigNumber(Math.floor((Date.now() - 10000) / 1000)), - }); - - const res = await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); - expect(res.logs).to.have.length(1); - const log = res.logs[0] as LogWithDecodedArgs; - const statusCode = log.args.statusId; - expect(statusCode).to.be.equal(ExchangeStatus.ORDER_EXPIRED); + return expect(exchangeWrapper.fillOrderAsync(signedOrder, takerAddress)).to.be.rejectedWith( + constants.REVERT, + ); }); - it('should log an error event if no value is filled', async () => { - signedOrder = orderFactory.newSignedOrder({}); + it('should throw if no value is filled', async () => { + signedOrder = orderFactory.newSignedOrder(); await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); - - const res = await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); - expect(res.logs).to.have.length(1); - const log = res.logs[0] as LogWithDecodedArgs; - const statusCode = log.args.statusId; - expect(statusCode).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + return expect(exchangeWrapper.fillOrderAsync(signedOrder, takerAddress)).to.be.rejectedWith( + constants.REVERT, + ); }); }); @@ -618,12 +605,11 @@ describe('Exchange core', () => { it('should be able to cancel a full order', async () => { await exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress); - await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { - takerAssetFillAmount: signedOrder.takerAssetAmount.div(2), - }); - - const newBalances = await erc20Wrapper.getBalancesAsync(); - expect(newBalances).to.be.deep.equal(erc20Balances); + return expect( + exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { + takerAssetFillAmount: signedOrder.takerAssetAmount.div(2), + }), + ).to.be.rejectedWith(constants.REVERT); }); it('should log 1 event with correct arguments', async () => { @@ -641,26 +627,39 @@ describe('Exchange core', () => { expect(orderHashUtils.getOrderHashHex(signedOrder)).to.be.equal(logArgs.orderHash); }); - it('should log an error if already cancelled', async () => { + it('should throw if already cancelled', async () => { await exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress); - - const res = await exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress); - expect(res.logs).to.have.length(1); - const log = res.logs[0] as LogWithDecodedArgs; - const statusCode = log.args.statusId; - expect(statusCode).to.be.equal(ExchangeStatus.ORDER_CANCELLED); + return expect(exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress)).to.be.rejectedWith( + constants.REVERT, + ); }); - it('should log error if order is expired', async () => { + it('should throw if order is expired', async () => { signedOrder = orderFactory.newSignedOrder({ expirationTimeSeconds: new BigNumber(Math.floor((Date.now() - 10000) / 1000)), }); + return expect(exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress)).to.be.rejectedWith( + constants.REVERT, + ); + }); - const res = await exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress); - expect(res.logs).to.have.length(1); - const log = res.logs[0] as LogWithDecodedArgs; - const statusCode = log.args.statusId; - expect(statusCode).to.be.equal(ExchangeStatus.ORDER_EXPIRED); + it('should throw if rounding error is greater than 0.1%', async () => { + signedOrder = orderFactory.newSignedOrder({ + makerAssetAmount: new BigNumber(1001), + takerAssetAmount: new BigNumber(3), + }); + + const fillTakerAssetAmount1 = new BigNumber(2); + await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { + takerAssetFillAmount: fillTakerAssetAmount1, + }); + + const fillTakerAssetAmount2 = new BigNumber(1); + return expect( + exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { + takerAssetFillAmount: fillTakerAssetAmount2, + }), + ).to.be.rejectedWith(constants.REVERT); }); }); diff --git a/packages/contracts/test/exchange/match_orders.ts b/packages/contracts/test/exchange/match_orders.ts index 07295d78a..24ee794bc 100644 --- a/packages/contracts/test/exchange/match_orders.ts +++ b/packages/contracts/test/exchange/match_orders.ts @@ -15,7 +15,6 @@ import { ERC721ProxyContract } from '../../src/contract_wrappers/generated/e_r_c import { CancelContractEventArgs, ExchangeContract, - ExchangeStatusContractEventArgs, FillContractEventArgs, } from '../../src/contract_wrappers/generated/exchange'; import { artifacts } from '../../src/utils/artifacts'; @@ -30,8 +29,8 @@ import { ContractName, ERC20BalancesByOwner, ERC721TokenIdsByOwner, - ExchangeStatus, OrderInfo, + OrderStatus, } from '../../src/utils/types'; import { provider, txDefaults, web3Wrapper } from '../../src/utils/web3_wrapper'; @@ -186,10 +185,10 @@ describe('matchOrders', () => { ); // Verify left order was fully filled const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); // Verify right order was fully filled const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); }); it('should transfer the correct amounts when orders completely fill each other and taker doesnt take a profit', async () => { @@ -227,10 +226,10 @@ describe('matchOrders', () => { ); // Verify left order was fully filled const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); // Verify right order was fully filled const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); // Verify taker did not take a profit expect(takerInitialBalances).to.be.deep.equal( newERC20BalancesByOwner[takerAddress][defaultERC20MakerAssetAddress], @@ -265,10 +264,10 @@ describe('matchOrders', () => { ); // Verify left order was fully filled const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); // Verify right order was partially filled const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FILLABLE); + expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FILLABLE); }); it('should transfer the correct amounts when right order is completely filled and left order is partially filled', async () => { @@ -299,10 +298,10 @@ describe('matchOrders', () => { ); // Verify left order was partially filled const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FILLABLE); + expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FILLABLE); // Verify right order was fully filled const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); }); it('should transfer the correct amounts when consecutive calls are used to completely fill the left order', async () => { @@ -338,10 +337,10 @@ describe('matchOrders', () => { ); // Verify left order was partially filled const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FILLABLE); + expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FILLABLE); // Verify right order was fully filled const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); // Construct second right order // Note: This order needs makerAssetAmount=90/takerAssetAmount=[anything <= 45] to fully fill the right order. // However, we use 100/50 to ensure a partial fill as we want to go down the "left fill" @@ -368,10 +367,10 @@ describe('matchOrders', () => { ); // Verify left order was fully filled const leftOrderInfo2: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo2.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(leftOrderInfo2.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); // Verify second right order was partially filled const rightOrderInfo2: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight2); - expect(rightOrderInfo2.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FILLABLE); + expect(rightOrderInfo2.orderStatus as OrderStatus).to.be.equal(OrderStatus.FILLABLE); }); it('should transfer the correct amounts when consecutive calls are used to completely fill the right order', async () => { @@ -408,10 +407,10 @@ describe('matchOrders', () => { ); // Verify left order was partially filled const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); // Verify right order was fully filled const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FILLABLE); + expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FILLABLE); // Create second left order // Note: This order needs makerAssetAmount=96/takerAssetAmount=48 to fully fill the right order. // However, we use 100/50 to ensure a partial fill as we want to go down the "right fill" @@ -441,10 +440,10 @@ describe('matchOrders', () => { ); // Verify second left order was partially filled const leftOrderInfo2: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft2); - expect(leftOrderInfo2.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FILLABLE); + expect(leftOrderInfo2.orderStatus as OrderStatus).to.be.equal(OrderStatus.FILLABLE); // Verify right order was fully filled const rightOrderInfo2: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo2.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(rightOrderInfo2.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); }); it('should transfer the correct amounts if fee recipient is the same across both matched orders', async () => { @@ -790,10 +789,10 @@ describe('matchOrders', () => { ); // Verify left order was fully filled const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); // Verify right order was fully filled const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); }); it('should transfer correct amounts when right order maker asset is an ERC721 token', async () => { @@ -825,10 +824,10 @@ describe('matchOrders', () => { ); // Verify left order was fully filled const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); // Verify right order was fully filled const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); }); }); }); // tslint:disable-line:max-file-line-count diff --git a/packages/contracts/test/exchange/transactions.ts b/packages/contracts/test/exchange/transactions.ts index 9af8b522b..6480b7e0b 100644 --- a/packages/contracts/test/exchange/transactions.ts +++ b/packages/contracts/test/exchange/transactions.ts @@ -194,9 +194,9 @@ describe('Exchange transactions', () => { it('should cancel the order when signed by maker and called by sender', async () => { await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress); - const res = await exchangeWrapper.fillOrderAsync(signedOrder, senderAddress); - const newBalances = await erc20Wrapper.getBalancesAsync(); - expect(newBalances).to.deep.equal(erc20Balances); + return expect(exchangeWrapper.fillOrderAsync(signedOrder, senderAddress)).to.be.rejectedWith( + constants.REVERT, + ); }); }); }); diff --git a/packages/contracts/test/exchange/wrapper.ts b/packages/contracts/test/exchange/wrapper.ts index a158ba8f3..583ec9f91 100644 --- a/packages/contracts/test/exchange/wrapper.ts +++ b/packages/contracts/test/exchange/wrapper.ts @@ -959,7 +959,7 @@ describe('Exchange wrappers', () => { const takerAssetCancelAmounts = _.map(signedOrders, signedOrder => signedOrder.takerAssetAmount); await exchangeWrapper.batchCancelOrdersAsync(signedOrders, makerAddress); - await exchangeWrapper.batchFillOrdersAsync(signedOrders, takerAddress, { + await exchangeWrapper.batchFillOrdersNoThrowAsync(signedOrders, takerAddress, { takerAssetFillAmounts: takerAssetCancelAmounts, }); const newBalances = await erc20Wrapper.getBalancesAsync(); -- cgit v1.2.3 From 351173e554b4259f6a95f066b15d97317835aa8d Mon Sep 17 00:00:00 2001 From: Jacob Evans Date: Mon, 4 Jun 2018 16:55:22 -0700 Subject: Rebase from v2-prototype --- packages/contracts/test/exchange/core.ts | 38 +----------------------- packages/contracts/test/exchange/transactions.ts | 2 +- 2 files changed, 2 insertions(+), 38 deletions(-) diff --git a/packages/contracts/test/exchange/core.ts b/packages/contracts/test/exchange/core.ts index 16492b8ac..91ead93f0 100644 --- a/packages/contracts/test/exchange/core.ts +++ b/packages/contracts/test/exchange/core.ts @@ -25,7 +25,7 @@ import { ERC721Wrapper } from '../../src/utils/erc721_wrapper'; import { ExchangeWrapper } from '../../src/utils/exchange_wrapper'; import { OrderFactory } from '../../src/utils/order_factory'; import { orderUtils } from '../../src/utils/order_utils'; -import { ContractName, ERC20BalancesByOwner, ExchangeStatus } from '../../src/utils/types'; +import { ContractName, ERC20BalancesByOwner, OrderStatus } from '../../src/utils/types'; import { provider, txDefaults, web3Wrapper } from '../../src/utils/web3_wrapper'; chaiSetup.configure(); @@ -131,42 +131,6 @@ describe('Exchange core', () => { erc20Balances = await erc20Wrapper.getBalancesAsync(); signedOrder = orderFactory.newSignedOrder(); }); - -<<<<<<< HEAD - it('should create an unfillable order', async () => { - signedOrder = orderFactory.newSignedOrder({ - makerAssetAmount: new BigNumber(1001), - takerAssetAmount: new BigNumber(3), - }); - - const takerAssetFilledAmountBefore = await exchangeWrapper.getTakerAssetFilledAmountAsync( - orderHashUtils.getOrderHashHex(signedOrder), - ); - expect(takerAssetFilledAmountBefore).to.be.bignumber.equal(0); - - const fillTakerAssetAmount1 = new BigNumber(2); - await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { - takerAssetFillAmount: fillTakerAssetAmount1, - }); - - const takerAssetFilledAmountAfter1 = await exchangeWrapper.getTakerAssetFilledAmountAsync( - orderHashUtils.getOrderHashHex(signedOrder), - ); - expect(takerAssetFilledAmountAfter1).to.be.bignumber.equal(fillTakerAssetAmount1); - - const fillTakerAssetAmount2 = new BigNumber(1); - await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { - takerAssetFillAmount: fillTakerAssetAmount2, - }); - - const takerAssetFilledAmountAfter2 = await exchangeWrapper.getTakerAssetFilledAmountAsync( - orderHashUtils.getOrderHashHex(signedOrder), - ); - expect(takerAssetFilledAmountAfter2).to.be.bignumber.equal(takerAssetFilledAmountAfter1); - }); - -======= ->>>>>>> Update Exchange statuses, revert instead of emmitting event on fill/cancel failures, and remove redundant logic in matchOrders it('should transfer the correct amounts when makerAssetAmount === takerAssetAmount', async () => { signedOrder = orderFactory.newSignedOrder({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), diff --git a/packages/contracts/test/exchange/transactions.ts b/packages/contracts/test/exchange/transactions.ts index 6480b7e0b..f31053ad3 100644 --- a/packages/contracts/test/exchange/transactions.ts +++ b/packages/contracts/test/exchange/transactions.ts @@ -18,7 +18,7 @@ import { ExchangeWrapper } from '../../src/utils/exchange_wrapper'; import { OrderFactory } from '../../src/utils/order_factory'; import { orderUtils } from '../../src/utils/order_utils'; import { TransactionFactory } from '../../src/utils/transaction_factory'; -import { ERC20BalancesByOwner, ExchangeStatus, SignedTransaction } from '../../src/utils/types'; +import { ERC20BalancesByOwner, OrderStatus, SignedTransaction } from '../../src/utils/types'; import { provider, txDefaults, web3Wrapper } from '../../src/utils/web3_wrapper'; chaiSetup.configure(); -- cgit v1.2.3 From 5c44db341fca8926fcf2329f96f0f7f7a5ce16f7 Mon Sep 17 00:00:00 2001 From: Jacob Evans Date: Mon, 4 Jun 2018 17:04:12 -0700 Subject: rename GT to GREATER_THAN --- .../contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol | 2 +- .../src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol | 2 +- .../src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol index 397ca89d2..8f9342739 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol @@ -99,7 +99,7 @@ contract MixinAssetProxyDispatcher is uint256 length = assetMetadata.length; require( length > 0, - LENGTH_GT_0_REQUIRED + LENGTH_GREATER_THAN_0_REQUIRED ); uint8 assetProxyId = uint8(assetMetadata[length - 1]); IAssetProxy assetProxy = assetProxies[assetProxyId]; diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol index ae7d053fd..48a0c5552 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol @@ -89,7 +89,7 @@ contract MixinSignatureValidator is // TODO: Domain separation: make hash depend on role. (Taker sig should not be valid as maker sig, etc.) require( signature.length > 0, - LENGTH_GT_0_REQUIRED + LENGTH_GREATER_THAN_0_REQUIRED ); // Pop last byte off of signature byte array. 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 8b2e3bb35..2b4bbeec4 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol @@ -56,7 +56,7 @@ contract LibExchangeErrors { string constant ASSET_PROXY_ID_MISMATCH = "ASSET_PROXY_ID_MISMATCH"; // newAssetProxyId does not match given assetProxyId. /// Length validation errors /// - string constant LENGTH_GT_0_REQUIRED = "LENGTH_GT_0_REQUIRED"; // Byte array must have a length greater than 0. + string constant LENGTH_GREATER_THAN_0_REQUIRED = "LENGTH_GREATER_THAN_0_REQUIRED"; // Byte array must have a length greater than 0. string constant LENGTH_0_REQUIRED = "LENGTH_1_REQUIRED"; // Byte array must have a length of 1. string constant LENGTH_65_REQUIRED = "LENGTH_66_REQUIRED"; // Byte array must have a length of 66. } -- cgit v1.2.3