Skip to content
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

UI to balance LN channels #859

Merged
merged 17 commits into from
Apr 13, 2025
Merged

Conversation

uwla
Copy link
Contributor

@uwla uwla commented Mar 15, 2024

Solves #831.

NOTE: work in progress, implementation not ready yet.

Done

  • Add new action in network store to autobalance channels.
  • Add button component that dispatch that action

To do

  • Make it work
  • Add tests

Help needed here.

Trying to auto-balance the channel I got the error Insufficient balance, but this should not happen since the source has more than enough funds to pay the target and that can be confirmed by logging the amounts in the console.

I decided to open the PR anyway, so I can get help with the bug.

@uwla
Copy link
Contributor Author

uwla commented Mar 15, 2024

I need some help here. After fixing the mentioned bug, I can add tests and finish the remaining work in the PR.

Copy link
Owner

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to auto-balance the channel I got the error Insufficient balance, but this should not happen since the source has more than enough funds to pay the target and that can be confirmed by logging the amounts in the console.

I tested this PR and it's working fine for me. I'm not sure why you're getting that error on your end. Can you share more of the LND logs from one of the nodes?

Since it works for me, I left some feedback on the code.

@uwla uwla force-pushed the autobalance-channels branch from f473d7a to 01e3f8c Compare March 23, 2024 19:13
@codecov-commenter
Copy link

codecov-commenter commented Mar 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (49af2a0) to head (e390da4).
Report is 17 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##            master      #859    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          154       156     +2     
  Lines         5571      5694   +123     
  Branches      1129      1095    -34     
==========================================
+ Hits          5571      5694   +123     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@uwla
Copy link
Contributor Author

uwla commented Mar 23, 2024

@jamaljsr I addressed nearly all issues except the dropdown menu. I can implement tests for the added functionality, but I stil get the following error:

lndService.ts:160 Uncaught (in promise) Error: insufficient_balance
    at LndService.payInvoice (lndService.ts:160)
    at async Object.fn (lightning.ts:249)
    at async Promise.all (:3000/index 0)
    at async Object.fn (network.ts:972)
VM7596:2 Uncaught ReferenceError: process is not defined
    at Object.4043 (<anonymous>:2:13168)
    at r (<anonymous>:2:306599)
    at Object.8048 (<anonymous>:2:9496)
    at r (<anonymous>:2:306599)
    at Object.8641 (<anonymous>:2:1379)
    at r (<anonymous>:2:306599)
    at <anonymous>:2:315627
    at <anonymous>:2:324225
    at <anonymous>:2:324229
    at HTMLIFrameElement.e.onload (index.js:1)

You said it worked on your side, so I ask you to test again. I'll try to figure out why it is not working on my machine.

Actually, I'm getting some buggy behavior unrelated to the PR: sometimes I can't click on anything, which I suppose not to be a polar bug but a bug with electron due to my window manager.

@uwla
Copy link
Contributor Author

uwla commented Apr 9, 2024

I'll be back to it in a few days.

@jamaljsr
Copy link
Owner

Hey @uwla are you planning to continue to work on this PR?

I took a look at your latest changes and noticed what is causing the insufficient_balance error. You have the target and source swapped in the autoBalanceChannels function. The target should be the node creating the invoice and the source should pay it.

        promisesToAwait.push(
          createInvoice({ node: target, amount }).then(invoice =>
            payInvoice({ node: source, amount, invoice }),
          ),
        );

@uwla
Copy link
Contributor Author

uwla commented Apr 24, 2024

Hey @uwla are you planning to continue to work on this PR?

I took a look at your latest changes and noticed what is causing the insufficient_balance error. You have the target and source swapped in the autoBalanceChannels function. The target should be the node creating the invoice and the source should pay it.

        promisesToAwait.push(
          createInvoice({ node: target, amount }).then(invoice =>
            payInvoice({ node: source, amount, invoice }),
          ),
        );

Thanks @jamaljsr ! I am back to the PR, working on it today

@uwla
Copy link
Contributor Author

uwla commented Apr 24, 2024

I got some problems:

  • LND can pay to LND
  • LND can pay to CoreLightning
  • LND cannot pay to Eclair neither Eclair can pay to LND:
