multi: make updatechanpolicy fields optional with *_specified flags#10500
multi: make updatechanpolicy fields optional with *_specified flags#10500yashbhutwala wants to merge 1 commit intolightningnetwork:masterfrom
Conversation
Summary of ChangesHello @yashbhutwala, 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 significantly improves the usability of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
yashbhutwala
force-pushed
the
feat/updatechanpolicy-optional-fields
branch
from
074ca36 to
d94d2e7
Compare
![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 correctly implements making updatechanpolicy fields optional, which is a great usability improvement. The changes are consistent across the stack, from the protobuf definitions to the CLI. The use of *_specified flags follows existing patterns in the codebase. The implementation correctly handles pointer-based optional fields in the internal APIs and respects the new flags in the RPC server logic. I've added a couple of comments regarding missing function documentation to align with the repository's style guide. Overall, this is a solid contribution.
This commit makes the base_fee_msat, fee_rate/fee_rate_ppm, and time_lock_delta fields optional in the UpdateChannelPolicy RPC, following the existing pattern used by min_htlc_msat_specified. Previously, these fields were required on every invocation, even when users only wanted to update a single field. This was a common pain point reported in issue lightningnetwork#1523. Protocol changes (lnrpc/lightning.proto): - Add base_fee_msat_specified (field 12) - Add fee_rate_specified (field 13) - Add time_lock_delta_specified (field 14) Internal API changes (routing/router.go): - FeeSchema.BaseFee: changed from value to pointer (*lnwire.MilliSatoshi) - FeeSchema.FeeRate: changed from value to pointer (*uint32) - ChannelPolicy.TimeLockDelta: changed from value to pointer (*uint32) RPC server changes (rpcserver.go): - Only validate and apply fields when their *_specified flag is true - When a field is not specified, the existing channel value is retained CLI changes (cmd/commands/commands.go): - Fields are now truly optional (no "argument missing" errors) - ctx.IsSet() determines whether to set *_specified flags - Default values shown in help text from chainreg constants - Help text updated to say "If unset, existing value is retained" Local channel manager changes (routing/localchans/manager.go): - updateEdge() only updates fields when pointers are non-nil - Follows the same pattern already used for MinHTLC This enables users to update individual policy fields independently: lncli updatechanpolicy --base_fee_msat 1000 --chan_point abc:0 Without needing to specify all other fields. NOTE: Proto regeneration required (make rpc). Fixes lightningnetwork#1523
yashbhutwala
force-pushed
the
feat/updatechanpolicy-optional-fields
branch
from
d94d2e7 to
79e7cbb
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable enhancement by making several fields in the UpdateChannelPolicy RPC optional through the use of *_specified flags. The changes are well-implemented across the CLI, RPC server, and internal logic. My review focuses on improving code maintainability by reducing duplication in argument parsing and fixing minor issues in test error messages. Overall, this is a solid improvement to the API's usability.
| // Parse base_fee_msat. If explicitly set, mark as specified. | ||
| switch { | ||
| case ctx.IsSet("base_fee_msat"): | ||
| baseFee = ctx.Int64("base_fee_msat") | ||
| baseFeeMsatSpecified = true | ||
| case args.Present(): | ||
| baseFee, err = strconv.ParseInt(args.First(), 10, 64) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to decode base_fee_msat: %w", | ||
| err) | ||
| } | ||
| baseFeeMsatSpecified = true | ||
| args = args.Tail() | ||
| default: | ||
| return fmt.Errorf("base_fee_msat argument missing") | ||
| } | ||
|
|
||
| // Parse fee_rate or fee_rate_ppm. If explicitly set, mark as specified. | ||
| switch { | ||
| case ctx.IsSet("fee_rate") && ctx.IsSet("fee_rate_ppm"): | ||
| return fmt.Errorf("fee_rate or fee_rate_ppm can not both be set") | ||
| case ctx.IsSet("fee_rate"): | ||
| feeRate = ctx.Float64("fee_rate") | ||
| feeRateSpecified = true | ||
| case ctx.IsSet("fee_rate_ppm"): | ||
| feeRatePpm = ctx.Uint64("fee_rate_ppm") | ||
| feeRateSpecified = true | ||
| case args.Present(): | ||
| feeRate, err = strconv.ParseFloat(args.First(), 64) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to decode fee_rate: %w", err) | ||
| } | ||
|
|
||
| feeRateSpecified = true | ||
| args = args.Tail() | ||
| default: | ||
| return fmt.Errorf("fee_rate or fee_rate_ppm argument missing") | ||
| } | ||
|
|
||
| // Parse time_lock_delta. If explicitly set, mark as specified. | ||
| switch { | ||
| case ctx.IsSet("time_lock_delta"): | ||
| timeLockDeltaStr := ctx.String("time_lock_delta") | ||
| timeLockDelta, err = parseTimeLockDelta(timeLockDeltaStr) | ||
| tld, err := parseTimeLockDelta(timeLockDeltaStr) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| timeLockDelta = uint32(tld) | ||
| timeLockDeltaSpecified = true | ||
| case args.Present(): | ||
| timeLockDelta, err = parseTimeLockDelta(args.First()) | ||
| tld, err := parseTimeLockDelta(args.First()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| timeLockDelta = uint32(tld) | ||
| timeLockDeltaSpecified = true | ||
| args = args.Tail() | ||
| default: | ||
| return fmt.Errorf("time_lock_delta argument missing") | ||
| } |
There was a problem hiding this comment.
The argument parsing logic for base_fee_msat, fee_rate*, and time_lock_delta is a bit repetitive across the three switch blocks. This also violates the style guide which requires comments for each case and spacing between them.
Consider refactoring this into a series of if/else if blocks to reduce code duplication and improve readability and style guide adherence.
For example, the time_lock_delta parsing could be simplified to something like this:
// Parse time_lock_delta. If explicitly set, mark as specified.
var timeLockDeltaStr string
var timeLockDeltaSet bool
if ctx.IsSet("time_lock_delta") {
timeLockDeltaStr = ctx.String("time_lock_delta")
timeLockDeltaSet = true
} else if args.Present() {
timeLockDeltaStr = args.First()
timeLockDeltaSet = true
args = args.Tail()
}
if timeLockDeltaSet {
tld, err := parseTimeLockDelta(timeLockDeltaStr)
if err != nil {
return err
}
timeLockDelta = uint32(tld)
timeLockDeltaSpecified = true
}A similar refactoring can be applied to the other argument parsing blocks.
References
- The style guide recommends adding a brief comment for each
casein aswitchstatement and separatingcaseblocks with a newline for better readability. The current implementation does not follow this. (link)
| } | ||
| if uint32(policy.FeeRate) != newPolicy.FeeRate { | ||
| if uint32(policy.FeeRate) != *newPolicy.FeeRate { | ||
| t.Fatal("unexpected base fee") |
There was a problem hiding this comment.
The error message here seems to be a copy-paste error. It should probably say 'unexpected fee rate' instead of 'unexpected base fee' to match the check being performed.
| t.Fatal("unexpected base fee") | |
| t.Fatal("unexpected fee rate") |
| } | ||
| if uint32(policy.FeeProportionalMillionths) != newPolicy.FeeRate { | ||
| if uint32(policy.FeeProportionalMillionths) != *newPolicy.FeeRate { | ||
| t.Fatal("unexpected base fee") |
There was a problem hiding this comment.
The error message here seems to be a copy-paste error. It should probably say 'unexpected fee rate' instead of 'unexpected base fee' to match the check being performed.
| t.Fatal("unexpected base fee") | |
| t.Fatal("unexpected fee rate") |
1 participant
Summary
This PR makes the
base_fee_msat,fee_rate/fee_rate_ppm, andtime_lock_deltafields truly optional in theUpdateChannelPolicyRPC.base_fee_msat_specified,fee_rate_specified,time_lock_delta_specifiedproto fieldsmake rpc) is neededRelated PR
Problem
Previously, users had to specify ALL fields on every invocation, even when they only wanted to update a single field. This was a major pain point reported in #1523.
Solution
Add
*_specifiedboolean flags (following the existingMinHtlcMsatSpecifiedpattern) that allow the server to distinguish between "user wants to set this to X" and "user didn't specify this, keep the existing value".Changes
Protocol changes (
lnrpc/lightning.proto)base_fee_msat_specified(field 12)fee_rate_specified(field 13)time_lock_delta_specified(field 14)Internal API changes (
routing/router.go)FeeSchema.BaseFee: changed from value to pointer (*lnwire.MilliSatoshi)FeeSchema.FeeRate: changed from value to pointer (*uint32)ChannelPolicy.TimeLockDelta: changed from value to pointer (*uint32)RPC server changes (
rpcserver.go)*_specifiedflag is trueCLI changes (
cmd/commands/commands.go)ctx.IsSet()determines whether to set*_specifiedflagschainregconstantsLocal channel manager changes (
routing/localchans/manager.go)updateEdge()only updates fields when pointers are non-nilMinHTLCUsage Example
Users can now update individual policy fields independently:
API Compatibility
This is a backwards compatible change:
*_specifiedflags default tofalse, preserving existing behaviorTest Plan
go build ./...- compiles successfullygo test ./routing/localchans/...- all tests passmake rpc) - manual step neededNote
lightning.pb.gofile was manually updated to match the proto changes. A propermake rpcregeneration is needed before merge.Fixes #1523