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

Support Ledger devices #951

Closed
hstove opened this issue Feb 3, 2021 · 41 comments · Fixed by #2242
Closed

Support Ledger devices #951

hstove opened this issue Feb 3, 2021 · 41 comments · Fixed by #2242
Assignees
Labels
area:ledger effort:large Expected to take over 1 week of integration work

Comments

@hstove
Copy link
Contributor

hstove commented Feb 3, 2021

I'm creating an issue here so that we can track it in the future.

It would be amazing to support Ledger devices in the Web Wallet. Even after our audit, it is reasonable to assume that many users won't be comfortable using a software wallet. Not supporting Ledger devices will eliminate the opportunity for apps that build on top of real monetary value - like Stacking, DeFi, etc.

My current understanding is that we already have everything we need (at a technical level) to support Ledger devices in the extension. We can re-use the same library from Zondax, and Ledger has solid support for the WebUSB standard. WebUSB is only supported on Chromium browsers (Chrome, Edge, Brave, etc).

Key features that need design:

  • Connect a Ledger device.
  • Support a "hardware" account as a special type of account
    • Able to switch to this account, view balances, etc.
    • Able to sign in with this account
      • note: Some complications around an appPrivateKey. We certainly can't extract the private key directly from the Ledger's keychain. However, plenty of options to have a good solution here.
  • Signing transactions
    • TX popup from an app
    • Send STX
    • Send FTs
@markmhendrickson markmhendrickson added this to Stacks Wallet (Web) in 🧊 UserX Icebox Feb 3, 2021
@markmhendrickson
Copy link
Collaborator

@friedger mentioned that this is an important issue for Friedger Pool's ability to support users via a web wallet

@psq
Copy link

psq commented Apr 6, 2021

and this will also be as equally important for using swapr, or apps that play with more than just a few tokens

@markmhendrickson markmhendrickson removed this from Stacks Wallet (Web) in 🧊 UserX Icebox Apr 13, 2021
@friedger
Copy link
Contributor

My pool members use Ledger devices because they handle larger amount of tokens. That means they can only use Friedger Pool web app once Web Wallet supports hardware wallets.

@whoabuddy
Copy link

With the new send-many interface created by @friedger and @OmurCataltepe this would definitely be a nice to have.

In the scenario of making payouts to multiple addresses from an account secured by a Ledger, right now, an extra TX is required to fund the web wallet address before making payouts. This would really streamline things!

https://github.com/friedger/stacks-send-many
https://stacks-send-many.pages.dev/

@MarvinJanssen
Copy link
Contributor

Just wanted to add here that we are starting to look at a provider abstraction layer at Ryder for this exact thing. Obviously we would not have an issue exporting the appPrivateKey. 😁

@617a7aa
Copy link

617a7aa commented Aug 10, 2021

Worth noting, I hooked up the foundation with Shift Crypto, who make the open source BitBox (bitcoin only) hardware wallet series. Nothing confirmed yet, but worth throwing it in here. If they do end up working on it, I'll try connect them here so we can get support for BitBoxes in the Hiro Wallet too.

@markmhendrickson
Copy link
Collaborator

Interest shown here in Discord:

image

And here:

image

@617a7aa
Copy link

617a7aa commented Aug 25, 2021

I'm also interested since it keeps my funds a lot more protected. I have my Bitcoin keys air gapped through my BitBox(es) and want the same on here. Also we'd like to keep some funds under multisig so more support for that would be great, even though it's not specific to this issue lol

@dollarsat
Copy link

Also interested in this feature, as I sent all my NFTs, including bns name to my hardware wallet. I thought this would be a safe place where to keep them, and assumed if the Desktop wallet supports Ledger, so does the web wallet. It's difficult for new user to make the difference between the two and for my part, I completely missed this crucial detail. Now I'm realizing I sent all my NFTs on a one-way trip because I'm not ready to compromise on the security of the seed (I also used a passphrase, which makes it more difficult).I wish I'd have known about this little gap in the user experience before sending everything!
The rest of my experience with Stacks has been phenomenal, love every aspect of it and getting more excited every day! Keeping my fingers crossed to see this included in an upcoming update, and finally unlock all those assets :)

@dollarsat
Copy link

For the design, here is a flow I quickly mocked up based on how the desktop wallet connects with Ledger devices. Let me know if there anything I can do to help :)

Mock

@markmhendrickson
Copy link
Collaborator

@markmhendrickson
Copy link
Collaborator

@jasperjansz Let's prepare a full set of mockups for Ledger integration in onboarding and sign in ahead of our next meeting with @jleni and the Zondax team about this next week, so we can review in person then.

@kyranjamie
Copy link
Collaborator

kyranjamie commented Sep 16, 2021

Recommended reading: https://github.com/LedgerHQ/ledgerjs/blob/master/docs/migrate_webusb.md

tl;dr we can support Chrome without "Ledger Live bridge", but Firefox doesn't have WebUSB support yet

@markmhendrickson
Copy link
Collaborator

@jasperjansz Could you post your designs here now that they're fleshed out?

@friedger
Copy link
Contributor

It is unlikely that Firefox will support WebUSB, they say: "..we believe that the security risks of exposing USB devices to the Web are too broad to risk exposing users to them or to explain properly to end users to obtain meaningful informed consent. It also poses risks that sites could use USB device identity or data stored on USB devices as tracking identifiers."

@markmhendrickson
Copy link
Collaborator

Figma designs can be found here:

image
image

