-
Notifications
You must be signed in to change notification settings - Fork 15
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
[Actor, TxHandler] add signing of txhandler through tagged signatures #511
Conversation
Added a new scriptkind parser for code quality with tests Implemented sighash calculation closure to avoid cloning and ensure immutability of the TxHandler inputs.
core/src/builder/script.rs
Outdated
@@ -162,8 +189,11 @@ impl SpendableScript for TimelockScript { | |||
} | |||
|
|||
impl TimelockScript { | |||
fn generate_witness(&self, signature: schnorr::Signature) -> Witness { | |||
Witness::from_slice(&[signature.serialize()]) | |||
pub fn generate_script_inputs(&self, signature: &Option<schnorr::Signature>) -> Witness { |
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.
Better use Option<&schnorr::Signature>
kickoff_tx_handler.get_spendable_output(3)?, | ||
SpendPath::ScriptSpend(1), |
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.
What's that 1? Magic number?
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.
yeah, we have a script indexing problem. We refer to scripts by their index. We should be tagging them someway else, open to suggestions here.
@@ -211,43 +270,6 @@ impl TxHandler<Unsigned> { | |||
Ok(()) | |||
} | |||
|
|||
// Candidate refactoring |
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.
Do you need to keep this code? Are you really going to use it in the future?
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 was thinking of doing something like this instead of script indexing (i.e. using magic numbers). We didn't do that. I don't really think this is an optimal solution (as it requires looping over all scripts), so decided to just remove it.
Also don't understand the comment here, do you want it back? I removed the lines (did not add them)
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.
Since we're now using SpendPaths, we'd need to embed some kind of script selector closure (like Box<dyn FnMut(&dyn SpendableScript) -> bool>
) into the SpendPath. That's going to be kind of expensive and slow
# Conflicts: # core/src/actor.rs # core/src/builder/script.rs
Description
Implements
partial_sign
andpartial_sign_commit
that signs the inputs that it supports.Linked Issues
Testing
Added signing for actor key/script spend scripts.
Docs
Added some rustdoc on functions that explain the behavior, but still missing some docs.