Uncaught (in promise) Error: no_route
    at LndService.payInvoice (lndService.ts:160)
    at async Object.fn (lightning.ts:249)
    at async Promise.all (:3000/index 0)
    at async Object.fn (network.ts:971)
Uncaught ReferenceError: process is not defined
    at Object.4043 (<anonymous>:2:13168)
    at r (<anonymous>:2:306599)
    at Object.8048 (<anonymous>:2:9496)
    at r (<anonymous>:2:306599)
    at Object.8641 (<anonymous>:2:1379)
    at r (<anonymous>:2:306599)
    at <anonymous>:2:315627
    at <anonymous>:2:324225
    at <anonymous>:2:324229
    at HTMLIFrameElement.e.onload (index.js:1)
  • CoreLightning cannot pay to CoreLightning:
Uncaught (in promise) Error: lightningd -32602: msatoshi parameter unnecessary
    at request (clightningApi.ts:38)
    at processTicksAndRejections (internal/process/task_queues.js:93)
    at async CLightningService.payInvoice (clightningService.ts:173)
    at async Object.fn (lightning.ts:249)
    at async Promise.all (:3000/index 0)
    at async Object.fn (network.ts:971)

After playing a bit, I found out I am also getting the same behavior when generating and paying invoices manually with the mentioned implementations: Create invoice -> Copy and Close -> Pay invoices.

The only which works pretty well is LND to LND.

@jamaljsr
Copy link
Owner

jamaljsr commented Apr 27, 2024

Thanks for the details on the issues your facing. I think I've found solutions to your outstanding problems.

LND cannot pay to Eclair neither Eclair can pay to LND:

I had to dive pretty deep into the weeds to figure this one out. I was able to reproduce the behavior on my end as well. The problem is essentially a configuration issue on the Eclair side. By default, it won't let you make a single payment that is more than 45% of the channel capacity. Check the reference.conf for a more detailed explanation. It would be nice if Eclair logged a clearer error message, but it doesn't. When I changed this to 100%, then Eclair began behaving correctly.

  --channel.max-htlc-value-in-flight-percent=100

Copy this config value into the default Eclair startup flags in constants.ts.

  • CoreLightning cannot pay to CoreLightning:
Uncaught (in promise) Error: lightningd -32602: msatoshi parameter unnecessary
    at request (clightningApi.ts:38)
    at processTicksAndRejections (internal/process/task_queues.js:93)
    at async CLightningService.payInvoice (clightningService.ts:173)
    at async Object.fn (lightning.ts:249)
    at async Promise.all (:3000/index 0)
    at async Object.fn (network.ts:971)

This error is actually saying that you should not provide the amount parameter to payInvoice since the invoice was created with an amount specified. The amount field should only be provided to CLN when the invoice contains a zero amount. The other implementations are more forgiving about this.

The API Docs state: "The pay RPC command attempts to find a route to the given destination, and send the funds it asks for. If the bolt11 does not contain an amount, amount_msat is required, otherwise if it is specified it must be null."

I also saw this error in my testing and remove the amount when calling payInvoice fixed it for me.

@jamaljsr
Copy link
Owner

After testing this a bit more, I feel like immediately balancing the channels when the button is clicked can be a bit unexpected. It would be very cool if it instead displayed a modal with the details of the payments that will be made on each channel. Then when the Submit button in the modal is clicked, the channels would get balanced. What do you think about this idea?

@uwla
Copy link
Contributor Author

uwla commented May 3, 2024

@jamaljsr I think it is a good idea.

So, we show a modal and for each channel:

  • left side shows the opening node, with input field to choose amount
  • followed by a slider
  • right side shows the other node, with input field ot choose amount

After all channels, we add a button on the bottom "auto balance all channels", which will autobalance the preview values.

Finally, at the very bottom, a CONFIRM button an a CANCEL button. If user confirms, each channel will be updated in batch if needed. Otherwise the modal is hidden and its state reset.

What about it now?

@uwla
Copy link
Contributor Author

uwla commented May 3, 2024

I can create the modal and add some tests for the coverage not to drop.

However, I have been facing an weird issue while developing polar locally: after launching the live Electron app, a lot of times I can not click on buttons, and the cursor icon does not alter (it is the same as normal, instead of a click pointer). This has been the most headache thing, because I have to open the devtools and do something like document.querySelector("myquery button").click() to trigger the click and see the effects of my code

