Ionic Bluetooth UI

[lubej]

Solution

This PR is build upon #1739, #1784 and #1769. It adds additional steps, for user to choose either USB or BLE Ledger device. BLE Ledger device is currently only enabled and supported on mobile devices(through Ionic), this may be subject to change in the future.

Links

Android test:
https://github.com/oasisprotocol/oasis-wallet-web/assets/9722540/6e0542f4-f92e-4788-aaba-ed9c6ba009cb

[codecov]

Codecov Report

Attention: Patch coverage is 62.79070% with 48 lines in your changes are missing coverage. Please review.

Project coverage is 80.77%. Comparing base (6e25fc9) to head (c88bb9a).
Report is 104 commits behind head on master.

❗ Current head c88bb9a differs from pull request most recent head 93426e7. Consider uploading reports for the commit 93426e7 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1841      +/-   ##
==========================================
- Coverage   81.18%   80.77%   -0.41%     
==========================================
  Files         193      195       +2     
  Lines        5101     5161      +60     
  Branches      944      949       +5     
==========================================
+ Hits         4141     4169      +28     
- Misses        960      992      +32     

Flag	Coverage Δ
cypress	45.21% <5.19%> (-0.68%)	⬇️
jest	76.72% <62.79%> (-0.32%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/app/pages/ConnectDevicePage/index.tsx	100.00% <ø> (ø)
src/app/pages/OpenWalletPage/index.tsx	100.00% <100.00%> (ø)
...pages/OpenWalletPage/Features/FromLedger/index.tsx	96.15% <95.23%> (+18.88%)	⬆️
...ge/Features/ImportAccountsSelectionModal/index.tsx	91.80% <33.33%> (ø)
src/commonRoutes.tsx	32.00% <0.00%> (-2.79%)	⬇️
src/app/pages/OpenWalletPage/webextension.tsx	0.00% <0.00%> (ø)
...es/OpenWalletPage/Features/FromUsbLedger/index.tsx	77.27% <77.27%> (ø)
...es/OpenWalletPage/Features/FromBleLedger/index.tsx	66.66% <66.66%> (ø)
...tPage/Features/ListBleLedgerDevicesModal/index.tsx	41.46% <41.46%> (ø)

... and 13 files with indirect coverage changes

[lubej]

Couple of things I noticed:

• "Opening Ledger through USB", needs to be replaced with either "Opening Ledger through Bluetooth", or just "Opening Ledger/Connecting with Ledger device";
• Modal height when listing Bluetooth device overflows for some reason;

[lubej]

@donouwens mentioned really confusing wording on instructions upon connecting Ledger device.

Should be for Bluetooth:

1. Connect your Ledger to this device via Bluetooth
2. Make sure your Ledger is paired with this device
3. Open the Oasis app on your Ledger

Should be for USB:

1. Connect your Ledger to this device via USB
2. Close the Ledger Live app on your computer
3. Open the Oasis app on your Ledger

[buberdds]

I have just looked at tests

[buberdds]

so the difference is that we use native modal for usb and custom one for ble?

[lubej]

Yes, sort of. Listing is not native, connect to a single device is. This will probably be reworked for desktop support - but the API is still not officially released yet(at least the last time I checked).

[buberdds]

There is one bug compared to master branch which affects extension

master
Ext Open Wallet -> Grant access -> Connect Ledger device -> Connect device -> Modal success -> Popup reloaded with Ledger accounts

PR:
Ext Open Wallet -> Grant access -> Connect Ledger device -> Connect device -> Modal success -> Popup not reloaded

[lubej]

There is one bug compared to master branch which affects extension

master Ext Open Wallet -> Grant access -> Connect Ledger device -> Connect device -> Modal success -> Popup reloaded with Ledger accounts

PR: Ext Open Wallet -> Grant access -> Connect Ledger device -> Connect device -> Modal success -> Popup not reloaded

Can you try again? And make a video of it. As I was unable to reproduce.

[buberdds]

master
connect Ledger

list Ledger accounts in popup

side branch

• nothing happens in popup after successful connection
  

make sure to turn off / turn on extension while switching branches

[lubej]

There is one bug compared to master branch which affects extension

master Ext Open Wallet -> Grant access -> Connect Ledger device -> Connect device -> Modal success -> Popup reloaded with Ledger accounts

PR: Ext Open Wallet -> Grant access -> Connect Ledger device -> Connect device -> Modal success -> Popup not reloaded

@buberdds this has been fixed now, even though the fix is not really sophisticated. The issue was, that the enumerating modal is not available on the FromLedger component. I know that there was a discussion to show both options on Extension as well (Bluetooth & USB) which breaks the functionality. We can either directly redirect to USB Ledger screen, which may cause problems in the future, if the Bluetooth functionality is ever added to extension. Make the USB Ledger enumeration modal global. Or leave it this way, with redirect to USB Ledger. Which after writing everything down, seems to me like a good enough option.

Thanks for noticing BTW 👍

[buberdds]

I am still having issues with using this:

1. desktop
   I hit a case where nothing happens. So if it's only working with capacitor why we are enabling it on desktop?

depends on browser or browser's settings we are not showing hint about disabled Ledger button anymore (this affects mobile too when user has BLE disabled)

disabled BLE button label - my laptop has BLE enabled, so this message fits mobile devices only

2. extension
   Ledger account list popup has extra scrollbars, which was fixed recently and was one of a blocker for a release

new extension popup Ledger options do not make sense - grant Ledger access and disabled Ledger button next to it

• mobile (apk installed on mobile device)
  not longer able to list Ledger devices ?

[lubej]

@buberdds

Desktop - I hit a case where nothing happens. So if it's only working with capacitor why we are enabling it on desktop?

Yes, so currently there is no official support for desktop - due to Web Bluetooth API still being in experimental mode. This has been changed now. Previously it was only disabled in extension. Now it checks if the app is running on native Capacitor platform(Android or iOS)

Desktop - depends on browser or browser's settings we are not showing hint about disabled Ledger button anymore (this affects mobile too when user has BLE disabled)

After our sync, I dropped the check. So now Ledger button is always enabled on this screen. And the unsupported messages will appear in the next screen. Also updated Bluetooth and WebUSB messages, to make it more generic

Extension - Ledger account list popup has extra scrollbars, which was fixed recently and was one of a blocker for a release

After rebase I was unable to reproduce, not sure if this was fixed in the meantime. Can you confirm it works for you as well?

Extension - new extension popup Ledger options do not make sense - grant Ledger access and disabled Ledger button next to it

As per our sync discussion, we want to make it consistent. Also, there might be Bluetooth support coming to extension, when Web Bluetooth API is no longer in experimental mode.

Mobile (apk installed on mobile device) - no longer able to list Ledger devices?

I was unable to reproduce. Did your Ledger maybe run out of battery? Or something similar? I see that you have your Ledger paired. Can you try again, and write the exact steps to reproduce?

[buberdds]

Extension Ledger views are broken

[buberdds]

will add GitHub issue to explore modal unification (usb /ble )