Skip to content

accounts, signer: fix Ledger Live account derivation path (clef) #21757

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Nov 29, 2020
Merged

accounts, signer: fix Ledger Live account derivation path (clef) #21757

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
213d80e
signer/core/api: fix derivation of ledger live accounts
svenski123 Oct 26, 2020
b719dbe
signer/core/api: derive ten default paths for all hardware wallets, p…
svenski123 Nov 2, 2020
d23a7d4
signer/core/api: as .../0'/0/0 already included by default paths, do …
svenski123 Nov 2, 2020
7b315a3
accounts, signer: implement path iterators for hd wallets
holiman Nov 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions signer/core/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,10 +349,13 @@ func (api *SignerAPI) startUSBListener() {
case accounts.WalletOpened:
status, _ := event.Wallet.Status()
log.Info("New wallet appeared", "url", event.Wallet.URL(), "status", status)
var derive = func(numToDerive int, base accounts.DerivationPath) {
var derive = func(numToDerive int, base accounts.DerivationPath, incrOffset int) {
// Derive first N accounts, hardcoded for now
var nextPath = make(accounts.DerivationPath, len(base))
copy(nextPath[:], base[:])
if incrOffset < 0 {
incrOffset += len(nextPath)
}

for i := 0; i < numToDerive; i++ {
acc, err := event.Wallet.Derive(nextPath, true)
Expand All @@ -361,16 +364,16 @@ func (api *SignerAPI) startUSBListener() {
} else {
log.Info("Derived account", "address", acc.Address, "path", nextPath)
}
nextPath[len(nextPath)-1]++
nextPath[incrOffset]++
}
}
if event.Wallet.URL().Scheme == "ledger" {
log.Info("Deriving ledger default paths")
derive(numberOfAccountsToDerive/2, accounts.DefaultBaseDerivationPath)
derive(numberOfAccountsToDerive, accounts.DefaultBaseDerivationPath, 2)
Copy link
Contributor

@holiman holiman Nov 2, 2020

Choose a reason for hiding this comment

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

This patch corrects Ledger Live account derivation but leaves the existing derivation for Ledger Legacy and non-Ledger devices.

This modifies the default derivation path, afaict. Shouldn't you do

log.Info("Deriving ledger default paths")
derive(numberOfAccountsToDerive, accounts.DefaultBaseDerivationPath, -1)
log.Info("Deriving ledger-live paths")
derive(numberOfAccountsToDerive, accounts.DefaultBaseDerivationPath,  2)
log.Info("Deriving ledger legacy paths")
derive(numberOfAccountsToDerive, accounts.LegacyLedgerBaseDerivationPath, -1)

?

Also, let's maybe do a sanity check that incrOffset < len(nextpath) so it doesn't just panic later on
Also,

Copy link
Contributor Author

@svenski123 svenski123 Nov 2, 2020

Choose a reason for hiding this comment

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

Thanks for taking a look.

The existing code makes a distinction between ledger and non-ledger hardware wallet by checking the url scheme for "ledger" and derives different accounts accordingly - I left this in place.

Changing it to derive 30 accounts - 10x at .../0'/0/N, 10x at .../N'/0/0 and 10x at .../0'/N irrespective of url scheme / hardware wallet type would also work in my opinion with the possible drawback of including many unused accounts to the account list and possibly confusing the user.

The array offsets are all hard coded as is the derivation path so a panic appropriate.
In fact it would be better if the locally defined derive() function could be a macro instead so bounds check errors could be caught at compile time but I don't know if golang offers this facility.

IMHO having the derivation path be defined in a separate file makes this code harder to read / maintain and would better if it were a locally defined constant (the prefix m/44'/60`/0' is very unlikely to change)

Copy link
Contributor Author

@svenski123 svenski123 Nov 2, 2020

Choose a reason for hiding this comment

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

I've added the default paths back for ledger, so you now get 10 default, 10 legacy, 10 live - so every address derived by the old code will be derived by the new code (plus more).
Also added logged warning on the level-to-increment index - should never occur with hard coded values but if something changes in future, should hopefully be helpful.

log.Info("Deriving ledger legacy paths")
derive(numberOfAccountsToDerive/2, accounts.LegacyLedgerBaseDerivationPath)
derive(numberOfAccountsToDerive, accounts.LegacyLedgerBaseDerivationPath, -1)
} else {
derive(numberOfAccountsToDerive, accounts.DefaultBaseDerivationPath)
derive(numberOfAccountsToDerive, accounts.DefaultBaseDerivationPath, -1)
}
case accounts.WalletDropped:
log.Info("Old wallet dropped", "url", event.Wallet.URL())
Expand Down