asset-swapper: Fix optimization of buy paths (#2655)

* `@0x/asset-swapper`: Fix optimization of buy paths

* `@0x/asset-swapper`: Fix optimization of buy paths

* `@0x/asset-swapper`: Optimize the optimizer.

* `@0x/asset-swapper`: Remove unused `Fill` fields

Co-authored-by: Lawrence Forman <me@merklejerk.com>

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
             }
         ]
     },

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<Fill & { adjustedRate: BigNumber }> = [];
     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);
+            if (di.gt(0)) {
-            output = output.plus(fill.adjustedOutput.times(di.div(fill.input)));
+                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);
+    const [input, output] = getPathAdjustedSize(path, targetInput);
-    if (output.eq(0)) {
+    return getRate(side, input, output);
-        return ZERO_AMOUNT;
-    }
-    return side === MarketOperation.Sell ? output.div(targetInput) : targetInput.div(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);
+}

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<Fill[] | undefined> {
-    // 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);
+    const _maxSteps = Math.max(maxSteps, 32);
-    let bestPath: Fill[] = pathA;
-    let bestPathInput = getPathSize(pathA, targetInput)[0];
-    let bestPathRate = getPathAdjustedRate(side, pathA, targetInput);
     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) {
+            for (let i = 0; i < remainingFills.length && steps < _maxSteps; ++i) {
-                const fill = allFills[i];
+                const fill = remainingFills[i];
-                const nextPath = [...path, fill];
                 // 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();
+                const nextRemainingFills = remainingFills.slice();
-                nextAllFills.splice(i, 1);
+                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 allFills = [...pathA, ...pathB];
-    const sources = allPaths.filter(f => f.index === 0).map(f => f.sourcePathId);
+    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(
+    // Sort subpaths by rate and keep fills contiguous to improve our
-        [],
+    // chances of walking ideal, valid paths first.
-        ZERO_AMOUNT,
+    const sortedFills = allFills.sort((a, b) => {
-        ZERO_AMOUNT,
+        if (a.sourcePathId !== b.sourcePathId) {
-        // Sort subpaths by rate and keep fills contiguous to improve our
+            return rateBySource[b.sourcePathId].comparedTo(rateBySource[a.sourcePathId]);
-        // chances of walking ideal, valid paths first.
+        }
-        allPaths.sort((a, b) => {
+        return a.index - b.index;
-            if (a.sourcePathId !== b.sourcePathId) {
+    });
-                return rateBySource[b.sourcePathId].comparedTo(rateBySource[a.sourcePathId]);
+    _walk([], ZERO_AMOUNT, ZERO_AMOUNT, 0, sortedFills);
-            }
+    if (!isValidPath(bestPath)) {
-            return a.index - b.index;
+        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);
+    return remainingInput.times(fill.output.div(fill.input)).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);
 }

packages/asset-swapper/src/utils/market_operation_utils/types.ts

@@ -130,10 +130,6 @@ export interface Fill<TFillData extends FillData = FillData> {
     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.

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.Eth2Dai] = [0.49, 0.02, 0.01, 0.01];
-                rates[ERC20BridgeSource.Uniswap] = [0.48, 0.47, 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],
                 ]);
             });
         });