Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: remove onUri callbacks in favor of adding single callback listener #3663

Merged
merged 4 commits into from
Jan 20, 2025
Merged
Show file tree
Hide file tree
Changes from all 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
23 changes: 23 additions & 0 deletions .changeset/selfish-comics-rule.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
---
'@reown/appkit-adapter-bitcoin': patch
'@reown/appkit-adapter-solana': patch
'@reown/appkit-adapter-wagmi': patch
'@reown/appkit': patch
'@reown/appkit-core': patch
'@reown/appkit-adapter-ethers': patch
'@reown/appkit-adapter-ethers5': patch
'@reown/appkit-utils': patch
'@reown/appkit-cdn': patch
'@reown/appkit-cli': patch
'@reown/appkit-common': patch
'@reown/appkit-experimental': patch
'@reown/appkit-polyfills': patch
'@reown/appkit-scaffold-ui': patch
'@reown/appkit-siwe': patch
'@reown/appkit-siwx': patch
'@reown/appkit-ui': patch
'@reown/appkit-wallet': patch
'@reown/appkit-wallet-button': patch
---

Remove all onUri callback drilling for all walletConnectConnect methods in favor of a single call when initializing the UniversalProvider
9 changes: 2 additions & 7 deletions packages/adapters/bitcoin/tests/BitcoinAdapter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,19 +53,14 @@ describe('BitcoinAdapter', () => {
})

it('should call connect from WALLET_CONNECT connector', async () => {
const onUri = vi.fn()
await adapter.connectWalletConnect(onUri)
;(
mockWalletConnect.provider.on as MockedFunction<typeof mockWalletConnect.provider.on>
).mock.calls.find(([name]) => name === 'display_uri')![1]('mock_uri')
await adapter.connectWalletConnect()

expect(onUri).toHaveBeenCalled()
expect(mockWalletConnect.provider.connect).toHaveBeenCalled()
})

it('should throw if caipNetworks is not defined', async () => {
adapter = new BitcoinAdapter({ api })
await expect(adapter.connectWalletConnect(vi.fn())).rejects.toThrow()
await expect(adapter.connectWalletConnect()).rejects.toThrow()
})

it('should set BitcoinWalletConnectConnector', async () => {
Expand Down
7 changes: 2 additions & 5 deletions packages/adapters/solana/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -331,11 +331,8 @@ export class SolanaAdapter extends AdapterBlueprint<SolanaProvider> {
)
}

public override async connectWalletConnect(
onUri: (uri: string) => void,
chainId?: string | number
) {
const result = await super.connectWalletConnect(onUri, chainId)
public override async connectWalletConnect(chainId?: string | number) {
const result = await super.connectWalletConnect(chainId)

const rpcUrl = this.caipNetworks?.find(n => n.id === chainId)?.rpcUrls.default.http[0] as string
const connection = new Connection(rpcUrl, this.connectionSettings)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,6 @@ export class SolanaWalletConnectProvider
this.getActiveChain = getActiveChain
}

// -- Universal Provider Events ------------------------ //
public onUri?: (uri: string) => void

// -- Public ------------------------------------------- //
public get session(): SessionTypes.Struct | undefined {
return this.provider.session
Expand Down Expand Up @@ -80,13 +77,7 @@ export class SolanaWalletConnectProvider
}

public async connect() {
if (!this.onUri) {
throw new Error('onUri callback is required to connect WalletConnectProvider')
}

await super.connectWalletConnect({
onUri: this.onUri
})
await super.connectWalletConnect()

const account = this.getAccount(true)

Expand Down
13 changes: 5 additions & 8 deletions packages/adapters/solana/src/tests/GenericProvider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,11 @@ const getActiveChain = vi.fn(() => TestConstants.chains[0])
const providers: { name: string; provider: Provider }[] = [
{
name: 'WalletConnectProvider',
provider: Object.assign(
new SolanaWalletConnectProvider({
provider: mockUniversalProvider(),
chains: TestConstants.chains,
getActiveChain
}),
{ onUri: vi.fn() }
)
provider: new SolanaWalletConnectProvider({
provider: mockUniversalProvider(),
chains: TestConstants.chains,
getActiveChain
})
},
{
name: 'WalletStandardProvider',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ describe('WalletConnectProvider specific tests', () => {
chains: TestConstants.chains,
getActiveChain
})
walletConnectProvider.onUri = vi.fn()
})

it('should call connect', async () => {
Expand Down
4 changes: 1 addition & 3 deletions packages/adapters/solana/src/tests/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -230,12 +230,10 @@ describe('SolanaAdapter', () => {

describe('SolanaAdapter - connectWalletConnect', () => {
it('should connect WalletConnect provider', async () => {
const onUri = vi.fn()
vi.mocked(adapter['connectors']).push(mockWalletConnectConnector)
await adapter.connectWalletConnect(onUri)
await adapter.connectWalletConnect()

expect(mockWalletConnectConnector.provider.connect).toHaveBeenCalled()
expect(mockWalletConnectConnector.provider.on).toHaveBeenCalledWith('display_uri', onUri)
expect(SolStoreUtil.setConnection).toHaveBeenCalled()
})
})
Expand Down
28 changes: 9 additions & 19 deletions packages/adapters/wagmi/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -478,38 +478,28 @@ export class WagmiAdapter extends AdapterBlueprint {
}
}

public override async connectWalletConnect(
onUri: (uri: string) => void,
chainId?: number | string
) {
public override async connectWalletConnect(chainId?: number | string) {
// Attempt one click auth first
const walletConnectConnector = this.getWalletConnectConnector()
const isAuthenticated = await walletConnectConnector.authenticate({ onUri })
const isAuthenticated = await walletConnectConnector.authenticate()

if (isAuthenticated) {
return { clientId: await walletConnectConnector.provider.client.core.crypto.getClientId() }
}

// Attempt to connect using wagmi connector
const connector = this.getWagmiConnector('walletConnect')
const wagmiConnector = this.getWagmiConnector('walletConnect')

if (!connector) {
if (!wagmiConnector) {
throw new Error('UniversalAdapter:connectWalletConnect - connector not found')
}

const provider = (await connector.getProvider()) as UniversalProvider

if (!this.caipNetworks || !provider) {
throw new Error(
'UniversalAdapter:connectWalletConnect - caipNetworks or provider is undefined'
)
}

provider.on('display_uri', onUri)

await connect(this.wagmiConfig, { connector, chainId: chainId ? Number(chainId) : undefined })
await connect(this.wagmiConfig, {
connector: wagmiConnector,
chainId: chainId ? Number(chainId) : undefined
})

return { clientId: await provider.client.core.crypto.getClientId() }
return { clientId: await walletConnectConnector.provider.client.core.crypto.getClientId() }
}

public async connect(
Expand Down
4 changes: 1 addition & 3 deletions packages/appkit/src/adapters/ChainAdapterBlueprint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,16 +187,14 @@ export abstract class AdapterBlueprint<

/**
* Connects to WalletConnect.
* @param {(uri: string) => void} onUri - Callback function to handle the WalletConnect URI
* @param {number | string} [_chainId] - Optional chain ID to connect to
*/
public async connectWalletConnect(
onUri: (uri: string) => void,
_chainId?: number | string
): Promise<undefined | { clientId: string }> {
const connector = this.getWalletConnectConnector()

const result = await connector.connectWalletConnect({ onUri })
const result = await connector.connectWalletConnect()

return { clientId: result.clientId }
}
Expand Down
9 changes: 7 additions & 2 deletions packages/appkit/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -829,14 +829,14 @@ export class AppKit {

private createClients() {
this.connectionControllerClient = {
connectWalletConnect: async (onUri: (uri: string) => void) => {
connectWalletConnect: async () => {
const adapter = this.getAdapter(ChainController.state.activeChain)

if (!adapter) {
throw new Error('Adapter not found')
}

const result = await adapter.connectWalletConnect(onUri, this.getCaipNetwork()?.id)
const result = await adapter.connectWalletConnect(this.getCaipNetwork()?.id)
this.close()

this.setClientId(result?.clientId || null)
Expand Down Expand Up @@ -1289,6 +1289,11 @@ export class AppKit {

private listenWalletConnect() {
if (this.universalProvider) {
this.universalProvider.on(
'display_uri',
ConnectionController.setUri.bind(ConnectionController)
)
Comment on lines +1292 to +1295
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this override other display_uri handlers if subscribed from outside of appkit? in the case they provide the UP externally they can self-manage

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 if this would override an external callback, any other of the setters on display_uri we had would do the same. Do you think previously of this change this would not happen? I'm not sure about this one


this.universalProvider.on('disconnect', () => {
this.chainNamespaces.forEach(namespace => {
this.resetAccount(namespace)
Expand Down
12 changes: 3 additions & 9 deletions packages/appkit/src/connectors/WalletConnectConnector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ export class WalletConnectConnector<Namespace extends ChainNamespace = ChainName
return this.caipNetworks
}

async connectWalletConnect(params: WalletConnectConnector.ConnectParams) {
const isAuthenticated = await this.authenticate(params)
async connectWalletConnect() {
const isAuthenticated = await this.authenticate()

if (!isAuthenticated) {
await this.provider.connect({
Expand All @@ -52,9 +52,7 @@ export class WalletConnectConnector<Namespace extends ChainNamespace = ChainName
await this.provider.disconnect()
}

async authenticate(params: WalletConnectConnector.ConnectParams): Promise<boolean> {
this.provider.on('display_uri', params.onUri)

async authenticate(): Promise<boolean> {
const chains = this.chains.map(network => network.caipNetworkId)

return SIWXUtil.universalProviderAuthenticate({
Expand All @@ -72,10 +70,6 @@ export namespace WalletConnectConnector {
namespace: Namespace
}

export type ConnectParams = {
onUri: (uri: string) => void
}

export type ConnectResult = {
clientId: string | null
session: SessionTypes.Struct
Expand Down
20 changes: 1 addition & 19 deletions packages/appkit/src/universal-adapter/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,7 @@ import type UniversalProvider from '@walletconnect/universal-provider'
import bs58 from 'bs58'

import { type ChainNamespace, ConstantsUtil } from '@reown/appkit-common'
import {
ChainController,
ConnectionController,
CoreHelperUtil,
OptionsController
} from '@reown/appkit-core'
import { ChainController, CoreHelperUtil } from '@reown/appkit-core'

import { AdapterBlueprint } from '../adapters/ChainAdapterBlueprint.js'
import { WalletConnectConnector } from '../connectors/WalletConnectConnector.js'
Expand All @@ -23,19 +18,6 @@ export class UniversalAdapter extends AdapterBlueprint {
)
}

public override async connectWalletConnect(
onUri: (uri: string) => void,
_chainId?: string | number | undefined
): Promise<{ clientId: string } | undefined> {
if (OptionsController.state.useInjectedUniversalProvider && ConnectionController.state.wcUri) {
onUri(ConnectionController.state.wcUri)

return undefined
}

return super.connectWalletConnect(onUri, _chainId)
}

public async connect(
params: AdapterBlueprint.ConnectParams
): Promise<AdapterBlueprint.ConnectResult> {
Expand Down
11 changes: 11 additions & 0 deletions packages/appkit/tests/appkit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1721,6 +1721,7 @@ describe('WalletConnect Events', () => {
let universalProvider: Mocked<Pick<UniversalProvider, 'on'>>

let chainChangedCallback: (chainId: string | number) => void
let displayUriCallback: (uri: string) => void

beforeEach(async () => {
appkit = new AppKit({
Expand All @@ -1739,6 +1740,9 @@ describe('WalletConnect Events', () => {
chainChangedCallback = universalProvider.on.mock.calls.find(
([event]) => event === 'chainChanged'
)?.[1]
displayUriCallback = universalProvider.on.mock.calls.find(
([event]) => event === 'display_uri'
)?.[1]
})

describe('chainChanged', () => {
Expand All @@ -1764,4 +1768,11 @@ describe('WalletConnect Events', () => {
expect(setCaipNetworkSpy).toHaveBeenNthCalledWith(2, newChain)
})
})

describe('display_uri', () => {
it('should call openUri', () => {
displayUriCallback('mock_uri')
expect(ConnectionController.setUri).toHaveBeenCalledWith('mock_uri')
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ describe('WalletConnectConnector', () => {
.spyOn(SIWXUtil, 'universalProviderAuthenticate')
.mockResolvedValueOnce(false)

await connector.connectWalletConnect({ onUri: vi.fn() })
await connector.connectWalletConnect()

expect(authenticateSpy).toHaveBeenCalledWith(
expect.objectContaining({
Expand All @@ -68,7 +68,7 @@ describe('WalletConnectConnector', () => {
Promise.resolve(true)
)

await connector.connectWalletConnect({ onUri: vi.fn() })
await connector.connectWalletConnect()

expect(provider.connect).not.toHaveBeenCalled()
})
Expand Down
12 changes: 3 additions & 9 deletions packages/appkit/tests/universal-adapter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,8 @@ describe('UniversalAdapter', () => {

describe('connectWalletConnect', () => {
it('should connect successfully', async () => {
const onUri = vi.fn()
await adapter.connectWalletConnect()

await adapter.connectWalletConnect(onUri)

expect(mockProvider.on).toHaveBeenCalledWith('display_uri', expect.any(Function))
expect(mockProvider.connect).toHaveBeenCalledWith({
optionalNamespaces: expect.any(Object)
})
Expand All @@ -79,13 +76,12 @@ describe('UniversalAdapter', () => {
writable: true
})

await expect(adapter.connectWalletConnect(() => {})).rejects.toThrow(
await expect(adapter.connectWalletConnect()).rejects.toThrow(
'WalletConnectConnector not found'
)
})

it('should call onUri when display_uri event is emitted', async () => {
const onUri = vi.fn()
const testUri = 'wc:test-uri'

// Call the callback directly when 'on' is called
Expand All @@ -96,9 +92,7 @@ describe('UniversalAdapter', () => {
return mockProvider
})

await adapter.connectWalletConnect(onUri)

expect(onUri).toHaveBeenCalledWith(testUri)
await adapter.connectWalletConnect()
})
})

Expand Down
Loading
Loading