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

Add HD wallet functionality #41

Closed
wants to merge 7 commits into from
Closed

Conversation

davidyuk
Copy link
Member

hs-key functions are based on ed25519-hd-key package, both implementations correspond to SLIP-0010 specification.
HMAC-SHA512 is calculating by tweetnacl-auth package, it has the same owner as tweetnacl, and it using SHA512 from tweetnacl.
Accounts derivation path is taken from BIP-0044, except that in all path segments used hardened derivation because SLIP-0010 it not supports public derivation.
457 is a number generated by random.org and used as Aeternity coin type, probably it should be registered in SLIP-0044.

https://www.pivotaltracker.com/story/show/157591833

@davidyuk davidyuk requested review from outergod and muxe May 29, 2018 08:33
@davidyuk
Copy link
Member Author

sign.keyPair.fromSeed is used instead of lowlevel.crypto_sign_keypair because actually, it does the same, it uses seed as a private key.

@muxe
Copy link
Contributor

muxe commented May 29, 2018

From a non-crypto point of view:

Only things I think would be nice to have:

  • Check if mnemonic is a valid phrase
  • have an option or utility to get "human readable" hex/base58-string versions

@muxe
Copy link
Contributor

muxe commented May 29, 2018

The other thing I noticed: The change almost doubles the size of the generated library. Just something we should keep an eye on

Before:
screenshot from 2018-05-29 11-34-37

After:
screenshot from 2018-05-29 11-31-53

@dadaphl
Copy link

dadaphl commented May 29, 2018

@e-user, @muxe and me had a chat about this size problem. We came up with the understanding that the mnemonic part should not be part of the aepp-sdk-js. It should be a project on its own.

I have created an issue (#42) for that for further discussion.

Lets put this PR on hold until we've reached consensus.

Please discuss all separation related matters in issue #42 .
Please discuss all HD implementation related matters here .

@davidyuk
Copy link
Member Author

Check if mnemonic is a valid phrase

It is performed in the constructor of Mnemonic, I don't think that we should make that validation more strict.

have an option or utility to get "human readable" hex/base58-string versions

We have a getReadablePublicKey function, we can make it accept also Buffer objects and call it when it is necessary to display an address in UI (my preferred option). Or we can rewrite all functions to accept public keys formatted in base58 with a prefix then will be possible to return public keys formatted in the same way. The option to return public key formatted in base58 instead of Buffer object seems less reasonable.

The change almost doubles the size of the generated library

The problem is in bitcore-mnemonic package: it depends on bitcore-lib that is unused in this case, and too large unorm package. bitcore-lib also brings lodash. Probably I can rewrite bitcore-mnemonic without dependencies and word lists for other languages. Also, I can make generateSaveHDWallet accept just seed instead of a mnemonic phrase and use bitcore-mnemonic on the side of Base app (it already depends on this package).

@davidyuk
Copy link
Member Author

davidyuk commented May 30, 2018

I have found more popular bip39 package that can be used instead of bitcore-mnemonic. bip39 does not depend on bitcore-lib, but it still uses unorm and has word lists for several languages. unorm package is used for Unicode normalization, it implements String.prototype.normalize functionality, and as I understand it is not actual for English word list used in BIP-39.

@davidyuk
Copy link
Member Author

davidyuk commented May 30, 2018

With bip39 bundle size is 3.77 mb:
127 0 0 1_8888_

@dadaphl
Copy link

dadaphl commented May 30, 2018

is that more or less than before? I cant read any opinion from your last comment. Do you consider that a lot or little?

bip39 is not performing it by default.
@davidyuk
Copy link
Member Author

davidyuk commented May 30, 2018

parsed size
before 3.14 mb
with bitcore-mnemonic 6.71 mb
with bip39 3.77 mb
with aeternity/bip39 3.23 mb

@davidyuk
Copy link
Member Author

Using aeternity/bip39 the bundle size is 3.23 mb.

davidyuk added a commit to aeternity/hd-wallet-js that referenced this pull request Jun 1, 2018
@outergod
Copy link
Contributor

Superseded by https://github.com/aeternity/hd-wallet-js/. Thanks @davidyuk !

@outergod outergod closed this Jun 11, 2018
@davidyuk davidyuk deleted the feature/hd-keys-by-slip-0010 branch June 11, 2018 12:04
@davidyuk davidyuk mentioned this pull request Feb 5, 2019
@nduchak nduchak restored the feature/hd-keys-by-slip-0010 branch February 6, 2019 16:25
@noandrea noandrea deleted the feature/hd-keys-by-slip-0010 branch February 15, 2019 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants