Merge pull request #2526 from 0xProject/fix/asset-swapper/optimizer-f…

…ee-bugs AssetSwapper: Fix quote optimizer fee bug
0xProject · Mar 24, 2020 · 99b0a95 · 99b0a95
2 parents 7a68260 + 0fdd318
commit 99b0a95
Show file tree

Hide file tree

Showing 4 changed files with 44 additions and 16 deletions.
diff --git 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
@@ -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
@@ -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
@@ -34,41 +34,55 @@ 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;
     let steps = 0;
     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)) {
             bestPath = path;
             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;