fix dead error variable and silent invoice cancel in setStateAbandoned#1177
fix dead error variable and silent invoice cancel in setStateAbandoned#1177chandachewe10 wants to merge 2 commits into
Conversation
…Abandoned The err variable is always nil when the final fmt.Errorf is reached (a non-nil err exits earlier), producing a trailing ', <nil>' in the error string visible to callers and in logs. Additionally, CancelInvoice errors were silently swallowed with a bare '_' assignment. The timeout path in the same file correctly checks for ErrInvoiceAlreadySettled; this commit makes the abandon path consistent: ignore already-settled invoices and log any other unexpected error so operators can diagnose issues without failing the abandon itself. Co-authored-by: Chanda Chewe <chandachewe10@users.noreply.github.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the robustness and clarity of the swap abandonment process in loopin.go. It ensures that invoice cancellation failures are handled consistently with other parts of the system and cleans up the returned error output to remove unnecessary noise. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the setStateAbandoned function in loopin.go to properly handle and log errors when canceling an invoice, and cleans up the returned error message. The reviewer recommends scoping the err variable directly within the if statement block to avoid potential issues with reusing function-scoped variables.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
hieblmi
left a comment
hieblmi
left a comment
There was a problem hiding this comment.
Hi @chandachewe10, thank you for the fix!
This LGTM! 💾
lnd returns ErrInvoiceAlreadySettled from its RPC handler as an uncoded gRPC error. On the client it is reconstructed as an Unknown status, so comparing it directly with the Go sentinel never matches. This made the abandon path log a warning for an expected condition. The older timeout path had the same comparison and could return the RPC error instead of finishing normal timeout processing. Add a shared classifier that accepts the local sentinel and the exact gRPC status representation, and use it in both cancellation paths. Add coverage for the classifier and timeout handling.
|
I rebased it and fix error classification in another commit. lnd returns ErrInvoiceAlreadySettled from its RPC handler as an uncoded gRPC error. On the client it is reconstructed as an Unknown status, so comparing it directly with the Go sentinel never matches. I verified empirically that previous version still produced a warning in this situation, while the fixed one correctly filters out this error. |
3 participants
Summary
This change fixes two issues in
setStateAbandonedinloopin.go.1. Removed a misleading
<nil>from the returned errorAfter
persistAndAnnounceStatesucceeds, execution can only reach the final return iferrisnil, since any non-nil error returns earlier. The previous implementation still includederrin the formatted error:As a result, the returned message always ended with
, <nil>, adding unnecessary noise to client responses and logs. The error message now only includes the relevant swap information.2. Handle
CancelInvoiceerrors consistentlyPreviously, the result of
CancelInvoicewas ignored, causing unexpected failures to go unnoticed.The abandon path now follows the same behavior as the timeout handling:
ErrInvoiceAlreadySettled, since the invoice may already have been settled before the swap was abandoned.Warnlevel so operators can diagnose the issue without preventing the abandon process from completing.Testing
go build ./...go vet ./...go test -run TestLoopIn ./...