chore: remove json tags from lnclient models#2375
Open
rolznz wants to merge 3 commits into
Open
Conversation
these should not be passed through the API directly
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR decouples the API layer from internal lnclient types by introducing local API model definitions, removing the CloseChannelResponse type, standardizing balance field naming to msat/sat variants, and adding explicit type conversion functions. All endpoint implementations now translate between API and lnclient types explicitly. ChangesAPI Abstraction and Type Conversion
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
api/api.go (1)
842-844: ⚡ Quick winWrap the newly propagated LN client errors with endpoint context.
These adapters add a new API layer, so returning raw backend errors makes it harder to tell which call failed once the same
lnclienterror bubbles up through multiple handlers.As per coding guidelines
**/*.go: Wrap errors withfmt.Errorf("context: %w", err)for debugging.Also applies to: 1143-1145, 1165-1167, 1867-1869
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/api.go` around lines 842 - 844, The lnclient calls (e.g., lnClient.GetNodeConnectionInfo(ctx)) are returning raw backend errors; update each to wrap the error with endpoint context using fmt.Errorf("GetNodeConnectionInfo: %w", err) (and analogous messages for the other sites mentioned) before returning so callers can distinguish which API adapter failed; ensure you import fmt if not present and apply the same wrapping pattern at the other call sites referenced (the calls around the locations noted: the GetNodeConnectionInfo invocation and the two other lnClient call sites).lnclient/ldk/ldk.go (1)
1096-1111: ⚡ Quick winWrap close failures with branch-specific context.
The new
error-only contract makes barereturn errharder to diagnose upstream. Wrap the cooperative and force-close failures separately so callers can tell which path failed.As per coding guidelines,
**/*.go: Wrap errors with fmt.Errorf("context: %w", err) for debugging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lnclient/ldk/ldk.go` around lines 1096 - 1111, In LDKService.CloseChannel, when calling ls.node.ForceCloseChannel and ls.node.CloseChannel, wrap returned errors with fmt.Errorf to add branch-specific context before returning (e.g., use fmt.Errorf("force close failed: %w", err) when closeChannelRequest.Force is true and fmt.Errorf("cooperative close failed: %w", err) for the other branch); locate the error handling in CloseChannel, wrap the err from ls.node.ForceCloseChannel and ls.node.CloseChannel accordingly, and return the wrapped error so callers can distinguish which close path failed.lnclient/lnd/lnd.go (1)
1172-1228: ⚡ Quick winAdd stage-specific context to
CloseChannelerrors.This now returns bare errors from the list, parse, close-stream, receive, and txid-decoding steps. Once the response payload is gone, those failures are much harder to distinguish upstream unless each return is wrapped with the failing stage.
As per coding guidelines,
**/*.go: Wrap errors with fmt.Errorf("context: %w", err) for debugging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lnclient/lnd/lnd.go` around lines 1172 - 1228, In LNDService.CloseChannel, wrap returned errors with stage-specific context using fmt.Errorf so callers can distinguish where failures occurred: when calling svc.client.ListChannels (wrap as "list channels: %w"), when parsing channel point via svc.parseChannelPoint ("parse channel point: %w"), when initiating close with svc.client.CloseChannel ("initiate close channel: %w"), when receiving from the stream (stream.Recv => "receive close update: %w"), and when decoding the closing txid via chainhash.NewHash ("decode closing txid: %w"); update each return err to return fmt.Errorf("<stage description>: %w", err) referencing the corresponding function/variable names (LNDService.CloseChannel, svc.client.ListChannels, svc.parseChannelPoint, svc.client.CloseChannel, stream.Recv, chainhash.NewHash).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/api.go`:
- Around line 1181-1190: The ConnectPeer handler forwards unchecked user input
to lnclient; add validation at the API boundary in func ConnectPeer (and
similarly for the other handlers noted) to reject invalid requests before
calling lnClient: check api.svc.GetLNClient() as already done, then validate
connectPeerRequest.Pubkey is non-empty, Address is non-empty (and optionally
well-formed), and Port is >0; return a clear API error (e.g., ErrInvalidRequest
or a new validation error) if any check fails, and only then construct
lnclient.ConnectPeerRequest and call lnClient.ConnectPeer; repeat this pattern
for the other request structs (e.g., where ConnectPeerRequest-like types and
AmountSats/Port/Pubkey fields are passed into lnclient) so all user inputs are
validated at the API boundary.
In `@api/models.go`:
- Around line 395-398: The public API structs currently expose backend-specific
types via interface{} fields (e.g., NodeStatus.InternalNodeStatus,
InternalBalances, NetworkGraphResponse) which can leak lnclient serialization
details; replace each interface{} with a concrete, API-local DTO that models
only the fields the API should expose or, if truly dynamic, change the type to
map[string]any and document the shape; update any (un)marshalling and
constructors/transformers that populate these fields to convert from the
lnclient types into the new DTOs (or map) so the API boundary no longer depends
on lnclient concrete types.
In `@lnclient/cashu/cashu.go`:
- Around line 181-182: The CloseChannel implementation in CashuService currently
returns nil which incorrectly signals success; update the
CashuService.CloseChannel(ctx context.Context, closeChannelRequest
*lnclient.CloseChannelRequest) method to return the appropriate unsupported
error (return errors.ErrUnsupported) so callers receive a clear failure; ensure
the errors package that defines ErrUnsupported is imported and used in that
function.
In `@lnclient/models.go`:
- Around line 8-12: TLVRecord is missing json tags so marshaling produces
"Type"/"Value" fields and breaks consumers; add struct tags on
lnclient.TLVRecord (e.g., `Type uint64 `json:"type"``, `Value string
`json:"value"``) so when transactions/transactions_service.go places
[]lnclient.TLVRecord into metadata["tlv_records"] and json.Marshal is called,
the output uses the expected lower-case keys; ensure this change preserves
compatibility with LDK/LND producers and any code that unmarshals
transaction.Metadata in nip47/models/transactions.go.
In `@lnclient/phoenixd/phoenixd.go`:
- Around line 393-394: The CloseChannel method currently returns nil which
incorrectly signals success; update PhoenixService.CloseChannel(ctx,
closeChannelRequest *lnclient.CloseChannelRequest) to return a proper
"unsupported" error (e.g., errors.New or a wrapped fmt.Errorf) indicating that
Phoenixd does not implement channel closing so callers get a clear failure.
Ensure the error is returned directly from CloseChannel and include the function
name in the message for clarity.
---
Nitpick comments:
In `@api/api.go`:
- Around line 842-844: The lnclient calls (e.g.,
lnClient.GetNodeConnectionInfo(ctx)) are returning raw backend errors; update
each to wrap the error with endpoint context using
fmt.Errorf("GetNodeConnectionInfo: %w", err) (and analogous messages for the
other sites mentioned) before returning so callers can distinguish which API
adapter failed; ensure you import fmt if not present and apply the same wrapping
pattern at the other call sites referenced (the calls around the locations
noted: the GetNodeConnectionInfo invocation and the two other lnClient call
sites).
In `@lnclient/ldk/ldk.go`:
- Around line 1096-1111: In LDKService.CloseChannel, when calling
ls.node.ForceCloseChannel and ls.node.CloseChannel, wrap returned errors with
fmt.Errorf to add branch-specific context before returning (e.g., use
fmt.Errorf("force close failed: %w", err) when closeChannelRequest.Force is true
and fmt.Errorf("cooperative close failed: %w", err) for the other branch);
locate the error handling in CloseChannel, wrap the err from
ls.node.ForceCloseChannel and ls.node.CloseChannel accordingly, and return the
wrapped error so callers can distinguish which close path failed.
In `@lnclient/lnd/lnd.go`:
- Around line 1172-1228: In LNDService.CloseChannel, wrap returned errors with
stage-specific context using fmt.Errorf so callers can distinguish where
failures occurred: when calling svc.client.ListChannels (wrap as "list channels:
%w"), when parsing channel point via svc.parseChannelPoint ("parse channel
point: %w"), when initiating close with svc.client.CloseChannel ("initiate close
channel: %w"), when receiving from the stream (stream.Recv => "receive close
update: %w"), and when decoding the closing txid via chainhash.NewHash ("decode
closing txid: %w"); update each return err to return fmt.Errorf("<stage
description>: %w", err) referencing the corresponding function/variable names
(LNDService.CloseChannel, svc.client.ListChannels, svc.parseChannelPoint,
svc.client.CloseChannel, stream.Recv, chainhash.NewHash).
🪄 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: 1dbbed8f-9602-45d5-98cf-8351e35b861e
📒 Files selected for processing (13)
api/api.goapi/models.golnclient/cashu/cashu.golnclient/cln/cln.golnclient/ldk/ldk.golnclient/lnd/lnd.golnclient/models.golnclient/phoenixd/phoenixd.gonip47/controllers/get_balance_controller.gonip47/controllers/pay_keysend_controller.goswaps/swaps_service.gotests/mock_ln_client.gotests/mocks/LNClient.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
lnclient models should not be passed through the API directly
Follow up from #2373
Summary by CodeRabbit
Refactor
Tests