lnrpc: rejects positive inbound fees by default #8627
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent update introduces a configuration option allowing Changes
Possibly related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Actionable comments outside the diff hunks (7)
cmd/lncli/commands.go (1)
Line range hint
2332-2333: The error message for missing channel backup inparseChanBackupsfunction could be more user-friendly by suggesting possible solutions or corrections.- var errMissingChanBackup = errors.New("missing channel backup") + var errMissingChanBackup = errors.New("missing channel backup, please specify one of --single_backup, --multi_backup, --single_file, or --multi_file")config.go (6)
Line range hint
1035-1055: LGTM! TheCleanAndExpandPathfunction correctly expands environment variables and the user's home directory, and cleans the path.
Line range hint
1057-1118: LGTM! ThesupplyEnvValuefunction correctly handles various formats for environment variable references and returns the appropriate values.
Line range hint
1120-1158: Consider using a buffered reader instead ofioutil.ReadAllto handle large configuration files more efficiently.- configContents, err := ioutil.ReadAll(btcdConfigFile) + configContents, err := bufio.NewReader(btcdConfigFile).ReadBytes('\n')
Line range hint
1160-1257: Consider using a buffered reader instead ofioutil.ReadAllfor reading thebitcoindconfiguration file to handle large files more efficiently, similar to the suggestion forextractBtcdRPCParams.- configContents, err := ioutil.ReadAll(bitcoindConfigFile) + configContents, err := bufio.NewReader(bitcoindConfigFile).ReadBytes('\n')
Line range hint
1259-1365: LGTM! TheconfigToFlatMapfunction correctly uses reflection to build a flat map of configuration options and identify deprecated options.
Line range hint
1367-1379: LGTM! ThelogWarningsForDeprecationfunction effectively logs warnings for deprecated config options.
Review Status
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between 6377f98 and 9b1b8418f86659750071f26c45520fa3e7e1558e.Files selected for processing (4)
- cmd/lncli/commands.go (1 hunks)
- config.go (1 hunks)
- rpcserver.go (1 hunks)
- sample-lnd.conf (1 hunks)
Additional comments not posted (2)
sample-lnd.conf (1)
441-445: The addition of theno-reject-positive-inbound-feesconfiguration option is well-implemented and aligns with the PR objectives. The cautionary note about its experimental nature is appropriate and helps inform users of the potential risks. This change provides users with the flexibility to tailor their fee acceptance policy according to their operational preferences.cmd/lncli/commands.go (1)
2376-2389: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2375-2376]
The check for
inboundBaseFeeMsatbeing withinmath.MinInt32andmath.MaxInt32is redundant sinceinboundBaseFeeMsatis of typeint64and these checks will always pass.- if inboundBaseFeeMsat < math.MinInt32 || - inboundBaseFeeMsat > math.MaxInt32 { + if false {
feelancer21
force-pushed
the
inbound-fees-reject
branch
from
9b1b841
to
62d4cbc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Actionable comments outside the diff hunks (3)
docs/release-notes/release-notes-0.18.0.md (3)
Line range hint
27-112: The "Bug Fixes" section is well-structured and clearly documents the fixes in this release. Great job maintaining clarity and providing direct links to the relevant pull requests.
Line range hint
251-312: The "Improvements" section clearly outlines the enhancements made across different components of the project, contributing to better functionality and user experience.
Line range hint
373-435: The "Technical and Architectural Updates" section effectively communicates significant changes and improvements in the project's architecture and technical specifications. The updates are well-documented and provide valuable insights into the project's evolution.
Review Status
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between 6377f98 and 62d4cbc2a4d43e0303636e308dbbb5393de2826b.Files selected for processing (5)
- cmd/lncli/commands.go (1 hunks)
- config.go (1 hunks)
- docs/release-notes/release-notes-0.18.0.md (2 hunks)
- rpcserver.go (1 hunks)
- sample-lnd.conf (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- cmd/lncli/commands.go
- config.go
- rpcserver.go
- sample-lnd.conf
Additional comments not posted (2)
docs/release-notes/release-notes-0.18.0.md (2)
120-128: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [116-189]
The "New Features" section effectively communicates the enhancements introduced in this release, including the notable addition of handling for inbound routing fees. The descriptions are concise and informative.
450-456: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [446-467]
The "Contributors" section appropriately acknowledges the individuals who contributed to this release, fostering a sense of community and appreciation for their efforts.
saubyk
added
inbound fee
feelancer21
force-pushed
the
inbound-fees-reject
branch
2 times, most recently
from
958a148
to
cb1eb5b
Compare
|
Linking alternative option to enable on the api level, for completeness. |
There was a problem hiding this comment.
I agree that the argument of unintentional use via third party tools is a valid one. If we'd add an RPC flag to enable positive fees, we'd offload the user education part onto a third party dev. Also, experimentation capabilities are increased here because of the removal of lncli limitations.
An API user could check via GetDebugInfo if the config value is set to know beforehand whether positive inbound fees are supported, which was an issue that I could see.
All of the three options we have would be fine in my opinion, keeping it as is, reject with config or reject with an api flag. But to be extra careful, the most consistent one is to choose this approach I think.
Have some remarks for improvements, otherwise this looks good!
Positive inbound are now rejected by default. The user can enable positive inbound fees with the option 'accept-positive-inbound-fees'.
feelancer21
force-pushed
the
inbound-fees-reject
branch
from
cb1eb5b
to
a2319e4
Compare
Positive inbound are now rejected by default. The user can enable positive inbound fees with the option 'no-reject-positive-inbound-fees'.
Change Description
Description of change / link to associated issue.
Steps to Test
Tested with
lncli updatechanpolicywith positive and negative inbound fees and with enabled and disabled optionPull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.