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

Support Ethereum-type derivation paths #773

Closed
joelamouche opened this issue Nov 17, 2020 · 13 comments
Closed

Support Ethereum-type derivation paths #773

joelamouche opened this issue Nov 17, 2020 · 13 comments

Comments

@joelamouche
Copy link
Contributor

In common/packages/keyring/src/keyring.ts
there is a comment : FIXME Need to support Ethereum-type derivation paths

Is this fixed yet? Are derivation path working for ethereum? If yes, what is the format?

Thanks in advance @jacogr

@joelamouche joelamouche changed the title Support Ethereum-type derivation paths` Support Ethereum-type derivation paths Nov 17, 2020
@jacogr
Copy link
Member

jacogr commented Nov 17, 2020

No. It only does the Substrate style. (Which is not compatible with Ethereum at all)

@joelamouche
Copy link
Contributor Author

Do you have an idea of the amount of work and complexity it would take to support that?

@jacogr
Copy link
Member

jacogr commented Nov 17, 2020

Implement bip-44, basically. The big issue I have with this is that I'm trying to get rid on not-frequently-used stuff, so generally not happy to just reach for JS libs and get it and all it's dependencies.

The only complexity is that Substrate always uses suri, so in the case of Ethereum the string (before stripping) won't be full compliant, i.e.

seed 0x1234 with path m/44'/0'/0'/1/1 would probably look like 0x1234/44'/0'/0'/1/1 in suri-like format before being used.

(There is a use for bip-44 style derivation in the case of eg. Ledger as well, where ed keys are derived that way, even though it supports Polkadot/Kusama)

@jacogr
Copy link
Member

jacogr commented Nov 17, 2020

I'm quite happy to do this on the apps UI first, with hdkey libs (none specific preferred), before adding it here.

So my concern with the crypto libs is stuff like this - https://github.com/cryptocoinjs/hdkey/blob/master/package.json#L39 - it actually took ages to get secp256k1 our and deduped (so elliptic is not used here only), both are absolutely massive.

So would suggest -

  • add via apps UI
  • apps UI does derivation
  • derived keys passed through the the keyring

It will also be less complex, just pull in libs and be done with it.

@crystalin
Copy link

Hi @jacogr ,
What is the status on this?

@jacogr
Copy link
Member

jacogr commented Jan 20, 2021

It is not on the to-do-immediately list, so not a priority to get done.

I have taken some looks at some of the derivations libs out there and it pulls in the wide whole world of dependencies in all cases I have seen. So atm have no clean, light solution for this - which is needed if it as to go into common.

@crystalin
Copy link

I'd be happy to get your suggestion on what should be done here and what library you suggest and maybe @joelamouche will work to have it implemented. It is not an urgent matter but it feels weird being able to use the private key and not the mnemonic for our chain.

@jacogr
Copy link
Member

jacogr commented Jan 20, 2021

So HDKey, aka https://github.com/cryptocoinjs/hdkey/blob/master/lib/hdkey.js, is actually a good candidate - and the guys are solid.

So if we can extract the derive (which links to deriveChild) from that and make it work with the keyring, it can work. (Out of the box they really all focus on their keypair types, which is not compatible.) So basically, my approach would be -

  • use hdkey as a base (we certainly on't need serialize, toJSON, etc.)
  • rip out the derives and do them in TS
  • adjust so it uses the libraries already available in util-crypto (all the stuff is there, eg. secp2561k1, hashing, ...)
  • make it work with the pairs (would need some adjustment)
  • add tests

I don't believe it is major, but don't see much of another option without some serious hacks. The above approach is very lightweight.

@joelamouche
Copy link
Contributor Author

@jacogr Hi, justg pinging you because I'm not sure you're receiving notifications about my progress and comments on #858

@jacogr
Copy link
Member

jacogr commented Feb 17, 2021

Will look at the comments, I have only peeked in at the code, sorry about that.

@joelamouche
Copy link
Contributor Author

Cool thanks

@joelamouche
Copy link
Contributor Author

done in #894

@polkadot-js-bot
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue if you think you have a related problem or query.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators May 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants