cli/swaps: fast fail swap cli on changed fees - #10840
Conversation
| raise UserFacingException( | ||
| _("Swap fees have changed since the dryrun was calculated. Do a new dryrun first.") | ||
| + f" ({required_onchain_amount_sat} != {onchain_amount_sat} sat)") |
There was a problem hiding this comment.
Line 433 in a357e86
(though we maybe should re-state that at the top of commands.py)
There was a problem hiding this comment.
Removed.
I added a commit 4368018 which breaks the _() function call in commands.py to enforce this programmatically.
SomberNight
left a comment
SomberNight
left a comment
There was a problem hiding this comment.
52a961f is mostly ok. some nits.
| required_onchain_amount_sat = sm.get_send_amount(lightning_amount_sat, is_reverse=False) | ||
| # same 1 sat rounding tolerance as in `request_normal_swap()` | ||
| if not required_onchain_amount_sat \ | ||
| or not onchain_amount_sat - 1 <= required_onchain_amount_sat <= onchain_amount_sat: |
There was a problem hiding this comment.
Can we use parens here? There is so much operator precedence stuff going on in this if, it's hard to imagine the AST in my head.
| or not onchain_amount_sat - 1 <= required_onchain_amount_sat <= onchain_amount_sat: | |
| or not (onchain_amount_sat - 1 <= required_onchain_amount_sat <= onchain_amount_sat): |
it's a small nit, but I also mean in general in the codebase :)
Too many layers of parens makes it hard to read but too few makes it easy to make a mistake in operator precedence. Delicate tradeoff :P
| funding_utxo_value_sat = requested_recv_amount_sat + claim_fee | ||
| onchain_recv_amount_sat = sm.get_recv_amount(lightning_amount_sat, is_reverse=True) | ||
| if not onchain_recv_amount_sat or onchain_recv_amount_sat < requested_recv_amount_sat: | ||
| raise UserFacingException( | ||
| _("Swap fees have changed since the dryrun was calculated. Do a new dryrun first.") | ||
| + f" ({onchain_recv_amount_sat} < {requested_recv_amount_sat} sat)") |
There was a problem hiding this comment.
Do I understand correctly that this is the "same" check as in reverse_swap()?
electrum/electrum/submarine_swaps.py
Lines 1296 to 1299 in a357e86
Except there in sswaps.py, both amounts being compared are offset by +claim_fee ?
Uff took me like 10 minutes to untangle that >.<
There was a problem hiding this comment.
Yes it's a similar check, it just re-calculates the amount with the latest known fees client side to reduce the time window for disagreements and potentially prevent a useless request to the swap provider as the other check only happens after we got the servers response.
CLI swaps happen in two steps. First the command is ran as "dryrun" to calculate the expected amounts (considering the feerates), then the swap is initiated with these amounts. If the mining fee estimates increase during the time between those two calls the swap provider might return a lower onchain amount for us, causing the `if onchain_amount < expected_onchain_amount_sat` check in `SwapManager` to fail the swap after a the network round trip with the swap provider. Instead we should fail early before the swap is registered with the provider. Note: the issue can still happen if the fees change in the brief moment during the request, so scripts consuming the CLI should handle failures either way.
The CLI is intentionally not translated, break the `_()` import to prevent accidental usage.
52a961f to
4368018
Compare
2 participants
CLI swaps happen in two steps.
First the command is ran as "dryrun" to calculate the expected amounts, then the swap is initiated with these amounts.
If the mining fee estimates increase during the time between those two calls, the swap provider might return an unexpected amount for us in their response, causing the response checks in
SwapManagerto fail a swap after a the network round trip with the swap provider.Instead we should fail early before the swap is registered with the provider, to avoid stale swaps in the servers db and give the CLI user a clear exception.
Note: the issue can still happen if the fees change in the brief moment during the request, so scripts consuming the CLI should handle a failure either way.
Also fixes a bug in that the
onchain_amountreturned by the swap branch of thereverse_swapcommand was inconsistent with the value returned by thedryrunbranch (it included theclaim_fee).Closes #10826