Conversation
🦋 Changeset detectedLatest commit: 09cb4b1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 25 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
10 Skipped Deployments
|
| "peerDependencies": { | ||
| "@ethersproject/sha2": "5.8.0", | ||
| "ethers": ">=6", | ||
| "@base-org/account": "2.4.0", |
There was a problem hiding this comment.
are these supposed to be in peerDependencies?
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| chain: this.namespace, | ||
| chains: [], | ||
| provider: this.ethersConfig?.[connector as keyof ProviderType] as Provider | ||
| provider: (await provider?.getProvider()) as Provider |
There was a problem hiding this comment.
Bug: Async forEach: Promises Unawaited, Sequencing Broken
Using forEach with an async callback doesn't await the promises, causing addConnector calls with await provider?.getProvider() to execute asynchronously without proper sequencing. This can lead to race conditions or connectors being added with unresolved provider promises.
Visual Regression Test Results ✅ Passed✨ No visual changes detected Chromatic Build: https://www.chromatic.com/build?appId=6493191bf4b10fed8ca7353f&number=370 |
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the ethers adapters to defer initialization of Coinbase SDK and Safe providers until the connection step, improving initial load performance by avoiding unnecessary API calls during adapter setup.
- Introduces new provider wrapper classes (
EthersProvider,InjectedProvider,BaseProvider,SafeProvider) to enable lazy initialization - Moves provider initialization logic from
createEthersConfig()to individual provider classes with deferred loading - Changes dependency management by moving Safe and Base packages from
optionalDependenciestopeerDependenciesin ethers adapter packages
Reviewed Changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Updates dependency resolution for provider packages across adapters |
| packages/appkit/exports/constants.ts | Decrements package version from 1.8.14 to 1.8.13 |
| packages/appkit-utils/src/ethers/SafeProvider.ts | Adds new SafeProvider wrapper class with lazy initialization |
| packages/appkit-utils/src/ethers/EthersProvider.ts | Introduces base EthersProvider abstract class and InjectedProvider implementation |
| packages/appkit-utils/src/ethers/BaseProvider.ts | Adds BaseProvider wrapper for Coinbase SDK with deferred loading |
| packages/appkit-utils/src/ethers/EthersTypesUtil.ts | Updates type definitions to use new provider wrapper classes |
| packages/appkit-utils/package.json | Moves provider packages to optionalDependencies |
| packages/appkit-utils/exports/ethers.ts | Exports new provider classes |
| packages/adapters/ethers5/src/client.ts | Refactors to use new provider wrapper pattern with lazy initialization |
| packages/adapters/ethers/src/utils/SafeProvider.ts | Removes local SafeProvider implementation (moved to appkit-utils) |
| packages/adapters/ethers/src/tests/client.test.ts | Updates tests to mock new provider classes |
| packages/adapters/ethers/src/client.ts | Refactors to use new provider wrapper pattern with lazy initialization |
| packages/adapters/ethers/package.json | Changes provider packages from optionalDependencies to peerDependencies |
| .changeset/lovely-pets-join.md | Documents the change to defer Coinbase SDK initialization |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
|
|
||
| connectors.forEach(connector => { | ||
| connectors.forEach(async connector => { |
There was a problem hiding this comment.
The forEach callback is now async, but the asynchronous operations within it (like await provider?.getProvider()) are not being awaited properly. This creates race conditions where addConnector may be called with unresolved promises or before provider initialization completes. Consider using Promise.all() with map() instead:
await Promise.all(
connectors.map(async connector => {
// ... existing logic
})
)| ) | ||
|
|
||
| connectors.forEach(connector => { | ||
| connectors.forEach(async connector => { |
There was a problem hiding this comment.
The forEach callback is now async, but the asynchronous operations within it (like await provider?.getProvider()) are not being awaited properly. This creates race conditions where addConnector may be called with unresolved promises or before provider initialization completes. Consider using Promise.all() with map() instead:
await Promise.all(
connectors.map(async connector => {
// ... existing logic
})
)|
|
||
| export class BaseProvider extends EthersProvider<ProviderInterface> { | ||
| async initialize(): Promise<void> { | ||
| const caipNetworks = ChainController.getCaipNetworks() |
There was a problem hiding this comment.
The console.log statement should be removed or replaced with proper logging. Debug console statements should not be committed to production code.
| const caipNetworks = ChainController.getCaipNetworks() |
| async initialize(): Promise<void> { | ||
| if (typeof window === 'undefined') { | ||
| return undefined | ||
| } | ||
|
|
||
| if (!window.ethereum) { | ||
| return undefined | ||
| } | ||
|
|
||
| this.provider = window.ethereum | ||
|
|
||
| this.initialized = true | ||
| } |
There was a problem hiding this comment.
The initialize() method is declared as Promise<void> but returns undefined in some code paths. TypeScript will accept this, but it's semantically incorrect. Either change the return type to Promise<void | undefined> or simply omit the return statements:
async initialize(): Promise<void> {
if (typeof window === 'undefined') {
return
}
if (!window.ethereum) {
return
}
this.provider = window.ethereum
this.initialized = true
}
There was a problem hiding this comment.
Bug: Unnecessary Dependencies Cause App Bloat
The Safe and Coinbase dependencies were moved from optionalDependencies to regular dependencies, but based on the PR discussion they should be in peerDependencies. This forces installation of these packages even when not needed, contradicting the lazy-loading goal and increasing bundle size unnecessarily.
packages/adapters/ethers/package.json#L21-L31
appkit/packages/adapters/ethers/package.json
Lines 21 to 31 in 13a95c8
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Description
@reown/appkit-utils/ethersand migrates adapters to use them.eth_requestAccounts.Reference:
fix/lazy-load-coinbasecomparisonMotivation
eth_requestAccounts), leading to adapter-specific workarounds.What changed
EthersProviderbase class (withinitialize()andgetProvider()).InjectedProviderforwindow.ethereum.BaseProvider(Coinbase Base Account) with deferred init.SafeProviderwitheth_requestAccountsnormalization.@reown/appkit-utils/ethers.EthersTypesUtilto include wrapper-drivenProviderType.provider.getProvider(); inconnect(), initialize wrapper on-demand.ethers.SafeProvider.tsinethersadapter (now shared).Behavioral details
InjectedProviderinitializes once at config time (no network calls).eth_requestAccountsis normalized toeth_accountsunder the hood.Type of change
Checklist
Note
Introduces shared provider wrappers and defers Coinbase SDK initialization to connection time, refactoring ethers and ethers5 adapters to use lazy/on-demand providers.
@reown/appkit-utils/ethers):EthersProvider,InjectedProvider,BaseProvider(Coinbase),SafeProvider(normalizeseth_requestAccounts).EthersTypesUtilto use wrapper-basedProviderType.optionalDependencies.ethersProvidersmap.BaseProvider) and initialize on connect; eagerly initSafeProvideronly in Safe context; initInjectedProvideronce.provider.getProvider()and, on connect,initialize()wrapper then setconnector.provider.utils/SafeProvider.ts.@reown/appkit-adapter-ethers,@reown/appkit-adapter-ethers5,@reown/appkit-utils,@reown/appkit.Written by Cursor Bugbot for commit 09cb4b1. This will update automatically on new commits. Configure here.