From e706fa76acfbf933479f767749755446cdaf438a Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Fri, 17 Aug 2018 12:11:51 -0700 Subject: Add overfill and price assertion to assertValidFill --- .../2.0.0/protocol/Exchange/MixinExchangeCore.sol | 32 ++++++++++++++++++++-- .../protocol/Exchange/mixins/MExchangeCore.sol | 2 ++ 2 files changed, 32 insertions(+), 2 deletions(-) (limited to 'packages') diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol index ab5c6e507..e6b2ddf3d 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol @@ -266,6 +266,7 @@ contract MixinExchangeCore is /// @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 makerAssetFilledAmount Amount of makerAsset that will be transfered. /// @param signature Proof that the orders was created by its maker. function assertValidFill( Order memory order, @@ -273,6 +274,7 @@ contract MixinExchangeCore is address takerAddress, uint256 takerAssetFillAmount, uint256 takerAssetFilledAmount, + uint256 makerAssetFilledAmount, bytes memory signature ) internal @@ -297,7 +299,7 @@ contract MixinExchangeCore is "INVALID_SENDER" ); } - + // Validate taker is allowed to fill this order if (order.takerAddress != address(0)) { require( @@ -317,7 +319,33 @@ contract MixinExchangeCore is "INVALID_ORDER_SIGNATURE" ); } - + + // Make sure taker does not pay more than desired amount + // NOTE: This assertion should never fail, it is here + // as an extra defence against potential bugs. + require( + takerAssetFilledAmount <= takerAssetFilledAmount, + "BUG_TAKER_OVERPAY" + ); + + // Make sure order is not overfilled + // NOTE: This assertion should never fail, it is here + // as an extra defence against potential bugs. + require( + safeAdd(orderInfo.orderTakerAssetFilledAmount, takerAssetFilledAmount) <= order.takerAssetAmount, + "BUG_ORDER_OVERFILL" + ); + + // Make sure order is filled at acceptable price + // NOTE: This assertion should never fail, it is here + // as an extra defence against potential bugs. + require( + safeMul(makerAssetFilledAmount, order.takerAssetAmount) + <= + safeMul(takerAssetFilledAmount, order.makerAssetAmount), + "BUG_ORDER_FILL_PRICING" + ); + // Validate fill order rounding require( !isRoundingError( diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol index c165b647c..eccb6a29d 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol @@ -90,6 +90,7 @@ contract MExchangeCore is /// @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 makerAssetFilledAmount Amount of makerAsset that will be transfered. /// @param signature Proof that the orders was created by its maker. function assertValidFill( LibOrder.Order memory order, @@ -97,6 +98,7 @@ contract MExchangeCore is address takerAddress, uint256 takerAssetFillAmount, uint256 takerAssetFilledAmount, + uint256 makerAssetFilledAmount, bytes memory signature ) internal -- cgit v1.2.3 From d92fd437911a5b9c0af15322e39dd4c2a1f4ab60 Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Fri, 17 Aug 2018 12:12:27 -0700 Subject: Update for new assertValidFill signature --- .../contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol | 9 +++++---- .../contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol | 4 +++- 2 files changed, 8 insertions(+), 5 deletions(-) (limited to 'packages') diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol index e6b2ddf3d..36060a1b6 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol @@ -98,6 +98,9 @@ contract MixinExchangeCore is uint256 remainingTakerAssetAmount = safeSub(order.takerAssetAmount, orderInfo.orderTakerAssetFilledAmount); uint256 takerAssetFilledAmount = min256(takerAssetFillAmount, remainingTakerAssetAmount); + // Compute proportional fill amounts + fillResults = calculateFillResults(order, takerAssetFilledAmount); + // Validate context assertValidFill( order, @@ -105,12 +108,10 @@ contract MixinExchangeCore is takerAddress, takerAssetFillAmount, takerAssetFilledAmount, + fillResults.makerAssetFilledAmount, signature ); - - // Compute proportional fill amounts - fillResults = calculateFillResults(order, takerAssetFilledAmount); - + // Update exchange internal state updateFilledState( order, diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol index 56b309a1b..4d0411a63 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol @@ -80,6 +80,7 @@ contract MixinMatchOrders is takerAddress, matchedFillResults.left.takerAssetFilledAmount, matchedFillResults.left.takerAssetFilledAmount, + matchedFillResults.left.makerAssetFilledAmount, leftSignature ); assertValidFill( @@ -88,9 +89,10 @@ contract MixinMatchOrders is takerAddress, matchedFillResults.right.takerAssetFilledAmount, matchedFillResults.right.takerAssetFilledAmount, + matchedFillResults.right.makerAssetFilledAmount, rightSignature ); - + // Update exchange state updateFilledState( leftOrder, -- cgit v1.2.3 From b16f5f55fb66aac624dfb82b881026b588f66b79 Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Fri, 17 Aug 2018 14:19:19 -0700 Subject: Check fillable early --- .../contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'packages') diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol index 36060a1b6..e281de264 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol @@ -90,6 +90,12 @@ contract MixinExchangeCore is { // Fetch order info OrderInfo memory orderInfo = getOrderInfo(order); + + // An order can only be filled if its status is FILLABLE. + require( + orderInfo.orderStatus == uint8(OrderStatus.FILLABLE), + "ORDER_UNFILLABLE" + ); // Fetch taker address address takerAddress = getCurrentContextAddress(); -- cgit v1.2.3 From 07e56b3cc7f1171cb199b5f0e286971d0704adb6 Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Thu, 23 Aug 2018 15:33:46 -0700 Subject: Fix taker overpay check --- packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'packages') diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol index e281de264..b3671e7e8 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol @@ -331,7 +331,7 @@ contract MixinExchangeCore is // NOTE: This assertion should never fail, it is here // as an extra defence against potential bugs. require( - takerAssetFilledAmount <= takerAssetFilledAmount, + takerAssetFilledAmount <= takerAssetFillAmount, "BUG_TAKER_OVERPAY" ); -- cgit v1.2.3 From a1d89435525795f6f412a823241740f10cbdf1d8 Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Thu, 23 Aug 2018 16:00:01 -0700 Subject: Document accetable price check --- .../src/2.0.0/protocol/Exchange/MixinExchangeCore.sol | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) (limited to 'packages') diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol index b3671e7e8..e5bd306dd 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol @@ -343,13 +343,27 @@ contract MixinExchangeCore is "BUG_ORDER_OVERFILL" ); - // Make sure order is filled at acceptable price + // Make sure order is filled at acceptable price. + // The order has an implied price from the makers perspective: + // order price = order.makerAssetAmount / order.takerAssetAmount + // i.e. the number of makerAsset maker is paying per takerAsset. The + // maker is guaranteed to get this price or a better (lower) one. The + // actual price maker is getting in this fill is: + // fill price = makerAssetFilledAmount / takerAssetFilledAmount + // We need `fill price <= order price` for the fill to be fair to maker. + // This amounts to: + // makerAssetFilledAmount order.makerAssetAmount + // ------------------------ <= ----------------------- + // takerAssetFilledAmount order.takerAssetAmount + // or, equivalently: + // makerAssetFilledAmount * order.takerAssetAmount <= + // order.makerAssetAmount * takerAssetFilledAmount // NOTE: This assertion should never fail, it is here // as an extra defence against potential bugs. require( safeMul(makerAssetFilledAmount, order.takerAssetAmount) <= - safeMul(takerAssetFilledAmount, order.makerAssetAmount), + safeMul(order.makerAssetAmount, takerAssetFilledAmount), "BUG_ORDER_FILL_PRICING" ); -- cgit v1.2.3 From e6e7bae4459595343e07c26891577a004b0062eb Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Thu, 23 Aug 2018 16:51:31 -0700 Subject: Remove BUG_ from revert reasons --- .../contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'packages') diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol index e5bd306dd..1a0a1a135 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol @@ -332,7 +332,7 @@ contract MixinExchangeCore is // as an extra defence against potential bugs. require( takerAssetFilledAmount <= takerAssetFillAmount, - "BUG_TAKER_OVERPAY" + "TAKER_OVERPAY" ); // Make sure order is not overfilled @@ -340,7 +340,7 @@ contract MixinExchangeCore is // as an extra defence against potential bugs. require( safeAdd(orderInfo.orderTakerAssetFilledAmount, takerAssetFilledAmount) <= order.takerAssetAmount, - "BUG_ORDER_OVERFILL" + "ORDER_OVERFILL" ); // Make sure order is filled at acceptable price. @@ -364,7 +364,7 @@ contract MixinExchangeCore is safeMul(makerAssetFilledAmount, order.takerAssetAmount) <= safeMul(order.makerAssetAmount, takerAssetFilledAmount), - "BUG_ORDER_FILL_PRICING" + "INVALID_FILL_PRICE" ); // Validate fill order rounding -- cgit v1.2.3 From 749c6ecc30d1e52de4ef4f0da5864024d75d2ecb Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Thu, 23 Aug 2018 16:55:13 -0700 Subject: Add revert reasons to types --- packages/types/src/index.ts | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'packages') diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 4375fc631..486f71a3a 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -165,6 +165,7 @@ export interface ERC721AssetData { tokenId: BigNumber; } +// TODO: DRY. These should be extracted from contract code. export enum RevertReason { OrderUnfillable = 'ORDER_UNFILLABLE', InvalidMaker = 'INVALID_MAKER', @@ -176,6 +177,10 @@ export enum RevertReason { InvalidSignature = 'INVALID_SIGNATURE', SignatureIllegal = 'SIGNATURE_ILLEGAL', SignatureUnsupported = 'SIGNATURE_UNSUPPORTED', + InvalidOrderSignature = 'INVALID_ORDER_SIGNATURE', + TakerOverpay = 'TAKER_OVERPAY', + OrderOverfill = 'ORDER_OVERFILL', + InvalidFillPrice = 'INVALID_FILL_PRICE', InvalidNewOrderEpoch = 'INVALID_NEW_ORDER_EPOCH', CompleteFillFailed = 'COMPLETE_FILL_FAILED', NegativeSpreadRequired = 'NEGATIVE_SPREAD_REQUIRED', -- cgit v1.2.3 From 3e4493b389334fa28ae0f0043e4dbda23f21adec Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Fri, 24 Aug 2018 10:49:05 -0700 Subject: Disallow self filling --- .../contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'packages') diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol index 1a0a1a135..b8b3899e8 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol @@ -314,7 +314,13 @@ contract MixinExchangeCore is "INVALID_TAKER" ); } - + + // Orders can not be self-filled (use cancel instead) + require( + order.makerAddress != takerAddress, + "INVALID_TAKER" + ); + // Validate Maker signature (check only if first time seen) if (orderInfo.orderTakerAssetFilledAmount == 0) { require( -- cgit v1.2.3 From 29971f36cf5c0096dfd3b52c014c52b812a9e9ac Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Fri, 24 Aug 2018 13:41:56 -0700 Subject: Split into assertFillable and assertValidFill --- .../2.0.0/protocol/Exchange/MixinExchangeCore.sol | 54 ++++++++++++---------- .../2.0.0/protocol/Exchange/MixinMatchOrders.sol | 18 ++++++-- 2 files changed, 43 insertions(+), 29 deletions(-) (limited to 'packages') diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol index b8b3899e8..4fd71092c 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol @@ -91,14 +91,11 @@ contract MixinExchangeCore is // Fetch order info OrderInfo memory orderInfo = getOrderInfo(order); - // An order can only be filled if its status is FILLABLE. - require( - orderInfo.orderStatus == uint8(OrderStatus.FILLABLE), - "ORDER_UNFILLABLE" - ); - // Fetch taker address address takerAddress = getCurrentContextAddress(); + + // Assert that the order is fillable by taker + assertFillableOrder(order, orderInfo, taker, signature); // Get amount of takerAsset to fill uint256 remainingTakerAssetAmount = safeSub(order.takerAssetAmount, orderInfo.orderTakerAssetFilledAmount); @@ -110,8 +107,6 @@ contract MixinExchangeCore is // Validate context assertValidFill( order, - orderInfo, - takerAddress, takerAssetFillAmount, takerAssetFilledAmount, fillResults.makerAssetFilledAmount, @@ -266,22 +261,16 @@ contract MixinExchangeCore is order.takerAssetData ); } - + /// @dev Validates context for fillOrder. Succeeds or throws. /// @param order to be filled. /// @param orderInfo OrderStatus, orderHash, and amount already filled of order. /// @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 makerAssetFilledAmount Amount of makerAsset that will be transfered. /// @param signature Proof that the orders was created by its maker. - function assertValidFill( + function assertFillableOrder( Order memory order, OrderInfo memory orderInfo, - address takerAddress, - uint256 takerAssetFillAmount, - uint256 takerAssetFilledAmount, - uint256 makerAssetFilledAmount, + address taker, bytes memory signature ) internal @@ -292,13 +281,7 @@ contract MixinExchangeCore is orderInfo.orderStatus == uint8(OrderStatus.FILLABLE), "ORDER_UNFILLABLE" ); - - // Revert if fill amount is invalid - require( - takerAssetFillAmount != 0, - "INVALID_TAKER_AMOUNT" - ); - + // Validate sender is allowed to fill this order if (order.senderAddress != address(0)) { require( @@ -332,6 +315,29 @@ contract MixinExchangeCore is "INVALID_ORDER_SIGNATURE" ); } + } + + /// @dev Validates context for fillOrder. Succeeds or throws. + /// @param order to be filled. + /// @param takerAssetFillAmount Desired amount of order to fill by taker. + /// @param takerAssetFilledAmount Amount of takerAsset that will be filled. + /// @param makerAssetFilledAmount Amount of makerAsset that will be transfered. + /// @param signature Proof that the orders was created by its maker. + function assertValidFill( + Order memory order, + uint256 takerAssetFillAmount, + uint256 takerAssetFilledAmount, + uint256 makerAssetFilledAmount, + bytes memory signature + ) + internal + view + { + // Revert if fill amount is invalid + require( + takerAssetFillAmount != 0, + "INVALID_TAKER_AMOUNT" + ); // Make sure taker does not pay more than desired amount // NOTE: This assertion should never fail, it is here diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol index 4d0411a63..5a5b0376e 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol @@ -61,8 +61,20 @@ contract MixinMatchOrders is // Fetch taker address address takerAddress = getCurrentContextAddress(); - + // Either our context is valid or we revert + assertFillableOrder( + leftOrder, + leftOrderInfo, + takerAddress, + leftSignature + ); + assertFillableOrder( + righttOrder, + righttOrderInfo, + takerAddress, + leftSignature + ); assertValidMatch(leftOrder, rightOrder); // Compute proportional fill amounts @@ -76,8 +88,6 @@ contract MixinMatchOrders is // Validate fill contexts assertValidFill( leftOrder, - leftOrderInfo, - takerAddress, matchedFillResults.left.takerAssetFilledAmount, matchedFillResults.left.takerAssetFilledAmount, matchedFillResults.left.makerAssetFilledAmount, @@ -85,8 +95,6 @@ contract MixinMatchOrders is ); assertValidFill( rightOrder, - rightOrderInfo, - takerAddress, matchedFillResults.right.takerAssetFilledAmount, matchedFillResults.right.takerAssetFilledAmount, matchedFillResults.right.makerAssetFilledAmount, -- cgit v1.2.3 From e6f5cac87887709b5a3baaec059005301723f0a5 Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Fri, 24 Aug 2018 13:42:10 -0700 Subject: Fix double definition of error --- packages/types/src/index.ts | 1 - 1 file changed, 1 deletion(-) (limited to 'packages') diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 486f71a3a..5576a6678 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -177,7 +177,6 @@ export enum RevertReason { InvalidSignature = 'INVALID_SIGNATURE', SignatureIllegal = 'SIGNATURE_ILLEGAL', SignatureUnsupported = 'SIGNATURE_UNSUPPORTED', - InvalidOrderSignature = 'INVALID_ORDER_SIGNATURE', TakerOverpay = 'TAKER_OVERPAY', OrderOverfill = 'ORDER_OVERFILL', InvalidFillPrice = 'INVALID_FILL_PRICE', -- cgit v1.2.3 From e21599285941a092a6c6f2dbf58f14f467dcca85 Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Fri, 24 Aug 2018 16:13:17 -0700 Subject: Fix mixin api --- .../2.0.0/protocol/Exchange/MixinExchangeCore.sol | 21 +++++++++++++-------- .../2.0.0/protocol/Exchange/MixinMatchOrders.sol | 14 +++++++------- .../protocol/Exchange/mixins/MExchangeCore.sol | 22 ++++++++++++++++------ 3 files changed, 36 insertions(+), 21 deletions(-) (limited to 'packages') diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol index 4fd71092c..dc62db448 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol @@ -95,7 +95,12 @@ contract MixinExchangeCore is address takerAddress = getCurrentContextAddress(); // Assert that the order is fillable by taker - assertFillableOrder(order, orderInfo, taker, signature); + assertFillableOrder( + order, + orderInfo, + takerAddress, + signature + ); // Get amount of takerAsset to fill uint256 remainingTakerAssetAmount = safeSub(order.takerAssetAmount, orderInfo.orderTakerAssetFilledAmount); @@ -107,10 +112,10 @@ contract MixinExchangeCore is // Validate context assertValidFill( order, + orderInfo, takerAssetFillAmount, takerAssetFilledAmount, - fillResults.makerAssetFilledAmount, - signature + fillResults.makerAssetFilledAmount ); // Update exchange internal state @@ -270,7 +275,7 @@ contract MixinExchangeCore is function assertFillableOrder( Order memory order, OrderInfo memory orderInfo, - address taker, + address takerAddress, bytes memory signature ) internal @@ -319,16 +324,16 @@ contract MixinExchangeCore is /// @dev Validates context for fillOrder. Succeeds or throws. /// @param order to be filled. + /// @param orderInfo OrderStatus, orderHash, and amount already filled of order. /// @param takerAssetFillAmount Desired amount of order to fill by taker. /// @param takerAssetFilledAmount Amount of takerAsset that will be filled. /// @param makerAssetFilledAmount Amount of makerAsset that will be transfered. - /// @param signature Proof that the orders was created by its maker. function assertValidFill( Order memory order, - uint256 takerAssetFillAmount, + OrderInfo memory orderInfo, + uint256 takerAssetFillAmount, // TODO: use FillResults uint256 takerAssetFilledAmount, - uint256 makerAssetFilledAmount, - bytes memory signature + uint256 makerAssetFilledAmount ) internal view diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol index 5a5b0376e..c860640c4 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol @@ -70,10 +70,10 @@ contract MixinMatchOrders is leftSignature ); assertFillableOrder( - righttOrder, - righttOrderInfo, + rightOrder, + rightOrderInfo, takerAddress, - leftSignature + rightSignature ); assertValidMatch(leftOrder, rightOrder); @@ -88,17 +88,17 @@ contract MixinMatchOrders is // Validate fill contexts assertValidFill( leftOrder, + leftOrderInfo, matchedFillResults.left.takerAssetFilledAmount, matchedFillResults.left.takerAssetFilledAmount, - matchedFillResults.left.makerAssetFilledAmount, - leftSignature + matchedFillResults.left.makerAssetFilledAmount ); assertValidFill( rightOrder, + rightOrderInfo, matchedFillResults.right.takerAssetFilledAmount, matchedFillResults.right.takerAssetFilledAmount, - matchedFillResults.right.makerAssetFilledAmount, - rightSignature + matchedFillResults.right.makerAssetFilledAmount ); // Update exchange state diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol index eccb6a29d..708cb329e 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol @@ -83,23 +83,33 @@ contract MExchangeCore is 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 orderInfo OrderStatus, orderHash, and amount already filled of order. /// @param takerAddress Address of order taker. + /// @param signature Proof that the orders was created by its maker. + function assertFillableOrder( + LibOrder.Order memory order, + LibOrder.OrderInfo memory orderInfo, + address takerAddress, + bytes memory signature + ) + internal + view; + + /// @dev Validates context for fillOrder. Succeeds or throws. + /// @param order to be filled. + /// @param orderInfo Status, orderHash, and amount already filled of order. /// @param takerAssetFillAmount Desired amount of order to fill by taker. /// @param takerAssetFilledAmount Amount of takerAsset that will be filled. /// @param makerAssetFilledAmount Amount of makerAsset that will be transfered. - /// @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, - uint256 makerAssetFilledAmount, - bytes memory signature + uint256 makerAssetFilledAmount ) internal view; -- cgit v1.2.3 From 6b866d60533c7e46446bfb69639b07affd1aeb17 Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Fri, 24 Aug 2018 17:29:52 -0700 Subject: Revert maker not equal taker check --- .../contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) (limited to 'packages') diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol index dc62db448..515606cb9 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol @@ -303,12 +303,6 @@ contract MixinExchangeCore is ); } - // Orders can not be self-filled (use cancel instead) - require( - order.makerAddress != takerAddress, - "INVALID_TAKER" - ); - // Validate Maker signature (check only if first time seen) if (orderInfo.orderTakerAssetFilledAmount == 0) { require( @@ -339,6 +333,7 @@ contract MixinExchangeCore is view { // Revert if fill amount is invalid + // TODO: reconsider necessity for v2.1 require( takerAssetFillAmount != 0, "INVALID_TAKER_AMOUNT" -- cgit v1.2.3