Skip to content

Implement a "phase 2" API for sign_closing_transaction #1064

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

Merged

Conversation

devrandom
Copy link
Member

This allows the signer to figure out which output is ours without potentially complicated parsing.

@codecov
Copy link

codecov bot commented Sep 1, 2021

Codecov Report

Merging #1064 (eebc0a9) into main (4c4d99b) will increase coverage by 0.25%.
The diff coverage is 79.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1064      +/-   ##
==========================================
+ Coverage   90.87%   91.13%   +0.25%     
==========================================
  Files          65       65              
  Lines       33229    34569    +1340     
==========================================
+ Hits        30196    31503    +1307     
- Misses       3033     3066      +33     
Impacted Files Coverage Δ
lightning/src/ln/chan_utils.rs 95.88% <78.08%> (-1.53%) ⬇️
lightning/src/ln/channel.rs 91.54% <80.76%> (+2.79%) ⬆️
lightning/src/chain/keysinterface.rs 94.86% <100.00%> (+0.17%) ⬆️
lightning/src/util/enforcing_trait_impls.rs 89.38% <100.00%> (+0.09%) ⬆️
lightning/src/ln/functional_tests.rs 97.47% <0.00%> (-0.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c4d99b...eebc0a9. Read the comment docs.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM.


if value_to_counterparty > 0 {
txouts.push((TxOut {
script_pubkey: counterparty_shutdown_script,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(here and in channel.rs) Calling this the "shutdown" script is a misnomer - the payout script is different from the shutdown one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must've been high, I'm not sure what this means. Either script or shutdown_script are fine...

@devrandom devrandom force-pushed the 2021-08-closing-tx-phase2 branch from f953f43 to 25d2e3e Compare September 3, 2021 11:57
Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just nits, LGTM

@devrandom devrandom force-pushed the 2021-08-closing-tx-phase2 branch from 25d2e3e to 6a870a0 Compare September 8, 2021 20:20
@devrandom devrandom force-pushed the 2021-08-closing-tx-phase2 branch from 6a870a0 to eebc0a9 Compare September 9, 2021 18:49
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only diff since Val's ack are addressing comments, will land after CI.

@TheBlueMatt TheBlueMatt merged commit 423f1b1 into lightningdevkit:main Sep 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants