perf: optimize rpc usage#515
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMarket rate enrichment pipeline refactored for configurability and efficiency: token resolution made optional with trust lists, rate enrichment now per-chain-cached and conditionally applied per component, RPC fallback gating driven by market exposure thresholds, hooks redesigned to enable/disable enrichment on-demand. ChangesMarket Rate Enrichment Pipeline Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
added
the
feature request
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useProcessedMarkets.ts (1)
45-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate JSDoc and verify callers need rate enrichment enabled.
enableRateEnrichmentnow defaults tofalse, but the JSDoc describes rate enrichment as always-on (step 4 and the "Identity-only consumers can disable…" paragraph suggest it's enabled by default). This mismatch is confusing. More importantly, no callers currently passenableRateEnrichment: true, meaning all 23 call sites silently lost rate enrichment without explicit opt-in.Clarify the JSDoc to reflect that rate enrichment is opt-in, then audit callers like
useVaultAllocations,useUserPositions, anduseMonarchTransactionsto confirm whether they actually need rolling rates and should passenableRateEnrichment: true.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useProcessedMarkets.ts` around lines 45 - 70, The JSDoc for useProcessedMarkets incorrectly implies rate enrichment is enabled by default while the implementation sets enableRateEnrichment = false; update the JSDoc (in useProcessedMarkets) to state that rolling 24h/7d/30d rate enrichment is opt-in and disabled by default, then audit all callers (e.g., useVaultAllocations, useUserPositions, useMonarchTransactions and other ~23 call sites) to determine which actually need rates and explicitly pass { enableRateEnrichment: true } where required; ensure you run/adjust any unit/integration tests or type checks to confirm callers that require enrichment opt in and document the decision in comments or changelog.
🧹 Nitpick comments (3)
src/utils/tokenMetadata.ts (2)
100-109: 💤 Low valueTiny perf nit: normalize the address once.
address.toLowerCase()runs on everynetworkfor every trusted token. Hoist it out of the inner check.♻️ suggested change
-const findResolvedToken = (address: string, chainId: SupportedNetworks, trustedTokens: ERC20Token[] = []): ERC20Token | undefined => { - const localToken = findToken(address, chainId); - if (localToken) { - return localToken; - } - - return trustedTokens.find((token) => - token.networks.some((network) => network.address.toLowerCase() === address.toLowerCase() && network.chain.id === chainId), - ); -}; +const findResolvedToken = (address: string, chainId: SupportedNetworks, trustedTokens: ERC20Token[] = []): ERC20Token | undefined => { + const localToken = findToken(address, chainId); + if (localToken) { + return localToken; + } + + const normalizedAddress = address.toLowerCase(); + return trustedTokens.find((token) => + token.networks.some((network) => network.chain.id === chainId && network.address.toLowerCase() === normalizedAddress), + ); +};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/tokenMetadata.ts` around lines 100 - 109, In findResolvedToken, avoid calling address.toLowerCase() repeatedly by normalizing it once at the start (e.g., const target = address.toLowerCase()) and use target in the trustedTokens.find predicate when comparing network.address; update the inner comparison from network.address.toLowerCase() === address.toLowerCase() to network.address.toLowerCase() === target (and similarly for any other address comparisons) so the lowercase is computed a single time per function call.
339-351: 💤 Low valueAvoid resolving each token twice.
findResolvedTokenis called once in the filter (343) and again in the loop (351) for every token. For larger market sets with sizeabletrustedTokens, that doubles the trusted-token scan. Resolve once and reuse.♻️ suggested change
const uniqueTokens = dedupeTokenInputs(tokens); const shouldResolveUnknownTokens = options.resolveUnknownTokens ?? true; const trustedTokens = options.trustedTokens ?? []; const resolvedTokenInfos = new Map<string, ResolvedTokenInfo>(); - const unresolvedTokens = uniqueTokens.filter((token) => !findResolvedToken(token.address, token.chainId, trustedTokens)); + const knownByKey = new Map<string, ERC20Token>(); + const unresolvedTokens: TokenAddressInput[] = []; + for (const token of uniqueTokens) { + const known = findResolvedToken(token.address, token.chainId, trustedTokens); + if (known) { + knownByKey.set(infoToKey(token.address, token.chainId), known); + } else { + unresolvedTokens.push(token); + } + } const unresolvedTokenInfos = !shouldResolveUnknownTokens || unresolvedTokens.length === 0 ? new Map<string, ResolvedTokenInfo>() : await fetchResolvedUnknownTokenInfosFromServer(unresolvedTokens).catch(() => new Map<string, ResolvedTokenInfo>()); for (const token of uniqueTokens) { const key = infoToKey(token.address, token.chainId); - const knownToken = findResolvedToken(token.address, token.chainId, trustedTokens); + const knownToken = knownByKey.get(key);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/tokenMetadata.ts` around lines 339 - 351, The code calls findResolvedToken twice per token (once when building unresolvedTokens and again in the for-loop), causing duplicated scans; to fix, compute and cache the resolved/trusted lookup once for each deduped token (e.g., build a Map keyed by infoToKey from uniqueTokens to the result of findResolvedToken using trustedTokens), use that map to derive unresolvedTokens and to get knownToken inside the for-loop, and keep using fetchResolvedUnknownTokenInfosFromServer/unresolvedTokenInfos and resolvedTokenInfos as before so you only resolve each token a single time.src/data-sources/monarch-api/markets.ts (1)
231-244: 💤 Low valueCompute the flag once.
options.resolveUnknownTokens ?? trueis evaluated twice; share a single local so the warn behavior can never drift from the resolution behavior.♻️ suggested change
- const tokenInfos = await resolveTokenInfos(getMarketTokenInputs(rows), customRpcUrls, { - resolveUnknownTokens: options.resolveUnknownTokens ?? true, - trustedTokens: options.trustedTokens, - }); - - const shouldResolveUnknownTokens = options.resolveUnknownTokens ?? true; + const shouldResolveUnknownTokens = options.resolveUnknownTokens ?? true; + const tokenInfos = await resolveTokenInfos(getMarketTokenInputs(rows), customRpcUrls, { + resolveUnknownTokens: shouldResolveUnknownTokens, + trustedTokens: options.trustedTokens, + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/data-sources/monarch-api/markets.ts` around lines 231 - 244, Compute the resolveUnknownTokens flag once and reuse it so resolution and warning behavior stay consistent: create a local like shouldResolveUnknownTokens = options.resolveUnknownTokens ?? true, pass that variable into resolveTokenInfos (instead of options.resolveUnknownTokens ?? true) and into the mapMonarchMarketToMarket call (warnOnMissingTokenInfo), ensuring tokenInfos (from resolveTokenInfos), getMarketTokenInputs, mapMonarchMarketToMarket, and the Market filtering logic all use the same shouldResolveUnknownTokens value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/hooks/useProcessedMarkets.ts`:
- Around line 45-70: The JSDoc for useProcessedMarkets incorrectly implies rate
enrichment is enabled by default while the implementation sets
enableRateEnrichment = false; update the JSDoc (in useProcessedMarkets) to state
that rolling 24h/7d/30d rate enrichment is opt-in and disabled by default, then
audit all callers (e.g., useVaultAllocations, useUserPositions,
useMonarchTransactions and other ~23 call sites) to determine which actually
need rates and explicitly pass { enableRateEnrichment: true } where required;
ensure you run/adjust any unit/integration tests or type checks to confirm
callers that require enrichment opt in and document the decision in comments or
changelog.
---
Nitpick comments:
In `@src/data-sources/monarch-api/markets.ts`:
- Around line 231-244: Compute the resolveUnknownTokens flag once and reuse it
so resolution and warning behavior stay consistent: create a local like
shouldResolveUnknownTokens = options.resolveUnknownTokens ?? true, pass that
variable into resolveTokenInfos (instead of options.resolveUnknownTokens ??
true) and into the mapMonarchMarketToMarket call (warnOnMissingTokenInfo),
ensuring tokenInfos (from resolveTokenInfos), getMarketTokenInputs,
mapMonarchMarketToMarket, and the Market filtering logic all use the same
shouldResolveUnknownTokens value.
In `@src/utils/tokenMetadata.ts`:
- Around line 100-109: In findResolvedToken, avoid calling address.toLowerCase()
repeatedly by normalizing it once at the start (e.g., const target =
address.toLowerCase()) and use target in the trustedTokens.find predicate when
comparing network.address; update the inner comparison from
network.address.toLowerCase() === address.toLowerCase() to
network.address.toLowerCase() === target (and similarly for any other address
comparisons) so the lowercase is computed a single time per function call.
- Around line 339-351: The code calls findResolvedToken twice per token (once
when building unresolvedTokens and again in the for-loop), causing duplicated
scans; to fix, compute and cache the resolved/trusted lookup once for each
deduped token (e.g., build a Map keyed by infoToKey from uniqueTokens to the
result of findResolvedToken using trustedTokens), use that map to derive
unresolvedTokens and to get knownToken inside the for-loop, and keep using
fetchResolvedUnknownTokenInfosFromServer/unresolvedTokenInfos and
resolvedTokenInfos as before so you only resolve each token a single time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 18847ea2-6ea4-44a1-bf24-f0cf3df82159
📒 Files selected for processing (14)
.env.local.examplesrc/constants/markets.tssrc/data-sources/monarch-api/markets.tssrc/features/markets/components/table/market-table-body.tsxsrc/features/markets/components/table/markets-table.tsxsrc/features/markets/markets-view.tsxsrc/features/positions/components/borrowed-morpho-blue-row-detail.tsxsrc/hooks/queries/useMarketsQuery.tssrc/hooks/useFilteredMarkets.tssrc/hooks/useProcessedMarkets.tssrc/utils/market-rate-enrichment.tssrc/utils/market-rpc-gating.tssrc/utils/marketFilters.tssrc/utils/tokenMetadata.ts
There was a problem hiding this comment.
Blocking this, narrowed to the request-path issues only.
I rechecked the Vercel preview with includeUnknownTokens=true forced in monarch_store_marketPreferences.
Observed on preview /markets:
includeUnknownTokens=true- 105 fetch/xhr total
- 28 Morpho GraphQL requests
- 18 Monarch GraphQL requests
- 19
data.monarchlend.xyz/v1/tokens/metadatarequests - 4 Pendle asset requests
- I still did not see direct browser RPC endpoint calls on initial load; the expensive path I can reproduce is GraphQL/token metadata fanout, not wallet/archive RPC from the browser.
Required changes:
-
useMarketsQuerymust not block the normal markets fetch onuseTokensQuerywhenincludeUnknownTokens=false. Right now line 45 always starts the Pendle token query and line 163 gates markets on!tokensLoading, even though line 84 only needs trusted external tokens when resolving unknown tokens. Use the local token list/default immediately for the default path; only wait for external tokens when unknown-token resolution is explicitly enabled. -
The markets query key is wrong for
trustedTokens. Line 50 only keys byallTokens.length, while the query function uses the actual token contents at line 85. If token addresses/symbols change with the same length, React Query can keep stale market resolution. Either removetrustedTokensfrom the default query path, or key by a stable token identity/fingerprint. -
includeUnknownTokens=truestill creates the expensive unknown-token metadata path and likely double markets fetch path.DataPrefetchercallsuseMarketsQuery()with the defaultincludeUnknownTokens=false, while the markets page callsuseMarketsQuery({ includeUnknownTokens }). Since the boolean is in the query key, users with unknown tokens enabled do not share the prefetched markets query; in my preview run this path produced 18 Monarch GraphQL requests plus 19 token-metadata calls. Make prefetcher use the same preference, remove global markets prefetch, or otherwise ensure one canonical markets query per page state.
Validation:
pnpm checkpassed.- Vercel checks passed.
- Preview network was measured directly in browser performance entries after forcing
includeUnknownTokens=true.
starksama
left a comment
starksama
left a comment
There was a problem hiding this comment.
Re-reviewed after f1d2338.
My previous blocking request is addressed enough to unblock:
DataPrefetcherno longer prefetches markets, so it no longer creates a second canonicaluseMarketsQuery()withincludeUnknownTokens=falsewhile the markets page uses the user preference.- The
trustedTokens/ Pendle dependency is an acceptable short-term tradeoff in the current architecture because it preserves external trusted-asset classification. Moving trusted token metadata/classification into Monarch market rows is the cleaner future fix. - I am not blocking on a token fingerprint key because that adds churn/complexity for a path that should be removed once the backend carries classification.
Validation:
pnpm checkpassed.pnpm buildpassed locally.- Local production server with
includeUnknownTokens=trueshowed no token-metadata fanout from duplicate market prefetch; the remaining request shape is from the single markets page query path / fallback path.
Approving.
2 participants
Summary by CodeRabbit
New Features
Improvements