From aae93bb6a71c59fecd68e6b45200a41aa5304797 Mon Sep 17 00:00:00 2001 From: Jacob Evans Date: Tue, 21 Jul 2020 17:06:05 +1000 Subject: [PATCH] fix: asset-swapper yield to the event loop every mix path (#2637) * fix: allow a empty overrides to signal no default * fix: asset-swapper yield to the event loop every mix path * fix: optimizations skip order find if Native excluded * changelogs * chore: update protocol fee multiplier * fix: tests async --- packages/asset-swapper/CHANGELOG.json | 13 +++ packages/asset-swapper/src/constants.ts | 2 +- packages/asset-swapper/src/swap_quoter.ts | 11 ++- .../market_operation_utils/balancer_utils.ts | 6 +- .../src/utils/market_operation_utils/index.ts | 91 ++++++++++--------- .../market_operation_utils/path_optimizer.ts | 9 +- .../test/exchange_swap_quote_consumer_test.ts | 4 +- .../test/market_operation_utils_test.ts | 10 +- .../test/order_prune_utils_test.ts | 6 +- .../test/order_state_utils_test.ts | 6 +- packages/migrations/CHANGELOG.json | 9 ++ packages/migrations/src/migration.ts | 2 +- 12 files changed, 100 insertions(+), 69 deletions(-) diff --git a/packages/asset-swapper/CHANGELOG.json b/packages/asset-swapper/CHANGELOG.json index dbb2362844..c8c823b9dd 100644 --- a/packages/asset-swapper/CHANGELOG.json +++ b/packages/asset-swapper/CHANGELOG.json @@ -1,4 +1,17 @@ [ + { + "version": "4.6.1", + "changes": [ + { + "note": "Allow an empty override for sampler overrides", + "pr": 2637 + }, + { + "note": "Potentially heavy CPU functions inside the optimizer now yield to the event loop. As such they are now async.", + "pr": 2637 + } + ] + }, { "version": "4.6.0", "changes": [ diff --git a/packages/asset-swapper/src/constants.ts b/packages/asset-swapper/src/constants.ts index 568bbed730..8886c5bcaa 100644 --- a/packages/asset-swapper/src/constants.ts +++ b/packages/asset-swapper/src/constants.ts @@ -31,7 +31,7 @@ const DEFAULT_ORDER_PRUNER_OPTS: OrderPrunerOpts = { // 6 seconds polling interval const PROTOCOL_FEE_UTILS_POLLING_INTERVAL_IN_MS = 6000; -const PROTOCOL_FEE_MULTIPLIER = new BigNumber(150000); +const PROTOCOL_FEE_MULTIPLIER = new BigNumber(70000); // default 50% buffer for selecting native orders to be aggregated with other sources const MARKET_UTILS_AMOUNT_BUFFER_PERCENTAGE = 0.5; diff --git a/packages/asset-swapper/src/swap_quoter.ts b/packages/asset-swapper/src/swap_quoter.ts index f43e172cc5..fd93848447 100644 --- a/packages/asset-swapper/src/swap_quoter.ts +++ b/packages/asset-swapper/src/swap_quoter.ts @@ -26,6 +26,7 @@ import { calculateLiquidity } from './utils/calculate_liquidity'; import { MarketOperationUtils } from './utils/market_operation_utils'; import { createDummyOrderForSampler } from './utils/market_operation_utils/orders'; import { DexOrderSampler } from './utils/market_operation_utils/sampler'; +import { ERC20BridgeSource } from './utils/market_operation_utils/types'; import { orderPrunerUtils } from './utils/order_prune_utils'; import { OrderStateUtils } from './utils/order_state_utils'; import { ProtocolFeeUtils } from './utils/protocol_fee_utils'; @@ -193,8 +194,7 @@ export class SwapQuoter { [this._contractAddresses.erc20BridgeSampler]: { code: samplerBytecode }, } : {}; - const samplerOverrides = _.merge( - {}, + const samplerOverrides = _.assign( { block: BlockParamLiteral.Latest, overrides: defaultCodeOverrides }, options.samplerOverrides, ); @@ -563,7 +563,12 @@ export class SwapQuoter { } // get batches of orders from different sources, awaiting sources in parallel const orderBatchPromises: Array> = []; - orderBatchPromises.push(this._getSignedOrdersAsync(makerAssetData, takerAssetData)); // order book + orderBatchPromises.push( + // Don't fetch from the DB if Native has been excluded + opts.excludedSources.includes(ERC20BridgeSource.Native) + ? Promise.resolve([]) + : this._getSignedOrdersAsync(makerAssetData, takerAssetData), + ); if ( opts.rfqt && opts.rfqt.intentOnFilling && diff --git a/packages/asset-swapper/src/utils/market_operation_utils/balancer_utils.ts b/packages/asset-swapper/src/utils/market_operation_utils/balancer_utils.ts index e412172a53..a5e091c3a7 100644 --- a/packages/asset-swapper/src/utils/market_operation_utils/balancer_utils.ts +++ b/packages/asset-swapper/src/utils/market_operation_utils/balancer_utils.ts @@ -1,7 +1,6 @@ import { BigNumber } from '@0x/utils'; import { bmath, getPoolsWithTokens, parsePoolData } from '@balancer-labs/sor'; import { Decimal } from 'decimal.js'; -import * as _ from 'lodash'; export interface BalancerPool { id: string; @@ -23,7 +22,7 @@ interface CacheValue { // tslint:disable:custom-no-magic-numbers const FIVE_SECONDS_MS = 5 * 1000; const DEFAULT_TIMEOUT_MS = 1000; -const MAX_POOLS_FETCHED = 3; +const MAX_POOLS_FETCHED = 2; const Decimal20 = Decimal.clone({ precision: 20 }); // tslint:enable:custom-no-magic-numbers @@ -31,6 +30,7 @@ export class BalancerPoolsCache { constructor( private readonly _cache: { [key: string]: CacheValue } = {}, public cacheExpiryMs: number = FIVE_SECONDS_MS, + private readonly maxPoolsFetched: number = MAX_POOLS_FETCHED, ) {} public async getPoolsForPairAsync( @@ -65,7 +65,7 @@ export class BalancerPoolsCache { const pools = parsePoolData(poolData, takerToken, makerToken).sort((a, b) => b.balanceOut.minus(a.balanceOut).toNumber(), ); - return pools.length > MAX_POOLS_FETCHED ? pools.slice(0, MAX_POOLS_FETCHED) : pools; + return pools.length > this.maxPoolsFetched ? pools.slice(0, this.maxPoolsFetched) : pools; } catch (err) { return []; } 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 ad4f27c3c7..4b67ccaa73 100644 --- a/packages/asset-swapper/src/utils/market_operation_utils/index.ts +++ b/packages/asset-swapper/src/utils/market_operation_utils/index.ts @@ -14,7 +14,7 @@ import { createSignedOrdersWithFillableAmounts, getNativeOrderTokens, } from './orders'; -import { findOptimalPath } from './path_optimizer'; +import { findOptimalPathAsync } from './path_optimizer'; import { DexOrderSampler, getSampleAmounts } from './sampler'; import { AggregationError, @@ -125,8 +125,8 @@ export class MarketOperationUtils { _opts, ); - const balancerPromise = this._sampler.executeAsync( - await DexOrderSampler.ops.getSellQuotesAsync( + const balancerPromise = DexOrderSampler.ops + .getSellQuotesAsync( difference([ERC20BridgeSource.Balancer], _opts.excludedSources), makerToken, takerToken, @@ -135,15 +135,15 @@ export class MarketOperationUtils { this._sampler.balancerPoolsCache, this._liquidityProviderRegistry, this._multiBridge, - ), - ); + ) + .then(async r => this._sampler.executeAsync(r)); const [ [orderFillableAmounts, liquidityProviderAddress, ethToMakerAssetRate, dexQuotes], rfqtIndicativeQuotes, [balancerQuotes], ] = await Promise.all([samplerPromise, rfqtPromise, balancerPromise]); - return this._generateOptimizedOrders({ + return this._generateOptimizedOrdersAsync({ orderFillableAmounts, nativeOrders, dexQuotes: dexQuotes.concat(balancerQuotes), @@ -247,7 +247,7 @@ export class MarketOperationUtils { [balancerQuotes], ] = await Promise.all([samplerPromise, rfqtPromise, balancerPromise]); - return this._generateOptimizedOrders({ + return this._generateOptimizedOrdersAsync({ orderFillableAmounts, nativeOrders, dexQuotes: dexQuotes.concat(balancerQuotes), @@ -325,42 +325,44 @@ export class MarketOperationUtils { const batchEthToTakerAssetRate = executeResults.splice(0, batchNativeOrders.length) as BigNumber[]; const batchDexQuotes = executeResults.splice(0, batchNativeOrders.length) as DexSample[][][]; - return batchNativeOrders.map((nativeOrders, i) => { - if (nativeOrders.length === 0) { - throw new Error(AggregationError.EmptyOrders); - } - const [makerToken, takerToken] = getNativeOrderTokens(nativeOrders[0]); - const orderFillableAmounts = batchOrderFillableAmounts[i]; - const ethToTakerAssetRate = batchEthToTakerAssetRate[i]; - const dexQuotes = batchDexQuotes[i]; - const makerAmount = makerAmounts[i]; - try { - return this._generateOptimizedOrders({ - orderFillableAmounts, - nativeOrders, - dexQuotes, - rfqtIndicativeQuotes: [], - inputToken: makerToken, - outputToken: takerToken, - side: MarketOperation.Buy, - inputAmount: makerAmount, - ethToOutputRate: ethToTakerAssetRate, - bridgeSlippage: _opts.bridgeSlippage, - maxFallbackSlippage: _opts.maxFallbackSlippage, - excludedSources: _opts.excludedSources, - feeSchedule: _opts.feeSchedule, - allowFallback: _opts.allowFallback, - shouldBatchBridgeOrders: _opts.shouldBatchBridgeOrders, - }); - } catch (e) { - // It's possible for one of the pairs to have no path - // rather than throw NO_OPTIMAL_PATH we return undefined - return undefined; - } - }); + return Promise.all( + batchNativeOrders.map(async (nativeOrders, i) => { + if (nativeOrders.length === 0) { + throw new Error(AggregationError.EmptyOrders); + } + const [makerToken, takerToken] = getNativeOrderTokens(nativeOrders[0]); + const orderFillableAmounts = batchOrderFillableAmounts[i]; + const ethToTakerAssetRate = batchEthToTakerAssetRate[i]; + const dexQuotes = batchDexQuotes[i]; + const makerAmount = makerAmounts[i]; + try { + return await this._generateOptimizedOrdersAsync({ + orderFillableAmounts, + nativeOrders, + dexQuotes, + rfqtIndicativeQuotes: [], + inputToken: makerToken, + outputToken: takerToken, + side: MarketOperation.Buy, + inputAmount: makerAmount, + ethToOutputRate: ethToTakerAssetRate, + bridgeSlippage: _opts.bridgeSlippage, + maxFallbackSlippage: _opts.maxFallbackSlippage, + excludedSources: _opts.excludedSources, + feeSchedule: _opts.feeSchedule, + allowFallback: _opts.allowFallback, + shouldBatchBridgeOrders: _opts.shouldBatchBridgeOrders, + }); + } catch (e) { + // It's possible for one of the pairs to have no path + // rather than throw NO_OPTIMAL_PATH we return undefined + return undefined; + } + }), + ); } - private _generateOptimizedOrders(opts: { + private async _generateOptimizedOrdersAsync(opts: { side: MarketOperation; inputToken: string; outputToken: string; @@ -379,7 +381,7 @@ export class MarketOperationUtils { shouldBatchBridgeOrders?: boolean; liquidityProviderAddress?: string; multiBridgeAddress?: string; - }): OptimizedMarketOrder[] { + }): Promise { const { inputToken, outputToken, side, inputAmount } = opts; const maxFallbackSlippage = opts.maxFallbackSlippage || 0; // Convert native orders and dex quotes into fill paths. @@ -397,7 +399,7 @@ export class MarketOperationUtils { feeSchedule: opts.feeSchedule, }); // Find the optimal path. - let optimalPath = findOptimalPath(side, paths, inputAmount, opts.runLimit) || []; + let optimalPath = (await findOptimalPathAsync(side, paths, inputAmount, opts.runLimit)) || []; if (optimalPath.length === 0) { throw new Error(AggregationError.NoOptimalPath); } @@ -407,7 +409,8 @@ export class MarketOperationUtils { // We create a fallback path that is exclusive of Native liquidity // This is the optimal on-chain path for the entire input amount const nonNativePaths = paths.filter(p => p.length > 0 && p[0].source !== ERC20BridgeSource.Native); - const nonNativeOptimalPath = findOptimalPath(side, nonNativePaths, inputAmount, opts.runLimit) || []; + const nonNativeOptimalPath = + (await findOptimalPathAsync(side, nonNativePaths, inputAmount, opts.runLimit)) || []; // Calculate the slippage of on-chain sources compared to the most optimal path const fallbackSlippage = getPathAdjustedSlippage( side, diff --git a/packages/asset-swapper/src/utils/market_operation_utils/path_optimizer.ts b/packages/asset-swapper/src/utils/market_operation_utils/path_optimizer.ts index 3d5a20dca0..f4870e5d2f 100644 --- a/packages/asset-swapper/src/utils/market_operation_utils/path_optimizer.ts +++ b/packages/asset-swapper/src/utils/market_operation_utils/path_optimizer.ts @@ -9,17 +9,21 @@ import { Fill } from './types'; // tslint:disable: prefer-for-of custom-no-magic-numbers completed-docs const RUN_LIMIT_DECAY_FACTOR = 0.8; +// Used to yield the event loop when performing CPU intensive tasks +// tislint:disable-next-line:no-inferred-empty-object-type +const setImmediateAsync = async (delay: number = 0) => + new Promise(resolve => setImmediate(() => resolve(), delay)); /** * Find the optimal mixture of paths that maximizes (for sells) or minimizes * (for buys) output, while meeting the input requirement. */ -export function findOptimalPath( +export async function findOptimalPathAsync( side: MarketOperation, paths: Fill[][], targetInput: BigNumber, runLimit: number = 2 ** 15, -): Fill[] | undefined { +): Promise { // Sort paths in descending order by adjusted output amount. const sortedPaths = paths .slice(0) @@ -27,6 +31,7 @@ export function findOptimalPath( let optimalPath = sortedPaths[0] || []; for (const [i, path] of sortedPaths.slice(1).entries()) { optimalPath = mixPaths(side, optimalPath, path, targetInput, runLimit * RUN_LIMIT_DECAY_FACTOR ** i); + await setImmediateAsync(); } return isPathComplete(optimalPath, targetInput) ? optimalPath : undefined; } diff --git a/packages/asset-swapper/test/exchange_swap_quote_consumer_test.ts b/packages/asset-swapper/test/exchange_swap_quote_consumer_test.ts index 06254a924b..fd1ca162a7 100644 --- a/packages/asset-swapper/test/exchange_swap_quote_consumer_test.ts +++ b/packages/asset-swapper/test/exchange_swap_quote_consumer_test.ts @@ -71,7 +71,6 @@ describe('ExchangeSwapQuoteConsumer', () => { let takerTokenAddress: string; let makerAssetData: string; let takerAssetData: string; - let wethAssetData: string; let contractAddresses: ContractAddresses; let exchangeContract: ExchangeContract; @@ -96,10 +95,9 @@ describe('ExchangeSwapQuoteConsumer', () => { userAddresses = await web3Wrapper.getAvailableAddressesAsync(); [coinbaseAddress, takerAddress, makerAddress, feeRecipient] = userAddresses; [makerTokenAddress, takerTokenAddress] = tokenUtils.getDummyERC20TokenAddresses(); - [makerAssetData, takerAssetData, wethAssetData] = [ + [makerAssetData, takerAssetData] = [ assetDataUtils.encodeERC20AssetData(makerTokenAddress), assetDataUtils.encodeERC20AssetData(takerTokenAddress), - assetDataUtils.encodeERC20AssetData(contractAddresses.etherToken), ]; erc20MakerTokenContract = new ERC20TokenContract(makerTokenAddress, provider); erc20TakerTokenContract = new ERC20TokenContract(takerTokenAddress, provider); diff --git a/packages/asset-swapper/test/market_operation_utils_test.ts b/packages/asset-swapper/test/market_operation_utils_test.ts index 506df0e6e5..52f291fae1 100644 --- a/packages/asset-swapper/test/market_operation_utils_test.ts +++ b/packages/asset-swapper/test/market_operation_utils_test.ts @@ -21,7 +21,7 @@ import { createFillPaths } from '../src/utils/market_operation_utils/fills'; import { DexOrderSampler } from '../src/utils/market_operation_utils/sampler'; import { DexSample, ERC20BridgeSource, FillData, NativeFillData } from '../src/utils/market_operation_utils/types'; -// tslint:disable: custom-no-magic-numbers +// tslint:disable: custom-no-magic-numbers promise-function-async describe('MarketOperationUtils tests', () => { const CHAIN_ID = 1; const contractAddresses = { ...getContractAddressesForChainOrThrow(CHAIN_ID), multiBridge: NULL_ADDRESS }; @@ -153,7 +153,7 @@ describe('MarketOperationUtils tests', () => { fillAmounts: BigNumber[], wethAddress: string, liquidityProviderAddress?: string, - ) => DexSample[][]; + ) => Promise; function createGetMultipleSellQuotesOperationFromRates(rates: RatesBySource): GetMultipleQuotesOperation { return ( @@ -163,7 +163,7 @@ describe('MarketOperationUtils tests', () => { fillAmounts: BigNumber[], _wethAddress: string, ) => { - return sources.map(s => createSamplesFromRates(s, fillAmounts, rates[s])); + return Promise.resolve(sources.map(s => createSamplesFromRates(s, fillAmounts, rates[s]))); }; } @@ -206,7 +206,9 @@ describe('MarketOperationUtils tests', () => { fillAmounts: BigNumber[], _wethAddress: string, ) => { - return sources.map(s => createSamplesFromRates(s, fillAmounts, rates[s].map(r => new BigNumber(1).div(r)))); + return Promise.resolve( + sources.map(s => createSamplesFromRates(s, fillAmounts, rates[s].map(r => new BigNumber(1).div(r)))), + ); }; } diff --git a/packages/asset-swapper/test/order_prune_utils_test.ts b/packages/asset-swapper/test/order_prune_utils_test.ts index 42ec1a7c7d..65829c5fc3 100644 --- a/packages/asset-swapper/test/order_prune_utils_test.ts +++ b/packages/asset-swapper/test/order_prune_utils_test.ts @@ -23,7 +23,7 @@ const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); const ONE_ETH_IN_WEI = new BigNumber(1000000000000000000); const TESTRPC_CHAIN_ID = devConstants.TESTRPC_CHAIN_ID; const GAS_PRICE = new BigNumber(devConstants.DEFAULT_GAS_PRICE); -const PROTOCOL_FEE_MULTIPLIER = 150000; +const PROTOCOL_FEE_MULTIPLIER = 70000; const PROTOCOL_FEE_PER_FILL = GAS_PRICE.times(PROTOCOL_FEE_MULTIPLIER); const UNLIMITED_ALLOWANCE_IN_BASE_UNITS = new BigNumber(2).pow(256).minus(1); // tslint:disable-line:custom-no-magic-numbers const EXPIRY_BUFFER_MS = 120000; @@ -44,7 +44,6 @@ describe('orderPrunerUtils', () => { let makerAssetData: string; let takerAssetData: string; let orderFactory: OrderFactory; - let wethAssetData: string; let contractAddresses: ContractAddresses; let nonOpenSignedOrder: SignedOrder; @@ -68,10 +67,9 @@ describe('orderPrunerUtils', () => { erc20TakerTokenContract = new ERC20TokenContract(takerTokenAddress, provider); exchangeContract = new ExchangeContract(contractAddresses.exchange, provider); - [makerAssetData, takerAssetData, wethAssetData] = [ + [makerAssetData, takerAssetData] = [ assetDataUtils.encodeERC20AssetData(makerTokenAddress), assetDataUtils.encodeERC20AssetData(takerTokenAddress), - assetDataUtils.encodeERC20AssetData(contractAddresses.etherToken), ]; // Configure order defaults diff --git a/packages/asset-swapper/test/order_state_utils_test.ts b/packages/asset-swapper/test/order_state_utils_test.ts index 77c00bc44d..c68eb4041b 100644 --- a/packages/asset-swapper/test/order_state_utils_test.ts +++ b/packages/asset-swapper/test/order_state_utils_test.ts @@ -23,7 +23,7 @@ const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); const ONE_ETH_IN_WEI = new BigNumber(1000000000000000000); const TESTRPC_CHAIN_ID = devConstants.TESTRPC_CHAIN_ID; const GAS_PRICE = new BigNumber(devConstants.DEFAULT_GAS_PRICE); -const PROTOCOL_FEE_MULTIPLIER = 150000; +const PROTOCOL_FEE_MULTIPLIER = 70000; const PROTOCOL_FEE_PER_FILL = GAS_PRICE.times(PROTOCOL_FEE_MULTIPLIER); const UNLIMITED_ALLOWANCE_IN_BASE_UNITS = new BigNumber(2).pow(256).minus(1); // tslint:disable-line:custom-no-magic-numbers @@ -51,7 +51,6 @@ describe('OrderStateUtils', () => { let makerAssetData: string; let takerAssetData: string; let orderFactory: OrderFactory; - let wethAssetData: string; let contractAddresses: ContractAddresses; let orderStateUtils: OrderStateUtils; @@ -78,10 +77,9 @@ describe('OrderStateUtils', () => { erc20TakerTokenContract = new ERC20TokenContract(takerTokenAddress, provider); exchangeContract = new ExchangeContract(contractAddresses.exchange, provider); - [makerAssetData, takerAssetData, wethAssetData] = [ + [makerAssetData, takerAssetData] = [ assetDataUtils.encodeERC20AssetData(makerTokenAddress), assetDataUtils.encodeERC20AssetData(takerTokenAddress), - assetDataUtils.encodeERC20AssetData(contractAddresses.etherToken), ]; // Configure order defaults diff --git a/packages/migrations/CHANGELOG.json b/packages/migrations/CHANGELOG.json index e2d4fecb0b..02e7c3a6e0 100644 --- a/packages/migrations/CHANGELOG.json +++ b/packages/migrations/CHANGELOG.json @@ -1,4 +1,13 @@ [ + { + "version": "6.4.1", + "changes": [ + { + "note": "Change test protocol fee to 70000.", + "pr": 2637 + } + ] + }, { "version": "6.4.0", "changes": [ diff --git a/packages/migrations/src/migration.ts b/packages/migrations/src/migration.ts index 51eb641be0..0646511e52 100644 --- a/packages/migrations/src/migration.ts +++ b/packages/migrations/src/migration.ts @@ -274,7 +274,7 @@ export async function runMigrationsAsync( await stakingProxy.addAuthorizedAddress(txDefaults.from).awaitTransactionSuccessAsync(txDefaults); await stakingDel.addExchangeAddress(exchange.address).awaitTransactionSuccessAsync(txDefaults); await exchange.setProtocolFeeCollectorAddress(stakingProxy.address).awaitTransactionSuccessAsync(txDefaults); - await exchange.setProtocolFeeMultiplier(new BigNumber(150000)).awaitTransactionSuccessAsync(txDefaults); + await exchange.setProtocolFeeMultiplier(new BigNumber(70000)).awaitTransactionSuccessAsync(txDefaults); await zrxVault.addAuthorizedAddress(txDefaults.from).awaitTransactionSuccessAsync(txDefaults); await zrxVault.setStakingProxy(stakingProxy.address).awaitTransactionSuccessAsync(txDefaults);