feat(crypto): Add ability to create a private key from a mnemonic #347
Conversation
crates/iota-crypto/src/ed25519.rs
Outdated
| &[ | ||
| crate::DERIVATION_PATH_PURPOSE_ED25519, | ||
| crate::DERIVATION_PATH_COIN_TYPE, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| ], |
There was a problem hiding this comment.
Fine to have this as default, but we should also allow other paths
There was a problem hiding this comment.
The whole path or just the last three?
There was a problem hiding this comment.
Hm not sure about this, the wallets we have only increase the account index for new accounts, maybe we should only allow this then so it's compatible
People that want something else can use a bip39 library directly to derive a private key with other bip32 paths
There was a problem hiding this comment.
@thibault-martinez thoughts?
There was a problem hiding this comment.
Is this question still relevant?
but we should also allow other paths
I see we are taking a path as parameter?
There was a problem hiding this comment.
This part not, as @DaughterOfMars added the path now, but the question is whether we want the full path or just an account index
| println!("Public Key With Flag: {flagged_public_key}"); | ||
| println!("Address: {address}"); | ||
|
|
||
| let private_key = Secp256r1PrivateKey::from_mnemonic(MNEMONIC, None, None)?; |
There was a problem hiding this comment.
Maybe at least one example with a password and/or path?
crates/iota-sdk-crypto/src/lib.rs
Outdated
| } | ||
|
|
||
| /// Defines the scheme of a private key | ||
| pub trait PrivateKeyScheme { |
There was a problem hiding this comment.
Splitting this in 2 feels over the top to me
There was a problem hiding this comment.
Well only some types can have a const scheme
There was a problem hiding this comment.
Ok but you can't really implement scheme() if you don't have one? Or what am I missing?
There was a problem hiding this comment.
SimpleKeypair can impl PrivateKeyScheme but not the const version
crates/iota-sdk-crypto/src/lib.rs
Outdated
| } | ||
|
|
||
| /// Defines a type which can be constructed from bytes | ||
| pub trait FromBytes { |
There was a problem hiding this comment.
Why are these 2 traits when ToFromBech32 is one?
There was a problem hiding this comment.
Because some types cannot impl FromBytes
There was a problem hiding this comment.
SimpleKeypair
|
|
||
| let path = path.into().unwrap_or_else(|| { | ||
| format!( | ||
| "m/{}'/{}'/0'/0'/0'", |
There was a problem hiding this comment.
I am asking this because I am unsure about this myself, but are you sure about the hardening in this path? AFAIK this scheme allows safe non-hardened derivation as compared to ED25519. So just checking whether you've thought about this.
There was a problem hiding this comment.
I have not thought about it and I do not know what the best way is
There was a problem hiding this comment.
monorepo has this
/// Secp256k1 follows BIP-32/44 using path where the first 3 levels are
/// hardened: m/54'/4218'/0'/0/{index} Secp256r1 follows BIP-32/44 using path
/// where the first 3 levels are hardened: m/74'/4218'/0'/0/{index}.
There was a problem hiding this comment.
Probably good to add these test cases https://github.com/iotaledger/iota/blob/beae5b82e5564ac4bdaaa68a608a46632cc1073f/crates/iota/src/unit_tests/keytool_tests.rs#L309-L411
There was a problem hiding this comment.
done, thanks
There was a problem hiding this comment.
I still wonder whether we should take the whole path. Maybe two functions?
| scheme: SignatureScheme, | ||
| phrase: &str, |
There was a problem hiding this comment.
maybe the phrase should come first, then the scheme?
There was a problem hiding this comment.
I feel like the scheme is the most important thing because it determines the type of key, which is why I put it first
There was a problem hiding this comment.
I agree with scheme being first since it's being matched on and the rest of the params are just being forwarded
There was a problem hiding this comment.
isn't the method called from_mnemonic which is represented more by the phrase than by the scheme?
Actually, I would even prefer having it in last position (even if its the the most important internally), reason being that the argument list should be consistent with the other methods. Wouldn't those signatures be better overall from a user standpoint?
.from_mnemonic(phrase, password, path)
.from_menmonic(phrase, password, path, scheme)
There was a problem hiding this comment.
TBH I would also be fine with simply removing this method and the impls on SimpleKeypair. You can just create the inner keys and construct SimpleKeypair from them 🤷♀️
There was a problem hiding this comment.
TBH I would also be fine with simply removing this method and the impls on
SimpleKeypair. You can just create the inner keys and constructSimpleKeypairfrom them 🤷♀️
I think I agree with that, we also don't have a method to create a random SimpleKeypair with one of the schemes
Description
Allow creating private keys from mnemonics. While working on this, it made sense to separate the
PrivateKeyExttrait into several smaller traits.Closes #266