-
Notifications
You must be signed in to change notification settings - Fork 240
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
Tweaks to eth signer #1526
Tweaks to eth signer #1526
Conversation
|
||
# Pick the signer implementation(s) you need by enabling the | ||
# corresponding features. Note: I had more difficulties getting | ||
# ecdsa compiling to WASM on my mac; following this comment helped: | ||
# https://github.com/rust-bitcoin/rust-bitcoin/issues/930#issuecomment-1215538699 | ||
sr25519 = ["schnorrkel"] | ||
ecdsa = ["secp256k1"] | ||
eth = ["keccak-hash", "secp256k1", "bip32"] | ||
unstable-eth = ["keccak-hash", "ecdsa", "secp256k1", "bip32"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We pull in the ecdsa::Keypair/sign/verify for instance, so need to ensure this stuff exists too :)
InvalidDerivationIndex(bip32::Error), | ||
/// Invalid phrase. | ||
#[display(fmt = "Cannot parse phrase: {_0}")] | ||
InvalidPhrase(bip39::Error), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We weren't using this error anywhere
#[cfg(feature = "unstable-eth")] | ||
#[cfg_attr(docsrs, doc(cfg(feature = "unstable-eth")))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Niklas suggested we could mark this unstable for now, which I think is a good idea just so that I don't need to worry about it being perfect just yet :) Maybe we find we need t osupport some other things to make this more useful or whatever.
impl Default for Derivation { | ||
fn default() -> Self { | ||
Self::Soft | ||
impl DerivationPath { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the derivation path stuff to a separate struct so that we can accept arbitrary derivation strings and have a couple of nice helpers for "common" things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great. needs a test for the empty derivation path and then its ready to go
|
||
// TODO: Currently, we use bip32 to derive private keys which under the hood uses | ||
// the Rust k256 crate. We _also_ use the secp256k1 crate (which is very similar). | ||
// It'd be great if we could 100% use just one of the two crypto libs. bip32 has |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's open a PR to the bip32 crate to update the secp256k1 crate then :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I almost did this hehe, but yup definitely; I think when this merges we can open an issue in Subxt to avoid thedupe crates and then go from there :)
…tion path mnemonics vs seeds
5e22a9f
to
3adc806
Compare
… taking secret key bytes
Ok, I added a test using some of the vectors in BIP39; these compare mnemonic + password vs seed. In the process of doing this, I realised that our CI isn't running but we'll spot any issues once it merges with the other branch I guess! |
@ryanleecode I finally got to having a pass over #1501 and thought I'd just suggest tweaks in the form of a branch! Lemme know what you reckon, and if all good then we can merge this into your PR and I reckon it's good to go :)
The tweaks:
DerivationPath
so that people can configure it more arbitrarily.