diff --git a/packages/asset-swapper/CHANGELOG.json b/packages/asset-swapper/CHANGELOG.json index 7c572eb760..13d08c94f0 100644 --- a/packages/asset-swapper/CHANGELOG.json +++ b/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", diff --git a/packages/asset-swapper/src/utils/market_operation_utils/index.ts b/packages/asset-swapper/src/utils/market_operation_utils/index.ts index 4f90f6b5aa..e96e0ea0c4 100644 --- a/packages/asset-swapper/src/utils/market_operation_utils/index.ts +++ b/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. - // 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); - } - } + // Generate a fallback path if required + await this._addOptionalFallbackAsync(side, inputAmount, optimalPath, fills, opts, penaltyOpts); 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>>, + opts: GenerateOptimizedOrdersOpts, + penaltyOpts: PathPenaltyOpts, + ): Promise { + 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 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 4b19d35c98..437f264296 100644 --- a/packages/asset-swapper/src/utils/market_operation_utils/path.ts +++ b/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 - ...fallback.fills.filter(f => !otherSourcePathIds.includes(f.sourcePathId)), + // Add the fills to the end that aren't already included + ...fallback.fills.filter(f => !otherFillIds.includes(fillToFillId(f))), ]; // Recompute the source flags this.sourceFlags = this.fills.reduce((flags, fill) => flags | fill.flags, BigInt(0)); diff --git a/packages/asset-swapper/test/market_operation_utils_test.ts b/packages/asset-swapper/test/market_operation_utils_test.ts index 6db141df7c..50c16b296f 100644 --- a/packages/asset-swapper/test/market_operation_utils_test.ts +++ b/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]; diff --git a/packages/asset-swapper/test/path_test.ts b/packages/asset-swapper/test/path_test.ts index 29dfff1cb3..d7c80c1780 100644 --- a/packages/asset-swapper/test/path_test.ts +++ b/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]); + }); });