From 0571a96cead70d1653bb3585570a53185e96a58a Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Fri, 3 Jan 2020 23:21:39 -0500 Subject: [PATCH] Fix asset-swapper bugs and misc improvements. (#2406) * `@0x/contracts-erc20-bridge-sampler`: Add gas limits to external quote calls. `@0x/contract-addresses`: Point `erc20BridgeSampler` to new version. * `@0x/asset-swapper`: Ignore zero sample results from the sampler contract. `@0x/asset-swapper`: Allow skipping Uniswap when dealing with low precision amounts with `minUniswapDecimals` option. `@0x/asset-swapper`: Increase default `runLimit` from `1024` to `4096`. `@0x/asset-swapper`: Increase default `numSamples` from `8` to `10` `@0x/asset-swapper`: Fix ordering of optimized orders. `@0x/asset-swapper`: Fix best and worst quotes being reversed sometimes. `@0x/asset-swapper`: Fix rounding of quoted asset amounts. * `@0x/contracts-utils`: Add kovan addresses to `DeploymentConstants`. `@0x/contract-addresses`: Add kovan `ERC20BridgeSampler` address. * `@0x/asset-swapper`: Change default `minUniswapDecimals` option from 8 to 7. * `@0x/contracts-erc20-bridge-sampler`: Fix changelog. * `@0x/asset-swapper`: Revert uniswap decimals fix. * `@0x/asset-swapper`: Undo bridge slippage when computing best case quote. * `@0x/asset-swapper`: Take asset data from input orders instead of output orders in quote result calculation. * `@0x/asset-swapper`: Move `SAMPLER_CONTRACT_GAS_LIMIT` constant to `market_operation_utils/constants`. * Compare equivalent asset data * Fix redundant zero check * Update CHANGELOG * Set fee amount in fillable amounts test Co-authored-by: Jacob Evans --- packages/asset-swapper/CHANGELOG.json | 37 +++++ packages/asset-swapper/src/constants.ts | 1 + packages/asset-swapper/src/swap_quoter.ts | 2 + packages/asset-swapper/src/utils/assert.ts | 6 +- .../utils/market_operation_utils/constants.ts | 5 +- .../market_operation_utils/create_order.ts | 7 +- .../src/utils/market_operation_utils/index.ts | 20 ++- .../src/utils/protocol_fee_utils.ts | 4 +- .../src/utils/swap_quote_calculator.ts | 47 +++++- .../src/utils/swap_quote_consumer_utils.ts | 3 +- packages/asset-swapper/src/utils/utils.ts | 71 ++++++++- .../test/fillable_amounts_utils_test.ts | 6 +- .../test/market_operation_utils_test.ts | 58 +++---- .../asset-swapper/test/sorting_utils_test.ts | 4 +- .../test/swap_quote_calculator_test.ts | 6 +- packages/asset-swapper/test/utils_test.ts | 144 ++++++++++++++++++ packages/order-utils/CHANGELOG.json | 9 ++ packages/order-utils/src/asset_data_utils.ts | 23 +++ 18 files changed, 389 insertions(+), 64 deletions(-) create mode 100644 packages/asset-swapper/test/utils_test.ts diff --git a/packages/asset-swapper/CHANGELOG.json b/packages/asset-swapper/CHANGELOG.json index d47fb9b5b5..bfef1fbd00 100644 --- a/packages/asset-swapper/CHANGELOG.json +++ b/packages/asset-swapper/CHANGELOG.json @@ -1,4 +1,41 @@ [ + { + "version": "3.0.3", + "changes": [ + { + "note": "Ignore zero sample results from the sampler contract.", + "pr": 2406 + }, + { + "note": "Increase default `runLimit` from `1024` to `4096`.", + "pr": 2406 + }, + { + "note": "Increase default `numSamples` from `8` to `10`", + "pr": 2406 + }, + { + "note": "Fix ordering of optimized orders.", + "pr": 2406 + }, + { + "note": "Fix best and worst quotes being reversed sometimes.", + "pr": 2406 + }, + { + "note": "Fix rounding of quoted asset amounts.", + "pr": 2406 + }, + { + "note": "Undo bridge slippage in best case quote calculation.", + "pr": 2406 + }, + { + "note": "Compare equivalent asset data when validating quotes and checking fee asset data.", + "pr": 2421 + } + ] + }, { "version": "3.0.2", "changes": [ diff --git a/packages/asset-swapper/src/constants.ts b/packages/asset-swapper/src/constants.ts index 9f5da96326..b58dea7b02 100644 --- a/packages/asset-swapper/src/constants.ts +++ b/packages/asset-swapper/src/constants.ts @@ -83,4 +83,5 @@ export const constants = { NULL_ERC20_ASSET_DATA, PROTOCOL_FEE_UTILS_POLLING_INTERVAL_IN_MS, MARKET_UTILS_AMOUNT_BUFFER_PERCENTAGE, + BRIDGE_ASSET_DATA_PREFIX: '0xdc1600f3', }; diff --git a/packages/asset-swapper/src/swap_quoter.ts b/packages/asset-swapper/src/swap_quoter.ts index 18874fcb31..2598501b28 100644 --- a/packages/asset-swapper/src/swap_quoter.ts +++ b/packages/asset-swapper/src/swap_quoter.ts @@ -22,6 +22,7 @@ import { import { assert } from './utils/assert'; import { calculateLiquidity } from './utils/calculate_liquidity'; import { MarketOperationUtils } from './utils/market_operation_utils'; +import { constants as marketOperationConstants } from './utils/market_operation_utils/constants'; import { dummyOrderUtils } from './utils/market_operation_utils/dummy_order_utils'; import { orderPrunerUtils } from './utils/order_prune_utils'; import { OrderStateUtils } from './utils/order_state_utils'; @@ -165,6 +166,7 @@ export class SwapQuoter { const samplerContract = new IERC20BridgeSamplerContract( this._contractAddresses.erc20BridgeSampler, this.provider, + { gas: marketOperationConstants.SAMPLER_CONTRACT_GAS_LIMIT }, ); this._marketOperationUtils = new MarketOperationUtils(samplerContract, this._contractAddresses, { chainId, diff --git a/packages/asset-swapper/src/utils/assert.ts b/packages/asset-swapper/src/utils/assert.ts index 60918a6233..92d62c1183 100644 --- a/packages/asset-swapper/src/utils/assert.ts +++ b/packages/asset-swapper/src/utils/assert.ts @@ -36,13 +36,13 @@ export const assert = { ): void { _.every(orders, (order: SignedOrder, index: number) => { assert.assert( - order.takerAssetData === takerAssetData, + utils.isAssetDataEquivalent(takerAssetData, order.takerAssetData), `Expected ${variableName}[${index}].takerAssetData to be ${takerAssetData} but found ${ order.takerAssetData }`, ); assert.assert( - order.makerAssetData === makerAssetData, + utils.isAssetDataEquivalent(makerAssetData, order.makerAssetData), `Expected ${variableName}[${index}].makerAssetData to be ${makerAssetData} but found ${ order.makerAssetData }`, @@ -72,7 +72,7 @@ export const assert = { }, isValidForwarderSignedOrder(variableName: string, order: SignedOrder, wethAssetData: string): void { assert.assert( - order.takerAssetData === wethAssetData, + utils.isExactAssetData(order.takerAssetData, wethAssetData), `Expected ${variableName} to have takerAssetData set as ${wethAssetData}, but is ${order.takerAssetData}`, ); }, diff --git a/packages/asset-swapper/src/utils/market_operation_utils/constants.ts b/packages/asset-swapper/src/utils/market_operation_utils/constants.ts index 61a4ce25a9..ca0e6d981f 100644 --- a/packages/asset-swapper/src/utils/market_operation_utils/constants.ts +++ b/packages/asset-swapper/src/utils/market_operation_utils/constants.ts @@ -24,11 +24,11 @@ export const SELL_SOURCES = [ERC20BridgeSource.Uniswap, ERC20BridgeSource.Eth2Da export const BUY_SOURCES = [ERC20BridgeSource.Uniswap, ERC20BridgeSource.Eth2Dai]; export const DEFAULT_GET_MARKET_ORDERS_OPTS: GetMarketOrdersOpts = { - runLimit: 1024, + runLimit: 4096, excludedSources: [], bridgeSlippage: 0.0005, dustFractionThreshold: 0.01, - numSamples: 8, + numSamples: 10, noConflicts: true, }; @@ -40,4 +40,5 @@ export const constants = { DEFAULT_GET_MARKET_ORDERS_OPTS, ERC20_PROXY_ID: '0xf47261b0', WALLET_SIGNATURE: '0x04', + SAMPLER_CONTRACT_GAS_LIMIT: 10e6, }; diff --git a/packages/asset-swapper/src/utils/market_operation_utils/create_order.ts b/packages/asset-swapper/src/utils/market_operation_utils/create_order.ts index 8a3db17a4d..d150da6376 100644 --- a/packages/asset-swapper/src/utils/market_operation_utils/create_order.ts +++ b/packages/asset-swapper/src/utils/market_operation_utils/create_order.ts @@ -4,6 +4,7 @@ import { AbiEncoder, BigNumber } from '@0x/utils'; import { constants } from '../../constants'; import { SignedOrderWithFillableAmounts } from '../../types'; +import { sortingUtils } from '../../utils/sorting_utils'; import { constants as marketOperationUtilConstants } from './constants'; import { AggregationError, ERC20BridgeSource, Fill, FillData, NativeFillData, OrderDomain } from './types'; @@ -45,7 +46,7 @@ export class CreateOrderUtils { ); } } - return orders; + return sortingUtils.sortOrders(orders); } // Convert buy fills into orders. @@ -76,7 +77,7 @@ export class CreateOrderUtils { ); } } - return orders; + return sortingUtils.sortOrders(orders); } private _getBridgeAddressFromSource(source: ERC20BridgeSource): string { @@ -135,7 +136,7 @@ function createCommonOrderFields( ): CommonOrderFields { const makerAssetAmountAdjustedWithSlippage = isBuy ? makerAssetAmount - : makerAssetAmount.times(1 - slippage).integerValue(BigNumber.ROUND_UP); + : makerAssetAmount.times(1 - slippage).integerValue(BigNumber.ROUND_DOWN); const takerAssetAmountAdjustedWithSlippage = isBuy ? takerAssetAmount.times(slippage + 1).integerValue(BigNumber.ROUND_UP) : takerAssetAmount; diff --git a/packages/asset-swapper/src/utils/market_operation_utils/index.ts b/packages/asset-swapper/src/utils/market_operation_utils/index.ts index 9a7964809c..46584a57c9 100644 --- a/packages/asset-swapper/src/utils/market_operation_utils/index.ts +++ b/packages/asset-swapper/src/utils/market_operation_utils/index.ts @@ -137,7 +137,6 @@ export class MarketOperationUtils { DexOrderSampler.getSampleAmounts(makerAmount, _opts.numSamples), difference(BUY_SOURCES, _opts.excludedSources), ); - const nativeOrdersWithFillableAmounts = createSignedOrdersWithFillableAmounts( nativeOrders, fillableAmounts, @@ -266,20 +265,27 @@ function createPathsFromDexQuotes(dexQuotes: DexSample[][], noConflicts: boolean for (const quote of dexQuotes) { // Native orders can be filled in any order, so they're all root nodes. const path: Fill[] = []; - paths.push(path); + let prevSample: DexSample | undefined; // tslint:disable-next-line: prefer-for-of for (let i = 0; i < quote.length; i++) { const sample = quote[i]; - const prev = i !== 0 ? quote[i - 1] : undefined; - const parent = i !== 0 ? path[path.length - 1] : undefined; + if (sample.output.eq(0) || (prevSample && prevSample.output.gte(sample.output))) { + // Stop if the output is zero or does not increase. + break; + } path.push({ - parent, + parent: path.length !== 0 ? path[path.length - 1] : undefined, flags: sourceToFillFlags(sample.source), exclusionMask: noConflicts ? sourceToExclusionMask(sample.source) : 0, - input: sample.input.minus(prev ? prev.input : 0), - output: sample.output.minus(prev ? prev.output : 0), + input: sample.input.minus(prevSample ? prevSample.input : 0), + output: sample.output.minus(prevSample ? prevSample.output : 0), fillData: { source: sample.source }, }); + prevSample = quote[i]; + } + if (path.length > 0) { + // Don't push empty paths. + paths.push(path); } } return paths; diff --git a/packages/asset-swapper/src/utils/protocol_fee_utils.ts b/packages/asset-swapper/src/utils/protocol_fee_utils.ts index 9719b834c0..7040202ba7 100644 --- a/packages/asset-swapper/src/utils/protocol_fee_utils.ts +++ b/packages/asset-swapper/src/utils/protocol_fee_utils.ts @@ -21,8 +21,10 @@ export class ProtocolFeeUtils { return constants.PROTOCOL_FEE_MULTIPLIER; } - // tslint:disable-next-line: prefer-function-over-method public async getGasPriceEstimationOrThrowAsync(shouldHardRefresh?: boolean): Promise { + if (this.gasPriceEstimation.eq(constants.ZERO_AMOUNT)) { + return this._getGasPriceFromGasStationOrThrowAsync(); + } if (shouldHardRefresh) { return this._getGasPriceFromGasStationOrThrowAsync(); } else { diff --git a/packages/asset-swapper/src/utils/swap_quote_calculator.ts b/packages/asset-swapper/src/utils/swap_quote_calculator.ts index 0d66b79a53..6463fae9eb 100644 --- a/packages/asset-swapper/src/utils/swap_quote_calculator.ts +++ b/packages/asset-swapper/src/utils/swap_quote_calculator.ts @@ -91,16 +91,17 @@ export class SwapQuoteCalculator { } // assetData information for the result - const takerAssetData = resultOrders[0].takerAssetData; - const makerAssetData = resultOrders[0].makerAssetData; + const takerAssetData = prunedOrders[0].takerAssetData; + const makerAssetData = prunedOrders[0].makerAssetData; const bestCaseQuoteInfo = await this._calculateQuoteInfoAsync( - resultOrders, + createBestCaseOrders(resultOrders, operation, opts.bridgeSlippage), assetFillAmount, gasPrice, operation, ); - // in order to calculate the maxRate, reverse the ordersAndFillableAmounts such that they are sorted from worst rate to best rate + // in order to calculate the maxRate, reverse the ordersAndFillableAmounts + // such that they are sorted from worst rate to best rate const worstCaseQuoteInfo = await this._calculateQuoteInfoAsync( _.reverse(resultOrders.slice()), assetFillAmount, @@ -176,7 +177,7 @@ export class SwapQuoteCalculator { const makerAssetAmount = takerAssetAmountWithFees .div(adjustedFillableTakerAssetAmount) .multipliedBy(adjustedFillableMakerAssetAmount) - .integerValue(BigNumber.ROUND_CEIL); + .integerValue(BigNumber.ROUND_DOWN); return { totalMakerAssetAmount: totalMakerAssetAmount.plus(makerAssetAmount), totalTakerAssetAmount: totalTakerAssetAmount.plus(takerAssetAmount), @@ -230,7 +231,7 @@ export class SwapQuoteCalculator { const takerAssetAmountWithFees = makerFillAmount .div(adjustedFillableMakerAssetAmount) .multipliedBy(adjustedFillableTakerAssetAmount) - .integerValue(BigNumber.ROUND_CEIL); + .integerValue(BigNumber.ROUND_UP); const { takerAssetAmount, feeTakerAssetAmount } = getTakerAssetAmountBreakDown( order, @@ -291,7 +292,7 @@ function getTakerAssetAmountBreakDown( const takerAssetAmount = takerFeeAmount .div(makerAssetFillAmount) .multipliedBy(takerAssetAmountWithFees) - .integerValue(BigNumber.ROUND_CEIL); + .integerValue(BigNumber.ROUND_UP); return { takerAssetAmount, feeTakerAssetAmount: takerAssetAmountWithFees.minus(takerAssetAmount), @@ -302,3 +303,35 @@ function getTakerAssetAmountBreakDown( takerAssetAmount: takerAssetAmountWithFees, }; } + +function createBestCaseOrders( + orders: SignedOrderWithFillableAmounts[], + operation: MarketOperation, + bridgeSlippage: number, +): SignedOrderWithFillableAmounts[] { + // Scale the asset amounts to undo the bridge slippage applied to + // bridge orders. + const bestCaseOrders: SignedOrderWithFillableAmounts[] = []; + for (const order of orders) { + if (!order.makerAssetData.startsWith(constants.BRIDGE_ASSET_DATA_PREFIX)) { + bestCaseOrders.push(order); + continue; + } + bestCaseOrders.push({ + ...order, + ...(operation === MarketOperation.Sell + ? { + makerAssetAmount: order.makerAssetAmount + .dividedBy(1 - bridgeSlippage) + .integerValue(BigNumber.ROUND_DOWN), + } + : // Buy operation + { + takerAssetAmount: order.takerAssetAmount + .dividedBy(bridgeSlippage + 1) + .integerValue(BigNumber.ROUND_UP), + }), + }); + } + return bestCaseOrders; +} diff --git a/packages/asset-swapper/src/utils/swap_quote_consumer_utils.ts b/packages/asset-swapper/src/utils/swap_quote_consumer_utils.ts index 5bf7294884..8cd5815960 100644 --- a/packages/asset-swapper/src/utils/swap_quote_consumer_utils.ts +++ b/packages/asset-swapper/src/utils/swap_quote_consumer_utils.ts @@ -15,6 +15,7 @@ import { SwapQuoteConsumerError, SwapQuoteExecutionOpts, } from '../types'; +import { utils } from '../utils/utils'; import { assert } from './assert'; @@ -65,7 +66,7 @@ export const swapQuoteConsumerUtils = { return _.every(orders, order => swapQuoteConsumerUtils.isValidForwarderSignedOrder(order, wethAssetData)); }, isValidForwarderSignedOrder(order: SignedOrder, wethAssetData: string): boolean { - return order.takerAssetData === wethAssetData; + return utils.isExactAssetData(order.takerAssetData, wethAssetData); }, async getExtensionContractTypeForSwapQuoteAsync( quote: SwapQuote, diff --git a/packages/asset-swapper/src/utils/utils.ts b/packages/asset-swapper/src/utils/utils.ts index 3d5b0623b9..b8cb04e28b 100644 --- a/packages/asset-swapper/src/utils/utils.ts +++ b/packages/asset-swapper/src/utils/utils.ts @@ -1,5 +1,6 @@ -import { Order } from '@0x/types'; -import { BigNumber } from '@0x/utils'; +import { assetDataUtils } from '@0x/order-utils'; +import { AssetData, ERC20AssetData, ERC20BridgeAssetData, Order } from '@0x/types'; +import { BigNumber, NULL_BYTES } from '@0x/utils'; import { Web3Wrapper } from '@0x/web3-wrapper'; import { AbiDefinition, ContractAbi, MethodAbi } from 'ethereum-types'; import * as _ from 'lodash'; @@ -27,10 +28,10 @@ export const utils = { ) as MethodAbi | undefined; }, isOrderTakerFeePayableWithMakerAsset(order: T): boolean { - return order.takerFeeAssetData === order.makerAssetData; + return !order.takerFee.isZero() && utils.isAssetDataEquivalent(order.takerFeeAssetData, order.makerAssetData); }, isOrderTakerFeePayableWithTakerAsset(order: T): boolean { - return order.takerFeeAssetData === order.takerAssetData; + return !order.takerFee.isZero() && utils.isAssetDataEquivalent(order.takerFeeAssetData, order.takerAssetData); }, getAdjustedMakerAndTakerAmountsFromTakerFees(order: T): [BigNumber, BigNumber] { const adjustedMakerAssetAmount = utils.isOrderTakerFeePayableWithMakerAsset(order) @@ -41,4 +42,66 @@ export const utils = { : order.takerAssetAmount; return [adjustedMakerAssetAmount, adjustedTakerAssetAmount]; }, + isExactAssetData(expectedAssetData: string, actualAssetData: string): boolean { + return expectedAssetData === actualAssetData; + }, + /** + * Compare the Asset Data for equivalency. Expected is the asset data the user provided (wanted), + * actual is the asset data found or created. + */ + isAssetDataEquivalent(expectedAssetData: string, actualAssetData: string): boolean { + if (utils.isExactAssetData(expectedAssetData, actualAssetData)) { + return true; + } + const decodedExpectedAssetData = assetDataUtils.decodeAssetDataOrThrow(expectedAssetData); + const decodedActualAssetData = assetDataUtils.decodeAssetDataOrThrow(actualAssetData); + // ERC20 === ERC20, ERC20 === ERC20Bridge + if ( + utils.isERC20EquivalentAssetData(decodedExpectedAssetData) && + utils.isERC20EquivalentAssetData(decodedActualAssetData) + ) { + const doesTokenAddressMatch = decodedExpectedAssetData.tokenAddress === decodedActualAssetData.tokenAddress; + return doesTokenAddressMatch; + } + // ERC1155 === ERC1155 + if ( + assetDataUtils.isERC1155TokenAssetData(decodedExpectedAssetData) && + assetDataUtils.isERC1155TokenAssetData(decodedActualAssetData) + ) { + const doesTokenAddressMatch = decodedExpectedAssetData.tokenAddress === decodedActualAssetData.tokenAddress; + // IDs may be out of order yet still equivalent + // i.e (["a", "b"], [1,2]) === (["b", "a"], [2, 1]) + // (["a", "b"], [2,1]) !== (["b", "a"], [2, 1]) + const hasAllIds = decodedExpectedAssetData.tokenIds.every( + id => decodedActualAssetData.tokenIds.findIndex(v => id.eq(v)) !== -1, + ); + const hasAllValues = decodedExpectedAssetData.tokenIds.every((id, i) => + decodedExpectedAssetData.tokenValues[i].eq( + decodedActualAssetData.tokenValues[decodedActualAssetData.tokenIds.findIndex(v => id.eq(v))], + ), + ); + // If expected contains callback data, ensure it is present + // if actual has callbackdata and expected provided none then ignore it + const hasEquivalentCallback = + decodedExpectedAssetData.callbackData === NULL_BYTES || + decodedExpectedAssetData.callbackData === decodedActualAssetData.callbackData; + return doesTokenAddressMatch && hasAllIds && hasAllValues && hasEquivalentCallback; + } + // ERC721 === ERC721 + if ( + assetDataUtils.isERC721TokenAssetData(decodedExpectedAssetData) || + assetDataUtils.isERC721TokenAssetData(decodedActualAssetData) + ) { + // Asset Data should exactly match for ERC721 + return utils.isExactAssetData(expectedAssetData, actualAssetData); + } + + // TODO(dekz): Unsupported cases + // ERCXX(token) === MAP(token, staticCall) + // MAP(a, b) === MAP(b, a) === MAP(b, a, staticCall) + return false; + }, + isERC20EquivalentAssetData(assetData: AssetData): assetData is ERC20AssetData | ERC20BridgeAssetData { + return assetDataUtils.isERC20TokenAssetData(assetData) || assetDataUtils.isERC20BridgeAssetData(assetData); + }, }; diff --git a/packages/asset-swapper/test/fillable_amounts_utils_test.ts b/packages/asset-swapper/test/fillable_amounts_utils_test.ts index b93e964def..d9de870ab1 100644 --- a/packages/asset-swapper/test/fillable_amounts_utils_test.ts +++ b/packages/asset-swapper/test/fillable_amounts_utils_test.ts @@ -12,13 +12,14 @@ chaiSetup.configure(); const expect = chai.expect; // tslint:disable:custom-no-magic-numbers -const FAKE_ERC20_TAKER_ASSET_DATA = '0xf47261b22222222222222222222222222222222222222222222222222222222222222222'; -const FAKE_ERC20_MAKER_ASSET_DATA = '0xf47261b11111111111111111111111111111111111111111111111111111111111111111'; +const FAKE_ERC20_TAKER_ASSET_DATA = '0xf47261b02222222222222222222222222222222222222222222222222222222222222222'; +const FAKE_ERC20_MAKER_ASSET_DATA = '0xf47261b01111111111111111111111111111111111111111111111111111111111111111'; const TAKER_ASSET_DENOMINATED_TAKER_FEE_ORDER = testOrderFactory.generateTestSignedOrderWithFillableAmounts({ takerAssetData: FAKE_ERC20_TAKER_ASSET_DATA, makerAssetData: FAKE_ERC20_MAKER_ASSET_DATA, takerFeeAssetData: FAKE_ERC20_TAKER_ASSET_DATA, + takerFee: baseUnitAmount(2), fillableMakerAssetAmount: baseUnitAmount(5), fillableTakerAssetAmount: baseUnitAmount(10), fillableTakerFeeAmount: baseUnitAmount(2), @@ -28,6 +29,7 @@ const MAKER_ASSET_DENOMINATED_TAKER_FEE_ORDER = testOrderFactory.generateTestSig takerAssetData: FAKE_ERC20_TAKER_ASSET_DATA, makerAssetData: FAKE_ERC20_MAKER_ASSET_DATA, takerFeeAssetData: FAKE_ERC20_MAKER_ASSET_DATA, + takerFee: baseUnitAmount(2), fillableMakerAssetAmount: baseUnitAmount(10), fillableTakerAssetAmount: baseUnitAmount(5), fillableTakerFeeAmount: baseUnitAmount(2), diff --git a/packages/asset-swapper/test/market_operation_utils_test.ts b/packages/asset-swapper/test/market_operation_utils_test.ts index 9b9ef296a9..f7b5575044 100644 --- a/packages/asset-swapper/test/market_operation_utils_test.ts +++ b/packages/asset-swapper/test/market_operation_utils_test.ts @@ -53,12 +53,12 @@ describe('MarketOperationUtils tests', () => { expirationTimeSeconds: getRandomInteger(0, 2 ** 64), makerAssetData: MAKER_ASSET_DATA, takerAssetData: TAKER_ASSET_DATA, - makerFeeAssetData: assetDataUtils.encodeERC20AssetData(randomAddress()), - takerFeeAssetData: assetDataUtils.encodeERC20AssetData(randomAddress()), + makerFeeAssetData: constants.NULL_BYTES, + takerFeeAssetData: constants.NULL_BYTES, makerAssetAmount: getRandomInteger(1, 1e18), takerAssetAmount: getRandomInteger(1, 1e18), - makerFee: getRandomInteger(1, 1e17), - takerFee: getRandomInteger(1, 1e17), + makerFee: constants.ZERO_AMOUNT, + takerFee: constants.ZERO_AMOUNT, signature: hexUtils.random(), ...overrides, }; @@ -99,7 +99,7 @@ describe('MarketOperationUtils tests', () => { const singleTakerAssetAmount = takerAssetAmount.div(rates.length).integerValue(BigNumber.ROUND_UP); return rates.map(r => createOrder({ - makerAssetAmount: singleTakerAssetAmount.times(r), + makerAssetAmount: singleTakerAssetAmount.times(r).integerValue(), takerAssetAmount: singleTakerAssetAmount, }), ); @@ -110,7 +110,7 @@ describe('MarketOperationUtils tests', () => { return (rates as any).map((r: Numberish) => createOrder({ makerAssetAmount: singleMakerAssetAmount, - takerAssetAmount: singleMakerAssetAmount.div(r), + takerAssetAmount: singleMakerAssetAmount.div(r).integerValue(), }), ); } @@ -479,22 +479,22 @@ describe('MarketOperationUtils tests', () => { { ...DEFAULT_OPTS, numSamples: 4, runLimit: 512, noConflicts: false }, ); expect(improvedOrders).to.be.length(4); - const orderSources = improvedOrders.map(o => getSourceFromAssetData(o.makerAssetData)).sort(); + const orderSources = improvedOrders.map(o => getSourceFromAssetData(o.makerAssetData)); const expectedSources = [ - ERC20BridgeSource.Native, - ERC20BridgeSource.Uniswap, - ERC20BridgeSource.Eth2Dai, ERC20BridgeSource.Kyber, - ].sort(); + ERC20BridgeSource.Eth2Dai, + ERC20BridgeSource.Uniswap, + ERC20BridgeSource.Native, + ]; expect(orderSources).to.deep.eq(expectedSources); }); it('excludes Kyber when `noConflicts` enabled and Uniswap or Eth2Dai are used first', async () => { const rates: RatesBySource = {}; - rates[ERC20BridgeSource.Native] = [0.4, 0.3, 0.2, 0.1]; + rates[ERC20BridgeSource.Native] = [0.3, 0.2, 0.1, 0.05]; rates[ERC20BridgeSource.Uniswap] = [0.5, 0.05, 0.05, 0.05]; rates[ERC20BridgeSource.Eth2Dai] = [0.6, 0.05, 0.05, 0.05]; - rates[ERC20BridgeSource.Kyber] = [0.7, 0.05, 0.05, 0.05]; + rates[ERC20BridgeSource.Kyber] = [0.4, 0.05, 0.05, 0.05]; const marketOperationUtils = new MarketOperationUtils( createSamplerFromSellRates(rates), contractAddresses, @@ -506,19 +506,19 @@ describe('MarketOperationUtils tests', () => { { ...DEFAULT_OPTS, numSamples: 4, runLimit: 512, noConflicts: true }, ); expect(improvedOrders).to.be.length(4); - const orderSources = improvedOrders.map(o => getSourceFromAssetData(o.makerAssetData)).sort(); + const orderSources = improvedOrders.map(o => getSourceFromAssetData(o.makerAssetData)); const expectedSources = [ - ERC20BridgeSource.Native, - ERC20BridgeSource.Native, - ERC20BridgeSource.Uniswap, ERC20BridgeSource.Eth2Dai, - ].sort(); + ERC20BridgeSource.Uniswap, + ERC20BridgeSource.Native, + ERC20BridgeSource.Native, + ]; expect(orderSources).to.deep.eq(expectedSources); }); it('excludes Uniswap and Eth2Dai when `noConflicts` enabled and Kyber is used first', async () => { const rates: RatesBySource = {}; - rates[ERC20BridgeSource.Native] = [0.4, 0.3, 0.2, 0.1]; + rates[ERC20BridgeSource.Native] = [0.1, 0.05, 0.05, 0.05]; rates[ERC20BridgeSource.Uniswap] = [0.15, 0.05, 0.05, 0.05]; rates[ERC20BridgeSource.Eth2Dai] = [0.15, 0.05, 0.05, 0.05]; rates[ERC20BridgeSource.Kyber] = [0.7, 0.05, 0.05, 0.05]; @@ -533,13 +533,13 @@ describe('MarketOperationUtils tests', () => { { ...DEFAULT_OPTS, numSamples: 4, runLimit: 512, noConflicts: true }, ); expect(improvedOrders).to.be.length(4); - const orderSources = improvedOrders.map(o => getSourceFromAssetData(o.makerAssetData)).sort(); + const orderSources = improvedOrders.map(o => getSourceFromAssetData(o.makerAssetData)); const expectedSources = [ - ERC20BridgeSource.Native, - ERC20BridgeSource.Native, - ERC20BridgeSource.Native, ERC20BridgeSource.Kyber, - ].sort(); + ERC20BridgeSource.Native, + ERC20BridgeSource.Native, + ERC20BridgeSource.Native, + ]; expect(orderSources).to.deep.eq(expectedSources); }); }); @@ -729,13 +729,13 @@ describe('MarketOperationUtils tests', () => { { ...DEFAULT_OPTS, numSamples: 4, runLimit: 512 }, ); expect(improvedOrders).to.be.length(4); - const orderSources = improvedOrders.map(o => getSourceFromAssetData(o.makerAssetData)).sort(); + const orderSources = improvedOrders.map(o => getSourceFromAssetData(o.makerAssetData)); const expectedSources = [ - ERC20BridgeSource.Native, - ERC20BridgeSource.Native, - ERC20BridgeSource.Uniswap, ERC20BridgeSource.Eth2Dai, - ].sort(); + ERC20BridgeSource.Uniswap, + ERC20BridgeSource.Native, + ERC20BridgeSource.Native, + ]; expect(orderSources).to.deep.eq(expectedSources); }); }); diff --git a/packages/asset-swapper/test/sorting_utils_test.ts b/packages/asset-swapper/test/sorting_utils_test.ts index e11e03f35d..e794ea303d 100644 --- a/packages/asset-swapper/test/sorting_utils_test.ts +++ b/packages/asset-swapper/test/sorting_utils_test.ts @@ -10,8 +10,8 @@ import { testOrderFactory } from './utils/test_order_factory'; chaiSetup.configure(); const expect = chai.expect; -const FAKE_ERC20_TAKER_ASSET_DATA = '0xf47261b22222222222222222222222222222222222222222222222222222222222222222'; -const FAKE_ERC20_MAKER_ASSET_DATA = '0xf47261b11111111111111111111111111111111111111111111111111111111111111111'; +const FAKE_ERC20_TAKER_ASSET_DATA = '0xf47261b02222222222222222222222222222222222222222222222222222222222222222'; +const FAKE_ERC20_MAKER_ASSET_DATA = '0xf47261b01111111111111111111111111111111111111111111111111111111111111111'; describe('sortingUtils', () => { describe('#sortOrders', () => { diff --git a/packages/asset-swapper/test/swap_quote_calculator_test.ts b/packages/asset-swapper/test/swap_quote_calculator_test.ts index 2b3673d4a1..a8c760d9a0 100644 --- a/packages/asset-swapper/test/swap_quote_calculator_test.ts +++ b/packages/asset-swapper/test/swap_quote_calculator_test.ts @@ -450,20 +450,20 @@ describe('swapQuoteCalculator', () => { ); // test if orders are correct expect(swapQuote.orders).to.deep.equal([ - testOrders.SIGNED_ORDERS_WITH_FILLABLE_AMOUNTS_FEE_IN_MAKER_ASSET[0], testOrders.SIGNED_ORDERS_WITH_FILLABLE_AMOUNTS_FEE_IN_MAKER_ASSET[1], + testOrders.SIGNED_ORDERS_WITH_FILLABLE_AMOUNTS_FEE_IN_MAKER_ASSET[0], ]); expect(swapQuote.takerAssetFillAmount).to.bignumber.equal(assetSellAmount); // test if rates are correct // 50 takerAsset units to fill the first order + 100 takerAsset units for fees - expect(swapQuote.bestCaseQuoteInfo).to.deep.equal({ + expect(swapQuote.worstCaseQuoteInfo).to.deep.equal({ feeTakerAssetAmount: baseUnitAmount(2), takerAssetAmount: assetSellAmount.minus(baseUnitAmount(2)), totalTakerAssetAmount: assetSellAmount, makerAssetAmount: baseUnitAmount(0.8), protocolFeeInWeiAmount: baseUnitAmount(30, 4), }); - expect(swapQuote.worstCaseQuoteInfo).to.deep.equal({ + expect(swapQuote.bestCaseQuoteInfo).to.deep.equal({ feeTakerAssetAmount: baseUnitAmount(2), takerAssetAmount: assetSellAmount.minus(baseUnitAmount(2)), totalTakerAssetAmount: assetSellAmount, diff --git a/packages/asset-swapper/test/utils_test.ts b/packages/asset-swapper/test/utils_test.ts new file mode 100644 index 0000000000..8236c1b46a --- /dev/null +++ b/packages/asset-swapper/test/utils_test.ts @@ -0,0 +1,144 @@ +import { tokenUtils } from '@0x/dev-utils'; +import { assetDataUtils } from '@0x/order-utils'; +import { BigNumber, NULL_ADDRESS, NULL_BYTES } from '@0x/utils'; +import * as chai from 'chai'; +import 'mocha'; + +import { utils } from '../src/utils/utils'; + +import { chaiSetup } from './utils/chai_setup'; + +chaiSetup.configure(); +const expect = chai.expect; + +describe('utils', () => { + describe('isAssetDataEquivalent', () => { + describe('ERC20', () => { + const [tokenA, tokenB] = tokenUtils.getDummyERC20TokenAddresses(); + it('should succeed ERC20 to be ERC20Bridge', () => { + const isEquivalent = utils.isAssetDataEquivalent( + assetDataUtils.encodeERC20AssetData(tokenA), + assetDataUtils.encodeERC20BridgeAssetData(tokenA, NULL_ADDRESS, NULL_BYTES), + ); + expect(isEquivalent).to.be.true(); + }); + it('should succeed ERC20Bridge to be ERC20', () => { + const isEquivalent = utils.isAssetDataEquivalent( + assetDataUtils.encodeERC20BridgeAssetData(tokenA, NULL_ADDRESS, NULL_BYTES), + assetDataUtils.encodeERC20AssetData(tokenA), + ); + expect(isEquivalent).to.be.true(); + }); + it('should succeed ERC20 to be ERC20', () => { + const isEquivalent = utils.isAssetDataEquivalent( + assetDataUtils.encodeERC20AssetData(tokenA), + assetDataUtils.encodeERC20AssetData(tokenA), + ); + expect(isEquivalent).to.be.true(); + }); + it('should fail if ERC20Bridge is not the same ERC20 token', () => { + const isEquivalent = utils.isAssetDataEquivalent( + assetDataUtils.encodeERC20AssetData(tokenA), + assetDataUtils.encodeERC20BridgeAssetData(tokenB, NULL_ADDRESS, NULL_BYTES), + ); + expect(isEquivalent).to.be.false(); + }); + it('should fail if ERC20 is not the same ERC20 token', () => { + const isEquivalent = utils.isAssetDataEquivalent( + assetDataUtils.encodeERC20AssetData(tokenA), + assetDataUtils.encodeERC20AssetData(tokenB), + ); + expect(isEquivalent).to.be.false(); + }); + }); + describe('ERC721', () => { + const [tokenA, tokenB] = tokenUtils.getDummyERC20TokenAddresses(); + const tokenIdA = new BigNumber(1); + const tokenIdB = new BigNumber(2); + it('should succeed if ERC721 the same ERC721 token and id', () => { + const isEquivalent = utils.isAssetDataEquivalent( + assetDataUtils.encodeERC721AssetData(tokenA, tokenIdA), + assetDataUtils.encodeERC721AssetData(tokenA, tokenIdA), + ); + expect(isEquivalent).to.be.true(); + }); + it('should fail if ERC721 is not the same ERC721 token', () => { + const isEquivalent = utils.isAssetDataEquivalent( + assetDataUtils.encodeERC721AssetData(tokenA, tokenIdA), + assetDataUtils.encodeERC721AssetData(tokenB, tokenIdA), + ); + expect(isEquivalent).to.be.false(); + }); + it('should fail if ERC721 is not the same ERC721 id', () => { + const isEquivalent = utils.isAssetDataEquivalent( + assetDataUtils.encodeERC721AssetData(tokenA, tokenIdA), + assetDataUtils.encodeERC721AssetData(tokenA, tokenIdB), + ); + expect(isEquivalent).to.be.false(); + }); + it('should fail if ERC721 is compared with ERC20', () => { + const isEquivalent = utils.isAssetDataEquivalent( + assetDataUtils.encodeERC721AssetData(tokenA, tokenIdA), + assetDataUtils.encodeERC20AssetData(tokenA), + ); + expect(isEquivalent).to.be.false(); + }); + }); + describe('ERC1155', () => { + const [tokenA, tokenB] = tokenUtils.getDummyERC20TokenAddresses(); + const tokenIdA = new BigNumber(1); + const tokenIdB = new BigNumber(2); + const valueA = new BigNumber(1); + const valueB = new BigNumber(2); + it('should succeed if ERC1155 is the same ERC1155 token and id', () => { + const isEquivalent = utils.isAssetDataEquivalent( + assetDataUtils.encodeERC1155AssetData(tokenA, [tokenIdA], [valueA], NULL_BYTES), + assetDataUtils.encodeERC1155AssetData(tokenA, [tokenIdA], [valueA], NULL_BYTES), + ); + expect(isEquivalent).to.be.true(); + }); + it('should succeed if ERC1155 is the same ERC1155 token and ids', () => { + const isEquivalent = utils.isAssetDataEquivalent( + assetDataUtils.encodeERC1155AssetData(tokenA, [tokenIdA, tokenIdB], [valueA, valueB], NULL_BYTES), + assetDataUtils.encodeERC1155AssetData(tokenA, [tokenIdA, tokenIdB], [valueA, valueB], NULL_BYTES), + ); + expect(isEquivalent).to.be.true(); + }); + it('should succeed if ERC1155 is the same ERC1155 token and ids in different orders', () => { + const isEquivalent = utils.isAssetDataEquivalent( + assetDataUtils.encodeERC1155AssetData(tokenA, [tokenIdB, tokenIdA], [valueB, valueA], NULL_BYTES), + assetDataUtils.encodeERC1155AssetData(tokenA, [tokenIdA, tokenIdB], [valueA, valueB], NULL_BYTES), + ); + expect(isEquivalent).to.be.true(); + }); + it('should succeed if ERC1155 is the same ERC1155 token and ids with different callback data', () => { + const isEquivalent = utils.isAssetDataEquivalent( + assetDataUtils.encodeERC1155AssetData(tokenA, [tokenIdB, tokenIdA], [valueB, valueA], NULL_BYTES), + assetDataUtils.encodeERC1155AssetData(tokenA, [tokenIdA, tokenIdB], [valueA, valueB], tokenA), + ); + expect(isEquivalent).to.be.true(); + }); + it('should fail if ERC1155 contains different ids', () => { + const isEquivalent = utils.isAssetDataEquivalent( + assetDataUtils.encodeERC1155AssetData(tokenA, [tokenIdB, tokenIdA], [valueB, valueA], NULL_BYTES), + assetDataUtils.encodeERC1155AssetData(tokenB, [tokenIdA], [valueB], NULL_BYTES), + ); + expect(isEquivalent).to.be.false(); + }); + it('should fail if ERC1155 is a different ERC1155 token', () => { + const isEquivalent = utils.isAssetDataEquivalent( + assetDataUtils.encodeERC1155AssetData(tokenA, [tokenIdB, tokenIdA], [valueB, valueA], NULL_BYTES), + assetDataUtils.encodeERC1155AssetData(tokenB, [tokenIdA, tokenIdB], [valueA, valueB], NULL_BYTES), + ); + expect(isEquivalent).to.be.false(); + }); + it('should fail if expected ERC1155 has different callback data', () => { + const isEquivalent = utils.isAssetDataEquivalent( + assetDataUtils.encodeERC1155AssetData(tokenA, [tokenIdB, tokenIdA], [valueB, valueA], tokenA), + assetDataUtils.encodeERC1155AssetData(tokenA, [tokenIdA, tokenIdB], [valueA, valueB], NULL_BYTES), + ); + expect(isEquivalent).to.be.false(); + }); + }); + }); +}); diff --git a/packages/order-utils/CHANGELOG.json b/packages/order-utils/CHANGELOG.json index 929647b54e..6489845a13 100644 --- a/packages/order-utils/CHANGELOG.json +++ b/packages/order-utils/CHANGELOG.json @@ -1,4 +1,13 @@ [ + { + "version": "10.1.0", + "changes": [ + { + "note": "Export `isERC20AssetData`, `isERC20BridgeAssetData` and other equivalents.", + "pr": 2421 + } + ] + }, { "timestamp": 1576540892, "version": "10.0.1", diff --git a/packages/order-utils/src/asset_data_utils.ts b/packages/order-utils/src/asset_data_utils.ts index 485758e15f..ead6837dde 100644 --- a/packages/order-utils/src/asset_data_utils.ts +++ b/packages/order-utils/src/asset_data_utils.ts @@ -2,9 +2,14 @@ import { IAssetDataContract } from '@0x/contract-wrappers'; import { AssetData, AssetProxyId, + ERC1155AssetData, + ERC20AssetData, + ERC20BridgeAssetData, + ERC721AssetData, MultiAssetData, MultiAssetDataWithRecursiveDecoding, SingleAssetData, + StaticCallAssetData, } from '@0x/types'; import { BigNumber, hexUtils, NULL_ADDRESS } from '@0x/utils'; import * as _ from 'lodash'; @@ -162,4 +167,22 @@ export const assetDataUtils = { nestedAssetData: flattenedDecodedNestedAssetData as SingleAssetData[], }; }, + isERC20TokenAssetData(assetData: AssetData): assetData is ERC20AssetData { + return assetData.assetProxyId === AssetProxyId.ERC20; + }, + isERC20BridgeAssetData(assetData: AssetData): assetData is ERC20BridgeAssetData { + return assetData.assetProxyId === AssetProxyId.ERC20Bridge; + }, + isERC1155TokenAssetData(assetData: AssetData): assetData is ERC1155AssetData { + return assetData.assetProxyId === AssetProxyId.ERC1155; + }, + isERC721TokenAssetData(assetData: AssetData): assetData is ERC721AssetData { + return assetData.assetProxyId === AssetProxyId.ERC721; + }, + isMultiAssetData(assetData: AssetData): assetData is MultiAssetData { + return assetData.assetProxyId === AssetProxyId.MultiAsset; + }, + isStaticCallAssetData(assetData: AssetData): assetData is StaticCallAssetData { + return assetData.assetProxyId === AssetProxyId.StaticCall; + }, };