Skip to content
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.

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

Misleading HD derivation path in UI #7

Open
webmaster128 opened this issue Aug 24, 2020 · 5 comments
Open

Misleading HD derivation path in UI #7

webmaster128 opened this issue Aug 24, 2020 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@webmaster128
Copy link

Hello everyone! I just tested keystation together with https://wallet.cosmostation.io/

I was very confused about the HD derivation path that is used. For Cosmos Hub, the path should be m/44'/118'/0'/0/0, so some of the components are hardened, some are not. In the UI it simply shows 44/118/0/0/0, which indicatiates a path with 5 non-hardened components is used.

But the address derivation is correct (e.g. menmonic economy stock theory fatal elder harbor betray wasp final emotion task crumble siren bottom lizard educate guess current outdoor pair theory focus wife stone leads to address cosmos1pkptre7fdkl6gfrzlesjjvhxhlc3r4gmmk8rs6, which is m/44'/118'/0'/0/0 and compatible with Cosmos SDK and CosmJS).

It would be nice if the UI was more precise about the derivation, which is critical for interoperability as well as privacy.

@Booyoun-Kim Booyoun-Kim self-assigned this Sep 8, 2020
@Booyoun-Kim
Copy link

I also thought about whether I should use the path starting with m. However, I decided to use the same as the Ledger Nano javascript library. I wanted to match the development specs of Ledger Nano and Keystation in our web wallet. If you have any better opinions, please let me know.

https://github.com/Zondax/ledger-cosmos-js/blob/master/tests/basic.ispec.js#L29

@webmaster128
Copy link
Author

webmaster128 commented Sep 8, 2020

I don't care much about the m/. I think it is good to display but not critical.

The main problem is 44 vs. 44' and 118 vs. 118'. Those are completely different path components.

The comment // Derivation path. First 3 items are automatically hardened! says that the resulting derivation path is not [44, 118, 0, 0, 0] but [44 + 2 ** 31, 118 + 2 ** 31, 0 + 2 ** 31, 0, 0].

The way the Ledger lib communicates with the Ledger app is really an implementation detail of Ledger and nothing that should be communicated to the user.

@Booyoun-Kim
Copy link

I see. I will discuss with our team member how to handle this. Thank you 😃

@webmaster128
Copy link
Author

webmaster128 commented Sep 8, 2020

By the way, CosmJS can do this for you:

import { pathToString, Slip10RawIndex } from "@cosmjs/crypto";

const path = [
  Slip10RawIndex.hardened(44),
  Slip10RawIndex.hardened(118),
  Slip10RawIndex.hardened(0),
  Slip10RawIndex.normal(0),
  Slip10RawIndex.normal(6),
];

console.log(pathToString(path)); // m/44'/118'/0'/0/6 

@Booyoun-Kim Booyoun-Kim added the bug Something isn't working label Sep 14, 2020
@Booyoun-Kim
Copy link

I will remove auto harden function. Users will be able to use the HD path as is. I'll add the parameter &version=2 to avoid confusion for existing users.
Version param: https://github.com/cosmostation/keystation/blob/dev/service/service.go#L68
Auto harden: https://github.com/cosmostation/keystation/blob/dev/www/js/import.js#L134
Auto harden: https://github.com/cosmostation/keystation/blob/dev/www/js/pin.js#L143

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants
@webmaster128 @Booyoun-Kim and others