From 93087324d9b57c87d48d0b7b3cbb28437927ffeb Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Tue, 15 May 2018 16:11:20 -0700 Subject: Throw if the left or right orders do not compute the correct fill results. I like this better than just logging an error and failing silently. --- .../current/protocol/Exchange/MixinMatchOrders.sol | 36 +++++++++------------- .../protocol/Exchange/libs/LibExchangeErrors.sol | 2 ++ .../current/protocol/Exchange/libs/LibMath.sol | 2 +- .../protocol/Exchange/mixins/MMatchOrders.sol | 6 +--- packages/contracts/test/exchange/match_orders.ts | 18 +++++------ 5 files changed, 27 insertions(+), 37 deletions(-) (limited to 'packages/contracts') diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol index 9ea44beab..9f9ad6a52 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol @@ -78,19 +78,14 @@ contract MixinMatchOrders is validateMatchOrThrow(leftOrder, rightOrder); // Compute proportional fill amounts - uint8 matchedFillResultsStatus; - ( matchedFillResultsStatus, - matchedFillResults - ) = calculateMatchedFillResults( + matchedFillResults = calculateMatchedFillResults( leftOrder, rightOrder, leftOrderInfo.orderStatus, rightOrderInfo.orderStatus, leftOrderInfo.orderFilledAmount, - rightOrderInfo.orderFilledAmount); - if (matchedFillResultsStatus != uint8(Status.SUCCESS)) { - return matchedFillResults; - } + rightOrderInfo.orderFilledAmount + ); // Validate fill contexts validateFillOrRevert( @@ -231,7 +226,6 @@ contract MixinMatchOrders is /// @param rightOrderStatus Order status of right order. /// @param leftOrderFilledAmount Amount of left order already filled. /// @param rightOrderFilledAmount Amount of right order already filled. - /// @return status Return status of calculating fill amounts. Returns Status.SUCCESS on success. /// @param matchedFillResults Amounts to fill and fees to pay by maker and taker of matched orders. function calculateMatchedFillResults( Order memory leftOrder, @@ -242,10 +236,7 @@ contract MixinMatchOrders is uint256 rightOrderFilledAmount ) internal - returns ( - uint8 status, - MatchedFillResults memory matchedFillResults - ) + returns (MatchedFillResults memory matchedFillResults) { // We settle orders at the price point defined by the right order (profit goes to the order taker) // The constraint can be either on the left or on the right. @@ -286,15 +277,17 @@ contract MixinMatchOrders is } // Calculate fill results for left order + uint8 status; (status, matchedFillResults.left) = calculateFillResults( leftOrder, leftOrderStatus, leftOrderFilledAmount, leftOrderAmountToFill ); - if (status != uint8(Status.SUCCESS)) { - return (status, matchedFillResults); - } + require( + status == uint8(Status.SUCCESS), + FAILED_TO_CALCULATE_FILL_RESULTS_FOR_LEFT_ORDER + ); // Calculate fill results for right order (status, matchedFillResults.right) = calculateFillResults( @@ -303,9 +296,10 @@ contract MixinMatchOrders is rightOrderFilledAmount, rightOrderAmountToFill ); - if (status != uint8(Status.SUCCESS)) { - return (status, matchedFillResults); - } + require( + status == uint8(Status.SUCCESS), + FAILED_TO_CALCULATE_FILL_RESULTS_FOR_RIGHT_ORDER + ); // Calculate amount given to taker matchedFillResults.takerFillAmount = safeSub( @@ -316,7 +310,7 @@ contract MixinMatchOrders is // Validate the fill results validateMatchOrThrow(matchedFillResults); - // Return status & fill results - return (status, matchedFillResults); + // Return fill results + return matchedFillResults; } } 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 5ef0a52c5..84f621f64 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol @@ -55,4 +55,6 @@ contract LibExchangeErrors { 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."; } 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 6bae03ff2..27d65c69f 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibMath.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibMath.sol @@ -61,7 +61,7 @@ contract LibMath is require( !isRoundingError(numerator, denominator, target), ROUNDING_ERROR_ON_PARTIAL_AMOUNT - ); + ); return getPartialAmount(numerator, denominator, target); } 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 60a266250..d8b9a22c3 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MMatchOrders.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MMatchOrders.sol @@ -59,7 +59,6 @@ contract MMatchOrders is /// @param rightOrderStatus Order status of right order. /// @param leftOrderFilledAmount Amount of left order already filled. /// @param rightOrderFilledAmount Amount of right order already filled. - /// @return status Return status of calculating fill amounts. Returns Status.SUCCESS on success. /// @param matchedFillResults Amounts to fill and fees to pay by maker and taker of matched orders. function calculateMatchedFillResults( LibOrder.Order memory leftOrder, @@ -70,8 +69,5 @@ contract MMatchOrders is uint256 rightOrderFilledAmount ) internal - returns ( - uint8 status, - LibFillResults.MatchedFillResults memory matchedFillResults - ); + returns (LibFillResults.MatchedFillResults memory matchedFillResults); } diff --git a/packages/contracts/test/exchange/match_orders.ts b/packages/contracts/test/exchange/match_orders.ts index 6ba88b5fb..74e4f6760 100644 --- a/packages/contracts/test/exchange/match_orders.ts +++ b/packages/contracts/test/exchange/match_orders.ts @@ -647,7 +647,7 @@ describe('matchOrders', () => { ); }); - it('Should not transfer any amounts if left order is not fillable', async () => { + it('Should throw if left order is not fillable', async () => { // Create orders to match const signedOrderLeft = orderFactoryLeft.newSignedOrder({ makerAddress: makerAddressLeft, @@ -668,13 +668,12 @@ describe('matchOrders', () => { // Cancel left order await exchangeWrapper.cancelOrderAsync(signedOrderLeft, signedOrderLeft.makerAddress); // Match orders - await exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress); - // Verify balances did not change - const newBalances = await erc20Wrapper.getBalancesAsync(); - expect(newBalances).to.be.deep.equal(erc20BalancesByOwner); + return expect( + exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress), + ).to.be.rejectedWith(constants.REVERT); }); - it('Should not transfer any amounts if right order is not fillable', async () => { + it('Should throw if right order is not fillable', async () => { // Create orders to match const signedOrderLeft = orderFactoryLeft.newSignedOrder({ makerAddress: makerAddressLeft, @@ -695,10 +694,9 @@ describe('matchOrders', () => { // Cancel right order await exchangeWrapper.cancelOrderAsync(signedOrderRight, signedOrderRight.makerAddress); // Match orders - await exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress); - // Verify balances did not change - const newBalances = await erc20Wrapper.getBalancesAsync(); - expect(newBalances).to.be.deep.equal(erc20BalancesByOwner); + return expect( + exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress), + ).to.be.rejectedWith(constants.REVERT); }); it('should throw if there is not a positive spread', async () => { -- cgit v1.2.3