fix: fallbacks duplicate check on source-index (#307)

packages/asset-swapper/CHANGELOG.json

@@ -1,4 +1,12 @@
 [
+    {
+        "version": "16.25.0",
+        "changes": [
+            {
+                "note": "Fix: fallback fills which have not been used, unique id by source-index"
+            }
+        ]
+    },
     {
         "timestamp": 1628665757,
         "version": "16.24.1",

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

@@ -38,7 +38,7 @@ import {
 import { createFills } from './fills';
 import { getBestTwoHopQuote } from './multihop_utils';
 import { createOrdersFromTwoHopSample } from './orders';
-import { PathPenaltyOpts } from './path';
+import { Path, PathPenaltyOpts } from './path';
 import { fillsToSortedPaths, findOptimalPathAsync } from './path_optimizer';
 import { DexOrderSampler, getSampleAmounts } from './sampler';
 import { SourceFilters } from './source_filters';
@@ -47,6 +47,8 @@ import {
     CollapsedFill,
     DexSample,
     ERC20BridgeSource,
+    Fill,
+    FillData,
     GenerateOptimizedOrdersOpts,
     GetMarketOrdersOpts,
     MarketSideLiquidity,
@@ -433,7 +435,6 @@ export class MarketOperationUtils {
             inputAmountPerEth,
         } = marketSideLiquidity;
         const { nativeOrders, rfqtIndicativeQuotes, dexQuotes } = quotes;
-        const maxFallbackSlippage = opts.maxFallbackSlippage || 0;
 
         const orderOpts = {
             side,
@@ -512,31 +513,8 @@ export class MarketOperationUtils {
             throw new Error(AggregationError.NoOptimalPath);
         }
 
-        // Generate a fallback path if sources requiring a fallback (fragile) are in the optimal path.
+        // Generate a fallback path if required
-        // Native is relatively fragile (limit order collision, expiry, or lack of available maker balance)
+        await this._addOptionalFallbackAsync(side, inputAmount, optimalPath, fills, opts, penaltyOpts);
-        // LiquidityProvider is relatively fragile (collision)
-        const fragileSources = [ERC20BridgeSource.Native, ERC20BridgeSource.LiquidityProvider];
-        const fragileFills = optimalPath.fills.filter(f => fragileSources.includes(f.source));
-        if (opts.allowFallback && fragileFills.length !== 0) {
-            // We create a fallback path that is exclusive of Native liquidity
-            // This is the optimal on-chain path for the entire input amount
-            const sturdyFills = fills.filter(p => p.length > 0 && !fragileSources.includes(p[0].source));
-            const sturdyOptimalPath = await findOptimalPathAsync(side, sturdyFills, inputAmount, opts.runLimit, {
-                ...penaltyOpts,
-                exchangeProxyOverhead: (sourceFlags: bigint) =>
-                    // tslint:disable-next-line: no-bitwise
-                    penaltyOpts.exchangeProxyOverhead(sourceFlags | optimalPath.sourceFlags),
-            });
-            // Calculate the slippage of on-chain sources compared to the most optimal path
-            // if within an acceptable threshold we enable a fallback to prevent reverts
-            if (
-                sturdyOptimalPath !== undefined &&
-                (fragileFills.length === optimalPath.fills.length ||
-                    sturdyOptimalPath.adjustedSlippage(optimalPathRate) <= maxFallbackSlippage)
-            ) {
-                optimalPath.addFallback(sturdyOptimalPath);
-            }
-        }
         const collapsedPath = optimalPath.collapse(orderOpts);
 
         return {
@@ -727,6 +705,44 @@ export class MarketOperationUtils {
             }),
         );
     }
+
+    // tslint:disable-next-line: prefer-function-over-method
+    private async _addOptionalFallbackAsync(
+        side: MarketOperation,
+        inputAmount: BigNumber,
+        optimalPath: Path,
+        fills: Array<Array<Fill<FillData>>>,
+        opts: GenerateOptimizedOrdersOpts,
+        penaltyOpts: PathPenaltyOpts,
+    ): Promise<void> {
+        const maxFallbackSlippage = opts.maxFallbackSlippage || 0;
+        const optimalPathRate = optimalPath ? optimalPath.adjustedRate() : ZERO_AMOUNT;
+        // Generate a fallback path if sources requiring a fallback (fragile) are in the optimal path.
+        // Native is relatively fragile (limit order collision, expiry, or lack of available maker balance)
+        // LiquidityProvider is relatively fragile (collision)
+        const fragileSources = [ERC20BridgeSource.Native, ERC20BridgeSource.LiquidityProvider];
+        const fragileFills = optimalPath.fills.filter(f => fragileSources.includes(f.source));
+        if (opts.allowFallback && fragileFills.length !== 0) {
+            // We create a fallback path that is exclusive of Native liquidity
+            // This is the optimal on-chain path for the entire input amount
+            const sturdyFills = fills.filter(p => p.length > 0 && !fragileSources.includes(p[0].source));
+            const sturdyOptimalPath = await findOptimalPathAsync(side, sturdyFills, inputAmount, opts.runLimit, {
+                ...penaltyOpts,
+                exchangeProxyOverhead: (sourceFlags: bigint) =>
+                    // tslint:disable-next-line: no-bitwise
+                    penaltyOpts.exchangeProxyOverhead(sourceFlags | optimalPath.sourceFlags),
+            });
+            // Calculate the slippage of on-chain sources compared to the most optimal path
+            // if within an acceptable threshold we enable a fallback to prevent reverts
+            if (
+                sturdyOptimalPath !== undefined &&
+                (fragileFills.length === optimalPath.fills.length ||
+                    sturdyOptimalPath.adjustedSlippage(optimalPathRate) <= maxFallbackSlippage)
+            ) {
+                optimalPath.addFallback(sturdyOptimalPath);
+            }
+        }
+    }
 }
 
 // tslint:disable: max-file-line-count

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

@@ -99,14 +99,18 @@ export class Path {
         // In the previous step we dropped any hanging Native partial fills, as to not fully fill
         const nativeFills = this.fills.filter(f => f.source === ERC20BridgeSource.Native);
         const otherFills = this.fills.filter(f => f.source !== ERC20BridgeSource.Native);
-        const otherSourcePathIds = otherFills.map(f => f.sourcePathId);
+
+        // Map to the unique source id and the index to represent a unique fill
+        const fillToFillId = (fill: Fill) => `${fill.sourcePathId}${fill.index}`;
+        const otherFillIds = otherFills.map(f => fillToFillId(f));
+
         this.fills = [
             // Append all of the native fills first
             ...nativeFills.filter(f => f !== lastNativeFillIfExists),
             // Add the other fills that are not native in the optimal path
             ...otherFills,
-            // Add the fallbacks to the end that aren't already included
+            // Add the fills to the end that aren't already included
-            ...fallback.fills.filter(f => !otherSourcePathIds.includes(f.sourcePathId)),
+            ...fallback.fills.filter(f => !otherFillIds.includes(fillToFillId(f))),
         ];
         // Recompute the source flags
         this.sourceFlags = this.fills.reduce((flags, fill) => flags | fill.flags, BigInt(0));

packages/asset-swapper/test/market_operation_utils_test.ts

@@ -1166,28 +1166,6 @@ describe('MarketOperationUtils tests', () => {
                 expect(orderSources.sort()).to.deep.eq(expectedSources.sort());
             });
 
-            it('fallback orders use different sources', async () => {
-                const rates: RatesBySource = {};
-                rates[ERC20BridgeSource.Native] = [0.9, 0.8, 0.5, 0.5];
-                rates[ERC20BridgeSource.Uniswap] = [0.6, 0.05, 0.01, 0.01];
-                rates[ERC20BridgeSource.Eth2Dai] = [0.4, 0.3, 0.01, 0.01];
-                rates[ERC20BridgeSource.Kyber] = [0.35, 0.2, 0.01, 0.01];
-                replaceSamplerOps({
-                    getSellQuotes: createGetMultipleSellQuotesOperationFromRates(rates),
-                });
-                const improvedOrdersResponse = await getMarketSellOrdersAsync(
-                    marketOperationUtils,
-                    createOrdersFromSellRates(FILL_AMOUNT, rates[ERC20BridgeSource.Native]),
-                    FILL_AMOUNT,
-                    { ...DEFAULT_OPTS, numSamples: 4, allowFallback: true },
-                );
-                const improvedOrders = improvedOrdersResponse.optimizedOrders;
-                const orderSources = improvedOrders.map(o => o.fills[0].source);
-                const firstSources = orderSources.slice(0, 4);
-                const secondSources = orderSources.slice(4);
-                expect(_.intersection(firstSources, secondSources)).to.be.length(0);
-            });
-
             it('does not create a fallback if below maxFallbackSlippage', async () => {
                 const rates: RatesBySource = {};
                 rates[ERC20BridgeSource.Native] = [1, 1, 0.01, 0.01];
@@ -1602,27 +1580,6 @@ describe('MarketOperationUtils tests', () => {
                 expect(orderSources.sort()).to.deep.eq(expectedSources.sort());
             });
 
-            it('fallback orders use different sources', async () => {
-                const rates: RatesBySource = { ...ZERO_RATES };
-                rates[ERC20BridgeSource.Native] = [0.9, 0.8, 0.5, 0.5];
-                rates[ERC20BridgeSource.Uniswap] = [0.6, 0.05, 0.01, 0.01];
-                rates[ERC20BridgeSource.Eth2Dai] = [0.4, 0.3, 0.01, 0.01];
-                replaceSamplerOps({
-                    getBuyQuotes: createGetMultipleBuyQuotesOperationFromRates(rates),
-                });
-                const improvedOrdersResponse = await getMarketBuyOrdersAsync(
-                    marketOperationUtils,
-                    createOrdersFromBuyRates(FILL_AMOUNT, rates[ERC20BridgeSource.Native]),
-                    FILL_AMOUNT,
-                    { ...DEFAULT_OPTS, numSamples: 4, allowFallback: true },
-                );
-                const improvedOrders = improvedOrdersResponse.optimizedOrders;
-                const orderSources = improvedOrders.map(o => o.fills[0].source);
-                const firstSources = orderSources.slice(0, 4);
-                const secondSources = orderSources.slice(4);
-                expect(_.intersection(firstSources, secondSources)).to.be.length(0);
-            });
-
             it('does not create a fallback if below maxFallbackSlippage', async () => {
                 const rates: RatesBySource = { ...ZERO_RATES };
                 rates[ERC20BridgeSource.Native] = [1, 1, 0.01, 0.01];

packages/asset-swapper/test/path_test.ts

@@ -7,6 +7,7 @@ import { ERC20BridgeSource, Fill } from '../src/utils/market_operation_utils/typ
 
 const createFill = (
     source: ERC20BridgeSource,
+    index: number = 0,
     input: BigNumber = new BigNumber(100),
     output: BigNumber = new BigNumber(100),
 ): Fill =>
@@ -18,6 +19,7 @@ const createFill = (
         adjustedOutput: output,
         flags: BigInt(0),
         sourcePathId: source,
+        index,
     } as Fill);
 
 describe('Path', () => {
@@ -83,4 +85,26 @@ describe('Path', () => {
         const sources = path.fills.map(f => f.source);
         expect(sources).to.deep.eq([ERC20BridgeSource.Uniswap, ERC20BridgeSource.LiquidityProvider]);
     });
+    it('Removes partial Native orders and replaces with unused fills', () => {
+        const targetInput = new BigNumber(100);
+        const path = Path.create(
+            MarketOperation.Sell,
+            [
+                createFill(ERC20BridgeSource.Uniswap, 0, new BigNumber(50)),
+                createFill(ERC20BridgeSource.Native, 0, new BigNumber(50)),
+            ],
+            targetInput,
+        );
+        const fallback = Path.create(
+            MarketOperation.Sell,
+            [
+                createFill(ERC20BridgeSource.Uniswap, 0, new BigNumber(50)),
+                createFill(ERC20BridgeSource.Uniswap, 1, new BigNumber(50)),
+            ],
+            targetInput,
+        );
+        path.addFallback(fallback);
+        const sources = path.fills.map(f => f.source);
+        expect(sources).to.deep.eq([ERC20BridgeSource.Uniswap, ERC20BridgeSource.Uniswap]);
+    });
 });