-
-
Notifications
You must be signed in to change notification settings - Fork 218
Add PPM toggle for channel fees #3588
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
base: master
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @kunal-595, 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 user experience for setting channel fees by introducing a flexible input mechanism. Users can now choose to specify fee rates in either traditional percentages or the more granular Parts Per Million (PPM), a common unit in the Lightning Network. This enhancement is specifically tailored for LND-based backends, providing a more intuitive and aligned interface for managing channel liquidity. Highlights
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
|
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 adds a useful toggle for setting channel fees in either percentage or PPM for LND-based backends. The implementation is mostly correct, but a critical issue in the fee rate conversion logic was found that would lead to incorrect and excessively high fees being set. The detailed comment provides the fix.
stores/FeeStore.ts
Outdated
| // convert PPM to percentage for LND | ||
| if (feeRateMode === 'ppm' && BackendUtils.isLNDBased()) { | ||
| feeRate = new BigNumber(feeRate).dividedBy(10000).toString(); | ||
| } |
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 conversion from PPM to the decimal fee rate for LND is incorrect. You are dividing by 10000, but it should be 1000000 to convert from Parts Per Million to a decimal rate.
For example, a 1000 PPM fee rate should be converted to 0.001 for the LND API. Your current implementation would convert it to 0.1, which is 100x larger and would result in a 10% fee, which is extremely high.
The comment is also a bit misleading. It's not converting to a percentage, but rather to a decimal fee rate that LND expects.
Here is the suggested correction:
| // convert PPM to percentage for LND | |
| if (feeRateMode === 'ppm' && BackendUtils.isLNDBased()) { | |
| feeRate = new BigNumber(feeRate).dividedBy(10000).toString(); | |
| } | |
| // convert PPM to decimal rate for LND | |
| if (feeRateMode === 'ppm' && BackendUtils.isLNDBased()) { | |
| feeRate = new BigNumber(feeRate).dividedBy(1000000).toString(); | |
| } |
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.
Pull request overview
This PR adds a toggle to switch between percentage and PPM (parts per million) input modes for channel fee rates when setting fees. The toggle is only displayed for LND-based backends since Core Lightning (CLN) natively uses PPM units.
Changes:
- Added PPM to percentage conversion logic in the fee store
- Implemented a toggle button UI component to switch between percentage and PPM input modes
- Updated labels and placeholders to reflect the selected unit mode
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| stores/FeeStore.ts | Added feeRateMode parameter and PPM-to-percentage conversion logic for LND backends |
| components/SetFeesForm.tsx | Added toggle button UI, updated labels/placeholders based on mode, and integrated mode selection with fee submission |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
stores/FeeStore.ts
Outdated
| let feeRate = newFeeRate.replace(/,/g, '.'); | ||
|
|
||
| // convert PPM to percentage for LND | ||
| if (feeRateMode === 'ppm' && BackendUtils.isLNDBased()) { |
Copilot
AI
Jan 24, 2026
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 conversion from PPM to percentage should handle empty or invalid input strings. When feeRate is an empty string, calling new BigNumber(feeRate) will throw an error. Consider adding a check to only perform the conversion when feeRate is non-empty: if (feeRateMode === 'ppm' && BackendUtils.isLNDBased() && feeRate) {
| if (feeRateMode === 'ppm' && BackendUtils.isLNDBased()) { | |
| if (feeRateMode === 'ppm' && BackendUtils.isLNDBased() && feeRate) { |
|
I think we'd want to set up some sort of utility for this in |
|
Sounds good, i will move that PPM → % logic into FeeUtils, add some unit tests, and update the store, pushing shortly. |
|
Looking mostly solid so far. Even if CLNRest uses PPM by default, you should be able to toggle between the two modes with it. I think the next step after that would be being able to toggle between the units on the |
|
That makes sense, i will hook that up in FeeBreakdown so users can switch modes on the fly, i will ping you here once i have pushed the update to the branch. |
fixes #1411
Adds a toggle to switch between percentage and PPM input for fee rate when setting channel fees. Only shows for LND-based backends since CLN already uses PPM natively