From bd49410464fe8b76bc1d05a649201940dbaf5ddd Mon Sep 17 00:00:00 2001 From: jsy1218 <91580504+jsy1218@users.noreply.github.com> Date: Tue, 1 Apr 2025 16:09:43 -0700 Subject: [PATCH 01/14] feat: get cached routes to filter out mismatched mixed routes against requested protocol versions --- .../route-caching/dynamo-route-caching-provider.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts index b551af183..d7450c1b5 100644 --- a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts +++ b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts @@ -9,7 +9,7 @@ import { metric, MetricLoggerUnit, routeToString, - SupportedRoutes, + SupportedRoutes } from '@uniswap/smart-order-router' import { AWSError, DynamoDB, Lambda } from 'aws-sdk' import { ChainId, Currency, CurrencyAmount, Fraction, Token, TradeType } from '@uniswap/sdk-core' @@ -142,6 +142,12 @@ 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) => !record.protocolsInvolved + // requested protocols do not involve MIXED, so there is no need to filter by protocolsInvolved + || !protocols.includes(Protocol.MIXED) + // we know MIXED is getting requested, in this case, we need to ensure that protocolsInvolved contains at least one of the requested protocols + || protocols.includes(record.protocolsInvolved)) .sort((a, b) => b.blockNumber - a.blockNumber) .slice(0, this.ROUTES_TO_TAKE_FROM_ROUTES_DB) From df29b9aa3e220964d4144cd6ee40b7c411fe5556 Mon Sep 17 00:00:00 2001 From: jsy1218 <91580504+jsy1218@users.noreply.github.com> Date: Thu, 3 Apr 2025 19:07:13 -0700 Subject: [PATCH 02/14] fix: use ProtocolsInvolved to filter out mixed routes that doesn't have the matching protocols --- .../route-caching/dynamo-route-caching-provider.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts index d7450c1b5..4ee7091dd 100644 --- a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts +++ b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts @@ -147,7 +147,7 @@ export class DynamoRouteCachingProvider extends IRouteCachingProvider { // requested protocols do not involve MIXED, so there is no need to filter by protocolsInvolved || !protocols.includes(Protocol.MIXED) // we know MIXED is getting requested, in this case, we need to ensure that protocolsInvolved contains at least one of the requested protocols - || protocols.includes(record.protocolsInvolved)) + || protocols.includes(record.protocolsInvolved.split(','))) .sort((a, b) => b.blockNumber - a.blockNumber) .slice(0, this.ROUTES_TO_TAKE_FROM_ROUTES_DB) From 7d98fafb9aa8140509cb5efae75af738710fb45e Mon Sep 17 00:00:00 2001 From: jsy1218 <91580504+jsy1218@users.noreply.github.com> Date: Thu, 3 Apr 2025 19:09:15 -0700 Subject: [PATCH 03/14] fix prettier --- .../dynamo-route-caching-provider.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts index 4ee7091dd..a95304263 100644 --- a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts +++ b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts @@ -9,7 +9,7 @@ import { metric, MetricLoggerUnit, routeToString, - SupportedRoutes + SupportedRoutes, } from '@uniswap/smart-order-router' import { AWSError, DynamoDB, Lambda } from 'aws-sdk' import { ChainId, Currency, CurrencyAmount, Fraction, Token, TradeType } from '@uniswap/sdk-core' @@ -143,11 +143,14 @@ export class DynamoRouteCachingProvider extends IRouteCachingProvider { // 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) => !record.protocolsInvolved - // requested protocols do not involve MIXED, so there is no need to filter by protocolsInvolved - || !protocols.includes(Protocol.MIXED) - // we know MIXED is getting requested, in this case, we need to ensure that protocolsInvolved contains at least one of the requested protocols - || protocols.includes(record.protocolsInvolved.split(','))) + .filter( + (record) => + !record.protocolsInvolved || + // requested protocols do not involve MIXED, so there is no need to filter by protocolsInvolved + !protocols.includes(Protocol.MIXED) || + // we know MIXED is getting requested, in this case, we need to ensure that protocolsInvolved contains at least one of the requested protocols + protocols.includes(record.protocolsInvolved.split(',')) + ) .sort((a, b) => b.blockNumber - a.blockNumber) .slice(0, this.ROUTES_TO_TAKE_FROM_ROUTES_DB) From 174ecd42cb3b64bbe17cf74209e5124c4d65080b Mon Sep 17 00:00:00 2001 From: jsy1218 <91580504+jsy1218@users.noreply.github.com> Date: Fri, 4 Apr 2025 10:29:26 -0700 Subject: [PATCH 04/14] protocols need to include every protocolsInvolved --- .../route-caching/dynamo-route-caching-provider.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts index a95304263..af774cefb 100644 --- a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts +++ b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts @@ -149,7 +149,7 @@ export class DynamoRouteCachingProvider extends IRouteCachingProvider { // requested protocols do not involve MIXED, so there is no need to filter by protocolsInvolved !protocols.includes(Protocol.MIXED) || // we know MIXED is getting requested, in this case, we need to ensure that protocolsInvolved contains at least one of the requested protocols - protocols.includes(record.protocolsInvolved.split(',')) + protocols.every((protocol) => (record.protocolsInvolved as String).split(',').includes(protocol)) ) .sort((a, b) => b.blockNumber - a.blockNumber) .slice(0, this.ROUTES_TO_TAKE_FROM_ROUTES_DB) From bc9db3d9ed67d3b855f1ca477ded1fdac6966318 Mon Sep 17 00:00:00 2001 From: jsy1218 <91580504+jsy1218@users.noreply.github.com> Date: Fri, 4 Apr 2025 12:17:51 -0700 Subject: [PATCH 05/14] offline discussion, it should be protocolsInvolved 100% matching all requested protocols --- .../route-caching/dynamo-route-caching-provider.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts index af774cefb..1e4d95d86 100644 --- a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts +++ b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts @@ -149,7 +149,7 @@ export class DynamoRouteCachingProvider extends IRouteCachingProvider { // requested protocols do not involve MIXED, so there is no need to filter by protocolsInvolved !protocols.includes(Protocol.MIXED) || // we know MIXED is getting requested, in this case, we need to ensure that protocolsInvolved contains at least one of the requested protocols - protocols.every((protocol) => (record.protocolsInvolved as String).split(',').includes(protocol)) + (record.protocolsInvolved as String).split(',').every((protocol) => protocols.includes(protocol)) ) .sort((a, b) => b.blockNumber - a.blockNumber) .slice(0, this.ROUTES_TO_TAKE_FROM_ROUTES_DB) From 778210176c1c531e0ca2e2235f7e4875b594c51b Mon Sep 17 00:00:00 2001 From: jsy1218 <91580504+jsy1218@users.noreply.github.com> Date: Fri, 4 Apr 2025 14:01:40 -0700 Subject: [PATCH 06/14] fix enum string type mismatch --- .../route-caching/dynamo-route-caching-provider.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts index 1e4d95d86..62e7837ae 100644 --- a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts +++ b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts @@ -148,8 +148,10 @@ export class DynamoRouteCachingProvider extends IRouteCachingProvider { !record.protocolsInvolved || // requested protocols do not involve MIXED, so there is no need to filter by protocolsInvolved !protocols.includes(Protocol.MIXED) || - // we know MIXED is getting requested, in this case, we need to ensure that protocolsInvolved contains at least one of the requested protocols - (record.protocolsInvolved as String).split(',').every((protocol) => protocols.includes(protocol)) + // 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) => (Object.values(protocols) as string[]).includes(protocol)) ) .sort((a, b) => b.blockNumber - a.blockNumber) .slice(0, this.ROUTES_TO_TAKE_FROM_ROUTES_DB) From 594a5ef65c5370206eb6d79e35d6bacb5d4c9df1 Mon Sep 17 00:00:00 2001 From: jsy1218 <91580504+jsy1218@users.noreply.github.com> Date: Mon, 14 Apr 2025 14:00:55 -0700 Subject: [PATCH 07/14] add e2e test that can repro --- test/mocha/e2e/quote.test.ts | 41 ++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/test/mocha/e2e/quote.test.ts b/test/mocha/e2e/quote.test.ts index b77b79afc..862c57d64 100644 --- a/test/mocha/e2e/quote.test.ts +++ b/test/mocha/e2e/quote.test.ts @@ -1495,6 +1495,47 @@ describe('quote', function () { } }) + 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 = await axios.get( + `${API}?${queryParams}`, + { headers: HEADERS_2_0 } + ) + + 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) + } + }) + /// 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 () => { From e2ece1086c15ddfdcd9ec74335d18f695528ac09 Mon Sep 17 00:00:00 2001 From: jsy1218 <91580504+jsy1218@users.noreply.github.com> Date: Mon, 14 Apr 2025 14:11:04 -0700 Subject: [PATCH 08/14] add e2e test to repro --- test/mocha/e2e/quote.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/mocha/e2e/quote.test.ts b/test/mocha/e2e/quote.test.ts index 862c57d64..72d580205 100644 --- a/test/mocha/e2e/quote.test.ts +++ b/test/mocha/e2e/quote.test.ts @@ -1495,6 +1495,8 @@ 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 @@ -1520,7 +1522,7 @@ describe('quote', function () { const response: AxiosResponse = await axios.get( `${API}?${queryParams}`, - { headers: HEADERS_2_0 } + { headers: HEADERS_1_2 } ) const { @@ -1533,6 +1535,7 @@ describe('quote', function () { // 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) } }) From 40fc15290b7de564d90e7acc4f22e182545d08e7 Mon Sep 17 00:00:00 2001 From: jsy1218 <91580504+jsy1218@users.noreply.github.com> Date: Tue, 15 Apr 2025 14:28:23 -0700 Subject: [PATCH 09/14] add integ test coverage --- .../dynamo-route-caching-provider.ts | 26 +++--- package-lock.json | 14 +-- package.json | 2 +- test/mocha/e2e/quote.test.ts | 13 ++- .../dynamo-route-caching-provider.test.ts | 85 +++++++++++++++---- 5 files changed, 100 insertions(+), 40 deletions(-) diff --git a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts index 62e7837ae..704fb94b1 100644 --- a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts +++ b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts @@ -1,4 +1,5 @@ import { + AlphaRouterConfig, CachedRoute, CachedRoutes, CacheMode, @@ -97,14 +98,15 @@ export class DynamoRouteCachingProvider extends IRouteCachingProvider { * @param protocols * @protected */ - protected async _getCachedRoute( + protected override async _getCachedRoute( chainId: ChainId, amount: CurrencyAmount, quoteCurrency: Currency, tradeType: TradeType, protocols: Protocol[], currentBlockNumber: number, - optimistic: boolean + optimistic: boolean, + alphaRouterConfig?: AlphaRouterConfig ): Promise { const { currencyIn, currencyOut } = this.determineCurrencyInOut(amount, quoteCurrency, tradeType) @@ -143,16 +145,20 @@ export class DynamoRouteCachingProvider extends IRouteCachingProvider { // 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) => + .filter((record) => { + return ( !record.protocolsInvolved || - // requested protocols do not involve MIXED, so there is no need to filter by protocolsInvolved !protocols.includes(Protocol.MIXED) || - // 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) => (Object.values(protocols) as string[]).includes(protocol)) - ) + // 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 && + // 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) diff --git a/package-lock.json b/package-lock.json index 680bb2572..a545a6e05 100644 --- a/package-lock.json +++ b/package-lock.json @@ -29,7 +29,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", @@ -4508,9 +4508,9 @@ } }, "node_modules/@uniswap/smart-order-router": { - "version": "4.20.10", - "resolved": "https://registry.npmjs.org/@uniswap/smart-order-router/-/smart-order-router-4.20.10.tgz", - "integrity": "sha512-0W9Y5vREx8C3TMjov3+1x91AqVopnkNGcOWrxxDaHWhn6Tumk7PFTm/Y3rdWp4Az07P2IzVrDLa+LZn81HOeEw==", + "version": "4.20.11", + "resolved": "https://registry.npmjs.org/@uniswap/smart-order-router/-/smart-order-router-4.20.11.tgz", + "integrity": "sha512-W4MFkI6PZ+sbRx5RnmN/kQzir5hNvmj8z2zDRkRXEccY2UBTUzorDJDW8gjSFD3ym+yi2se5azXt1n43aaqs+A==", "dependencies": { "@eth-optimism/sdk": "^3.2.2", "@types/brotli": "^1.3.4", @@ -27936,9 +27936,9 @@ } }, "@uniswap/smart-order-router": { - "version": "4.20.10", - "resolved": "https://registry.npmjs.org/@uniswap/smart-order-router/-/smart-order-router-4.20.10.tgz", - "integrity": "sha512-0W9Y5vREx8C3TMjov3+1x91AqVopnkNGcOWrxxDaHWhn6Tumk7PFTm/Y3rdWp4Az07P2IzVrDLa+LZn81HOeEw==", + "version": "4.20.11", + "resolved": "https://registry.npmjs.org/@uniswap/smart-order-router/-/smart-order-router-4.20.11.tgz", + "integrity": "sha512-W4MFkI6PZ+sbRx5RnmN/kQzir5hNvmj8z2zDRkRXEccY2UBTUzorDJDW8gjSFD3ym+yi2se5azXt1n43aaqs+A==", "requires": { "@eth-optimism/sdk": "^3.2.2", "@types/brotli": "^1.3.4", diff --git a/package.json b/package.json index 45d8c3649..32b770b05 100644 --- a/package.json +++ b/package.json @@ -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", diff --git a/test/mocha/e2e/quote.test.ts b/test/mocha/e2e/quote.test.ts index 72d580205..a9861db1e 100644 --- a/test/mocha/e2e/quote.test.ts +++ b/test/mocha/e2e/quote.test.ts @@ -1497,7 +1497,7 @@ 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() => { + it(`erc20 -> erc20 cached routes should not return mixed route`, async () => { if (type !== 'exactIn') { return } @@ -1520,10 +1520,9 @@ describe('quote', function () { const queryParams = qs.stringify(quoteReq) - const response: AxiosResponse = await axios.get( - `${API}?${queryParams}`, - { headers: HEADERS_1_2 } - ) + const response: AxiosResponse = await axios.get(`${API}?${queryParams}`, { + headers: HEADERS_1_2, + }) const { data: { route }, @@ -1534,8 +1533,8 @@ describe('quote', function () { // 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) + 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) } }) diff --git a/test/mocha/integ/handlers/router-entities/route-caching/dynamo-route-caching-provider.test.ts b/test/mocha/integ/handlers/router-entities/route-caching/dynamo-route-caching-provider.test.ts index d02091426..55d7f8ed6 100644 --- a/test/mocha/integ/handlers/router-entities/route-caching/dynamo-route-caching-provider.test.ts +++ b/test/mocha/integ/handlers/router-entities/route-caching/dynamo-route-caching-provider.test.ts @@ -13,15 +13,15 @@ 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' @@ -29,6 +29,7 @@ import { NEW_CACHED_ROUTES_ROLLOUT_PERCENT } from '../../../../../../lib/util/ne 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) @@ -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, @@ -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( @@ -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 }) }) From 22c1100357cd1d21344258c9dbef043ec7e71877 Mon Sep 17 00:00:00 2001 From: jsy1218 <91580504+jsy1218@users.noreply.github.com> Date: Tue, 15 Apr 2025 14:45:09 -0700 Subject: [PATCH 10/14] enable fix at 1% --- lib/handlers/quote/quote.ts | 1 + .../dynamo-route-caching-provider.ts | 17 +++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/lib/handlers/quote/quote.ts b/lib/handlers/quote/quote.ts index e8acf5436..a70f0ccb4 100644 --- a/lib/handlers/quote/quote.ts +++ b/lib/handlers/quote/quote.ts @@ -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) diff --git a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts index 704fb94b1..10a3c7438 100644 --- a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts +++ b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts @@ -162,6 +162,23 @@ export class DynamoRouteCachingProvider extends IRouteCachingProvider { .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 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) From 4bb87134a73dd9b4c777051b2f48ee4a4aca53c9 Mon Sep 17 00:00:00 2001 From: jsy1218 <91580504+jsy1218@users.noreply.github.com> Date: Tue, 15 Apr 2025 14:46:18 -0700 Subject: [PATCH 11/14] null protection --- .../route-caching/dynamo-route-caching-provider.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts index 10a3c7438..726fc2381 100644 --- a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts +++ b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts @@ -165,6 +165,7 @@ export class DynamoRouteCachingProvider extends IRouteCachingProvider { if (protocols.includes(Protocol.MIXED)) { filteredItems.forEach((record) => { if ( + record.protocolsInvolved && !(record.protocolsInvolved as String) .split(',') .every((protocol) => protocols.includes(protocol as Protocol)) From ac6ca4dabaae31e13ca76b161ef2f327fa3cf5e4 Mon Sep 17 00:00:00 2001 From: jsy1218 <91580504+jsy1218@users.noreply.github.com> Date: Tue, 15 Apr 2025 14:47:35 -0700 Subject: [PATCH 12/14] update JDOC --- .../route-caching/dynamo-route-caching-provider.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts index 726fc2381..c9905323e 100644 --- a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts +++ b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts @@ -96,6 +96,9 @@ export class DynamoRouteCachingProvider extends IRouteCachingProvider { * @param quoteCurrency * @param tradeType * @param protocols + * @param currentBlockNumber + * @param optimistic + * @param alphaRouterConfig * @protected */ protected override async _getCachedRoute( From b45a63643aa9681efd945d018c13b234bc07afb6 Mon Sep 17 00:00:00 2001 From: jsy1218 <91580504+jsy1218@users.noreply.github.com> Date: Tue, 15 Apr 2025 17:36:06 -0700 Subject: [PATCH 13/14] feedback --- lib/handlers/quote/quote.ts | 2 +- .../route-caching/dynamo-route-caching-provider.ts | 6 +++++- package-lock.json | 14 +++++++------- package.json | 2 +- 4 files changed, 14 insertions(+), 10 deletions(-) diff --git a/lib/handlers/quote/quote.ts b/lib/handlers/quote/quote.ts index a70f0ccb4..0c0abf294 100644 --- a/lib/handlers/quote/quote.ts +++ b/lib/handlers/quote/quote.ts @@ -353,7 +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. + enableMixedRouteWithUR1_2: 1 >= Math.random() * 100, // 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) diff --git a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts index c9905323e..48131ba8f 100644 --- a/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts +++ b/lib/handlers/router-entities/route-caching/dynamo-route-caching-provider.ts @@ -152,9 +152,13 @@ export class DynamoRouteCachingProvider extends IRouteCachingProvider { return ( !record.protocolsInvolved || !protocols.includes(Protocol.MIXED) || + // if UR header is v2.0, then it does not make sense to have mixed route filter by protocols + // only when UR header is v1.2, we explicitly delete the V4 from protocols in + // https://github.com/Uniswap/smart-order-router/blob/main/src/routers/alpha-router/alpha-router.ts#L1461 + alphaRouterConfig?.universalRouterVersion === UniversalRouterVersion.V2_0 || // 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 && + (alphaRouterConfig?.enableMixedRouteWithUR1_2 && // 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) diff --git a/package-lock.json b/package-lock.json index a545a6e05..c4bc87d9a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -29,7 +29,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.11", + "@uniswap/smart-order-router": "4.20.12", "@uniswap/token-lists": "^1.0.0-beta.33", "@uniswap/universal-router-sdk": "^4.19.5", "@uniswap/v2-sdk": "^4.15.2", @@ -4508,9 +4508,9 @@ } }, "node_modules/@uniswap/smart-order-router": { - "version": "4.20.11", - "resolved": "https://registry.npmjs.org/@uniswap/smart-order-router/-/smart-order-router-4.20.11.tgz", - "integrity": "sha512-W4MFkI6PZ+sbRx5RnmN/kQzir5hNvmj8z2zDRkRXEccY2UBTUzorDJDW8gjSFD3ym+yi2se5azXt1n43aaqs+A==", + "version": "4.20.12", + "resolved": "https://registry.npmjs.org/@uniswap/smart-order-router/-/smart-order-router-4.20.12.tgz", + "integrity": "sha512-ygNByiB94LIcUkWsM+RDhZAC6++MHMz6MTBh3SGR/QUheLSaCaFZJvX0xTspUPycpz78trHy1SfjXs7HNyNvFA==", "dependencies": { "@eth-optimism/sdk": "^3.2.2", "@types/brotli": "^1.3.4", @@ -27936,9 +27936,9 @@ } }, "@uniswap/smart-order-router": { - "version": "4.20.11", - "resolved": "https://registry.npmjs.org/@uniswap/smart-order-router/-/smart-order-router-4.20.11.tgz", - "integrity": "sha512-W4MFkI6PZ+sbRx5RnmN/kQzir5hNvmj8z2zDRkRXEccY2UBTUzorDJDW8gjSFD3ym+yi2se5azXt1n43aaqs+A==", + "version": "4.20.12", + "resolved": "https://registry.npmjs.org/@uniswap/smart-order-router/-/smart-order-router-4.20.12.tgz", + "integrity": "sha512-ygNByiB94LIcUkWsM+RDhZAC6++MHMz6MTBh3SGR/QUheLSaCaFZJvX0xTspUPycpz78trHy1SfjXs7HNyNvFA==", "requires": { "@eth-optimism/sdk": "^3.2.2", "@types/brotli": "^1.3.4", diff --git a/package.json b/package.json index 32b770b05..056cd43ba 100644 --- a/package.json +++ b/package.json @@ -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.11", + "@uniswap/smart-order-router": "4.20.12", "@uniswap/token-lists": "^1.0.0-beta.33", "@uniswap/universal-router-sdk": "^4.19.5", "@uniswap/v2-sdk": "^4.15.2", From f51f54e3c58a202137042b55cc1201b7be1faaa4 Mon Sep 17 00:00:00 2001 From: jsy1218 <91580504+jsy1218@users.noreply.github.com> Date: Tue, 15 Apr 2025 17:41:08 -0700 Subject: [PATCH 14/14] fix integ test --- .../dynamo-route-caching-provider.test.ts | 45 ++++++++++++++++--- 1 file changed, 40 insertions(+), 5 deletions(-) diff --git a/test/mocha/integ/handlers/router-entities/route-caching/dynamo-route-caching-provider.test.ts b/test/mocha/integ/handlers/router-entities/route-caching/dynamo-route-caching-provider.test.ts index 55d7f8ed6..99dd946df 100644 --- a/test/mocha/integ/handlers/router-entities/route-caching/dynamo-route-caching-provider.test.ts +++ b/test/mocha/integ/handlers/router-entities/route-caching/dynamo-route-caching-provider.test.ts @@ -30,6 +30,7 @@ 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' +import { UniversalRouterVersion } from '@uniswap/universal-router-sdk' chai.use(chaiAsPromised) @@ -428,7 +429,7 @@ describe('DynamoRouteCachingProvider', async () => { false, { ...DEFAULT_ROUTING_CONFIG_BY_CHAIN(ChainId.MAINNET), - enableMixedRouteWithUR1_2Percent: 100, + enableMixedRouteWithUR1_2: true, } ) expect(route).to.not.be.undefined @@ -444,7 +445,7 @@ describe('DynamoRouteCachingProvider', async () => { false, { ...DEFAULT_ROUTING_CONFIG_BY_CHAIN(ChainId.MAINNET), - enableMixedRouteWithUR1_2Percent: 100, + enableMixedRouteWithUR1_2: true, } ) expect(mixedRoutes).to.not.be.undefined @@ -460,7 +461,7 @@ describe('DynamoRouteCachingProvider', async () => { false, { ...DEFAULT_ROUTING_CONFIG_BY_CHAIN(ChainId.MAINNET), - enableMixedRouteWithUR1_2Percent: 100, + enableMixedRouteWithUR1_2: true, } ) expect(mixedRouteWithExtraV2).to.not.be.undefined @@ -476,11 +477,28 @@ describe('DynamoRouteCachingProvider', async () => { false, { ...DEFAULT_ROUTING_CONFIG_BY_CHAIN(ChainId.MAINNET), - enableMixedRouteWithUR1_2Percent: 100, + enableMixedRouteWithUR1_2: true, } ) expect(nonExistV3OnlyMixedRoutes).to.be.undefined + const nonExistV3OnlyMixedRoutesUR2_0 = 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), + universalRouterVersion: UniversalRouterVersion.V2_0, + enableMixedRouteWithUR1_2: true, + } + ) + expect(nonExistV3OnlyMixedRoutesUR2_0).not.to.be.undefined + const nonExistV4OnlyMixedRoutes = await dynamoRouteCache.getCachedRoute( ChainId.MAINNET, currencyAmount, @@ -492,9 +510,26 @@ describe('DynamoRouteCachingProvider', async () => { false, { ...DEFAULT_ROUTING_CONFIG_BY_CHAIN(ChainId.MAINNET), - enableMixedRouteWithUR1_2Percent: 100, + enableMixedRouteWithUR1_2: true, } ) expect(nonExistV4OnlyMixedRoutes).to.be.undefined + + const nonExistV4OnlyMixedRoutesUR2_0 = 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), + universalRouterVersion: UniversalRouterVersion.V2_0, + enableMixedRouteWithUR1_2: true, + } + ) + expect(nonExistV4OnlyMixedRoutesUR2_0).not.to.be.undefined }) })