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] Extension unable to disconnect after public key has already been dissociated from the wallet #332

Closed
RabebOthmani opened this issue Jun 13, 2024 · 16 comments · Fixed by #366
Assignees
Labels
blocked bug Something isn't working

Comments

@RabebOthmani
Copy link
Collaborator

Steps to reproduce

  1. Go to rafiki.money --> Settings --> Developer Keys
  2. Revoke key from wallet
  3. On the extension UI, click on the disconnect button
  4. Extension unable to disconnect

Expected result

Extension should gracefully disconnect from Wallet

Actual result

Extension shows payment screen immediately after clicking on disconnect.
Going back to the extension settings screen shows wallet still connected

Screenshots or videos

No response

Additional context

No response

Operating system

Windows

Operating system version

No response

Browsers

Chrome

Browser version

No response

Extension version

Alpha

@RabebOthmani RabebOthmani added bug Something isn't working triage Waiting to be assigned labels labels Jun 13, 2024
@raducristianpopa
Copy link
Member

For the disconnecting part, we can check if the returned error message is invalid_client when making the API call. If it is returning invalid_client can we make the assumption that the user does not have the key anymore and we can safely clear the storage`?

if this is happening during the monetization flow, should we have an error state for the extension? We can open the popup pragmatically when this happens. Maybe the error page in the popup can prompt the public key for the user again to be reuploaded, alongside a disconnect button that just clears the storage?

CC @sidvishnoi @dianafulga @tselit

@sidvishnoi
Copy link
Member

Sounds good.

We should also update the error message on connect screen when key isn't synced with wallet (instead of bad request one).

@sidvishnoi
Copy link
Member

sidvishnoi commented Jun 14, 2024

For the action-required error states, I think we can add routes under /errors/*. This we we can keep the current default screens same, instead of adding alerts in them (which end up adding more scrollbars). I've that MissingHostPermission route, can move that under errors (not to be mixed with react-router errors).
Ideally, we can add a ErrorLayout and "slot" content inside it for each error page (See MissingHostPermission for example).

@tselit tselit moved this from Backlog to Todo in Web Monetization Extension Jun 17, 2024
@tselit
Copy link

tselit commented Jun 18, 2024

To do: @tselit to update the Figma design file. Maybe create a story for public key input.
Points noted by @RabebOthmani:

  • Is the behaviour and interactions for different wallets consistent (the same)?
  • Consider that UX may be impacted as users may not necessarily understand the concept of keys.

@tselit tselit added this to the WM extension beta milestone Jun 20, 2024
@sidvishnoi sidvishnoi removed the triage Waiting to be assigned labels label Jun 20, 2024
@sidvishnoi sidvishnoi moved this from Todo to In Progress in Web Monetization Extension Jun 21, 2024
@sidvishnoi
Copy link
Member

sidvishnoi commented Jun 24, 2024

For the disconnecting part, we can check if the returned error message is invalid_client when making the API call. If it is returning invalid_client can we make the assumption that the user does not have the key anymore and we can safely clear the storage`?

Just realized the client doesn't return any response on a bad request, so we can't really know if it's a invalid_client error or something else 😬. I think this is something that should be handled at client level, or in Rafiki - so we get better error message in return.

Update: Filed interledger/open-payments#482

@sidvishnoi
Copy link
Member

sidvishnoi commented Jun 24, 2024

Alternatively,

  • we can assume 400 bad request in given state means key removed,
  • or, use our own OpenPayments client, so we've better control (added benefit of less webpack/tooling fiddling, but more maintenance)

@raducristianpopa
Copy link
Member

Alternatively,

we can assume 400 bad request in given state means key removed,

or, use our own OpenPayments client, so we've better control (added benefit of less webpack/tooling fiddling, but more maintenance)

This should be done at the SDK level. Let's not implement our own version for the OP SDK.

@sidvishnoi
Copy link
Member

For now, I'll assume with 400 bad request means that only, and add a TODO when SDK has better error code.

@sidvishnoi
Copy link
Member

sidvishnoi commented Jun 25, 2024

Further investigation:

during payment
  - create quote fails with 401 + Signature validation error: could not find key in list of client keys
  - create outgoing payment with same
  - create incoming payment grant fails with 400 + invalid_client

As there can be multiple reasons for each 400/401, I think it's better to wait on SDK improvements for this so we know which error is it. interledger/open-payments#482

Sending draft PR to allow disconnect on-demand at least.

@sidvishnoi
Copy link
Member

@raducristianpopa @RabebOthmani @tselit

If a key is revoked in between a payment, user can add the key again to same wallet and continue like before (no need to connect again; tested with rafiki.money at least). They can also disconnect and connect again from scratch.

Should we support both options in the error screen? Is the first option complicated for user to follow - as in which one they should go for? Also, after user has copied the key, they need to signal to extension to "try payments again" (or we can keep checking for key validity over and over in background after user copies the public key until we've the successful integration).

Thoughts?

@sidvishnoi
Copy link
Member

Had a call with @raducristianpopa

We think we should show both copy-key and disconnect options.

With copy-key (to add again), we'll show them public key and wallet address again, along with a button to copy it. When user has copied the key, they'd need to press a "I already copied the key" button in popup. On that, we'll verify if we have the right key by trying to rotate the grant token.

Secondly, we'll show the Disconnect button, which will clear the storage (existing grant is cleared from storage, but it'll stay stale in user's wallet). The user should copy key if they don't want the stale grant in their wallet. On disconnect, we'll show the connect screen from scratch that we show on first install.

To make the decision/UX simpler, we can use a wizard to guide user and show other details only when asked for. We'd need UX feedback on this. cc @RabebOthmani @tselit

@sidvishnoi
Copy link
Member

sidvishnoi commented Jun 26, 2024

UI as of 1e33ae7

Screenshot

image

@sidvishnoi
Copy link
Member

I realized we're not using the font we import, so above screenshot is with my system defaults.

Looks like this with that font

image

@RabebOthmani
Copy link
Collaborator Author

Basically, what we're saying is that there are 2 scenarios here: something went wrong accidently and the key was dissociated from the wallet or the user made the decision to revoke the key.
The scenario that is more likely to be true should guide the first option/solution offered to the user.
My hypothesis (at least based on the Rafiki wallet) is that it's very hard and unlikely that a user dissociated a key by accident, therefore the first option would be to dissociate the wallet then the user is redirected to the configuration screen.
The second option in case, the dissociation was an accident would be to ask them to copy the key again and reconnect.
@tselit @sidvishnoi @raducristianpopa thoughts?

@sidvishnoi sidvishnoi moved this from In Progress to Blocked in Web Monetization Extension Jun 28, 2024
@sidvishnoi
Copy link
Member

Some relevant notes from Slack:

I just tired to link the key to another wallet and got “internal server error” understandably as I didn’t remove it from the previous wallet - @RabebOthmani

Adding existing key to any address/account leads to same error. So, user either needs to first remove the existing key (e.g. they want to change their wallet address), or we need to add an option to "regenerate key".

@tselit
Copy link

tselit commented Jul 3, 2024

Updated the UI & flow for the key dissociated scenario in Figma:
https://www.figma.com/design/0S8Oj8ZB5MvkGtIrVRswDH/Web-monetization?node-id=517-504&t=rMLJcdULlNgocWTE-4

This ^^ is ready for peer review, and then we can send it off to Madalina (Design) for final review/approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants