diff --git a/packages/asset-swapper/CHANGELOG.json b/packages/asset-swapper/CHANGELOG.json index 72ebb1b118..0a03df5f06 100644 --- a/packages/asset-swapper/CHANGELOG.json +++ b/packages/asset-swapper/CHANGELOG.json @@ -33,6 +33,10 @@ { "note": "Add support for buy token affiliate fees", "pr": 2658 + }, + { + "note": "Fix optimization of buy paths", + "pr": 2655 } ] }, diff --git a/packages/asset-swapper/src/utils/market_operation_utils/fills.ts b/packages/asset-swapper/src/utils/market_operation_utils/fills.ts index 05e9da0d72..ccc015b33e 100644 --- a/packages/asset-swapper/src/utils/market_operation_utils/fills.ts +++ b/packages/asset-swapper/src/utils/market_operation_utils/fills.ts @@ -58,7 +58,7 @@ function nativeOrdersToPath( ): Fill[] { const sourcePathId = hexUtils.random(); // Create a single path from all orders. - let path: Fill[] = []; + let path: Array = []; for (const order of orders) { const makerAmount = fillableAmountsUtils.getMakerAssetAmountSwappedAfterOrderFees(order); const takerAmount = fillableAmountsUtils.getTakerAssetAmountSwappedAfterOrderFees(order); @@ -67,7 +67,6 @@ function nativeOrdersToPath( const penalty = ethToOutputRate.times( fees[ERC20BridgeSource.Native] === undefined ? 0 : fees[ERC20BridgeSource.Native]!(), ); - const rate = makerAmount.div(takerAmount); // targetInput can be less than the order size // whilst the penalty is constant, it affects the adjusted output // only up until the target has been exhausted. @@ -86,11 +85,10 @@ function nativeOrdersToPath( } path.push({ sourcePathId, - input: clippedInput, - output: clippedOutput, - rate, adjustedRate, adjustedOutput, + input: clippedInput, + output: clippedOutput, flags: 0, index: 0, // TBD parent: undefined, // TBD @@ -135,15 +133,11 @@ function dexQuotesToPaths( ? ethToOutputRate.times(fee) : ZERO_AMOUNT; const adjustedOutput = side === MarketOperation.Sell ? output.minus(penalty) : output.plus(penalty); - const rate = side === MarketOperation.Sell ? output.div(input) : input.div(output); - const adjustedRate = side === MarketOperation.Sell ? adjustedOutput.div(input) : input.div(adjustedOutput); path.push({ sourcePathId, input, output, - rate, - adjustedRate, adjustedOutput, source, fillData, @@ -193,8 +187,12 @@ export function getPathAdjustedSize(path: Fill[], targetInput: BigNumber = POSIT for (const fill of path) { if (input.plus(fill.input).gte(targetInput)) { const di = targetInput.minus(input); - input = input.plus(di); - output = output.plus(fill.adjustedOutput.times(di.div(fill.input))); + if (di.gt(0)) { + input = input.plus(di); + // Penalty does not get interpolated. + const penalty = fill.adjustedOutput.minus(fill.output); + output = output.plus(fill.output.times(di.div(fill.input)).plus(penalty)); + } break; } else { input = input.plus(fill.input); @@ -223,6 +221,10 @@ export function isValidPath(path: Fill[], skipDuplicateCheck: boolean = false): } flags |= path[i].flags; } + return arePathFlagsAllowed(flags); +} + +export function arePathFlagsAllowed(flags: number): boolean { const multiBridgeConflict = FillFlags.MultiBridge | FillFlags.ConflictsWithMultiBridge; return (flags & multiBridgeConflict) !== multiBridgeConflict; } @@ -266,12 +268,14 @@ export function collapsePath(path: Fill[]): CollapsedFill[] { return collapsed; } +export function getPathAdjustedCompleteRate(side: MarketOperation, path: Fill[], targetInput: BigNumber): BigNumber { + const [input, output] = getPathAdjustedSize(path, targetInput); + return getCompleteRate(side, input, output, targetInput); +} + export function getPathAdjustedRate(side: MarketOperation, path: Fill[], targetInput: BigNumber): BigNumber { - const [, output] = getPathAdjustedSize(path, targetInput); - if (output.eq(0)) { - return ZERO_AMOUNT; - } - return side === MarketOperation.Sell ? output.div(targetInput) : targetInput.div(output); + const [input, output] = getPathAdjustedSize(path, targetInput); + return getRate(side, input, output); } export function getPathAdjustedSlippage( @@ -287,3 +291,29 @@ export function getPathAdjustedSlippage( const rateChange = maxRate.minus(totalRate); return rateChange.div(maxRate).toNumber(); } + +export function getCompleteRate( + side: MarketOperation, + input: BigNumber, + output: BigNumber, + targetInput: BigNumber, +): BigNumber { + if (input.eq(0) || output.eq(0) || targetInput.eq(0)) { + return ZERO_AMOUNT; + } + // Penalize paths that fall short of the entire input amount by a factor of + // input / targetInput => (i / t) + if (side === MarketOperation.Sell) { + // (o / i) * (i / t) => (o / t) + return output.div(targetInput); + } + // (i / o) * (i / t) + return input.div(output).times(input.div(targetInput)); +} + +export function getRate(side: MarketOperation, input: BigNumber, output: BigNumber): BigNumber { + if (input.eq(0) || output.eq(0)) { + return ZERO_AMOUNT; + } + return side === MarketOperation.Sell ? output.div(input) : input.div(output); +} 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 dd5627fa10..f649c59e07 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 @@ -3,10 +3,18 @@ import { BigNumber } from '@0x/utils'; import { MarketOperation } from '../../types'; import { ZERO_AMOUNT } from './constants'; -import { getPathAdjustedRate, getPathSize, isValidPath } from './fills'; +import { + arePathFlagsAllowed, + getCompleteRate, + getPathAdjustedCompleteRate, + getPathAdjustedRate, + getPathAdjustedSize, + getPathSize, + isValidPath, +} from './fills'; import { Fill } from './types'; -// tslint:disable: prefer-for-of custom-no-magic-numbers completed-docs +// tslint:disable: prefer-for-of custom-no-magic-numbers completed-docs no-bitwise const RUN_LIMIT_DECAY_FACTOR = 0.5; @@ -20,11 +28,13 @@ export async function findOptimalPathAsync( targetInput: BigNumber, runLimit: number = 2 ** 8, ): Promise { - // Sort paths by descending adjusted rate. + // Sort paths by descending adjusted completed rate. const sortedPaths = paths .slice(0) .sort((a, b) => - getPathAdjustedRate(side, b, targetInput).comparedTo(getPathAdjustedRate(side, a, targetInput)), + getPathAdjustedCompleteRate(side, b, targetInput).comparedTo( + getPathAdjustedCompleteRate(side, a, targetInput), + ), ); let optimalPath = sortedPaths[0] || []; for (const [i, path] of sortedPaths.slice(1).entries()) { @@ -42,11 +52,12 @@ function mixPaths( targetInput: BigNumber, maxSteps: number, ): Fill[] { - const _maxSteps = Math.max(maxSteps, 16); - let bestPath: Fill[] = pathA; - let bestPathInput = getPathSize(pathA, targetInput)[0]; - let bestPathRate = getPathAdjustedRate(side, pathA, targetInput); + const _maxSteps = Math.max(maxSteps, 32); let steps = 0; + // We assume pathA is the better of the two initially. + let bestPath: Fill[] = pathA; + let [bestPathInput, bestPathOutput] = getPathAdjustedSize(pathA, targetInput); + let bestPathRate = getCompleteRate(side, bestPathInput, bestPathOutput, targetInput); const _isBetterPath = (input: BigNumber, rate: BigNumber) => { if (bestPathInput.lt(targetInput)) { return input.gt(bestPathInput); @@ -55,64 +66,77 @@ function mixPaths( } return false; }; - const _walk = (path: Fill[], input: BigNumber, output: BigNumber, allFills: Fill[]) => { + const _walk = (path: Fill[], input: BigNumber, output: BigNumber, flags: number, remainingFills: Fill[]) => { steps += 1; - const rate = getRate(side, targetInput, output); + const rate = getCompleteRate(side, input, output, targetInput); if (_isBetterPath(input, rate)) { bestPath = path; bestPathInput = input; + bestPathOutput = output; bestPathRate = rate; } const remainingInput = targetInput.minus(input); if (remainingInput.gt(0)) { - for (let i = 0; i < allFills.length && steps < _maxSteps; ++i) { - const fill = allFills[i]; - const nextPath = [...path, fill]; + for (let i = 0; i < remainingFills.length && steps < _maxSteps; ++i) { + const fill = remainingFills[i]; // Only walk valid paths. - if (!isValidPath(nextPath, true)) { + if (!isValidNextPathFill(path, flags, fill)) { continue; } // Remove this fill from the next list of candidate fills. - const nextAllFills = allFills.slice(); - nextAllFills.splice(i, 1); + const nextRemainingFills = remainingFills.slice(); + nextRemainingFills.splice(i, 1); // Recurse. _walk( - nextPath, + [...path, fill], input.plus(BigNumber.min(remainingInput, fill.input)), output.plus( // Clip the output of the next fill to the remaining // input. clipFillAdjustedOutput(fill, remainingInput), ), - nextAllFills, + flags | fill.flags, + nextRemainingFills, ); } } }; - const allPaths = [...pathA, ...pathB]; - const sources = allPaths.filter(f => f.index === 0).map(f => f.sourcePathId); + const allFills = [...pathA, ...pathB]; + const sources = allFills.filter(f => f.index === 0).map(f => f.sourcePathId); const rateBySource = Object.assign( {}, ...sources.map(s => ({ - [s]: getPathAdjustedRate(side, allPaths.filter(f => f.sourcePathId === s), targetInput), + [s]: getPathAdjustedRate(side, allFills.filter(f => f.sourcePathId === s), targetInput), })), ); - _walk( - [], - ZERO_AMOUNT, - ZERO_AMOUNT, - // Sort subpaths by rate and keep fills contiguous to improve our - // chances of walking ideal, valid paths first. - allPaths.sort((a, b) => { - if (a.sourcePathId !== b.sourcePathId) { - return rateBySource[b.sourcePathId].comparedTo(rateBySource[a.sourcePathId]); - } - return a.index - b.index; - }), - ); + // Sort subpaths by rate and keep fills contiguous to improve our + // chances of walking ideal, valid paths first. + const sortedFills = allFills.sort((a, b) => { + if (a.sourcePathId !== b.sourcePathId) { + return rateBySource[b.sourcePathId].comparedTo(rateBySource[a.sourcePathId]); + } + return a.index - b.index; + }); + _walk([], ZERO_AMOUNT, ZERO_AMOUNT, 0, sortedFills); + if (!isValidPath(bestPath)) { + throw new Error('nooope'); + } return bestPath; } +function isValidNextPathFill(path: Fill[], pathFlags: number, fill: Fill): boolean { + if (path.length === 0) { + return !fill.parent; + } + if (path[path.length - 1] === fill.parent) { + return true; + } + if (fill.parent) { + return false; + } + return arePathFlagsAllowed(pathFlags | fill.flags); +} + function isPathComplete(path: Fill[], targetInput: BigNumber): boolean { const [input] = getPathSize(path); return input.gte(targetInput); @@ -122,16 +146,7 @@ function clipFillAdjustedOutput(fill: Fill, remainingInput: BigNumber): BigNumbe if (fill.input.lte(remainingInput)) { return fill.adjustedOutput; } + // Penalty does not get interpolated. const penalty = fill.adjustedOutput.minus(fill.output); - return remainingInput.times(fill.rate).plus(penalty); -} - -function getRate(side: MarketOperation, input: BigNumber, output: BigNumber): BigNumber { - if (input.eq(0) || output.eq(0)) { - return ZERO_AMOUNT; - } - if (side === MarketOperation.Sell) { - return output.div(input); - } - return input.div(output); + return remainingInput.times(fill.output.div(fill.input)).plus(penalty); } diff --git a/packages/asset-swapper/src/utils/market_operation_utils/types.ts b/packages/asset-swapper/src/utils/market_operation_utils/types.ts index f36f27f0ec..3772daf538 100644 --- a/packages/asset-swapper/src/utils/market_operation_utils/types.ts +++ b/packages/asset-swapper/src/utils/market_operation_utils/types.ts @@ -130,10 +130,6 @@ export interface Fill { input: BigNumber; // Output fill amount (maker asset amount in a sell, taker asset amount in a buy). output: BigNumber; - // The maker/taker rate. - rate: BigNumber; - // The maker/taker rate, adjusted by fees. - adjustedRate: BigNumber; // The output fill amount, ajdusted by fees. adjustedOutput: BigNumber; // Fill that must precede this one. This enforces certain fills to be contiguous. diff --git a/packages/asset-swapper/test/market_operation_utils_test.ts b/packages/asset-swapper/test/market_operation_utils_test.ts index 42e54e214d..bbf8d39729 100644 --- a/packages/asset-swapper/test/market_operation_utils_test.ts +++ b/packages/asset-swapper/test/market_operation_utils_test.ts @@ -813,9 +813,9 @@ describe('MarketOperationUtils tests', () => { expect(improvedOrders).to.be.length(3); const orderFillSources = improvedOrders.map(o => o.fills.map(f => f.source)); expect(orderFillSources).to.deep.eq([ - [ERC20BridgeSource.Uniswap, ERC20BridgeSource.Curve], + [ERC20BridgeSource.Uniswap], [ERC20BridgeSource.Native], - [ERC20BridgeSource.Eth2Dai], + [ERC20BridgeSource.Eth2Dai, ERC20BridgeSource.Curve], ]); }); }); @@ -1114,8 +1114,8 @@ describe('MarketOperationUtils tests', () => { it('batches contiguous bridge sources', async () => { const rates: RatesBySource = {}; rates[ERC20BridgeSource.Native] = [0.5, 0.01, 0.01, 0.01]; - rates[ERC20BridgeSource.Eth2Dai] = [0.49, 0.01, 0.01, 0.01]; - rates[ERC20BridgeSource.Uniswap] = [0.48, 0.47, 0.01, 0.01]; + rates[ERC20BridgeSource.Eth2Dai] = [0.49, 0.02, 0.01, 0.01]; + rates[ERC20BridgeSource.Uniswap] = [0.48, 0.01, 0.01, 0.01]; replaceSamplerOps({ getBuyQuotesAsync: createGetMultipleBuyQuotesOperationFromRates(rates), }); @@ -1133,7 +1133,7 @@ describe('MarketOperationUtils tests', () => { const orderFillSources = improvedOrders.map(o => o.fills.map(f => f.source)); expect(orderFillSources).to.deep.eq([ [ERC20BridgeSource.Native], - [ERC20BridgeSource.Uniswap, ERC20BridgeSource.Eth2Dai], + [ERC20BridgeSource.Eth2Dai, ERC20BridgeSource.Uniswap], ]); }); });