@jamaljsr
Copy link
Owner

jamaljsr commented May 3, 2024

Hey, I really like the idea of the sliders. That would be very cool. I'm not sure about the input fields because that can really clutter up the limited space in the small modal. I think it might be easier to implement and more visually appealing to just display the node name and balance on each side with the slider in between.

However, I have been facing an weird issue while developing polar locally: after launching the live Electron app, a lot of times I can not click on buttons, and the cursor icon does not alter (it is the same as normal, instead of a click pointer). This has been the most headache thing, because I have to open the devtools and do something like document.querySelector("myquery button").click() to trigger the click and see the effects of my code

I have also noticed this happening on my machine whenever I save my changes and the UI hot reloads. I'm not sure where this is coming from, but my hunch is it's related to webpack. There is an invisible iframe being injected on the page which covers the whole screen so you can't click on anything.
image

I have been working around this using two methods depending on whether I want to lose my current place in the UI or not. Note: I'm on a Mac so the keyboard shortcuts may be different if you're on another OS.

  1. Hit CMD + SHIFT + C then click anywhere in the UI. This will select the iframe element in the DevTools Elements tab. Then I right-click on the <iframe> DOM element then delete it. Now Im able to interact with the page normally.
  2. Just hit CMD + R. This will reload the app and most times the iframe will be gone.

I plan on investigating where this iframe is coming from and fixing it over the next few days. It's been pretty annoying since I've also been working on some updates lately.

@jamaljsr
Copy link
Owner

jamaljsr commented May 3, 2024

After writing the above response, I did a quick google search and found the cause of the issue. It is the react-error-overlay package injecting the iframe. I just needed to force yarn to use v6.0.9 and that resolves the problem. I just pushed 4df612c to master, so if you rebase this branch, you won't need to use the workarounds I suggested. Please let me know if this is still an issue for you.

@uwla
Copy link
Contributor Author

uwla commented May 7, 2024

@jamaljsr I have sync my fork and now the bug is fixed.

I'm working on the dropdown menu and the modal.
It will be awesome when finished!

@uwla
Copy link
Contributor Author

uwla commented May 10, 2024

Hey @jamaljsr, when creating a 250,000 sats channels, which is the default, I noticed if the implementation is LND, then the source starts with 246530 sats and the target 0 sats. Where are the missing sats?

@uwla uwla force-pushed the autobalance-channels branch from 01e3f8c to cb98bbe Compare May 10, 2024 20:19
@uwla
Copy link
Contributor Author

uwla commented May 10, 2024

I have not finished the PR, but I pushed it anyway so you can see how it is going.

I added the dropdown and the modal.

I still need to: implement business logic, add tests, code styling refactoring.

@jamaljsr
Copy link
Owner

Thanks for the update. I will check it out over the next few days.

@uwla
Copy link
Contributor Author

uwla commented May 14, 2024

I'll add business logic today. The laast PR added UI, but it did not implement any logic

@jamaljsr
Copy link
Owner

Hey @jamaljsr, when creating a 250,000 sats channels, which is the default, I noticed if the implementation is LND, then the source starts with 246530 sats and the target 0 sats. Where are the missing sats?

Sorry, I forgot to answer this question. The balance is decreased by the fee for the commitment transaction and the amount allocated to the anchor output, since Anchor channels are created by default in LND. You can use lncli listchannels to get the commitment fee (commit_fee). For anchor outputs, the amount is always 330 sats.

For example, here's the output for one 250K sat channel I just created.

