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

Detect injected providers #449

Merged
merged 6 commits into from
Dec 6, 2024
Merged

Detect injected providers #449

merged 6 commits into from
Dec 6, 2024

Conversation

Lbqds
Copy link
Member

@Lbqds Lbqds commented Nov 18, 2024

When both the onekey wallet and the alephium wallet are enabled(I can't connect to the onekey extension wallet because I don't have the onekey hardware wallet.):

alephium-and-onekey.mov

When only the alephium wallet is enabled

Screen.Recording.2024-11-18.at.11.39.09.mov

When only the onekey wallet is enabled

onekey.mov


const detectProviders = () => {
if (typeof window !== 'undefined') {
const oneKeyProvider = window['alephium']
Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, the onekey wallet does not announce the alephium provider, and since onekey sets window.alephium, we will try to load the onekey provider from window.alephium.

addNewProvider(oneKeyProvider)
}

const defaultProvider = getWalletObject(alephiumProvider.id)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is to ensure compatibility with older versions of the alephium extension wallet. If only the alephium wallet is enabled and no provider is announced, we will directly try to load the provider from window.alephiumProviders.alephium.

let allProviders: AlephiumWindowObject[] = []

const addNewProvider = (provider: AlephiumWindowObject) => {
if (allProviders.find((p) => p.icon === provider.icon) === undefined) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, the onekey wallet and the alephium wallet use the same id and name, so we can only differentiate between them using the icon.

await options.onConnected({ account: enabledAccount, signerProvider: provider })
setLastConnectedAccount('injected', enabledAccount, options.network)
// eslint-disable-next-line
;(provider as any)['onDisconnected'] = options.onDisconnected
Copy link
Member Author

Choose a reason for hiding this comment

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

This is to work around the enableIfConnected serialization issue in onekey wallet: https://github.com/OneKeyHQ/cross-inpage-provider/blob/23a0e8e3ef2ace42e8a0359be49ddfd2a653d281/packages/providers/onekey-alph-provider/src/OnekeyAlphProvider.ts#L147, it should handle the callback in the same way as unsafeEnable.

Copy link
Member

@polarker polarker left a comment

Choose a reason for hiding this comment

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

LGTM

@Lbqds
Copy link
Member Author

Lbqds commented Nov 21, 2024

Please don't merge this PR. I tested with react-dapp-template and nextjs-app-dapp-template without any issues, but when testing with bridge ui, sometimes it renders multiple times, causing issues with wallet listing and reconnection. I will continue to investigate and resolve this issue.


return providers
}
export const useInjectedProviders = () =>
Copy link
Member Author

Choose a reason for hiding this comment

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

Even though we check if the provider already exists when adding, sometimes duplicate providers appear when refreshing the page due to caching. This issue does not occur in react-dapp-template or nextjs-app-dapp-template but sometimes occurs in bridge ui.

In the latest commit, useSyncExternalStore will be used instead of useEffect/useState. The useSyncExternalStore hook was introduced in React 18, but to maintain compatibility with React 17, we need to import it from use-sync-external-store/shim, which is an official library provided by React.

}

const reconnectByInjected = async (providers: AlephiumWindowObject[], times = 0) => {
if (times >= 20) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I made this change is that our extension wallet injects the inpage provider using a <script> tag, which may run asynchronously in chrome manifest v3 due to this issue. As a result, the dApp sends a requestProvider request but may not immediately receive a response. I tested react-dapp-template, nextjs-app-dapp-template, and bridge ui. Only bridge ui has this issue.

In the ETH ecosystem, detecting injected providers works like this:

  1. The extension wallet calls window.addEventListener('requestProvider', () => window.dispatchEvent('announceProvider', provider)) to handle the dApp's requestProvider requests.
  2. The dApp calls window.dispatchEvent('requestProvider') to request providers.

A crucial assumption is that step 1 must be completed before step 2 begins. Otherwise, the dApp may not receive the provider, and wagmi is based on this assumption.

I checked the OneKey extension wallet and the MetaMask extension wallet:

  1. OneKey wallet places injected.js in content_scripts. In injected.js, OneKey registers the wallet provider for each chain, which ensures that injected.js completed before the dApp starts.
  2. Just like our extension wallet, MetaMask used to inject the in-page provider using the <script> tag before. However, due to the chrome bug mentioned above, it now uses chrome.scripting.registerContentScripts instead in this PR.

I tried using the approach from OneKey and MetaMask to modify our extension wallet, but it didn't work as expected. I'm not sure why yet, perhaps it needs some other related code changes. So I've fixed the issue in the SDK, and it can work properly in different environments.

Copy link
Member

Choose a reason for hiding this comment

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

Could you check how this is resolved in Argent X's latest code?

Copy link
Member Author

@Lbqds Lbqds Nov 24, 2024

Choose a reason for hiding this comment

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

I just checked the starknetkit code; unfortunately, they haven't addressed this issue. Starknetkit tries to load injected providers when the dApp starts, but due to the issue I described above, it sometimes fails to detect the installed Argent-X wallet. Here's a simple video demo where you can see that I have the Argent-X wallet installed, but when I refresh the page, there's a high chance that it will prompt me again to install the Argent-X wallet:

Screen.Recording.2024-11-24.at.10.35.26.mov

I'm not sure why no one has raised this issue with starknetkit. One possible reason is that they haven't tested with create-react-app. In the environments I've tested, this issue only occurs when using create-react-app. It never happens with vite or nextjs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried using the MetaMask approach, and it works now. So we can remove the last commit of this PR.

Do you guys think we should fix this issue in the extension wallet? If so, I will test it in different browsers later and create a PR to update the extension wallet. cc @h0ngcha0

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please fix the extension wallet accordingly! Thanks

This reverts commit 56c481c.
@polarker
Copy link
Member

polarker commented Dec 5, 2024

@Lbqds should we merge this now?

@Lbqds Lbqds merged commit 966ae58 into master Dec 6, 2024
7 checks passed
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