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

[Bug]: Pending confirmations should be cleared on reload #22375

Open
nbran opened this issue Dec 21, 2023 · 6 comments
Open

[Bug]: Pending confirmations should be cleared on reload #22375

nbran opened this issue Dec 21, 2023 · 6 comments
Labels
external-contributor regression-prod-11.7.1 Regression bug that was found in production in release 11.7.1 Sev2-normal Normal severity; minor loss of service or inconvenience. team-confirmations Push issues to confirmations team type-bug Something isn't working

Comments

@nbran
Copy link

nbran commented Dec 21, 2023

Describe the bug

Currently, pending confirmations are cleared if the notification window is closed and/or if a request is explicitly rejected by the user. However, if the window is reloaded while the notification window is displayed, the confirmation will remain pending which can be problematic. Below I attempt to describe how this is problematic and some possible solutions.

If the window is reloaded whilst the notification window is open, it's not immediately obvious to the user that their wallet needs their input after the fact. Although the extension's badge is displayed, it could mean any number of things and in any case, it is not very prominent due to the limited space (if any) that an extension icon occupies on the average browser.

At this point any and all new requests will result in an error because the wallet is stuck in a "pending confirmation" state.

Ideally a reload whilst the notification window is open should result in a rejection, which in my opinion would match the current behavior of interpreting the closing of the notification window as a rejection.

In the absence of the above, then it would be helpful to have the ability to programmatically cancel pending confirmations originated by a specific origin so that developers can work around the "pending confirmation" state.

At the moment, it is possible to react to the "stuck" state by catching the accompanying error thrown when a new request is sent whilst a previous one is pending:

{code: -32002, message: "Request of type 'wallet_requestPermissions' already pending for for origin [ORIGIN]. Please wait."

However, it's not really feasible to react elegantly to it from a UX perspective since it's not possible to cancel pending confirmations. Even if we were to ask the user to do something about it (e.g. "Please click on the extension, and cancel or approve the request") the application has no way of being notified by the wallet that a previously issued request has been approved/rejected. That said, asking the user to address the "stuck state" would still be preferable to the current status quo as it would allow the application to inform the user that they need to reload the application in order to align both the wallet and the application states.

Expected behavior

Summarizing, one or all of the following could help in dealing with this scenario:

  • Clearing pending notifications on window reload
  • Providing a method to clear pending requests issued by a specific origin
  • Providing a notification mechanism when a pending request is finally serviced

Screenshots/Recordings

The Uniswap app does not know about pending confirmation, nor is it able to restart the process
metamask-1


Previously issued confirmation has been confirmed, but the Uniswap app is unaware
metamask-2

Steps to reproduce

Finally steps to reproduce and observe the behavior:

  1. Head to https://app.uniswap.org/
  2. Click on the Connect button in the upper right
  3. Select MetaMask from the list of wallets
  4. After the notification window appears, reload the browser, without rejecting or approving
  5. Open Dev Tools after the reload is complete
  6. Notice that the app state conflicts with the wallet state:
    • The application is inviting the user to Connect (which implies no connection attempt has been made), vs.
    • The wallet has a pending connection badge (which implies a connection attempt has been initiated and is pending)
  7. Go ahead and click on the Connect button in the upper right again
  8. Select MetaMask from the list of wallets again
  9. You get an unhelpful error from the application asking you to try again, which at this point will not fix the issue and will result in the exact same error
  10. Additionally, notice error -32002 (mentioned above) in your console
  11. Note that at this point it would make sense for the application to cancel the pending request and issue a new one, as a way of handling itself out of this state this state elegantly

Hopefully all of this highlights how this is problematic and not conducive to good UX, and while we are at it let's do the following in order to emphasize the issue:

  1. Go ahead and open the extension and approve the request
  2. Notice that the application still is unaware that the request has been approved and that the wallet is now connected
  3. You would need to reload the application or go through the connection process again to allow the application to recognize that the wallet connection has been approved and reflect it

Error messages or log output

{code: -32002, message: "Request of type 'wallet_requestPermissions' already pending for for origin [ORIGIN]. Please wait."```

Version

11.7.1

Build type

Beta

Browser

Firefox, Brave

Operating system

Linux

Hardware wallet

No response

Additional context

No response

Severity

No response

@nbran nbran added the type-bug Something isn't working label Dec 21, 2023
@metamaskbot metamaskbot added external-contributor regression-prod-11.7.1 Regression bug that was found in production in release 11.7.1 labels Dec 21, 2023
@gauthierpetetin gauthierpetetin added the Sev2-normal Normal severity; minor loss of service or inconvenience. label Jan 4, 2024
@gauthierpetetin
Copy link
Contributor

Hi @bschorchit @kevinghim would you say this belongs to the scope of the confirmations team or UX team?

@gauthierpetetin gauthierpetetin added team-extension-client team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead team-confirmations-planning (only for internal use within Confirmations team) labels Jan 4, 2024
@nbran
Copy link
Author

nbran commented Jan 4, 2024

Further context describing what I referred to as "current behavior":
#10302

@kevinghim
Copy link
Contributor

@bschorchit can your team look into?

Copy link
Contributor

github-actions bot commented May 9, 2024

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 45 days if there is no further activity. The MetaMask team intends on reviewing this issue before close, and removing the stale label if it is still a bug. We welcome new comments on this issue. We do not intend on closing issues if they report bugs that are still reproducible. Thank you for your contributions.

@github-actions github-actions bot added the stale issues and PRs marked as stale label May 9, 2024
@gauthierpetetin gauthierpetetin added team-confirmations Push issues to confirmations team and removed team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead team-confirmations-planning (only for internal use within Confirmations team) labels May 14, 2024
@github-actions github-actions bot removed the stale issues and PRs marked as stale label May 14, 2024
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 45 days if there is no further activity. The MetaMask team intends on reviewing this issue before close, and removing the stale label if it is still a bug. We welcome new comments on this issue. We do not intend on closing issues if they report bugs that are still reproducible. Thank you for your contributions.

@github-actions github-actions bot added the stale issues and PRs marked as stale label Aug 12, 2024
@gauthierpetetin gauthierpetetin removed the stale issues and PRs marked as stale label Aug 18, 2024
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 45 days if there is no further activity. The MetaMask team intends on reviewing this issue before close, and removing the stale label if it is still a bug. We welcome new comments on this issue. We do not intend on closing issues if they report bugs that are still reproducible. Thank you for your contributions.

@github-actions github-actions bot added the stale issues and PRs marked as stale label Nov 16, 2024
@gauthierpetetin gauthierpetetin removed the stale issues and PRs marked as stale label Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-contributor regression-prod-11.7.1 Regression bug that was found in production in release 11.7.1 Sev2-normal Normal severity; minor loss of service or inconvenience. team-confirmations Push issues to confirmations team type-bug Something isn't working
Projects
Status: To be fixed
Status: To be fixed
Development

No branches or pull requests

4 participants