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

Remove deprecated send_payment_with_route API and friends #3430

Merged
merged 4 commits into from
Dec 12, 2024
Merged

Remove deprecated send_payment_with_route API and friends #3430

merged 4 commits into from
Dec 12, 2024

Conversation

valentinewallace
Copy link
Contributor

@valentinewallace valentinewallace commented Nov 27, 2024 •
edited
Loading

Mark the deprecated ChannelManager::send_payment_with_route API as test-only, partially addressing #2390.

This method has been deprecated for several versions in favor of
ChannelManager::send_payment, and we want to remove it from the public API
entirely for the 0.1 release.

Also went ahead and removed the old API for send_spontaneous_payment that allowed specifying a route, though it wasn't officially deprecated.

@valentinewallace valentinewallace added this to the 0.1 milestone Nov 27, 2024
@valentinewallace valentinewallace force-pushed the 2024-11-remove-old-send-api branch 2 times, most recently from 151ec49 to 3956661 Compare December 2, 2024 22:14
@codecov Codecov
Copy link

codecov bot commented Dec 2, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 78.84615% with 22 lines in your changes missing coverage. Please review.

Project coverage is 89.69%. Comparing base (94411bc) to head (bcaba29).
Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/outbound_payment.rs 12.50% 19 Missing and 2 partials ⚠️
lightning/src/ln/channelmanager.rs 97.50% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3430      +/-   ##
==========================================
+ Coverage   89.65%   89.69%   +0.04%     
==========================================
  Files         130      130              
  Lines      107422   107401      -21     
  Branches   107422   107401      -21     
==========================================
+ Hits        96306    96336      +30     
+ Misses       8718     8664      -54     
- Partials     2398     2401       +3

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

@TheBlueMatt
Copy link
Collaborator

Nice, basically LGTM, why is this draft?

@valentinewallace
Copy link
Contributor Author

Ah, I wanted to make sure the fuzz changes passed CI. Undrafting

@valentinewallace valentinewallace marked this pull request as ready for review December 3, 2024 21:24
@valentinewallace
Copy link
Contributor Author

I can also rename RetryableSendFailure (make it the new PaymentSendFailure?) if we want that.

@TheBlueMatt
Copy link
Collaborator

I can also rename RetryableSendFailure (make it the new PaymentSendFailure?) if we want that.

I like the clear implication in the name - " if you see one of these you can retry" vs just "this failed", I think.

@arik-so
Copy link
Contributor

arik-so commented Dec 4, 2024

Looks good to me, too, but there is some weirdness in beta Rust versions in CI. I think it's unrelated though.

@TheBlueMatt TheBlueMatt linked an issue Dec 5, 2024 that may be closed by this pull request
Remove old sending API #2390
Closed
@arik-so
Copy link
Contributor

arik-so commented Dec 10, 2024

Would you mind rebasing this? I think that should fix all CI issues.

@valentinewallace
Mark deprecated send_payment_with_route as test and fuzz only
90b1e7d 
This method has been deprecated for several versions in favor of
ChannelManager::send_payment, and we want to remove it from the public API
entirely prior to the 0.1 release. However, >150 tests use it so put off
removing the method entirely.
@valentinewallace
Remove support for specifying route with send_spontaneous_payment
351efc8 
The old API is confusing and we want to remove it for 0.1.
@valentinewallace
Stop using PaymentSendFailure within ProbeSendFailure
66bdc62 
Removes the final usage of PaymentSendFailure from public API.

This (confusing) error matched with prior versions of LDK where users had to
handle payment retries themselves. Since auto-retry was introduced, the only
non-deprecated use remaining was for probe send errors. Probes only have
one path, though, so refactor ProbeSendFailure to omit usage of
PaymentSendFailure.

We don't make this error private yet because it's still used by some fuzzing
code as well as internally to outbound_payments, but it isn't returned by any
public functions anymore.
@valentinewallace
Remove deprecated send_payment_with_route usage from fuzzing
bcaba29 
This allows us to make the PaymentSendFailure error type private, as well as
reduce the visibility of the vestigial send_payment_with_route method that was
already made test and fuzz-only in a previous commit.
@valentinewallace valentinewallace force-pushed the 2024-11-remove-old-send-api branch from 3956661 to bcaba29 Compare December 10, 2024 20:24
TheBlueMatt
TheBlueMatt approved these changes Dec 12, 2024
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.

Thanks!

@arik-so arik-so merged commit 641e40f into lightningdevkit:main Dec 12, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arik-so arik-so arik-so approved these changes

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

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

Successfully merging this pull request may close these issues.

Remove old sending API
3 participants
@valentinewallace @TheBlueMatt @arik-so