Skip to content

feat: get cached routes to filter out mismatched mixed routes against requested protocol versions #1065

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions lib/handlers/quote/quote.ts
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,7 @@ export class QuoteHandler extends APIGLambdaHandler<
...(excludedProtocolsFromMixed ? { excludedProtocolsFromMixed } : {}),
shouldEnableMixedRouteEthWeth: shouldEnableMixedRouteEthWeth,
...(cachedRoutesRouteIds ? { cachedRoutesRouteIds } : {}),
enableMixedRouteWithUR1_2Percent: 1, // enable mixed route with UR v1.2 fix at 1%, to see whether we see quote endpoint perf improvement.
}

metric.putMetric(`${intent}Intent`, 1, MetricLoggerUnit.Count)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {
AlphaRouterConfig,
CachedRoute,
CachedRoutes,
CacheMode,
Expand Down Expand Up @@ -95,16 +96,20 @@ export class DynamoRouteCachingProvider extends IRouteCachingProvider {
* @param quoteCurrency
* @param tradeType
* @param protocols
* @param currentBlockNumber
* @param optimistic
* @param alphaRouterConfig
* @protected
*/
protected async _getCachedRoute(
protected override async _getCachedRoute(
chainId: ChainId,
amount: CurrencyAmount<Currency>,
quoteCurrency: Currency,
tradeType: TradeType,
protocols: Protocol[],
currentBlockNumber: number,
optimistic: boolean
optimistic: boolean,
alphaRouterConfig?: AlphaRouterConfig
): Promise<CachedRoutes | undefined> {
const { currencyIn, currencyOut } = this.determineCurrencyInOut(amount, quoteCurrency, tradeType)

Expand Down Expand Up @@ -142,9 +147,42 @@ export class DynamoRouteCachingProvider extends IRouteCachingProvider {
const filteredItems = result.Items
// Older routes might not have the protocol field, so we keep them if they don't have it
.filter((record) => !record.protocol || protocols.includes(record.protocol))
// Older routes might not have the protocolsInvolved field, so we keep them if they don't have it
.filter((record) => {
return (
!record.protocolsInvolved ||
!protocols.includes(Protocol.MIXED) ||
// We want to roll out the mixed route with UR v1_2 with percent control,
// along with the cached routes so that we can test the performance of the mixed route with UR v1_2ss
((alphaRouterConfig?.enableMixedRouteWithUR1_2Percent ?? 0) >= Math.random() * 100 &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as we discussed offline this should be passed as a bool, e.g. enableMixedRouteWithUR1_2.
But do we also need to always allow if UR2_0?

// requested protocols do not involve MIXED, so there is no need to filter by protocolsInvolved
// we know MIXED is getting requested, in this case, we need to ensure that protocolsInvolved contains all the requested protocols
(record.protocolsInvolved as String)
.split(',')
.every((protocol) => protocols.includes(protocol as Protocol)))
)
})
.sort((a, b) => b.blockNumber - a.blockNumber)
.slice(0, this.ROUTES_TO_TAKE_FROM_ROUTES_DB)

if (protocols.includes(Protocol.MIXED)) {
filteredItems.forEach((record) => {
if (
record.protocolsInvolved &&
!(record.protocolsInvolved as String)
.split(',')
.every((protocol) => protocols.includes(protocol as Protocol))
) {
metric.putMetric(
'RoutesDbFilteredEntriesMixedRoutesContainNonMatchingProtocols',
1,
MetricLoggerUnit.Count
)
log.error(`Cached Route entry ${JSON.stringify(record)} contains non matching protocols ${protocols}`)
}
})
}

result.Items = filteredItems

return this.parseCachedRoutes(result, chainId, currentBlockNumber, optimistic, partitionKey, amount, protocols)
Expand Down
14 changes: 7 additions & 7 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@
"@uniswap/permit2-sdk": "^1.3.0",
"@uniswap/router-sdk": "^2.0.2",
"@uniswap/sdk-core": "^7.7.1",
"@uniswap/smart-order-router": "4.20.10",
"@uniswap/smart-order-router": "4.20.11",
"@uniswap/token-lists": "^1.0.0-beta.33",
"@uniswap/universal-router-sdk": "^4.19.5",
"@uniswap/v2-sdk": "^4.15.2",
Expand Down
43 changes: 43 additions & 0 deletions test/mocha/e2e/quote.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1495,6 +1495,49 @@ describe('quote', function () {
}
})

// Test that repros https://uniswapteam.slack.com/archives/C08JM8SKMV2/p1742424330903569?thread_ts=1742424322.824189&cid=C08JM8SKMV2
// Where quote request contains v2,v3
it(`erc20 -> erc20 cached routes should not return mixed route`, async () => {
if (type !== 'exactIn') {
return
}

const quoteReq: QuoteQueryParams = {
tokenInAddress: 'ETH',
tokenInChainId: 1,
tokenOutAddress: '0xdAC17F958D2ee523a2206206994597C13D831ec7',
tokenOutChainId: 1,
amount: '1000000000000000000',
type,
recipient: alice.address,
slippageTolerance: SLIPPAGE,
deadline: '360',
algorithm: 'alpha',
protocols: 'v2,v3',
enableUniversalRouter: true,
enableDebug: true,
}

const queryParams = qs.stringify(quoteReq)

const response: AxiosResponse<QuoteResponse> = await axios.get<QuoteResponse>(`${API}?${queryParams}`, {
headers: HEADERS_1_2,
})

const {
data: { route },
status,
} = response

expect(status).to.equal(200)

// Verify no mixed routes returned when using cached routes
for (const r of route) {
expect(r.every((pool) => pool.type === 'v3-pool' || pool.type === 'v2-pool')).to.equal(true)
expect(r.every((pool) => pool.type !== 'v4-pool')).to.equal(true)
}
})

/// Tests for routes likely to result in MixedRoutes being returned
if (type === 'exactIn') {
it.skip(`erc20 -> erc20 forceMixedRoutes not specified for v2,v3 does not return mixed route even when it is better`, async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,23 @@ import { encodeSqrtRatioX96, FeeAmount, Pool as V3Pool } from '@uniswap/v3-sdk'
import { Pool as V4Pool } from '@uniswap/v4-sdk'
import { WNATIVE_ON } from '../../../../../utils/tokens'
import {
CacheMode,
CachedRoute,
CachedRoutes,
CacheMode,
MetricLoggerUnit,
MixedRoute,
nativeOnChain,
UNI_MAINNET,
USDC_MAINNET,
V3Route,
nativeOnChain,
MetricLoggerUnit,
MixedRoute,
} from '@uniswap/smart-order-router'
import { DynamoDBTableProps } from '../../../../../../bin/stacks/routing-database-stack'
import { V4Route } from '@uniswap/smart-order-router/build/main/routers'
import { NEW_CACHED_ROUTES_ROLLOUT_PERCENT } from '../../../../../../lib/util/newCachedRoutesRolloutPercent'
import sinon, { SinonSpy } from 'sinon'
import { metric } from '@uniswap/smart-order-router/build/main/util/metric'
import { DynamoDB } from 'aws-sdk'
import { DEFAULT_ROUTING_CONFIG_BY_CHAIN } from '../../../../../../lib/handlers/shared'

chai.use(chaiAsPromised)

Expand Down Expand Up @@ -392,11 +393,6 @@ describe('DynamoRouteCachingProvider', async () => {

it('Mixed cached routes persists protocolsInvolved', async () => {
const currencyAmount = CurrencyAmount.fromRawAmount(WETH, JSBI.BigInt(1 * 10 ** WETH.decimals))
const currencyAmountETH = CurrencyAmount.fromRawAmount(
nativeOnChain(ChainId.MAINNET),
JSBI.BigInt(1 * 10 ** nativeOnChain(ChainId.MAINNET).decimals)
)

const cacheMode = await dynamoRouteCache.getCacheMode(
ChainId.MAINNET,
currencyAmount,
Expand All @@ -406,7 +402,7 @@ describe('DynamoRouteCachingProvider', async () => {
)
expect(cacheMode).to.equal(CacheMode.Livemode)

const insertedIntoCacheMixed = await dynamoRouteCache.setCachedRoute(TEST_CACHED_MIXED_ROUTES, currencyAmountETH)
const insertedIntoCacheMixed = await dynamoRouteCache.setCachedRoute(TEST_CACHED_MIXED_ROUTES, currencyAmount)
expect(insertedIntoCacheMixed).to.be.true

const cacheModeFromCachedRoutes = await dynamoRouteCache.getCacheModeFromCachedRoutes(
Expand All @@ -427,19 +423,78 @@ describe('DynamoRouteCachingProvider', async () => {
currencyAmount,
UNI_MAINNET,
TradeType.EXACT_INPUT,
[Protocol.MIXED],
TEST_CACHED_MIXED_ROUTES.blockNumber
[Protocol.MIXED, Protocol.V3, Protocol.V4],
TEST_CACHED_MIXED_ROUTES.blockNumber,
false,
{
...DEFAULT_ROUTING_CONFIG_BY_CHAIN(ChainId.MAINNET),
enableMixedRouteWithUR1_2Percent: 100,
}
)
expect(route).to.not.be.undefined

const mixedRoutes = await dynamoRouteCache.getCachedRoute(
ChainId.MAINNET,
currencyAmountETH,
currencyAmount,
UNI_MAINNET,
TradeType.EXACT_INPUT,
[Protocol.MIXED],
TEST_CACHED_MIXED_ROUTES.blockNumber
// we fetch mixed route with the exact protocols version match (v3 + v4), expect to retrieve mixed route
[Protocol.MIXED, Protocol.V3, Protocol.V4],
TEST_CACHED_MIXED_ROUTES.blockNumber,
false,
{
...DEFAULT_ROUTING_CONFIG_BY_CHAIN(ChainId.MAINNET),
enableMixedRouteWithUR1_2Percent: 100,
}
)
expect(mixedRoutes).to.not.be.undefined

const mixedRouteWithExtraV2 = await dynamoRouteCache.getCachedRoute(
ChainId.MAINNET,
currencyAmount,
UNI_MAINNET,
TradeType.EXACT_INPUT,
// we fetch mixed route with extra v2, expect to retrieve mixed route
[Protocol.MIXED, Protocol.V2, Protocol.V3, Protocol.V4],
TEST_CACHED_MIXED_ROUTES.blockNumber,
false,
{
...DEFAULT_ROUTING_CONFIG_BY_CHAIN(ChainId.MAINNET),
enableMixedRouteWithUR1_2Percent: 100,
}
)
expect(mixedRouteWithExtraV2).to.not.be.undefined

const nonExistV3OnlyMixedRoutes = await dynamoRouteCache.getCachedRoute(
ChainId.MAINNET,
currencyAmount,
UNI_MAINNET,
TradeType.EXACT_INPUT,
// we fetch mixed route with only v3 missing v4, expect to retrieve nothing
[Protocol.MIXED, Protocol.V3],
TEST_CACHED_MIXED_ROUTES.blockNumber,
false,
{
...DEFAULT_ROUTING_CONFIG_BY_CHAIN(ChainId.MAINNET),
enableMixedRouteWithUR1_2Percent: 100,
}
)
expect(nonExistV3OnlyMixedRoutes).to.be.undefined

const nonExistV4OnlyMixedRoutes = await dynamoRouteCache.getCachedRoute(
ChainId.MAINNET,
currencyAmount,
UNI_MAINNET,
TradeType.EXACT_INPUT,
// we fetch mixed route with only v4 missing v3, expect to retrieve nothing
[Protocol.MIXED, Protocol.V4],
TEST_CACHED_MIXED_ROUTES.blockNumber,
false,
{
...DEFAULT_ROUTING_CONFIG_BY_CHAIN(ChainId.MAINNET),
enableMixedRouteWithUR1_2Percent: 100,
}
)
expect(nonExistV4OnlyMixedRoutes).to.be.undefined
})
})