-
Notifications
You must be signed in to change notification settings - Fork 7
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
Support user implemented signer. #495
Conversation
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.
utACK.
This doesn't touch BtcSwapTx::partial_sign
, which we use in the context of refund_tx_wrapper.partial_sign()
, which take as arg the swap's refund keypair and internally calls MusigSession::partial_sign
.
However this doesn't depend on user keys, so indeed the user-supplied signer shouldn't play a role for partial_sign
.
lib/core/src/signer.rs
Outdated
.to_keypair(&secp); | ||
let s = msg.as_slice(); | ||
|
||
let double_hashed_msg: Message = Message::from_digest_slice(s) |
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.
nit: variable name is misleading as we don't know it's double hashed
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.
Fixed
lib/core/src/wallet.rs
Outdated
// Prefix and double hash message | ||
let mut engine = sha256::HashEngine::default(); | ||
engine.write_all(LN_MESSAGE_PREFIX)?; | ||
engine.write_all(message.as_bytes())?; | ||
let hashed_msg = sha256::Hash::from_engine(engine); | ||
let double_hashed_msg = Message::from(sha256::Hash::hash(&hashed_msg)); | ||
let double_hashed_msg = Message::from_digest(hashed_msg.into_inner()); |
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.
This doesn't seem to be double hashing any more?
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.
Indeed calling sign-message
/ check-message
with the CLI finds the signature as invalid.
Changing the line to
let double_hashed_msg = Message::from_digest(sha256::Hash::hash(&hashed_msg).into_inner());
fixes the problem.
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.
Good catch guys. I fixed that and added a test for this case.
lib/core/src/model.rs
Outdated
// The master xpub encoded as 78 bytes length as defined in bip32 specification. | ||
// For reference: https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#user-content-Serialization_format | ||
fn xpub(&self) -> Result<Vec<u8>, SignerError>; | ||
|
||
// The derived xpub encoded as 78 bytes length as defined in bip32 specification. | ||
// The derivation path is a string represents the shorter notation of the key tree to derive. For example: | ||
// m/49'/1'/0'/0/0 | ||
// m/48'/1'/0'/0/0 | ||
// For reference: https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#user-content-The_key_tree | ||
fn derive_xpub(&self, derivation_path: String) -> Result<Vec<u8>, SignerError>; | ||
|
||
// Sign an ECDSA message using the private key derived from the given derivation path | ||
fn sign_ecdsa(&self, msg: Vec<u8>, derivation_path: String) -> Result<Vec<u8>, SignerError>; | ||
|
||
// Sign an ECDSA message using the private key derived from the master key | ||
fn sign_ecdsa_recoverable(&self, msg: Vec<u8>) -> Result<Vec<u8>, SignerError>; | ||
|
||
// Return the master blinding key for SLIP77: https://github.com/satoshilabs/slips/blob/master/slip-0077.md | ||
fn slip77_master_blinding_key(&self) -> Result<Vec<u8>, SignerError>; | ||
|
||
// HMAC-SHA256 using the private key derived from the given derivation path | ||
// This is used to calculate the linking key of lnurl-auth specification: https://github.com/lnurl/luds/blob/luds/05.md | ||
fn hmac_sha256(&self, msg: Vec<u8>, derivation_path: String) -> Result<Vec<u8>, SignerError>; |
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.
Can you ///
them for rustdocs?
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.
fixed
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 good, those flutter example files just need removing
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.
All commits to packages/flutter/example
can be removed, the example app removed in PR #510
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.
LGTM
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.
LGTM
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.
Tested ACK
This PR allows users o initialize the sdk via their own signer implementation instead of passing the mnemonic phrase to the sdk.
A new function was added connect_with_signer and the Signer trait should be implemented and passed to the function.
For now we excluded flutter and react native because of binding issues.