Conversation
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 delivers a collection of minor but important fixes across various components of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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. Changelog
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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 introduces a series of fixes across the staticaddr functionality. The changes include correcting comments and log messages, fixing data races and potential nil pointer dereferences, and adding missing error handling. The logic for handling context cancellation during cleanup has also been improved. Overall, the changes enhance the robustness and correctness of the code. I've found one minor issue with a log message that was not updated along with the code logic.
hieblmi
force-pushed
the
static-fixes
branch
from
e77d326 to
8435549
Compare
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
LGTM! 🚀
Great work! I left few comments.
hieblmi
force-pushed
the
static-fixes
branch
3 times, most recently
from
cb93650 to
b1f944f
Compare
hieblmi
force-pushed
the
static-fixes
branch
4 times, most recently
from
601e008 to
b6d86f1
Compare
hieblmi
force-pushed
the
static-fixes
branch
6 times, most recently
from
93f6cd8 to
1471df6
Compare
…ex 0 When createHtlcTx produces a transaction with a change output at index 0, createHtlcSweepTx correctly determines htlcInputIndex=1 to identify the HTLC output, but then unconditionally reads TxOut[0].Value for htlcOutValue. This uses the change amount instead of the HTLC amount, producing an incorrect sweep transaction with wrong fee calculation. Use TxOut[htlcInputIndex].Value so the sweep always references the correct HTLC output.
SweepHtlcTimeoutAction used a select with a default case containing a blocking time.After. When the context was canceled, it logged the error but continued the retry loop instead of returning. The default case also meant that ctx.Done was only checked when it was already signaled, while an hour-long sleep blocked without listening for cancellation. Replace the default+time.After pattern with a proper select on both ctx.Done and time.After so the function exits promptly on shutdown.
recoverLoopIns wrote to the activeLoopIns map from inside a goroutine without any synchronization. The map is shared state accessed from the manager's main Run loop, creating a data race. Move the map write before the goroutine spawn so it happens synchronously during recovery. Also fix the stale log message that said "OnStart" instead of "OnRecover".
handleWithdrawal did not check the error returned by RegisterSpendNtfn before spawning a goroutine to read from the notification channels. If registration failed, spentChan would be nil and the goroutine would block forever on a nil channel read. Add the missing error check so the function returns early on registration failure.
The %w formatting verb is only meaningful for fmt.Errorf where it enables error wrapping. In log.Errorf it prints as %%!w(error=...), producing garbled log output. Use %v and add the missing colon separator.
In manager.go, deferred shim cleanup was calling FundingStateStep with the original request ctx. If the user had already canceled that context, the cleanup RPC would run with a canceled context and could fail to remove the pending shim. I changed that cleanup path to use context.WithoutCancel(ctx) so the cancellation RPC still has a live context.
Remove unused errChan fields from the loopin, openchannel, and withdraw managers. These channels were declared and initialized but never read from or written to. Remove the unused activeLoopIns map from the loopin manager. The map was only written to but never read, making it dead code. Remove the stale withdraw.Store interface whose method signatures no longer match the concrete SqlStore API used by the manager. Remove unused config fields from openchannel.Config (Server, AddressManager, ChainNotifier, Signer) and deposit.ManagerConfig (AddressClient, SwapClient, ChainParams) along with their daemon wiring. Also remove the now-orphaned openchannel.AddressManager interface. Remove the unused GetStaticAddress and Close methods from address.SqlStore and the GetStaticAddress method from the address.Store interface, as the codebase only uses GetAllStaticAddresses.
`ReleaseRoutingPlugin` called `Done` with the caller context. In the loop-out payment flow this cleanup is deferred, so the context may already be canceled by the time teardown runs. That can prevent mission-control restoration in `Done`, leaving plugin-induced routing state behind after a swap exits. Fix this by detaching cancellation in `ReleaseRoutingPlugin` and passing `context.WithoutCancel(ctx)` to `Done`, so teardown still executes during caller cancellation. Also add a regression test that cancels the caller context before release and asserts the plugin `Done` call still receives a live context.
The probe monitor goroutine cancelled the probe invoice with the same monitoring context used by awaitProbe. The caller cancels that context as soon as swap initiation returns. If cancellation races with or precedes the invoice-cancel RPC, the cleanup can be skipped and the probe invoice may remain open longer than intended. Fix by calling CancelInvoice with context.WithoutCancel(ctx) so cleanup is not aborted by caller cancellation. Add a regression test that cancels the parent context between probe success and invoice cleanup, and verifies CancelInvoice still receives a live context.
Send the batch confirmation after AssertRegisterConf so the batch exits cleanly, and use a cancellable context so the batcher stops before leaktest runs.
hieblmi
force-pushed
the
static-fixes
branch
3 times, most recently
from
f88770c to
67bcb25
Compare
Also bump cellbuf, colorprofile, and term to versions compatible with the new ansi API.
2 participants
A
smallcollection of fixes for issues that agents identified.