staticaddr: dynamic deposit conf target#1097
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 enhances the static address loop-in client to gracefully handle dynamic confirmation requirements imposed by the server. By reducing the client's default minimum confirmation threshold and intelligently prioritizing deposits with higher confirmation counts, the system can now facilitate quicker and more reliable loop-in swaps, especially for smaller transactions. Additionally, improved error logging provides users with actionable insights when deposits require more confirmations. 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
Activity
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
The pull request introduces dynamic deposit confirmation targets, which is a valuable improvement for optimizing swap attempts. The changes include reducing the minimum confirmations, prioritizing more-confirmed deposits, and adding client-side logging for insufficient confirmation errors. New protobuf messages have been added to support structured error responses. The overall direction of these changes is positive for improving the flexibility and user feedback of the static address loop-in client.
However, there is an inconsistency in the SelectDeposits function's sorting logic. While the comments indicate three sorting priorities (more confirmations, larger amounts, and expiring sooner), the implemented code only accounts for the first two. The 'expiring sooner first' logic, which was present in the previous version as a tie-breaker, appears to have been removed from the current implementation. This could lead to suboptimal selection of deposits, especially for time-sensitive ones.
hieblmi
force-pushed
the
dyn-deposit-confs
branch
4 times, most recently
from
203c29a to
8321cc2
Compare
|
@claude review this |
Add DepositConfirmationStatus and InsufficientConfirmationsDetails messages to support structured error responses when deposits lack sufficient confirmations for dynamic risk requirements. (cherry picked from commit 69a0798)
Reduce MinConfs from 6 to 3 to allow faster swap attempts while the server enforces risk-based confirmation requirements. Update SelectDeposits to prioritize more-confirmed deposits first, increasing the likelihood of server acceptance. Add client-side logging of insufficient confirmation details from server error responses. (cherry picked from commit 66a17f4)
hieblmi
force-pushed
the
dyn-deposit-confs
branch
from
8321cc2 to
d22be64
Compare
| // deposit to be considered available for loop-ins, coop-spends and | ||
| // timeouts. | ||
| MinConfs = 6 | ||
| MinConfs = 3 |
There was a problem hiding this comment.
We need to hold this PR until we deploy the server side. Otherwise a new client might send its deposits too early to an old server.
| // SelectDeposits sorts the deposits to optimize for successful swaps with | ||
| // dynamic confirmation requirements: 1) more confirmations first (higher chance | ||
| // of server acceptance), 2) larger amounts first (to minimize number of deposits | ||
| // used), 3) expiring sooner first (prioritize time-sensitive deposits). |
There was a problem hiding this comment.
SelectDeposits can now turn a valid amount-only swap into a more expensive or failing one because it switched to a confirmations-first greedy prefix instead of actually optimizing the chosen subset. The new logic sorts primarily by confirmations and then keeps taking the first deposits until the target is covered in staticaddr/loopin/manager.go:853-930.
That means a set like 500k@10 conf, 500k@9 conf, 1,000k@8 conf for a 1,000k target now selects two inputs instead of the exact one-input match. This is not just a UX issue: the chosen input count feeds the quote path in staticaddr/loopin/manager.go:729-742, and the later HTLC fee guards scale with f.loopIn.htlcWeight(hasChange) in staticaddr/loopin/actions.go:221-270. So the new order can produce avoidable ErrSwapFeeTooHigh or ErrFeeTooHigh failures, or simply worse fees, even when a slightly younger single deposit would already have been acceptable to the server.
If the client wants to prefer older deposits, they need a subset selection strategy that still minimizes input count and fee impact, or at least a second pass that prefers exact or lower-input covers before falling back to the confirmations-first prefix.
| if err != nil { | ||
| // Check if this is an insufficient confirmations error and log | ||
| // the details to help the user understand what's needed. | ||
| logInsufficientConfirmationsDetails(err) |
There was a problem hiding this comment.
The new normal insufficient-confirmation path leaks an invoice. InitHtlcAction creates the swap invoice before it calls ServerStaticAddressLoopIn in staticaddr/loopin/actions.go:109-148. If the server rejects the request, the new branch only logs the structured details and returns HandleError in staticaddr/loopin/actions.go:149-157.
The only invoice cancel path I found is much later in the payment-timeout monitor in staticaddr/loopin/actions.go:558-569, which is never reached for this early failure.
Early ServerStaticAddressLoopIn failures, and the follow-on fee-validation failures in staticaddr/loopin/actions.go:231-270, should cancel the freshly created invoice before unlocking the deposits.
4 participants
Update static address loop-in client to support dynamic server-side confirmation requirements.
Changes:
Proto updates:
Backwards compatible: Clients with older servers will continue to work normally. The server determines actual confirmation requirements.