lnd@alice:/$ lncli listchannels
{
    "channels": [
        {
            "active": true,
            "remote_pubkey": "0347925fd31b4a2c6661fc0cc5e60fea9e549d5f2d94587048b4929aa6d8a2baa7",
            "channel_point": "03a75ce3208bf7d3cfc386d118638e5d0fe47c28caaa063220cda80c12dd6ec2:0",
            "chan_id": "131941395398656",
            "capacity": "250000",
            "local_balance": "246530",
            "remote_balance": "0",
            "commit_fee": "3140",
            "commit_weight": "772",
            "fee_per_kw": "2500",
            "unsettled_balance": "0",
            "total_satoshis_sent": "0",
            "total_satoshis_received": "0",
            "num_updates": "0",
            "pending_htlcs": [],
            "csv_delay": 144,
            "private": false,
            "initiator": true,
            "chan_status_flags": "ChanStatusDefault",
            "local_chan_reserve_sat": "2500",
            "remote_chan_reserve_sat": "2500",
            "static_remote_key": false,
            "commitment_type": "ANCHORS",
            "lifetime": "9",
            "uptime": "9",
            "close_address": "",
            "push_amount_sat": "0",
            "thaw_height": 0,
            "local_constraints": {
                "csv_delay": 144,
                "chan_reserve_sat": "2500",
                "dust_limit_sat": "354",
                "max_pending_amt_msat": "247500000",
                "min_htlc_msat": "1",
                "max_accepted_htlcs": 483
            },
            "remote_constraints": {
                "csv_delay": 144,
                "chan_reserve_sat": "2500",
                "dust_limit_sat": "354",
                "max_pending_amt_msat": "247500000",
                "min_htlc_msat": "1",
                "max_accepted_htlcs": 483
            },
            "alias_scids": [],
            "zero_conf": false,
            "zero_conf_confirmed_scid": "0",
            "peer_alias": "bob",
            "peer_scid_alias": "0",
            "memo": ""
        }
    ]
}

The balance breakdown is as follows:

    250,000   <- Channel capacity
-     3,140   <- Commitment transaction fee
-       330   <- Anchor output
-----------
    246,530   <- Local balance

@uwla uwla force-pushed the autobalance-channels branch from cb98bbe to 4cd78dd Compare May 16, 2024 19:03
@uwla
Copy link
Contributor Author

uwla commented May 16, 2024

Thanks @jamaljsr for the updates.

My last push had some UI improvements. I will implement business logic tomorrow, by adding a new store action. Things are working pretty well now that I can click buttons and test things with hot-reload, after fixing the react overlay bug.

@uwla
Copy link
Contributor Author

uwla commented May 17, 2024

With the last push, it is WORKING!

At least with LND nodes. It stills does not work well with Clightning and Eclair due to the mentioned bugs, which we were discussing previously.

Now, what remains:

  1. solve the issues with clightning and eclair
  2. add automated tests
  3. minor UI improvents: nicer buttons and autoclose the modal afterwards
  4. refactoring (I just want a working prototype, so I may have added functions and interfaces in the wrong places)

@jamaljsr I need your help with (1), since you know what is causing it. I also need your opinion with (4), because I am not familiar with Polar's preferences for code style, where the added functions and interfaces should be placed.

@jamaljsr
Copy link
Owner

@jamaljsr I need your help with (1), since you know what is causing it. I also need your opinion with (4), because I am not familiar with Polar's preferences for code style, where the added functions and interfaces should be placed.

Did you apply the fixes I mentioned in this prior comment? This resolved the Core Lightning and Eclair issues for me.

4. refactoring (I just want a working prototype, so I may have added functions and interfaces in the wrong places)

I will review your latest changes by tomorrow for sure.

@uwla
Copy link
Contributor Author

uwla commented Jun 6, 2024

I'll solve the issues with Eclair and Clightning as you explained before @jamaljsr
Let's make this cool feature happen

@uwla
Copy link
Contributor Author

uwla commented Jun 6, 2024

Some updates here.

So, I have changed the constant.ts to include the flags in the Eclair node:

      '--channel.max-htlc-value-in-flight-percent=100',
      '--channel.max-htlc-value-in-flight-msat=5000000000000', 

I created a new network with 1 LND, 1 Clightning and 1 Eclair.

Still got 3 types of errors, specially when trying to transact the whole amount:

Unhandled Rejection (Error): balance too low
EclairService.getPaymentStatus
src/lib/lightning/eclair/eclairService.ts:229

Unhandled Rejection (Error): route not found
EclairService.getPaymentStatus
src/lib/lightning/eclair/eclairService.ts:229

Unhandled Rejection (Error): lightningd 210: Ran out of routes to try after 65 attempts: see `paystatus`
request
src/lib/lightning/clightning/clightningApi.ts:38

