Add CLN #355
Add CLN #355
Conversation
m3dwards
force-pushed
the
add-cln
branch
5 times, most recently
from
dce9296 to
9332f26
Compare
m3dwards
force-pushed
the
add-cln
branch
4 times, most recently
from
912e4e5 to
dc4a27f
Compare
willcl-ark
left a comment
willcl-ark
left a comment
There was a problem hiding this comment.
Thanks for this PR, just taken a brief surface-level skim.
It seems to me that we have ended up with lnnode.py just including a tonne of if...else statements to support the two implementations. If we ever decide to add LDK this will only get worse. Perhaps we should, before merging this, create an LNNode base class, and subclass LndNode and CLNNode as we did previously for the two backends?
That way we keep them clean, and make adding new nodes much easier in the future?
It would also let us abstract away things like: https://github.com/bitcoin-dev-project/warnet/pull/355/files#diff-0bfb3fbf6aa1bb068dffe115137cfec50240a3e8713463149f47d8e1354ceef1R100-R107 too...?
pinheadmz
left a comment
pinheadmz
left a comment
There was a problem hiding this comment.
Reviewed and tested, really cool to see cln in there. I was able to get invoices and make payments in the 3 node graph. I agree with Will though, I think the implementations should be subclasses of LNNode. If that sounds like too much work for now we can address it in a follow-up. Functionally this PR works great for now.
I also wonder how we should handle warcli lncli... commands. Like, we should we abstract the LN commands such that all lnd commands work for cln and ldk as well?
| def channel_match(ch1, ch2): | ||
| if ch1["capacity"] != ch2["capacity"]: | ||
| return False | ||
| if policy_match(ch1["node1_policy"], ch2["node1_policy"]) and policy_match( | ||
| ch1["node2_policy"], ch2["node2_policy"] | ||
| ): | ||
| return True | ||
| return policy_match(ch1["node1_policy"], ch2["node2_policy"]) and policy_match( | ||
| ch1["node2_policy"], ch2["node1_policy"] | ||
| ) | ||
|
|
||
|
|
||
| def policy_match(pol1, pol2): | ||
| return ( | ||
| max(int(pol1["time_lock_delta"]), 18) == max(int(pol2["time_lock_delta"]), 18) | ||
| and max(int(pol1["min_htlc"]), 1) == max(int(pol2["min_htlc"]), 1) | ||
| and pol1["fee_base_msat"] == pol2["fee_base_msat"] | ||
| and pol1["fee_rate_milli_msat"] == pol2["fee_rate_milli_msat"] | ||
| ) |
There was a problem hiding this comment.
Aren't these defined in utils.py? Do they need to be stubbed here?
There was a problem hiding this comment.
I really hoped you wouldn't notice this but you are too smart for me.
The difference is that in in utils.py it now works with LNChannel objects which are returned from helper functions from LNNode. LNChannel is an abstraction over channels objects as returned by LND or CLN.
Why I couldn't use that in the test is that the test framework only uses the RPC which in this test is returning specific JSON from LND. I couldn't think of a quick solution and so added these helper functions here to keep the test passing. Ideally I think more and more things should move to use the abstractions and the CLI should provide functions for common lightning functions.
Agreed. Much nicer. |
m3dwards
force-pushed
the
add-cln
branch
3 times, most recently
from
f8d6594 to
2168590
Compare
|
@willcl-ark @pinheadmz switched to lightning subclasses. |
willcl-ark
left a comment
willcl-ark
left a comment
There was a problem hiding this comment.
Looks and seems to work great! nice work.
I left a single comment here, and also prepared 3 additional commits which you may be interested in taking here, or in a followup?
main...willcl-ark:warnet:add-cln-review
If you'd prefer followup, we can merge when @pinheadmz question about the --exclude option is resolved IMO
Taken |
m3dwards
force-pushed
the
add-cln
branch
3 times, most recently
from
330c09b to
c04f193
Compare
Co-authored-by: willcl-ark <will@256k1.dev>
Adds CLN as a lightning node option