From 5b438bcbd81acbb89cc529902fb573e1b3ef470f Mon Sep 17 00:00:00 2001 From: Jacob Evans Date: Thu, 12 Mar 2020 14:03:50 +1000 Subject: [PATCH] Forwarder Market sell specified amount or throw --- .../contracts/src/Forwarder.sol | 66 ++++++++++ .../contracts/src/MixinExchangeWrapper.sol | 116 ++++++++++++++---- .../src/libs/LibForwarderRichErrors.sol | 19 +++ .../test/forwarder/forwarder_test.ts | 21 +++- .../test/forwarder/forwarder_test_factory.ts | 45 +++++++ .../exchange-forwarder/revert_errors.ts | 14 +++ 6 files changed, 258 insertions(+), 23 deletions(-) diff --git a/contracts/exchange-forwarder/contracts/src/Forwarder.sol b/contracts/exchange-forwarder/contracts/src/Forwarder.sol index b8c76f5345..fbf3754d91 100644 --- a/contracts/exchange-forwarder/contracts/src/Forwarder.sol +++ b/contracts/exchange-forwarder/contracts/src/Forwarder.sol @@ -151,6 +151,72 @@ contract Forwarder is _unwrapAndTransferEth(wethRemaining); } + /// @dev Purchases as much of orders' makerAssets as possible by selling the specified amount of ETH + /// accounting for order and forwarder fees. This functions throws if ethSellAmount was not reached. + /// @param orders Array of order specifications used containing desired makerAsset and WETH as takerAsset. + /// @param ethSellAmount Desired amount of ETH to sell. + /// @param signatures Proofs that orders have been created by makers. + /// @param ethFeeAmounts Amounts of ETH, denominated in Wei, that are paid to corresponding feeRecipients. + /// @param feeRecipients Addresses that will receive ETH when orders are filled. + /// @return wethSpentAmount Amount of WETH spent on the given set of orders. + /// @return makerAssetAcquiredAmount Amount of maker asset acquired from the given set of orders. + function marketSellAmountWithEth( + LibOrder.Order[] memory orders, + uint256 ethSellAmount, + bytes[] memory signatures, + uint256[] memory ethFeeAmounts, + address payable[] memory feeRecipients + ) + public + payable + returns ( + uint256 wethSpentAmount, + uint256 makerAssetAcquiredAmount + ) + { + if (ethSellAmount > msg.value) { + LibRichErrors.rrevert(LibForwarderRichErrors.CompleteSellFailedError( + ethSellAmount, + msg.value + )); + } + // Pay ETH affiliate fees to all feeRecipient addresses + uint256 wethRemaining = _transferEthFeesAndWrapRemaining( + ethFeeAmounts, + feeRecipients + ); + // Need enough remaining to ensure we can sell ethSellAmount + if (wethRemaining < ethSellAmount) { + LibRichErrors.rrevert(LibForwarderRichErrors.OverspentWethError( + wethRemaining, + ethSellAmount + )); + } + // Spends up to ethSellAmount to fill orders, transfers purchased assets to msg.sender, + // and pays WETH order fees. + ( + wethSpentAmount, + makerAssetAcquiredAmount + ) = _marketSellExactAmountNoThrow( + orders, + ethSellAmount, + signatures + ); + // Ensure we sold the specified amount (note: wethSpentAmount includes fees) + if (wethSpentAmount < ethSellAmount) { + LibRichErrors.rrevert(LibForwarderRichErrors.CompleteSellFailedError( + ethSellAmount, + wethSpentAmount + )); + } + + // Calculate amount of WETH that hasn't been spent. + wethRemaining = wethRemaining.safeSub(wethSpentAmount); + + // Refund remaining ETH to msg.sender. + _unwrapAndTransferEth(wethRemaining); + } + /// @dev Attempt to buy makerAssetBuyAmount of makerAsset by selling ETH provided with transaction. /// The Forwarder may *fill* more than makerAssetBuyAmount of the makerAsset so that it can /// pay takerFees where takerFeeAssetData == makerAssetData (i.e. percentage fees). diff --git a/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol b/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol index 1a7e2519ae..b44d5f8c14 100644 --- a/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol +++ b/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol @@ -73,6 +73,12 @@ contract MixinExchangeWrapper { EXCHANGE_V2 = IExchangeV2(_exchangeV2); } + struct SellFillResults { + uint256 wethSpentAmount; + uint256 makerAssetAcquiredAmount; + uint256 protocolFeePaid; + } + /// @dev Fills the input order. /// Returns false if the transaction would otherwise revert. /// @param order Order struct containing order specifications. @@ -115,10 +121,7 @@ contract MixinExchangeWrapper { uint256 remainingTakerAssetFillAmount ) internal - returns ( - uint256 wethSpentAmount, - uint256 makerAssetAcquiredAmount - ) + returns (SellFillResults memory sellFillResults) { // No taker fee or percentage fee if ( @@ -132,11 +135,11 @@ contract MixinExchangeWrapper { signature ); - wethSpentAmount = singleFillResults.takerAssetFilledAmount - .safeAdd(singleFillResults.protocolFeePaid); + sellFillResults.wethSpentAmount = singleFillResults.takerAssetFilledAmount; + sellFillResults.protocolFeePaid = singleFillResults.protocolFeePaid; // Subtract fee from makerAssetFilledAmount for the net amount acquired. - makerAssetAcquiredAmount = singleFillResults.makerAssetFilledAmount + sellFillResults.makerAssetAcquiredAmount = singleFillResults.makerAssetFilledAmount .safeSub(singleFillResults.takerFeePaid); // WETH fee @@ -157,20 +160,20 @@ contract MixinExchangeWrapper { ); // WETH is also spent on the taker fee, so we add it here. - wethSpentAmount = singleFillResults.takerAssetFilledAmount - .safeAdd(singleFillResults.takerFeePaid) - .safeAdd(singleFillResults.protocolFeePaid); - - makerAssetAcquiredAmount = singleFillResults.makerAssetFilledAmount; + sellFillResults.wethSpentAmount = singleFillResults.takerAssetFilledAmount + .safeAdd(singleFillResults.takerFeePaid); + sellFillResults.makerAssetAcquiredAmount = singleFillResults.makerAssetFilledAmount; + sellFillResults.protocolFeePaid = singleFillResults.protocolFeePaid; // Unsupported fee } else { LibRichErrors.rrevert(LibForwarderRichErrors.UnsupportedFeeError(order.takerFeeAssetData)); } - return (wethSpentAmount, makerAssetAcquiredAmount); + return sellFillResults; } + /// @dev Synchronously executes multiple calls of fillOrder until total amount of WETH has been sold by taker. /// @param orders Array of order specifications. /// @param wethSellAmount Desired amount of WETH to sell. @@ -211,10 +214,7 @@ contract MixinExchangeWrapper { balanceBefore = IERC20Token(tokenAddress).balanceOf(address(this)); } - ( - uint256 wethSpentAmount, - uint256 makerAssetAcquiredAmount - ) = _marketSellSingleOrder( + SellFillResults memory sellFillResults = _marketSellSingleOrder( orders[i], signatures[i], remainingTakerAssetFillAmount @@ -223,24 +223,96 @@ contract MixinExchangeWrapper { // Account for the ERC20Bridge transfering more of the maker asset than expected. if (makerAssetProxyId == erc20BridgeProxyId) { uint256 balanceAfter = IERC20Token(tokenAddress).balanceOf(address(this)); - makerAssetAcquiredAmount = LibSafeMath.max256( + sellFillResults.makerAssetAcquiredAmount = LibSafeMath.max256( balanceAfter.safeSub(balanceBefore), - makerAssetAcquiredAmount + sellFillResults.makerAssetAcquiredAmount ); } - orders[i].makerAssetData.transferOut(makerAssetAcquiredAmount); + orders[i].makerAssetData.transferOut(sellFillResults.makerAssetAcquiredAmount); totalWethSpentAmount = totalWethSpentAmount - .safeAdd(wethSpentAmount); + .safeAdd(sellFillResults.wethSpentAmount) + .safeAdd(sellFillResults.protocolFeePaid); totalMakerAssetAcquiredAmount = totalMakerAssetAcquiredAmount - .safeAdd(makerAssetAcquiredAmount); + .safeAdd(sellFillResults.makerAssetAcquiredAmount); + + // Stop execution if the entire amount of WETH has been sold + if (totalWethSpentAmount >= wethSellAmount) { + break; + } + } + } + + /// @dev Synchronously executes multiple calls of fillOrder until total amount of WETH (exclusive of protocol fee) + /// has been sold by taker. + /// @param orders Array of order specifications. + /// @param wethSellAmount Desired amount of WETH to sell. + /// @param signatures Proofs that orders have been signed by makers. + /// @return totalWethSpentAmount Total amount of WETH spent on the given orders. + /// @return totalMakerAssetAcquiredAmount Total amount of maker asset acquired from the given orders. + function _marketSellExactAmountNoThrow( + LibOrder.Order[] memory orders, + uint256 wethSellAmount, + bytes[] memory signatures + ) + internal + returns ( + uint256 totalWethSpentAmount, + uint256 totalMakerAssetAcquiredAmount + ) + { + bytes4 erc20BridgeProxyId = IAssetData(address(0)).ERC20Bridge.selector; + uint256 totalProtocolFeePaid; + + for (uint256 i = 0; i != orders.length; i++) { + // Preemptively skip to avoid division by zero in _marketSellSingleOrder + if (orders[i].makerAssetAmount == 0 || orders[i].takerAssetAmount == 0) { + continue; + } + + // The remaining amount of WETH to sell + uint256 remainingTakerAssetFillAmount = wethSellAmount + .safeSub(totalWethSpentAmount); + + // If the maker asset is ERC20Bridge, take a snapshot of the Forwarder contract's balance. + bytes4 makerAssetProxyId = orders[i].makerAssetData.readBytes4(0); + address tokenAddress; + uint256 balanceBefore; + if (makerAssetProxyId == erc20BridgeProxyId) { + tokenAddress = orders[i].makerAssetData.readAddress(16); + balanceBefore = IERC20Token(tokenAddress).balanceOf(address(this)); + } + + SellFillResults memory sellFillResults = _marketSellSingleOrder( + orders[i], + signatures[i], + remainingTakerAssetFillAmount + ); + + // Account for the ERC20Bridge transfering more of the maker asset than expected. + if (makerAssetProxyId == erc20BridgeProxyId) { + uint256 balanceAfter = IERC20Token(tokenAddress).balanceOf(address(this)); + sellFillResults.makerAssetAcquiredAmount = LibSafeMath.max256( + balanceAfter.safeSub(balanceBefore), + sellFillResults.makerAssetAcquiredAmount + ); + } + + orders[i].makerAssetData.transferOut(sellFillResults.makerAssetAcquiredAmount); + + totalWethSpentAmount = totalWethSpentAmount + .safeAdd(sellFillResults.wethSpentAmount); + totalMakerAssetAcquiredAmount = totalMakerAssetAcquiredAmount + .safeAdd(sellFillResults.makerAssetAcquiredAmount); + totalProtocolFeePaid = totalProtocolFeePaid.safeAdd(sellFillResults.protocolFeePaid); // Stop execution if the entire amount of WETH has been sold if (totalWethSpentAmount >= wethSellAmount) { break; } } + totalWethSpentAmount = totalWethSpentAmount.safeAdd(totalProtocolFeePaid); } /// @dev Executes a single call of fillOrder according to the makerAssetBuyAmount and diff --git a/contracts/exchange-forwarder/contracts/src/libs/LibForwarderRichErrors.sol b/contracts/exchange-forwarder/contracts/src/libs/LibForwarderRichErrors.sol index 9fc9d7c527..44047e040b 100644 --- a/contracts/exchange-forwarder/contracts/src/libs/LibForwarderRichErrors.sol +++ b/contracts/exchange-forwarder/contracts/src/libs/LibForwarderRichErrors.sol @@ -29,6 +29,10 @@ library LibForwarderRichErrors { bytes4 internal constant COMPLETE_BUY_FAILED_ERROR_SELECTOR = 0x91353a0c; + // bytes4(keccak256("CompleteSellFailedError(uint256,uint256)")) + bytes4 internal constant COMPLETE_SELL_FAILED_ERROR_SELECTOR = + 0x450a0219; + // bytes4(keccak256("UnsupportedFeeError(bytes)")) bytes4 internal constant UNSUPPORTED_FEE_ERROR_SELECTOR = 0x31360af1; @@ -61,6 +65,21 @@ library LibForwarderRichErrors { ); } + function CompleteSellFailedError( + uint256 expectedAssetSellAmount, + uint256 actualAssetSellAmount + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + COMPLETE_SELL_FAILED_ERROR_SELECTOR, + expectedAssetSellAmount, + actualAssetSellAmount + ); + } + function UnsupportedFeeError( bytes memory takerFeeAssetData ) diff --git a/contracts/integrations/test/forwarder/forwarder_test.ts b/contracts/integrations/test/forwarder/forwarder_test.ts index 9062b51dc9..b156066a08 100644 --- a/contracts/integrations/test/forwarder/forwarder_test.ts +++ b/contracts/integrations/test/forwarder/forwarder_test.ts @@ -157,7 +157,7 @@ blockchainTests('Forwarder integration tests', env => { ); }); }); - blockchainTests.resets('marketSellOrdersWithEth without extra fees', () => { + blockchainTests.resets.only('marketSellOrdersWithEth without extra fees', () => { it('should fill a single order without a taker fee', async () => { const orderWithoutFee = await maker.signOrderAsync(); await testFactory.marketSellTestAsync([orderWithoutFee], 0.78); @@ -448,6 +448,25 @@ blockchainTests('Forwarder integration tests', env => { }); }); }); + blockchainTests.resets('marketSellAmountWithEth', () => { + const protocolFee = new BigNumber(150000).times(DeploymentManager.gasPrice); + it('should fail if the supplied amount is not sold', async () => { + const order = await maker.signOrderAsync(); + const ethSellAmount = order.takerAssetAmount; + const revertError = new ExchangeForwarderRevertErrors.CompleteSellFailedError( + ethSellAmount, + order.takerAssetAmount.times(0.5).plus(protocolFee), + ); + await testFactory.marketSellAmountTestAsync([order], ethSellAmount, 0.5, { + revertError, + }); + }); + it('should sell the supplied amount', async () => { + const order = await maker.signOrderAsync(); + const ethSellAmount = order.takerAssetAmount; + await testFactory.marketSellAmountTestAsync([order], ethSellAmount, 1); + }); + }); blockchainTests.resets('marketBuyOrdersWithEth without extra fees', () => { it('should buy the exact amount of makerAsset in a single order', async () => { const order = await maker.signOrderAsync({ diff --git a/contracts/integrations/test/forwarder/forwarder_test_factory.ts b/contracts/integrations/test/forwarder/forwarder_test_factory.ts index a3c1467f45..c1ac563680 100644 --- a/contracts/integrations/test/forwarder/forwarder_test_factory.ts +++ b/contracts/integrations/test/forwarder/forwarder_test_factory.ts @@ -146,6 +146,51 @@ export class ForwarderTestFactory { await this._checkResultsAsync(txReceipt, orders, expectedOrderStatuses, expectedBalances); } } + public async marketSellAmountTestAsync( + orders: SignedOrder[], + ethSellAmount: BigNumber, + fractionalNumberOfOrdersToFill: number, + options: Partial = {}, + ): Promise { + const orderInfoBefore = await Promise.all( + orders.map(order => this._deployment.exchange.getOrderInfo(order).callAsync()), + ); + const expectedOrderStatuses = orderInfoBefore.map((orderInfo, i) => + fractionalNumberOfOrdersToFill >= i + 1 && !(options.noopOrders || []).includes(i) + ? OrderStatus.FullyFilled + : orderInfo.orderStatus, + ); + + const { balances: expectedBalances, wethSpentAmount } = await this._simulateForwarderFillAsync( + orders, + orderInfoBefore, + fractionalNumberOfOrdersToFill, + options, + ); + + const forwarderFeeAmounts = options.forwarderFeeAmounts || []; + const forwarderFeeRecipientAddresses = options.forwarderFeeRecipientAddresses || []; + + const tx = this._forwarder + .marketSellAmountWithEth( + orders, + ethSellAmount, + orders.map(signedOrder => signedOrder.signature), + forwarderFeeAmounts, + forwarderFeeRecipientAddresses, + ) + .awaitTransactionSuccessAsync({ + value: wethSpentAmount.plus(BigNumber.sum(0, ...forwarderFeeAmounts)), + from: this._taker.address, + }); + + if (options.revertError !== undefined) { + await expect(tx).to.revertWith(options.revertError); + } else { + const txReceipt = await tx; + await this._checkResultsAsync(txReceipt, orders, expectedOrderStatuses, expectedBalances); + } + } private async _checkResultsAsync( txReceipt: TransactionReceiptWithDecodedLogs, diff --git a/packages/utils/src/revert_errors/exchange-forwarder/revert_errors.ts b/packages/utils/src/revert_errors/exchange-forwarder/revert_errors.ts index 03dc953de8..0b078269ea 100644 --- a/packages/utils/src/revert_errors/exchange-forwarder/revert_errors.ts +++ b/packages/utils/src/revert_errors/exchange-forwarder/revert_errors.ts @@ -22,6 +22,19 @@ export class CompleteBuyFailedError extends RevertError { } } +export class CompleteSellFailedError extends RevertError { + constructor( + expectedAssetSellAmount?: BigNumber | number | string, + actualAssetSellAmount?: BigNumber | number | string, + ) { + super( + 'CompleteSellFailedError', + 'CompleteSellFailedError(uint256 expectedAssetSellAmount, uint256 actualAssetSellAmount)', + { expectedAssetSellAmount, actualAssetSellAmount }, + ); + } +} + export class UnsupportedFeeError extends RevertError { constructor(takerFeeAssetData?: string) { super('UnsupportedFeeError', 'UnsupportedFeeError(bytes takerFeeAssetData)', { takerFeeAssetData }); @@ -46,6 +59,7 @@ export class MsgValueCannotEqualZeroError extends RevertError { const types = [ UnregisteredAssetProxyError, CompleteBuyFailedError, + CompleteSellFailedError, UnsupportedFeeError, OverspentWethError, MsgValueCannotEqualZeroError,