lncli: add source and last_hop params to queryroutes #4
Conversation
Add two new optional parameters to the lncli queryroutes command: --source: Allows specifying the source node for the route query. --last_hop: Allows specifying the penultimate node in the payment path.
Summary of ChangesHello @feelancer21, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
Adds two new optional command-line parameters to the lncli queryroutes command to provide more granular control over route finding by allowing specification of the source node and penultimate hop in the payment path.
- Added
--sourceparameter to specify the source node for route queries - Added
--last_hopparameter to specify the penultimate node in the payment path - Updated release notes to document the new functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/commands/cmd_payments.go | Implements the new CLI flags and request field mapping for source and last_hop parameters |
| docs/release-notes/release-notes-0.20.0.md | Documents the new queryroutes parameters and adds contributor credit |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| lastHop, err := hex.DecodeString(ctx.String("last_hop")) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid last_hop argument: %w", err) | ||
| } |
There was a problem hiding this comment.
The hex decoding validation for last_hop should include a length check to ensure it's exactly 33 bytes, similar to how other public key validations are typically handled in the codebase.
| } | |
| } | |
| if len(lastHop) != 33 { | |
| return fmt.Errorf("last_hop must be 33 bytes (compressed pubkey), got %d bytes", len(lastHop)) | |
| } |
| req := &lnrpc.QueryRoutesRequest{ | ||
| PubKey: dest, | ||
| SourcePubKey: ctx.String("source"), |
There was a problem hiding this comment.
The source parameter should have hex decoding validation similar to last_hop, rather than passing the raw string directly to the request. This ensures consistent input validation for public key parameters.
| req := &lnrpc.QueryRoutesRequest{ | |
| PubKey: dest, | |
| SourcePubKey: ctx.String("source"), | |
| // Validate the source parameter if provided. | |
| source := ctx.String("source") | |
| if source != "" { | |
| _, err := hex.DecodeString(source) | |
| if err != nil { | |
| return fmt.Errorf("invalid source pubkey: %v", err) | |
| } | |
| } | |
| req := &lnrpc.QueryRoutesRequest{ | |
| PubKey: dest, | |
| SourcePubKey: source, |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds --source and --last_hop parameters to the lncli queryroutes command, enhancing its flexibility. The changes are well-implemented, but I have a couple of suggestions to improve code consistency and robustness. Specifically, I've pointed out a duplicated flag definition and a missing validation check for a public key.
| lastHop, err := hex.DecodeString(ctx.String("last_hop")) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid last_hop argument: %w", err) | ||
| } | ||
| req.LastHopPubkey = lastHop |
There was a problem hiding this comment.
The public key for last_hop is decoded from hex, but its length is not validated. This could lead to malformed data being sent to the backend if a key with an incorrect length is provided. For consistency with how other public keys are handled in this file (e.g., for sendpayment), and to ensure correctness, I suggest using route.NewVertexFromStr. This function handles both hex decoding and length validation.
| lastHop, err := hex.DecodeString(ctx.String("last_hop")) | |
| if err != nil { | |
| return fmt.Errorf("invalid last_hop argument: %w", err) | |
| } | |
| req.LastHopPubkey = lastHop | |
| lastHop, err := route.NewVertexFromStr(ctx.String("last_hop")) | |
| if err != nil { | |
| return fmt.Errorf("invalid last_hop argument: %w", err) | |
| } | |
| req.LastHopPubkey = lastHop[:] |
| cli.StringFlag{ | ||
| Name: "last_hop", | ||
| Usage: "(optional) the 33-byte hex-encoded public key " + | ||
| "for the last hop (penultimate node in the path) " + | ||
| "to route through for this payment", | ||
| }, |
There was a problem hiding this comment.
This new last_hop flag duplicates the functionality of the existing global lastHopFlag defined on line 46. To improve maintainability and avoid code duplication, consider reusing the existing flag here. The Usage string in this new flag is more descriptive, so it might be a good idea to update the global lastHopFlag with this improved text and use it in both places.
Pull Request Test Coverage Report for Build 18129288550Details
💛 - Coveralls |
Add two new optional parameters to the lncli queryroutes command:
--source: Allows specifying the source node for the route query.
--last_hop: Allows specifying the penultimate node in the payment path.
Change Description
Description of change / link to associated issue.
Steps to Test
Steps for reviewers to follow to test the change.
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.