From 0fdd31892a12f098c0b8997fa3347f75c8cbf546 Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Fri, 20 Mar 2020 15:48:46 -0400 Subject: [PATCH] `@0x/asset-swapper`: Fix quote optimizer bug not properly accounting for fees. --- packages/asset-swapper/CHANGELOG.json | 4 ++ packages/asset-swapper/src/constants.ts | 4 +- .../src/utils/market_operation_utils/fills.ts | 12 +++--- .../market_operation_utils/path_optimizer.ts | 40 ++++++++++++++----- 4 files changed, 44 insertions(+), 16 deletions(-) diff --git a/packages/asset-swapper/CHANGELOG.json b/packages/asset-swapper/CHANGELOG.json index cf8e9b6274..e9cd9afe2e 100644 --- a/packages/asset-swapper/CHANGELOG.json +++ b/packages/asset-swapper/CHANGELOG.json @@ -29,6 +29,10 @@ { "note": "Fix fee schedule not being scaled by gas price.", "pr": 2522 + }, + { + "note": "Fix quote optimizer bug not properly accounting for fees.", + "pr": 2526 } ] }, diff --git a/packages/asset-swapper/src/constants.ts b/packages/asset-swapper/src/constants.ts index 7a4c65f9e5..f9b94b4ffb 100644 --- a/packages/asset-swapper/src/constants.ts +++ b/packages/asset-swapper/src/constants.ts @@ -29,8 +29,8 @@ const DEFAULT_ORDER_PRUNER_OPTS: OrderPrunerOpts = { ]), // Default asset-swapper for CFL oriented fee types }; -// 15 seconds polling interval -const PROTOCOL_FEE_UTILS_POLLING_INTERVAL_IN_MS = 15000; +// 6 seconds polling interval +const PROTOCOL_FEE_UTILS_POLLING_INTERVAL_IN_MS = 6000; const PROTOCOL_FEE_MULTIPLIER = new BigNumber(150000); // default 50% buffer for selecting native orders to be aggregated with other sources 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 0f7564330e..11a472caef 100644 --- a/packages/asset-swapper/src/utils/market_operation_utils/fills.ts +++ b/packages/asset-swapper/src/utils/market_operation_utils/fills.ts @@ -197,7 +197,7 @@ export function getPathAdjustedSize(path: Fill[], targetInput: BigNumber = POSIT return [input.integerValue(), output.integerValue()]; } -export function isValidPath(path: Fill[]): boolean { +export function isValidPath(path: Fill[], skipDuplicateCheck: boolean = false): boolean { let flags = 0; for (let i = 0; i < path.length; ++i) { // Fill must immediately follow its parent. @@ -206,10 +206,12 @@ export function isValidPath(path: Fill[]): boolean { return false; } } - // Fill must not be duplicated. - for (let j = 0; j < i; ++j) { - if (path[i] === path[j]) { - return false; + if (!skipDuplicateCheck) { + // Fill must not be duplicated. + for (let j = 0; j < i; ++j) { + if (path[i] === path[j]) { + return false; + } } } flags |= path[i].flags; 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 61b0d46215..51bae80427 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 @@ -34,7 +34,6 @@ function mixPaths( targetInput: BigNumber, maxSteps: number = 2 ** 15, ): Fill[] { - const allFills = [...pathA, ...pathB].sort((a, b) => b.rate.comparedTo(a.rate)); let bestPath: Fill[] = []; let bestPathInput = ZERO_AMOUNT; let bestPathRate = ZERO_AMOUNT; @@ -42,12 +41,12 @@ function mixPaths( const _isBetterPath = (input: BigNumber, rate: BigNumber) => { if (bestPathInput.lt(targetInput)) { return input.gt(bestPathInput); - } else if (input.gte(bestPathInput)) { + } else if (input.gte(targetInput)) { return rate.gt(bestPathRate); } return false; }; - const _walk = (path: Fill[], input: BigNumber, output: BigNumber) => { + const _walk = (path: Fill[], input: BigNumber, output: BigNumber, allFills: Fill[]) => { steps += 1; const rate = getRate(side, input, output); if (_isBetterPath(input, rate)) { @@ -55,20 +54,35 @@ function mixPaths( bestPathInput = input; bestPathRate = rate; } - if (input.lt(targetInput)) { - for (const fill of allFills) { - if (steps >= maxSteps) { + const remainingInput = targetInput.minus(input); + if (remainingInput.gt(0)) { + for (let i = 0; i < allFills.length; ++i) { + const fill = allFills[i]; + if (steps + 1 >= maxSteps) { break; } const childPath = [...path, fill]; - if (!isValidPath(childPath)) { + if (!isValidPath(childPath, true)) { continue; } - _walk(childPath, input.plus(fill.input), output.plus(fill.adjustedOutput)); + // Remove this fill from the next list of candidate fills. + const nextAllFills = allFills.slice(); + nextAllFills.splice(i, 1); + // Recurse. + _walk( + childPath, + input.plus(BigNumber.min(remainingInput, fill.input)), + output.plus( + // Clip the output of the next fill to the remaining + // input. + clipFillAdjustedOutput(fill, remainingInput), + ), + nextAllFills, + ); } } }; - _walk(bestPath, ZERO_AMOUNT, ZERO_AMOUNT); + _walk(bestPath, ZERO_AMOUNT, ZERO_AMOUNT, [...pathA, ...pathB].sort((a, b) => b.rate.comparedTo(a.rate))); return bestPath; } @@ -77,6 +91,14 @@ function isPathComplete(path: Fill[], targetInput: BigNumber): boolean { return input.gte(targetInput); } +function clipFillAdjustedOutput(fill: Fill, remainingInput: BigNumber): BigNumber { + if (fill.input.lte(remainingInput)) { + return fill.adjustedOutput; + } + const penalty = fill.adjustedOutput.minus(fill.output); + return fill.output.times(remainingInput.div(fill.input)).plus(penalty); +} + function getRate(side: MarketOperation, input: BigNumber, output: BigNumber): BigNumber { if (input.eq(0) || output.eq(0)) { return ZERO_AMOUNT;