The Eclair node is unable to find a "route" to a node it is directly connected? And the Clightning too.

Also, even though I have passed the flags to the default Eclair flags, it still says "balance is too low" when the remaining balance is 0.

When leaving each channel with at least around 1000 satoshis it works and the "too low" error does not show up, but sometimes the "route not found" error does happen.

The Eclair and Clightning implementation are not good compared to LND

@uwla uwla force-pushed the autobalance-channels branch from 4cd78dd to 49be98d Compare June 9, 2024 18:33
Copy link
Owner

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @uwla thanks for updating this. Apologies for taking so long to review.

I tested the functionality and it works great! I also reviewed all of the code and don't have any further suggestions there.

The only thing left to do is to get the tests passing and coverage back up to 100%. Are you able to work on this in order to get this PR merged?

@uwla
Copy link
Contributor Author

uwla commented Sep 9, 2024

Hi @jamaljsr I'll try to find some free time on the coming weeks and attempt to get the tests passing. I hope I'll be able to get coverage back to 100% and will post updates here if help is needed

@uwla
Copy link
Contributor Author

uwla commented Nov 1, 2024

Hi @jamaljsr . It's been over 1 month and I have been very busy, so unfortunately I won't be able to work on adding tests soon (

Can you delegate/assign the commit which adds tests to somebody else?

But please, let me have the credits in the git history for the commit which adds the feature itself.

@jamaljsr
Copy link
Owner

jamaljsr commented Nov 1, 2024

Hey @uwla, I appreciate the update. I'll work on adding the tests when time permits. I will retain your existing commits in the git history.

@Jem256
Copy link
Contributor

Jem256 commented Jan 1, 2025

Hey @uwla, I appreciate the update. I'll work on adding the tests when time permits. I will retain your existing commits in the git history.

Hey @jamaljsr can I help with adding the tests if you've not worked on them yet?

@jamaljsr
Copy link
Owner

jamaljsr commented Jan 2, 2025

Hey @Jem256, yes please feel free to add the tests. Thank you 🙏

@Jem256
Copy link
Contributor

Jem256 commented Apr 6, 2025

Hey @uwla I added tests for this feature. Please check out my PR on your branch

@uwla
Copy link
Contributor Author

uwla commented Apr 7, 2025

thanks @Jem256 for the contribution, I had forgotten about this.

Let us make this feature done.

While I have not used Polar for a while, tt is likely I will need this software soon. So, if anything is needed from my side, I will be glad to help and potentially make more contributions to this software 👍

@uwla
Copy link
Contributor Author

uwla commented Apr 7, 2025

We have got it passed @jamaljsr

image

Nonethless, I have not manually tested the UI for the last commits.
I only tested the UI last year, before the tests were added, and it worked perfectly.

@uwla uwla changed the title feat: add button to auto-balance LN channels UI to balance LN channels Apr 7, 2025
uwla and others added 17 commits April 12, 2025 22:44
The modal has slicers to manually balance LN channels.
It also has a button to evenly balance the channels.
In this commit we add a role to the balance channel button so we
can target it in the next commit and test.
This commit then tests the button and its intended functionlities
In this commit we did a little refactor to the BalanceChannelsModal
component to make it easier to test, which we will do in the next
commit.
In this commit we test the refactored BalanceChannelsModal
component, to cover for the function call.
Since channelsInfo is always an array and we loop through it
before anything means if it's empty we just do nothing. Also this
makes it easier to test.
This commit tests for when resetChannelsInfo was called on a
channel with invalid id or a node withing the channel that has no
valid id for its 'to' node.
In this commit we test for when we try to manually try to update the
channel balance but passed in invalid index.
@jamaljsr jamaljsr force-pushed the autobalance-channels branch from ff813a2 to e390da4 Compare April 13, 2025 04:03
@jamaljsr
Copy link
Owner

Thanks for wrapping this up @uwla @Jem256 @Abdulkbk 🙏

I have rebased this on master and just added one minor UX improvement to disable the primary button after a click in the Balance Channels modal to prevent user's from triggering the process multiple times concurrently.

This now looks good to merge. 🚀

@jamaljsr jamaljsr merged commit 2420ead into jamaljsr:master Apr 13, 2025
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature request: add button to balance all existing channels in current network
5 participants