staticaddr: dynamic deposit conf target#1097
staticaddr: dynamic deposit conf target#1097hieblmi wants to merge 2 commits intolightninglabs:masterfrom
Conversation
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)
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.
| sort.Slice(deposits, func(i, j int) bool { | ||
| if deposits[i].Value == deposits[j].Value { | ||
| iExp := uint32(deposits[i].ConfirmationHeight) + | ||
| csvExpiry - blockHeight | ||
| jExp := uint32(deposits[j].ConfirmationHeight) + | ||
| csvExpiry - blockHeight | ||
|
|
||
| return iExp < jExp | ||
| // Primary: more confirmations first. Guard against the | ||
| // theoretical case where ConfirmationHeight > blockHeight | ||
| // (e.g. during a transient reorg inconsistency). | ||
| var iConfs, jConfs uint32 | ||
| if blockHeight > uint32(deposits[i].ConfirmationHeight) { | ||
| iConfs = blockHeight - | ||
| uint32(deposits[i].ConfirmationHeight) | ||
| } | ||
| if blockHeight > uint32(deposits[j].ConfirmationHeight) { | ||
| jConfs = blockHeight - | ||
| uint32(deposits[j].ConfirmationHeight) | ||
| } | ||
| if iConfs != jConfs { | ||
| return iConfs > jConfs | ||
| } | ||
|
|
||
| // Secondary: larger amounts first. | ||
| return deposits[i].Value > deposits[j].Value | ||
| }) |
There was a problem hiding this comment.
The updated comment for SelectDeposits (lines 853-859) and the inline comment here (lines 880-883) state three sorting priorities:
- More confirmations first.
- Larger amounts first.
- Expiring sooner first.
However, the sort.Slice implementation only includes the first two priorities. The third priority, "expiring sooner first," which was previously a tie-breaker for deposits with equal values, is no longer present in the sorting logic. This discrepancy between the documented behavior and the actual implementation could lead to suboptimal deposit selection, particularly for deposits that are time-sensitive but have similar confirmation counts and values.
Consider re-introducing the expiry-based tie-breaker or updating the comments to accurately reflect the implemented sorting logic.
sort.Slice(deposits, func(i, j int) bool {
// Primary: more confirmations first. Guard against the
// theoretical case where ConfirmationHeight > blockHeight
// (e.g. during a transient reorg inconsistency).
var iConfs, jConfs uint32
if blockHeight > uint32(deposits[i].ConfirmationHeight) {
iConfs = blockHeight -
uint32(deposits[i].ConfirmationHeight)
}
if blockHeight > uint32(deposits[j].ConfirmationHeight) {
jConfs = blockHeight -
uint32(deposits[j].ConfirmationHeight)
}
if iConfs != jConfs {
return iConfs > jConfs
}
// Secondary: larger amounts first.
if deposits[i].Value != deposits[j].Value {
return deposits[i].Value > deposits[j].Value
}
// Tertiary: expiring sooner first.
iExp := uint32(deposits[i].ConfirmationHeight) +
csvExpiry - blockHeight
jExp := uint32(deposits[j].ConfirmationHeight) +
csvExpiry - blockHeight
return iExp < jExp
})| // Sorting priority: 1) more confirmations first, 2) larger amounts first, | ||
| // 3) expiring sooner first. |
There was a problem hiding this comment.
The comment here states "3) expiring sooner first" as a sorting priority, but the SelectDeposits function's implementation (in staticaddr/loopin/manager.go) does not include this logic. This creates an inconsistency between the test description and the actual code behavior. Please update the comment to accurately reflect the implemented sorting priorities, or adjust the SelectDeposits function to include this priority.
1 participant
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.