-
Notifications
You must be signed in to change notification settings - Fork 2.2k
multi: Remove endpoints sendpayment
, sendtoroute
, sendtoroutesync
, and sendpaymentsync
#8348
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
base: master
Are you sure you want to change the base?
Conversation
e6bc25f
to
7314e6c
Compare
…oroutesync`, and `sendpaymentsync`. This pull request removes the deprecated endpoints `sendpayment`, `sendtoroute`, `sendtoroutesync`, and `sendpaymentsync` in `lightning.proto`, `rpcserver.go`, `lntest/rpc/lnd.go`, and its related test cases in `itest/lnd_routing_test.go` and `itest/lnd_channel_policy_test.go`. docs: Update release notes for 0.18.0 Added release note in version 0.18.0 to notify about the upcoming removal of deprecated endpoints (`sendpayment`, `sendtoroute`, `sendtoroutesync`, and `sendpaymentsync`). Referenced PR (lightningnetwork#8348) for more details.
Hey @yyforyongyu, to keep you updated all the test cases are passed in the Travis CI run. That said we need to double check on those assertions: |
Hey, @yyforyongyu @guggero is there any way we can get this in? :) |
@mohamedawnallah Thanks for the PR! As mentioned in the issue, we won't add this in the upcoming release so this will be marked as low priority atm. |
7314e6c
to
922e59d
Compare
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
922e59d
to
a1d795e
Compare
sendpayment
, sendtoroute
, sendtoroutesync
, and sendpaymentsync
sendpayment
, sendtoroute
, sendtoroutesync
, and sendpaymentsync
sendpayment
, sendtoroute
, sendtoroutesync
, and sendpaymentsync
sendpayment
, sendtoroute
, sendtoroutesync
, and sendpaymentsync
a1d795e
to
c4dfad6
Compare
Hi @yyforyongyu, It seems that the LND v0.19 release is now on track, as I see in the LND projects dashboard. I've rebased onto the master branch and resolved the conflicts. I would love to receive any feedback on that PR! 🙏 |
I am also in favour of notifying the user in 19 and removing it in 20 (maybe in a msg. in the release notes). That being said I think we can move this PR in the LND 20 scope as a consequence. |
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 am also in favour of notifying the user in 19 and removing it in 20 (maybe in a msg. in the release notes). That being said I think we can move this PR in the LND 20 scope as a consequence.
I agree that we need to publicize this removal a bit more since the endpoints are quite important. Thank you for this work, @mohamedawnallah, could you open a separate PR to add an entry to the release notes of v0.19, to notify users about this planned change (we could use camel case for the notation for the endpoints, as they appear in https://lightning.engineering/api-docs/category/lightning-service/, I think that would make the warning more recognizable for developers)?
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.
Thanks for addressing this issue.
I think there is still more to deprecate:
We have a file called: router_server_deprecated.go
which maps SendToRoute, SendPayment grpc calls to the newer versions, we should remove them as well in LND 20.
Moreover check all files whether there are some leftover descriptions of the old endpoints, for example in python.md we still reference the SendPayment stub which will not exist after this PR is merged:
for payment in stub.SendPayment(request_iterable):
print(payment)
Just an idea that we could also try to figure out how many projects still use those endpoints, a starting point could be by looking through the dependents https://github.com/lightningnetwork/lnd/network/dependents?package_id=UGFja2FnZS0yMjY0ODI4ODY5, or searching for the endpoints, for example https://github.com/search?q=%22SendToRouteSync%22+language%3Ago&type=code. |
sendpayment
, sendtoroute
, sendtoroutesync
, and sendpaymentsync
sendpayment
, sendtoroute
, sendtoroutesync
, and sendpaymentsync
So this PR now removes the listed endpoints and has 0.20 scope. |
Thanks, @ziggie1984, for the code review. It seems this PR will be merged in release 0.21, where we remove the deprecated endpoints after the warning in #9456. EDIT: |
@mohamedawnallah, remember to re-request review from reviewers when ready |
!lightninglabs-deploy mute |
In this commit, we remove deprecated endpoints `sendpayment`, `sendtoroute`, `sendtoroutesync`, and `sendpaymentsync`.
In this commit, we update the `SendPayment` gRPC server-streaming example to use routerrpc `SendPaymentV2` gRPC in `docs/grpc/python.md`.
In this commit, we update the `SendPayment` gRPC server-streaming example to use routerrpc `SendPaymentV2` in `docs/grpc/c#.md`.
c4dfad6
to
87156eb
Compare
Change Description
This pull request removes the deprecated endpoints
sendpayment
,sendtoroute
,sendtoroutesync
, andsendpaymentsync
inlightning.proto
,rpcserver.go
,lntest/rpc/lnd.go
, and its related test cases initest/lnd_routing_test.go
anditest/lnd_channel_policy_test.go
.Closes #8175
Notes
The actual removal for those endpoints in
0.21
release was based on the discussion in this PR #9456.Steps to Test
I ran the test cases using
make check
to make sure there were no regressions and all the test cases passed.Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]
in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.