feat: add CLN backend#2026
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Core Lightning (CLN) as a new LN backend across backend wiring, config/env, API, frontend UI/types, a full CLN gRPC client (with optional hold-plugin support and subscriptions), 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 (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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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:
For the full path I used |
|
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.
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
…different service
daywalker90
force-pushed
the
cln-backend
branch
from
bcfc82e to
bad8985
Compare
|
rebased.
This might be because of how Albyhub looks up and saves transactions in combination with the different ways a backend can implement lookup of transactions. CLN just recently (v26.04) shipped with ALL grpc notifications for streaming. I have not used those in this PR. I think lnd does that to notify albyhub of all node transactions. With this PR we only have the lookup rpc functions but albyhub doesn't do "lookup all" calls, only specific payment hashes for transactions it wants to track. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
lnclient/cln/cln.go (2)
1759-1761: Polish error message inSendKeysend.
"preimage not supported for keysends"reads awkwardly; consider"keysend with a user-supplied preimage is not supported by CLN"to match the actual reason and aid debugging on the client side.♻️ Proposed fix
if preimage != "" { - return nil, errors.New("preimage not supported for keysends") + return nil, errors.New("keysend with a user-supplied preimage is not supported by CLN") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lnclient/cln/cln.go` around lines 1759 - 1761, Update the error returned when a user-supplied preimage is detected in SendKeysend: replace the current message "preimage not supported for keysends" with a clearer phrase such as "keysend with a user-supplied preimage is not supported by CLN" so callers understand the specific reason; locate the check that tests preimage (the branch returning errors.New(...)) and swap the string accordingly.
1418-1418: Use%wfor error wrapping and lowercase error strings.Several
fmt.Errorfcalls use%vto interpolate the underlying error rather than wrapping with%w, which preventserrors.Is/errors.Asfrom working downstream. A few also begin with capital letters, which goes against Go's convention. Examples include lines 1418, 1454, 1494, 1504, 1524, 1534, 1554, 1558, 1589, 1596, and 1674.As per coding guidelines, "Wrap errors with fmt.Errorf("context: %w", err) for debugging".
♻️ Representative diffs
- return nil, fmt.Errorf("Invalid payment hash: %v", err) + return nil, fmt.Errorf("invalid payment hash: %w", err)- return nil, fmt.Errorf("Failed to make hold invoice: %v", err) + return nil, fmt.Errorf("failed to make hold invoice: %w", err)- return fmt.Errorf("Invalid preimage: %v", err) + return fmt.Errorf("invalid preimage: %w", err)- return nil, fmt.Errorf("Could not convert throughNodePubkey to bytes") + return nil, fmt.Errorf("could not convert throughNodePubkey to bytes: %w", err)- return nil, fmt.Errorf("Could not convert Pubkey to bytes") + return nil, fmt.Errorf("could not convert Pubkey to bytes: %w", err)As per coding guidelines, "Wrap errors with fmt.Errorf("context: %w", err) for debugging".
Also applies to: 1454-1454, 1494-1494, 1504-1504, 1524-1524, 1534-1534, 1554-1554, 1558-1558, 1589-1589, 1596-1596, 1674-1674
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lnclient/cln/cln.go` at line 1418, Replace raw fmt.Errorf calls that interpolate errors with %v and capitalized messages to use error wrapping and lowercase messages; e.g. change fmt.Errorf("Invalid payment hash: %v", err) to fmt.Errorf("invalid payment hash: %w", err). Apply the same change to the other occurrences that currently use fmt.Errorf(... "%v", err) (the reported instances with messages like "Invalid ...", etc.), ensuring each message starts lowercase and uses %w so errors.Is/errors.As will work downstream.lnclient/cln/README.md (1)
29-45: Minor: Inconsistent box-drawing characters and missing fenced code language.The directory tree mixes ASCII
|(lines 38-41) with Unicode├/│/└(lines 31-37, 42-44), which renders unevenly. Additionally, markdownlint flags the fenced block as missing a language specifier (MD040).♻️ Proposed fix
-``` +```text ~/dev/ ├── lightning/ │ └── cln-grpc/ │ └── proto/ │ ├── node.proto │ ├── primitives.proto │ └── ... ├── hub/ -| └── lnclient/ -| └── cln/ -| └── clngrpc/ -| └── clngrpc_hold/ +│ └── lnclient/ +│ └── cln/ +│ ├── clngrpc/ +│ └── clngrpc_hold/ └── hold/ └── protos/ └── hold.proto ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lnclient/cln/README.md` around lines 29 - 45, Update the fenced directory tree in lnclient/cln/README.md to use consistent box-drawing characters (replace the ASCII '|' lines with '│') and add a fenced-code language specifier (e.g., ```text) so markdownlint MD040 is satisfied; specifically edit the code block that shows the ~/dev/ tree (the block containing lightning/, hub/, hold/) to change the lines under hub/ to use '│ └── lnclient/' and the subsequent children to use '│' prefixes and ensure the top fence includes the language tag.frontend/src/screens/setup/node/CLNForm.tsx (2)
23-23: Address the TODO comment for proper onboarding.The
// TODO: proper onboardingindicates the user-facing setup flow is still placeholder-quality. Given this PR is targeting v1.22, it's worth tracking this explicitly so it isn't lost.Want me to open a follow-up issue to track the proper CLN onboarding flow (e.g., guided cert path discovery, validation, friendlier explanations)?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/screens/setup/node/CLNForm.tsx` at line 23, The inline comment "// TODO: proper onboarding" in the CLNForm component is a placeholder—create a tracked follow-up issue (e.g., GitHub issue) describing the full CLN onboarding work (guided cert path discovery, validation, friendlier UX) and update the comment in frontend/src/screens/setup/node/CLNForm.tsx to reference that issue ID and a short action item; also add a brief user-facing fallback note in the CLNForm render (or a TODO-marked helper function) so the code clearly documents current behavior and where the onboarding improvements will be implemented (refer to the CLNForm component name when making these changes).
2-2: Change import to usereact-routerfor consistency with the codebase.CLNForm.tsx is the only file in the frontend/src directory importing
useNavigatefromreact-router-dom; all other files (including LDKForm.tsx, CashuForm.tsx, PresetNodeForm.tsx, LNDForm.tsx, and PhoenixdForm.tsx in the same directory) import fromreact-router.♻️ Proposed fix
-import { useNavigate } from "react-router-dom"; +import { useNavigate } from "react-router";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/screens/setup/node/CLNForm.tsx` at line 2, Update the import of useNavigate in CLNForm.tsx to import from "react-router" instead of "react-router-dom" for consistency with the rest of the codebase; locate the import statement at the top of CLNForm.tsx that currently reads import { useNavigate } from "react-router-dom" and change the module source to "react-router" so symbols like useNavigate remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lnclient/cln/cln.go`:
- Around line 1207-1212: The error returned in the ListPeers function is a
copy/paste leftover referencing "listpeerchannels failed"; update the error to
reflect the actual call by changing the message in the fmt.Errorf when
c.client.ListPeers(...) fails (in the ListPeers method of CLNService) to a
correct, descriptive string such as "listpeers failed" or "ListPeers failed" so
the log matches the invoked method.
- Around line 47-150: The function NewCLNService has an err shadowing bug: inner
declarations like tlsConfigHold, err := ..., connHold, err = ..., and the
clientHold.List call update a new inner err so the deferred cleanup (which
checks the outer err) doesn't run on failures and connections leak; fix by
converting the function to use a named error return (e.g., result
lnclient.LNClient, err error) or by renaming inner errs (e.g., errHold) and
assigning the outer err before any return, ensure all error returns wrap with
fmt.Errorf("...: %w", err), and verify the defer block sees the non-nil err so
cancel(), conn.Close(), and connHold.Close() run on failures from tlsConfigHold,
connHold, or clientHold.List.
---
Nitpick comments:
In `@frontend/src/screens/setup/node/CLNForm.tsx`:
- Line 23: The inline comment "// TODO: proper onboarding" in the CLNForm
component is a placeholder—create a tracked follow-up issue (e.g., GitHub issue)
describing the full CLN onboarding work (guided cert path discovery, validation,
friendlier UX) and update the comment in
frontend/src/screens/setup/node/CLNForm.tsx to reference that issue ID and a
short action item; also add a brief user-facing fallback note in the CLNForm
render (or a TODO-marked helper function) so the code clearly documents current
behavior and where the onboarding improvements will be implemented (refer to the
CLNForm component name when making these changes).
- Line 2: Update the import of useNavigate in CLNForm.tsx to import from
"react-router" instead of "react-router-dom" for consistency with the rest of
the codebase; locate the import statement at the top of CLNForm.tsx that
currently reads import { useNavigate } from "react-router-dom" and change the
module source to "react-router" so symbols like useNavigate remain unchanged.
In `@lnclient/cln/cln.go`:
- Around line 1759-1761: Update the error returned when a user-supplied preimage
is detected in SendKeysend: replace the current message "preimage not supported
for keysends" with a clearer phrase such as "keysend with a user-supplied
preimage is not supported by CLN" so callers understand the specific reason;
locate the check that tests preimage (the branch returning errors.New(...)) and
swap the string accordingly.
- Line 1418: Replace raw fmt.Errorf calls that interpolate errors with %v and
capitalized messages to use error wrapping and lowercase messages; e.g. change
fmt.Errorf("Invalid payment hash: %v", err) to fmt.Errorf("invalid payment hash:
%w", err). Apply the same change to the other occurrences that currently use
fmt.Errorf(... "%v", err) (the reported instances with messages like "Invalid
...", etc.), ensuring each message starts lowercase and uses %w so
errors.Is/errors.As will work downstream.
In `@lnclient/cln/README.md`:
- Around line 29-45: Update the fenced directory tree in lnclient/cln/README.md
to use consistent box-drawing characters (replace the ASCII '|' lines with '│')
and add a fenced-code language specifier (e.g., ```text) so markdownlint MD040
is satisfied; specifically edit the code block that shows the ~/dev/ tree (the
block containing lightning/, hub/, hold/) to change the lines under hub/ to use
'│ └── lnclient/' and the subsequent children to use '│' prefixes and ensure
the top fence includes the language tag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7cd9c5b4-e068-45b3-8151-64b0fb1dae12
⛔ Files ignored due to path filters (6)
frontend/src/assets/images/node/cln.pngis excluded by!**/*.pnglnclient/cln/clngrpc/node.pb.gois excluded by!**/*.pb.golnclient/cln/clngrpc/node_grpc.pb.gois excluded by!**/*.pb.golnclient/cln/clngrpc/primitives.pb.gois excluded by!**/*.pb.golnclient/cln/clngrpc_hold/hold.pb.gois excluded by!**/*.pb.golnclient/cln/clngrpc_hold/hold_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
.env.exampleREADME.mdapi/api.goapi/models.goconfig/config.goconfig/models.gofrontend/src/lib/backendType.tsfrontend/src/routes.tsxfrontend/src/screens/setup/SetupNode.tsxfrontend/src/screens/setup/SetupSecurity.tsxfrontend/src/screens/setup/node/CLNForm.tsxfrontend/src/types.tslnclient/cln/README.mdlnclient/cln/cln.go
✅ Files skipped from review due to trivial changes (5)
- frontend/src/screens/setup/SetupSecurity.tsx
- frontend/src/lib/backendType.ts
- .env.example
- api/models.go
- frontend/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- api/api.go
4 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