feat: add caching to SDK Bridging #521
Conversation
WalkthroughAdds a TTL-backed caching layer (Memory + localStorage) to BridgingSdk for intermediate tokens and buy-tokens, threads caches through strategy instances, extends request shapes to accept caches/TTLs, refactors strategies into base classes, re-exports TTLCache, and exposes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant SDK as BridgingSdk
participant Cache as TTLCache (buyTokens)
participant Provider as BridgeProvider
App->>SDK: getBuyTokens(providerId, sellChainId, buyChainId, sellToken)
SDK->>Cache: get(cacheKey)
alt cache hit
Cache-->>SDK: cached result
SDK-->>App: return cached result
else cache miss
SDK->>Provider: getBuyTokens(...)
Provider-->>SDK: result
SDK->>Cache: set(cacheKey, result)
SDK-->>App: return result
end
sequenceDiagram
autonumber
actor App
participant SDK as getQuoteWithBridge
participant Base as getBaseBridgeQuoteRequest
participant ICache as TTLCache (intermediateTokens)
participant Provider as BridgeProvider
App->>SDK: getQuoteWithBridge(params{..., intermediateTokensCache?, intermediateTokensTtl?})
SDK->>Base: build base request (includes cache params)
Base->>ICache: get(cacheKey)
alt cache hit
ICache-->>Base: cached TokenInfo[]
else cache miss
Base->>Provider: getIntermediateTokens(...)
Provider-->>Base: TokenInfo[]
Base->>ICache: set(cacheKey, tokens)
end
Base-->>SDK: QuoteBridgeRequest (uses first intermediate token)
SDK-->>App: BridgeQuoteResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (1)
66-66: BigInt-safe logging to avoid runtime crash.
JSON.stringifyon objects containing BigInt throws even if logging is disabled (args evaluated before call). Use the provided replacer.Apply:
- log(`Using mocked hook for swap gas estimation: ${JSON.stringify(mockedHook)}`) + log(`Using mocked hook for swap gas estimation: ${JSON.stringify(mockedHook, jsonWithBigintReplacer)}`)
🧹 Nitpick comments (21)
packages/common/src/utils/ttlCache.ts (2)
124-131: Guard against non-positive TTLsStoring entries with ttl <= 0 makes them immediately expired but still persisted until read/cleanup. Early-return avoids wasted writes and reduces churn. The diff above includes this guard in set().
63-97: Optional: share fallback memory across wrappersLocalStorageWrapper creates a new MemoryStorage per instance, so fallback persistence isn’t shared across TTLCache instances. If you want same-tab persistence when localStorage is unavailable, consider a static fallback:
-class LocalStorageWrapper implements CacheStorage { - private fallback = new MemoryStorage() +class LocalStorageWrapper implements CacheStorage { + private static fallbackStorage = new MemoryStorage() @@ - return typeof localStorage !== 'undefined' ? localStorage.getItem(key) : this.fallback.getItem(key) + return typeof localStorage !== 'undefined' ? localStorage.getItem(key) : LocalStorageWrapper.fallbackStorage.getItem(key) @@ - this.fallback.setItem(key, value) + LocalStorageWrapper.fallbackStorage.setItem(key, value) @@ - this.fallback.removeItem(key) + LocalStorageWrapper.fallbackStorage.removeItem(key)Not required for correctness; improves ergonomics in environments without localStorage.
packages/bridging/src/BridgingSdk/BridgingSdk.test.ts (2)
110-115: Nit: test name doesn’t match assertion“no networks are supported” reads oddly; the assertion checks ALL_SUPPORTED_CHAINS. Suggest renaming for clarity:
- describe('getSourceNetworks', () => { - it('no networks are supported', async () => { + describe('getSourceNetworks', () => { + it('returns ALL_SUPPORTED_CHAINS', async () => {
126-139: Nit: test name doesn’t match behaviorConsider “errors bubble up”:
- it('errors buble up', async () => { + it('errors bubble up', async () => {packages/bridging/src/BridgingSdk/getQuoteWithBridge.test.ts (2)
207-221: Strengthen assertion: validate validTo is propagatedYou can assert the overridden validTo reaches the second getUnsignedBridgeCall invocation:
@@ - expect(getQuoteMock).toHaveBeenCalledTimes(2) - // Verify the validTo is passed correctly to the bridge request - expect(getUnsignedBridgeCallMock.mock.calls[1]).toBeDefined() + expect(getQuoteMock).toHaveBeenCalledTimes(2) + const secondBridgeReq = getUnsignedBridgeCallMock.mock.calls[1]?.[0] + expect(secondBridgeReq).toBeDefined() + expect(secondBridgeReq.validTo).toBe(customValidTo)
101-109: Test helper hides provider parameter — simplify signaturepostOrderWithIntermediateTokensCache ignores request.provider and always uses mockProvider. Either remove provider from the helper’s parameter type or forward it to getQuoteWithBridge to avoid confusion.
-async function postOrderWithIntermediateTokensCache(request: GetQuoteWithBridgeParams<BridgeQuoteResult>) { +async function postOrderWithIntermediateTokensCache(request: Omit<GetQuoteWithBridgeParams<BridgeQuoteResult>, 'provider' | 'tradingSdk'>) { return getQuoteWithBridge({ swapAndBridgeRequest: request.swapAndBridgeRequest, - provider: mockProvider, - tradingSdk, + provider: mockProvider, + tradingSdk, intermediateTokensCache: request.intermediateTokensCache, intermediateTokensTtl: request.intermediateTokensTtl, }) }packages/bridging/src/BridgingSdk/interfaces.ts (4)
11-13: Clarify cache scope in JSDoc.“target networks and buy tokens” is misleading. This flag controls caching for intermediate tokens and buy tokens.
Apply:
- * Enable caching for target networks and buy tokens + * Enable caching for intermediate tokens and buy tokens
45-49: Allow partial cache overrides in options.Typing
cacheConfigas fullBridgingSdkCacheConfigforces callers to re‑specify TTLs when they just want to tweak one value or toggle caching. AcceptPartial<...>and keep the merge with defaults.Apply:
- cacheConfig?: BridgingSdkCacheConfig + cacheConfig?: Partial<BridgingSdkCacheConfig>
15-21: Validate TTLs are non‑negative (doc or runtime).If a negative TTL slips in, entries will instantly expire or create undefined behavior across environments. Either document “must be >= 0” or clamp at construction.
64-68: Document default env behavior.Note that when omitted, env falls back to
orderBookApi.context.envfrom Trading SDK configuration.Apply:
- * The environment of the order + * The environment of the order. Defaults to Trading SDK's OrderBookApi context env if omitted.packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (7)
47-49: Grammar fix in error message.Minor polish.
Apply:
- throw new Error('Bridging only support SELL orders') + throw new Error('Bridging only supports SELL orders')
68-71: Consistent naming: “intermediate” vs “intermediary”.Use a single term to reduce cognitive load.
Apply:
- const { sellTokenAddress: intermediateToken, sellTokenDecimals: intermediaryTokenDecimals } = + const { sellTokenAddress: intermediateToken, sellTokenDecimals: intermediateTokenDecimals } = bridgeRequestWithoutAmountAnd update references below.
110-113: Avoid unsafe BigInt conversion for 10decimals.**
BigInt(10 ** n)for n≥16 can be inaccurate due to double rounding. Use BigInt exponentiation.Apply:
- log( - `Expected to receive ${intermediateTokenAmount} of the intermediate token (${(intermediateTokenAmount / BigInt(10 ** intermediaryTokenDecimals)).toString()} formatted)`, - ) + log( + `Expected to receive ${intermediateTokenAmount} of the intermediate token (${(intermediateTokenAmount / (10n ** BigInt(intermediateTokenDecimals))).toString()} formatted)`, + )(If you kept the previous variable name, replace accordingly.)
231-233: Typo in comment.Apply:
- * Ge the base params (without the amount) for the bridge quote request + * Get the base params (without the amount) for the bridge quote request
244-252: Avoid double cache lookups.Read once, branch on the value.
Apply:
- if (intermediateTokensCache && intermediateTokensCache.get(cacheKey)) { - intermediateTokens = intermediateTokensCache.get(cacheKey) as TokenInfo[] - } else { + const cached = intermediateTokensCache?.get(cacheKey) + if (cached) { + intermediateTokens = cached + } else { intermediateTokens = await provider.getIntermediateTokens(quoteBridgeRequest) if (intermediateTokensCache && intermediateTokensTtl && intermediateTokens.length > 0) { intermediateTokensCache.set(cacheKey, intermediateTokens, intermediateTokensTtl) } }
260-261: Log meaningful token info instead of “[object Object]”.Improves debuggability.
Apply:
- log(`Using ${intermediateToken} as intermediate tokens`) + log(`Using ${intermediateToken.address} (decimals=${intermediateToken.decimals}) as intermediate token`)
262-264: Redundant null check after length guard.
intermediateTokens.length === 0already returns earlier.Apply:
- if (!intermediateToken) { - throw new BridgeProviderQuoteError(BridgeQuoteErrors.NO_INTERMEDIATE_TOKENS, { intermediateTokens }) - } + // unreachable: guarded by length check abovepackages/bridging/src/BridgingSdk/BridgingSdk.ts (4)
47-47: Exclude enableLogging from config payload.Avoid leaking
enableLoggingintothis.configwhich is typed to exclude it.Apply:
- const { providers, cacheConfig, ...restOptions } = options + const { providers, cacheConfig, enableLogging: _ignoreEnableLogging, ...restOptions } = options
68-74: Clamp/sanitize TTL values after merging defaults.Prevents negative/NaN TTLs from causing surprising behavior.
Apply:
- this.cacheConfig = { ...DEFAULT_CACHE_CONFIG, ...cacheConfig } + this.cacheConfig = { ...DEFAULT_CACHE_CONFIG, ...cacheConfig } + // Sanitize TTLs + this.cacheConfig.intermediateTokensTtl = Math.max(0, Number(this.cacheConfig.intermediateTokensTtl) || 0) + this.cacheConfig.buyTokensTtl = Math.max(0, Number(this.cacheConfig.buyTokensTtl) || 0)
72-74: Decouple “enabled” from persistence backend.Passing
enabledas theuseLocalStorageflag conflates caching toggle with persistence choice. Consider a separatepersistInLocalStorageoption so you can disable caching logic while still inspecting persisted stats (or vice versa).
117-119: Avoid double cache lookups.Small perf/readability win.
Apply:
- if (this.cacheConfig.enabled && this.buyTokensCache.get(cacheKey)) { - return this.buyTokensCache.get(cacheKey) as GetProviderBuyTokens - } + if (this.cacheConfig.enabled) { + const cached = this.buyTokensCache.get(cacheKey) + if (cached) return cached + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/bridging/src/BridgingSdk/BridgingSdk.test.ts(3 hunks)packages/bridging/src/BridgingSdk/BridgingSdk.ts(6 hunks)packages/bridging/src/BridgingSdk/getQuoteWithBridge.test.ts(6 hunks)packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts(3 hunks)packages/bridging/src/BridgingSdk/interfaces.ts(1 hunks)packages/bridging/src/BridgingSdk/types.ts(3 hunks)packages/common/src/utils/index.ts(1 hunks)packages/common/src/utils/ttlCache.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-30T05:56:12.625Z
Learnt from: shoom3301
PR: cowprotocol/cow-sdk#324
File: src/bridging/types.ts:384-385
Timestamp: 2025-05-30T05:56:12.625Z
Learning: The BridgingDepositParams interface in src/bridging/types.ts includes a bridgingId: string field along with other deposit parameters like inputTokenAddress, outputTokenAddress, inputAmount, outputAmount, owner, quoteTimestamp, fillDeadline, recipient, sourceChainId, and destinationChainId.
Applied to files:
packages/bridging/src/BridgingSdk/types.tspackages/bridging/src/BridgingSdk/interfaces.ts
🧬 Code graph analysis (6)
packages/bridging/src/BridgingSdk/types.ts (3)
packages/common/src/utils/ttlCache.ts (1)
TTLCache(103-265)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/BridgingSdk/interfaces.ts (1)
BridgingSdkOptions(24-49)
packages/bridging/src/BridgingSdk/interfaces.ts (4)
packages/bridging/src/types.ts (2)
BridgeProvider(156-279)BridgeQuoteResult(53-99)packages/trading/src/tradingSdk.ts (1)
TradingSdk(28-190)packages/order-book/src/api.ts (1)
OrderBookApi(146-445)packages/config/src/types/configs.ts (1)
CowEnv(35-35)
packages/bridging/src/BridgingSdk/BridgingSdk.test.ts (2)
packages/config/src/chains/types.ts (1)
TargetChainId(34-34)packages/bridging/src/BridgingSdk/BridgingSdk.ts (1)
BridgingSdk(33-218)
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (3)
packages/common/src/utils/ttlCache.ts (1)
TTLCache(103-265)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/types.ts (1)
QuoteBridgeRequestWithoutAmount(51-51)
packages/bridging/src/BridgingSdk/BridgingSdk.ts (5)
packages/bridging/src/BridgingSdk/interfaces.ts (1)
BridgingSdkCacheConfig(9-22)packages/bridging/src/BridgingSdk/types.ts (1)
BridgingSdkConfig(70-70)packages/common/src/utils/ttlCache.ts (1)
TTLCache(103-265)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/types.ts (1)
GetProviderBuyTokens(138-141)
packages/bridging/src/BridgingSdk/getQuoteWithBridge.test.ts (5)
packages/bridging/src/BridgingSdk/types.ts (1)
GetQuoteWithBridgeParams(14-50)packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (2)
getQuoteWithBridge(29-228)postSwapOrderFromQuote(180-226)packages/bridging/src/errors.ts (1)
BridgeProviderQuoteError(12-20)packages/common/src/utils/ttlCache.ts (1)
TTLCache(103-265)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Package
- GitHub Check: test
- GitHub Check: eslint
🔇 Additional comments (6)
packages/bridging/src/BridgingSdk/types.ts (1)
10-13: Types look good; ensure TTLCache is resolvable from sdk-common
- The optional cache hooks on GetQuoteWithBridgeParams are well-scoped.
- BridgingSdkConfig alias is correct.
Please confirm TTLCache is exported from @cowprotocol/sdk-common (root). See verification snippet in utils/index.ts comment.
Also applies to: 41-50, 70-71
packages/bridging/src/BridgingSdk/getQuoteWithBridge.test.ts (3)
195-205: Good: provider quote error path is assertedCovers BridgeProviderQuoteError for getQuote.
222-246: LGTM: intermediate tokens are cached when cache is providedTest correctly verifies single provider hit with TTLCache present.
248-266: LGTM: no caching when no cache is providedBehavior is explicit and tested.
packages/common/src/utils/index.ts (1)
6-6: LGTM — public re-export is correctConfirmed: packages/common/src/index.ts re-exports './utils' and packages/common/src/utils/index.ts re-exports './ttlCache', so import { TTLCache } from '@cowprotocol/sdk-common' works.
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (1)
242-242: Cache key OK if providers expose a stable, prefixed dappIdBridgeProviderInfo includes dappId and the repo defines HOOK_DAPP_BRIDGE_PROVIDER_PREFIX ('cow-sdk://bridging/providers') with provider-specific constants (e.g. BUNGEE_HOOK_DAPP_ID, ACROSS_HOOK_DAPP_ID), so using provider.info.dappId in the cache key is acceptable — ensure all production providers use a stable, prefixed dappId. MockBridgeProvider uses a non-prefixed 'mockProvider' (packages/bridging/src/providers/mock/MockBridgeProvider.ts:119) — keep as test-only or update to the prefixed constant if needed.
| import { BridgingSdkCacheConfig, BridgingSdkOptions, GetOrderParams } from './interfaces' | ||
| import { BridgingSdkConfig } from './types' |
There was a problem hiding this comment.
Nitpick: does it make sense to split interfaces and types into 2 files? To me in TS they are all pretty much the same.
| this.isMemoryStorage = this.storage instanceof MemoryStorage | ||
|
|
||
| // Check if localStorage is actually available even when requested | ||
| if (useLocalStorage && typeof localStorage === 'undefined') { | ||
| this.isMemoryStorage = true |
There was a problem hiding this comment.
Other than setting the flag, wouldn't you need to set this.storage with MemoryStorage too?
| * Get the number of cache entries (approximate for localStorage) | ||
| */ | ||
| size(): number { | ||
| if (this.isMemoryStorage && this.storage instanceof MemoryStorage) { |
There was a problem hiding this comment.
If flag cannot be trusted, what about doing it as a method where it does the check there directly?
| } else { | ||
| intermediateTokens = await provider.getIntermediateTokens(quoteBridgeRequest) | ||
|
|
||
| if (intermediateTokensCache && intermediateTokensTtl && intermediateTokens.length > 0) { |
There was a problem hiding this comment.
It's also possible that there are no tokens in the response, no? In that case it would still make sense to cache the result IMO.
There was a problem hiding this comment.
yes but few lines down we do
if (intermediateTokens.length === 0) {
throw new BridgeProviderQuoteError(BridgeQuoteErrors.NO_INTERMEDIATE_TOKENS)
}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/bridging/src/BridgingSdk/types.ts (3)
42-50: Param surface for caching is fine, but document units consistentlyComments say “TTL” — good. Add “milliseconds” to both fields to avoid ambiguity in downstream usage (same wording as cacheConfig).
- /** - * TTL for intermediate tokens. - */ + /** + * TTL for intermediate tokens (milliseconds). + */ intermediateTokensTtl?: number
53-57: Typo: double “Result” in type nameRename to avoid stutter.
-export interface GetBridgeResultResult { +export interface GetBridgeResult { bridgeResult: BridgeQuoteResults bridgeHook: BridgeHook appDataInfo: TradingAppDataInfo }If you want, I can generate a repo‑wide rename script.
71-89: Cache config type is clear; consider guarding non‑positive TTLs at the API boundaryOptional: define TTLs as positive numbers to prevent immortal entries if 0/NaN slips through.
export interface BridgingSdkCacheConfig { /** * Enable caching for target networks and buy tokens */ enabled: boolean /** - * TTL in milliseconds for getIntermediateTokens cache + * TTL in milliseconds for getIntermediateTokens cache (must be > 0) */ intermediateTokensTtl: number /** - * TTL in milliseconds for getBuyTokens cache + * TTL in milliseconds for getBuyTokens cache (must be > 0) */ buyTokensTtl: number }packages/common/src/utils/ttlCache.ts (1)
151-176: Guard against invalid serialized entriesIf entry.ttl is NaN or missing, the item becomes immortal. Treat invalid TTL as expired.
- const entry: CacheEntry<T> = JSON.parse(item) + const entry: CacheEntry<T> = JSON.parse(item) const now = Date.now() - if (now - entry.timestamp > entry.ttl) { + if (!Number.isFinite(entry.ttl) || now - entry.timestamp > entry.ttl) {packages/bridging/src/BridgingSdk/BridgingSdk.ts (4)
22-27: Sane defaults for cache TTLsDefaults look reasonable (5m/2m). Consider documenting them in README for SDK consumers.
68-73: Don’t couple “enabled” with persistence choicePassing enabled as the TTLCache “useLocalStorage” flag conflates “feature on/off” with “persistence backend.” Prefer always enabling persistence (let TTLCache fall back automatically) and gate reads/writes with enabled.
- this.intermediateTokensCache = new TTLCache<TokenInfo[]>('bridging-intermediate-tokens', this.cacheConfig.enabled) - this.buyTokensCache = new TTLCache<GetProviderBuyTokens>('bridging-buy-tokens', this.cacheConfig.enabled) + // Persist if available; functional enablement is handled in call sites + this.intermediateTokensCache = new TTLCache<TokenInfo[]>('bridging-intermediate-tokens', true) + this.buyTokensCache = new TTLCache<GetProviderBuyTokens>('bridging-buy-tokens', true)
112-118: Cache key normalization LGTM; avoid double get()Lowercasing address fixes duplication. Minor: store the cached value to avoid two lookups.
- if (this.cacheConfig.enabled && this.buyTokensCache.get(cacheKey)) { - return this.buyTokensCache.get(cacheKey) as GetProviderBuyTokens - } + if (this.cacheConfig.enabled) { + const cached = this.buyTokensCache.get(cacheKey) + if (cached) return cached + }
120-124: Guard against non‑positive TTLs from configAvoid inserting immortal/instantly-expired entries if TTLs are 0/NaN.
- if (this.cacheConfig.enabled) { - this.buyTokensCache.set(cacheKey, result, this.cacheConfig.buyTokensTtl) - } + if (this.cacheConfig.enabled && Number.isFinite(this.cacheConfig.buyTokensTtl) && this.cacheConfig.buyTokensTtl > 0) { + this.buyTokensCache.set(cacheKey, result, this.cacheConfig.buyTokensTtl) + }If you’d like, I can add the same guard where intermediateTokensCache is used in getQuoteWithBridge.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/bridging/src/BridgingSdk/BridgingSdk.ts(6 hunks)packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts(3 hunks)packages/bridging/src/BridgingSdk/types.ts(3 hunks)packages/common/src/utils/ttlCache.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-30T05:56:12.625Z
Learnt from: shoom3301
PR: cowprotocol/cow-sdk#324
File: src/bridging/types.ts:384-385
Timestamp: 2025-05-30T05:56:12.625Z
Learning: The BridgingDepositParams interface in src/bridging/types.ts includes a bridgingId: string field along with other deposit parameters like inputTokenAddress, outputTokenAddress, inputAmount, outputAmount, owner, quoteTimestamp, fillDeadline, recipient, sourceChainId, and destinationChainId.
Applied to files:
packages/bridging/src/BridgingSdk/types.ts
🧬 Code graph analysis (2)
packages/bridging/src/BridgingSdk/types.ts (5)
packages/common/src/utils/ttlCache.ts (1)
TTLCache(103-274)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/types.ts (2)
BridgeProvider(156-279)BridgeQuoteResult(53-99)packages/order-book/src/api.ts (1)
OrderBookApi(146-445)packages/config/src/types/configs.ts (1)
CowEnv(35-35)
packages/bridging/src/BridgingSdk/BridgingSdk.ts (4)
packages/bridging/src/BridgingSdk/types.ts (2)
BridgingSdkCacheConfig(76-89)BridgingSdkConfig(71-71)packages/common/src/utils/ttlCache.ts (1)
TTLCache(103-274)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/types.ts (1)
GetProviderBuyTokens(138-141)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (6)
packages/bridging/src/BridgingSdk/types.ts (2)
2-5: Public surface imports LGTMRe‑exporting SignerLike, TTLCache, TokenInfo, OrderBookApi, CowEnv, SupportedChainId is appropriate for SDK consumers.
121-135: GetOrderParams looks good and matches OrderBookApi usageOptional env override with CowEnv aligns with getOrder/getOrderMultiEnv patterns.
packages/bridging/src/BridgingSdk/BridgingSdk.ts (3)
46-66: Options destructuring and config assembly LGTMProvider gating and defaulting tradingSdk/orderBookApi is sound.
151-159: Correct propagation of intermediateTokens cacheWiring cache + TTL into getQuoteWithBridge is correct.
192-216: Admin APIs for cache are helpful; depend on TTLCache fixesMethods look good; they’ll fully work once TTLCache fallback handling is fixed.
Please run tests in SSR/Node and Safari private mode to confirm clear/size/cleanup affect fallback entries.
packages/common/src/utils/ttlCache.ts (1)
186-210: Fix: clear()/size()/cleanup() don’t handle LocalStorageWrapper fallback; entries leak in SSR/private modeWhen localStorage exists but throws (Safari private mode/quota) LocalStorageWrapper falls back to an internal MemoryStorage, but TTLCache still enumerates only real localStorage or MemoryStorage, so entries can’t be cleared or expired. Track keys internally to make these ops backend‑agnostic.
export class TTLCache<T> { private storage: CacheStorage private keyPrefix: string private _isMemoryStorage: boolean = false + // Track keys regardless of backend to support clear/size/cleanup in fallback scenarios + private keys = new Set<string>() @@ - constructor(keyPrefix = 'ttl-cache', useLocalStorage = true) { + constructor(keyPrefix = 'ttl-cache', useLocalStorage = true) { this.keyPrefix = keyPrefix this.storage = useLocalStorage ? new LocalStorageWrapper() : new MemoryStorage() @@ - if (useLocalStorage && typeof localStorage === 'undefined') { + if (useLocalStorage && typeof localStorage === 'undefined') { this.isMemoryStorage = true this.storage = new MemoryStorage() } } @@ - set(key: string, value: T, ttl: number): void { + set(key: string, value: T, ttl: number): void { + // Ignore non‑positive or non‑finite TTLs + if (!Number.isFinite(ttl) || ttl <= 0) return const entry: CacheEntry<T> = { value, timestamp: Date.now(), ttl, } const storageKey = this.getStorageKey(key) try { this.storage.setItem(storageKey, JSON.stringify(entry)) } catch (error) { console.warn('TTLCache: Failed to store cache entry', error) } + this.keys.add(storageKey) } @@ - delete(key: string): void { + delete(key: string): void { const storageKey = this.getStorageKey(key) this.storage.removeItem(storageKey) + this.keys.delete(storageKey) } @@ - clear(): void { - if (this.isMemoryStorage) { - ;(this.storage as MemoryStorage).clear(this.keyPrefix + ':') - return - } - - if (typeof localStorage !== 'undefined') { - try { - const keysToDelete: string[] = [] - for (let i = 0; i < localStorage.length; i++) { - const key = localStorage.key(i) - if (key && key.startsWith(this.keyPrefix + ':')) { - keysToDelete.push(key) - } - } - keysToDelete.forEach((key) => localStorage.removeItem(key)) - } catch { - // Fallback: If localStorage access fails, we can't clear it - console.warn('TTLCache: Failed to clear localStorage cache') - } - } - } + clear(): void { + const prefix = this.keyPrefix + ':' + // Try clearing localStorage namespace (best effort) + if (typeof localStorage !== 'undefined') { + try { + const toDelete: string[] = [] + for (let i = 0; i < localStorage.length; i++) { + const k = localStorage.key(i) + if (k && k.startsWith(prefix)) toDelete.push(k) + } + toDelete.forEach((k) => localStorage.removeItem(k)) + } catch { + console.warn('TTLCache: Failed to clear localStorage cache') + } + } + // Always clear instance‑tracked keys (covers wrapper fallback/SSR) + for (const k of Array.from(this.keys)) { + try { + this.storage.removeItem(k) + } catch { /* noop */ } + this.keys.delete(k) + } + // If using pure MemoryStorage, also clear by prefix for completeness + if (this.storage instanceof MemoryStorage) { + ;(this.storage as MemoryStorage).clear(prefix) + } + } @@ - size(): number { - if (this.isMemoryStorage) { - return (this.storage as MemoryStorage).size(this.keyPrefix + ':') - } - - if (typeof localStorage !== 'undefined') { - try { - let count = 0 - for (let i = 0; i < localStorage.length; i++) { - const key = localStorage.key(i) - if (key && key.startsWith(this.keyPrefix + ':')) { - count++ - } - } - return count - } catch { - return 0 - } - } - return 0 - } + size(): number { + const prefix = this.keyPrefix + ':' + if (this.storage instanceof MemoryStorage) { + return (this.storage as MemoryStorage).size(prefix) + } + // Instance index covers wrapper fallback and SSR + if (this.keys.size > 0) return this.keys.size + // Best‑effort count from localStorage + if (typeof localStorage !== 'undefined') { + try { + let count = 0 + for (let i = 0; i < localStorage.length; i++) { + const k = localStorage.key(i) + if (k && k.startsWith(prefix)) count++ + } + return count + } catch { + return 0 + } + } + return 0 + } @@ - cleanup(): void { - if (typeof localStorage !== 'undefined') { - try { - const keysToDelete: string[] = [] - const now = Date.now() - - for (let i = 0; i < localStorage.length; i++) { - const key = localStorage.key(i) - if (key && key.startsWith(this.keyPrefix + ':')) { - const item = localStorage.getItem(key) - if (item) { - try { - const entry: CacheEntry<T> = JSON.parse(item) - if (now - entry.timestamp > entry.ttl) { - keysToDelete.push(key) - } - } catch { - // Remove corrupted entries - keysToDelete.push(key) - } - } - } - } - - keysToDelete.forEach((key) => localStorage.removeItem(key)) - } catch { - console.warn('TTLCache: Failed to cleanup expired entries') - } - } - } + cleanup(): void { + const prefix = this.keyPrefix + ':' + const now = Date.now() + // LocalStorage namespace + if (typeof localStorage !== 'undefined') { + try { + const toDelete: string[] = [] + for (let i = 0; i < localStorage.length; i++) { + const k = localStorage.key(i) + if (k && k.startsWith(prefix)) { + const item = localStorage.getItem(k) + if (!item) continue + try { + const entry: CacheEntry<T> = JSON.parse(item) + if (!Number.isFinite(entry.ttl) || now - entry.timestamp > entry.ttl) toDelete.push(k) + } catch { + toDelete.push(k) + } + } + } + toDelete.forEach((k) => localStorage.removeItem(k)) + } catch { + console.warn('TTLCache: Failed to cleanup expired entries') + } + } + // Instance‑tracked keys (covers wrapper fallback and SSR) + for (const k of Array.from(this.keys)) { + const item = this.storage.getItem(k) + if (!item) { + this.keys.delete(k) + continue + } + try { + const entry: CacheEntry<T> = JSON.parse(item) + if (!Number.isFinite(entry.ttl) || now - entry.timestamp > entry.ttl) { + this.storage.removeItem(k) + this.keys.delete(k) + } + } catch { + this.storage.removeItem(k) + this.keys.delete(k) + } + } + }Also applies to: 215-235, 240-269, 116-128, 133-146, 181-184
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bridging/src/BridgingSdk/BridgingSdk.ts (1)
208-216: Multi‑quotes path ignores the new cache.
getMultiQuotes()doesn’t pass the intermediate‑tokens cache/TTL, so parallel provider calls miss caching benefits.Apply:
- const quote = await getQuoteWithBridge({ - swapAndBridgeRequest: quoteBridgeRequest, - advancedSettings, - tradingSdk: this.config.tradingSdk, - provider, - bridgeHookSigner: advancedSettings?.quoteSigner, - }) + const base = { + swapAndBridgeRequest: quoteBridgeRequest, + advancedSettings, + tradingSdk: this.config.tradingSdk, + provider, + bridgeHookSigner: advancedSettings?.quoteSigner, + } as const + const params = this.cacheConfig.enabled + ? { + ...base, + intermediateTokensCache: this.intermediateTokensCache, + intermediateTokensTtl: this.cacheConfig.intermediateTokensTtl, + } + : base + const quote = await getQuoteWithBridge(params)
🧹 Nitpick comments (2)
packages/bridging/src/BridgingSdk/BridgingSdk.ts (2)
114-120: Avoid double cache reads and “undefined” in the key.
- Cache is read twice; a TTL boundary could flip between calls.
${undefined}ends up as the literal string "undefined". Prefer empty string.Apply:
async getBuyTokens(params: BuyTokensParams): Promise<GetProviderBuyTokens> { const providerId = this.provider.info.dappId - const cacheKey = `${providerId}-${params.buyChainId}-${params.sellChainId}-${params.sellTokenAddress?.toLowerCase()}` - - if (this.cacheConfig.enabled && this.buyTokensCache.get(cacheKey)) { - return this.buyTokensCache.get(cacheKey) as GetProviderBuyTokens - } + const sellAddr = (params.sellTokenAddress ?? '').toLowerCase() + const cacheKey = `${providerId}-${params.buyChainId}-${params.sellChainId}-${sellAddr}` + const cached = + this.cacheConfig.enabled ? this.buyTokensCache.get(cacheKey) : undefined + if (cached) { + return cached as GetProviderBuyTokens + }
69-75: Don’t conflate “enabled” with “persistence to localStorage”.
new TTLCache(..., this.cacheConfig.enabled)toggles localStorage usage, not caching itself. With caching disabled, you’re still constructing caches (fine) but risk accidental use elsewhere (as seen above). Consider a separatepersistInLocalStorage(default true) to independently control persistence.If you adopt it:
// types.ts export interface BridgingSdkCacheConfig { enabled: boolean + persistInLocalStorage?: boolean intermediateTokensTtl: number buyTokensTtl: number } // BridgingSdk.ts - this.intermediateTokensCache = new TTLCache<TokenInfo[]>('bridging-intermediate-tokens', this.cacheConfig.enabled) - this.buyTokensCache = new TTLCache<GetProviderBuyTokens>('bridging-buy-tokens', this.cacheConfig.enabled) + const persist = this.cacheConfig.persistInLocalStorage ?? true + this.intermediateTokensCache = new TTLCache<TokenInfo[]>('bridging-intermediate-tokens', persist) + this.buyTokensCache = new TTLCache<GetProviderBuyTokens>('bridging-buy-tokens', persist)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bridging/src/BridgingSdk/BridgingSdk.test.ts(3 hunks)packages/bridging/src/BridgingSdk/BridgingSdk.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bridging/src/BridgingSdk/BridgingSdk.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/bridging/src/BridgingSdk/BridgingSdk.ts (4)
packages/bridging/src/BridgingSdk/types.ts (2)
BridgingSdkCacheConfig(76-89)BridgingSdkConfig(71-71)packages/common/src/utils/ttlCache.ts (1)
TTLCache(103-274)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/types.ts (1)
GetProviderBuyTokens(139-142)
🔇 Additional comments (2)
packages/bridging/src/BridgingSdk/BridgingSdk.ts (2)
116-116: Good fix: normalized address casing in cache key.This addresses duplicate-key issues for checksummed vs lowercase addresses.
257-281: Cache management API looks good.Clear, cleanup, and stats methods are straightforward and align with TTLCache semantics.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/bridging/src/BridgingSdk/helpers.ts (1)
1-13: Harden cache key against empty/undefined tokenAddress; keep key stable.Use a safe fallback when tokenAddress is falsy and avoid double-calling toLowerCase.
export const getCacheKey = ({ id, buyChainId, sellChainId = 'noSellChainID', tokenAddress = 'noTokenAddress', }: { id: string buyChainId: string sellChainId?: string tokenAddress?: string }) => { - return `${id}-${buyChainId}-${sellChainId}-${tokenAddress.toLowerCase()}` + const addr = (tokenAddress || 'noTokenAddress').toLowerCase() + return `${id}-${buyChainId}-${sellChainId}-${addr}` }packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (3)
111-114: Fix BigInt scaling: avoid unsafe Number → BigInt conversion.
BigInt(10 ** decimals)loses precision for decimals ≥ 16. Use pure BigInt math.- log( - `Expected to receive ${intermediateTokenAmount} of the intermediate token (${(intermediateTokenAmount / BigInt(10 ** intermediaryTokenDecimals)).toString()} formatted)`, - ) + const scale = BigInt(10) ** BigInt(intermediaryTokenDecimals) + log( + `Expected to receive ${intermediateTokenAmount} of the intermediate token (${(intermediateTokenAmount / scale).toString()} formatted)`, + )
244-259: Make cache use consistent and avoid redundant lookups; don’t gate on TTL here.
- Call
getonce and reuse result.- TTL is configured in the cache instance; don’t gate
set()onintermediateTokensTtl.- if (intermediateTokensCache && intermediateTokensCache.get(cacheKey)) { - intermediateTokens = intermediateTokensCache.get(cacheKey) as TokenInfo[] - } else { - intermediateTokens = await provider.getIntermediateTokens(quoteBridgeRequest) - - if (intermediateTokensCache && intermediateTokensTtl) { - intermediateTokensCache.set(cacheKey, intermediateTokens) - } - } + const cached = intermediateTokensCache?.get(cacheKey) + if (cached !== undefined) { + intermediateTokens = cached as TokenInfo[] + } else { + intermediateTokens = await provider.getIntermediateTokens(quoteBridgeRequest) + intermediateTokensCache?.set(cacheKey, intermediateTokens) + }
265-271: Improve log output; remove redundant null check.
- Logging the token object prints “[object Object]”. Log address/symbol.
- The
!intermediateTokencheck is redundant after the length check above.- log(`Using ${intermediateToken} as intermediate tokens`) + log(`Using intermediate token ${intermediateToken.symbol ?? ''} (${intermediateToken.address})`) @@ - if (!intermediateToken) { - throw new BridgeProviderQuoteError(BridgeQuoteErrors.NO_INTERMEDIATE_TOKENS, { intermediateTokens }) - } + // Redundant after length check; safe to remove.packages/bridging/src/BridgingSdk/BridgingSdk.ts (2)
123-134: Avoid duplicate cache lookups.Call
getonce and return early.- if (this.cacheConfig.enabled && this.buyTokensCache.get(cacheKey)) { - return this.buyTokensCache.get(cacheKey) as GetProviderBuyTokens - } + if (this.cacheConfig.enabled) { + const cached = this.buyTokensCache.get(cacheKey) + if (cached !== undefined) return cached + }
73-83: Optional: rely on TTLCache’s constructor TTL; drop TTL param threading.
intermediateTokensTtlis already set in the cache instances; forwarding TTL through call sites is redundant and invites divergence.- this.intermediateTokensCache = new TTLCache<TokenInfo[]>( + this.intermediateTokensCache = new TTLCache<TokenInfo[]>( 'bridging-intermediate-tokens', this.cacheConfig.enabled, this.cacheConfig.intermediateTokensTtl, ) @@ - this.buyTokensCache = new TTLCache<GetProviderBuyTokens>( + this.buyTokensCache = new TTLCache<GetProviderBuyTokens>( 'bridging-buy-tokens', this.cacheConfig.enabled, this.cacheConfig.buyTokensTtl, )Follow-up: remove
intermediateTokensTtlfromgetQuoteWithBridgeparams and its usages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/contracts-ts/src/generated/packageVersion.tsis excluded by!**/generated/**
📒 Files selected for processing (4)
packages/bridging/src/BridgingSdk/BridgingSdk.ts(6 hunks)packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts(4 hunks)packages/bridging/src/BridgingSdk/helpers.ts(1 hunks)packages/common/src/utils/ttlCache.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-19T22:06:52.139Z
Learnt from: anxolin
PR: cowprotocol/cow-sdk#251
File: src/bridging/BridgingSdk/getQuoteWithBridging.ts:34-35
Timestamp: 2025-03-19T22:06:52.139Z
Learning: The implementation in the Across bridge provider deliberately selects only the first intermediate token from the list (`intermediateTokens[0]`) as a simplification for the initial implementation, with potential for future enhancement.
Applied to files:
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts
📚 Learning: 2025-03-20T09:45:27.965Z
Learnt from: anxolin
PR: cowprotocol/cow-sdk#251
File: src/bridging/providers/across/AcrossBridgeProvider.ts:85-100
Timestamp: 2025-03-20T09:45:27.965Z
Learning: In the AcrossBridgeProvider's getIntermediateTokens method, not finding an intermediate token is expected behavior in some cases and should not be logged as a warning.
Applied to files:
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts
🧬 Code graph analysis (2)
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (4)
packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-280)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/types.ts (1)
QuoteBridgeRequestWithoutAmount(52-52)packages/bridging/src/BridgingSdk/helpers.ts (1)
getCacheKey(1-13)
packages/bridging/src/BridgingSdk/BridgingSdk.ts (5)
packages/bridging/src/BridgingSdk/types.ts (2)
BridgingSdkCacheConfig(76-89)BridgingSdkConfig(71-71)packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-280)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/types.ts (1)
GetProviderBuyTokens(139-142)packages/bridging/src/BridgingSdk/helpers.ts (1)
getCacheKey(1-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: eslint
- GitHub Check: test
🔇 Additional comments (1)
packages/bridging/src/BridgingSdk/BridgingSdk.ts (1)
173-175: Only passintermediateTokensCachewhencacheConfig.enabledis true
The SDK currently always passes a TTLCache instance (the second constructor arg only controls storage backend), so settingenabled=falsemerely swaps to in-memory storage instead of disabling caching. Guard the cache params behindcacheConfig.enabled.File: packages/bridging/src/BridgingSdk/BridgingSdk.ts – inside
getQuote- return getQuoteWithBridge({ - swapAndBridgeRequest: quoteBridgeRequest, - advancedSettings, - tradingSdk, - provider: this.provider, - bridgeHookSigner: advancedSettings?.quoteSigner, - intermediateTokensCache: this.intermediateTokensCache, - intermediateTokensTtl: this.cacheConfig.intermediateTokensTtl, - }) + const baseParams = { + swapAndBridgeRequest: quoteBridgeRequest, + advancedSettings, + tradingSdk, + provider: this.provider, + bridgeHookSigner: advancedSettings?.quoteSigner, + } as const + const params = this.cacheConfig.enabled + ? { + ...baseParams, + intermediateTokensCache: this.intermediateTokensCache, + intermediateTokensTtl: this.cacheConfig.intermediateTokensTtl, + } + : baseParams + return getQuoteWithBridge(params)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (1)
111-114: Fix BigInt exponent: current code can throw at runtime for 18‑dec tokens.
BigInt(10 ** n)overflows for n ≥ 16 (Number not safely representable). Use BigInt exponentiation.- log( - `Expected to receive ${intermediateTokenAmount} of the intermediate token (${(intermediateTokenAmount / BigInt(10 ** intermediaryTokenDecimals)).toString()} formatted)`, - ) + const formattedIntermediate = (intermediateTokenAmount / (10n ** BigInt(intermediaryTokenDecimals))).toString() + log( + `Expected to receive ${intermediateTokenAmount} of the intermediate token (${formattedIntermediate} formatted)`, + )
🧹 Nitpick comments (6)
packages/bridging/src/BridgingSdk/helpers.ts (1)
1-13: Normalize all key parts (providerId and chainIds) for deterministic cache keys.Lowercasing only the address leaves room for duplicate/missed hits if id/buyChainId/sellChainId vary in case. Safe to normalize all.
export const getCacheKey = ({ id, buyChainId, sellChainId = 'noSellChainID', tokenAddress = 'noTokenAddress', }: { id: string buyChainId: string sellChainId?: string tokenAddress?: string }) => { - return `${id}-${buyChainId}-${sellChainId}-${tokenAddress.toLowerCase()}` + return `${id.toLowerCase()}-${buyChainId.toLowerCase()}-${sellChainId.toLowerCase()}-${tokenAddress.toLowerCase()}` }packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (2)
237-259: Avoid double cache lookups and remove TTL‑gate from set().
- You
get()twice.- Caching should not depend on an external TTL param;
TTLCachealready embeds TTL. Set when cache exists.- let intermediateTokens: TokenInfo[] = [] - - const cacheKey = getCacheKey({ + let intermediateTokens: TokenInfo[] = [] + const cacheKey = getCacheKey({ id: provider.info.dappId, buyChainId: quoteBridgeRequest.buyTokenChainId.toString(), sellChainId: quoteBridgeRequest.sellTokenChainId.toString(), tokenAddress: quoteBridgeRequest.buyTokenAddress, }) - - if (intermediateTokensCache && intermediateTokensCache.get(cacheKey)) { - intermediateTokens = intermediateTokensCache.get(cacheKey) as TokenInfo[] - } else { - intermediateTokens = await provider.getIntermediateTokens(quoteBridgeRequest) - - if (intermediateTokensCache && intermediateTokensTtl) { - intermediateTokensCache.set(cacheKey, intermediateTokens) - } - } + const cached = intermediateTokensCache?.get(cacheKey) as TokenInfo[] | undefined + if (cached !== undefined) { + intermediateTokens = cached + } else { + intermediateTokens = await provider.getIntermediateTokens(quoteBridgeRequest) + if (intermediateTokensCache) { + intermediateTokensCache.set(cacheKey, intermediateTokens) + } + }
267-268: Improve log readability (avoid “[object Object]”).Log address/symbol instead of the object.
- log(`Using ${intermediateToken} as intermediate tokens`) + log(`Using ${intermediateToken.symbol ?? 'token'} (${intermediateToken.address}) as intermediate token`)packages/common/src/utils/ttlCache.ts (1)
121-134: Constructor: prefer deriving memory mode from storage, not a mutable flag.
isMemoryStorageis set and later re‑evaluated viainstanceof, but wrapper fallback can’t be detected. Consider makingisMemoryStoragea method that checks backend dynamically.packages/bridging/src/BridgingSdk/BridgingSdk.ts (2)
288-312: Expose cache stats/ops even when disabled; minor doc note.APIs operate on instances regardless of
enabled. Consider documenting that: stats return 0 when disabled; clear/cleanup are no‑ops.
20-24: Optional: construct caches only when enabled.You already guard usage; lazily construct when
enabledto reduce footprint.- private intermediateTokensCache: TTLCache<TokenInfo[]> - private buyTokensCache: TTLCache<GetProviderBuyTokens> + private intermediateTokensCache!: TTLCache<TokenInfo[]> + private buyTokensCache!: TTLCache<GetProviderBuyTokens> @@ - this.intermediateTokensCache = new TTLCache<TokenInfo[]>( - 'bridging-intermediate-tokens', - this.cacheConfig.enabled, - this.cacheConfig.intermediateTokensTtl, - ) - this.buyTokensCache = new TTLCache<GetProviderBuyTokens>( - 'bridging-buy-tokens', - this.cacheConfig.enabled, - this.cacheConfig.buyTokensTtl, - ) + if (this.cacheConfig.enabled) { + this.intermediateTokensCache = new TTLCache<TokenInfo[]>( + 'bridging-intermediate-tokens', + true, + this.cacheConfig.intermediateTokensTtl, + ) + this.buyTokensCache = new TTLCache<GetProviderBuyTokens>( + 'bridging-buy-tokens', + true, + this.cacheConfig.buyTokensTtl, + ) + }Also applies to: 73-83
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/contracts-ts/src/generated/packageVersion.tsis excluded by!**/generated/**
📒 Files selected for processing (4)
packages/bridging/src/BridgingSdk/BridgingSdk.ts(7 hunks)packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts(4 hunks)packages/bridging/src/BridgingSdk/helpers.ts(1 hunks)packages/common/src/utils/ttlCache.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-19T22:06:52.139Z
Learnt from: anxolin
PR: cowprotocol/cow-sdk#251
File: src/bridging/BridgingSdk/getQuoteWithBridging.ts:34-35
Timestamp: 2025-03-19T22:06:52.139Z
Learning: The implementation in the Across bridge provider deliberately selects only the first intermediate token from the list (`intermediateTokens[0]`) as a simplification for the initial implementation, with potential for future enhancement.
Applied to files:
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts
📚 Learning: 2025-03-20T09:45:27.965Z
Learnt from: anxolin
PR: cowprotocol/cow-sdk#251
File: src/bridging/providers/across/AcrossBridgeProvider.ts:85-100
Timestamp: 2025-03-20T09:45:27.965Z
Learning: In the AcrossBridgeProvider's getIntermediateTokens method, not finding an intermediate token is expected behavior in some cases and should not be logged as a warning.
Applied to files:
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts
🧬 Code graph analysis (2)
packages/bridging/src/BridgingSdk/BridgingSdk.ts (5)
packages/bridging/src/BridgingSdk/types.ts (1)
BridgingSdkCacheConfig(76-89)packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-280)packages/bridging/src/types.ts (1)
GetProviderBuyTokens(139-142)packages/bridging/src/BridgingSdk/helpers.ts (1)
getCacheKey(1-13)packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (1)
getQuoteWithBridge(30-229)
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (3)
packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-280)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/types.ts (1)
QuoteBridgeRequestWithoutAmount(52-52)
🔇 Additional comments (4)
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (1)
56-62: LGTM: forwarding cache params.Passing cache and TTL through to the base request is correct and keeps the callsite clean.
packages/bridging/src/BridgingSdk/BridgingSdk.ts (2)
123-140: LGTM: buy‑tokens cache key and normalization.Key uses providerId/buyChainId/sellChainId/lowercased sellTokenAddress; fetch/set gated by
enabled. Looks good.
168-182: Good fix: omit cache params when caching disabled.Prevents accidental in‑memory caching. Thanks for addressing.
packages/common/src/utils/ttlCache.ts (1)
192-216: clear()/size()/cleanup() miss LocalStorageWrapper’s in‑memory fallback; entries can’t be enumerated/cleared in SSR/private‑mode.When localStorage exists but is unusable, LocalStorageWrapper falls back to memory, but TTLCache methods only scan real localStorage or MemoryStorage. Add an internal key index and operate on it to cover all backends.
@@ export class TTLCache<T> { private storage: CacheStorage private keyPrefix: string private ttl: number private _isMemoryStorage: boolean = false + // Track keys regardless of backend (covers wrapper fallback/SSR) + private keys = new Set<string>() @@ set(key: string, value: T): void { @@ const storageKey = this.getStorageKey(key) try { this.storage.setItem(storageKey, JSON.stringify(entry)) } catch (error) { console.warn('TTLCache: Failed to store cache entry', error) } + this.keys.add(storageKey) } @@ delete(key: string): void { const storageKey = this.getStorageKey(key) this.storage.removeItem(storageKey) + this.keys.delete(storageKey) } @@ clear(): void { - if (this.isMemoryStorage) { - ;(this.storage as MemoryStorage).clear(this.keyPrefix + ':') - return - } + const prefix = this.keyPrefix + ':' + if (this.isMemoryStorage && this.storage instanceof MemoryStorage) { + ;(this.storage as MemoryStorage).clear(prefix) + this.keys.clear() + return + } if (typeof localStorage !== 'undefined') { try { const keysToDelete: string[] = [] for (let i = 0; i < localStorage.length; i++) { const key = localStorage.key(i) - if (key && key.startsWith(this.keyPrefix + ':')) { + if (key && key.startsWith(prefix)) { keysToDelete.push(key) } } keysToDelete.forEach((key) => localStorage.removeItem(key)) } catch { // Fallback: If localStorage access fails, we can't clear it console.warn('TTLCache: Failed to clear localStorage cache') } } + // Always clear tracked keys (covers wrapper fallback/SSR) + for (const key of Array.from(this.keys)) { + try { this.storage.removeItem(key) } catch {} + this.keys.delete(key) + } } @@ size(): number { - if (this.isMemoryStorage) { - return (this.storage as MemoryStorage).size(this.keyPrefix + ':') - } + const prefix = this.keyPrefix + ':' + if (this.isMemoryStorage && this.storage instanceof MemoryStorage) { + return (this.storage as MemoryStorage).size(prefix) + } + if (typeof localStorage === 'undefined') { + return this.keys.size + } if (typeof localStorage !== 'undefined') { try { let count = 0 for (let i = 0; i < localStorage.length; i++) { const key = localStorage.key(i) - if (key && key.startsWith(this.keyPrefix + ':')) { + if (key && key.startsWith(prefix)) { count++ } } return count } catch { return 0 } } return 0 } @@ cleanup(): void { - if (typeof localStorage !== 'undefined') { + const prefix = this.keyPrefix + ':' + if (typeof localStorage !== 'undefined') { try { const keysToDelete: string[] = [] const now = Date.now() for (let i = 0; i < localStorage.length; i++) { const key = localStorage.key(i) - if (key && key.startsWith(this.keyPrefix + ':')) { + if (key && key.startsWith(prefix)) { const item = localStorage.getItem(key) if (item) { try { const entry: CacheEntry<T> = JSON.parse(item) if (now - entry.timestamp > entry.ttl) { keysToDelete.push(key) } } catch { // Remove corrupted entries keysToDelete.push(key) } } } } keysToDelete.forEach((key) => localStorage.removeItem(key)) } catch { console.warn('TTLCache: Failed to cleanup expired entries') } } + // Also cleanup tracked keys (wrapper fallback/SSR) + const now = Date.now() + for (const key of Array.from(this.keys)) { + const item = this.storage.getItem(key) + if (!item) { this.keys.delete(key); continue } + try { + const entry: CacheEntry<T> = JSON.parse(item) + if (now - entry.timestamp > entry.ttl) { + this.storage.removeItem(key) + this.keys.delete(key) + } + } catch { + this.storage.removeItem(key) + this.keys.delete(key) + } + } }Also applies to: 221-241, 246-275
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.ts (1)
25-31: Pass through cache fields to getQuoteWithBridge.Without forwarding, intermediate-token caching never activates.
Apply:
return getQuoteWithBridge({ swapAndBridgeRequest: quoteBridgeRequest, advancedSettings, tradingSdk, provider, bridgeHookSigner: advancedSettings?.quoteSigner, + intermediateTokensCache: request.intermediateTokensCache, + intermediateTokensTtl: request.intermediateTokensTtl, })
🧹 Nitpick comments (5)
packages/bridging/src/BridgingSdk/BridgingSdk.ts (2)
82-92: Avoid constructing caches when caching is disabled (optional).You still instantiate TTLCache instances when caching is disabled. Not harmful, but avoidable.
Consider lazy-instantiating the caches only when
this.cacheConfig.enabledis true to reduce overhead.
141-143: Avoid double cache lookup and JSON parse.Store the cached value to prevent parsing twice.
Apply:
- if (this.cacheConfig.enabled && this.buyTokensCache.get(cacheKey)) { - return this.buyTokensCache.get(cacheKey) as GetProviderBuyTokens - } + if (this.cacheConfig.enabled) { + const cached = this.buyTokensCache.get(cacheKey) + if (cached) return cached + }packages/bridging/src/types.ts (1)
452-460: Duplicate MultiQuoteRequest definition — unify single source of truth.This duplicates the interface also defined in strategies/QuoteStrategy.ts. Divergence risk.
Prefer defining SingleQuoteRequest/MultiQuoteRequest once (e.g., here) and importing in strategies. Alternatively, remove this copy and re-export from strategies to keep one definition.
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts (2)
221-223: Test assertion is too weak; it doesn’t validate “first provider error”.
This only checks membership. Make it deterministic and assert the earliest failure is returned.Apply this change within the assertion:
- expect(['mockProvider', 'cow-sdk://bridging/providers/mock2', 'cow-sdk://bridging/providers/mock3']).toContain( - result?.providerDappId, - ) + expect(result?.providerDappId).toBe('mockProvider') + expect(result?.error).toBe(error1)And update the mocks in this test to enforce failure order:
// Before building the request: mockProvider.getQuote = jest.fn().mockImplementation(async () => { await new Promise((r) => setTimeout(r, 10)) // earliest failure throw error1 }) mockProvider2.getQuote = jest.fn().mockImplementation(async () => { await new Promise((r) => setTimeout(r, 30)) throw error2 }) mockProvider3.getQuote = jest.fn().mockImplementation(async () => { await new Promise((r) => setTimeout(r, 50)) throw error3 })
260-261: Avoid brittle exact-string error matching; use a regex.
More resilient to message tweaks.- 'getMultiQuotes() and getBestQuote() are only for cross-chain bridging', + /only for cross-chain bridging/,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/bridging/src/BridgingSdk/BridgingSdk.test.ts(3 hunks)packages/bridging/src/BridgingSdk/BridgingSdk.ts(8 hunks)packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts(5 hunks)packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.ts(1 hunks)packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.test.ts(3 hunks)packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.ts(1 hunks)packages/bridging/src/BridgingSdk/strategies/QuoteStrategy.ts(2 hunks)packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.test.ts(4 hunks)packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.ts(1 hunks)packages/bridging/src/BridgingSdk/utils.ts(1 hunks)packages/bridging/src/types.ts(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.ts
- packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.ts
- packages/bridging/src/BridgingSdk/utils.ts
- packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T16:17:50.800Z
Learnt from: cowdan
PR: cowprotocol/cow-sdk#492
File: packages/bridging/src/providers/bungee/BungeeApi.ts:393-395
Timestamp: 2025-09-12T16:17:50.800Z
Learning: In the cowprotocol/cow-sdk bridging package, tests for BungeeApi are designed to hit real public Bungee endpoints without affiliate headers for integration testing, rather than using mocked network calls. This approach is preferred by the maintainers as it provides real API integration coverage.
Applied to files:
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts
🧬 Code graph analysis (4)
packages/bridging/src/BridgingSdk/BridgingSdk.test.ts (2)
packages/config/src/chains/types.ts (1)
TargetChainId(34-34)packages/bridging/src/BridgingSdk/BridgingSdk.ts (1)
BridgingSdk(44-283)
packages/bridging/src/types.ts (2)
packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-280)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)
packages/bridging/src/BridgingSdk/strategies/QuoteStrategy.ts (2)
packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-280)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)
packages/bridging/src/BridgingSdk/BridgingSdk.ts (6)
packages/bridging/src/BridgingSdk/types.ts (2)
BridgingSdkCacheConfig(76-89)BridgingSdkConfig(71-71)packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-280)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/types.ts (2)
GetProviderBuyTokens(139-142)MultiQuoteRequest(453-460)packages/bridging/src/BridgingSdk/helpers.ts (1)
getCacheKey(1-13)packages/bridging/src/BridgingSdk/strategies/QuoteStrategy.ts (2)
SingleQuoteRequest(26-31)MultiQuoteRequest(36-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: eslint
🔇 Additional comments (17)
packages/bridging/src/BridgingSdk/BridgingSdk.ts (3)
34-39: Sensible defaults for cache config.Defaults look reasonable for first pass.
171-183: Ensure the strategy forwards cache params to getQuoteWithBridge.You correctly build
requestwith cache fields when enabled, butSingleQuoteStrategyImplcurrently drops them. Fix in SingleQuoteStrategy.ts (see comment there) so the cache is actually used.
203-210: Fix inverted cache-params logic in getMultiQuotes.Cache params are added when caching is disabled and omitted when enabled.
Apply:
- const multiQuoteRequest: MultiQuoteRequest = this.cacheConfig.enabled - ? request - : { - ...request, - intermediateTokensCache: this.intermediateTokensCache, - intermediateTokensTtl: this.cacheConfig.intermediateTokensTtl, - } + const multiQuoteRequest: MultiQuoteRequest = this.cacheConfig.enabled + ? { + ...request, + intermediateTokensCache: this.intermediateTokensCache, + intermediateTokensTtl: this.cacheConfig.intermediateTokensTtl, + } + : requestpackages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.test.ts (3)
31-31: Type import move LGTM.Importing BridgingSdkConfig from ../types is consistent with the new public types module.
267-273: Formatting-only change is fine.No behavior change; test intent remains clear.
300-301: Error message assertion remains consistent.Matches thrown error in strategy.
packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.ts (1)
4-4: Type import move LGTM.Consistent with types consolidation.
packages/bridging/src/types.ts (1)
14-14: TTLCache usage in public types LGTM.Makes cache types available where needed.
packages/bridging/src/BridgingSdk/strategies/QuoteStrategy.ts (2)
9-11: Imports aligned with new types.No issues.
29-31: New cache fields on SingleQuoteRequest LGTM.Ensure callers/strategies propagate them (see fix in SingleQuoteStrategy.ts).
packages/bridging/src/BridgingSdk/BridgingSdk.test.ts (2)
48-52: Good hygiene: clear localStorage per test.Prevents cross-test flakiness.
340-346: Fix test: it never re-queries after cleanup, so it can’t verify cleanup worked.Call getBuyTokens again and assert the second provider hit.
Apply:
- // Cleanup expired entries - shortTtlSdk.cleanupExpiredCache() - - // Cache should be cleaned up (note: localStorage cleanup is async and may not reflect immediately in size()) - // So we test that expired entries are not returned - expect(mockProvider.getBuyTokens).toHaveBeenCalledTimes(1) // Should call provider again + // Cleanup expired entries + shortTtlSdk.cleanupExpiredCache() + + // Next call should miss cache and hit provider again + await shortTtlSdk.getBuyTokens({ buyChainId: 137 as TargetChainId }) + expect(mockProvider.getBuyTokens).toHaveBeenCalledTimes(2)packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts (5)
16-16: Import formatting looks good.
No issues with the consolidated import.
461-463: LGTM.
Formatting-only change; assertion remains correct.
468-470: LGTM.
Formatting-only change; assertion remains correct.
481-481: No-op formatting.
Nothing to address.
19-19: Type import path updated to ../types — LGTM. No remaining imports of BridgingSdkConfig from paths containing "BridgingSdk"; current imports reference './types' or '../types' and BridgingSdkConfig is exported at packages/bridging/src/BridgingSdk/types.ts:71.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts (1)
221-223: Minor: Consider extracting repeated BigInt comparisons.The test code has repeated BigInt amount comparisons that could be extracted to helper functions for better readability, though this is optional since the tests are clear as written.
// Optional helper to improve test readability const expectBuyAmount = (result: MultiQuoteResult | null, expectedAmount: string) => { expect(result?.quote?.bridge.amountsAndCosts.afterSlippage.buyAmount).toBe(BigInt(expectedAmount)) } // Usage example: expectBuyAmount(progressiveResults[0], '50000000000000000000') expectBuyAmount(progressiveResults[1], '70000000000000000000')Also applies to: 461-463, 468-470
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/bridging/src/BridgingSdk/BridgingSdk.ts(8 hunks)packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts(8 hunks)packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.ts(4 hunks)packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.test.ts(6 hunks)packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.ts(3 hunks)packages/bridging/src/BridgingSdk/strategies/QuoteStrategy.ts(2 hunks)packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.test.ts(7 hunks)packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.ts(2 hunks)packages/bridging/src/BridgingSdk/strategies/createStrategies.ts(1 hunks)packages/bridging/src/types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.test.ts
- packages/bridging/src/types.ts
- packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T16:17:50.800Z
Learnt from: cowdan
PR: cowprotocol/cow-sdk#492
File: packages/bridging/src/providers/bungee/BungeeApi.ts:393-395
Timestamp: 2025-09-12T16:17:50.800Z
Learning: In the cowprotocol/cow-sdk bridging package, tests for BungeeApi are designed to hit real public Bungee endpoints without affiliate headers for integration testing, rather than using mocked network calls. This approach is preferred by the maintainers as it provides real API integration coverage.
Applied to files:
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts
🧬 Code graph analysis (7)
packages/bridging/src/BridgingSdk/strategies/createStrategies.ts (5)
packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-280)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.ts (1)
SingleQuoteStrategy(12-58)packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.ts (1)
MultiQuoteStrategy(24-144)packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.ts (1)
BestQuoteStrategy(23-130)
packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.ts (3)
packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-280)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (1)
getQuoteWithBridge(30-229)
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts (1)
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.ts (1)
BestQuoteStrategy(23-130)
packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.ts (3)
packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-280)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (1)
getQuoteWithBridge(30-229)
packages/bridging/src/BridgingSdk/strategies/QuoteStrategy.ts (4)
packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-280)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/BridgingSdk/types.ts (1)
BridgingSdkConfig(71-71)packages/bridging/src/types.ts (2)
CrossChainQuoteAndPost(288-288)MultiQuoteResult(411-415)
packages/bridging/src/BridgingSdk/BridgingSdk.ts (6)
packages/bridging/src/BridgingSdk/types.ts (2)
BridgingSdkCacheConfig(76-89)BridgingSdkConfig(71-71)packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-280)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/types.ts (1)
GetProviderBuyTokens(138-141)packages/bridging/src/BridgingSdk/strategies/createStrategies.ts (1)
createStrategies(7-13)packages/bridging/src/BridgingSdk/helpers.ts (1)
getCacheKey(1-13)
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.ts (3)
packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-280)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (1)
getQuoteWithBridge(30-229)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: eslint
- GitHub Check: test
- GitHub Check: Build Package
🔇 Additional comments (16)
packages/bridging/src/BridgingSdk/strategies/createStrategies.ts (1)
1-13: LGTM! Clean factory function implementation.The factory function correctly instantiates all three strategy types with consistent cache parameters, providing a clean abstraction for creating cache-enabled strategies. The imports are well-organized and the function signature is clear.
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.ts (2)
1-28: Well-structured refactoring to support caching.The class properly extends the base class and correctly accepts optional caching parameters in the constructor. The imports are well-organized and include all necessary dependencies for the caching functionality.
81-96: Good conditional request construction pattern.The implementation correctly builds a base parameters object and conditionally adds caching-related fields only when both cache and TTL are provided. This ensures proper integration with the caching system while maintaining backward compatibility.
packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.ts (2)
1-29: Consistent refactoring across strategy classes.The MultiQuoteStrategy follows the same pattern as BestQuoteStrategy with proper inheritance from the base class and correct caching parameter handling. The consistency across strategy implementations is good for maintainability.
95-110: Proper cache integration pattern.The same conditional request construction pattern is correctly implemented here, ensuring consistent cache behavior across all strategy types.
packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.ts (2)
1-17: Clean integration with caching infrastructure.The SingleQuoteStrategy correctly implements the same caching pattern as the other strategies, maintaining consistency across the strategy hierarchy.
31-48: Appropriate conditional cache usage for cross-chain flows.The caching is only applied to cross-chain swaps (when
sellTokenChainId !== buyTokenChainId), which makes sense since single-chain swaps likely don't need intermediate token caching. The conditional request construction follows the established pattern.packages/bridging/src/BridgingSdk/strategies/QuoteStrategy.ts (2)
2-3: Good architectural foundation for caching.The conversion from interfaces to abstract base classes provides a clean way to enforce the caching parameter signature across all strategy implementations. This ensures consistent constructor patterns while maintaining the abstract contract.
Also applies to: 17-27
52-68: Well-defined abstract strategy hierarchy.The abstract base classes properly define the expected strategy names and maintain clear separation of concerns. Each base class correctly specifies its expected request and result types.
packages/bridging/src/BridgingSdk/BridgingSdk.ts (6)
25-30: Reasonable default cache configuration.The default TTL values (5 minutes for intermediate tokens, 2 minutes for buy tokens) seem appropriate for bridging scenarios where token availability and routing can change but not too frequently.
37-42: Clean separation of cache and strategy state.The private fields are well-organized and properly typed. The separation between cache instances and strategy instances provides clear architectural boundaries.
73-95: Well-structured cache initialization.The cache initialization correctly uses the merged configuration and passes appropriate parameters to the TTLCache constructor. The strategy factory integration is clean and maintains the caching parameter flow.
137-152: Proper cache key generation and usage.The cache key generation uses the helper function correctly with normalized address casing. The conditional caching logic properly respects the enabled flag.
174-181: Clean strategy delegation.The quote methods properly delegate to the strategy instances created by the factory, maintaining clean separation of concerns and consistent caching behavior.
Also applies to: 199-199, 218-218
243-267: Useful cache management APIs.The public cache management methods provide good utilities for testing, debugging, and maintenance. The API is clean and self-explanatory.
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts (1)
1-1: Import paths and class references are consistent — approved
BestQuoteStrategy is exported (packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.ts) and re-exported (index.ts); imports in createStrategies.ts and BestQuoteStrategy.test.ts reference the same symbol.
|
@cowdan could you resolve rabbit comments please? |
|
@coderabbitai can you summarise a list of issues still open, considering the latest commit and your comments on this PR? |
🧩 Analysis chainBased on my analysis of the PR and checking for my previous comments, here's the current status: @cowdan I've reviewed the PR and my previous comments. Here are the open issues that still need attention: Currently Open Issues:
Recent Changes Reviewed:The latest commits show substantial caching functionality has been implemented including:
Recommendations for Final Review:Since I don't see any outstanding unresolved comments from my previous reviews, I recommend:
Would you like me to perform a fresh comprehensive review of all the changes to ensure everything is ready for merge? 🏁 Script executed: Length of output: 29922 🏁 Script executed: Length of output: 106259 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 46 🏁 Script executed: Length of output: 0 @cowdan Based on my analysis of the PR reviews and the latest commit, here's a summary of the remaining open issues: 🔴 Critical Issues Still Open1. TTLCache Implementation Gaps
2. BigInt Conversion Runtime Risk
🟡 Medium Priority Issues3. Cache Integration Inconsistencies
4. Test Coverage Gaps
🟢 Minor Issues5. Type Definition Duplicates
6. Cache Key Normalization
Recommendations
The caching feature is functionally working but has some edge case reliability issues that should be addressed before merge. Would you like me to provide specific code fixes for any of these remaining issues? 🏁 Script executed: Length of output: 23958 🏁 Script executed: Length of output: 4148 🏁 Script executed: Length of output: 441 Based on my comprehensive analysis of the PR reviews and the latest commit "chore: adapt cache to quote strategies", here's the updated summary of remaining open issues: @cowdan Here are the remaining open issues considering the latest commit and my review history: 🔴 Critical Issues Still Outstanding1. TTLCache Memory Fallback Bug
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.test.ts (1)
128-129: Use BigInt exponentiation to avoid precision loss/RangeErrorBigInt(100 * 10 ** decimals) can lose precision or throw for large decimals. Use 10n ** BigInt(decimals).
- amount: BigInt(100 * 10 ** intermediateTokenDecimals).toString(), + amount: (100n * (10n ** BigInt(intermediateTokenDecimals))).toString(),packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (1)
111-114: Fix unsafe BigInt power‑of‑ten computationBigInt(10 ** decimals) goes through Number math (precision/Infinity risks). Use native BigInt exponentiation.
- log( - `Expected to receive ${intermediateTokenAmount} of the intermediate token (${(intermediateTokenAmount / BigInt(10 ** intermediaryTokenDecimals)).toString()} formatted)`, - ) + log( + `Expected to receive ${intermediateTokenAmount} of the intermediate token (${(intermediateTokenAmount / (10n ** BigInt(intermediaryTokenDecimals))).toString()} formatted)`, + )
🧹 Nitpick comments (23)
packages/common/src/utils/ttlCache.ts (1)
169-181: Optional: keep key index consistent in get() when evictingWhen evicting expired/corrupted entries, also drop them from the key index.
if (now - entry.timestamp > entry.ttl) { // Entry has expired, remove it this.storage.removeItem(storageKey) + this.keys.delete(storageKey) return undefined } @@ console.warn('TTLCache: Failed to parse cache entry', error) // Remove corrupted entry this.storage.removeItem(storageKey) + this.keys.delete(storageKey) return undefinedpackages/bridging/src/types.ts (4)
451-456: Avoid duplicating MultiQuoteRequest definitionComment says it duplicates QuoteStrategy.ts. Prefer a single source of truth to prevent drift.
Proposed approach:
- Export MultiQuoteRequest from strategies/QuoteStrategy.ts and import it here.
- Or extract it to a shared types module used by both.
144-149: Fix typos in docs: “provideer” → “provider”Minor text quality polish.
- * A bridge deposit. It includes the provideer information, sell amount and the minimum buy amount. + * A bridge deposit. It includes the provider information, sell amount and the minimum buy amount.
217-223: Fix typo in docs: “initate” → “initiate”- * The hook contains the ethereum call that the trampoline contract will need to execute during the settlement to initate the bridge. + * The hook contains the ethereum call that the trampoline contract will need to execute during the settlement to initiate the bridge.
387-399: Optional: tighten types for chain IDsConsider using ChainId/SupportedChainId instead of number for stronger type safety in BridgingDepositParams.
packages/bridging/src/BridgingSdk/strategies/createStrategies.ts (1)
7-13: Prefer passing a single options object to strategy constructorsTwo positional params are easy to misorder and will grow if we add more knobs. Consider
new SingleQuoteStrategy({ intermediateTokensCache, intermediateTokensTtl })for future-proofing and readability.- singleQuoteStrategy: new SingleQuoteStrategy(intermediateTokensCache, intermediateTokensTtl), - multiQuoteStrategy: new MultiQuoteStrategy(intermediateTokensCache, intermediateTokensTtl), - bestQuoteStrategy: new BestQuoteStrategy(intermediateTokensCache, intermediateTokensTtl), + singleQuoteStrategy: new SingleQuoteStrategy({ intermediateTokensCache, intermediateTokensTtl }), + multiQuoteStrategy: new MultiQuoteStrategy({ intermediateTokensCache, intermediateTokensTtl }), + bestQuoteStrategy: new BestQuoteStrategy({ intermediateTokensCache, intermediateTokensTtl }),packages/bridging/src/BridgingSdk/helpers.ts (1)
1-13: Make chain IDs robust to number|string and normalize inputsTests pass numeric chainIds; the helper expects strings. Also, make normalization explicit and resilient.
-export const getCacheKey = ({ - id, - buyChainId, - sellChainId = 'noSellChainID', - tokenAddress = 'noTokenAddress', -}: { - id: string - buyChainId: string - sellChainId?: string - tokenAddress?: string -}) => { - return `${id}-${buyChainId}-${sellChainId}-${tokenAddress.toLowerCase()}` -} +export const getCacheKey = ({ + id, + buyChainId, + sellChainId = 'noSellChainID', + tokenAddress = 'noTokenAddress', +}: { + id: string + buyChainId: string | number + sellChainId?: string | number + tokenAddress?: string +}) => { + const buy = String(buyChainId) + const sell = sellChainId == null ? 'noSellChainID' : String(sellChainId) + const addr = String(tokenAddress).toLowerCase() + // Note: hyphen-delimited; if ambiguity becomes an issue, consider JSON.stringify([id,buy,sell,addr]) + return `${id}-${buy}-${sell}-${addr}` +}packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.test.ts (1)
247-282: CI flakiness guard for timing assertionsThe parallelism assertions with <50ms and <180ms margins can be flaky under CI load. Consider widening thresholds or using fake timers to stabilize.
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.ts (1)
81-97: Always pass cache object when provided; decide on TTL=0 semanticsCurrent guard requires both cache and ttl to be truthy. If TTL=0 should intentionally disable caching, this is fine. If TTL=0 should mean “use cache with immediate expiry”, the condition will unexpectedly drop the cache. Clarify semantics or adjust the guard.
- const request = - this.intermediateTokensCache && this.intermediateTokensTtl + const request = + this.intermediateTokensCache != null && this.intermediateTokensTtl != null ? { ...baseParams, intermediateTokensCache: this.intermediateTokensCache, intermediateTokensTtl: this.intermediateTokensTtl, } : baseParamspackages/bridging/src/BridgingSdk/getQuoteWithBridge.test.ts (2)
101-109: Test helper ignores passed provider/tradingSdkUse the passed-in values to avoid hidden coupling to outer mocks, or narrow the helper’s type.
- async function postOrderWithIntermediateTokensCache(request: GetQuoteWithBridgeParams<BridgeQuoteResult>) { - return getQuoteWithBridge({ - swapAndBridgeRequest: request.swapAndBridgeRequest, - provider: mockProvider, - tradingSdk, - intermediateTokensCache: request.intermediateTokensCache, - intermediateTokensTtl: request.intermediateTokensTtl, - }) - } + async function postOrderWithIntermediateTokensCache(request: GetQuoteWithBridgeParams<BridgeQuoteResult>) { + return getQuoteWithBridge({ + swapAndBridgeRequest: request.swapAndBridgeRequest, + provider: request.provider ?? mockProvider, + tradingSdk: request.tradingSdk ?? tradingSdk, + intermediateTokensCache: request.intermediateTokensCache, + intermediateTokensTtl: request.intermediateTokensTtl, + }) + }
207-221: Assert validTo override explicitlyCurrently only checks that a second call exists. Validate the propagated validTo on the second bridge call.
await postSwapOrderFromQuote({ quoteRequest: { validTo: customValidTo, }, }) - expect(getQuoteMock).toHaveBeenCalledTimes(2) - // Verify the validTo is passed correctly to the bridge request - expect(getUnsignedBridgeCallMock.mock.calls[1]).toBeDefined() + expect(getQuoteMock).toHaveBeenCalledTimes(2) + // Verify validTo reached the bridge call inputs + const secondBridgeCallArgs = getUnsignedBridgeCallMock.mock.calls[1]?.[0] + expect(secondBridgeCallArgs).toBeDefined() + expect(secondBridgeCallArgs.validTo).toBe(customValidTo)packages/bridging/src/BridgingSdk/types.ts (1)
71-116: Cache config API is clear; consider documenting semantics around TTL=0Docs say “TTL in milliseconds”; specify behavior for 0 or negative values (disable vs immediate-expiry) to avoid ambiguity across call sites.
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts (3)
28-39: Consider adding a test path with caching enabledInstantiate strategy with an in‑memory TTLCache and assert cache usage across two executes to cover the new caching integration.
221-223: Test title vs assertion mismatchThe title says “first provider error” but assertion allows any provider. Rename the test for clarity.
- it('should return first provider error when all providers fail', async () => { + it('should return one provider error when all providers fail', async () => {
349-351: Reduce flakiness in timeout assertionTiming in CI can drift. Loosen the threshold or use fake timers.
- expect(elapsed).toBeLessThan(150) // Should not wait for slow providers + expect(elapsed).toBeLessThan(250) // Should not wait for slow providerspackages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.ts (1)
95-111: Deduplicate request construction across strategiesThis “add cache fields if present” block is repeated in Single/Multi/Best. Consider a protected helper in the base class to attach cache fields to a base request to keep wiring consistent.
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (2)
231-233: Typo in comment- * Ge the base params (without the amount) for the bridge quote request + * Get the base params (without the amount) for the bridge quote request
267-269: Improve log readability for token objectLog address/symbol instead of [object Object].
- log(`Using ${intermediateToken} as intermediate tokens`) + log(`Using intermediate token: ${intermediateToken.symbol || ''} ${intermediateToken.address}`)packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.ts (1)
31-47: Extract “attach cache params” helper to baseSame conditional spread appears in Multi and Best strategies. Factor into a protected utility to reduce drift.
packages/bridging/src/BridgingSdk/strategies/QuoteStrategy.ts (1)
70-77: Potentially orphan/duplicated ProviderQuoteContextA ProviderQuoteContext is also defined/used elsewhere with different shape. Consider removing/renaming this one to avoid confusion.
packages/bridging/src/BridgingSdk/BridgingSdk.ts (3)
76-87: Ensure caching is fully disabled when enabled=false (avoid in‑memory caching via strategies).If TTLCache’s second arg only toggles persistence (common pattern), strategies will still cache in memory. Pass the cache to strategies only when enabled.
Apply this diff (adjust createStrategies signature to accept undefineds if needed):
- const { singleQuoteStrategy, multiQuoteStrategy, bestQuoteStrategy } = createStrategies( - this.intermediateTokensCache, - this.cacheConfig.intermediateTokensTtl, - ) + const cachingEnabled = this.cacheConfig.enabled + const cacheForStrategies = cachingEnabled ? this.intermediateTokensCache : undefined + const ttlForStrategies = cachingEnabled ? this.cacheConfig.intermediateTokensTtl : undefined + const { singleQuoteStrategy, multiQuoteStrategy, bestQuoteStrategy } = createStrategies( + cacheForStrategies, + ttlForStrategies, + )If TTLCache already disables in‑memory when enabled=false, ignore this. Please confirm TTLCache semantics in @cowprotocol/sdk-common.
Also applies to: 88-91
135-152: Avoid double cache lookup and short‑circuit when disabled.Minor perf/readability improvement and avoids computing keys when caching is off.
Apply this diff:
async getBuyTokens(params: BuyTokensParams): Promise<GetProviderBuyTokens> { const providerId = this.provider.info.dappId const cacheKey = getCacheKey({ id: providerId, buyChainId: params.buyChainId.toString(), sellChainId: params.sellChainId?.toString(), tokenAddress: params.sellTokenAddress, }) - if (this.cacheConfig.enabled && this.buyTokensCache.get(cacheKey)) { - return this.buyTokensCache.get(cacheKey) as GetProviderBuyTokens - } + if (!this.cacheConfig.enabled) { + return this.provider.getBuyTokens(params) + } + const cached = this.buyTokensCache.get(cacheKey) + if (cached) return cached as GetProviderBuyTokens const result = await this.provider.getBuyTokens(params) - if (this.cacheConfig.enabled) { - this.buyTokensCache.set(cacheKey, result) - } + this.buyTokensCache.set(cacheKey, result) return result }Also verify getCacheKey normalizes undefined fields consistently (e.g., sellChainId missing) to avoid accidental collisions.
262-267: Cache stats may underreport in fallback modes.If TTLCache.size() doesn’t enumerate fallback storage in SSR/Safari private mode, stats could mislead. Consider documenting this or exposing a more robust stats method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
packages/bridging/src/BridgingSdk/BridgingSdk.test.ts(3 hunks)packages/bridging/src/BridgingSdk/BridgingSdk.ts(8 hunks)packages/bridging/src/BridgingSdk/getQuoteWithBridge.test.ts(6 hunks)packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts(4 hunks)packages/bridging/src/BridgingSdk/helpers.ts(1 hunks)packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts(8 hunks)packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.ts(4 hunks)packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.test.ts(6 hunks)packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.ts(3 hunks)packages/bridging/src/BridgingSdk/strategies/QuoteStrategy.ts(2 hunks)packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.test.ts(7 hunks)packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.ts(2 hunks)packages/bridging/src/BridgingSdk/strategies/createStrategies.ts(1 hunks)packages/bridging/src/BridgingSdk/types.ts(3 hunks)packages/bridging/src/BridgingSdk/utils.ts(1 hunks)packages/bridging/src/types.ts(2 hunks)packages/common/src/utils/index.ts(1 hunks)packages/common/src/utils/ttlCache.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-05-30T05:56:12.625Z
Learnt from: shoom3301
PR: cowprotocol/cow-sdk#324
File: src/bridging/types.ts:384-385
Timestamp: 2025-05-30T05:56:12.625Z
Learning: The BridgingDepositParams interface in src/bridging/types.ts includes a bridgingId: string field along with other deposit parameters like inputTokenAddress, outputTokenAddress, inputAmount, outputAmount, owner, quoteTimestamp, fillDeadline, recipient, sourceChainId, and destinationChainId.
Applied to files:
packages/bridging/src/types.tspackages/bridging/src/BridgingSdk/types.ts
📚 Learning: 2025-09-12T16:17:50.800Z
Learnt from: cowdan
PR: cowprotocol/cow-sdk#492
File: packages/bridging/src/providers/bungee/BungeeApi.ts:393-395
Timestamp: 2025-09-12T16:17:50.800Z
Learning: In the cowprotocol/cow-sdk bridging package, tests for BungeeApi are designed to hit real public Bungee endpoints without affiliate headers for integration testing, rather than using mocked network calls. This approach is preferred by the maintainers as it provides real API integration coverage.
Applied to files:
packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.test.tspackages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts
📚 Learning: 2025-03-19T22:06:52.139Z
Learnt from: anxolin
PR: cowprotocol/cow-sdk#251
File: src/bridging/BridgingSdk/getQuoteWithBridging.ts:34-35
Timestamp: 2025-03-19T22:06:52.139Z
Learning: The implementation in the Across bridge provider deliberately selects only the first intermediate token from the list (`intermediateTokens[0]`) as a simplification for the initial implementation, with potential for future enhancement.
Applied to files:
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts
📚 Learning: 2025-03-20T09:45:27.965Z
Learnt from: anxolin
PR: cowprotocol/cow-sdk#251
File: src/bridging/providers/across/AcrossBridgeProvider.ts:85-100
Timestamp: 2025-03-20T09:45:27.965Z
Learning: In the AcrossBridgeProvider's getIntermediateTokens method, not finding an intermediate token is expected behavior in some cases and should not be logged as a warning.
Applied to files:
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts
🔇 Additional comments (34)
packages/common/src/utils/ttlCache.ts (7)
121-134: isMemoryStorage flag is brittle; prefer computing from the actual backendThe flag can desynchronize from the real backend (wrapper fallback). Either drop the setter and compute
this.storage instanceof MemoryStorageinline where needed, or keep the flag internal-only and ensure it mirrors backend changes.
195-216: Fix: clear() ignores LocalStorageWrapper’s in‑memory fallback; entries leak in SSR/Safari Private Modeclear() only handles real localStorage or raw MemoryStorage. When LocalStorageWrapper falls back to its internal memory, these entries aren’t cleared.
Apply this diff to clear both localStorage and the wrapper’s fallback via an instance key index:
clear(): void { - if (this.isMemoryStorage) { - ;(this.storage as MemoryStorage).clear(this.keyPrefix + ':') - return - } - - if (typeof localStorage !== 'undefined') { + const prefix = this.keyPrefix + ':' + if (this.isMemoryStorage && this.storage instanceof MemoryStorage) { + ;(this.storage as MemoryStorage).clear(prefix) + this.keys.clear() + return + } + + if (typeof localStorage !== 'undefined') { try { const keysToDelete: string[] = [] for (let i = 0; i < localStorage.length; i++) { const key = localStorage.key(i) - if (key && key.startsWith(this.keyPrefix + ':')) { + if (key && key.startsWith(prefix)) { keysToDelete.push(key) } } keysToDelete.forEach((key) => localStorage.removeItem(key)) } catch { // Fallback: If localStorage access fails, we can't clear it console.warn('TTLCache: Failed to clear localStorage cache') } } + // Always clear instance-tracked keys (covers LocalStorageWrapper fallback/SSR) + for (const key of Array.from(this.keys)) { + try { + this.storage.removeItem(key) + } catch { + // noop + } + this.keys.delete(key) + } }
221-241: Fix: size() misses wrapper fallback; returns wrong counts in SSR/Private Modesize() scans localStorage or MemoryStorage only. Wrapper fallback keys aren’t counted.
size(): number { - if (this.isMemoryStorage) { - return (this.storage as MemoryStorage).size(this.keyPrefix + ':') - } - - if (typeof localStorage !== 'undefined') { + const prefix = this.keyPrefix + ':' + if (this.isMemoryStorage && this.storage instanceof MemoryStorage) { + return (this.storage as MemoryStorage).size(prefix) + } + // In SSR or wrapper-fallback scenarios, rely on the per-instance index + if (typeof localStorage === 'undefined') { + return this.keys.size + } + + if (typeof localStorage !== 'undefined') { try { let count = 0 for (let i = 0; i < localStorage.length; i++) { const key = localStorage.key(i) - if (key && key.startsWith(this.keyPrefix + ':')) { + if (key && key.startsWith(prefix)) { count++ } } return count } catch { return 0 } } return 0 }
246-275: Fix: cleanup() doesn’t purge wrapper-fallback or SSR entriesOnly localStorage is scanned; in-memory fallback entries persist past TTL.
cleanup(): void { - if (typeof localStorage !== 'undefined') { + const prefix = this.keyPrefix + ':' + if (typeof localStorage !== 'undefined') { try { const keysToDelete: string[] = [] const now = Date.now() for (let i = 0; i < localStorage.length; i++) { const key = localStorage.key(i) - if (key && key.startsWith(this.keyPrefix + ':')) { + if (key && key.startsWith(prefix)) { const item = localStorage.getItem(key) if (item) { try { const entry: CacheEntry<T> = JSON.parse(item) if (now - entry.timestamp > entry.ttl) { keysToDelete.push(key) } } catch { // Remove corrupted entries keysToDelete.push(key) } } } } keysToDelete.forEach((key) => localStorage.removeItem(key)) } catch { console.warn('TTLCache: Failed to cleanup expired entries') } } + // Also clean instance-tracked keys (covers LocalStorageWrapper fallback/SSR) + const now = Date.now() + for (const key of Array.from(this.keys)) { + const item = this.storage.getItem(key) + if (!item) { + this.keys.delete(key) + continue + } + try { + const entry: CacheEntry<T> = JSON.parse(item) + if (now - entry.timestamp > entry.ttl) { + this.storage.removeItem(key) + this.keys.delete(key) + } + } catch { + // Corrupted entry + this.storage.removeItem(key) + this.keys.delete(key) + } + } }
139-152: Track keys and ignore non-positive TTLs in set()Needed to support wrapper fallback management and to avoid storing disabled/expired entries.
set(key: string, value: T): void { + if (this.ttl <= 0) return const entry: CacheEntry<T> = { value, timestamp: Date.now(), ttl: this.ttl, } const storageKey = this.getStorageKey(key) try { this.storage.setItem(storageKey, JSON.stringify(entry)) } catch (error) { console.warn('TTLCache: Failed to store cache entry', error) } + this.keys.add(storageKey) }
187-190: Delete must also update the key indexPrevents stale accounting when removing specific entries.
delete(key: string): void { const storageKey = this.getStorageKey(key) - this.storage.removeItem(storageKey) + this.storage.removeItem(storageKey) + this.keys.delete(storageKey) }
107-112: Add per-instance key index to cover LocalStorageWrapper fallback/SSRThis enables clear/size/cleanup to work regardless of backend.
export class TTLCache<T> { private storage: CacheStorage private keyPrefix: string private ttl: number private _isMemoryStorage: boolean = false + // Track keys written by this instance (covers LocalStorageWrapper fallback/SSR) + private keys = new Set<string>()packages/common/src/utils/index.ts (1)
6-6: LGTM: Re-exporting TTLCache from utilsMakes TTLCache available to consumers via sdk-common.
packages/bridging/src/BridgingSdk/utils.ts (1)
10-10: LGTM: Import path aligned with new public types surfaceBridgingSdkConfig import from ./types matches the reorg.
packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.test.ts (1)
1-1: LGTM: Strategy class importTest now targets SingleQuoteStrategy directly; consistent with the refactor.
packages/bridging/src/types.ts (1)
14-14: TTLCache export verified — no action requiredConfirmed: TTLCache is declared in packages/common/src/utils/ttlCache.ts and re-exported via packages/common/src/utils/index.ts and packages/common/src/index.ts.
packages/bridging/src/BridgingSdk/strategies/createStrategies.ts (1)
7-13: Factory wiring looks goodThe strategies are correctly instantiated with the injected cache and TTL. This centralizes construction and keeps the call sites clean.
packages/bridging/src/BridgingSdk/BridgingSdk.test.ts (4)
200-238: Fix mock shape for getBuyTokens in TTL testProvider returns
{ tokens, isRouteAvailable }, not an array. Align mock to contract.- const mockIntermediateTokens = [ - { address: '0x123', name: 'Token1', symbol: 'TK1', decimals: 18, chainId: 137 }, - { address: '0x456', name: 'Token2', symbol: 'TK2', decimals: 6, chainId: 137 }, - ] - mockProvider.getBuyTokens = jest.fn().mockResolvedValue(mockIntermediateTokens) + const mockBuyTokens = { + tokens: [ + { address: '0x123', name: 'Token1', symbol: 'TK1', decimals: 18, chainId: 137 }, + { address: '0x456', name: 'Token2', symbol: 'TK2', decimals: 6, chainId: 137 }, + ], + isRouteAvailable: true, + } + mockProvider.getBuyTokens = jest.fn().mockResolvedValue(mockBuyTokens)
311-346: Test doesn’t verify cleanup — re-query after cleanupYou never call getBuyTokens after cleanup; add a post-cleanup call and assert a second provider hit.
// Cleanup expired entries shortTtlSdk.cleanupExpiredCache() - // Cache should be cleaned up (note: localStorage cleanup is async and may not reflect immediately in size()) - // So we test that expired entries are not returned - expect(mockProvider.getBuyTokens).toHaveBeenCalledTimes(1) // Should call provider again + // Next call should miss cache and hit provider again + await shortTtlSdk.getBuyTokens({ buyChainId: 137 as TargetChainId }) + expect(mockProvider.getBuyTokens).toHaveBeenCalledTimes(2)
293-309: Fix mock shape for default cache-config testReturn
{ tokens, isRouteAvailable }.- const mockBuyTokens = [{ address: '0x123', name: 'Token1', symbol: 'TK1', decimals: 18, chainId: 137 }] - mockProvider.getBuyTokens = jest.fn().mockResolvedValue(mockBuyTokens) + const mockBuyTokens = { + tokens: [{ address: '0x123', name: 'Token1', symbol: 'TK1', decimals: 18, chainId: 137 }], + isRouteAvailable: true, + } + mockProvider.getBuyTokens = jest.fn().mockResolvedValue(mockBuyTokens)
348-366: Fix mock shape for persistence testReturn
{ tokens, isRouteAvailable }.- const mockBuyTokens = [{ address: '0x123', name: 'Token1', symbol: 'TK1', decimals: 18, chainId: 137 }] - mockProvider.getBuyTokens = jest.fn().mockResolvedValue(mockBuyTokens) + const mockBuyTokens = { + tokens: [{ address: '0x123', name: 'Token1', symbol: 'TK1', decimals: 18, chainId: 137 }], + isRouteAvailable: true, + } + mockProvider.getBuyTokens = jest.fn().mockResolvedValue(mockBuyTokens)packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.test.ts (1)
1-1: LGTM: public API rename reflected in testsImporting and instantiating MultiQuoteStrategy directly aligns with the new surface; tests remain focused on behavior.
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.ts (1)
23-28: Constructor forwarding for cache is cleanForwarding cache and TTL to the base strategy is straightforward and keeps this class focused on “best” logic.
packages/bridging/src/BridgingSdk/getQuoteWithBridge.test.ts (1)
222-246: Intermediate-token caching test is solidGood coverage ensuring cache hits skip provider calls within TTL.
packages/bridging/src/BridgingSdk/types.ts (1)
15-51: Public types extended correctly for cache supportAdding intermediateTokensCache/TTL to GetQuoteWithBridgeParams is consistent with strategy wiring and tests.
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts (1)
1-1: Public symbol switch to BestQuoteStrategy looks goodImport path/name change aligns with the implementation rename.
packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.ts (1)
24-29: Constructor‑based cache injection is appropriateCleanly threads cache/TTL to the base. No issues.
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (2)
56-63: Plumbing cache options through getBaseBridgeQuoteRequest looks goodThe params are forwarded correctly.
244-259: Use one cache.get(); TTL is set on TTLCache construction (don't pass TTL into set)
- TTLCache.set(key, value) has signature set(key: string, value: T) — TTL is taken from the TTLCache instance (constructor third arg). Do not pass a per-set TTL.
- Avoid double get(): call intermediateTokensCache?.get(cacheKey) once and reuse the result.
Suggested replacement (packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts, ~lines 244-259):
const cached = intermediateTokensCache?.get(cacheKey) as TokenInfo[] | undefined
if (cached) {
intermediateTokens = cached
} else {
intermediateTokens = await provider.getIntermediateTokens(quoteBridgeRequest)
if (intermediateTokensCache && intermediateTokensTtl) {
intermediateTokensCache.set(cacheKey, intermediateTokens)
}
}If you need per-call TTL, construct the TTLCache with that TTL or extend TTLCache to accept per-set TTL.
Likely an incorrect or invalid review comment.
packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.ts (1)
12-17: Class rename + cache injection LGTMConstructor cleanly passes cache/TTL to base.
packages/bridging/src/BridgingSdk/strategies/QuoteStrategy.ts (1)
17-27: Abstract base with cache constructor is a good directionEnables consistent DI of caches across strategies.
packages/bridging/src/BridgingSdk/BridgingSdk.ts (8)
18-23: Imports and layering look good.Consolidates helpers/strategies and pulls TTLCache from common with correct generics.
25-29: Sensible cache defaults.Defaults are reasonable for UX and testability.
37-42: State wiring is coherent.Private cache/strategy fields are clear and typed.
174-180: Strategy delegation LGTM.Clean handoff to SingleQuoteStrategy with config passed separately.
199-199: Multi-quote strategy wiring LGTM.
218-219: Best-quote strategy wiring LGTM.
243-249: Clear cache API LGTM.Good for tests and debugging.
251-257: Expired-entry cleanup LGTM.Ensure TTLCache.cleanup() also handles any non-persistent fallback mode paths (SSR/private mode).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/common/src/utils/ttlCache.ts (1)
115-136: Simplify memory-backend detection (optional)The mutable _isMemoryStorage flag plus instanceof checks can drift. Prefer a computed getter based solely on the backend, or expose a method (e.g., usingMemoryBackend()) that checks storage type.
Happy to draft a concise refactor if you want to remove the setter and rely on storage instanceof checks only.
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (2)
244-259: Avoid double get(); rely on cache instance TTL instead of separate flagMinor cleanups: call get() once; let TTLCache.set() decide based on its configured ttl (it no-ops when ttl <= 0). This removes the need to gate on intermediateTokensTtl here.
- const cacheKey = getCacheKey({ + const cacheKey = getCacheKey({ id: provider.info.dappId, buyChainId: quoteBridgeRequest.buyTokenChainId.toString(), sellChainId: quoteBridgeRequest.sellTokenChainId.toString(), tokenAddress: quoteBridgeRequest.buyTokenAddress, }) - if (intermediateTokensCache && intermediateTokensCache.get(cacheKey)) { - intermediateTokens = intermediateTokensCache.get(cacheKey) as TokenInfo[] - } else { - intermediateTokens = await provider.getIntermediateTokens(quoteBridgeRequest) - - if (intermediateTokensCache && intermediateTokensTtl) { - intermediateTokensCache.set(cacheKey, intermediateTokens) - } - } + const cached = intermediateTokensCache?.get(cacheKey) + if (cached) { + intermediateTokens = cached + } else { + intermediateTokens = await provider.getIntermediateTokens(quoteBridgeRequest) + intermediateTokensCache?.set(cacheKey, intermediateTokens) + }
265-268: Log a useful identifier for the intermediate tokenStringifying the object prints [object Object]. Log symbol or address.
- log(`Using ${intermediateToken} as intermediate tokens`) + log( + `Using ${intermediateToken.symbol ?? intermediateToken.address} as intermediate token`, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/bridging/src/BridgingSdk/BridgingSdk.test.ts(3 hunks)packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts(5 hunks)packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.test.ts(8 hunks)packages/common/src/utils/ttlCache.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.test.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-19T22:06:52.139Z
Learnt from: anxolin
PR: cowprotocol/cow-sdk#251
File: src/bridging/BridgingSdk/getQuoteWithBridging.ts:34-35
Timestamp: 2025-03-19T22:06:52.139Z
Learning: The implementation in the Across bridge provider deliberately selects only the first intermediate token from the list (`intermediateTokens[0]`) as a simplification for the initial implementation, with potential for future enhancement.
Applied to files:
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts
📚 Learning: 2025-03-20T09:45:27.965Z
Learnt from: anxolin
PR: cowprotocol/cow-sdk#251
File: src/bridging/providers/across/AcrossBridgeProvider.ts:85-100
Timestamp: 2025-03-20T09:45:27.965Z
Learning: In the AcrossBridgeProvider's getIntermediateTokens method, not finding an intermediate token is expected behavior in some cases and should not be logged as a warning.
Applied to files:
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts
🧬 Code graph analysis (2)
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (3)
packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-327)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/BridgingSdk/helpers.ts (1)
getCacheKey(1-13)
packages/bridging/src/BridgingSdk/BridgingSdk.test.ts (2)
packages/config/src/chains/types.ts (1)
TargetChainId(34-34)packages/bridging/src/BridgingSdk/BridgingSdk.ts (1)
BridgingSdk(35-272)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: eslint
🔇 Additional comments (8)
packages/bridging/src/BridgingSdk/BridgingSdk.test.ts (5)
48-51: LGTM: localStorage cleared per testPrevents cross-test pollution when running under jsdom.
343-346: LGTM: post-cleanup re-query validates evictionThis properly verifies cleanup() behavior.
216-221: Mock shape mismatch: getBuyTokens should resolve to { tokens, isRouteAvailable }Tests currently mock an array; provider contract returns an object. Align to avoid false positives.
- const mockIntermediateTokens = [ - { address: '0x123', name: 'Token1', symbol: 'TK1', decimals: 18, chainId: 137 }, - { address: '0x456', name: 'Token2', symbol: 'TK2', decimals: 6, chainId: 137 }, - ] - mockProvider.getBuyTokens = jest.fn().mockResolvedValue(mockIntermediateTokens) + const mockBuyTokens = { + tokens: [ + { address: '0x123', name: 'Token1', symbol: 'TK1', decimals: 18, chainId: 137 }, + { address: '0x456', name: 'Token2', symbol: 'TK2', decimals: 6, chainId: 137 }, + ], + isRouteAvailable: true, + } + mockProvider.getBuyTokens = jest.fn().mockResolvedValue(mockBuyTokens)
299-301: Same mock-shape issue hereReturn object with tokens + isRouteAvailable.
- const mockBuyTokens = [{ address: '0x123', name: 'Token1', symbol: 'TK1', decimals: 18, chainId: 137 }] - mockProvider.getBuyTokens = jest.fn().mockResolvedValue(mockBuyTokens) + const mockBuyTokens = { + tokens: [{ address: '0x123', name: 'Token1', symbol: 'TK1', decimals: 18, chainId: 137 }], + isRouteAvailable: true, + } + mockProvider.getBuyTokens = jest.fn().mockResolvedValue(mockBuyTokens)
351-353: Same mock-shape issue in persistence testAlign to provider contract.
- const mockBuyTokens = [{ address: '0x123', name: 'Token1', symbol: 'TK1', decimals: 18, chainId: 137 }] - mockProvider.getBuyTokens = jest.fn().mockResolvedValue(mockBuyTokens) + const mockBuyTokens = { + tokens: [{ address: '0x123', name: 'Token1', symbol: 'TK1', decimals: 18, chainId: 137 }], + isRouteAvailable: true, + } + mockProvider.getBuyTokens = jest.fn().mockResolvedValue(mockBuyTokens)packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (1)
113-114: LGTM: BigInt exponentiation is safeUsing 10n ** BigInt(decimals) avoids Number overflow and runtime throws.
packages/common/src/utils/ttlCache.ts (2)
271-322: cleanup() is a no-op in SSR (localStorage undefined) — move instance-key cleanup outside the if-blockWhen localStorage is undefined (SSR/Node), cleanup() returns without touching in-memory/fallback entries. Expired entries will leak.
Apply:
cleanup(): void { const prefix = this.keyPrefix + ':' - if (typeof localStorage !== 'undefined') { + if (typeof localStorage !== 'undefined') { try { const keysToDelete: string[] = [] const now = Date.now() @@ keysToDelete.forEach((key) => localStorage.removeItem(key)) } catch { console.warn('TTLCache: Failed to cleanup expired entries') } - - // Also clean instance-tracked keys (covers wrapper fallback/SSR) - const now = Date.now() - for (const key of Array.from(this.keys)) { - const item = this.storage.getItem(key) - if (!item) { - this.keys.delete(key) - continue - } - - try { - const entry: CacheEntry<T> = JSON.parse(item) - if (now - entry.timestamp > entry.ttl) { - this.storage.removeItem(key) - this.keys.delete(key) - } - } catch { - this.storage.removeItem(key) - this.keys.delete(key) - } - } } + + // Always clean instance-tracked keys (covers wrapper fallback and SSR) + const now = Date.now() + for (const key of Array.from(this.keys)) { + const item = this.storage.getItem(key) + if (!item) { + this.keys.delete(key) + continue + } + try { + const entry: CacheEntry<T> = JSON.parse(item) + if (now - entry.timestamp > entry.ttl) { + this.storage.removeItem(key) + this.keys.delete(key) + } + } catch { + this.storage.removeItem(key) + this.keys.delete(key) + } + } }
251-266: size(): fall back to keys.size when localStorage access throwsIn Safari private mode, localStorage exists but throws; returning 0 hides entries stored in fallback. Prefer keys.size.
try { let count = 0 for (let i = 0; i < localStorage.length; i++) { const key = localStorage.key(i) if (key && key.startsWith(prefix)) { count++ } } return count - } catch { - return 0 + } catch { + return this.keys.size }
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/common/src/utils/ttlCache.ts (2)
115-121: Potential inconsistency with isMemoryStorage getter logic.The getter
isMemoryStoragechecks boththis._isMemoryStorage && this.storage instanceof MemoryStorage, but based on the constructor logic, when localStorage is unavailable (lines 132-135),_isMemoryStorageis set to true but storage is replaced with MemoryStorage. This makes the instanceof check redundant in that case.Consider simplifying the logic or making it more explicit:
get isMemoryStorage(): boolean { - return this._isMemoryStorage && this.storage instanceof MemoryStorage + return this.storage instanceof MemoryStorage }Alternatively, if the flag serves a different purpose, consider adding documentation to clarify the distinction.
107-136: Constructor logic verified — no functional issue; optional simplificationInitial this.isMemoryStorage assignment is redundant but harmless: the getter also checks storage instanceof MemoryStorage and the subsequent localStorage-availability branch updates _isMemoryStorage and storage when needed. isMemoryStorage is only used inside this file. Optional: move the memory-storage detection after the availability check to remove the short-lived assignment (packages/common/src/utils/ttlCache.ts:129–135).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/common/src/utils/ttlCache.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: Build Package
- GitHub Check: eslint
🔇 Additional comments (5)
packages/common/src/utils/ttlCache.ts (5)
63-97: LGTM! Solid graceful fallback implementation for localStorage.The LocalStorageWrapper correctly handles Safari's private mode issues where "Safari sets storage's limit to 0" and "doesn't allow accessing localStorage" by implementing a transparent fallback to MemoryStorage when localStorage is unavailable or throws errors.
141-158: LGTM! TTL validation and key tracking implemented correctly.The
setmethod now properly:
- Validates TTL > 0 to prevent storing disabled cache entries
- Tracks keys in the internal Set for proper cleanup in fallback scenarios
- Handles storage errors gracefully with warning logs
163-190: LGTM! Expired entry cleanup now maintains key tracking consistency.The
getmethod correctly removes keys from the internal Set when entries are expired (line 178) or corrupted (line 187), which addresses the previous issue wheresize()would be inflated in SSR/fallback scenarios.
204-238: LGTM! Comprehensive clear() implementation handles all storage scenarios.The clear method now correctly:
- Handles direct MemoryStorage with prefix-based clearing
- Cleans localStorage entries when available
- Always cleans instance-tracked keys for LocalStorageWrapper fallback scenarios
- Provides proper error handling and logging
This addresses the critical issue where cache entries couldn't be cleared in SSR or Safari private browsing scenarios.
243-268: LGTM! Size calculation covers all storage backends.The
size()method now properly:
- Uses MemoryStorage's native size method when available
- Falls back to the instance key Set size in SSR scenarios
- Scans localStorage when available with proper error handling
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/common/src/utils/ttlCache.ts (4)
207-211: Remove redundant double-check for MemoryStorage in clear()No need to check both the flag and instanceof. Instanceof is sufficient and narrows the type.
Apply:
- if (this.isMemoryStorage && this.storage instanceof MemoryStorage) { - ;(this.storage as MemoryStorage).clear(prefix) + if (this.storage instanceof MemoryStorage) { + this.storage.clear(prefix) this.keys.clear() return }
229-235: Unify log style; prefer warn with consistent prefixOther logs use "TTLCache:" and warn level. Align for consistency.
Apply:
- try { - this.storage.removeItem(key) - } catch (error) { - console.error('TTL Cache clear', error) - } + try { + this.storage.removeItem(key) + } catch (error) { + console.warn('TTLCache: Failed to remove cache key', error) + }
111-121: Simplify isMemoryStorage to a computed getter; remove mutable flagThe flag can drift from reality after storage swaps. A computed getter over storage instanceof is enough.
Apply:
- private _isMemoryStorage: boolean = false // Track keys written by this instance (covers wrapper fallback/SSR) private keys = new Set<string>() get isMemoryStorage(): boolean { - return this._isMemoryStorage && this.storage instanceof MemoryStorage - } - - set isMemoryStorage(value: boolean) { - this._isMemoryStorage = value + return this.storage instanceof MemoryStorage }
128-136: Follow-up to the previous refactor: stop assigning isMemoryStorage in constructorWith a computed getter, these assignments are unnecessary. Just set the storage.
Apply:
- // Detect if we're using memory storage (fallback scenario) - this.isMemoryStorage = this.storage instanceof MemoryStorage - - // Check if localStorage is actually available even when requested - if (useLocalStorage && typeof localStorage === 'undefined') { - this.isMemoryStorage = true - this.storage = new MemoryStorage() - } + // If localStorage was requested but is unavailable, fall back to memory + if (useLocalStorage && typeof localStorage === 'undefined') { + this.storage = new MemoryStorage() + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/common/src/utils/ttlCache.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Package
- GitHub Check: test
- GitHub Check: eslint
🔇 Additional comments (2)
packages/common/src/utils/ttlCache.ts (2)
171-189: Nice: expired/corrupted entries remove from storage and keys indexHandles keys Set sync on both expiration and parse failure.
304-323: LGTM: cleanup runs regardless of localStorage availabilityThe instance-tracked sweep executes outside the localStorage guard, covering SSR and wrapper fallback.
|
Build is a bit broken |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/common/src/utils/ttlCache.ts (1)
195-229: Clear covers all backends incl. fallback; minor nit on redundant checkLogic correctly clears prefixed localStorage keys and always drops instance-tracked keys. The
this.isMemoryStorage && this.storage instanceof MemoryStoragedouble check is redundant;this.isMemoryStoragesuffices.Apply:
- if (this.isMemoryStorage && this.storage instanceof MemoryStorage) { + if (this.isMemoryStorage) { ;(this.storage as MemoryStorage).clear(prefix) this.keys.clear() return }packages/bridging/src/BridgingSdk/getQuoteWithBridge.test.ts (1)
206-220: Strengthen the validTo assertionYou currently assert the call exists, but not that validTo propagated. Consider asserting the value on the second call args.
- expect(getUnsignedBridgeCallMock.mock.calls[1]).toBeDefined() + expect(getUnsignedBridgeCallMock.mock.calls[1][0]?.validTo).toBe(customValidTo)packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (1)
261-264: Log the token address/symbol instead of objectCurrent log interpolates the whole object, resulting in “[object Object]”.
- log(`Using ${intermediateToken} as intermediate tokens`) + log(`Using ${intermediateToken.symbol ?? intermediateToken.address} (${intermediateToken.address}) as intermediate token`)packages/bridging/src/BridgingSdk/BridgingSdk.ts (1)
76-86: Optional: don’t instantiate caches when disabledYou already gate buyTokens caching by
enabled, and after fixing strategy wiring, intermediate tokens won’t use the cache when disabled. Consider skipping instantiation entirely when disabled to reduce memory/side effects.Example:
- this.intermediateTokensCache = new TTLCache<TokenInfo[]>( - 'bridging-intermediate-tokens', - this.cacheConfig.enabled, - this.cacheConfig.intermediateTokensTtl, - ) - this.buyTokensCache = new TTLCache<GetProviderBuyTokens>( - 'bridging-buy-tokens', - this.cacheConfig.enabled, - this.cacheConfig.buyTokensTtl, - ) + this.intermediateTokensCache = this.cacheConfig.enabled + ? new TTLCache<TokenInfo[]>('bridging-intermediate-tokens', true, this.cacheConfig.intermediateTokensTtl) + : new TTLCache<TokenInfo[]>('bridging-intermediate-tokens', false, 0) + this.buyTokensCache = this.cacheConfig.enabled + ? new TTLCache<GetProviderBuyTokens>('bridging-buy-tokens', true, this.cacheConfig.buyTokensTtl) + : new TTLCache<GetProviderBuyTokens>('bridging-buy-tokens', false, 0)(Alternatively, set these fields to undefined and guard at call sites.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/bridging/src/BridgingSdk/BridgingSdk.ts(8 hunks)packages/bridging/src/BridgingSdk/getQuoteWithBridge.test.ts(6 hunks)packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts(5 hunks)packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.ts(4 hunks)packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.ts(3 hunks)packages/bridging/src/BridgingSdk/strategies/QuoteStrategy.ts(2 hunks)packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.ts(2 hunks)packages/bridging/src/BridgingSdk/strategies/createStrategies.ts(1 hunks)packages/bridging/src/BridgingSdk/types.ts(3 hunks)packages/common/src/utils/ttlCache.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/bridging/src/BridgingSdk/strategies/createStrategies.ts
- packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.ts
- packages/bridging/src/BridgingSdk/types.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-03-19T22:06:52.139Z
Learnt from: anxolin
PR: cowprotocol/cow-sdk#251
File: src/bridging/BridgingSdk/getQuoteWithBridging.ts:34-35
Timestamp: 2025-03-19T22:06:52.139Z
Learning: The implementation in the Across bridge provider deliberately selects only the first intermediate token from the list (`intermediateTokens[0]`) as a simplification for the initial implementation, with potential for future enhancement.
Applied to files:
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts
📚 Learning: 2025-03-20T09:45:27.965Z
Learnt from: anxolin
PR: cowprotocol/cow-sdk#251
File: src/bridging/providers/across/AcrossBridgeProvider.ts:85-100
Timestamp: 2025-03-20T09:45:27.965Z
Learning: In the AcrossBridgeProvider's getIntermediateTokens method, not finding an intermediate token is expected behavior in some cases and should not be logged as a warning.
Applied to files:
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts
📚 Learning: 2025-05-30T13:00:36.623Z
Learnt from: sebastiantf
PR: cowprotocol/cow-sdk#317
File: src/bridging/providers/bungee/util.ts:0-0
Timestamp: 2025-05-30T13:00:36.623Z
Learning: The `calculateFeeBps` function in the Bungee bridge provider includes proper validation checks (division by zero, fee amount > input amount) and uses BigInt arithmetic with rounding for precision before converting to Number. This approach is safe because fee amounts are always less than input amounts, ensuring the resulting bps value stays under 10,000.
Applied to files:
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.ts
🧬 Code graph analysis (6)
packages/bridging/src/BridgingSdk/BridgingSdk.ts (6)
packages/bridging/src/BridgingSdk/types.ts (2)
BridgingSdkCacheConfig(71-84)BridgingSdkConfig(66-66)packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-323)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/types.ts (1)
GetProviderBuyTokens(138-141)packages/bridging/src/BridgingSdk/strategies/createStrategies.ts (1)
createStrategies(7-13)packages/bridging/src/BridgingSdk/helpers.ts (1)
getCacheKey(1-13)
packages/bridging/src/BridgingSdk/strategies/QuoteStrategy.ts (4)
packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-323)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/BridgingSdk/types.ts (1)
BridgingSdkConfig(66-66)packages/bridging/src/types.ts (2)
CrossChainQuoteAndPost(288-288)MultiQuoteResult(411-415)
packages/bridging/src/BridgingSdk/getQuoteWithBridge.test.ts (7)
packages/bridging/src/BridgingSdk/types.ts (1)
GetQuoteWithBridgeParams(15-46)packages/bridging/src/types.ts (1)
BridgeQuoteResult(53-99)packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (2)
getQuoteWithBridge(30-228)postSwapOrderFromQuote(180-226)packages/bridging/src/errors.ts (1)
BridgeProviderQuoteError(12-20)packages/bridging/src/BridgingSdk/mock/bridgeRequestMocks.ts (1)
quoteBridgeRequest(54-68)packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-323)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (4)
packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-323)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/types.ts (1)
QuoteBridgeRequestWithoutAmount(51-51)packages/bridging/src/BridgingSdk/helpers.ts (1)
getCacheKey(1-13)
packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.ts (3)
packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-323)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (1)
getQuoteWithBridge(30-228)
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.ts (3)
packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-323)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (1)
getQuoteWithBridge(30-228)
🪛 GitHub Actions: Build
packages/bridging/src/BridgingSdk/BridgingSdk.ts
[error] 90-90: src/BridgingSdk/BridgingSdk.ts(90,7): error TS2554: Expected 0-1 arguments, but got 2.
🪛 GitHub Actions: Unit tests & Coverage
packages/bridging/src/BridgingSdk/BridgingSdk.ts
[error] 90-90: TS2554: Expected 0-1 arguments, but got 2.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: eslint
🔇 Additional comments (13)
packages/common/src/utils/ttlCache.ts (3)
118-127: Constructor fallback logic: good coverage for SSR/blocked localStorageLocalStorageWrapper + explicit SSR check cover both “undefined” and “throws” cases. The per‑instance key tracking fixes prior cleanup/clear/size gaps.
234-262: Size() fallback paths are correctCorrectly returns instance-tracked size when localStorage is undefined or blocked.
267-318: Cleanup now runs in all environmentsRuns localStorage scan (when available) and also cleans instance-tracked keys. This addresses the Safari Private/SSR gaps.
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.ts (2)
26-28: Constructor wiring of cache looks goodPassing the optional intermediateTokensCache through the base class keeps the request building simple and centralized.
81-95: Conditional cache param forwarding is correctBuilding
requestonly whenthis.intermediateTokensCacheexists prevents accidental caching when strategies are constructed without a cache.packages/bridging/src/BridgingSdk/getQuoteWithBridge.test.ts (1)
221-243: Nice cache behavior testVerifies single fetch and subsequent cache hit with TTL. Good coverage.
packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.ts (2)
27-29: Constructor wiring of cache looks goodConsistent with BestQuoteStrategy and base class.
95-109: Conditional cache param forwarding is correctMirrors BestQuoteStrategy. Keeps caching opt‑in based on constructor param presence.
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (2)
110-113: BigInt exponent fix: correct and safeUsing
10n ** BigInt(...)avoids Number overflow and runtime throws.
242-255: Cache key generation: good normalization and scopeUsing getCacheKey with provider id, chainIds, and lowercased address ensures stable keys across checksum variants.
packages/bridging/src/BridgingSdk/strategies/QuoteStrategy.ts (1)
17-24: Base strategy abstraction: solidCommon constructor enforces cache param across strategies; keeps execute() contract clean.
packages/bridging/src/BridgingSdk/BridgingSdk.ts (2)
137-155: getBuyTokens: cache usage is correct and guardedKey uses getCacheKey with normalized address; get/set gated by
enabled. Looks good.
245-269: Cache maintenance APIs are a good additionclearCache/cleanupExpiredCache/getCacheStats provide useful hooks for tests and ops.
alfetopito
left a comment
alfetopito
left a comment
There was a problem hiding this comment.
2 pending comments, otherwise looks good
| // We just pick the first intermediate token for now | ||
| const intermediateToken = intermediateTokens[0] | ||
| log(`Using ${intermediateToken} as intermediate tokens`) | ||
| log(`Using ${intermediateToken?.address ?? intermediateToken?.address} as intermediate tokens`) |
There was a problem hiding this comment.
Why is the same expression (intermediateToken?.address) followed by itself?
📦 GitHub Packages PublishedLast updated: Sep 29, 2025, 01:29:00 PM UTC The following packages have been published to GitHub Packages with pre-release version
InstallationThese packages require authentication to install from GitHub Packages. First, configure your token: # Set your GitHub token
# 1. create one at https://github.com/settings/tokens. Make sure you select the option "Generate new token (classic)"
# 2. Check only the "read:packages" scope
# 3. Add the token to the npm config
npm config set //npm.pkg.github.com/:_authToken YOUR_GITHUB_TOKENThen install any of the packages above: # Yarn
yarn add @cowprotocol/[email protected] --registry https://npm.pkg.github.com
# pnpm
pnpm install @cowprotocol/[email protected] --registry https://npm.pkg.github.com
# NPM
npm install @cowprotocol/[email protected] --registry https://npm.pkg.github.comView PackagesYou can view the published packages at: https://github.com/cowprotocol/cow-sdk/packages |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.test.ts (1)
28-28: Same here — whole suite is skipped.Consider enabling a minimal subset (e.g., structure/strategyName) to keep CI signal.
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (3)
49-49: Grammar nit.“Bridging only support SELL orders” → “Bridging only supports SELL orders”.
Apply:
- throw new Error('Bridging only support SELL orders') + throw new Error('Bridging only supports SELL orders')
231-233: Docstring typo.“Ge the base params” → “Get the base params”.
-/** - * Ge the base params (without the amount) for the bridge quote request - */ +/** + * Get the base params (without the amount) for the bridge quote request + */
262-264: Log message wording.Singular: “intermediate token” (you pick index 0).
- log(`Using ${intermediateToken?.name ?? intermediateToken?.address} as intermediate tokens`) + log(`Using ${intermediateToken?.name ?? intermediateToken?.address} as intermediate token`)packages/bridging/src/BridgingSdk/BridgingSdk.ts (1)
22-23: Use type-only imports to avoid accidental value imports.Import strategies as types to reduce bundle surface.
-import { SingleQuoteStrategy, MultiQuoteStrategy, BestQuoteStrategy } from './strategies' +import type { SingleQuoteStrategy, MultiQuoteStrategy, BestQuoteStrategy } from './strategies'packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts (1)
27-27: Unskip the lightweightstrategyNametest
In packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts, change the top-leveldescribe.skipat line 27 todescribe, leaving only the nesteddescribe.skip('execute', …)intact.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/bridging/src/BridgingSdk/BridgingSdk.ts(8 hunks)packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts(5 hunks)packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts(4 hunks)packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.test.ts(5 hunks)packages/common/src/utils/ttlCache.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/common/src/utils/ttlCache.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-19T22:06:52.139Z
Learnt from: anxolin
PR: cowprotocol/cow-sdk#251
File: src/bridging/BridgingSdk/getQuoteWithBridging.ts:34-35
Timestamp: 2025-03-19T22:06:52.139Z
Learning: The implementation in the Across bridge provider deliberately selects only the first intermediate token from the list (`intermediateTokens[0]`) as a simplification for the initial implementation, with potential for future enhancement.
Applied to files:
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts
📚 Learning: 2025-03-20T09:45:27.965Z
Learnt from: anxolin
PR: cowprotocol/cow-sdk#251
File: src/bridging/providers/across/AcrossBridgeProvider.ts:85-100
Timestamp: 2025-03-20T09:45:27.965Z
Learning: In the AcrossBridgeProvider's getIntermediateTokens method, not finding an intermediate token is expected behavior in some cases and should not be logged as a warning.
Applied to files:
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts
🧬 Code graph analysis (4)
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts (1)
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.ts (1)
BestQuoteStrategy(23-128)
packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.test.ts (2)
packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.ts (1)
MultiQuoteStrategy(24-142)packages/bridging/src/BridgingSdk/mock/bridgeRequestMocks.ts (1)
bridgeQuoteResult(127-161)
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (3)
packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-323)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/BridgingSdk/helpers.ts (1)
getCacheKey(1-13)
packages/bridging/src/BridgingSdk/BridgingSdk.ts (9)
packages/bridging/src/BridgingSdk/types.ts (2)
BridgingSdkCacheConfig(71-84)BridgingSdkConfig(66-66)packages/common/src/utils/ttlCache.ts (1)
TTLCache(107-323)packages/config/src/types/tokens.ts (1)
TokenInfo(6-14)packages/bridging/src/types.ts (1)
GetProviderBuyTokens(138-141)packages/bridging/src/BridgingSdk/strategies/SingleQuoteStrategy.ts (1)
SingleQuoteStrategy(12-56)packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.ts (1)
MultiQuoteStrategy(24-142)packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.ts (1)
BestQuoteStrategy(23-128)packages/bridging/src/BridgingSdk/strategies/createStrategies.ts (1)
createStrategies(7-13)packages/bridging/src/BridgingSdk/helpers.ts (1)
getCacheKey(1-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: eslint
- GitHub Check: Publish to GitHub Packages
- GitHub Check: test
- GitHub Check: Build Package
🔇 Additional comments (8)
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts (1)
1-1: API alignment looks good.Switch to public BestQuoteStrategy and types is correct and consistent with the new surface.
Also applies to: 19-19, 28-28, 39-39
packages/bridging/src/BridgingSdk/strategies/MultiQuoteStrategy.test.ts (1)
257-274: Parallelism test reads clean and deterministic with fake timers.The pattern with advanceTimersByTimeAsync is correct; no flakiness flags.
Also applies to: 268-274
packages/bridging/src/BridgingSdk/getQuoteWithBridge.ts (2)
112-113: BigInt formatting is now safe.Using
10n ** BigInt(decimals)avoids the priorBigInt(10 ** decimals)pitfall. LGTM.
242-255: Cache key normalization confirmed; no bespoke builders remain
AllcacheKeyassignments in the bridging package usegetCacheKey, and negative-result caching is applied consistently.packages/bridging/src/BridgingSdk/BridgingSdk.ts (4)
25-30: Sensible defaults.TTL defaults (5m/2m) are reasonable starting points.
If you plan to tweak via env, we can wire an override helper — say the word.
76-87: Caching enable/disable wiring is correct.
- TTLCache constructed with persistence gated by
enabled.- Strategies receive cache only when
enabledis true.Also applies to: 88-91
133-153: Cache key construction for getBuyTokens is consistent with helper.Lowercased address via
getCacheKeyprevents duplicate/missed hits. Guarded get/set honorsenabled.
244-268: Nice: cache maintenance helpers.API for clear/cleanup/stats will help in tests and runtime ops.
Summary
Add caching options to the Bridging SDK, for the methods:
getBuyTokensgetIntermediateTokensSummary by CodeRabbit
New Features
Public API / Types
Tests