@eugeniadigon to add illustrations to both flows

@kyranjamie
Copy link
Collaborator

One note here re the current UI designs: There aren't any error flows described.

There are a million things that can go wrong with a Ledger. Off the top of my head:

  • Being on the wrong Ledger app
  • The screen being locked
  • Disconnecting your device mid-flow, or at the very end where you're comparing the details

One lesson learnt from the desktop wallet would be that we need to hand hold when things go wrong. Giving what feedback we can, linking to support pages etc.

@markmhendrickson
Copy link
Collaborator

@eugeniadigon let's work standard error message components into these when you have a chance, per @kyranjamie's note above 🙏

@markmhendrickson
Copy link
Collaborator

@eugeniadigon to add illustrations for connecting device and confirming transactions on device

@markmhendrickson
Copy link
Collaborator

@eugeniadigon we may want to add a notice here to both Ledger connection flows that warns the user that they should close Ledger Live before proceeding. Keeping it open has been known to cause users issues with the desktop wallet in that Ledger Live prevents the Ledger device from communicating with Hiro Wallet.

@eugeniadigon
Copy link

@eugeniadigon we may want to add a notice here to both Ledger connection flows that warns the user that they should close Ledger Live before proceeding. Keeping it open has been known to cause users issues with the desktop wallet in that Ledger Live prevents the Ledger device from communicating with Hiro Wallet.

Is this issue known only when connecting or also when signing a transaction?

@markmhendrickson
Copy link
Collaborator

Both times since it interferes whenever the wallet needs to talk with the Ledger device

@kyranjamie kyranjamie added this to the Ledger support milestone Nov 3, 2021
@eugeniadigon
Copy link

I added some instructive illustrations to the user flow + an option with the Ledger Live warning, both on the Onboarding and Transaction signing. @markmhx please advise if I should use a better phrasing for the warning

Onboarding: https://www.figma.com/file/VupGh90FJtT0dcBj2Sh7bb/%F0%9F%93%8FSpec?node-id=1021%3A11552

Transaction signing: https://www.figma.com/file/VupGh90FJtT0dcBj2Sh7bb/%F0%9F%93%8FSpec?node-id=1021%3A11553

@markmhendrickson
Copy link
Collaborator

@eugeniadigon this looks great! We'll discuss as a UserX team today to see if any modifications are needed given @kyranjamie's work so far.

@markmhendrickson markmhendrickson added the effort:large Expected to take over 1 week of integration work label Nov 15, 2021
@markmhendrickson
Copy link
Collaborator

This issue needs decomposition into sub-issues.

@kyranjamie
Copy link
Collaborator

@timstackblock
Copy link
Contributor

@eugeniadigon quick question is there a screen that prompts us to pick ledger or a software wallet when creating a new wallet similar to this screen for desktop wallet?

Screen Shot 2021-12-14 at 11 35 14 AM

@john-light
Copy link

WebUSB is only supported on Chromium browsers (Chrome, Edge, Brave, etc).

How does MetaMask support hardware wallets in Firefox? I just tested connecting a Ledger Nano S to MetaMask in Firefox 95.0.1 and it worked fine. Perhaps Hiro Wallet could use the same hardware wallet connection technique MetaMask is using?

@kyranjamie
Copy link
Collaborator

Metamask Ledger integration has since been fixed, and uses WebHID. We'll do the same.

@john-light
Copy link

@kyranjamie this compatibility table shows that Firefox does not support WebHID. I'm not sure what MetaMask is using to get hardware wallet support on Firefox, but perhaps you can look into it and copy their technique for Firefox so that Hiro Wallet hardware wallet support works on both Chromium-based browsers and Firefox? I think they're using U2F for this but not 100% sure (I am not a programmer and can't read their code).

Screenshot 2022-01-10 at 10-43-13 WebHID API - Web APIs MDN

@markmhendrickson
Copy link
Collaborator

@kyranjamie to provide code review instructions (with focus on authentication) while QA gets underway on full regression testing of Ledger support

@markmhendrickson markmhendrickson linked a pull request May 23, 2022 that will close this issue
13 tasks
@kyranjamie
Copy link
Collaborator

Not much to add to Mark's comment. Since it was reviewed previously, we've added jwt signing support, so this code is mostly unreviewed cc/ @beguene @fbwoolf @MrHeidar

@the-artilleryman
Copy link

@kyranjamie to provide code review instructions (with focus on authentication) while QA gets underway on full regression testing of Ledger support

I recently tested this with the latest ledger version (0.22.4) & the last POC build from Friday. I could get as far as signing the JWT hash (e.g. on byzantion, gamma) but the wallet wasn't actually connected to the site, and no further actions could be executed.

Is there more work to be done here, or will I find today's build offers more functionality (sign in fully, execute actions)?

Do the dapps need to update anything on their end?

Just trying to understand the process & where we're at. Cheers.

@kyranjamie
Copy link
Collaborator

kyranjamie commented May 23, 2022

Thanks for testing @the-artilleryman. It not going to work out the box in some cases, I'm afraid. I tried Gamma too and their code will need to be updated to not solely rely on the appPrivateKey, and some other dependencies also need to be updated, @stacks/connect for the sites that use it

@the-artilleryman
Copy link

@kyranjamie No problem. And thank you for the reply. That's useful to know. I'll continue to monitor.

Appreciate all the hard work you & the team are putting in here. Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:ledger effort:large Expected to take over 1 week of integration work
Projects
None yet
Development

Successfully merging a pull request may close this issue.