-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Deprecate dust-threshold config value #9182
Conversation
f8ce024
to
c0f0f04
Compare
c0f0f04
to
e31c4d8
Compare
7514a69
to
4c395e8
Compare
4c395e8
to
7ff237e
Compare
7ff237e
to
7454f12
Compare
7454f12
to
17e9bd0
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.
LGTM 🎚️
@yyforyongyu: review reminder |
config.go
Outdated
// Don't allow both the old dust-threshold and the new | ||
// channel-max-fee-exposure to be set. | ||
defaultExposure := uint64(htlcswitch.DefaultMaxFeeExposure.ToSatoshis()) | ||
if cfg.DustThreshold != 0 && cfg.MaxFeeExposure != defaultExposure { |
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.
Q: why are we checking cfg.MaxFeeExposure != defaultExposur
but not cfg.MaxFeeExposure != 0
?
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.
I see it's set in the default, hmmm, then we'd have this issue, what if i run,
./lnd-debug --dust-threshold=100 --channel-max-fee-exposure=500000
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.
ok changed thanks for pointing this out
17e9bd0
to
7252248
Compare
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 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Replace ambigious config value "dust-treshold" with a more clear "channel-max-fee-exposure" exposure value. The old value is deprecated and will be removed in the near future.
7252248
to
69b5e3d
Compare
Because we need to remain backwards compatible with the old `dust-threshold` value we set the default value for `channel-max-fee-exposure` later in the program flow. Given the restrictions of the sample config check we need to exclude this value from the check.
69b5e3d
to
aebf03a
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.
tACK🎉
Introduced a more clear config value and deprecated the old
dust-threshold
value. The new setting is calledchannel-max-fee-exposure
. Nothing has changed code wise, the new value behaves as the old one but is way clearer and removes the ambiguity