From fb21ca5404fa558d6ef6195c1111f8e3770a6148 Mon Sep 17 00:00:00 2001 From: Jacob Evans Date: Thu, 15 Oct 2020 17:31:22 +1000 Subject: [PATCH] fix: [asset-swapper] MultiHop edge cases (#2730) * fix: [asset-swapper] MultiHop edge cases * CHANGELOG --- packages/asset-swapper/CHANGELOG.json | 4 ++++ .../src/utils/market_operation_utils/index.ts | 15 ++++++++---- .../market_operation_utils/multihop_utils.ts | 23 ++++++++++++++----- .../sampler_operations.ts | 5 ++-- 4 files changed, 34 insertions(+), 13 deletions(-) diff --git a/packages/asset-swapper/CHANGELOG.json b/packages/asset-swapper/CHANGELOG.json index 8d12babd61..e9fd8e7331 100644 --- a/packages/asset-swapper/CHANGELOG.json +++ b/packages/asset-swapper/CHANGELOG.json @@ -153,6 +153,10 @@ { "note": "Added `DODO`", "pr": 2701 + }, + { + "note": "Fix for some edge cases with `includedSources` and `MultiHop`", + "pr": 2730 } ] }, 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 58fcf395c5..60c40d70fc 100644 --- a/packages/asset-swapper/src/utils/market_operation_utils/index.ts +++ b/packages/asset-swapper/src/utils/market_operation_utils/index.ts @@ -138,7 +138,9 @@ export class MarketOperationUtils { const [makerToken, takerToken] = getNativeOrderTokens(nativeOrders[0]); const sampleAmounts = getSampleAmounts(takerAmount, _opts.numSamples, _opts.sampleDistributionBase); const requestFilters = new SourceFilters().exclude(_opts.excludedSources).include(_opts.includedSources); - const feeSourceFilters = this._feeSources.merge(requestFilters); + // We don't exclude from the fee sources as we always want to be able to get a price + // of taker to Eth or maker to Eth, especially for MultiHop + const feeSourceFilters = this._feeSources; const quoteSourceFilters = this._sellSources.merge(requestFilters); const { @@ -576,11 +578,9 @@ export class MarketOperationUtils { ethToInputRate, exchangeProxyOverhead: opts.exchangeProxyOverhead || (() => ZERO_AMOUNT), }; + const optimalPath = await findOptimalPathAsync(side, fills, inputAmount, opts.runLimit, optimizerOpts); - if (optimalPath === undefined) { - throw new Error(AggregationError.NoOptimalPath); - } - const optimalPathRate = optimalPath.adjustedRate(); + const optimalPathRate = optimalPath ? optimalPath.adjustedRate() : ZERO_AMOUNT; const { adjustedRate: bestTwoHopRate, quote: bestTwoHopQuote } = getBestTwoHopQuote( marketSideLiquidity, @@ -596,6 +596,11 @@ export class MarketOperationUtils { }; } + // If there is no optimal path AND we didn't return a MultiHop quote, then throw + if (optimalPath === undefined) { + throw new Error(AggregationError.NoOptimalPath); + } + // Generate a fallback path if native orders are in the optimal path. const nativeFills = optimalPath.fills.filter(f => f.source === ERC20BridgeSource.Native); if (opts.allowFallback && nativeFills.length !== 0) { diff --git a/packages/asset-swapper/src/utils/market_operation_utils/multihop_utils.ts b/packages/asset-swapper/src/utils/market_operation_utils/multihop_utils.ts index 9d6832b858..0a70ab1814 100644 --- a/packages/asset-swapper/src/utils/market_operation_utils/multihop_utils.ts +++ b/packages/asset-swapper/src/utils/market_operation_utils/multihop_utils.ts @@ -46,26 +46,37 @@ export function getBestTwoHopQuote( exchangeProxyOverhead?: ExchangeProxyOverhead, ): { quote: DexSample | undefined; adjustedRate: BigNumber } { const { side, inputAmount, ethToOutputRate, twoHopQuotes } = marketSideLiquidity; - if (twoHopQuotes.length === 0) { - return { adjustedRate: ZERO_AMOUNT, quote: undefined }; + // Ensure the expected data we require exists. In the case where all hops reverted + // or there were no sources included that allowed for multi hop, + // we can end up with empty, but not undefined, fill data + const filteredQuotes = twoHopQuotes.filter( + quote => + quote && + quote.fillData && + quote.fillData.firstHopSource && + quote.fillData.secondHopSource && + quote.output.isGreaterThan(ZERO_AMOUNT), + ); + if (filteredQuotes.length === 0) { + return { quote: undefined, adjustedRate: ZERO_AMOUNT }; } - const best = twoHopQuotes + const best = filteredQuotes .map(quote => getTwoHopAdjustedRate(side, quote, inputAmount, ethToOutputRate, feeSchedule, exchangeProxyOverhead), ) .reduce( (prev, curr, i) => - curr.isGreaterThan(prev.adjustedRate) ? { adjustedRate: curr, quote: twoHopQuotes[i] } : prev, + curr.isGreaterThan(prev.adjustedRate) ? { adjustedRate: curr, quote: filteredQuotes[i] } : prev, { adjustedRate: getTwoHopAdjustedRate( side, - twoHopQuotes[0], + filteredQuotes[0], inputAmount, ethToOutputRate, feeSchedule, exchangeProxyOverhead, ), - quote: twoHopQuotes[0], + quote: filteredQuotes[0], }, ); return best; diff --git a/packages/asset-swapper/src/utils/market_operation_utils/sampler_operations.ts b/packages/asset-swapper/src/utils/market_operation_utils/sampler_operations.ts index 1851b92e83..3ef05d0c94 100644 --- a/packages/asset-swapper/src/utils/market_operation_utils/sampler_operations.ts +++ b/packages/asset-swapper/src/utils/market_operation_utils/sampler_operations.ts @@ -600,11 +600,12 @@ export class SamplerOperations { const [firstHop, secondHop, buyAmount] = this._samplerContract.getABIDecodedReturnData< [HopInfo, HopInfo, BigNumber] >('sampleTwoHopSell', callResults); + // Ensure the hop sources are set even when the buy amount is zero + fillData.firstHopSource = firstHopOps[firstHop.sourceIndex.toNumber()]; + fillData.secondHopSource = secondHopOps[secondHop.sourceIndex.toNumber()]; if (buyAmount.isZero()) { return [ZERO_AMOUNT]; } - fillData.firstHopSource = firstHopOps[firstHop.sourceIndex.toNumber()]; - fillData.secondHopSource = secondHopOps[secondHop.sourceIndex.toNumber()]; fillData.firstHopSource.handleCallResults(firstHop.returnData); fillData.secondHopSource.handleCallResults(secondHop.returnData); return [buyAmount];