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

fix: Proposal to repair refresh/reconnect problem #3165

Closed
wants to merge 1 commit into from

Conversation

richtera
Copy link

@richtera richtera commented Oct 27, 2023

Description

When refreshing the page or tag the code runs autoConnect. Currently the providers are not being passed the desired chainId
and address making it not possible for the provider to check whether they are already connected and not prompting the user.
Adding these two fields during the "reconnecting" state and then implementing the code in the injected provider to check
if it's connected to the correct thing.

Additional Information

Your ENS/address:

A lot of these changes are stale since they are in files which have been deleted.

@changeset-bot
Copy link

changeset-bot bot commented Oct 27, 2023

🦋 Changeset detected

Latest commit: 7b12275

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@wagmi/core Patch
@wagmi/connectors Patch
wagmi Patch

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

@vercel
Copy link

vercel bot commented Oct 27, 2023

@richtera is attempting to deploy a commit to the wagmi Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Mar 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
wagmi ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 21, 2024 11:35am

if (!isReconnecting) {
accounts = await this.getAccounts().catch(() => null)
let accounts: readonly Address[] = []
if (isReconnecting) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this meant to be changed?

Copy link
Author

@richtera richtera Mar 18, 2024

Choose a reason for hiding this comment

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

This is where the actual change is. !isReconnecting to isReconnecting. The rest of the typescript specific type related stuff was to get lint a d build to work.

Copy link
Author

Choose a reason for hiding this comment

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

I refactored so only the necessary changes are in the PR. This line is really the only line which needs changing.

@richtera
Copy link
Author

I removed all of the typescript changes, my local biome config was a little bit different than yours so that it required more changes.

@frozeman
Copy link

frozeman commented Mar 21, 2024

Maybe to provide context.
Currently it calls eth_requestAccounts again on page reload here, because of this our browser extension pops up again asking the user on page reload, which shouldn't happen, if there is already a connected account.

eth_requestAccounts is meant to be called if the user is to be asked for an account change, not to retrieve the accounts. that is what eth_accounts is for.

For context, I created the ethereum provider object back in 2016, as well as worked on web3.js, which lead to your library. Maybe this gives more relevance ot this change request that we need. @jxom @tmm

@richtera
Copy link
Author

Basically during a reconnect we don't want this to happen

const requestedAccounts = await provider.request({

We do want this to happen as the comment already is indicating

// Attempt to show another prompt for selecting account if already connected

Therefore I believe the condition on this line was in error since it was running when isReconnecting was false.

if (isReconnecting) {

@frozeman
Copy link

@jxom @tmm

@richtera
Copy link
Author

richtera commented Apr 1, 2024

@jxom @tmm I just tried to inject the provider with normal EIP 6963 messaging and the same problem persists. I really think the issue is with this "if" statement. Have you been able to look at it any further. I would rather not change our wallet to not respond to eth_requestAccounts without showing a dialog to the user. This would make it a lot harder to be able to change accounts and there is already eth_accounts which does the job and in fact is already being called inside of the "if" clause in question. I am not quite sure how to proceed. It seems clear that the condition should be reversed unless there is something else going on I am not aware of.

async connect({ chainId, isReconnecting } = {}) {
      .
      .
      .

      let accounts: readonly Address[] | null = null

/**
 * This section of the code really only makes sense for isReconnecting and not for !isReconnecting
 * because if the user is not reconnecting then this.getAccounts() will be empty for sure.
 */
      if (!isReconnecting) {
        accounts = await this.getAccounts().catch(() => null)
        // Attempt to show another prompt for selecting account if already connected and `shimDisconnect` flag is enabled
        const isAuthorized = shimDisconnect && !!accounts?.length
        if (isAuthorized)
          try {
            const permissions = await provider.request({
              method: 'wallet_requestPermissions',
              params: [{ eth_accounts: {} }],
            })
            accounts = (permissions[0]?.caveats?.[0]?.value as string[])?.map(
              (x) => getAddress(x),
            )
          } catch (err) {
            const error = err as RpcError
            // Not all injected providers support `wallet_requestPermissions` (e.g. MetaMask iOS).
            // Only bubble up error if user rejects request
            if (error.code === UserRejectedRequestError.code)
              throw new UserRejectedRequestError(error)
            // Or prompt is already open
            if (error.code === ResourceUnavailableRpcError.code) throw error
          }
      }
      .
      .
      .

    },

jxom added a commit that referenced this pull request Apr 1, 2024
@jxom
Copy link
Member

jxom commented Apr 1, 2024

Sorry about the late response here. I have opened a new PR up at #3779 to address this issue. Let me know what you think!

@jxom jxom closed this Apr 1, 2024
jxom added a commit that referenced this pull request Apr 1, 2024
* fix: resolves #3165

* Create dull-pants-juggle.md
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.

4 participants