Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions packages/coinbase-wallet/src/index.spec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { createWeb3ReactStoreAndActions } from '@web3-react/store'
import type { Actions, Web3ReactStore } from '@web3-react/types'
import { CoinbaseWallet } from '.'
import { MockEIP1193Provider } from '../../eip1193/src/index.spec'
import { MockEIP1193Provider } from '../../eip1193/src/mock'

jest.mock(
'@coinbase/wallet-sdk',
Expand All @@ -19,7 +19,7 @@ const accounts: string[] = []
describe('Coinbase Wallet', () => {
let store: Web3ReactStore
let connector: CoinbaseWallet
let mockConnector: MockEIP1193Provider
let mockProvider: MockEIP1193Provider

describe('connectEagerly = true', () => {
beforeEach(async () => {
Expand All @@ -34,14 +34,20 @@ describe('Coinbase Wallet', () => {
})
await connector.connectEagerly().catch(() => {})

mockConnector = connector.provider as unknown as MockEIP1193Provider
mockConnector.chainId = chainId
mockConnector.accounts = accounts
mockProvider = connector.provider as unknown as MockEIP1193Provider
mockProvider.chainId = chainId
mockProvider.accounts = accounts
})

test('#activate', async () => {
await connector.activate()

expect(mockProvider.eth_requestAccounts).toHaveBeenCalled()
expect(mockProvider.eth_accounts).not.toHaveBeenCalled()
expect(mockProvider.eth_chainId).toHaveBeenCalled()
expect(mockProvider.eth_chainId.mock.invocationCallOrder[0])
.toBeGreaterThan(mockProvider.eth_requestAccounts.mock.invocationCallOrder[0])

expect(store.getState()).toEqual({
chainId: Number.parseInt(chainId, 16),
accounts,
Expand Down
91 changes: 42 additions & 49 deletions packages/coinbase-wallet/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,17 +85,14 @@ export class CoinbaseWallet extends Connector {
try {
await this.isomorphicInitialize()

if (!this.connected) throw new Error('No existing connection')

return Promise.all([
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
this.provider!.request<string>({ method: 'eth_chainId' }),
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
this.provider!.request<string[]>({ method: 'eth_accounts' }),
]).then(([chainId, accounts]) => {
if (!accounts.length) throw new Error('No accounts returned')
this.actions.update({ chainId: parseChainId(chainId), accounts })
})
if (!this.provider || !this.connected) throw new Error('No existing connection')

// Wallets may resolve eth_chainId and hang on eth_accounts pending user interaction, which may include changing
// chains; they should be requested serially, with accounts first, so that the chainId can settle.
const accounts = await this.provider.request<string[]>({ method: 'eth_accounts' })
const chainId = await this.provider.request<string>({ method: 'eth_chainId' })
if (!accounts.length) throw new Error('No accounts returned')
this.actions.update({ chainId: parseChainId(chainId), accounts })
} catch (error) {
cancelActivation()
throw error
Expand All @@ -117,20 +114,18 @@ export class CoinbaseWallet extends Connector {
? desiredChainIdOrChainParameters
: desiredChainIdOrChainParameters?.chainId

if (this.connected) {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
if (!desiredChainId || desiredChainId === parseChainId(this.provider!.chainId)) return
if (this.provider && this.connected) {
if (!desiredChainId || desiredChainId === parseChainId(this.provider.chainId)) return

const desiredChainIdHex = `0x${desiredChainId.toString(16)}`
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
return this.provider!.request<void>({
return this.provider.request<void>({
method: 'wallet_switchEthereumChain',
params: [{ chainId: desiredChainIdHex }],
}).catch(async (error: ProviderRpcError) => {
if (error.code === 4902 && typeof desiredChainIdOrChainParameters !== 'number') {
if (!this.provider) throw new Error('No provider')
// if we're here, we can try to add a new network
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
return this.provider!.request<void>({
return this.provider.request<void>({
method: 'wallet_addEthereumChain',
params: [{ ...desiredChainIdOrChainParameters, chainId: desiredChainIdHex }],
})
Expand All @@ -144,38 +139,36 @@ export class CoinbaseWallet extends Connector {

try {
await this.isomorphicInitialize()
if (!this.provider) throw new Error('No provider')

return Promise.all([
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
this.provider!.request<string>({ method: 'eth_chainId' }),
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
this.provider!.request<string[]>({ method: 'eth_requestAccounts' }),
]).then(([chainId, accounts]) => {
const receivedChainId = parseChainId(chainId)

if (!desiredChainId || desiredChainId === receivedChainId)
return this.actions.update({ chainId: receivedChainId, accounts })

// if we're here, we can try to switch networks
const desiredChainIdHex = `0x${desiredChainId.toString(16)}`
return this.provider
?.request<void>({
method: 'wallet_switchEthereumChain',
params: [{ chainId: desiredChainIdHex }],
})
.catch(async (error: ProviderRpcError) => {
if (error.code === 4902 && typeof desiredChainIdOrChainParameters !== 'number') {
// if we're here, we can try to add a new network
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
return this.provider!.request<void>({
method: 'wallet_addEthereumChain',
params: [{ ...desiredChainIdOrChainParameters, chainId: desiredChainIdHex }],
})
}

throw error
})
})
// Wallets may resolve eth_chainId and hang on eth_accounts pending user interaction, which may include changing
// chains; they should be requested serially, with accounts first, so that the chainId can settle.
const accounts = await this.provider.request<string[]>({ method: 'eth_requestAccounts' })
const chainId = await this.provider.request<string>({ method: 'eth_chainId' })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: probably out of scope for this PR, but I would consider adding to a todo list: potential candidate for abstracting away? Looks like a common code path here and few lines earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that web3-react is the abstraction but, more importantly, I don't want to make this a large change because it's meant to be a bug fix.

const receivedChainId = parseChainId(chainId)

if (!desiredChainId || desiredChainId === receivedChainId)
return this.actions.update({ chainId: receivedChainId, accounts })

// if we're here, we can try to switch networks
const desiredChainIdHex = `0x${desiredChainId.toString(16)}`
return this.provider
?.request<void>({
method: 'wallet_switchEthereumChain',
params: [{ chainId: desiredChainIdHex }],
})
.catch(async (error: ProviderRpcError) => {
if (error.code === 4902 && typeof desiredChainIdOrChainParameters !== 'number') {
if (!this.provider) throw new Error('No provider')
// if we're here, we can try to add a new network
return this.provider.request<void>({
method: 'wallet_addEthereumChain',
params: [{ ...desiredChainIdOrChainParameters, chainId: desiredChainIdHex }],
})
}

throw error
})
} catch (error) {
cancelActivation()
throw error
Expand Down
48 changes: 6 additions & 42 deletions packages/eip1193/src/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { createWeb3ReactStoreAndActions } from '@web3-react/store'
import type { Actions, ProviderRpcError, RequestArguments, Web3ReactStore } from '@web3-react/types'
import { EventEmitter } from 'node:events'
import { EIP1193 } from '.'
import { MockEIP1193Provider } from './mock'

class MockProviderRpcError extends Error {
public code: number
Expand All @@ -13,47 +14,6 @@ class MockProviderRpcError extends Error {
}
}

export class MockEIP1193Provider extends EventEmitter {
public chainId?: string
public accounts?: string[]

public eth_chainId = jest.fn((chainId?: string) => chainId)
public eth_accounts = jest.fn((accounts?: string[]) => accounts)
public eth_requestAccounts = jest.fn((accounts?: string[]) => accounts)

public request(x: RequestArguments): Promise<unknown> {
// make sure to throw if we're "not connected"
if (!this.chainId) return Promise.reject(new Error())

switch (x.method) {
case 'eth_chainId':
return Promise.resolve(this.eth_chainId(this.chainId))
case 'eth_accounts':
return Promise.resolve(this.eth_accounts(this.accounts))
case 'eth_requestAccounts':
return Promise.resolve(this.eth_requestAccounts(this.accounts))
default:
throw new Error()
}
}

public emitConnect(chainId: string) {
this.emit('connect', { chainId })
}

public emitDisconnect(error: ProviderRpcError) {
this.emit('disconnect', error)
}

public emitChainChanged(chainId: string) {
this.emit('chainChanged', chainId)
}

public emitAccountsChanged(accounts: string[]) {
this.emit('accountsChanged', accounts)
}
}

const chainId = '0x1'
const accounts: string[] = []

Expand Down Expand Up @@ -132,7 +92,7 @@ describe('EIP1193', () => {
mockProvider.accounts = accounts

connector = new EIP1193({ actions, provider: mockProvider })
await connector.connectEagerly().catch(() => {})
await connector.connectEagerly()

expect(store.getState()).toEqual({
chainId: 1,
Expand All @@ -143,6 +103,8 @@ describe('EIP1193', () => {
expect(mockProvider.eth_chainId.mock.calls.length).toBe(1)
expect(mockProvider.eth_accounts.mock.calls.length).toBe(1)
expect(mockProvider.eth_requestAccounts.mock.calls.length).toBe(0)
expect(mockProvider.eth_chainId.mock.invocationCallOrder[0])
.toBeGreaterThan(mockProvider.eth_accounts.mock.invocationCallOrder[0])
})
})

Expand Down Expand Up @@ -170,6 +132,8 @@ describe('EIP1193', () => {
expect(mockProvider.eth_chainId.mock.calls.length).toBe(1)
expect(mockProvider.eth_accounts.mock.calls.length).toBe(0)
expect(mockProvider.eth_requestAccounts.mock.calls.length).toBe(1)
expect(mockProvider.eth_chainId.mock.invocationCallOrder[0])
.toBeGreaterThan(mockProvider.eth_requestAccounts.mock.invocationCallOrder[0])
})

test(`chainId = ${chainId}`, async () => {
Expand Down
43 changes: 17 additions & 26 deletions packages/eip1193/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,39 +42,30 @@ export class EIP1193 extends Connector {
})
}

/** {@inheritdoc Connector.connectEagerly} */
public async connectEagerly(): Promise<void> {
private async connect(eager: boolean): Promise<void> {
const cancelActivation = this.actions.startActivation()

return Promise.all([
this.provider.request({ method: 'eth_chainId' }) as Promise<string>,
this.provider.request({ method: 'eth_accounts' }) as Promise<string[]>,
])
.then(([chainId, accounts]) => {
this.actions.update({ chainId: parseChainId(chainId), accounts })
})
.catch((error) => {
try {
// Wallets may resolve eth_chainId and hang on eth_accounts pending user interaction, which may include changing
// chains; they should be requested serially, with accounts first, so that the chainId can settle.
const accounts = await (eager ? this.provider.request({ method: 'eth_accounts' }) : this.provider
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question about this eager flag. I don't see it being used before? We were running eth_accounts in both instances. Is this change intentional?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see this has to do with eager that can be true or false, depending on the call-site. Is there a better way to describe its intent? Is it something like connect if already authorised vs explicitly authorise? In that case, why fallback to eth_accounts when eth_requestAccounts failed? This is probably a bit out of scope for this PR, but a good opportunity to expand my web3-react knowledge a bit!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is done because EIP1193 does not necessarily support eth_requestAccounts, so a fallback is necessary. You don't see this in other connectors because they are not explicitly EIP1193 connectors.

The codepaths for connectEagerly and activate were identical except for the usage of eth_accounts vs eth_requestAccounts, so I added connect as a shared method to express that with less code duplication.

I've refactored to activateAccounts(requestAccounts: () => Promise<string[]>).

.request({ method: 'eth_requestAccounts' })
.catch(() => this.provider.request({ method: 'eth_accounts' }))) as string[]
const chainId = await this.provider.request({ method: 'eth_chainId' }) as string
this.actions.update({ chainId: parseChainId(chainId), accounts })
} catch (error) {
cancelActivation()
throw error
})
}
}

/** {@inheritdoc Connector.connectEagerly} */
public async connectEagerly(): Promise<void> {
return this.connect(true)
}

/** {@inheritdoc Connector.activate} */
public async activate(): Promise<void> {
const cancelActivation = this.actions.startActivation()

return Promise.all([
this.provider.request({ method: 'eth_chainId' }) as Promise<string>,
this.provider
.request({ method: 'eth_requestAccounts' })
.catch(() => this.provider.request({ method: 'eth_accounts' })) as Promise<string[]>,
])
.then(([chainId, accounts]) => {
this.actions.update({ chainId: parseChainId(chainId), accounts })
})
.catch((error) => {
cancelActivation()
throw error
})
return this.connect(false)
}
}
43 changes: 43 additions & 0 deletions packages/eip1193/src/mock.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import type { ProviderRpcError, RequestArguments } from '@web3-react/types'
import { EventEmitter } from 'node:events'

export class MockEIP1193Provider extends EventEmitter {
public chainId?: string
public accounts?: string[]

public eth_chainId = jest.fn((chainId?: string) => chainId)
public eth_accounts = jest.fn((accounts?: string[]) => accounts)
public eth_requestAccounts = jest.fn((accounts?: string[]) => accounts)

public request(x: RequestArguments): Promise<unknown> {
// make sure to throw if we're "not connected"
if (!this.chainId) return Promise.reject(new Error())

switch (x.method) {
case 'eth_chainId':
return Promise.resolve(this.eth_chainId(this.chainId))
case 'eth_accounts':
return Promise.resolve(this.eth_accounts(this.accounts))
case 'eth_requestAccounts':
return Promise.resolve(this.eth_requestAccounts(this.accounts))
default:
throw new Error()
}
}

public emitConnect(chainId: string) {
this.emit('connect', { chainId })
}

public emitDisconnect(error: ProviderRpcError) {
this.emit('disconnect', error)
}

public emitChainChanged(chainId: string) {
this.emit('chainChanged', chainId)
}

public emitAccountsChanged(accounts: string[]) {
this.emit('accountsChanged', accounts)
}
}
29 changes: 27 additions & 2 deletions packages/metamask/src/index.spec.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { createWeb3ReactStoreAndActions } from '@web3-react/store'
import type { Actions, Web3ReactStore } from '@web3-react/types'
import { MetaMask } from '.'
import { MockEIP1193Provider } from '../../eip1193/src/index.spec'
import { MockEIP1193Provider } from '../../eip1193/src/mock'

const chainId = '0x1'
const accounts: string[] = []
const accounts: string[] = ['0x0000000000000000000000000000000000000000']

describe('MetaMask', () => {
let mockProvider: MockEIP1193Provider
Expand All @@ -26,12 +26,37 @@ describe('MetaMask', () => {
connector = new MetaMask({ actions })
})

test('#connectEagerly', async () => {
mockProvider.chainId = chainId
mockProvider.accounts = accounts

await connector.connectEagerly()

expect(mockProvider.eth_requestAccounts).not.toHaveBeenCalled()
expect(mockProvider.eth_accounts).toHaveBeenCalled()
expect(mockProvider.eth_chainId).toHaveBeenCalled()
expect(mockProvider.eth_chainId.mock.invocationCallOrder[0])
.toBeGreaterThan(mockProvider.eth_accounts.mock.invocationCallOrder[0])

expect(store.getState()).toEqual({
chainId: Number.parseInt(chainId, 16),
accounts,
activating: false,
})
})

test('#activate', async () => {
mockProvider.chainId = chainId
mockProvider.accounts = accounts

await connector.activate()

expect(mockProvider.eth_requestAccounts).toHaveBeenCalled()
expect(mockProvider.eth_accounts).not.toHaveBeenCalled()
expect(mockProvider.eth_chainId).toHaveBeenCalled()
expect(mockProvider.eth_chainId.mock.invocationCallOrder[0])
.toBeGreaterThan(mockProvider.eth_requestAccounts.mock.invocationCallOrder[0])

expect(store.getState()).toEqual({
chainId: Number.parseInt(chainId, 16),
accounts,
Expand Down
Loading