✨ app: provision alchemy rpc urls across lifi chains#833
✨ app: provision alchemy rpc urls across lifi chains#833cruzdanilo wants to merge 3 commits intorecoverfrom
Conversation
🦋 Changeset detectedLatest commit: 3de6511 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughProvisions Alchemy RPC URLs for LiFi by scanning ChangesChangeset
LiFi Configuration
Sequence DiagramsequenceDiagram
participant App as Application
participant Ensure as ensureConfig
participant Infra as infra exports
participant LiFi as createLifiConfig
participant Loader as getChains / config.setChains
participant MetaMask as MetaMask (metamask.rpcUrls)
App->>Ensure: call ensureConfig()
Ensure->>Infra: scan exports for { id, rpcUrls.alchemy.http }
Infra-->>Ensure: return rpc URL candidates
Ensure->>LiFi: createLifiConfig(..., preloadChains:false) with rpcUrls
Ensure->>Loader: set config.loading -> getChains(...)
Loader->>Loader: filter chains with rpcUrls
Loader->>MetaMask: clear metamask.rpcUrls for those chains
Loader->>LiFi: call config.setChains(...)
Loader-->>Ensure: resolve (errors sent to reportError)
Ensure-->>App: LiFi config ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ 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 |
Summary of ChangesHello @cruzdanilo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's ability to interact with various blockchain networks by implementing a dynamic system for provisioning Alchemy RPC URLs across all supported LiFi chains. This change improves the flexibility and scalability of the LiFi integration, ensuring that the application can automatically configure RPC endpoints for new or existing chains without manual updates, leading to a more robust and maintainable system. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the LiFi configuration to dynamically provision Alchemy RPC URLs for all supported EVM chains from @account-kit/infra, which is a good improvement over hardcoding them. However, a high-severity security vulnerability was identified: a hardcoded LiFi API key in src/utils/lifi.ts that must be externalized to an environment variable to prevent secret exposure. Additionally, two potential runtime errors were found in the new logic: a missing null check when iterating through infra chains and an unguarded spread of an optional property when modifying chain configurations, both of which could lead to crashes. The rest of the changes, including @lifi/sdk dependency modifications, do not appear to introduce new security risks and include beneficial validation enhancements.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## recover #833 +/- ##
===========================================
- Coverage 71.53% 70.96% -0.58%
===========================================
Files 240 239 -1
Lines 9251 9260 +9
Branches 2973 2974 +1
===========================================
- Hits 6618 6571 -47
- Misses 2377 2422 +45
- Partials 256 267 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
dieguezguille
force-pushed
the
lifi
branch
2 times, most recently
from
b86be93 to
c9e125d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0604116d-d4d4-4362-8076-23f8a0e5d2aa
📒 Files selected for processing (2)
.changeset/bright-llamas-provision.mdsrc/utils/lifi.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92efd0dad6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/utils/lifi.ts (2)
109-140:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftAwait LiFi chain loading before marking config ready.
configuredflips totrueimmediately afterconfig.loadingis assigned, so later calls can entergetQuote/getTokenBalancesByChainbeforeconfig.setChains(...)has run. The failure path is also sticky: ifgetChains(...)rejects once, the catch logs it but leavesconfigured = true, so initialization is never retried.Make
ensureConfig()return the loading promise, await it at each caller, and clear the guard on failure.♻️ Suggested direction
-let configured = false; -function ensureConfig() { - if (configured || chain.testnet || chain.id === anvil.id) return; +let loading: Promise<void> | undefined; +function ensureConfig() { + if (chain.testnet || chain.id === anvil.id) return Promise.resolve(); + if (loading) return loading; createLifiConfig({ integrator: "exa_app", apiKey: "4bdb54aa-4f28-4c61-992a-a2fdc87b0a0b.251e33ad-ef5e-40cb-9b0f-52d634b99e8f", preloadChains: false, providers: [EVM({ getWalletClient: () => Promise.resolve(publicClient) })], rpcUrls: { // ... }, }); - config.loading = getChains({ chainTypes: [ChainType.EVM] }) + loading = getChains({ chainTypes: [ChainType.EVM] }) .then((availableChains) => { const rpcs = config.get().rpcUrls as Partial<Record<number, readonly string[]>>; config.setChains( availableChains.map((c) => (rpcs[c.id]?.length ? { ...c, metamask: { ...c.metamask, rpcUrls: [] } } : c)), ); queryClient.setQueryData(lifiChainsOptions.queryKey, availableChains); }) .catch((error: unknown) => { + loading = undefined; reportError(error); }); - configured = true; + config.loading = loading; queryClient.prefetchQuery(lifiTokensOptions).catch(reportError); + return loading; }-ensureConfig(); +await ensureConfig();For `@lifi/sdk` 3.7.7, with createConfig({ preloadChains: false }) and a manually assigned config.loading promise, do getQuote() and getTokenBalancesByChain() wait for config.loading internally, or must callers await the loading promise / call setChains() first?
118-119:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
nullbefore probing dynamic infra exports.
typeof null === "object", so the current check still allowsnullto reach"id" in item/"rpcUrls" in itemand throws during initialization. Add a truthiness guard before the property checks.🛡️ Minimal fix
- if (typeof item !== "object" || !("id" in item) || !("rpcUrls" in item)) return result; + if (!item || typeof item !== "object" || !("id" in item) || !("rpcUrls" in item)) return result;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 755c889d-01d5-4135-ba85-0852f6f7086d
📒 Files selected for processing (2)
.changeset/bright-llamas-provision.mdsrc/utils/lifi.ts
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/utils/lifi.ts (3)
108-141:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
configured = trueon failure permanently prevents chain retry for the session.This was flagged in the previous review and remains unresolved.
When
getChainsrejects (transient network error on first load):
- The
.catch()handler callsreportErrorand returnsundefined, soconfig.loadingresolves withoutconfig.setChains()ever being called.configured = true(line 139) is already set, soensureConfig()is a no-op on every subsequent call.- MetaMask
rpcUrlsare never cleared for Alchemy-configured chains for the remainder of the session, degrading routing quality.- The user must fully reload the app to recover.
The SDK preloads chains during initialization;
preloadChains: falseis the flag to disable that and let you specify chains yourself — but that transfers the retry responsibility entirely to the caller.The previous review proposed returning the loading promise from
ensureConfigand resetting on failure; that approach remains the correct fix.🛡️ Proposed fix
-let configured = false; -function ensureConfig() { - if (configured || chain.testnet || chain.id === anvil.id) return; +let loading: Promise<void> | undefined; +function ensureConfig() { + if (loading || chain.testnet || chain.id === anvil.id) return loading; createLifiConfig({ integrator: "exa_app", apiKey: "4bdb54aa-4f28-4c61-992a-a2fdc87b0a0b.251e33ad-ef5e-40cb-9b0f-52d634b99e8f", preloadChains: false, providers: [EVM({ getWalletClient: () => Promise.resolve(publicClient) })], rpcUrls: { /* ... */ }, }); - config.loading = getChains({ chainTypes: [ChainType.EVM] }) + loading = getChains({ chainTypes: [ChainType.EVM] }) .then((availableChains) => { const rpcs = config.get().rpcUrls as Partial<Record<number, readonly string[]>>; config.setChains( availableChains.map((c) => (rpcs[c.id]?.length ? { ...c, metamask: { ...c.metamask, rpcUrls: [] } } : c)), ); queryClient.setQueryData(lifiChainsOptions.queryKey, availableChains); }) .catch((error: unknown) => { + loading = undefined; reportError(error); }); - configured = true; + config.loading = loading; queryClient.prefetchQuery(lifiTokensOptions).catch(reportError); + return loading; }All call sites that need to await chain configuration should then
await ensureConfig().
116-126:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThree unaddressed guards in
rpcUrlsconstruction.The previous review flagged the null guard and
alchemyRpcUrlissues; neither has been fixed:
Line 118 — null bypass:
typeof null === "object", sonullexports pass thetypeofcheck and"id" in nullthrowsTypeError. Add!item ||before the typeof guard.Line 122 — falsy
alchemyAPIKey: WhenalchemyAPIKeyisnull/undefined/"", the template literal produces an invalid URL (e.g."https://eth-mainnet.g.alchemy.com/v2/undefined"). Add an early exit whenalchemyAPIKeyis falsy — or gate the entire block, since the same condition already disables the alchemy transport inpublicClient.ts.Line 125 —
alchemyRpcUrlcan beundefined: WhenalchemyAPIKeyis falsy,publicClientuses anhttp()transport that has noalchemyRpcUrlproperty, making[publicClient.transport.alchemyRpcUrl]evaluate to[undefined].🛡️ Proposed fix
- rpcUrls: { - ...Object.values(infra).reduce<Record<number, string[]>>((result, item) => { - if (typeof item !== "object" || !("id" in item) || !("rpcUrls" in item)) return result; - const c = item as { id: number; rpcUrls: { alchemy?: { http?: readonly string[] } } }; - const url = c.rpcUrls.alchemy?.http?.[0]; - if (!url) return result; - result[c.id] = [`${url}/${alchemyAPIKey}`]; - return result; - }, {}), - [chain.id]: [publicClient.transport.alchemyRpcUrl], - }, + rpcUrls: { + ...(alchemyAPIKey + ? Object.values(infra).reduce<Record<number, string[]>>((result, item) => { + if (!item || typeof item !== "object" || !("id" in item) || !("rpcUrls" in item)) return result; + const c = item as { id: number; rpcUrls: { alchemy?: { http?: readonly string[] } } }; + const url = c.rpcUrls.alchemy?.http?.[0]; + if (!url) return result; + result[c.id] = [`${url}/${alchemyAPIKey}`]; + return result; + }, {}) + : {}), + ...(publicClient.transport.alchemyRpcUrl + ? { [chain.id]: [publicClient.transport.alchemyRpcUrl] } + : {}), + },
406-408:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
tokensByChainonly coverschain.id; all other bridge-source chains getundefined.This was flagged in the previous review and remains unresolved.
getBridgeSourcespopulatesbalancesByChainfor every chain where the wallet holds balances, and consumers iteratetokensByChain[id]for each of those chains. For any chain other thanchain.id, the lookup returnsundefined, breaking source-token selection across chains.🛡️ Proposed fix
- const tokensByChain: Record<number, Token[]> = { - [chain.id]: destinationTokens.filter((token) => (token.chainId as number) === chain.id), - }; + const tokensByChain = Object.fromEntries( + supportedChains.map(({ id }) => [id, destinationTokens.filter((token) => (token.chainId as number) === id)]), + ) as Record<number, Token[]>;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: add91936-d93a-4d45-be8f-b7435bdd9170
📒 Files selected for processing (2)
.changeset/bright-llamas-provision.mdsrc/utils/lifi.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e4c215e00
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 859664b2f5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
2 participants
Greptile Summary
replaces hardcoded optimism rpc url with dynamic provisioning across all lifi-supported evm chains by iterating through
@account-kit/infraexports. addspreloadChains: falseand async chain loading viaconfig.loadingto defer chain configuration until after the initial lifi setup.@account-kit/infrapackageconfig.loadingpromiseconfigured = trueis set beforeconfig.loadingcompletes, which may cause subsequent function calls to proceed before chains are fully loadedConfidence Score: 2/5
configuredis set totruebefore the asyncconfig.loadingpromise completes could cause functions likegetRoute,getAllTokens, and others to execute before chains are fully configured viaconfig.setChains. this may lead to unpredictable behavior when lifi operations are called quickly after initialization.ensureConfigfunctionImportant Files Changed
@account-kit/infraexports and adds async chain loading withpreloadChains: falseLast reviewed commit: a3f82fa
Summary by CodeRabbit
Chores
Bug Fixes & Improvements