feat(cli): wallet uses eip2333 for keys

[iancoleman]

Wallet keys are derived from an eip2333 mnemonic. The mnemonic is optional so existing wallets (which are just a key) will still load as usual. Currently the mnemonic is not used for anything but will be incorporated into a future safe wallet backup/restore command.

It's much simpler to safely store a 24 word mnemonic than it is to store a 64 char hex key.

---

I'm not 100% sure if the dependency management is appropriate, here's a bit of a story about how it came to be this way:

crates.io has a crate that claims to do eip2333 - bls_key_derivation

However, the tests in the crates.io version of bls_key_derivation do not match the eip2333 spec tests.

bls_key_derivation tests
does not match
eip2333 spec
Note the seed is the same but the derived child is different.

This crate was forked to https://github.com/taiyi-research-institute/rust-bls-derivation. For our purposes the main fix was to "modifying the function hkdf_mod_r" so it's compliant to the latest eip2333 spec.

However this forked crate is not available on crates.io and it doesn't currently build.

So I forked it to https://github.com/iancoleman/rust-bls-derivation and updated the broken parts in commit 0025897.

Unfortunately, fixing the broken curv dependency required a fork to https://github.com/iancoleman/curv with fix in commit dc03c70.

So we depend on two broken crates which were forked to my own copies, rust-bls-derivation and curv.

This feels like a very messy dependency chain. But it works and is correct to the current eip2333 spec, as well as cross compatible with other eip2333 implementations such as this eip2333 tool.

Description

reviewpad:summary

[joshuef]

I guess these the key-derivation dep should not need much work going forwards? The specs are met, and we're there? There'll be no new features etc. So we could just publish these to crates and look to pull in any significant changes from the fixed dep there.

I'm not sure if you want to publish those under your own name, or we should roll that into maidsafe? I'm happy either way.

w/r/t curve, do you think your fix can be upstreamed?

[iancoleman]

Yeah the spec for eip2333 is very stable now, I can't foresee any changes needed to these crates in the future, unless there's dependency updates (eg rand 0.7 -> 0.8 was a bit of a pest from a dependency viewpoint, wasm compatibility another consideration)...

I'm not keen to publish crates under my name.

The curv crate I forked from https://github.com/ZenGo-X/curv hasn't been updated for over a year and has 38 open issues, 12 open PRs, so I'd say probably not much chance of getting this change in.

If I were to look bigger picture and had the time, I'd probably aim to develop a crate called eip2333 with a nicer api than bls_key_derivation but am not able to do it just now. Might be something maidsafe could sponsor in the future.

[joshuef]

Okay fair enough.

Can we port those repos to maidsafe and I'll get them published as crates under maidsafe, perhaps some sn_ specific naming to avoid confusion. Does that sound okay to you?

Aye an improved crate there would not be a bad thing for later on, for sure 👍

[iancoleman]

Can we port those repos to maidsafe and I'll get them published as crates under maidsafe, perhaps some sn_ specific naming to avoid confusion. Does that sound okay to you?

Sure can. Should we do it same as blsttc, transfer the repos over to maidsafe? Prefix with sn_ for crates.io uniqueness?

[joshuef]

Sure can. Should we do it same as blsttc, transfer the repos over to maidsafe? Prefix with sn_ for crates.io uniqueness?

Yup and yup. 👍 🙇

[iancoleman]

Tried to transfer iancoleman/sn_curv but got an error:

You don’t have the permission to create public repositories on maidsafe

Not sure the best way to get around this? Docs indicate I'll need this permission within maidsafe to transfer.

[joshuef]

You should have that now.

[iancoleman]

Ok sn_curv and sn_rust_bls_derivation have been transferred, please remove that permission when you can.

I'm going to move mnemonics out of sn_transfers and into higher-level sn_cli.

This keeps avoids adding high level stuff in the sn_transfers crate, and hopefully allows the mnemonic to be more easily used for a variety of cli related things, like account packets and backups.

[joshuef]

Excellent, yes I suspect we'll want the account packet to use mnemonics, or be able to start a packet from an existing wallet/mnemonic (I guess we can't actually check the difference there).

[joshuef]

I'll get both crates published now.

[joshuef]

curv is published, bls_derive is hitting issues Caused by: failed to select a version for the requirement `curv-kzen = "^0.10.1"` candidate versions found which didn't match: 0.10.0, 0.9.0, 0.8.3, ... location searched: crates.io index required by package `sn_bls_ckd v0.2.1 (/Users/josh/Projects/sn_rust_bls_derivation/target/package/sn_bls_ckd-0.2.1)` if you are looking for the prerelease package it needs to be specified explicitly curv-kzen = { version = "0.8.0-rc3" } perhaps a crate was updated and forgotten to be re-vendored?

Missing crate there? I've readded perms in case that's another to transfer over

[iancoleman]

Ok that should be fixed with the latest commit maidsafe/sn_rust_bls_derivation@a74c0ad

[joshuef]

Okay, that's published, though i didn't notice, it's as sn_bls_ckd , that makes sense?

[JasonPaulGithub]

This is still in Draft. Just checking in.👋

[joshuef]

FYI @iancoleman this pr here I think gets us a good location for adding in mnemonics to the account packet flows.

[joshuef]

Closing as merged via #1635