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

Add EIP-6963 wallet support #42

Merged
merged 7 commits into from
Apr 19, 2024
Merged

Add EIP-6963 wallet support #42

merged 7 commits into from
Apr 19, 2024

Conversation

lubej
Copy link
Contributor

@lubej lubej commented Feb 15, 2024

Solution

Add EIP-6963 support. Each EIP-6963 wallet makes an announcement, which makes it easy to distinguish between different providers. Providers get listed in Select wallet modal.

Links

Flow here:

@lubej lubej force-pushed the ml/eip-6963 branch 2 times, most recently from 0e9b2f5 to 70912a8 Compare February 23, 2024 15:20
@lubej
Copy link
Contributor Author

lubej commented Feb 23, 2024

Since I improvised with the design, @donouwens could you suggest some better design for select wallet modal.

@lubej
Copy link
Contributor Author

lubej commented Feb 23, 2024

TODO: Should we filter out some wallets? Make some kind of allowed list?

TODO: There should be provider listing consistency. i.e. ordering by name(Now it is possible that they are in different order, every time you refresh the page).

@lubej lubej marked this pull request as ready for review February 23, 2024 15:34
this.currentProviderDetail?.provider.removeListener(eventName, listener)

if (this.proxyListeners[eventName]) {
const index = this.proxyListeners[eventName]?.indexOf(listener)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const index = this.proxyListeners[eventName]?.indexOf(listener)
const index = this.proxyListeners[eventName]?.lastIndexOf(listener)

I tried adding duplicate listeners on MetaMask and Brave and they remove the last instance:

const providers = []
window.addEventListener("eip6963:announceProvider", (event) => {
  providers.push(event.detail);
});
window.dispatchEvent(new Event("eip6963:requestProvider"));

const listener = (e) => console.log('listener')
providers[0].provider.on('chainChanged', () => console.log(1))
providers[0].provider.on('chainChanged', listener)
providers[0].provider.on('chainChanged', () => console.log(2))
providers[0].provider.on('chainChanged', listener)
providers[0].provider.on('chainChanged', () => console.log(3))
providers[0].provider.on('chainChanged', listener)
providers[0].provider.on('chainChanged', () => console.log(4))

await providers[0].provider.request({ method: 'eth_chainId' })
// switch chain
// > 1, listener, 2, listener, 3, listener, 4

providers[0].provider.removeListener('chainChanged', listener)
// switch chain
// > 1, listener, 2, listener, 3, 4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you added 7 chainChanged listeners, which is highly unlikely scenario. The current implementation will deny any additional listeners - so that it ensures that each listener is registered only once:

const _addEventListenersOnce = (() => {
let eventListenersInitialized = false
return (ethProvider: typeof window.ethereum) => {
if (eventListenersInitialized) {
return
}
ethProvider?.on?.('accountsChanged', _accountsChanged)
ethProvider?.on?.('chainChanged', _chainChanged)
ethProvider?.on?.('connect', _connect)
ethProvider?.on?.('disconnect', _disconnect)
eventListenersInitialized = true
}
})()

Copy link
Member

Choose a reason for hiding this comment

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

Oh, more bugs: _addEventListenersOnce is created many times. If you call connectWallet(ProviderType.EIP6963) again later it will attach more listeners.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but this happens only once in the current app, but true, _addEventListenersOnce needs to be wrapped inside useCallback, as it will return new instance every time. Or move eventListenersInitialized outside the ContextProvider, which makes more sense I guess.

onClick={() => onSelectProvider(rdns)}
disabled={isLoading}
>
<img className={classes.providerOptionLogo} src={icon} alt={name} />
Copy link
Member

Choose a reason for hiding this comment

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

Validate that icon is data URI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is always Data URI, as per definition in docs: https://eips.ethereum.org/EIPS/eip-6963#imagesicons

Copy link
Member

Choose a reason for hiding this comment

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

To reduce impact of malicious extensions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In what way? There is no way to run JS, unless you render svg directly in the DOM. And if you have installed malicious extension, you are screwed even more, as you probably already lost your private key.

src/hooks/useEIP1193.ts Outdated Show resolved Hide resolved
@lubej lubej merged commit 18e892c into master Apr 19, 2024
2 checks passed
@lubej lubej deleted the ml/eip-6963 branch April 19, 2024 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants