Skip to content

qml: let user finalize forward swap onchain tx before initiating swap #9992

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

accumulator
Copy link
Member

No description provided.

@accumulator accumulator force-pushed the qml_forward_swap_user_tx_finalize branch 2 times, most recently from 8595781 to 44a9a1f Compare June 27, 2025 15:14
@accumulator accumulator marked this pull request as ready for review July 15, 2025 10:33
@accumulator accumulator force-pushed the qml_forward_swap_user_tx_finalize branch from e525c3a to 0fe131b Compare July 15, 2025 10:34
@f321x
Copy link
Member

f321x commented Jul 17, 2025

Tested, works fine for me. Maybe the minimal allowed tx fee should be limited in the tx finalizer to an ETA limit < LOCKTIME_DELTA_REFUND (+ buffer), otherwise it seems easy to lose money by cheaping out on the tx fees.
E.g. the user selects 1 sat/vb for a non urgent swap in the hope it will confirm in the next weeks (or months) during a large mempool, not knowing there are time limitations to swaps. Then he will have to broadcast a refund tx later and waste fees for nothing (ignoring the case of not opening the wallet at all, as we show a warning for this).

Also noticed these two qml logs while testing:
After confirming the swap, while receiving the htlcs:
331.59 | W | gui.qml.qeapp | file:///home/user/code/electrum-fork/electrum/gui/qml/components/Channels.qml:124:13: QML FlatButton: Binding loop detected for property "enabled"

When clicking on the input/output dropdown in the tx finalization dialog:
147.06 | W | gui.qml.qeapp | Qt Quick Layouts: Detected recursive rearrange. Aborting after two iterations.
This seems to be an issue with the this dialog, also happens when i do a regular onchain tx.

I can also open separate issues if they are out of scope of this PR.

@accumulator accumulator self-assigned this Jul 21, 2025
@accumulator
Copy link
Member Author

accumulator commented Jul 21, 2025

Tested, works fine for me. Maybe the minimal allowed tx fee should be limited in the tx finalizer to an ETA limit < LOCKTIME_DELTA_REFUND (+ buffer), otherwise it seems easy to lose money by cheaping out on the tx fees. E.g. the user selects 1 sat/vb for a non urgent swap in the hope it will confirm in the next weeks (or months) during a large mempool, not knowing there are time limitations to swaps. Then he will have to broadcast a refund tx later and waste fees for nothing (ignoring the case of not opening the wallet at all, as we show a warning for this).

Yes, good idea. I'm leaning towards disabling static fees in these cases, only allowing finalizing when dynamic fees (ETA, mempool) are available. What do you think @f321x?

Also noticed these two qml logs while testing: After confirming the swap, while receiving the htlcs: 331.59 | W | gui.qml.qeapp | file:///home/user/code/electrum-fork/electrum/gui/qml/components/Channels.qml:124:13: QML FlatButton: Binding loop detected for property "enabled"

When clicking on the input/output dropdown in the tx finalization dialog: 147.06 | W | gui.qml.qeapp | Qt Quick Layouts: Detected recursive rearrange. Aborting after two iterations. This seems to be an issue with the this dialog, also happens when i do a regular onchain tx.

I can also open separate issues if they are out of scope of this PR.

There are a few places where these frontend issues are still logged. For the binding loops I don't really see how they are getting triggered. They seem to be harmless. The layout issues are sometimes logged when anchoring an Item outside a layout to items within a Layout. Also seems mostly harmless.

@accumulator accumulator force-pushed the qml_forward_swap_user_tx_finalize branch from 0fe131b to 60c5145 Compare July 21, 2025 09:03
@f321x
Copy link
Member

f321x commented Jul 21, 2025

@accumulator i think only allowing ETA and capping the max allowed ETA would be the safest way. Static rates and mempool seem easy to get wrong for the funding tx. It seems nice to have the flexibility to save some fees for non-urgent swaps but the risk of a failing (refunded) swap by setting too low fees should be minimal imo.

@accumulator accumulator force-pushed the qml_forward_swap_user_tx_finalize branch from 7a8466f to c059037 Compare July 23, 2025 08:01
@accumulator accumulator force-pushed the qml_forward_swap_user_tx_finalize branch from c059037 to 72b16eb Compare July 23, 2025 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants