diff --git a/packages/asset-swapper/CHANGELOG.json b/packages/asset-swapper/CHANGELOG.json index 3b3bfa2da4..96d1f72bf5 100644 --- a/packages/asset-swapper/CHANGELOG.json +++ b/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": [ diff --git a/packages/asset-swapper/src/utils/market_operation_utils/path.ts b/packages/asset-swapper/src/utils/market_operation_utils/path.ts index 055765937b..4b19d35c98 100644 --- a/packages/asset-swapper/src/utils/market_operation_utils/path.ts +++ b/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; diff --git a/packages/asset-swapper/test/path_test.ts b/packages/asset-swapper/test/path_test.ts new file mode 100644 index 0000000000..29dfff1cb3 --- /dev/null +++ b/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]); + }); +});