Add ledger hardware wallet support

[danfinlay]

Would be cool to support this hardware wallet: https://www.ledgerwallet.com

Currently blocked by #3249

[danfinlay]

Most of the JS integration has already been written by the MyEtherWallet team here!

[izqui]

This would be really cool indeed! Happy to help make it possible

Now that this milestone is done https://github.com/MetaMask/metamask-plugin/milestone/14?closed=1, would it be possible to implement Ledger support?

[danfinlay]

Hi @izqui, thanks for the interest!

Our architecture will now support it, but we still haven't settled on what the UI/UX is going to be. If you know the constraints of the Ledger well, maybe you could help propose an account adding / signing flow.

One proposal:

1. We're planning to add an "Import Account" menu item to the top-right menu.
2. That menu will allow selection of different types of accounts to support.
3. One of those account types could be Ledger.

What does that UI look like? What code can we use under the hood?

The other big question for Ledger support:

What does transaction signing look like? I generally like the idea that reviewing a transaction can continue to have a common interface, and as we improve that, it improves all signing strategies, but when the user clicks "Accept", some types of accounts (like Ledger) will need to show another view, and instruct the user how to complete the interaction. What does that look like?

[kumavis]

@izqui also - how does a ledger wallet integration work? do you start a webserver stored on the USB device? Haven't looked into it yet but certainly is possible.

[danfinlay]

Last time I asked, @kumavis, I was told to basically copy MEW's code, but it didn't look quite as neatly abstracted as I'd hoped. I'm hoping there'd be a Ledger.js lib that we could drop in for getting listed accounts & requesting signatures.

[izqui]

@flyswatter My Ledger Nano S is arriving on Friday. Will think about what a good flow could be then.

Regarding signing transactions, i think showing the Metamask confirmation screen but without buttons, indicating to perform the confirmation using Ledger's hardware could be an option.

[izqui]

I have been doing some research and it seems that the Ledger only allows to sign transactions and not arbitrary data.

AFAIK, this would make deploying contracts or signing proofs impossible.

Also, regarding My Ether Wallet implementation:

