diff --git a/package-lock.json b/package-lock.json index cc4f86c02..5187d2b8d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@uniswap/smart-order-router", - "version": "4.7.4", + "version": "4.7.5", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@uniswap/smart-order-router", - "version": "4.7.4", + "version": "4.7.5", "license": "GPL", "dependencies": { "@eth-optimism/sdk": "^3.2.2", diff --git a/package.json b/package.json index de0f6b1c4..8a3e878d8 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@uniswap/smart-order-router", - "version": "4.7.4", + "version": "4.7.5", "description": "Uniswap Smart Order Router", "main": "build/main/index.js", "typings": "build/main/index.d.ts", diff --git a/src/routers/alpha-router/alpha-router.ts b/src/routers/alpha-router/alpha-router.ts index b798c62be..c433c599d 100644 --- a/src/routers/alpha-router/alpha-router.ts +++ b/src/routers/alpha-router/alpha-router.ts @@ -10,7 +10,6 @@ import { TradeType, } from '@uniswap/sdk-core'; import { TokenList } from '@uniswap/token-lists'; -import { UniversalRouterVersion } from '@uniswap/universal-router-sdk'; import { Pool, Position, SqrtPriceMath, TickMath } from '@uniswap/v3-sdk'; import retry from 'async-retry'; import JSBI from 'jsbi'; @@ -149,6 +148,8 @@ import { V4Route, } from '../router'; +import { UniversalRouterVersion } from '@uniswap/universal-router-sdk'; +import { INTENT } from '../../util/intent'; import { DEFAULT_ROUTING_CONFIG_BY_CHAIN, ETH_GAS_STATION_API_URL, @@ -324,16 +325,17 @@ export type AlphaRouterParams = { */ mixedSupported?: ChainId[]; - /** - * The version of the universal router to use. - */ - universalRouterVersion?: UniversalRouterVersion; - /** * The v4 pool params to be used for the v4 pool provider. * fee tiers, tickspacings, and hook addresses */ v4PoolParams?: Array<[number, number, string]>; + + /** + * We want to rollout the cached routes cache invalidation carefully. + * Because a wrong fix might impact prod success rate and/or latency. + */ + cachedRoutesCacheInvalidationFixRolloutPercentage?: number; }; export class MapWithLowerCaseKey extends Map { @@ -511,6 +513,10 @@ export type AlphaRouterConfig = { * The version of the universal router to use. */ universalRouterVersion?: UniversalRouterVersion; + /** + * pass in routing-api intent to align the intent between routing-api and SOR + */ + intent?: INTENT; }; export class AlphaRouter @@ -550,8 +556,8 @@ export class AlphaRouter protected v2Supported?: ChainId[]; protected v4Supported?: ChainId[]; protected mixedSupported?: ChainId[]; - protected universalRouterVersion?: UniversalRouterVersion; protected v4PoolParams?: Array<[number, number, string]>; + protected cachedRoutesCacheInvalidationFixRolloutPercentage?: number; constructor({ chainId, @@ -582,8 +588,8 @@ export class AlphaRouter v2Supported, v4Supported, mixedSupported, - universalRouterVersion, v4PoolParams, + cachedRoutesCacheInvalidationFixRolloutPercentage, }: AlphaRouterParams) { this.chainId = chainId; this.provider = provider; @@ -1047,8 +1053,9 @@ export class AlphaRouter this.v2Supported = v2Supported ?? V2_SUPPORTED; this.v4Supported = v4Supported ?? V4_SUPPORTED; this.mixedSupported = mixedSupported ?? MIXED_SUPPORTED; - this.universalRouterVersion = - universalRouterVersion ?? UniversalRouterVersion.V1_2; + + this.cachedRoutesCacheInvalidationFixRolloutPercentage = + cachedRoutesCacheInvalidationFixRolloutPercentage; } public async routeToRatio( @@ -1645,6 +1652,77 @@ export class AlphaRouter } } + let newSetCachedRoutesPath = false; + const shouldEnableCachedRoutesCacheInvalidationFix = + Math.random() * 100 < + (this.cachedRoutesCacheInvalidationFixRolloutPercentage ?? 0); + + // we have to write cached routes right before checking swapRouteRaw is null or not + // because getCachedRoutes in routing-api do not use the blocks-to-live to filter out the expired routes at all + // there's a possibility the cachedRoutes is always populated, but swapRouteFromCache is always null, because we don't update cachedRoutes in this case at all, + // as long as it's within 24 hours sliding window TTL + if (shouldEnableCachedRoutesCacheInvalidationFix) { + // theoretically, when routingConfig.intent === INTENT.CACHING, optimisticCachedRoutes should be false + // so that we can always pass in cachedRoutes?.notExpired(await blockNumber, !routingConfig.optimisticCachedRoutes) + // but just to be safe, we just hardcode true when checking the cached routes expiry for write update + // we decide to not check cached routes expiry in the read path anyway + if (!cachedRoutes?.notExpired(await blockNumber, true)) { + // optimisticCachedRoutes === false means at routing-api level, we only want to set cached routes during intent=caching, not intent=quote + // this means during the online quote endpoint path, we should not reset cached routes + if (routingConfig.intent === INTENT.CACHING) { + // due to fire and forget nature, we already take note that we should set new cached routes during the new path + newSetCachedRoutesPath = true; + metric.putMetric(`SetCachedRoute_NewPath`, 1, MetricLoggerUnit.Count); + + // there's a chance that swapRouteFromChain might be populated already, + // when there's no cachedroutes in the dynamo DB. + // in that case, we don't try to swap route from chain again + const swapRouteFromChainAgain = + swapRouteFromChain ?? + // we have to intentionally await here, because routing-api lambda has a chance to return the swapRoute/swapRouteWithSimulation + // before the routing-api quote handler can finish running getSwapRouteFromChain (getSwapRouteFromChain is runtime intensive) + (await this.getSwapRouteFromChain( + amount, + currencyIn, + currencyOut, + protocols, + quoteCurrency, + tradeType, + routingConfig, + v3GasModel, + v4GasModel, + mixedRouteGasModel, + gasPriceWei, + v2GasModel, + swapConfig, + providerConfig + )); + + if (swapRouteFromChainAgain) { + const routesToCache = CachedRoutes.fromRoutesWithValidQuotes( + swapRouteFromChainAgain.routes, + this.chainId, + currencyIn, + currencyOut, + protocols.sort(), + await blockNumber, + tradeType, + amount.toExact() + ); + + this.setCachedRoutesAndLog( + amount, + currencyIn, + currencyOut, + tradeType, + 'SetCachedRoute_NewPath', + routesToCache + ); + } + } + } + } + if (!swapRouteRaw) { return null; } @@ -1659,12 +1737,29 @@ export class AlphaRouter estimatedGasUsedGasToken, } = swapRouteRaw; + // we intentionally dont add shouldEnableCachedRoutesCacheInvalidationFix in if condition below + // because we know cached routes in prod dont filter by blocks-to-live + // so that we know that swapRouteFromChain is always not populated, because + // if (!cachedRoutes || cacheMode !== CacheMode.Livemode) above always have the cachedRoutes as populated if ( this.routeCachingProvider && routingConfig.writeToCachedRoutes && cacheMode !== CacheMode.Darkmode && swapRouteFromChain ) { + if (newSetCachedRoutesPath) { + // SetCachedRoute_NewPath and SetCachedRoute_OldPath metrics might have counts during short timeframe. + // over time, we should expect to see less SetCachedRoute_OldPath metrics count. + // in AWS metrics, one can investigate, by: + // 1) seeing the overall metrics count of SetCachedRoute_NewPath and SetCachedRoute_OldPath. SetCachedRoute_NewPath should steadily go up, while SetCachedRoute_OldPath should go down. + // 2) using the same requestId, one should see eventually when SetCachedRoute_NewPath metric is logged, SetCachedRoute_OldPath metric should not be called. + metric.putMetric( + `SetCachedRoute_OldPath_INTENT_${routingConfig.intent}`, + 1, + MetricLoggerUnit.Count + ); + } + // Generate the object to be cached const routesToCache = CachedRoutes.fromRoutesWithValidQuotes( swapRouteFromChain.routes, @@ -1677,45 +1772,14 @@ export class AlphaRouter amount.toExact() ); - if (routesToCache) { - // Attempt to insert the entry in cache. This is fire and forget promise. - // The catch method will prevent any exception from blocking the normal code execution. - this.routeCachingProvider - .setCachedRoute(routesToCache, amount) - .then((success) => { - const status = success ? 'success' : 'rejected'; - metric.putMetric( - `SetCachedRoute_${status}`, - 1, - MetricLoggerUnit.Count - ); - }) - .catch((reason) => { - log.error( - { - reason: reason, - tokenPair: this.tokenPairSymbolTradeTypeChainId( - currencyIn, - currencyOut, - tradeType - ), - }, - `SetCachedRoute failure` - ); - - metric.putMetric( - `SetCachedRoute_failure`, - 1, - MetricLoggerUnit.Count - ); - }); - } else { - metric.putMetric( - `SetCachedRoute_unnecessary`, - 1, - MetricLoggerUnit.Count - ); - } + this.setCachedRoutesAndLog( + amount, + currencyIn, + currencyOut, + tradeType, + 'SetCachedRoute_OldPath', + routesToCache + ); } metric.putMetric( @@ -1838,6 +1902,55 @@ export class AlphaRouter return swapRoute; } + private async setCachedRoutesAndLog( + amount: CurrencyAmount, + currencyIn: Currency, + currencyOut: Currency, + tradeType: TradeType, + metricsPrefix: string, + routesToCache?: CachedRoutes + ): Promise { + if (routesToCache) { + // Attempt to insert the entry in cache. This is fire and forget promise. + // The catch method will prevent any exception from blocking the normal code execution. + this.routeCachingProvider + ?.setCachedRoute(routesToCache, amount) + .then((success) => { + const status = success ? 'success' : 'rejected'; + metric.putMetric( + `${metricsPrefix}_${status}`, + 1, + MetricLoggerUnit.Count + ); + }) + .catch((reason) => { + log.error( + { + reason: reason, + tokenPair: this.tokenPairSymbolTradeTypeChainId( + currencyIn, + currencyOut, + tradeType + ), + }, + `SetCachedRoute failure` + ); + + metric.putMetric( + `${metricsPrefix}_failure`, + 1, + MetricLoggerUnit.Count + ); + }); + } else { + metric.putMetric( + `${metricsPrefix}_unnecessary`, + 1, + MetricLoggerUnit.Count + ); + } + } + private async getSwapRouteFromCache( currencyIn: Currency, currencyOut: Currency, diff --git a/src/util/index.ts b/src/util/index.ts index b6abf44da..414cafc06 100644 --- a/src/util/index.ts +++ b/src/util/index.ts @@ -1,6 +1,7 @@ export * from './addresses'; export * from './amounts'; export * from './chains'; +export * from './intent'; export * from './log'; export * from './metric'; export * from './protocols'; diff --git a/src/util/intent.ts b/src/util/intent.ts new file mode 100644 index 000000000..d9932e036 --- /dev/null +++ b/src/util/intent.ts @@ -0,0 +1,8 @@ +// NOTE: intent is a routing-api concept, +// but we have to introduce this strongly-typed enum in SOR to ensure some codepath only gets executed during async path +export enum INTENT { + CACHING = 'caching', + QUOTE = 'quote', + SWAP = 'swap', + PRICING = 'pricing', +} diff --git a/test/integ/routers/alpha-router/alpha-router.integration.test.ts b/test/integ/routers/alpha-router/alpha-router.integration.test.ts index c437bc4ee..1f8177b8e 100644 --- a/test/integ/routers/alpha-router/alpha-router.integration.test.ts +++ b/test/integ/routers/alpha-router/alpha-router.integration.test.ts @@ -98,7 +98,8 @@ import { WETH9, WLD_WORLDCHAIN, WNATIVE_ON, - WRAPPED_NATIVE_CURRENCY + WRAPPED_NATIVE_CURRENCY, + CacheMode } from '../../../../src'; import { PortionProvider } from '../../../../src/providers/portion-provider'; import { @@ -121,6 +122,12 @@ import { } from '../../../test-util/mock-data'; import { WHALES } from '../../../test-util/whales'; import { V4SubgraphProvider } from '../../../../build/main'; +import { + InMemoryRouteCachingProvider +} from '../../../unit/providers/caching/route/test-util/inmemory-route-caching-provider'; +import { + getInvalidCachedRoutesStub +} from '../../../unit/routers/alpha-router/gas-models/test-util/mocked-dependencies'; const FORK_BLOCK = 20444945; const UNIVERSAL_ROUTER_ADDRESS_V1_2 = UNIVERSAL_ROUTER_ADDRESS_BY_CHAIN(UniversalRouterVersion.V1_2, 1); @@ -770,6 +777,7 @@ describe('alpha router integration', () => { v3PoolProvider, v4PoolProvider, simulator: ethEstimateGasSimulator, + cachedRoutesCacheInvalidationFixRolloutPercentage: 100, }); feeOnTransferAlphaRouter = new AlphaRouter({ @@ -779,6 +787,7 @@ describe('alpha router integration', () => { v2PoolProvider: cachingV2PoolProvider, v3PoolProvider, simulator, + cachedRoutesCacheInvalidationFixRolloutPercentage: 100, }); }); @@ -3113,6 +3122,7 @@ describe('alpha router integration', () => { provider: hardhat.providers[0]!, multicall2Provider, gasPriceProvider, + cachedRoutesCacheInvalidationFixRolloutPercentage: 100, }); const swap = await customAlphaRouter.route( @@ -3133,6 +3143,71 @@ describe('alpha router integration', () => { await validateSwapRoute(quote, quoteGasAdjusted, tradeType, 100, 10); }); + + // This test is easy to set up at SOR level, but hard at routing-api level + // because at routing-api level, its difficult to trigger a quote to ensure an invalid cached routes get persisted into DB + // then we want to repro the invalid cached routes at SOR level + it(`erc20 -> erc20 cached routes cache invalidation`, async () => { + // invalid cached routes test setup is only set for exact-in in getInvalidCachedRoutesStub + if (tradeType !== TradeType.EXACT_INPUT) { + return + } + + const tokenIn = USDC_MAINNET; + const tokenOut = DAI_MAINNET; + const amount = parseAmount('1.1', tokenIn); + const routeCachingProvider = new InMemoryRouteCachingProvider(); + routeCachingProvider.cacheMode = CacheMode.Livemode; + routeCachingProvider.blocksToLive = Number.MAX_VALUE; + routeCachingProvider.expired = false; + + // Insert a invalid cached routes for this test purpose to make sure it can get cleaned up + await routeCachingProvider.setCachedRoute(getInvalidCachedRoutesStub(FORK_BLOCK)!, CurrencyAmount.fromRawAmount(USDC_MAINNET, 100)); + + // Create a new AlphaRouter + const customAlphaRouter: AlphaRouter = new AlphaRouter({ + chainId: 1, + provider: hardhat.providers[0]!, + multicall2Provider, + routeCachingProvider, + cachedRoutesCacheInvalidationFixRolloutPercentage: 100 + }); + + routeCachingProvider.expired = true; + + let swap = await customAlphaRouter.route( + amount, + getQuoteToken(tokenIn, tokenOut, tradeType), + tradeType, + undefined, + { + ...ROUTING_CONFIG, + protocols: [Protocol.V2, Protocol.V3, Protocol.MIXED], + optimisticCachedRoutes: false // to trigger cache invalidation during intent=caching + }, + ); + // first time the swap is gonna be null, because even with the new set cached routes codepath, + // i intentionally made it a fire and forget, i.e. non-blocking on the aloha-router request processing path + // first time swap from cache from the request processing path will be null + expect(swap).toBeNull(); // first time expect swapRouteFromCache to be null + + swap = await customAlphaRouter.route( + amount, + getQuoteToken(tokenIn, tokenOut, tradeType), + tradeType, + undefined, + { + ...ROUTING_CONFIG, + optimisticCachedRoutes: false // to make sure the updated in-memory cache contains the valid cached routes now + }, + ); + + // second time we will ensure the quote exists + // because the first time we had the sets cached routes in the new code path + // with second time we will have the cache hit from the new codepath cached routes + expect(swap).toBeDefined(); + expect(swap).not.toBeNull(); + }); }); } @@ -3607,6 +3682,7 @@ describe('quote for other networks', () => { [3000, 60, '0x0000000000000000000000000000000000000020'], ] ), + cachedRoutesCacheInvalidationFixRolloutPercentage: 100, }); } else { alphaRouter = new AlphaRouter({ @@ -3621,6 +3697,7 @@ describe('quote for other networks', () => { [3000, 60, '0x0000000000000000000000000000000000000020'], ] ), + cachedRoutesCacheInvalidationFixRolloutPercentage: 100, }); } }); diff --git a/test/test-util/mock-data.ts b/test/test-util/mock-data.ts index 183207473..cca893e09 100644 --- a/test/test-util/mock-data.ts +++ b/test/test-util/mock-data.ts @@ -349,6 +349,22 @@ export const UNI_WETH_MEDIUM = new V3Pool( 0 ); +export const NONTOKEN = new Token( + ChainId.MAINNET, + ADDRESS_ZERO, + 18, + 'NONTOKEN', + 'Non Token' +) +export const WETH_NONTOKEN_MEDIUM = new V3Pool( + WRAPPED_NATIVE_CURRENCY[1]!, + NONTOKEN, + FeeAmount.MEDIUM, + encodeSqrtRatioX96(1, 1), + 500, + 0 +); + // Mock V2 Pools export const DAI_USDT = new Pair( CurrencyAmount.fromRawAmount(DAI, 10000000000), diff --git a/test/unit/providers/caching/route/test-util/inmemory-route-caching-provider.ts b/test/unit/providers/caching/route/test-util/inmemory-route-caching-provider.ts index ca469c772..ea4ace494 100644 --- a/test/unit/providers/caching/route/test-util/inmemory-route-caching-provider.ts +++ b/test/unit/providers/caching/route/test-util/inmemory-route-caching-provider.ts @@ -5,6 +5,7 @@ import { CachedRoutes, CacheMode, IRouteCachingProvider } from '../../../../../. export class InMemoryRouteCachingProvider extends IRouteCachingProvider { public routesCache: Map = new Map(); public blocksToLive: number = 1; + public expired?: boolean = undefined; public cacheMode: CacheMode = CacheMode.Darkmode; public forceFail: boolean = false; public internalGetCacheRouteCalls: number = 0; @@ -15,6 +16,14 @@ export class InMemoryRouteCachingProvider extends IRouteCachingProvider { return this.blocksToLive; } + protected override filterExpiredCachedRoutes( + cachedRoutes: CachedRoutes | undefined, + blockNumber: number, + optimistic: boolean + ): CachedRoutes | undefined { + return this.expired === undefined ? super.filterExpiredCachedRoutes(cachedRoutes, blockNumber, optimistic) : cachedRoutes; + } + protected override async _getCachedRoute( chainId: ChainId, amount: CurrencyAmount, diff --git a/test/unit/routers/alpha-router/gas-models/test-util/mocked-dependencies.ts b/test/unit/routers/alpha-router/gas-models/test-util/mocked-dependencies.ts index 7157ca899..8496026d2 100644 --- a/test/unit/routers/alpha-router/gas-models/test-util/mocked-dependencies.ts +++ b/test/unit/routers/alpha-router/gas-models/test-util/mocked-dependencies.ts @@ -4,38 +4,50 @@ import { Pool as V3Pool } from '@uniswap/v3-sdk'; import { Pool as V4Pool } from '@uniswap/v4-sdk'; import sinon from 'sinon'; import { + CachedRoutes, CurrencyAmount, + DAI_MAINNET, IGasModel, MixedRouteWithValidQuote, USDC_MAINNET as USDC, V2PoolProvider, V2RouteWithValidQuote, V3PoolProvider, + V3Route, V3RouteWithValidQuote, + V3RouteWithValidQuoteParams, V4PoolProvider, } from '../../../../../../src'; import { buildMockV2PoolAccessor, buildMockV3PoolAccessor, buildMockV4PoolAccessor, DAI_USDT, - DAI_USDT_LOW, DAI_USDT_V4_LOW, + DAI_USDT_LOW, + DAI_USDT_V4_LOW, DAI_WETH, - DAI_WETH_MEDIUM, DAI_WETH_V4_MEDIUM, + DAI_WETH_MEDIUM, + DAI_WETH_V4_MEDIUM, + NONTOKEN, UNI_WETH_MEDIUM, UNI_WETH_V4_MEDIUM, USDC_DAI, USDC_DAI_LOW, - USDC_DAI_MEDIUM, USDC_DAI_V4_LOW, + USDC_DAI_MEDIUM, + USDC_DAI_V4_LOW, USDC_DAI_V4_MEDIUM, - USDC_USDT_MEDIUM, USDC_USDT_V4_MEDIUM, + USDC_USDT_MEDIUM, + USDC_USDT_V4_MEDIUM, USDC_WETH, USDC_WETH_LOW, USDC_WETH_V4_LOW, WBTC_WETH, WETH9_USDT_LOW, WETH9_USDT_V4_LOW, + WETH_NONTOKEN_MEDIUM, WETH_USDT } from '../../../../../test-util/mock-data'; +import { ChainId, TradeType, WETH9 } from '@uniswap/sdk-core'; +import { Protocol } from '@uniswap/router-sdk'; export function getMockedMixedGasModel(): IGasModel { const mockMixedGasModel = { @@ -144,3 +156,31 @@ export function getMockedV2PoolProvider(): V2PoolProvider { })); return mockV2PoolProvider; } + +export function getV3RouteWithInValidQuoteStub( + overrides?: Partial +): V3RouteWithValidQuote { + const route = new V3Route([WETH_NONTOKEN_MEDIUM], NONTOKEN, WETH9[ChainId.MAINNET]!); + + return new V3RouteWithValidQuote({ + amount: CurrencyAmount.fromRawAmount(WETH9[ChainId.MAINNET]!, 1), + rawQuote: BigNumber.from(100), + sqrtPriceX96AfterList: [BigNumber.from(1)], + initializedTicksCrossedList: [1], + quoterGasEstimate: BigNumber.from(100000), // unused + percent: 100, + route, + gasModel: getMockedV3GasModel(), + quoteToken: DAI_MAINNET, + tradeType: TradeType.EXACT_INPUT, + v3PoolProvider: getMockedV3PoolProvider(), + ...overrides, + }); +} + +export function getInvalidCachedRoutesStub( + blockNumber: number +): CachedRoutes | undefined { + return CachedRoutes.fromRoutesWithValidQuotes([getV3RouteWithInValidQuoteStub()], ChainId.MAINNET, USDC, DAI_MAINNET, [Protocol.V2, Protocol.V3, Protocol.MIXED], blockNumber, TradeType.EXACT_INPUT, '1.1'); +} +