From a1aee7111a66f217f21a19f7f9944b40aac659d3 Mon Sep 17 00:00:00 2001 From: Michael Zhu Date: Mon, 22 Jul 2019 16:46:07 -0700 Subject: [PATCH] minor fixes --- .../contracts/src/MixinExchangeWrapper.sol | 20 ++++++------------- .../contracts/src/libs/LibForwarderErrors.sol | 1 + .../test/utils/forwarder_test_factory.ts | 14 ++++++++++--- packages/types/src/index.ts | 2 ++ 4 files changed, 20 insertions(+), 17 deletions(-) diff --git a/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol b/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol index 6a59d421ff..55f3e6f9a1 100644 --- a/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol +++ b/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol @@ -181,24 +181,17 @@ contract MixinExchangeWrapper is ) { uint256 ordersLength = orders.length; - for (uint256 i = 0; i != ordersLength; i++) { require( orders[i].makerAssetData.equals(orders[0].makerAssetData), "MAKER_ASSET_MISMATCH" ); - // Calculate the remaining amount of makerAsset to buy - uint256 remainingMakerAssetFillAmount = _safeSub(makerAssetFillAmount, makerAssetAcquiredAmount); - - // Convert the remaining amount of makerAsset to buy into remaining amount - // of takerAsset to sell, assuming entire amount can be sold in the current order. - // We round up because the exchange rate computed by fillOrder rounds in favor - // of the Maker. In this case we want to overestimate the amount of takerAsset. + // Calculate the remaining amount of takerAsset to sell uint256 remainingTakerAssetFillAmount = _getPartialAmountCeil( orders[i].takerAssetAmount, orders[i].makerAssetAmount, - remainingMakerAssetFillAmount + _safeSub(makerAssetFillAmount, makerAssetAcquiredAmount) ); // Attempt to sell the remaining amount of takerAsset @@ -207,7 +200,6 @@ contract MixinExchangeWrapper is remainingTakerAssetFillAmount, signatures[i] ); - // Update amounts filled and fees paid by maker and taker if (orders[i].makerAssetData.equals(orders[i].takerFeeAssetData)) { makerAssetAcquiredAmount = _safeAdd( @@ -221,23 +213,23 @@ contract MixinExchangeWrapper is } else { makerAssetAcquiredAmount = _safeAdd( makerAssetAcquiredAmount, - _safeSub(singleFillResults.makerAssetFilledAmount, singleFillResults.takerFeePaid) + singleFillResults.makerAssetFilledAmount ); wethSpentAmount = _safeAdd( wethSpentAmount, - singleFillResults.takerAssetFilledAmount + _safeAdd(singleFillResults.takerAssetFilledAmount, singleFillResults.takerFeePaid) ); } _addFillResults(totalFillResults, singleFillResults); // Stop execution if the entire amount of makerAsset has been bought - if (makerAssetAcquiredAmount >= makerAssetFillAmount) { + if (totalFillResults.makerAssetFilledAmount >= makerAssetFillAmount) { break; } } require( - makerAssetAcquiredAmount >= makerAssetFillAmount, + totalFillResults.makerAssetFilledAmount >= makerAssetFillAmount, "COMPLETE_FILL_FAILED" ); return (totalFillResults, wethSpentAmount, makerAssetAcquiredAmount); diff --git a/contracts/exchange-forwarder/contracts/src/libs/LibForwarderErrors.sol b/contracts/exchange-forwarder/contracts/src/libs/LibForwarderErrors.sol index 15ab1899b7..ddbb32ac5d 100644 --- a/contracts/exchange-forwarder/contracts/src/libs/LibForwarderErrors.sol +++ b/contracts/exchange-forwarder/contracts/src/libs/LibForwarderErrors.sol @@ -31,4 +31,5 @@ contract LibForwarderErrors { string constant DEFAULT_FUNCTION_WETH_CONTRACT_ONLY = "DEFAULT_FUNCTION_WETH_CONTRACT_ONLY"; // Fallback function may only be used for WETH withdrawals. string constant INVALID_MSG_VALUE = "INVALID_MSG_VALUE"; // msg.value must be greater than 0. string constant INVALID_AMOUNT = "INVALID_AMOUNT"; // Amount must equal 1. + string constant MAKER_ASSET_MISMATCH = "MAKER_ASSET_MISMATCH"; // Maker asset must be the same across all orders in a given input } diff --git a/contracts/exchange-forwarder/test/utils/forwarder_test_factory.ts b/contracts/exchange-forwarder/test/utils/forwarder_test_factory.ts index 2c4ffb14c2..bbc2628ae8 100644 --- a/contracts/exchange-forwarder/test/utils/forwarder_test_factory.ts +++ b/contracts/exchange-forwarder/test/utils/forwarder_test_factory.ts @@ -135,11 +135,13 @@ export class ForwarderTestFactory { expectedResults.takerAssetFillAmount, forwarderFeeOptions.baseFeePercentage, ); + const feePercentage = ForwarderTestFactory.getPercentageOfValue(constants.PERCENTAGE_DENOMINATOR, forwarderFeeOptions.baseFeePercentage); + const ethValue = expectedResults.takerAssetFillAmount .plus(expectedResults.wethFees) + .plus(expectedResults.maxOversoldWeth) .plus(ethSpentOnForwarderFee) .plus(ethValueAdjustment); - const feePercentage = ForwarderTestFactory.getPercentageOfValue(constants.PERCENTAGE_DENOMINATOR, forwarderFeeOptions.baseFeePercentage); if (ethValueAdjustment.isNegative()) { return expectTransactionFailedAsync( @@ -190,10 +192,12 @@ export class ForwarderTestFactory { expectedResults.takerAssetFillAmount, forwarderFeeOptions.baseFeePercentage, ); + const feePercentage = ForwarderTestFactory.getPercentageOfValue(constants.PERCENTAGE_DENOMINATOR, forwarderFeeOptions.baseFeePercentage); + const ethValue = expectedResults.takerAssetFillAmount .plus(expectedResults.wethFees) + .plus(expectedResults.maxOversoldWeth) .plus(ethSpentOnForwarderFee); - const feePercentage = ForwarderTestFactory.getPercentageOfValue(constants.PERCENTAGE_DENOMINATOR, forwarderFeeOptions.baseFeePercentage); const tx = await this._forwarderWrapper.marketSellOrdersWithEthAsync( orders, @@ -233,7 +237,11 @@ export class ForwarderTestFactory { const fowarderFeeRecipientEthBalanceAfter = await web3Wrapper.getBalanceInWeiAsync(this._forwarderFeeRecipientAddress); const newBalances = await this._erc20Wrapper.getBalancesAsync(); - expect(takerEthBalanceAfter).to.be.bignumber.equal(takerEthBalanceBefore.minus(totalEthSpent)); + expectBalanceWithin( + takerEthBalanceAfter, + takerEthBalanceBefore.minus(totalEthSpent).minus(expectedResults.maxOversoldWeth), + takerEthBalanceBefore.minus(totalEthSpent), + ); if (ethSpentOnForwarderFee.gt(0)) { expect(fowarderFeeRecipientEthBalanceAfter).to.be.bignumber.equal( forwarderFeeRecipientEthBalanceBefore.plus(ethSpentOnForwarderFee), diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index f9c2205d83..6223a85cc6 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -340,6 +340,8 @@ export enum RevertReason { TargetNotEven = 'TARGET_NOT_EVEN', UnexpectedStaticCallResult = 'UNEXPECTED_STATIC_CALL_RESULT', TransfersSuccessful = 'TRANSFERS_SUCCESSFUL', + // Forwader + MakerAssetMismatch = 'MAKER_ASSET_MISMATCH', } export enum StatusCodes {