From ecfbd6280f245c2649bb4e66e9e03fe3006306da Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Thu, 28 May 2020 11:55:25 -0400 Subject: [PATCH] `@0x/contracts-zero-ex`: Address review feedback. --- .../errors/LibTransformERC20RichErrors.sol | 9 +- .../src/transformers/FillQuoteTransformer.sol | 125 ++++++++++-------- .../src/transformers/PayTakerTransformer.sol | 1 - .../src/transformers/WethTransformer.sol | 8 +- .../contracts/test/TestDelegateCaller.sol | 1 - .../zero-ex/src/transformer_data_encoders.ts | 16 ++- .../fill_quote_transformer_test.ts | 49 ++++--- .../transformers/weth_transformer_test.ts | 7 +- 8 files changed, 133 insertions(+), 83 deletions(-) diff --git a/contracts/zero-ex/contracts/src/errors/LibTransformERC20RichErrors.sol b/contracts/zero-ex/contracts/src/errors/LibTransformERC20RichErrors.sol index cc1352eac4..2cee9e18b7 100644 --- a/contracts/zero-ex/contracts/src/errors/LibTransformERC20RichErrors.sol +++ b/contracts/zero-ex/contracts/src/errors/LibTransformERC20RichErrors.sol @@ -130,7 +130,13 @@ library LibTransformERC20RichErrors { ); } + enum InvalidTransformDataErrorCode { + INVALID_TOKENS, + INVALID_ARRAY_LENGTH + } + function InvalidTransformDataError( + InvalidTransformDataErrorCode errorCode, bytes memory transformData ) internal @@ -138,7 +144,8 @@ library LibTransformERC20RichErrors { returns (bytes memory) { return abi.encodeWithSelector( - bytes4(keccak256("InvalidTransformDataError(bytes)")), + bytes4(keccak256("InvalidTransformDataError(uint8,bytes)")), + errorCode, transformData ); } diff --git a/contracts/zero-ex/contracts/src/transformers/FillQuoteTransformer.sol b/contracts/zero-ex/contracts/src/transformers/FillQuoteTransformer.sol index 39604ce763..bd76a7c4d1 100644 --- a/contracts/zero-ex/contracts/src/transformers/FillQuoteTransformer.sol +++ b/contracts/zero-ex/contracts/src/transformers/FillQuoteTransformer.sol @@ -35,8 +35,16 @@ import "./LibERC20Transformer.sol"; contract FillQuoteTransformer is Transformer { + /// @dev Whether we are performing a market sell or buy. + enum Side { + Sell, + Buy + } + /// @dev Transform data to ABI-encode and pass into `transform()`. struct TransformData { + // Whether we aer performing a market sell or buy. + Side side; // The token being sold. // This should be an actual token, not the ETH pseudo-token. IERC20TokenV06 sellToken; @@ -52,11 +60,10 @@ contract FillQuoteTransformer is // For sells, this will be the maximum sell amount (taker asset). // For buys, this will be the maximum buy amount (maker asset). uint256[] maxOrderFillAmounts; - // Amount of `sellToken` to sell. May be `uint256(-1)` to sell entire - // amount of `sellToken` received. Zero if performing a market buy. - uint256 sellAmount; - // Amount of `buyToken` to buy. Zero if performing a market sell. - uint256 buyAmount; + // Amount of `sellToken` to sell or `buyToken` to buy. + // For sells, this may be `uint256(-1)` to sell the entire balance of + // `sellToken`. + uint256 fillAmount; } /// @dev Results of a call to `_fillOrder()`. @@ -96,7 +103,6 @@ contract FillQuoteTransformer is /// @dev Sell this contract's entire balance of of `sellToken` in exchange /// for `buyToken` by filling `orders`. Protocol fees should be attached /// to this call. `buyToken` and excess ETH will be transferred back to the caller. - /// This function cannot be re-entered. /// @param data_ ABI-encoded `TransformData`. /// @return rlpDeploymentNonce RLP-encoded deployment nonce of the deployer /// when this transformer was deployed. This is used to verify that @@ -113,23 +119,29 @@ contract FillQuoteTransformer is TransformData memory data = abi.decode(data_, (TransformData)); // Validate data fields. - if (data.sellToken.isTokenETH() || - data.buyToken.isTokenETH() || - data.orders.length != data.signatures.length) - { - LibTransformERC20RichErrors.InvalidTransformDataError(data_).rrevert(); + if (data.sellToken.isTokenETH() || data.buyToken.isTokenETH()) { + LibTransformERC20RichErrors.InvalidTransformDataError( + LibTransformERC20RichErrors.InvalidTransformDataErrorCode.INVALID_TOKENS, + data_ + ).rrevert(); + } + if (data.orders.length != data.signatures.length) { + LibTransformERC20RichErrors.InvalidTransformDataError( + LibTransformERC20RichErrors.InvalidTransformDataErrorCode.INVALID_ARRAY_LENGTH, + data_ + ).rrevert(); } - // If `sellAmount == -1` and `buyAmount == 0` then we are selling - // the entire balance of `sellToken`. This is useful in cases where - // the exact sell amount is not exactly known in advance, like when - // unwrapping Chai/cUSDC/cDAI. - if (data.sellAmount == uint256(-1) && data.buyAmount == 0) { - data.sellAmount = data.sellToken.getTokenBalanceOf(address(this)); + if (data.side == Side.Sell && data.fillAmount == uint256(-1)) { + // If `sellAmount == -1 then we are selling + // the entire balance of `sellToken`. This is useful in cases where + // the exact sell amount is not exactly known in advance, like when + // unwrapping Chai/cUSDC/cDAI. + data.fillAmount = data.sellToken.getTokenBalanceOf(address(this)); } // Approve the ERC20 proxy to spend `sellToken`. - data.sellToken.approveIfBelow(erc20Proxy, data.sellAmount); + data.sellToken.approveIfBelow(erc20Proxy, data.fillAmount); // Fill the orders. uint256 singleProtocolFee = exchange.protocolFeeMultiplier().safeMul(tx.gasprice); @@ -138,14 +150,14 @@ contract FillQuoteTransformer is uint256 soldAmount = 0; for (uint256 i = 0; i < data.orders.length; ++i) { // Check if we've hit our targets. - if (data.buyAmount == 0) { + if (data.side == Side.Sell) { // Market sell check. - if (soldAmount >= data.sellAmount) { + if (soldAmount >= data.fillAmount) { break; } } else { // Market buy check. - if (boughtAmount >= data.buyAmount) { + if (boughtAmount >= data.fillAmount) { break; } } @@ -159,14 +171,14 @@ contract FillQuoteTransformer is // Fill the order. FillOrderResults memory results; - if (data.buyAmount == 0) { + if (data.side == Side.Sell) { // Market sell. results = _sellToOrder( data.buyToken, data.sellToken, data.orders[i], data.signatures[i], - data.sellAmount.safeSub(soldAmount).min256( + data.fillAmount.safeSub(soldAmount).min256( data.maxOrderFillAmounts.length > i ? data.maxOrderFillAmounts[i] : uint256(-1) @@ -180,7 +192,7 @@ contract FillQuoteTransformer is data.sellToken, data.orders[i], data.signatures[i], - data.buyAmount.safeSub(boughtAmount).min256( + data.fillAmount.safeSub(boughtAmount).min256( data.maxOrderFillAmounts.length > i ? data.maxOrderFillAmounts[i] : uint256(-1) @@ -196,24 +208,24 @@ contract FillQuoteTransformer is } // Ensure we hit our targets. - if (data.buyAmount == 0) { + if (data.side == Side.Sell) { // Market sell check. - if (soldAmount < data.sellAmount) { + if (soldAmount < data.fillAmount) { LibTransformERC20RichErrors .IncompleteFillSellQuoteError( address(data.sellToken), soldAmount, - data.sellAmount + data.fillAmount ).rrevert(); } } else { // Market buy check. - if (boughtAmount < data.buyAmount) { + if (boughtAmount < data.fillAmount) { LibTransformERC20RichErrors .IncompleteFillBuyQuoteError( address(data.buyToken), boughtAmount, - data.buyAmount + data.fillAmount ).rrevert(); } } @@ -238,9 +250,8 @@ contract FillQuoteTransformer is private returns (FillOrderResults memory results) { - IERC20TokenV06 takerFeeToken = order.takerFeeAssetData.length == 0 - ? IERC20TokenV06(address(0)) - : _getTokenFromERC20AssetData(order.takerFeeAssetData); + IERC20TokenV06 takerFeeToken = + _getTokenFromERC20AssetData(order.takerFeeAssetData); uint256 takerTokenFillAmount = sellAmount; @@ -261,7 +272,7 @@ contract FillQuoteTransformer is takerTokenFillAmount = LibMathV06.getPartialAmountCeil( order.takerAssetAmount, order.takerAssetAmount.safeAdd(order.takerFee), - takerTokenFillAmount + sellAmount ); } else { // Only support taker or maker asset denominated taker fees. @@ -306,24 +317,27 @@ contract FillQuoteTransformer is private returns (FillOrderResults memory results) { - IERC20TokenV06 takerFeeToken = order.takerFeeAssetData.length == 0 - ? IERC20TokenV06(address(0)) - : _getTokenFromERC20AssetData(order.takerFeeAssetData); - - uint256 makerTokenFillAmount = buyAmount; + IERC20TokenV06 takerFeeToken = + _getTokenFromERC20AssetData(order.takerFeeAssetData); + // Compute the default taker token fill amount. + uint256 takerTokenFillAmount = LibMathV06.getPartialAmountCeil( + buyAmount, + order.makerAssetAmount, + order.takerAssetAmount + ); if (order.takerFee != 0) { if (takerFeeToken == makerToken) { // Taker fee is payable in the maker token. - // Increase the fill amount to account for maker tokens being - // lost to the taker fee. - // makerTokenFillAmount' = - // (order.makerAssetAmount * makerTokenFillAmount) / + // Adjust the taker token fill amount to account for maker + // tokens being lost to the taker fee. + // takerTokenFillAmount' = + // (order.takerAssetAmount * buyAmount) / // (order.makerAssetAmount - order.takerFee) - makerTokenFillAmount = LibMathV06.getPartialAmountCeil( - order.makerAssetAmount, + takerTokenFillAmount = LibMathV06.getPartialAmountCeil( + buyAmount, order.makerAssetAmount.safeSub(order.takerFee), - makerTokenFillAmount + order.takerAssetAmount ); // Approve the proxy to spend the maker token. // It isn't worth computing the actual taker fee @@ -338,14 +352,10 @@ contract FillQuoteTransformer is } } - // Convert maker fill amount to taker fill amount. - uint256 takerTokenFillAmount = LibSafeMathV06.min256( + // Clamp to order size. + takerTokenFillAmount = LibSafeMathV06.min256( order.takerAssetAmount, - LibMathV06.getPartialAmountCeil( - makerTokenFillAmount, - order.makerAssetAmount, - order.takerAssetAmount - ) + takerTokenFillAmount ); // Perform the fill. @@ -380,7 +390,7 @@ contract FillQuoteTransformer is returns (FillOrderResults memory results) { // Track changes in the maker token balance. - results.makerTokenBoughtAmount = makerToken.balanceOf(address(this)); + uint256 initialMakerTokenBalance = makerToken.balanceOf(address(this)); try exchange.fillOrder {value: protocolFee} @@ -389,7 +399,7 @@ contract FillQuoteTransformer is { // Update maker quantity based on changes in token balances. results.makerTokenBoughtAmount = makerToken.balanceOf(address(this)) - .safeSub(results.makerTokenBoughtAmount); + .safeSub(initialMakerTokenBalance); // We can trust the other fill result quantities. results.protocolFeePaid = fillResults.protocolFeePaid; results.takerTokenSoldAmount = fillResults.takerAssetFilledAmount; @@ -400,18 +410,21 @@ contract FillQuoteTransformer is results.takerTokenSoldAmount.safeAdd(fillResults.takerFeePaid); } } catch (bytes memory) { - // If the fill fails, zero out fill quantities. - results.makerTokenBoughtAmount = 0; + // Swallow failures, leaving all results as zero. } } /// @dev Extract the token from plain ERC20 asset data. + /// If the asset-data is empty, a zero token address will be returned. /// @param assetData The order asset data. function _getTokenFromERC20AssetData(bytes memory assetData) private pure returns (IERC20TokenV06 token) { + if (assetData.length == 0) { + return IERC20TokenV06(address(0)); + } if (assetData.length != 36 || LibBytesV06.readBytes4(assetData, 0) != ERC20_ASSET_PROXY_ID) { diff --git a/contracts/zero-ex/contracts/src/transformers/PayTakerTransformer.sol b/contracts/zero-ex/contracts/src/transformers/PayTakerTransformer.sol index 7c09c9ddd4..095aa08b4a 100644 --- a/contracts/zero-ex/contracts/src/transformers/PayTakerTransformer.sol +++ b/contracts/zero-ex/contracts/src/transformers/PayTakerTransformer.sol @@ -49,7 +49,6 @@ contract PayTakerTransformer is /// @dev Create this contract. /// @param deploymentNonce_ The nonce of the deployer when deploying this contract. - /// @dev Construct the transformer and store the WETH address in an immutable. constructor(uint256 deploymentNonce_) public Transformer(deploymentNonce_) diff --git a/contracts/zero-ex/contracts/src/transformers/WethTransformer.sol b/contracts/zero-ex/contracts/src/transformers/WethTransformer.sol index 35678dbf17..78a442b2de 100644 --- a/contracts/zero-ex/contracts/src/transformers/WethTransformer.sol +++ b/contracts/zero-ex/contracts/src/transformers/WethTransformer.sol @@ -47,10 +47,9 @@ contract WethTransformer is using LibSafeMathV06 for uint256; using LibERC20Transformer for IERC20TokenV06; - /// @dev Create this contract. + /// @dev Construct the transformer and store the WETH address in an immutable. /// @param weth_ The weth token. /// @param deploymentNonce_ The nonce of the deployer when deploying this contract. - /// @dev Construct the transformer and store the WETH address in an immutable. constructor(IEtherTokenV06 weth_, uint256 deploymentNonce_) public Transformer(deploymentNonce_) @@ -74,7 +73,10 @@ contract WethTransformer is { TransformData memory data = abi.decode(data_, (TransformData)); if (!data.token.isTokenETH() && data.token != weth) { - LibTransformERC20RichErrors.InvalidTransformDataError(data_).rrevert(); + LibTransformERC20RichErrors.InvalidTransformDataError( + LibTransformERC20RichErrors.InvalidTransformDataErrorCode.INVALID_TOKENS, + data_ + ).rrevert(); } uint256 amount = data.amount; diff --git a/contracts/zero-ex/contracts/test/TestDelegateCaller.sol b/contracts/zero-ex/contracts/test/TestDelegateCaller.sol index 0fcbf3aef5..7996e03ecf 100644 --- a/contracts/zero-ex/contracts/test/TestDelegateCaller.sol +++ b/contracts/zero-ex/contracts/test/TestDelegateCaller.sol @@ -20,7 +20,6 @@ pragma solidity ^0.6.5; pragma experimental ABIEncoderV2; - contract TestDelegateCaller { function executeDelegateCall( address target, diff --git a/contracts/zero-ex/src/transformer_data_encoders.ts b/contracts/zero-ex/src/transformer_data_encoders.ts index b44bea7631..d9da6a0e7c 100644 --- a/contracts/zero-ex/src/transformer_data_encoders.ts +++ b/contracts/zero-ex/src/transformer_data_encoders.ts @@ -26,6 +26,7 @@ export const fillQuoteTransformerDataEncoder = AbiEncoder.create([ name: 'data', type: 'tuple', components: [ + { name: 'side', type: 'uint8' }, { name: 'sellToken', type: 'address' }, { name: 'buyToken', type: 'address' }, { @@ -35,23 +36,30 @@ export const fillQuoteTransformerDataEncoder = AbiEncoder.create([ }, { name: 'signatures', type: 'bytes[]' }, { name: 'maxOrderFillAmounts', type: 'uint256[]' }, - { name: 'sellAmount', type: 'uint256' }, - { name: 'buyAmount', type: 'uint256' }, + { name: 'fillAmount', type: 'uint256' }, ], }, ]); +/** + * Market operation for `FillQuoteTransformerData`. + */ +export enum FillQuoteTransformerSide { + Sell, + Buy, +} + /** * `FillQuoteTransformer.TransformData` */ export interface FillQuoteTransformerData { + side: FillQuoteTransformerSide; sellToken: string; buyToken: string; orders: Array>; signatures: string[]; maxOrderFillAmounts: BigNumber[]; - sellAmount: BigNumber; - buyAmount: BigNumber; + fillAmount: BigNumber; } /** diff --git a/contracts/zero-ex/test/transformers/fill_quote_transformer_test.ts b/contracts/zero-ex/test/transformers/fill_quote_transformer_test.ts index 7f8c312e45..5460c62d94 100644 --- a/contracts/zero-ex/test/transformers/fill_quote_transformer_test.ts +++ b/contracts/zero-ex/test/transformers/fill_quote_transformer_test.ts @@ -13,7 +13,11 @@ import { BigNumber, hexUtils, ZeroExRevertErrors } from '@0x/utils'; import * as _ from 'lodash'; import { rlpEncodeNonce } from '../../src/nonce_utils'; -import { encodeFillQuoteTransformerData, FillQuoteTransformerData } from '../../src/transformer_data_encoders'; +import { + encodeFillQuoteTransformerData, + FillQuoteTransformerData, + FillQuoteTransformerSide, +} from '../../src/transformer_data_encoders'; import { artifacts } from '../artifacts'; import { FillQuoteTransformerContract, @@ -217,13 +221,13 @@ blockchainTests.resets('FillQuoteTransformer', env => { function encodeTransformData(fields: Partial = {}): string { return encodeFillQuoteTransformerData({ + side: FillQuoteTransformerSide.Sell, sellToken: takerToken.address, buyToken: makerToken.address, orders: [], signatures: [], maxOrderFillAmounts: [], - sellAmount: MAX_UINT256, - buyAmount: ZERO_AMOUNT, + fillAmount: MAX_UINT256, ...fields, }); } @@ -455,7 +459,7 @@ blockchainTests.resets('FillQuoteTransformer', env => { encodeTransformData({ orders, signatures, - sellAmount: qfr.takerAssetSpent, + fillAmount: qfr.takerAssetSpent, }), ) .awaitTransactionSuccessAsync({ value: qfr.protocolFeePaid }); @@ -479,7 +483,7 @@ blockchainTests.resets('FillQuoteTransformer', env => { encodeTransformData({ orders, signatures, - sellAmount: qfr.takerAssetSpent, + fillAmount: qfr.takerAssetSpent, }), ) .awaitTransactionSuccessAsync({ value: qfr.protocolFeePaid }); @@ -614,7 +618,8 @@ blockchainTests.resets('FillQuoteTransformer', env => { encodeTransformData({ orders, signatures, - buyAmount: qfr.makerAssetBought, + side: FillQuoteTransformerSide.Buy, + fillAmount: qfr.makerAssetBought, }), ) .awaitTransactionSuccessAsync({ value: qfr.protocolFeePaid }); @@ -636,7 +641,8 @@ blockchainTests.resets('FillQuoteTransformer', env => { encodeTransformData({ orders, signatures, - buyAmount: qfr.makerAssetBought, + side: FillQuoteTransformerSide.Buy, + fillAmount: qfr.makerAssetBought, }), ) .awaitTransactionSuccessAsync({ value: qfr.protocolFeePaid }); @@ -661,7 +667,8 @@ blockchainTests.resets('FillQuoteTransformer', env => { encodeTransformData({ orders, signatures, - buyAmount: qfr.makerAssetBought, + side: FillQuoteTransformerSide.Buy, + fillAmount: qfr.makerAssetBought, }), ) .awaitTransactionSuccessAsync({ value: qfr.protocolFeePaid }); @@ -684,7 +691,8 @@ blockchainTests.resets('FillQuoteTransformer', env => { encodeTransformData({ orders, signatures, - buyAmount: qfr.makerAssetBought, + side: FillQuoteTransformerSide.Buy, + fillAmount: qfr.makerAssetBought, }), ) .awaitTransactionSuccessAsync({ value: maxProtocolFees }); @@ -709,7 +717,8 @@ blockchainTests.resets('FillQuoteTransformer', env => { encodeTransformData({ orders, signatures, - buyAmount: qfr.makerAssetBought, + side: FillQuoteTransformerSide.Buy, + fillAmount: qfr.makerAssetBought, }), ) .awaitTransactionSuccessAsync({ value: qfr.protocolFeePaid }); @@ -736,7 +745,8 @@ blockchainTests.resets('FillQuoteTransformer', env => { encodeTransformData({ orders, signatures, - buyAmount: qfr.makerAssetBought, + side: FillQuoteTransformerSide.Buy, + fillAmount: qfr.makerAssetBought, }), ) .awaitTransactionSuccessAsync({ value: qfr.protocolFeePaid }); @@ -760,7 +770,8 @@ blockchainTests.resets('FillQuoteTransformer', env => { encodeTransformData({ orders, signatures, - buyAmount: qfr.makerAssetBought.plus(1), + side: FillQuoteTransformerSide.Buy, + fillAmount: qfr.makerAssetBought.plus(1), }), ) .awaitTransactionSuccessAsync({ value: qfr.protocolFeePaid }); @@ -789,7 +800,8 @@ blockchainTests.resets('FillQuoteTransformer', env => { encodeTransformData({ orders, signatures, - buyAmount: qfr.makerAssetBought, + side: FillQuoteTransformerSide.Buy, + fillAmount: qfr.makerAssetBought, }), ) .awaitTransactionSuccessAsync({ value: qfr.protocolFeePaid }); @@ -812,7 +824,8 @@ blockchainTests.resets('FillQuoteTransformer', env => { encodeTransformData({ orders, signatures, - buyAmount: qfr.makerAssetBought, + side: FillQuoteTransformerSide.Buy, + fillAmount: qfr.makerAssetBought, }), ) .awaitTransactionSuccessAsync({ value: qfr.protocolFeePaid }); @@ -835,7 +848,8 @@ blockchainTests.resets('FillQuoteTransformer', env => { encodeTransformData({ orders, signatures, - buyAmount: qfr.makerAssetBought, + side: FillQuoteTransformerSide.Buy, + fillAmount: qfr.makerAssetBought, }), ) .awaitTransactionSuccessAsync({ value: qfr.protocolFeePaid }); @@ -855,7 +869,8 @@ blockchainTests.resets('FillQuoteTransformer', env => { encodeTransformData({ orders, signatures, - buyAmount: qfr.makerAssetBought, + side: FillQuoteTransformerSide.Buy, + fillAmount: qfr.makerAssetBought, // Skip the first order. maxOrderFillAmounts: [ZERO_AMOUNT], }), @@ -879,6 +894,8 @@ blockchainTests.resets('FillQuoteTransformer', env => { encodeTransformData({ orders, signatures, + side: FillQuoteTransformerSide.Buy, + fillAmount: qfr.makerAssetBought, }), ) .callAsync({ value: qfr.protocolFeePaid }); diff --git a/contracts/zero-ex/test/transformers/weth_transformer_test.ts b/contracts/zero-ex/test/transformers/weth_transformer_test.ts index e5c1cc8c26..6548e40872 100644 --- a/contracts/zero-ex/test/transformers/weth_transformer_test.ts +++ b/contracts/zero-ex/test/transformers/weth_transformer_test.ts @@ -61,7 +61,12 @@ blockchainTests.resets('WethTransformer', env => { const tx = host .executeTransform(amount, transformer.address, data) .awaitTransactionSuccessAsync({ value: amount }); - return expect(tx).to.revertWith(new ZeroExRevertErrors.TransformERC20.InvalidTransformDataError(data)); + return expect(tx).to.revertWith( + new ZeroExRevertErrors.TransformERC20.InvalidTransformDataError( + ZeroExRevertErrors.TransformERC20.InvalidTransformDataErrorCode.InvalidTokens, + data, + ), + ); }); it('can unwrap WETH', async () => {