• MEW doesn't allow using Ledger for signing messages (https://www.myetherwallet.com/#sign-message)
• MEW offers the option to deploy a contract using the Ledger, but I cannot get it to work (https://www.myetherwallet.com/#contracts)

I have talked to Ledger support to see if they can give me an status update on this.

[danfinlay]

Good initial research.

Since Ledger has apps, I wonder if we could convince them to add message signing support.

In addition to removing buttons on the approval screen, I'd want a message, and then there is still the question of what the UI to pair should look like.

[izqui]

Quick update: I emailed Ledger support regarding arbitrary data signing, and referenced me to their public trello (https://trello.com/b/5nQ1mdzt/ledger-roadmap) where there is no reference of this feature/bug.

I was invited to join their Slack to continue the conversation (EDIT: Was just told that "we're already looking at that for other integrations, should happen shortly" 🙌)

Regarding confirmation screen, this is what they have in their Ethereum Wallet implementation (MEW doesn't show any UI related to the ledger)

The 'pair' should offer a list of all Ethereum addresses derived from the Ledger seed (MEW solves this well) and when the user selects one, save that address and the fact that is a Ledger address (no private key) that requires Ledger interaction to work with.

[recmo]

@LogvinovLeon and I implemented a LedgerWalletProvider that connects to the Ledger wallet over U2F. This should be an easier starting point than the MyEtherWallet code.

[recmo]

The Ledger wallets are based on a HD wallets and use derivation paths. This is a bit of a mismatch with Ethereuem/web3 that is based on a flat list of accounts.

The Ledger supports (as far as I have tested) any derivation path of the form m/44'/60'/*where * can be empty. (61 for ETC)

Their own Ethereum Wallet currently only uses the address available on m/44'/60'/0'/0. From their slack channel future versions will support multiple addresses, of the form m/44'/60'/${account}'/${address}. But this is not final and will be given further thought before implementation.

MyEtherWallet uses addresses available on m/44'/60'/0'/${address}.

For the LedgerWalletProvider we currently only implement m/44'/60'/0'/0 (our goal is to be compatible rather than flexible).

[danfinlay]

@Recmo That is fantastic stuff! The linked code is a perfect starting point.

I'll need to either fork it to break out the core functionality or wrap it strangely though, since we don't use signing subproviders directly.

Maybe once I break that out into a small module, we could share that piece to share improvements and fixes.

Anyways, thanks a lot, I hope to work off this soon!

[recmo]

Thanks! Feel free to fork it, we are very open to pull-requests. It implements the Web3-providers API, but has no dependencies on it. You should be able to use it on it's own just as easily:

const Ledger = require('ledger-wallet-provider')
ledger = new Ledger()
ledger.getAccounts(console.log)
ledger.signTransaction(...

The happy-paths are working, we will soon be adding more error handling and feedback (ie. "Browser unsupported").

[danfinlay]

Oh that surface is actually closer to what I wanted than I thought! Very cool!

[chafey]

+1, would love to see this feature done. I have a ledger nano s and ledger blue so can help test, maybe code if given some guidance

[linagee]

Just looking at ledger-wallet-provider mentioned above, nice! https://github.com/Neufund/ledger-wallet-provider

[danfinlay]

Noting another nice looking ledger client available: https://github.com/LedgerHQ/ledger-node-js-api

[MicahZoltu]

EDIT @flyswatter helped me find out that MetaMask is currently following BIP 44 spec. As long as you all keep that up then I'm happy with your implementation. :) Leaving this here for future readers that end up in this space, but you can mostly ignore this unless there is a plan to move away from BIP 44.

Cross posting my comment here. Please don't do what MEW and the Ledger app did (rumors suggest it was an accident that made it to release and now change is hard) and use a m/44'/* path without actually following the rest of the (BIP 44 spec). When you all get around to implementing this, please follow the spec and either create a new purpose (first segment) or use purpose 44' and follow the spec associated with that : m / purpose' / coin_type' / account' / change / address_index

Alternatively, do something totally different with regards to the path, just don't have it start with 44'. :)

Soapbox aside, what are your plans for this? As a dApp engineer working on Ledger integration, compatibility with MetaMask is important to us and we would like to make sure we are on the same page with the various wallet/integration providers.

You can see what other wallets are doing over in that thread (I believe you all already have some representation there) but it is more even more important to follow the standard when integrating with a HW wallet because users will expect the hardware wallet to show the same accounts no matter which tool they use to interact with the wallet. It is unfortunate that a bug in an early release of the Ledger app has resulted in this much community disconnect, but I think getting back on track with current standards until there is an official Ethereum one is critical for good UX for all of our users.

[GramDev1]

Do you have an ETA on this? It would really enhance the functionality of the ledger to use smart contracts.

[WestonReed]

I think this would be an excellent feature, and add a lot of security and usability to MetaMask. +1

[danfinlay]

For the people here asking for an update: At MetaMask, we also want this feature incredibly badly, and are currently doing some UX preparatory work to pave the way for a good integration.

Things have been a little slow to progress lately, we've had to put more time on fixes and support than usual, which has been a healthy expression of a still-exponential growth rate.

That said, after a few more core tuneups around how we manage nonces, and maybe a little token support, this is one of my top goals for MetaMask.

I think you can probably expect this by the end of the summer.

[woodydeck]

It's absolutely vital the hardware wallet support. There will be zero adoption of ethereum without commerce being made easy (metamask), and there will be no commerce unless there is a safe way to transact. Too many trojans around for people to ever trust anything more than walking around money in metamask. I would prefer metamask to be my main hotwallet with a ledger or Trezor.

[burz]

Any updates on this? Ledger support would be very nice.

[2-am-zzz]

We've been burdened with meeting demands for token support and maintaining bug fixes that we have not begun work on hardware support, even though several members of the team really want to get started on it. We'd love it if the community could help.

[burz]

@Zanibas: Awesome. Is there any documentation by any chance on how one would have to interface with MetaMask and the hardware wallets (or I guess wallets in general) in order to get this done?

[kumavis]

@burz first part is to match the public API of our keyring class https://github.com/metamask/eth-simple-keyring https://github.com/metamask/eth-hd-keyring then next step will be to do the ui elements

[danfinlay]

Nice looking tutorial on Ledger integrating code: https://blog.neufund.org/integrating-ledger-nanos-into-your-dapp-using-neufunds-js-wallet-provider-ecb71ca8f7b6

[vonpupp]

Support for ledger nano s would be awesome! It is becoming the most popular hw wallet.

[danfinlay]

Ledger reached out to let us know about a new integration guide:

https://blog.neufund.org/integrating-ledger-nanos-into-your-dapp-using-neufunds-js-wallet-provider-ecb71ca8f7b6

[LogvinovLeon]

@danfinlay Looks like it's the same guide You've posted on the 1st of September ;)

[danfinlay]

Whoops. Well, should get some time towards this a bit after Devcon.

[BlinkyStitt]

Any progress on this? Anything blocking that I can maybe help with?

[2-am-zzz]

@WyseNynja if you have any ideas on what an ideal interface for what the integration would look like, post them here! Also, see @kumavis last post above for details about the API.

The new excuse is: Cryptokitties and the resultant explosion of users has slowed us down again. Perhaps now is a great time for us to put this up to a bounty.

[drreen]

This!

[alexwagg]

Hello, What is the status on this? What can I do to help? If you send some links I will look into things, this is a very very important feature for metamask, surprised you didn't have this already!

[danfinlay]

Partly, we still need design on this. Our one designer is stretched pretty thin lately, we have a full new UI coming very soon. Once it's launched, we will be putting some focus on new account types, including hardware.

Until that general protocol is established, this is a very transitional time to try to contribute to this feature.

My goal is to make "new signers" easy to add, and this requires a bit more consideration than just cramming in Ledger or Trezor support. We're trying to do it right, and pave the way for a continuously good user experience.

Also, this week, we're completely consumed in gas related issues, scaling issues, issues that are hit by the many new users who keep coming on to the platform.

[gre]

I was about to start working on this and per @danfinlay comment, I'm going to HODL a bit (after investigating a bit, I figured out what just need to be done is to implement a "Keyring" class, and this should be quite straightforward. but on the UI perspective, obviously there are things to figure out).

This feature means that MetaMask could basically be a proxy to the Ledger device and that would be HUGE because you benefit both of the hardware wallet security and you benefit that MetaMask is supported on many websites (e.g. RadarRelay).

Anyway, feel free to ping me if you need help integrating things, we have recently refactored our JS libraries @LedgerHQ ( https://github.com/LedgerHQ/ledger-node-js-api/pull/37 which should be merged soon)

[bluebirdly]

@Recmo is getMultipleAccounts(path, offset, accountno) operational please? I've tried the standard path = "44'/60'/0'/0", but the callback return empty.

[Rafaqut]

Can't wait for this to be implemented. I really like using metamask!

[chuckishungry]

It would be great if MetaMask support for Ledger devices was prioritized. Lots of people are switching to MEW because they already support Ledger Nano and Nano S.

[cslarson]

You can't use MEW in with a dapp though so yeah for simple transfers that's fine, but to use something like create a dai cdp MEW doesn't help and having hardware wallet support would be wonderful.

Just throwing the idea out there @danfinlay but is there a bounty the community (r/ethtrader, for example) could contribute to to expedite hardware wallet support in metamask?

[ghost]

@cslarson you actually can use MEW for DAI CDP if you're willing to fiddle around with the contract ABI in MEW. It's not particularly easy but I have done it before when wanting to store a CDP on my ledger.

I found the best way is to transfer ETH out to MM to create the CDP, then 'give' the CDP back to my ledger address. Then the only thing you need to use MEW to do is call the 'give' function on the contract to return it to your MM address to manage it again.

[ghost]

Following on from the bounty suggestion, https://gitcoin.co could be a good option

[gre]

following previous comment, we have released at Ledger (and continue improving on) a starter kit to make dapps with both metamask and ledger device: https://www.npmjs.com/package/create-dapp . NB: In that starter, the current auth workflow is basically something similar to MEW: which probably isn't the final answer to the problem exposed in here (because dapps need to manually migrate to this approach), but at least it's a working solution we have today.

That said, it's totally possible to have metamask directly connecting to the Ledger device, and, actually, you can inspire from our work implementing this starter kit. basically you can use @ledgerhq/web3-subprovider library that implements a subprovider to work with provider-engine. This library is still bleeding edge and API is likely to change a bit.

[danfinlay]

Again, this is a top priority for us at MetaMask, but we're not going to just throw it together, we're ensuring that the way we add multiple signing strategies is modular, so that we can more easily add different signers in the future.

We are mostly bottlenecked by our number of core contributors, and we are actively hiring to relieve this bottleneck.

While we are big fans of bounties, and have used GitCoin for a variety of enhancements and bug fixes already, we are unlikely to use bounties for this particular feature, since it's highly architectural, and involves changes with a large portion of the project, so it would require tight enough integration with the core team that it might as well be done by the core team anyways.

I promise, this is one of our top priorities for the moment, up there with per-domain log-in for privacy. No bounty nor +1 on this thread is going to speed it up. It's already a top priority. We are simply scaling our currently very small team while balancing maintenance, integrations, and support.

[mickayz]

One thought I have had is that maybe Metamask can be forked and have all the guts of transaction signing yanked out and replaced with the ledger API? That way the forked Metamask wont have to worry about multiple accounts, just the one with the ledger.

Of course, that wouldn't be worth doing if Metamask will be adding this feature immediately after, but if it will be taking some time to support, then maybe it is worthwhile as a stop gap solution?

Right now users that choose to protect their funds in the best way possible are being excluded from much of the blooming dApp ecosystem, which is a pretty big bummer.

[ghost]

No bounty [...] on this thread is going to speed it up.

@danfinlay , dare to bet?

[danfinlay]

One thought I have had is that maybe Metamask can be forked and have all the guts of transaction signing yanked out and replaced with the ledger API? That way the forked Metamask wont have to worry about multiple accounts, just the one with the ledger.

Be my guest. Everyone says they want to fork whenever we're slow on a feature, but actually maintaining software is hard.

@danfinlay , dare to bet?

I mean, if you're going to put it that way, I'd just have to clarify: We would hold this feature to the highest standard, and be very picky about what we had accepted.

That said, what size bounty would get you pursuing it on the side? You've done good bounty work so far. I might be willing to write up acceptance criteria for those brash enough to dare the challenge.

[mickayz]

To clarify, I meant no disrespect. I appreciate the work you and your team put into this project and understand that you don't owe us anything. I only mean to brainstorm solutions for the impatient in case anyone has the resources to create an intermediate solution. Thank you for supporting this project :)

[danfinlay]

Forking is a bit like war: we all know it's on the table.