feat(lncli): Add --route_hints flag to sendpayment --keysend and queryroutes #9721
Conversation
|
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 (
|
appilon
force-pushed
the
appilon/6601
branch
2 times, most recently
from
842cca7 to
5207897
Compare
|
@saubyk I fixed the linting errors I believe (strange though |
appilon
force-pushed
the
appilon/6601
branch
4 times, most recently
from
8cf6aca to
978d4cf
Compare
saubyk
added
the
good first review
appilon
changed the title
appilon
force-pushed
the
appilon/6601
branch
2 times, most recently
from
31fe6a6 to
4b9fac7
Compare
|
@saubyk PR should be ready for review and re-running of CI. |
|
Sorry about lint error 🤦 forgot to re-rerun locally. fixed now |
There was a problem hiding this comment.
Good Job 👍
I’ve successfully run the integration tests and also performed some manual tests using a regtest environment — including sendpayment with keysend and queryroutes.
The only thing missing now are the manual AMP tests, which I’ll be running in the next few days. Everything seems to be working properly so far, but I noticed a logic issue related to when a pay_req is provided — I believe it should be handled a bit differently.
appilon
force-pushed
the
appilon/6601
branch
2 times, most recently
from
c216bdc to
d54268d
Compare
|
@MPins @Lokeshranjan8 All review comments have been addressed, thank you for your time reviewing. |
Hello @appilon, Looking deeper into the --amp case, I believe it should be treated like a regular invoice, where route hints are included at creation time, as defined in BOLT #11. Therefore, we should keep this change limited to It would be great to get @saubyk 's input on this as well. |
|
@MPins I'm a lot less familiar with the specs and protocols so if AMP payments already handle route hinting similar to a bolt 11 pay req then yes I agree I can limit the flag to just support |
There was a problem hiding this comment.
Hello, I just ran a test with AMP invoices, and they do accept route hints when creating the invoice.
Therefore, I think you should move forward and limit this change to sendpayment --keysend and queryroutes only.
Also, I recommend reading the ideal git commit structure guide to improve the commit style.
…tningnetwork#6601) Adds --route_hints flag to sendpayment for --keysend payments. Hints should be JSON encoded (see usage for example). Adds --route_hints flag to queryroutes (no restrictions). Adds integration tests for query routes over RPC, and manual keysend over RPC to emulate the new feature. Testing revealed route hinting did not work for standard payment (w/ or w/o --pay_addr).
appilon
changed the title
|
@MPins Addressed, as for the commit, is style in general (like tense or how I broke up the sentences/paragraphs) an issue or the fact that it's one commit? I know the header is > 50 chars, I kept it under 72, If 50 chars is some hard mandate I'll try my best to compress it. |
| Usage: "route hints for reaching the destination of a" + | ||
| " manual --keysend payment. eg: " + | ||
| `[{"hop_hints":[{"node_id":"A","chan_id":1,` + | ||
| `"fee_base_msat":2,` + | ||
| `"fee_proportional_millionths":3,` + | ||
| `"cltv_expiry_delta":4}]}]`, |
There was a problem hiding this comment.
Consider avoiding starting the line with a space, mixing " and ` in the same string concatenation, and preserving the single quotes (') around the example shown to the user.
Change Description
This PR implements the
--route_hintsflag for thelncli sendpayment --keysendandlncli queryroutescommands, addressing issue #6601. Testing revealed route hinting did not work for standard payments (w/ or w/o--pay_addr). CLI should error if--keysendis not present.I had a lot of difficulty in testing initially because I was trying have an integration test perform hinting with a standard payment. I'm too much of a newbie to know why, but it seems there just isn't enough context on the backend for standard manual payments (Gemini thought maybe something to do with feature flags and onion routing, all new terms for me, much to learn still). I did manage to get it to work with keysend and amp. The added integration tests are aimed at "emulating" the logic seen in
cmd_payments.goin constructing a SendPayment request despite them not covering the pretty trivial argparsing that was actually added in this PR (I just wanted to be sure it worked, which proved valuable given the failed scenarios).Closes #6601
Steps to Test
Verify
lncliargument parsing:--keysendflags present expect error. (use dummy values)$ lncli --network=simnet --no-macaroons sendpayment --dest=03aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa --amt=1000 --payment_hash=0000000000000000000000000000000000000000000000000000000000000000 --final_cltv_delta=40 --route_hints='[]'--keysend, expect a connection error, indicating parsing succeeded. (use dummy values)$ lncli --network=simnet --no-macaroons sendpayment --keysend --dest=03aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa --amt=1000 --final_cltv_delta=40 --route_hints='[{"hop_hints":[{"node_id":"02bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb","chan_id":12345,"fee_base_msat":100,"fee_proportional_millionths":10,"cltv_expiry_delta":18}]}]'--route_hints(e.g., remove a quote). Expect a JSON parsing error.lncli queryroutes --route_hints='...'.Run the new integration tests:
make itest ITEST_FLAGS='-test.run=TestLightningNetworkDaemon/tranche00/80-of-296/btcd/query_routes_routehints'make itest ITEST_FLAGS='-test.run=TestLightningNetworkDaemon/tranche00/102-of-296/btcd/send_payment_routehints_keysend'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.