Skip to content

Commit

Permalink
chore: bump sor to 4.7.5 - fix: cached routes cache invalidation (#747)
Browse files Browse the repository at this point in the history
- **What kind of change does this PR introduce?** (Bug fix, feature, docs update, ...)
Bug fix

- **What is the current behavior?** (You can also link to an open issue here)
if the cached routes is invalid, meaning it cant get any swap route at all, it does not get cache invalidated.

- **What is the new behavior (if this is a feature change)?**
we will invalidate the cached routes by checking to see if the swap route from cache is invalid

- **Other information**:
we have to roll out with percentage. because there's a chance there are many invalid cached routes in prod.

This is somewhat critical to fix prior to V4, because there's a chance that routing caches invalid v4-pool routes. Previously this wasn't much of a concern,. since V3 launched in 2021, and cached routes was rolled out in 2023, so the v2 and v3 pool TVLs won't change that much, although concentrated liquidities can change based on the market pricing.

I expect with v4, there will be more dynamics from 0 TVL to some TVLs, hence cached routes have more chance of caching invalid v4-pool routes.
  • Loading branch information
jsy1218 authored Oct 28, 2024
1 parent 7fc5a46 commit 4067969
Show file tree
Hide file tree
Showing 9 changed files with 321 additions and 57 deletions.
4 changes: 2 additions & 2 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
@@ -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",
Expand Down
211 changes: 162 additions & 49 deletions src/routers/alpha-router/alpha-router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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<V> extends Map<string, V> {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -582,8 +588,8 @@ export class AlphaRouter
v2Supported,
v4Supported,
mixedSupported,
universalRouterVersion,
v4PoolParams,
cachedRoutesCacheInvalidationFixRolloutPercentage,
}: AlphaRouterParams) {
this.chainId = chainId;
this.provider = provider;
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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;
}
Expand All @@ -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,
Expand All @@ -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(
Expand Down Expand Up @@ -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<void> {
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,
Expand Down
1 change: 1 addition & 0 deletions src/util/index.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down
8 changes: 8 additions & 0 deletions src/util/intent.ts
Original file line number Diff line number Diff line change
@@ -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',
}
Loading

0 comments on commit 4067969

Please sign in to comment.