leather-io / extension

Leather browser extension
https://leather.io
MIT License
294 stars 140 forks source link

Support Ledger devices #951

Closed hstove closed 2 years ago

hstove commented 3 years ago

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:

markmhendrickson commented 3 years ago

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

psq commented 3 years ago

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

friedger commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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. 😁

Zk2u commented 3 years ago

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 commented 3 years ago

Interest shown here in Discord:

image

And here:

image

Zk2u commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

See request from @btcidiot with support on Twitter

image

markmhendrickson commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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

friedger commented 3 years ago

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 commented 2 years ago

Figma designs can be found here:

image image

@eugeniadigon to add illustrations to both flows

kyranjamie commented 2 years ago

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:

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 commented 2 years ago

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

sirnovikov commented 2 years ago

Pardon the intrusion, just stumbled upon this discussion while searching whether Hiro Wallet Web supports hardware wallets. Sorry if it's been discussed and I missed it — are you considering WalletConnect (just announced 2.0 version) as well as (or instead of) WebUSB?

I know nothing about either, but WalletConnect seems to be a better idea long term: browser-agnostic and wallet-agnostic — that must be a dream come true?

kyranjamie commented 2 years ago

@sirnovikov input is always welcome

As far as I understand, these are really two separate concerns:

1) WalletConnect being a standard for how dApps can interact with a wallet, a good idea, and something for which we hope to use (either directly, or something similar to it) 2) Ledger hardware wallet support. How we can implement, internal to the extension wallet, transaction signing with Ledger devices

WalletConnect doesn't describe anything specific to supporting hardware wallets afaik

sirnovikov commented 2 years ago

@kyranjamie oh, ok, I see your point. I was thinking about the web wallet in this context as a dApp that interacts with the hardware wallet via WalletConnect (which Ledger supports). On second thought, that seems a bit convoluted

markmhendrickson commented 2 years ago

@sirnovikov you may be interested in this separate issue, in which we are considering WalletConnect: https://github.com/blockstack/connect/issues/101

ginny-d commented 2 years ago

@kyranjamie how does these look for the missing UI error flows?

Is there any other error scenario?

kyranjamie commented 2 years ago

Thanks Ginny. These are helpful. There may be some other cases I'm forgetting/may realise when building it, but this is good to start.

markmhendrickson commented 2 years ago

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

markmhendrickson commented 2 years ago

@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.

ginny-d commented 2 years ago

@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 commented 2 years ago

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

ginny-d commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

This issue needs decomposition into sub-issues.

kyranjamie commented 2 years ago

See ledger milestone https://github.com/hirosystems/stacks-wallet-web/milestone/53

timstackblock commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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

john-light commented 2 years ago

@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 commented 2 years ago

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

kyranjamie commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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.