From c6cd47ebba21d520d5d78f557f0386590fccc832 Mon Sep 17 00:00:00 2001 From: Sander van Grieken Date: Wed, 23 Oct 2024 12:02:01 +0200 Subject: [PATCH 1/2] qml: refactor QEWallet.sign() to sign() and sign_and_broadcast(), add user feedback when signing fails. --- .../gui/qml/components/WalletMainView.qml | 8 +++ electrum/gui/qml/qetxdetails.py | 4 +- electrum/gui/qml/qetxfinalizer.py | 9 ++- electrum/gui/qml/qewallet.py | 58 +++++++++++-------- electrum/plugins/trustedcoin/trustedcoin.py | 2 + 5 files changed, 53 insertions(+), 28 deletions(-) diff --git a/electrum/gui/qml/components/WalletMainView.qml b/electrum/gui/qml/components/WalletMainView.qml index f9c0228cb..76123d99c 100644 --- a/electrum/gui/qml/components/WalletMainView.qml +++ b/electrum/gui/qml/components/WalletMainView.qml @@ -644,6 +644,14 @@ Item { } _confirmPaymentDialog.destroy() } + onSignError: (message) => { + var dialog = app.messageDialog.createObject(mainView, { + title: qsTr('Error'), + text: [qsTr('Could not sign tx'), message].join('\n\n'), + iconSource: '../../../icons/warning.png' + }) + dialog.open() + } } // TODO: lingering confirmPaymentDialogs can raise exceptions in // the child finalizer when currentWallet disappears, but we need diff --git a/electrum/gui/qml/qetxdetails.py b/electrum/gui/qml/qetxdetails.py index ead709a30..d6a894063 100644 --- a/electrum/gui/qml/qetxdetails.py +++ b/electrum/gui/qml/qetxdetails.py @@ -395,8 +395,10 @@ class QETxDetails(QObject, QtEventListener): if broadcast: self._wallet.broadcastSucceeded.connect(self.onBroadcastSucceeded) self._wallet.broadcastFailed.connect(self.onBroadcastFailed) + self._wallet.sign_and_broadcast(self._tx, on_success=self.on_signed_tx) + else: + self._wallet.sign(self._tx, on_success=self.on_signed_tx) - self._wallet.sign(self._tx, broadcast=broadcast, on_success=self.on_signed_tx) # side-effect: signing updates self._tx # we rely on this for broadcast diff --git a/electrum/gui/qml/qetxfinalizer.py b/electrum/gui/qml/qetxfinalizer.py index 5273d71a4..d17d8eb04 100644 --- a/electrum/gui/qml/qetxfinalizer.py +++ b/electrum/gui/qml/qetxfinalizer.py @@ -272,6 +272,7 @@ class QETxFinalizer(TxFeeSlider): _logger = get_logger(__name__) finished = pyqtSignal([bool, bool, bool], arguments=['signed', 'saved', 'complete']) + signError = pyqtSignal([str], arguments=['message']) def __init__(self, parent=None, *, make_tx=None, accept=None): super().__init__(parent) @@ -419,7 +420,7 @@ class QETxFinalizer(TxFeeSlider): self.f_accept(self._tx) return - self._wallet.sign(self._tx, broadcast=True, on_success=partial(self.on_signed_tx, False)) + self._wallet.sign_and_broadcast(self._tx, on_success=partial(self.on_signed_tx, False), on_failure=self.on_sign_failed) @pyqtSlot() def sign(self): @@ -427,7 +428,7 @@ class QETxFinalizer(TxFeeSlider): self._logger.error('no valid tx') return - self._wallet.sign(self._tx, broadcast=False, on_success=partial(self.on_signed_tx, True)) + self._wallet.sign(self._tx, on_success=partial(self.on_signed_tx, True), on_failure=self.on_sign_failed) def on_signed_tx(self, save: bool, tx: Transaction): self._logger.debug('on_signed_tx') @@ -439,6 +440,10 @@ class QETxFinalizer(TxFeeSlider): self._logger.error('Could not save tx') self.finished.emit(True, saved, tx.is_complete()) + def on_sign_failed(self, msg: str = None): + self._logger.debug('on_sign_failed') + self.signError.emit(msg) + @pyqtSlot(result='QVariantList') def getSerializedTx(self): txqr = self._tx.to_qr_data() diff --git a/electrum/gui/qml/qewallet.py b/electrum/gui/qml/qewallet.py index fb9079e30..bddf245a3 100644 --- a/electrum/gui/qml/qewallet.py +++ b/electrum/gui/qml/qewallet.py @@ -3,7 +3,7 @@ import base64 import queue import threading import time -from typing import TYPE_CHECKING, Callable +from typing import TYPE_CHECKING, Callable, Optional, Any from functools import partial from PyQt6.QtCore import pyqtProperty, pyqtSignal, pyqtSlot, QObject, QTimer @@ -518,41 +518,48 @@ class QEWallet(AuthMixin, QObject, QtEventListener): self.isLightningChanged.emit() self.dataChanged.emit() - @auth_protect(message=_('Sign on-chain transaction?')) # FIXME auth msg cannot be explicit due to "broadcast" param - def sign(self, tx, *, broadcast: bool = False, on_success: Callable[[Transaction], None] = None, on_failure: Callable[[], None] = None): - sign_hook = run_hook('tc_sign_wrapper', self.wallet, tx, partial(self.on_sign_complete, broadcast, on_success), partial(self.on_sign_failed, on_failure)) - if sign_hook: - success = self.do_sign(tx, False) - if success: - self._logger.debug('plugin needs to sign tx too') - sign_hook(tx) - return - else: - success = self.do_sign(tx, broadcast) - - if success: - if on_success: - on_success(tx) - else: - if on_failure: - on_failure() - - def do_sign(self, tx, broadcast): + @auth_protect(message=_('Sign and send on-chain transaction?')) + def sign_and_broadcast(self, tx, *, + on_success: Callable[[Transaction], None] = None, + on_failure: Callable[[Optional[Any]], None] = None) -> None: + self.do_sign(tx, True, on_success, on_failure) + + @auth_protect(message=_('Sign on-chain transaction?')) + def sign(self, tx, *, + on_success: Callable[[Transaction], None] = None, + on_failure: Callable[[Optional[Any]], None] = None) -> None: + self.do_sign(tx, False, on_success, on_failure) + + def do_sign(self, tx, broadcast, on_success: Callable[[Transaction], None] = None, on_failure: Callable[[Optional[Any]], None] = None): + # tc_sign_wrapper is only used by 2fa. don't pass on_failure handler, it is handled via otpFailed signal + sign_hook = run_hook('tc_sign_wrapper', self.wallet, tx, + partial(self.on_sign_complete, broadcast, on_success), + partial(self.on_sign_failed, None)) try: # ignore_warnings=True, because UI checks and asks user confirmation itself tx = self.wallet.sign_transaction(tx, self.password, ignore_warnings=True) except BaseException as e: self._logger.error(f'{e!r}') - self.signFailed.emit(str(e)) + self.signFailed.emit(str(e)) # TODO: remove, signal never used? + if on_failure: + on_failure(str(e)) + return if tx is None: self._logger.info('did not sign') - return False + if on_failure: + on_failure() + return + + if sign_hook: + self._logger.debug('plugin needs to sign tx too') + sign_hook(tx) + return txid = tx.txid() self._logger.debug(f'do_sign(), txid={txid}') - self.signSucceeded.emit(txid) + self.signSucceeded.emit(txid) # TODO: remove, signal never used? if not tx.is_complete(): self._logger.debug('tx not complete') @@ -564,7 +571,8 @@ class QEWallet(AuthMixin, QObject, QtEventListener): # not broadcasted, so refresh history here self.historyModel.initModel(True) - return True + if on_success: + on_success(tx) # this assumes a 2fa wallet, but there are no other tc_sign_wrapper hooks, so that's ok def on_sign_complete(self, broadcast, cb: Callable[[Transaction], None] = None, tx: Transaction = None): diff --git a/electrum/plugins/trustedcoin/trustedcoin.py b/electrum/plugins/trustedcoin/trustedcoin.py index 7925b400c..a5ecda3b1 100644 --- a/electrum/plugins/trustedcoin/trustedcoin.py +++ b/electrum/plugins/trustedcoin/trustedcoin.py @@ -458,9 +458,11 @@ class TrustedCoinPlugin(BasePlugin): if not wallet.keystores['x3'].can_sign(tx, ignore_watching_only=True): self.logger.info("twofactor: xpub3 not needed") return + def wrapper(tx): assert tx self.prompt_user_for_otp(wallet, tx, on_success, on_failure) + return wrapper def prompt_user_for_otp(self, wallet, tx, on_success, on_failure) -> None: From 1363d8c87836d297f69717ba55ed3815b1f15b92 Mon Sep 17 00:00:00 2001 From: Sander van Grieken Date: Mon, 28 Oct 2024 10:08:32 +0100 Subject: [PATCH 2/2] qml: remove unused signals QEWallet.signSucceeded and QEWallet.signFailed --- electrum/gui/qml/qewallet.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/electrum/gui/qml/qewallet.py b/electrum/gui/qml/qewallet.py index bddf245a3..6386eaf20 100644 --- a/electrum/gui/qml/qewallet.py +++ b/electrum/gui/qml/qewallet.py @@ -65,8 +65,6 @@ class QEWallet(AuthMixin, QObject, QtEventListener): paymentSucceeded = pyqtSignal([str], arguments=['key']) paymentFailed = pyqtSignal([str, str], arguments=['key', 'reason']) requestNewPassword = pyqtSignal() - signSucceeded = pyqtSignal([str], arguments=['txid']) - signFailed = pyqtSignal([str], arguments=['message']) broadcastSucceeded = pyqtSignal([str], arguments=['txid']) broadcastFailed = pyqtSignal([str, str, str], arguments=['txid', 'code', 'reason']) saveTxSuccess = pyqtSignal([str], arguments=['txid']) @@ -540,7 +538,6 @@ class QEWallet(AuthMixin, QObject, QtEventListener): tx = self.wallet.sign_transaction(tx, self.password, ignore_warnings=True) except BaseException as e: self._logger.error(f'{e!r}') - self.signFailed.emit(str(e)) # TODO: remove, signal never used? if on_failure: on_failure(str(e)) return @@ -559,8 +556,6 @@ class QEWallet(AuthMixin, QObject, QtEventListener): txid = tx.txid() self._logger.debug(f'do_sign(), txid={txid}') - self.signSucceeded.emit(txid) # TODO: remove, signal never used? - if not tx.is_complete(): self._logger.debug('tx not complete') broadcast = False