-
Notifications
You must be signed in to change notification settings - Fork 119
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
drain and fill channel plugin #22
Conversation
4e5a008
to
de0383d
Compare
f0e0a88
to
0b533db
Compare
9c7c248
to
81af30d
Compare
5475d81
to
eeaaba1
Compare
ac762c3
to
f235c65
Compare
b6521a6
to
c694756
Compare
I removed the "work in progress" TAG, as the plugin is very usable aready and I dont have good clues about the remaining TODOs and optimizations. I could need a review now... volunteers? Note: If you get "Could not find a route" errors, when draining a channel, try using |
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.
Here is what caught my eyes (it's mostly stylish) :
- A docstring by function would be useful
- Type hints : you use them for some function, and not for some other, or even for some arguments of a function and not some other.
- Some nits (see below)
I also got an error when trying drain
:
Error while processing drain: TypeError('__int__ returned non-int (type float)')
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
22bea89
to
2346058
Compare
Btw if you think it's mergeable you should remove the WIP tag |
I removed the WIP flag, still Im going to fix this one way or another, now that we talked about this issues it is at least known. If you are fine we can merge, I lost my commit access because I have been inactive for a while, so we need someone else to merge it.
Yes ! |
I don't have one so we'll have to wait for ackbot ;-) |
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.
Mostly nits and questions, which most of them will need user testing to be resolved. I think it's mergeable (modulo cleanup and pylightning version ;-))
d98e716
to
ff19dc0
Compare
a617ef9
to
89b1775
Compare
I updated the documentation a bit to make clear how it is currently supposed to work. |
Ok so I have a channel with 60% theirs, 40% ours. ├────────┼─────────────┤ 245x2x0 I think the wording is misleading as if I want to drain, say 30 percent of the channel (so that the balance results in 90% theirs, 10% ours) ├──┼──────────────────────┤ 245x2x0 I dont cli drain <scid> 30 But cli drain <scid> 90 So effectively passing to the drain command the expected percent on cli set_theirs <scid> 90 And, if we keep the The same goes for ├──┼──────────────────────┤ 245x2x0 and I want my side to have 90% capacity (so passing from ├────────────────────┼─┤ 245x2x0 I would expect to drain from them 80% of the capacity (hence cli fill <scid> 90 That's why I propose to rename it to cli set_ours <scid> 90 |
a188a0c
to
c4a8d31
Compare
Sorry for the delay, I seem to have missed the non-WIP window 😉 I'm quite happy to merge the plugin even if it has a edge-case that may fail some times. fwiw I have a c-lightning PR pending that should allow us to start testing plugins, so pinning down the issue and reproducing it will become easier: ElementsProject/lightning#3218 |
9df451c
to
9a0f38d
Compare
lightning-cli fill scid [percentage] [chunks] [maxfeepercent] [retry_for] [exemptfee] | ||
``` | ||
|
||
Another useful command is the `setbalance` that will fill up or drain your side |
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.
🎉 ! 😁
9a0f38d
to
2ab1622
Compare
2ab1622
to
2f825de
Compare
Hi, After my first two days of my new 'Altersteilzeit' I managed to get this ready. @darosior I added the cheers, |
You guys also might want to test this, but for my setup and testnet and mainnet it works smoothly. |
So you don't have the spontaneous errors you were talking about ? |
Still have them from time to time, which is why I think it's not plug-in but daemon related. |
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.
ACK 2f825de
Tested this on my node, and it behaves exactly as I would expect it to behave. Playing with the chunk feature allows to rebalance channels it was not possible to rebalance previously: this is a really cool plugin ! 🎉
This plugin offers three new commands:
drain
,fill
andsetbalance
.It works similar to the
rebalance
plugin but with some key differences:scid
parameter but figures out the other one by routing.cunks
is not set, number of required chunks is autodetected.100
fordrain
andfill
and50
forsetbalance
.Note: If draining 100%, the plugin guesses the correct HTLC fee by try and error. For new servers that report HTLC fee via exception (ElementsProject/lightning#2691) it takes the exact amount.
Issues: