Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds Core Lightning (CLN) as a new LN backend across config, API, frontend, startup wiring, and a full CLN gRPC client (with hold-invoice support and subscriptions); includes docs and .env formatting/comments. Changes
Sequence DiagramsequenceDiagram
actor User
participant Frontend as Frontend UI
participant API as API Server
participant Config as Config Service
participant Service as Node Service
participant CLN as CLN gRPC
User->>Frontend: select CLN & submit CLNForm
Frontend->>API: POST /setup (includes clnAddress, clnLightningDir, clnAddressHold)
API->>Config: SetUpdate("CLNAddress"/"CLNLightningDir"/"CLNAddressHold")
Config-->>API: persist result / error
API-->>Frontend: setup response
Service->>Config: read CLN config on start
Service->>CLN: NewCLNService(connect w/ TLS, optional hold plugin)
CLN-->>Service: connection established / subscriptions active
Frontend->>User: navigate to next setup step
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@daywalker90 awesome, thanks for the PR! what's the best way to test this? can I use Polar? |
|
Never used Polar but they list CLN aswell on their website. The configs Myself i have CLN installed from compiling the source and then you can run a script from the CLN repo like this: which gives these commands: You can install a plugin like the hold plugin by copying/linking it in a newly created directory |
|
Polar works! I didn't try HOLD invoices yet. I setup using the advanced onboarding UI, not via .env |
|
@daywalker90 the overall structure is great. I had some issues doing basic tests though. For example, I made an invoice inside Polar from one node and tried to pay it with my CLN node from the Alby Hub wallet page. It said "Unimplemented". Are you able to use this basically as a daily driver for a while and make sure things are working as expected? |
|
So the file structure is like this: and i replaced all the individual file paths with just the lightning directory which includes the network directory. I also made a note in the README for people running CLN/Albyhub remotely and need to copy those files. There is no maintained go module for CLN's grpc that i'm aware of and i have added a README.md to the cln directory to document how it works. I have tested this myself:
I don't know how you managed to get an "unimplemented" error?! |
|
What i really like to test is a call to the listtransactions function. Is it just the "export transactions" button? Because for payment history it only does lookupinvoice. If so the ui wants me to get alby pro^^ |
|
I also tested both advanced setup UI and with the .env |
@daywalker90 testing list_transactions via NWC would be better because you have full control over all the filters. But I am happy to give you a 1 year free Alby Pro subscription so you can better test Alby Hub - just send an email to roland@getalby.com with the email you use and I will give you a coupon code. |
Here's what I did:
|
|
Ah, my CLN in Polar is v23.05.2. I will update Polar. |
|
I'm testing the NWC interface with my tests from my NWC plugin and already found something that needed fixing. Even when doing a keysend request without a preimage AlbyHub injects one into the request. CLN does not support keysend with a given preimage, so i added a check to return an error if a preimage is given. |
|
Via NWC |
|
The "Export Transactions" button also uses LookupInvoice ... Is it possible that ListTransactions is deprecated or something? I have put a panic at the top of it and everything still works fine?! |
|
@daywalker90 you said |
|
Well the CLN backend only supports the |
|
Ah, you're right. I think |
daywalker90
force-pushed
the
cln-backend
branch
from
8d07d07 to
bbe7f55
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In @.env.example:
- Around line 45-52: Update the example env entry for the CLN hold plugin so it
uses the separate gRPC port: change the CLN_ADDRESS_HOLD value from
127.0.0.1:9737 to 127.0.0.1:9738 (keeping CLN_ADDRESS at 127.0.0.1:9737) and
ensure the commented line for CLN_ADDRESS_HOLD reflects the hold plugin’s
distinct service port to match the README.md documentation.
In `@lnclient/cln/cln.go`:
- Around line 1616-1622: The current assignment to req.Feerate uses
uint32(uint64) arithmetic that can overflow when computing uint32(*feeRate) *
1000; fix by doing the multiplication in a wider type, check it against the
uint32 maximum, and only cast when safe: compute an intermediate value (e.g.,
satPerKw := uint64(*feeRate) * 1000), compare satPerKw to the maximum
representable uint32 (uint64(^uint32(0))), and either return an error or clamp
to the max before assigning to req.Feerate.Perkb (the clngrpc.Feerate_Perkb
field) so no overflow/truncation occurs.
- Around line 523-605: GetNetworkGraph currently calls ListNodes and
ListChannels without filters and pulls the entire gossip graph; instead build
and use filtered requests: for nodes, decode each nodeId from the nodeIds
parameter and call c.client.ListNodes(ctx, &clngrpc.ListnodesRequest{Id:
[][]byte{...}}) (or batch them) to only fetch the requested nodes (replace the
unfiltered ListNodes call that populates listnodes); for channels, avoid the
unfiltered ListChannels call and either query per known short channel ids via
clngrpc.ListchannelsRequest{ShortChannelId: []string{...}} or query channels
related to each node (by source/destination/pubkey filter if supported) so you
only fetch channels relevant to the requested nodeIds before building nodes and
channels and returning networkGraph.
- Around line 1719-1722: The current checks return fmt.Errorf("decode result
empty: %w", err) while err is already nil, causing a nil wrap; update the three
occurrences (the check after decoding, and the similar checks in SignMessage and
UpdateChannel) to avoid wrapping a nil error: either return a simple error like
fmt.Errorf("decode result empty") or, if you want to preserve the original error
when present, use an if err != nil { return nil, fmt.Errorf("decode result
empty: %w", err) } else { return nil, fmt.Errorf("decode result empty") } so no
nil is wrapped.
- Around line 902-920: GetSupportedNIP47Methods is advertising
MAKE_HOLD_INVOICE_METHOD, SETTLE_HOLD_INVOICE_METHOD, and
CANCEL_HOLD_INVOICE_METHOD even when the hold plugin (c.clientHold) is not
configured, and GetSupportedNIP47NotificationTypes likewise includes
HOLD_INVOICE_ACCEPTED_NOTIFICATION; update both functions to only include those
hold-related methods and notification types when c.clientHold != nil (or an
equivalent configured flag), i.e., conditionally append the
MAKE_HOLD_INVOICE_METHOD, SETTLE_HOLD_INVOICE_METHOD, CANCEL_HOLD_INVOICE_METHOD
in GetSupportedNIP47Methods and HOLD_INVOICE_ACCEPTED_NOTIFICATION in
GetSupportedNIP47NotificationTypes based on the presence of clientHold so the
advertised capabilities match actual runtime support.
- Around line 1232-1292: In clnInvoiceToTransaction, avoid nil-pointer derefs by
adding explicit nil checks and safe fallbacks: when computing amount, check
invoice.AmountReceivedMsat != nil before reading Msat and fall back to 0 if nil;
when setting paid_at, ensure invoice.PaidAt != nil before dereferencing and
leave paid_at nil if absent; when setting Description use a nil-safe read (treat
nil as empty string) instead of dereferencing *invoice.Description; update uses
of invoice.PaymentPreimage and invoice.PaymentHash similarly if they can be nil
(use empty slice/zero string) and keep the error handling around Decode as-is.
Ensure these checks are applied inside clnInvoiceToTransaction so the
Transaction struct fields are filled with safe defaults rather than causing
panics.
- Around line 45-133: NewCLNService leaks resources: connHold is not stored on
the CLNService, cancel() from context.WithCancel is never invoked on error
paths, and connHold (if created) is not closed when later errors occur (e.g.,
after GetInfo fails). Fix by adding a connHold field to the CLNService struct
and assign connHold when created, ensure you call cancel() and close both conn
and connHold on every early-return error path in NewCLNService (including after
the hold plugin is created and after GetInfo fails), and update Shutdown() to
close svc.connHold in addition to svc.conn and invoke svc.cancel if not nil.
- Around line 1762-1774: The Shutdown() method on CLNService currently calls
c.client.Stop() which shuts down the external CLN daemon and should be removed;
instead, modify CLNService.Shutdown to stop only local resources by calling
c.cancel() and then closing the gRPC connection c.conn (ensure you check for nil
and handle/return any close error appropriately), and remove the
c.client.Stop(c.ctx, &clngrpc.StopRequest{}) call and its error path so behavior
matches other backends (use logger.Logger.WithError when closing c.conn if an
error occurs).
- Around line 1434-1455: In CLNService.MakeInvoice, remove the forced
requirement that description must be present when descriptionHash is provided
and instead only validate the hash when both fields are supplied: if
descriptionHash != "" and description != "" then compute myDescriptionHash and
compare with descriptionHash (return error on mismatch); set deschashonly to
true when description is empty but descriptionHash is present (i.e., description
== "" && descriptionHash != ""); keep existing behavior when both are present.
This change touches the MakeInvoice function and the variables description,
descriptionHash, deschashonly, and myDescriptionHash.
In `@README.md`:
- Line 229: Update the outdated sentence that says "Currently only LND can be
configured via env" to reflect that both LND and CLN support environment
configuration; locate the sentence in README.md (near the CLN section and the
existing line stating LND env-only) and replace it with a concise statement such
as "Both LND and CLN can be configured via env or the UI" so the README is
consistent with the CLN documentation above.
🧹 Nitpick comments (4)
lnclient/cln/README.md (1)
29-45: Add a language identifier to the fenced code block.The directory structure code block on Line 29 is missing a language specifier. Static analysis (markdownlint MD040) flags this. Use
textorplaintextas the language identifier.Suggested fix
-``` +```text ~/dev/ ├── lightning/lnclient/cln/cln.go (3)
156-162: SetMinVersionon TLS config to enforce TLS 1.3 (or at minimum TLS 1.2).Without an explicit
MinVersion, Go defaults to TLS 1.0 in some contexts. Since this is communicating with a local/controlled CLN gRPC server, enforcing a modern TLS version is straightforward and eliminates a security posture gap.🔒 Proposed fix
return &tls.Config{ Certificates: []tls.Certificate{clientCert}, RootCAs: certPool, ServerName: serverName, // CLN uses "cln" as default ServerName, hold plugin uses "hold" + MinVersion: tls.VersionTLS12, }, nil
34-43: Unusedwgfield inCLNServicestruct.The
sync.WaitGroupfieldwgis declared but never used anywhere in the implementation.Proposed fix
type CLNService struct { ctx context.Context client clngrpc.NodeClient clientHold *clngrpcHold.HoldClient conn *grpc.ClientConn eventPublisher events.EventPublisher pubkey string cancel context.CancelFunc - wg sync.WaitGroup }This also allows removing the
"sync"import.
1113-1196: N+1 RPC calls inListPeers— oneListNodescall per channel.Each peer channel results in a separate
ListNodesRPC call. For nodes with many channels this will be slow. Consider fetching all nodes in a single call or batching by unique peer IDs.
| func (c *CLNService) MakeInvoice(ctx context.Context, amount int64, description string, descriptionHash string, expiry int64, throughNodePubkey *string) (transaction *lnclient.Transaction, err error) { | ||
| logger.Logger.WithFields(logrus.Fields{ | ||
| "amount": amount, | ||
| "description": description, | ||
| "description_hash": descriptionHash, | ||
| "expiry": expiry, | ||
| "through_node_pubkey": throughNodePubkey, | ||
| }).Debug("Make Invoice") | ||
|
|
||
| label := "AlbyHub-" + uuid.NewString() | ||
|
|
||
| var deschashonly bool | ||
| if descriptionHash != "" { | ||
| if description == "" { | ||
| return nil, fmt.Errorf("Must have description when using description_hash") | ||
| } | ||
| myDescriptionHash := sha256.Sum256([]byte(description)) | ||
| if descriptionHash != hex.EncodeToString(myDescriptionHash[:]) { | ||
| return nil, fmt.Errorf("description_hash does not match description") | ||
| } | ||
| deschashonly = true | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how LND backend handles descriptionHash vs description in MakeInvoice
rg -n -A 10 'descriptionHash|DescriptionHash' --type go -g '**/lnd/*.go'Repository: getAlby/hub
Length of output: 6432
🏁 Script executed:
#!/bin/bash
# Also search for MakeInvoice implementation in lnd backend
rg -n -A 15 'func.*MakeInvoice' --type go -g '**/lnd/*.go'Repository: getAlby/hub
Length of output: 1072
🏁 Script executed:
#!/bin/bash
# Search for NIP-47 related documentation or code
rg -n 'NIP.47|nip.47|descriptionHash' --type go -g '**/*.go' | head -30Repository: getAlby/hub
Length of output: 5385
🏁 Script executed:
#!/bin/bash
# Search for NIP-47 specification or references
rg -n 'NIP.47|nip47|spec.*description.*hash' --type go --type md -i | head -20Repository: getAlby/hub
Length of output: 2009
🏁 Script executed:
#!/bin/bash
# Check if there are test cases or callers using descriptionHash without description
rg -n 'MakeInvoice.*descriptionHash' --type go -B 2 -A 2 | head -40Repository: getAlby/hub
Length of output: 3907
🏁 Script executed:
#!/bin/bash
# Search for NIP-47 specification or documentation files
fd -e md -e txt -e go | xargs grep -l "description.hash\|descriptionHash" 2>/dev/null | grep -i nip47Repository: getAlby/hub
Length of output: 172
🏁 Script executed:
#!/bin/bash
# Look for NIP-47 handler/implementation to understand spec requirements
rg -n 'MakeInvoice' -g '**/nip47/**' --type go -B 3 -A 5Repository: getAlby/hub
Length of output: 4981
🏁 Script executed:
#!/bin/bash
# Check if there are any requests/callers that set descriptionHash without description
rg -n '"descriptionHash"' --type go -B 5 -A 2 | head -50Repository: getAlby/hub
Length of output: 3767
🏁 Script executed:
#!/bin/bash
# Look at the makeInvoiceParams structure to see field definitions
rg -n 'type makeInvoiceParams' --type go -A 10Repository: getAlby/hub
Length of output: 980
🏁 Script executed:
#!/bin/bash
# Check if phoenixd and other backends handle descriptionHash differently
rg -n 'descriptionHash' -g '**/lnclient/**' --type go -B 2 -A 2 | grep -E 'func|descriptionHash|if.*description'Repository: getAlby/hub
Length of output: 5902
🏁 Script executed:
#!/bin/bash
# Look for any documentation or spec references
find . -name "*.md" -o -name "*.txt" | xargs grep -l "NIP.47\|nip.47" 2>/dev/nullRepository: getAlby/hub
Length of output: 66
🏁 Script executed:
#!/bin/bash
# Check the README for NIP-47 spec reference
grep -n "NIP.47\|nip.47\|specification" README.md | head -20Repository: getAlby/hub
Length of output: 2154
🏁 Script executed:
#!/bin/bash
# Look at LDK implementation more closely to confirm it accepts descriptionHash without description
sed -n '772,790p' lnclient/ldk/ldk.goRepository: getAlby/hub
Length of output: 593
CLN's MakeInvoice requires description when descriptionHash is provided, breaking NIP-47 compatibility.
Lines 1446-1449 enforce that a description must be present if descriptionHash is provided. However, LND, Phoenixd, and LDK all accept descriptionHash without a description. NIP-47 treats both fields as independently optional in the makeInvoiceParams structure, meaning either can be provided alone. This difference may break callers that provide only descriptionHash per the spec.
🤖 Prompt for AI Agents
In `@lnclient/cln/cln.go` around lines 1434 - 1455, In CLNService.MakeInvoice,
remove the forced requirement that description must be present when
descriptionHash is provided and instead only validate the hash when both fields
are supplied: if descriptionHash != "" and description != "" then compute
myDescriptionHash and compare with descriptionHash (return error on mismatch);
set deschashonly to true when description is empty but descriptionHash is
present (i.e., description == "" && descriptionHash != ""); keep existing
behavior when both are present. This change touches the MakeInvoice function and
the variables description, descriptionHash, deschashonly, and myDescriptionHash.
There was a problem hiding this comment.
CLN's invoice method does not support just supplying a descriptionHash. You may only supply a description and a flag if you want the description or hash in the bolt11.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the CLN gRPC proto definition for Invoice request to understand the fields
rg -n 'message.*InvoiceRequest|message.*Invoice.*Request' lnclient/cln/clngrpc/ -A 20 --type proto 2>/dev/null || \
rg -n 'message.*InvoiceRequest|message.*Invoice.*Request' lnclient/cln/ -A 20
# Also check how the invoice request is being constructed in MakeInvoice
sed -n '1434,1490p' lnclient/cln/cln.go
Length of output: 1661
🏁 Script executed:
#!/bin/bash
# Find the exact MakeInvoice implementation
rg -n 'func.*MakeInvoice' lnclient/cln/cln.go -A 50 | head -80
# Look for the CLN invoice RPC call structure
rg -n 'c.client.Invoice' lnclient/cln/cln.go -B 5 -A 10
Length of output: 2420
@daywalker90, thank you for the clarification! You're absolutely right – I can see in the code that CLN's InvoiceRequest always requires the Description field (line 1533), and the deschashonly flag simply controls whether the bolt11 includes the description text or its hash.
The current validation logic (lines 1508-1517) is correct for CLN's API constraints. This is indeed a backend capability difference: while LND, LDK, and Phoenixd accept descriptionHash independently, CLN requires both the description and the flag to determine encoding.
I'm withdrawing my original suggestion – the implementation correctly handles CLN's API requirements. This is a documented limitation of the CLN backend relative to NIP-47's spec, where both fields are independently optional.
…different service
daywalker90
force-pushed
the
cln-backend
branch
from
bbe7f55 to
acf11bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lnclient/cln/cln.go`:
- Around line 1314-1323: The switch that sets created_at from decoded_invoice
currently dereferences InvoiceCreatedAt and CreatedAt without nil checks; update
the block in cln.go where decoded_invoice is handled (the switch on
decoded_invoice.ItemType that assigns to created_at) to first verify that
decoded_invoice.InvoiceCreatedAt and decoded_invoice.CreatedAt are non-nil
before dereferencing, returning a clear error (e.g., "invoice created_at
missing") if the expected timestamp pointer is nil, and only then cast to int64;
ensure both cases (clngrpc.DecodeResponse_BOLT12_INVOICE and
clngrpc.DecodeResponse_BOLT11_INVOICE) perform these nil checks.
- Around line 547-573: The error paths inside the nodeIds loop currently return
an empty string ("") which is the wrong type for the function's success return
(map[string]interface{}); update each error return (the ones after
hex.DecodeString, c.client.ListNodes, and both c.client.ListChannels calls) to
return nil along with the error (e.g., return nil, err) so the function
consistently returns a nil map on error; locate the returns in the nodeIds loop
that involve hex.DecodeString, ListnodesRequest/ListChannelsRequest and
variables listnodes/listchannels and replace the "" returns with nil.
🧹 Nitpick comments (6)
lnclient/cln/README.md (1)
29-45: Add a language identifier to the fenced code block.The directory tree block is missing a language specifier, which triggers a markdownlint warning (MD040). Use
textorplaintextas the language identifier.-``` +```text ~/dev/ ├── lightning/frontend/src/screens/setup/node/CLNForm.tsx (2)
48-56: Consider adding placeholder text to guide users on expected input format.The CLN address and lightning directory inputs lack placeholders. Other backend forms typically provide example values to reduce setup friction.
Suggested improvement
<Input required name="cln-address" onChange={(e) => setClnAddress(e.target.value)} value={clnAddress} id="cln-address" + placeholder="127.0.0.1:9737" />Similarly for the lightning directory:
<Input required name="cln-lightning-dir" onChange={(e) => setClnLightningDir(e.target.value)} value={clnLightningDir} type="text" id="cln-lightning-dir" + placeholder="/home/user/.lightning/bitcoin" />
33-39:handleSubmitis markedasyncbut contains noawaitexpressions.Minor nit — the
asynckeyword is unnecessary here and could be misleading.Suggested fix
- async function handleSubmit(data: object) { + function handleSubmit(data: object) {lnclient/cln/cln.go (3)
173-177: SetMinVersionon the TLS config to enforce TLS 1.3.CLN's gRPC server typically supports TLS 1.3. Without
MinVersion, Go defaults to TLS 1.2 for client connections. Explicitly pinning to TLS 1.3 hardens the connection.Proposed fix
return &tls.Config{ Certificates: []tls.Certificate{clientCert}, RootCAs: certPool, ServerName: serverName, + MinVersion: tls.VersionTLS13, }, nil
988-988: Regex patterns compiled on every call.
reChanHeight(line 988) andreStatus(line 1083) are compiled each timeListChannels/confirmationsRequiredFromStatusis invoked. Promoting them to package-levelvar(compiled once viaregexp.MustCompile) avoids repeated compilation overhead.Proposed fix
+var ( + reChanHeight = regexp.MustCompile(`(\d+)x.*`) + reStatus = regexp.MustCompile(`.*Funding needs (\d+) more confirmations to be ready.*`) +) + func (c *CLNService) ListChannels(ctx context.Context) (channels []lnclient.Channel, err error) { ... - reChanHeight := regexp.MustCompile(`(\d+)x.*`) ... } func confirmationsRequiredFromStatus(status []string) (uint32, *string) { - reStatus := regexp.MustCompile(`.*Funding needs (\d+) more confirmations to be ready.*`) ... }Also applies to: 1083-1083
1813-1814:SendPaymentSyncreturns a non-nilerralongside the success response.Line 1814 returns
errfrom theXpaycall, but if execution reaches this point,errisnil(the error was checked on line 1799). Returningerrhere is harmless but misleading—prefer an explicitnil.return &lnclient.PayInvoiceResponse{ Preimage: hex.EncodeToString(resp.PaymentPreimage), Fee: feePaid, - }, err + }, nil
|
Addressed the clanker issues. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@lnclient/cln/cln.go`:
- Around line 561-573: GetNetworkGraph is appending channels from separate
ListChannels calls and can produce duplicate channel entries when the same
channel appears in different queries; after collecting listchannels (the slice
populated from c.client.ListChannels calls using nodeIdBytes and the other
similar block at ~621-632), deduplicate by the channel's short channel id and
direction (use scid+direction as the unique key) before building the response.
Update the code that appends to listchannels (the blocks calling
c.client.ListChannels) to collect into a map keyed by scid+direction, then
rebuild listchannels from the map (and apply the same dedupe change to the other
similar block referenced) so duplicates are removed before further processing.
- Around line 173-177: The TLS config returned (the tls.Config constructed with
Certificates, RootCAs and ServerName) must explicitly set MinVersion to a modern
TLS constant; update the returned tls.Config in lnclient/cln/cln.go to include
MinVersion: tls.VersionTLS12 (or tls.VersionTLS13 if the CLN gRPC server
supports 1.3) alongside Certificates: []tls.Certificate{clientCert}, RootCAs:
certPool and ServerName: serverName so the connection enforces the minimum
protocol version.
- Around line 1003-1006: The code is shadowing the named return err with a local
variable err of type *string from confirmationsRequiredFromStatus, causing
logger.Logger.Error(err) to log a pointer address; fix by renaming the local
error variable (e.g., confErr or confRequiredErr) or otherwise avoid shadowing,
dereference the *string when logging and appending (use *confErr or
*confRequired), and ensure the append to errorStrings uses the string value
rather than the pointer; specifically update the block handling
confirmationsRequiredFromStatus so logger.Logger.Error receives the string value
and errorStrings gets the dereferenced string.
- Around line 1159-1242: ListPeers currently iterates resp.Channels (one per
channel) causing duplicate peers and N+1 ListNodes calls; fix by deduplicating
by peer ID (use a map keyed by string(peer.PeerId) or
hex.EncodeToString(peer.PeerId)) so you only process each peer once, cache
ListNodes results per peer ID to avoid repeated RPCs, and when building the
Address use the nil-safe getter selected.GetAddress() instead of dereferencing
*selected.Address; update the logic in the ListPeers function (references:
ListPeerChannels response resp.Channels, ListNodes RPC, selected variable, and
returned lnclient.PeerDetails) to implement these changes.
- Around line 180-217: subscribeOpenHoldInvoices currently returns immediately
on the first c.clientHold.List error (in subscribeOpenHoldInvoices), causing
missed subscriptions; instead wrap the List call in a retry loop with
exponential/backoff (or limited retries) that respects ctx cancellation, log
each failure with logger.Logger.WithError at error level and escalate if retries
exhausted, and only return after ctx.Done or after retries fail; ensure you
still collect all pages (start/lsr.Invoices) after successful List and that
subscribeSingleInvoice is launched for UNPAID invoices as before.
🧹 Nitpick comments (1)
lnclient/cln/cln.go (1)
988-988: Compile regexes at package level instead of per-call.
regexp.MustCompileat line 988 (and line 1083 inconfirmationsRequiredFromStatus) is called on every invocation ofListChannels. Move them to package-levelvardeclarations to avoid repeated compilation overhead.Proposed fix
+var ( + reChanHeight = regexp.MustCompile(`(\d+)x.*`) + reStatus = regexp.MustCompile(`.*Funding needs (\d+) more confirmations to be ready.*`) +) + func (c *CLNService) ListChannels(ctx context.Context) (channels []lnclient.Channel, err error) { ... - reChanHeight := regexp.MustCompile(`(\d+)x.*`) ...And in
confirmationsRequiredFromStatus:func confirmationsRequiredFromStatus(status []string) (uint32, *string) { - reStatus := regexp.MustCompile(`.*Funding needs (\d+) more confirmations to be ready.*`) ...
|
@daywalker90 great. We have a pending v1.21.5 hotfix but this will be a priority to get merged for v1.22. I hope @im-adithya and @frnandu will help review once v1.21.5 is released. |
2 participants
Hi alby team :)
I know the CLN API pretty well by now and i only had to get into
goandtypescript(it was fun) to get this one done.if err != nilbtw.This is barely tested. I'm pretty sure some functions were never called during my time clicking around in the web frontend. Please let me know how to test this in a good way.
Also please let me know what else i missed or got wrong.
While CLN ships with gRPC support, CLN doesn't have native support for hold invoices so i added optional configs for boltz's hold plugin.
Not tested but i think the minimum version for CLN is v24.11 and hold v0.3.3
Summary by CodeRabbit
New Features
UI
Documentation