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.

Sign up for GitHub

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

Jump to bottom

Refactor how extension requests ledger access (don't rely on state sync) #2094

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Refactor how extension requests ledger access (don't rely on state sync) #2094

wants to merge 7 commits into from

Conversation

lukaw3d
Copy link
Member

@lukaw3d lukaw3d commented Nov 12, 2024

Extracted from #2084

Related to #2084 (comment)

Previous flow:

  • '#/open-wallet/ledger' has "Grant access to your Ledger" button
  • it opens '#/open-wallet/connect-device' in permanent popup as well as navigates self-closing popup to '#/open-wallet/ledger/usb' (both stay open)
  • connect-device popup has "Connect Ledger device" button that requests usb permission, selects device, and triggers listing accounts in redux. That is synced into self-closing popup
  • when done in connect-device popup, user tries to continue in self-closing popup, but it closes
  • when reopening, and navigating to import from ledger again, it skips some steps because it still has ledger listed accounts in redux
  • some user actions will clear that store. Then if user navigates to import from ledger again, it will show "Grant access to your Ledger" and popup again

New flow:

  • '#/open-wallet/ledger' has "Grant access to your Ledger" button if it has not been granted permission before
  • it opens '#/open-wallet/connect-device' in permanent popup
  • connect-device popup has "Connect Ledger device" button that requests usb permission, and selects device. This already gives necessary permissions to self-closing popup
  • when done in connect-device popup, user tries to continue in self-closing popup, but it closes
  • when reopening, and navigating to import from ledger again, it shows "USB Ledger" button this time (same every time user comes back with already granted permissions)

(I don't have a test ledger device right now. Please check if some of these notes are incorrect)

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 12, 2024 •
edited
Loading

Deployed to Cloudflare Pages

Latest commit: a91234265ae06d3cf2e31dc129ab7f1291dd9cda
Status:✅ Deploy successful!
Preview URL: https://78dd5ff5.oasis-wallet.pages.dev
Alias: https://pr-2094.oasis-wallet.pages.dev

@buberdds
Copy link
Contributor

The Ledger flow is already not ideal, and this change makes it even less user-friendly. I discussed this with @donouwens, and here are some of his suggestions:

In "Grant access to your Ledger" window when user connects a device, nothing happens in popup so it is harder for user to understand what action has been completed.

Can we:

  1. Automatically close the "Device connected" success popup after a set number of seconds? (we assume losing focus will also close the popup), or
  2. Enhance the messaging by adding a brief explanation about what happened once the device is connected?

@lukaw3d
Copy link
Member Author

lukaw3d commented Nov 12, 2024

ah, yeah
Maybe its even possible to prevent the popup1 from closing if we re-focus it before closing persistent popup2 on a timeout

lukaw3d and others added 6 commits November 19, 2024 14:45
@lukaw3d @buberdds
Rename ledger popup prop
2829f9a
@lukaw3d @buberdds
Refactor how extension requests ledger access (don't rely on state sync)
39935ef 
Before:
- redux was shared between persistent popup (can request USB permissions) and
  default popup
- persistent popup listed ledger accounts into store
- when reopening default popup listed accounts were kept
- when reopening default popup after clearing listed accounts: all steps repeat

After:
- persistent popup requests permissions
- this gives permissions to default popup too
- when reopening default popup: use new permissions to list accounts from ledger
@buberdds
Handle async TransportWebUSB method
cd7ca03
@buberdds
Move extension logic into external file
d6e9686
@buberdds
Add communication between ext popup and window
f93ceb3
@buberdds
Add changelog
c2953ff
@buberdds
Copy link
Contributor

what about message passing instead of state sync ? This way we can keep self-closing popup up to date with user actions

@lukaw3d
Copy link
Member Author

lukaw3d commented Nov 21, 2024

I'd still rather avoid that complexity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@buberdds buberdds Awaiting requested review from buberdds

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lukaw3d @buberdds