Skip to content

Add trezor HD path for ledger wallets#10616

Merged
Gudahtt merged 1 commit intodevelopfrom
add-bip44-HD-path
Mar 10, 2021
Merged

Add trezor HD path for ledger wallets#10616
Gudahtt merged 1 commit intodevelopfrom
add-bip44-HD-path

Conversation

@Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Mar 9, 2021

Fixes: #10531

Explanation:

Allows Trezor users to access their accounts from a Ledger

Manual testing steps:

Load Trezor from mnemonic.
Load Ledger from mnemonic.
Connect to Trezor in Metamask and note the first 10 accounts.
Connect to Ledger in Metamask using Trezor from dropdown for HD path.

User should be able to access all accounts they had on Trezor from their ledger.

This is a re-post of PR #10532 by @bgits

@Gudahtt Gudahtt requested a review from a team as a code owner March 9, 2021 20:50
@Gudahtt Gudahtt requested a review from brad-decker March 9, 2021 20:50
@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2021

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@Gudahtt Gudahtt mentioned this pull request Mar 9, 2021
Closed
@brad-decker brad-decker requested review from darkwing and removed request for brad-decker March 9, 2021 21:03
@brad-decker
Copy link
Contributor

brad-decker commented Mar 9, 2021

LGTM, retagged @darkwing for review as he had already provided feedback on the previous iteration.

@metamaskbot
Copy link
Collaborator

metamaskbot commented Mar 9, 2021

Builds ready [57bfb2e]
Page Load Metrics (646 ± 50 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint46835994
domContentLoaded33682264510450
load33782464610450
domInteractive33682264410450

@Gudahtt
Copy link
Member Author

Gudahtt commented Mar 9, 2021

The main thing holding this up is testing. I'm about to test this myself locally by importing a seed from a Trezor device onto a Ledger device, to ensure the derived account is correct and functions correctly.

@Gudahtt
Copy link
Member Author

Gudahtt commented Mar 9, 2021

I have tested this with a Ledger X, and I was able to successfully import a seedphrase generated on a trezor device, and to connect and sign with accounts from this device (I tested with the 2nd account, because the first is identical between the Ledger Live and the BIP44 HD paths).

LGTM!

darkwing
darkwing approved these changes Mar 10, 2021
View reviewed changes
Copy link
Contributor

@darkwing darkwing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent! This worked great for me!

@Gudahtt Gudahtt merged commit 669ab18 into develop Mar 10, 2021
@Gudahtt Gudahtt deleted the add-bip44-HD-path branch March 10, 2021 18:26
@github-actions github-actions bot locked and limited conversation to collaborators Mar 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@darkwing darkwing darkwing approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Ledger does not support Trezor HD path

5 participants

@Gudahtt @brad-decker @metamaskbot @darkwing @bgits