Key derivation in ethstore & rpc #4515
Conversation
NikVolf
added
A0-pleasereview 🤓
| if !account.check_password(password) { | ||
| continue; | ||
| } | ||
| let extended = self.generate(account.crypto.secret(password)?, derivation)?; |
There was a problem hiding this comment.
is this a bottleneck? can we cache extended key-pairs?
There was a problem hiding this comment.
it's not in release build at all
ethstore/src/ethstore.rs
Outdated
| continue; | ||
| } | ||
| let extended = self.generate(account.crypto.secret(password)?, derivation)?; | ||
| let secret = extended.secret().secret(); |
There was a problem hiding this comment.
would a Deref or Into implementation make this less weird?
There was a problem hiding this comment.
well it's not that weird
first secret() is ExtendedSecret of the extended key pair and second secret() is a raw H256 secret of this extended secret, i don't see logical reason to Deref them in something they are not in any way equal to
There was a problem hiding this comment.
It's a strange API design for sure. Doesn't help readability that you have to dig into the internals to know what's happening.
let secret = extended.secret(); // I have the secret!
let secret = secret.secret(); // I have the...secret secret?Would definitely make more sense (to read) as x.secret().as_raw() or something
There was a problem hiding this comment.
yeah i didn't like it either, but when it comes to individual function names, their names are legit, it's just when they are used one after another weirdness shows :)
as_raw looks nice, yeah, will rename
|
It seems like we could unify the |
|
Would be nice to adopt BIP-44 string format. Maybe in another PR. |
rpc/src/v1/impls/parity_accounts.rs
Outdated
| @@ -25,7 +25,7 @@ use ethcore::account_provider::AccountProvider; | |||
| use jsonrpc_core::Error; | |||
| use v1::helpers::errors; | |||
| use v1::traits::ParityAccounts; | |||
| use v1::types::{H160 as RpcH160, H256 as RpcH256, DappId}; | |||
| use v1::types::{H160 as RpcH160, H256 as RpcH256, DappId, Derivate, DerivateHierarchical, DerivateHash}; | |||
There was a problem hiding this comment.
Derivate not a word, should be Derive
|
@rphmeier Not sure if i want to break existing api just to introduce key derivation which is rather small and humble part of it |
|
@arkpar |
arkpar
added
A8-looksgood 🦄
gavofyork
added
A7-looksgoodcantmerge 🙄
NikVolf
added
A8-looksgood 🦄
NikVolf
force-pushed
the
hd-wallet-rpc
branch
from
9cb03ca
to
e659066
Compare
Now client can create derived address for any of his accounts and optionally save it as a new account (or just get raw address and it's up to him how to use it)
Two types of derivation:
hierarchical deterministic wallet (kind of bitcoin-compatible standard of bip0032), where you provide a series of indices for one or more sequental derivations (for example, Soft(0)/Soft(1)/Hard(0))
hash derivation where you provide just
H256(for example, invoice or other content hash) and derivation type (soft/hard)Right now it's up to client how to use derived keys - as a full account or just save it in address book after derivation (to use it later - client must then derive this address again from the same account with the same parameters and
save: true).Another option is to sign transaction with the derived key directly - coming in following pr to keep things small here