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) and close popup #2118

Merged
merged 5 commits into from
Jan 30, 2025
Merged

Refactor how extension requests ledger access (don't rely on state sync) and close popup #2118

merged 5 commits into from
Jan 30, 2025

Conversation

lukaw3d
Copy link
Member

@lukaw3d lukaw3d commented Jan 24, 2025 •
edited
Loading

Supersedes #2094

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)

@lukaw3d lukaw3d requested review from buberdds, csillag and lubej January 24, 2025 20:42
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 24, 2025 •
edited
Loading

Deployed to Cloudflare Pages

Latest commit: 7ed4a6e2982f444f60607b403d6c445f5737b545
Status:✅ Deploy successful!
Preview URL: https://6dde684e.oasis-wallet.pages.dev
Alias: https://pr-2118.oasis-wallet.pages.dev

@lukaw3d lukaw3d force-pushed the lw/refactor-ledger-access-2 branch 2 times, most recently from dd89392 to f47c859 Compare January 25, 2025 08:43
@lukaw3d lukaw3d mentioned this pull request Jan 25, 2025
Merged
@lukaw3d
Copy link
Member Author

lukaw3d commented Jan 27, 2025

This is based only on the first two commits of #2094. Here's the small diff between those two (rebased) and this PR: https://github.com/oasisprotocol/wallet/compare/966dbf312e67564246dd11019ff4d735f9703b7a..f47c85967c540108f2e07d9e5eebaf0e5ac13dbf

@lukaw3d lukaw3d mentioned this pull request Jan 27, 2025
Merged
@lukaw3d lukaw3d self-assigned this Jan 29, 2025
buberdds
buberdds approved these changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@buberdds buberdds left a comment

Choose a reason for hiding this comment

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

translations issues addressed in #2119

src/app/useRouteRedirects.tsx Show resolved Hide resolved
@lukaw3d
Rename ledger popup prop
142edf4
@lukaw3d
Refactor how extension requests ledger access (don't rely on state sync)
4df4378 
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
@lukaw3d
Remove unneeded instructions in extension ledger access flow
97c7d7e 
Ledger doesn't even need to be unlocked to grant USB access.
@lukaw3d
Automatically close ledger access popups
9fdcec8 
This makes it clearer what user should interact with.
@lukaw3d
After granting usb access to ext, navigate next popup to ledger import
7ed4a6e 
Improves UX after ConnectDevice popup closes and user has to re-open the normal
popup to continue the flow.
@lukaw3d lukaw3d force-pushed the lw/refactor-ledger-access-2 branch from f47c859 to 7ed4a6e Compare January 30, 2025 18:06
@lukaw3d lukaw3d merged commit d912c81 into master Jan 30, 2025
13 checks passed
@lukaw3d lukaw3d deleted the lw/refactor-ledger-access-2 branch January 30, 2025 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@buberdds buberdds buberdds approved these changes

@csillag csillag Awaiting requested review from csillag

@lubej lubej Awaiting requested review from lubej

Assignees

@lukaw3d lukaw3d

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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