Clarify misleading updatechanpolicy cli help

[SachinMeier]

trafficstars

Change Description

Associated Issue: https://github.com/lightningnetwork/lnd/issues/1523

The updatechanpolicy cli command uses IntFlag and FloatFlag, which autopopulate a (default: 0) at the end of flag descriptions. These are unhelpful since we ignore flags that are not set by this command, and do not set them to zero.

Steps to Test

No lnd behavior should change. For lncli, no behavior outside of the help message for updatechanpolicy should change.

To test:

• Run lnd with at least one channel.
• Set fees to a specific non-zero rate using fee_rate flag.
• Verify that fees were accurately set using the feereport command.
• Set fees to zero by setting fee_rate to 0.0.
• Verify that fees on that channel are now 0.
• Set fees to non-zero rate using fee_rate_ppm flag.
• Verify that fee rate updated using feereport command.
• Set fees to zero by setting fee_rate_ppm to 0.
• Verify that fees on that channel are now 0.

Pull Request Checklist

Testing

• [ ] Your PR passes all CI checks.
• [ ] Tests covering the positive and negative (error paths) are included.
• [ ] Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• [x] The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• [x] Commits follow the Ideal Git Commit Structure.
• [ ] Any new logging statements use an appropriate subsystem and logging level.
• [x] There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

[SachinMeier]

Updated commit message, thanks!

[SachinMeier]

@yyforyongyu: review reminder @SachinMeier, remember to re-request review from reviewers when ready

Thank you. @C-Otto has an alternative MR to switch to v2 of the cli. @Roasbeef didn't seem to like that every arg is now a string, but in my opinion, this is the simplest way to get rid of inaccurate argument descriptions for all args, so I have not made any updates yet. If anyone has further suggestions, I am happy to go with another idea.

[C-Otto]

Note that I recommend to NOT switch to v2, see the comments in my PR.

[lightninglabs-deploy]

@sachinmeier, remember to re-request review from reviewers when ready

[SachinMeier]

After receiving feedback from a few people, I still think the current changes would result in the simplest interface for CLI users, even if we are interpreting integer arguments as strings and then parsing integers internally. The other methods for removing the defaults added even more code, and I do not think this command should set any values implicitly if they were not specified explicitly.

[guggero]

So the way I see it, we have two options here:

1. Use string flags to get rid of the misleading (default: 0) suffix and do all integer parsing manually.
2. Set the Value field for all fields to their default values lnd would use and then implement ctx.IsSet() for all of the optional flags to find out if the user wanted to set them or not (it IsSet() returns false, we'd set the value to 0 on the RPC to make sure we don't overwrite the value).

@Roasbeef, @yyforyongyu what's your opinion on this? Personally I like option 2, even if that is a bit more effort to implement.

[SachinMeier]

thank you @guggero. I am satisfied with option 2 as well. I'll add those changes this week if possible.

WRT release notes, which release should I target with this MR? i have a conflict in the old release notes I need to resolve.

[guggero]

WRT release notes, which release should I target with this MR? i have a conflict in the old release notes I need to resolve.

I think v0.16.0-beta is still the aim here.

[yyforyongyu]

So the way I see it, we have two options here:

1. Use string flags to get rid of the misleading (default: 0) suffix and do all integer parsing manually.
2. Set the Value field for all fields to their default values lnd would use and then implement ctx.IsSet() for all of the optional flags to find out if the user wanted to set them or not (it IsSet() returns false, we'd set the value to 0 on the RPC to make sure we don't overwrite the value).

@Roasbeef, @yyforyongyu what's your opinion on this? Personally I like option 2, even if that is a bit more effort to implement.

Yeah I'd say option 2 is better, option 1 seems hacky.

[guggero]

@SachinMeier it doesn't look like you're using ctx.IsSet() at all in the latest version?

[SachinMeier]

My apologies for the delay. I have updated the PR to make CLTV optional. I did not do so with baseFee and feeRate since we'd need flags to differentiate between unset and 0 values. This seemed like overkill to me, so I didn't implement it.

it doesn't look like you're using ctx.IsSet() at all in the latest version?

@guggero is this how you intended for ctx.IsSet to be used? I think i misinterpreted you last time.

[lightninglabs-deploy]

@sachinmeier, remember to re-request review from reviewers when ready

[lightninglabs-deploy]

@sachinmeier, remember to re-request review from reviewers when ready

[SachinMeier]

I am unclear on what people think is the best approach, so I'm going to close this