Adding TREZOR support #330
Conversation
|
hm, it seems there were some changes in the HD paths. I will try to rebase and do something with the conflicts. |
|
@kvhnuke just refactored some stuff as this week as been the week of adjusting paths. I don't know exactly what he did but I think the intention was to have a separate config file where we could have them all & for all the different chains and avoid this in the future. If you can rebase it easily, go for it. Otherwise, @kvhnuke can merge and clean it up as he literally just set it up. I would rather not have you waste your time moving paths around. (How is it that we can have three separate PRs regarding the paths in a week. 😂 ) |
|
Haha, ok :) I will try to rebase it to make sure it works for both trezor + ledger (and for trezor it uses the |
|
@Runn1ng :( I felt super bad when I saw the PR, you can leave it as it is and I can make the changes necessary. |
|
np! I at least had the chance to discuss internally if we want to use longer paths or not for, and in the end we settled for longer paths (with "change level" added). It means we are now incompatible with ledger :/ but that happens. So yeah now it's final :) |
Uh well no, this should be avoided as much as possible. I've no problem moving to the new paths if there's one single location to review them and some assurance that they're going to stick around. |
Well, there is not really one. BIP44 (well, SLIP-0044) defines the ETH/ETC cointypes, which defines 60' / 61' / 1' distinction for the three coins (eth / etc / all testnets of all coins) However, it's still not clear how to use "accounts" and mainly "internal/external chain" in the context of ethereum. There is this discussion that is going on since march, but I don't think there is anything definitive there - ethereum/EIPs#84 . Some wallets decide to skip the chain id, some don't. I didn't want to move Ledger paths in the myetherwallet code, since people would lose their ethers just for purity's sake. :) |
|
Also, I saw that in the documentation, you use 44’/60'/160720'/index'/0 for ETC, which is different from myetherwallet paths. But I didn't actually test your chrome app with ETC. http://support.ledgerwallet.com/knowledge_base/topics/general-ledger-nano-s-faq |
|
yes, that path was temporary. Ok, so apparently the situation didn't improve much. In that case I think the best scenario for users would be to work on some common migration guide at some point and have an advanced option to specify the BIP 32 path when using a hardware wallet or a mnemonic seed in MEW. |
|
What do you propose now, short-term? Of course we want the support rather sooner than later :), and explaining various path formats (and the rationale of moving) to users well is hard... With respect to mnemonic seed: that option is there now (very recent commit, not live yet). You can choose one of the 3 paths. It's not in hardware wallet option and I don't think it belongs there - it's not up to user to choose which path do they want, I think (when they are more or less equivalent, safety-wise, in the end). |
|
long-term, it would be probably good to make an EIP for this, but I am not feeling well versed for that, since I don't really know Ethereum community that much :)) But I don't want the integration to wait until that EIP is done... :/ |
|
FWIW, I believe one of the PRs this week was custom paths on our end. Not sure the status of it, or how it works but it's there. I've had a rough week so I still haven't had time to play with all these new-fangled things. 😣 If it works as I imagine, adding messaging or commonly used paths (e.g. bip 32) wouldn't be too hard. The problem with the above is it forces the user to have some understanding of paths and different BIPs, which isn't ideal. One of the bigger issues with ETH + HD wallets is being able to find which address are in use. With the addition of tokens (and ETC), checking for an ETH balance for X addresses (let's say 10) is 10 requests. Checking for ETH & token balances (let's say 20) for 10 addresses is 200. Add checking 2 paths would make that 400 requests. As the number of tokens grow and users use their wallets more, that number will only grow. Which means that we're going to have to rely on the user to provide the path/address, or rely on some other piece of info to, at the very least, make recommendations. As far as I understand it (and I don't really so please correct me) each time you spend BTC, the next address is used as the change address, correct? Without UXTO in ETH, what happens in ETH? And does it happen with tokens as well? Def stepping into EIP territory now. I'm just thinking that it may be prudent to address the "how the hell do we reliably figure out which address(es) are in use" and that may end up providing a workable solution for not-perfectly-standard paths as well. (Especially considering the wrench that ETC has thrown into things.) |
|
I like the longer path (with change level) better. It is what BIP44 described and is what most wallets support. It's also provides one extra level for future extensions. I think it's safe to say the future EIP will set that as default too. |
|
At MetaMask we use we use npm modules Note! There is an unfixed bug in lightwallet (and bitcore) libraries that misgenerates 1/256 bip32 keys. |
|
From my end, I don't really care about the paths all that much as long as it works, so I am OK with whatever you feel appropriate in the end. :)) |
|
I'd just like it to be some kind of standard rather than a discussion in a github pull request because it seems we already have too many of those :) |
|
@btchip have fun ethereum/EIPs#84 |
|
It seems that path chosen by Karel is standard in most wallets and, according to comments above, will become a default choice soon. So +1 for merging this. |
|
So, what is the plan now? :) I don't want for this to wait forever until some EIP standard is approved. I am OK with both using the same paths as Ledger, or using separate paths (as in the PR). But I don't want the integration to wait in limbo :) |
|
Yea I'd go with the path you chose for now, if things ever changes in the future it'll be just one push away to fix it. 😊 |
|
Using separate paths in fine for me, we'll just document that in our recovery procedures, and I'd suggest we all use the EIP 84 thread to specify paths for each wallet so that upper layers can link to a single document - seems faster. |
|
Hey @Runn1ng are you planning to make the modifications to support the latest codebase, if you want me to work on it please let me know 😊 |
|
He already made the changes ...
|
|
Oh my bad! |
|
i made couple of changes on d22ecce, mainly to do the generation of the TX under one place, this way we can support contract interface too |
|
When can we expect the code released into production? |
|
we are working on couple of other issues that are affecting the contracts page, most likely by the end of today (PST) it should be live :) |
|
It is live now, yey! |
I have added TREZOR support to myetherwallet.
The communication works through Trezor Connect. It opens a popup window on connect.trezor.io domain, that handles all the "trezor stuff" and rezurns the message back. All the HW communication logic happens outside of myetherwallet in the popup.
It should work wherever wallet.trezor.io works.
Just fyi: The current Trezor firmware sends transactions without EIP-155; the new firmware, released soon-ish, will send it with EIP-155. It requires no change in the code.
I have added some things that were mentioned in the discussion here wrt. different BIP44 paths for different chains - #327 .