Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Change WalletSource::sign_tx to sign_psbt #2775

Merged
merged 1 commit into from
Dec 12, 2023
Merged

Change WalletSource::sign_tx to sign_psbt #2775

merged 1 commit into from
Dec 12, 2023

Conversation

benthecarman
Copy link
Contributor

@benthecarman benthecarman commented Dec 7, 2023 •
edited

Closes #2713

Might want to add a test for taproot inputs to be extra sure, but the debug asserts I added should be sufficient

@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2023 •
edited

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (fcf47a4) 88.57% compared to head (7121290) 88.65%.
Report is 63 commits behind head on main.

❗ Current head 7121290 differs from pull request most recent head 489b408. Consider uploading reports for the commit 489b408 to get more accurate results

Files Patch % Lines
lightning/src/events/bump_transaction.rs 93.10% 0 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2775      +/-   ##
==========================================
+ Coverage   88.57%   88.65%   +0.07%     
==========================================
  Files         115      115              
  Lines       90687    91188     +501     
  Branches    90687    91188     +501     
==========================================
+ Hits        80327    80841     +514     
+ Misses       7962     7922      -40     
- Partials     2398     2425      +27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

wpaulino
wpaulino reviewed Dec 7, 2023
View reviewed changes
Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

Thanks! Would appreciate if you could finish testing the anchors integration on your end and report back before landing this to make sure we're not missing anything else.

lightning/src/events/bump_transaction.rs Show resolved Hide resolved
lightning/src/events/bump_transaction.rs Show resolved Hide resolved
@TheBlueMatt TheBlueMatt added this to the 0.0.119 milestone Dec 8, 2023
TheBlueMatt
TheBlueMatt reviewed Dec 8, 2023
View reviewed changes
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.

I'm kinda meh on forcing users to use PSBTs here vs just having the option. I guess I don't really have a good feel for if "everything" has moved over to PSBTs yet, vs some stuff out there still using raw transactions during signing, but if not ISTM we should make this an option?

lightning/src/util/test_utils.rs Outdated Show resolved Hide resolved
@benthecarman
Copy link
Contributor Author

I'm kinda meh on forcing users to use PSBTs here vs just having the option. I guess I don't really have a good feel for if "everything" has moved over to PSBTs yet, vs some stuff out there still using raw transactions during signing, but if not ISTM we should make this an option?

It feels like everything has moved over to PSBT, but maybe I have a biased view, idk if any of the multi coin libraries support it

@benthecarman benthecarman force-pushed the sign-psbt branch 2 times, most recently from bd09cd4 to 7121290 Compare December 11, 2023 17:27
@wpaulino
Copy link
Contributor

I'm kinda meh on forcing users to use PSBTs here vs just having the option. I guess I don't really have a good feel for if "everything" has moved over to PSBTs yet, vs some stuff out there still using raw transactions during signing, but if not ISTM we should make this an option?

They can still get the unsigned transaction out of it and sign it manually?

@TheBlueMatt
Copy link
Collaborator

Yea, I didn't realize initially that you could just extract_tx even on an unsigned tx.

@benthecarman
Copy link
Contributor Author

I'll add that to the docs

TheBlueMatt
TheBlueMatt reviewed Dec 11, 2023
View reviewed changes
lightning/src/events/bump_transaction.rs Outdated Show resolved Hide resolved
lightning/src/events/bump_transaction.rs Outdated Show resolved Hide resolved
lightning/src/events/bump_transaction.rs Outdated Show resolved Hide resolved
@benthecarman
Copy link
Contributor Author

Is there a compile error on master? The CI failure looks unrelated to this PR

@TheBlueMatt
Copy link
Collaborator

I think rust-bitcoin broke again :(

@TheBlueMatt
Copy link
Collaborator

Specifically, rust-bitcoin/rust-bitcoinconsensus#74 but we should just land this without waiting on that.

@TheBlueMatt TheBlueMatt merged commit 68e25c6 into lightningdevkit:main Dec 12, 2023
2 of 15 checks passed
@benthecarman benthecarman deleted the sign-psbt branch December 12, 2023 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

@wpaulino wpaulino wpaulino approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.0.119
Development

Successfully merging this pull request may close these issues.

Change WalletSource::sign_tx to sign_psbt
4 participants
@benthecarman @codecov-commenter @wpaulino @TheBlueMatt