-
Notifications
You must be signed in to change notification settings - Fork 1
fix: handle dust change in transfers and sends #416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
b22724f to
fa57404
Compare
|
@ben-kaufman should this fix #200? |
|
I actually couldn't reproduce #200 |
|
- Use two-pass fee estimation in SpendingAmount to ensure LSP fee calculation matches actual order creation (fixes off-by-few-sats errors) - Detect when change would be dust in SpendingConfirm and use sendAll to avoid creating unspendable outputs - Add isMaxAmount parameter to payOrder to properly handle sendAll case - Validate max sendable amount hasn't changed due to fee rate changes
fa57404 to
dbfa89d
Compare
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. Note: The PR introduces Bitcoin/Lightning operations in view files, which technically violates CLAUDE.md's guideline ("All Bitcoin/Lightning operations belong in the service layer, never in views"). However, this pattern is consistent with 35+ existing files in the codebase, and addressing it would require an architectural refactoring beyond the scope of this PR. |
|
The exact case from #200 is still failing for me here giving "Coin selection error". Interestingly it passes on Android.
Note the |
piotr-iohk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving since #200 issue was not originally intended to be fixed here, can be addressed separately.
Description
This PR fixes accurate full-balance sending for Blocktank channel purchases:
Technical Details
The core issue was that
lspBalancevalues depend onclientBalanceSat, so fee estimation withmaxSendableproduced different fees than actual order creation withclientBalance. The two-pass estimation now:maxSendableto get approximateclientBalancelspBalanceusingapproxClientBalanceto match what order creation will useLinked Issues/Tasks
Fixes MAX transfer to spending failing with insufficient funds error when calculated change would be dust.
How to Test
MAX Transfer to Spending
Near-MAX Normal Send