fix: PLP fallback (#272)

* fix: PLP fallback

* CHANGELOG

packages/asset-swapper/CHANGELOG.json

@@ -1,4 +1,13 @@
 [
+    {
+        "version": "16.19.1",
+        "changes": [
+            {
+                "note": "Fix LiquidityProvider fallback",
+                "pr": 272
+            }
+        ]
+    },
     {
         "version": "16.19.0",
         "changes": [

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

@@ -78,6 +78,11 @@ export class Path {
         return this;
     }
 
+    /**
+     * Add a fallback path to the current path
+     * Fallback must contain exclusive fills that are
+     * not present in this path
+     */
     public addFallback(fallback: Path): this {
         // If the last fill is Native and penultimate is not, then the intention was to partial fill
         // In this case we drop it entirely as we can't handle a failure at the end and we don't
@@ -93,7 +98,16 @@ export class Path {
         // an additional protocol fee. I.e [Uniswap,Native,Kyber] becomes [Native,Uniswap,Kyber]
         // 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);
-        this.fills = [...nativeFills.filter(f => f !== lastNativeFillIfExists), ...fallback.fills];
+        const otherFills = this.fills.filter(f => f.source !== ERC20BridgeSource.Native);
+        const otherSourcePathIds = otherFills.map(f => f.sourcePathId);
+        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
+            ...fallback.fills.filter(f => !otherSourcePathIds.includes(f.sourcePathId)),
+        ];
         // Recompute the source flags
         this.sourceFlags = this.fills.reduce((flags, fill) => flags | fill.flags, BigInt(0));
         return this;

packages/asset-swapper/test/path_test.ts

@@ -0,0 +1,86 @@
+import { expect } from '@0x/contracts-test-utils';
+import { BigNumber } from '@0x/utils';
+
+import { MarketOperation } from '../src/types';
+import { Path } from '../src/utils/market_operation_utils/path';
+import { ERC20BridgeSource, Fill } from '../src/utils/market_operation_utils/types';
+
+const createFill = (
+    source: ERC20BridgeSource,
+    input: BigNumber = new BigNumber(100),
+    output: BigNumber = new BigNumber(100),
+): Fill =>
+    // tslint:disable-next-line: no-object-literal-type-assertion
+    ({
+        source,
+        input,
+        output,
+        adjustedOutput: output,
+        flags: BigInt(0),
+        sourcePathId: source,
+    } as Fill);
+
+describe('Path', () => {
+    it('Adds a fallback', () => {
+        const targetInput = new BigNumber(100);
+        const path = Path.create(
+            MarketOperation.Sell,
+            [createFill(ERC20BridgeSource.Native), createFill(ERC20BridgeSource.Native)],
+            targetInput,
+        );
+        const fallback = Path.create(MarketOperation.Sell, [createFill(ERC20BridgeSource.Uniswap)], targetInput);
+        path.addFallback(fallback);
+        const sources = path.fills.map(f => f.source);
+        expect(sources).to.deep.eq([ERC20BridgeSource.Native, ERC20BridgeSource.Native, ERC20BridgeSource.Uniswap]);
+    });
+
+    it('Adds a fallback with LiquidityProvider', () => {
+        const targetInput = new BigNumber(100);
+        const path = Path.create(
+            MarketOperation.Sell,
+            [createFill(ERC20BridgeSource.Native), createFill(ERC20BridgeSource.LiquidityProvider)],
+            targetInput,
+        );
+        const fallback = Path.create(MarketOperation.Sell, [createFill(ERC20BridgeSource.Uniswap)], targetInput);
+        path.addFallback(fallback);
+        const sources = path.fills.map(f => f.source);
+        expect(sources).to.deep.eq([
+            ERC20BridgeSource.Native,
+            ERC20BridgeSource.LiquidityProvider,
+            ERC20BridgeSource.Uniswap,
+        ]);
+    });
+
+    it('Removes partial Native orders', () => {
+        const targetInput = new BigNumber(100);
+        const path = Path.create(
+            MarketOperation.Sell,
+            [
+                createFill(ERC20BridgeSource.Uniswap),
+                createFill(ERC20BridgeSource.LiquidityProvider),
+                createFill(ERC20BridgeSource.Native),
+            ],
+            targetInput,
+        );
+        const fallback = Path.create(MarketOperation.Sell, [createFill(ERC20BridgeSource.Kyber)], targetInput);
+        path.addFallback(fallback);
+        const sources = path.fills.map(f => f.source);
+        expect(sources).to.deep.eq([
+            ERC20BridgeSource.Uniswap,
+            ERC20BridgeSource.LiquidityProvider,
+            ERC20BridgeSource.Kyber,
+        ]);
+    });
+    it('Handles duplicates', () => {
+        const targetInput = new BigNumber(100);
+        const path = Path.create(
+            MarketOperation.Sell,
+            [createFill(ERC20BridgeSource.Uniswap), createFill(ERC20BridgeSource.LiquidityProvider)],
+            targetInput,
+        );
+        const fallback = Path.create(MarketOperation.Sell, [createFill(ERC20BridgeSource.Uniswap)], targetInput);
+        path.addFallback(fallback);
+        const sources = path.fills.map(f => f.source);
+        expect(sources).to.deep.eq([ERC20BridgeSource.Uniswap, ERC20BridgeSource.LiquidityProvider]);
+    });
+});