-
Notifications
You must be signed in to change notification settings - Fork 2.2k
commands: fix how we parse chan ids args at CLI level #10100
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
Conversation
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.
Summary of Changes
Hello @Abdulkbk, 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 addresses a limitation in how channel IDs were handled as command-line arguments. Previously, int64
was used, which could not accommodate all possible custom or alias short channel ID (SCID) ranges. I've resolved this by switching the CLI flags to accept string inputs for channel IDs and then robustly parsing these strings into uint64
values, ensuring full compatibility with the expected SCID range.
Highlights
- CLI Argument Type Change: I've updated the CLI flag types for channel IDs (
outgoing_chan_id
inpayinvoice
/sendpayment
andincoming_chan_ids
/outgoing_chan_ids
infwdinghistory
) fromcli.Int64SliceFlag
tocli.StringSliceFlag
. This allows for a broader range of channel ID values, including those that exceed theint64
maximum. - Robust Channel ID Parsing: The parsing logic for these channel ID flags has been enhanced. Instead of direct type assertion, I'm now explicitly using
strconv.ParseUint(c, 10, 64)
to convert the string input touint64
. This includes error handling to catch invalid channel ID formats provided by the user.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
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 .gemini/
folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This PR correctly fixes an issue where uint64
channel IDs could not be passed via the CLI by switching from Int64SliceFlag
to StringSliceFlag
. The implementation is correct. My review includes suggestions to refactor duplicated code into a helper function for better maintainability, and to adjust the formatting of error messages to be more compact and align with the project's style guide.
cmd/commands/cmd_payments.go
Outdated
return fmt.Errorf("unable to decode "+ | ||
"outgoing_chan_id: %w", | ||
err) |
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.
The formatting of this error message can be more compact to better align with the style guide's preference for minimizing lines for error messages.1
return fmt.Errorf("unable to decode "+
"outgoing_chan_id: %w", err)
Style Guide References
Footnotes
cmd/commands/cmd_payments.go
Outdated
outgoingChannelIDs := ctx.StringSlice("outgoing_chan_ids") | ||
if len(outgoingChannelIDs) != 0 { | ||
req.OutgoingChanIds = make([]uint64, len(outgoingChannelIDs)) | ||
for i, c := range outgoingChannelIDs { | ||
req.OutgoingChanIds[i] = uint64(c) | ||
scid, err := strconv.ParseUint(c, 10, 64) | ||
if err != nil { | ||
return fmt.Errorf("unable to decode "+ | ||
"outgoing_chan_ids: %w", | ||
err) | ||
} | ||
req.OutgoingChanIds[i] = scid | ||
} | ||
} | ||
|
||
incomingChannelIDs := ctx.Int64Slice("incoming_chan_ids") | ||
incomingChannelIDs := ctx.StringSlice("incoming_chan_ids") | ||
if len(incomingChannelIDs) != 0 { | ||
req.IncomingChanIds = make([]uint64, len(incomingChannelIDs)) | ||
for i, c := range incomingChannelIDs { | ||
req.IncomingChanIds[i] = uint64(c) | ||
scid, err := strconv.ParseUint(c, 10, 64) | ||
if err != nil { | ||
return fmt.Errorf("unable to decode "+ | ||
"incoming_chan_ids: %w", | ||
err) | ||
} | ||
req.IncomingChanIds[i] = scid | ||
} | ||
} |
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.
The logic for parsing a string slice of channel IDs into a []uint64
is duplicated here for both outgoing_chan_ids
and incoming_chan_ids
, and is also present in SendPaymentRequest
(lines 524-536).
To improve maintainability and reduce code duplication, consider extracting this logic into a helper function. This function could take the string slice and the flag name (for use in error messages) as arguments.
For example, you could add a helper function like this to cmd/commands/commands.go
(or locally in this file if preferred):
func parseChanIDs(idStrings []string, flagName string) ([]uint64, error) {
chanIDs := make([]uint64, len(idStrings))
for i, idStr := range idStrings {
scid, err := strconv.ParseUint(idStr, 10, 64)
if err != nil {
return nil, fmt.Errorf("unable to decode %s: %w", flagName, err)
}
chanIDs[i] = scid
}
return chanIDs, nil
}
Then you could simplify this block and the one in SendPaymentRequest
significantly.
cmd/commands/cmd_payments.go
Outdated
return fmt.Errorf("unable to decode "+ | ||
"outgoing_chan_ids: %w", | ||
err) |
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.
The formatting of this error message can be more compact to better align with the style guide's preference for minimizing lines for error messages.1
return fmt.Errorf("unable to decode "+
"outgoing_chan_ids: %w", err)
Style Guide References
Footnotes
cmd/commands/cmd_payments.go
Outdated
return fmt.Errorf("unable to decode "+ | ||
"incoming_chan_ids: %w", | ||
err) |
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.
The formatting of this error message can be more compact to better align with the style guide's preference for minimizing lines for error messages.1
return fmt.Errorf("unable to decode "+
"incoming_chan_ids: %w", err)
Style Guide References
Footnotes
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.
Summary of Changes
Hello @Abdulkbk, 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 addresses a limitation in how lncli
commands handle channel IDs by improving the robustness of CLI argument parsing. The change allows for the correct processing of uint64
channel IDs, which can exceed the int64
range, thereby supporting custom or alias SCIDs that might have larger values. This enhancement specifically impacts the payinvoice
, sendpayment
, and fwdinghistory
commands, making them more resilient to a wider range of valid channel ID inputs.
Highlights
- CLI Argument Type Change: I've updated the command-line interface (CLI) argument types for channel IDs in
payinvoice
,sendpayment
, andfwdinghistory
commands. Specifically,outgoing_chan_id
,incoming_chan_ids
, andoutgoing_chan_ids
flags have been changed fromcli.Int64SliceFlag
tocli.StringSliceFlag
. - Robust Channel ID Parsing: To support the full range of short channel IDs (SCIDs), including those that exceed the
int64
maximum value, I've implemented explicit parsing usingstrconv.ParseUint
for these string-based channel IDs. This ensures thatuint64
values are correctly handled and includes error handling for invalid inputs.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
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 .gemini/
folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request updates how the application handles CLI arguments for channel IDs by switching from Int64Slice
to StringSlice
to accommodate larger channel ID ranges. The changes include necessary error handling and address the identified issue.
cmd/commands/cmd_payments.go
Outdated
outChan := ctx.StringSlice("outgoing_chan_id") | ||
if len(outChan) != 0 { | ||
req.OutgoingChanIds = make([]uint64, len(outChan)) | ||
for i, c := range outChan { | ||
req.OutgoingChanIds[i] = uint64(c) | ||
scid, err := strconv.ParseUint(c, 10, 64) | ||
if err != nil { | ||
return fmt.Errorf("unable to decode "+ | ||
"outgoing_chan_id: %w", | ||
err) | ||
} | ||
req.OutgoingChanIds[i] = scid | ||
} |
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.
The logic for parsing the outgoing_chan_id
is duplicated in forwardingHistory
. Consider refactoring this logic into a helper function to improve maintainability and reduce redundancy.
scid, err := strconv.ParseUint(c, 10, 64)
if err != nil {
return fmt.Errorf("unable to decode "+
"outgoing_chan_id: %w",
err)
}
req.OutgoingChanIds[i] = scid
cmd/commands/cmd_payments.go
Outdated
for i, c := range outgoingChannelIDs { | ||
req.OutgoingChanIds[i] = uint64(c) | ||
scid, err := strconv.ParseUint(c, 10, 64) | ||
if err != nil { | ||
return fmt.Errorf("unable to decode "+ | ||
"outgoing_chan_ids: %w", | ||
err) | ||
} | ||
req.OutgoingChanIds[i] = scid | ||
} | ||
} | ||
|
||
incomingChannelIDs := ctx.Int64Slice("incoming_chan_ids") | ||
incomingChannelIDs := ctx.StringSlice("incoming_chan_ids") | ||
if len(incomingChannelIDs) != 0 { | ||
req.IncomingChanIds = make([]uint64, len(incomingChannelIDs)) | ||
for i, c := range incomingChannelIDs { | ||
req.IncomingChanIds[i] = uint64(c) | ||
scid, err := strconv.ParseUint(c, 10, 64) | ||
if err != nil { | ||
return fmt.Errorf("unable to decode "+ | ||
"incoming_chan_ids: %w", | ||
err) | ||
} | ||
req.IncomingChanIds[i] = scid | ||
} |
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.
The logic for parsing outgoing_chan_ids
and incoming_chan_ids
is duplicated. A very similar block of code also exists in the SendPaymentRequest
function for outgoing_chan_id
. To improve maintainability and reduce redundancy, consider refactoring this logic into a helper function.
scid, err := strconv.ParseUint(c, 10, 64)
if err != nil {
return fmt.Errorf("unable to decode "+
"outgoing_chan_ids: %w",
err)
}
req.OutgoingChanIds[i] = scid
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 the update!
Looks good to me, but I think we should extract the logic into a generalized function as Gemini suggests.
50f34f1
to
0668f5f
Compare
/gemini review |
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.
Code Review
This PR correctly addresses the issue of parsing uint64
channel IDs from the CLI by switching from Int64Slice
to StringSlice
. The introduction of the parseChanIDs
helper function is a good refactoring that improves code clarity and reduces duplication.
9f1e3e2
to
3ae3229
Compare
In this commit we use stringSlice flag for incoming and outgoing channel ids args, for forwarding history command. The Int64Slice range does not cover all possible scids in the custom alias range.
In this commit we replace the Int64Slice flag with StringSlice for chan id flag, in sendpayment request.
In this commit we use the `parseChanIDs` method introduced in a previous commit to process the `outgoing_chan_id` flag for `queryroutes`.
3ae3229
to
603fbcd
Compare
In this commit we add test for `parseChanIDs` to make sure it parses the chan ids correctly and return errors when invalid value is passed. Signed-off-by: Abdullahi Yunus <abdoollahikbk@gmail.com>
603fbcd
to
754c254
Compare
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.
Very nice! utACK, LGTM 🎉
@Abdulkbk, remember to re-request review from reviewers when ready |
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.
very nice 🙏
Follow-up on #10057 (comment)
This PR updates how we handle CLI args for channel ID in
fwdinghistory
,sendpayment
, andpayinvoice
. We were initially using theInt64Slice
flag, which doesn't cover all custom/aliasscid
range. Now, we're using theStringSlice
flag and parsing the chanID touint64
later.Steps to Test
lncli payinvoice --outgoing_chan_id=17592186044552773633 # out of int64 range