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

Allow get_shutdown_scriptpubkey and get_destination_script to return an Error #2213

Merged
merged 1 commit into from
May 2, 2023
Merged

Allow get_shutdown_scriptpubkey and get_destination_script to return an Error #2213

merged 1 commit into from
May 2, 2023

Conversation

benthecarman
Copy link
Contributor

There are tons of cases where you can get an error when generating an address and it would be nice if LDK handled that. I did this in the simplest way of just making the error type (), not sure if it'd be better to add a special error type here and implementors can return as it seems like just using () was sufficient.

@benthecarman benthecarman force-pushed the error-sign-provider-addrs branch from 76b903b to b8476a5 Compare April 22, 2023 06:11
@codecov-commenter
Copy link

codecov-commenter commented Apr 22, 2023 •
edited
Loading

Codecov Report

Patch coverage: 74.28% and project coverage change: -0.02 ⚠️

Comparison is base (5f96d13) 91.56% compared to head (46a36d8) 91.55%.

❗ Current head 46a36d8 differs from pull request most recent head 0b8bdbf. Consider uploading reports for the commit 0b8bdbf to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2213      +/-   ##
==========================================
- Coverage   91.56%   91.55%   -0.02%     
==========================================
  Files         104      104              
  Lines       51548    51567      +19     
  Branches    51548    51567      +19     
==========================================
+ Hits        47199    47210      +11     
- Misses       4349     4357       +8
Impacted Files Coverage Δ
lightning/src/ln/channelmanager.rs 88.82% <ø> (ø)
lightning/src/util/test_utils.rs 76.95% <25.00%> (+0.07%) ⬆️
lightning/src/ln/channel.rs 89.92% <75.00%> (-0.11%) ⬇️
lightning/src/chain/keysinterface.rs 88.75% <100.00%> (ø)
lightning/src/ln/shutdown_tests.rs 98.05% <100.00%> (ø)

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@TheBlueMatt
Copy link
Collaborator

We should clearly document both in the channel open method on ChannelManager and the shutdown method on ChannelManager that if the signer returns an error the channel open will fail/channel will force-close instead. We should also note that in the signing methods.

@TheBlueMatt
Copy link
Collaborator

Somewat related, #2214.

@benthecarman benthecarman force-pushed the error-sign-provider-addrs branch from b8476a5 to 134b865 Compare April 22, 2023 19:27
@benthecarman
Copy link
Contributor Author

@TheBlueMatt added some docs

tnull
tnull reviewed Apr 24, 2023
View reviewed changes
@benthecarman benthecarman force-pushed the error-sign-provider-addrs branch from 134b865 to 46a36d8 Compare April 24, 2023 10:01
TheBlueMatt
TheBlueMatt reviewed Apr 24, 2023
View reviewed changes
@benthecarman benthecarman force-pushed the error-sign-provider-addrs branch from 46a36d8 to b0ddf31 Compare April 24, 2023 19:16
tnull
tnull previously approved these changes Apr 26, 2023
View reviewed changes
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

LGTM

alecchendev
alecchendev reviewed Apr 28, 2023
View reviewed changes
Copy link
Contributor

@alecchendev alecchendev left a comment

Choose a reason for hiding this comment

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

LGTM, just a question on docs

lightning/src/ln/channelmanager.rs Outdated
@@ -2098,6 +2101,11 @@ where
///
/// May generate a [`SendShutdown`] message event on success, which should be relayed.
///
/// Raises [`APIError::ChannelUnavailable`] if the channel cannot be closed due to failing to
/// generate a shutdown scriptpubkey or destination script set by
/// [`SignerProvider::get_shutdown_scriptpubkey`] and [`SignerProvider::get_destination_script`].
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC I don't think SignerProvider::destination_script is used when closing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, fixed some of the docs

@benthecarman benthecarman force-pushed the error-sign-provider-addrs branch from b0ddf31 to 39ace7e Compare April 28, 2023 05:15
TheBlueMatt
TheBlueMatt previously approved these changes Apr 28, 2023
View reviewed changes
@benthecarman benthecarman force-pushed the error-sign-provider-addrs branch from 39ace7e to 0b8bdbf Compare May 2, 2023 07:40
@TheBlueMatt TheBlueMatt merged commit 101c09f into lightningdevkit:main May 2, 2023
@benthecarman benthecarman deleted the error-sign-provider-addrs branch May 2, 2023 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnull tnull tnull approved these changes

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

@alecchendev alecchendev Awaiting requested review from alecchendev

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

Successfully merging this pull request may close these issues.
5 participants
@benthecarman @codecov-commenter @TheBlueMatt @tnull @alecchendev