From 7f64ecc4bdf14f61ba48f66c04cdef28e3f222a5 Mon Sep 17 00:00:00 2001 From: SomberNight Date: Mon, 20 Nov 2023 18:00:23 +0000 Subject: [PATCH] wallet.bump_fee: the glorious return of BumpFeeStrategy :D gui/qt/rbf_dialog.py (old) lines 57-64 were implementing logic that should not be part of GUI code. Case in point, gui/qml/qetxfinalizer.py (old) lines 511-513 duplicated half of that logic but not the other half. That logic is now moved to wallet.get_bumpfee_strategies_for_tx(). More context: a user on irc got confused when using the qml gui. They were sending "max" and wanted to bump_fee. The qml gui selected the "preserve_payment" strategy by default, using which there was no solution, and the user did not notice that the strategy can be altered (via the "method" dropdown). The qt gui had logic to select "decrease_payment" by default in such a case (which does find a solution to bump) but this logic was not duplicated in the qml gui. Instead of duplicating the logic, this commit moves it to shared lib code. --- electrum/commands.py | 4 +- .../gui/qml/components/RbfBumpFeeDialog.qml | 6 +-- electrum/gui/qml/qetxfinalizer.py | 28 +++++------ electrum/gui/qt/rbf_dialog.py | 29 ++++-------- electrum/tests/test_wallet_vertical.py | 14 +++--- electrum/wallet.py | 47 +++++++++++++++++-- 6 files changed, 78 insertions(+), 50 deletions(-) diff --git a/electrum/commands.py b/electrum/commands.py index 9b5647ed3..2ddd37f2b 100644 --- a/electrum/commands.py +++ b/electrum/commands.py @@ -52,7 +52,7 @@ from .transaction import (Transaction, multisig_script, TxOutput, PartialTransac from . import transaction from .invoices import PR_PAID, PR_UNPAID, PR_UNKNOWN, PR_EXPIRED from .synchronizer import Notifier -from .wallet import Abstract_Wallet, create_new_wallet, restore_wallet_from_text, Deterministic_Wallet +from .wallet import Abstract_Wallet, create_new_wallet, restore_wallet_from_text, Deterministic_Wallet, BumpFeeStrategy from .address_synchronizer import TX_HEIGHT_LOCAL from .mnemonic import Mnemonic from .lnutil import SENT, RECEIVED @@ -819,7 +819,7 @@ class Commands: tx=tx, txid=tx.txid(), coins=coins, - decrease_payment=decrease_payment, + strategy=BumpFeeStrategy.DECREASE_PAYMENT if decrease_payment else BumpFeeStrategy.PRESERVE_PAYMENT, new_fee_rate=new_fee_rate) if not unsigned: wallet.sign_transaction(new_tx, password) diff --git a/electrum/gui/qml/components/RbfBumpFeeDialog.qml b/electrum/gui/qml/components/RbfBumpFeeDialog.qml index c8ed4239b..28229f29a 100644 --- a/electrum/gui/qml/components/RbfBumpFeeDialog.qml +++ b/electrum/gui/qml/components/RbfBumpFeeDialog.qml @@ -62,15 +62,11 @@ ElDialog { ElComboBox { id: bumpMethodComboBox - enabled: rbffeebumper.canChangeBumpMethod textRole: 'text' valueRole: 'value' - model: [ - { text: qsTr('Preserve payment'), value: 'preserve_payment' }, - { text: qsTr('Decrease payment'), value: 'decrease_payment' } - ] + model: rbffeebumper.bumpMethodsAvailable onCurrentValueChanged: { if (activeFocus) rbffeebumper.bumpMethod = currentValue diff --git a/electrum/gui/qml/qetxfinalizer.py b/electrum/gui/qml/qetxfinalizer.py index 1c40fce93..7e4e6938a 100644 --- a/electrum/gui/qml/qetxfinalizer.py +++ b/electrum/gui/qml/qetxfinalizer.py @@ -8,7 +8,7 @@ from electrum.logging import get_logger from electrum.i18n import _ from electrum.transaction import PartialTxOutput, PartialTransaction, Transaction from electrum.util import NotEnoughFunds, profiler -from electrum.wallet import CannotBumpFee, CannotDoubleSpendTx, CannotCPFP +from electrum.wallet import CannotBumpFee, CannotDoubleSpendTx, CannotCPFP, BumpFeeStrategy from electrum.plugin import run_hook from .qewallet import QEWallet @@ -460,13 +460,8 @@ class QETxRbfFeeBumper(TxFeeSlider, TxMonMixin): self._oldfee_rate = 0 self._orig_tx = None self._rbf = True - self._bump_method = 'preserve_payment' - self._can_change_bump_method = True - - canChangeBumpMethodChanged = pyqtSignal() - @pyqtProperty(bool, notify=canChangeBumpMethodChanged) - def canChangeBumpMethod(self): - return self._can_change_bump_method + self._bump_method = BumpFeeStrategy.PRESERVE_PAYMENT.name + self._bump_methods_available = [] oldfeeChanged = pyqtSignal() @pyqtProperty(QEAmount, notify=oldfeeChanged) @@ -496,21 +491,26 @@ class QETxRbfFeeBumper(TxFeeSlider, TxMonMixin): return self._bump_method @bumpMethod.setter - def bumpMethod(self, bumpmethod): - assert self._can_change_bump_method + def bumpMethod(self, bumpmethod: str) -> None: if self._bump_method != bumpmethod: self._bump_method = bumpmethod self.bumpMethodChanged.emit() self.update() + bumpMethodsAvailableChanged = pyqtSignal() + @pyqtProperty('QVariantList', notify=bumpMethodsAvailableChanged) + def bumpMethodsAvailable(self): + return self._bump_methods_available + def get_tx(self): assert self._txid self._orig_tx = self._wallet.wallet.db.get_transaction(self._txid) assert self._orig_tx - if self._wallet.wallet.get_swap_by_funding_tx(self._orig_tx): - self._can_change_bump_method = False - self.canChangeBumpMethodChanged.emit() + strategies, def_strat_idx = self._wallet.wallet.get_bumpfee_strategies_for_tx(tx=self._orig_tx, txid=self._txid) + self._bump_methods_available = [{'value': strat.name, 'text': strat.text()} for strat in strategies] + self.bumpMethodsAvailableChanged.emit() + self.bumpMethod = strategies[def_strat_idx].name if not isinstance(self._orig_tx, PartialTransaction): self._orig_tx = PartialTransaction.from_tx(self._orig_tx) @@ -547,7 +547,7 @@ class QETxRbfFeeBumper(TxFeeSlider, TxMonMixin): tx=self._orig_tx, txid=self._txid, new_fee_rate=new_fee_rate, - decrease_payment=self._bump_method=='decrease_payment' + strategy=BumpFeeStrategy[self._bump_method], ) except CannotBumpFee as e: self._valid = False diff --git a/electrum/gui/qt/rbf_dialog.py b/electrum/gui/qt/rbf_dialog.py index 346e473b2..db04f99f4 100644 --- a/electrum/gui/qt/rbf_dialog.py +++ b/electrum/gui/qt/rbf_dialog.py @@ -15,7 +15,7 @@ from .util import (ColorScheme, WindowModalDialog, Buttons, from electrum.i18n import _ from electrum.transaction import PartialTransaction -from electrum.wallet import CannotRBFTx +from electrum.wallet import CannotRBFTx, BumpFeeStrategy if TYPE_CHECKING: from .main_window import ElectrumWindow @@ -54,19 +54,13 @@ class _BaseRBFDialog(TxEditor): self.feerate_e.setAmount(new_fee_rate) self.update() self.fee_slider.deactivate() - # are we paying max? - invoices = self.wallet.get_relevant_invoices_for_tx(txid) - if len(invoices) == 1 and len(invoices[0].outputs) == 1: - if invoices[0].outputs[0].value == '!': - self.set_decrease_payment() - # do not decrease payment if it is a swap - if self.wallet.get_swap_by_funding_tx(self.old_tx): - self.method_combo.setEnabled(False) def create_grid(self): self.method_label = QLabel(_('Method') + ':') self.method_combo = QComboBox() - self.method_combo.addItems([_('Preserve payment'), _('Decrease payment')]) + self._strategies, def_strat_idx = self.wallet.get_bumpfee_strategies_for_tx(tx=self.old_tx, txid=self.old_txid) + self.method_combo.addItems([strat.text() for strat in self._strategies]) + self.method_combo.setCurrentIndex(def_strat_idx) self.method_combo.currentIndexChanged.connect(self.trigger_update) self.method_combo.setFocusPolicy(Qt.NoFocus) old_size_label = TxSizeLabel() @@ -93,12 +87,6 @@ class _BaseRBFDialog(TxEditor): grid.addWidget(self.locktime_e, 5, 1, 1, 2) return grid - def is_decrease_payment(self): - return self.method_combo.currentIndex() == 1 - - def set_decrease_payment(self): - self.method_combo.setCurrentIndex(1) - def run(self) -> None: if not self.exec_(): return @@ -133,10 +121,12 @@ class _BaseRBFDialog(TxEditor): if not self.tx: return delta = self.tx.get_fee() - self.old_tx.get_fee() - if not self.is_decrease_payment(): + if self._strategies[self.method_combo.currentIndex()] == BumpFeeStrategy.PRESERVE_PAYMENT: msg = _("You will pay {} more.").format(self.main_window.format_amount_and_units(delta)) - else: + elif self._strategies[self.method_combo.currentIndex()] == BumpFeeStrategy.DECREASE_PAYMENT: msg = _("The recipient will receive {} less.").format(self.main_window.format_amount_and_units(delta)) + else: + raise Exception(f"unknown strategy: {self=}") messages.insert(0, msg) return messages @@ -164,7 +154,8 @@ class BumpFeeDialog(_BaseRBFDialog): txid=self.old_txid, new_fee_rate=fee_rate, coins=self.main_window.get_coins(nonlocal_only=True, confirmed_only=confirmed_only), - decrease_payment=self.is_decrease_payment()) + strategy=self._strategies[self.method_combo.currentIndex()], + ) class DSCancelDialog(_BaseRBFDialog): diff --git a/electrum/tests/test_wallet_vertical.py b/electrum/tests/test_wallet_vertical.py index bf5d77a03..841b07762 100644 --- a/electrum/tests/test_wallet_vertical.py +++ b/electrum/tests/test_wallet_vertical.py @@ -12,7 +12,7 @@ from electrum import SimpleConfig from electrum import util from electrum.address_synchronizer import TX_HEIGHT_UNCONFIRMED, TX_HEIGHT_UNCONF_PARENT from electrum.wallet import (sweep, Multisig_Wallet, Standard_Wallet, Imported_Wallet, - restore_wallet_from_text, Abstract_Wallet, CannotBumpFee) + restore_wallet_from_text, Abstract_Wallet, CannotBumpFee, BumpFeeStrategy) from electrum.util import ( bfh, NotEnoughFunds, UnrelatedTransactionException, UserFacingException) @@ -1229,7 +1229,7 @@ class TestWalletSending(ElectrumTestCase): tx = wallet.bump_fee( tx=tx_from_any(orig_rbf_tx.serialize()), new_fee_rate=60, - decrease_payment=True, + strategy=BumpFeeStrategy.DECREASE_PAYMENT, ) tx.locktime = 1936085 tx.version = 2 @@ -1271,7 +1271,7 @@ class TestWalletSending(ElectrumTestCase): tx = wallet.bump_fee( tx=tx_from_any(orig_rbf_tx.serialize()), new_fee_rate=60, - decrease_payment=True, + strategy=BumpFeeStrategy.DECREASE_PAYMENT, ) tx.locktime = 1936095 tx.version = 2 @@ -1313,19 +1313,19 @@ class TestWalletSending(ElectrumTestCase): tx = wallet.bump_fee( tx=tx_from_any(orig_rbf_tx.serialize()), new_fee_rate=99999, - decrease_payment=True, + strategy=BumpFeeStrategy.DECREASE_PAYMENT, ) with self.assertRaises(CannotBumpFee): tx = wallet.bump_fee( tx=tx_from_any(orig_rbf_tx.serialize()), new_fee_rate=99999, - decrease_payment=False, + strategy=BumpFeeStrategy.PRESERVE_PAYMENT, ) tx = wallet.bump_fee( tx=tx_from_any(orig_rbf_tx.serialize()), new_fee_rate=60, - decrease_payment=True, + strategy=BumpFeeStrategy.DECREASE_PAYMENT, ) tx.locktime = 1936085 tx.version = 2 @@ -1647,7 +1647,7 @@ class TestWalletSending(ElectrumTestCase): self.assertEqual((0, 0, 0), wallet.get_balance()) # bump tx - tx = wallet.bump_fee(tx=tx_from_any(tx.serialize()), new_fee_rate=70.0, decrease_payment=True) + tx = wallet.bump_fee(tx=tx_from_any(tx.serialize()), new_fee_rate=70.0, strategy=BumpFeeStrategy.DECREASE_PAYMENT) tx.locktime = 1325500 tx.version = 1 if simulate_moving_txs: diff --git a/electrum/wallet.py b/electrum/wallet.py index 570e395de..76ac6a55b 100644 --- a/electrum/wallet.py +++ b/electrum/wallet.py @@ -247,6 +247,23 @@ class InternalAddressCorruption(Exception): "Please restore your wallet from seed, and compare the addresses in both files") +class BumpFeeStrategy(enum.Enum): + PRESERVE_PAYMENT = enum.auto() + DECREASE_PAYMENT = enum.auto() + + @classmethod + def all(cls) -> Sequence['BumpFeeStrategy']: + return list(BumpFeeStrategy.__members__.values()) + + def text(self) -> str: + if self == self.PRESERVE_PAYMENT: + return _('Preserve payment') + elif self == self.DECREASE_PAYMENT: + return _('Decrease payment') + else: + raise Exception(f"unknown strategy: {self=}") + + class ReceiveRequestHelp(NamedTuple): # help texts (warnings/errors): address_help: str @@ -1955,6 +1972,28 @@ class Abstract_Wallet(ABC, Logger, EventListener): def can_export(self): return not self.is_watching_only() and hasattr(self.keystore, 'get_private_key') + def get_bumpfee_strategies_for_tx( + self, + *, + tx: Transaction, + txid: str = None, + ) -> Tuple[Sequence[BumpFeeStrategy], int]: + """Returns tuple(list of available strategies, idx of recommended option among those).""" + txid = txid or tx.txid() + assert txid + assert tx.txid() in (None, txid) + all_strats = BumpFeeStrategy.all() + # are we paying max? + invoices = self.get_relevant_invoices_for_tx(txid) + if len(invoices) == 1 and len(invoices[0].outputs) == 1: + if invoices[0].outputs[0].value == '!': + return all_strats, all_strats.index(BumpFeeStrategy.DECREASE_PAYMENT) + # do not decrease payment if it is a swap + if self.get_swap_by_funding_tx(tx): + return [BumpFeeStrategy.PRESERVE_PAYMENT], 0 + # default + return all_strats, all_strats.index(BumpFeeStrategy.PRESERVE_PAYMENT) + def bump_fee( self, *, @@ -1962,7 +2001,7 @@ class Abstract_Wallet(ABC, Logger, EventListener): txid: str = None, new_fee_rate: Union[int, float, Decimal], coins: Sequence[PartialTxInput] = None, - decrease_payment=False, + strategy: BumpFeeStrategy = BumpFeeStrategy.PRESERVE_PAYMENT, ) -> PartialTransaction: """Increase the miner fee of 'tx'. 'new_fee_rate' is the target min rate in sat/vbyte @@ -1991,7 +2030,7 @@ class Abstract_Wallet(ABC, Logger, EventListener): if new_fee_rate <= old_fee_rate: raise CannotBumpFee(_("The new fee rate needs to be higher than the old fee rate.")) - if not decrease_payment: + if strategy == BumpFeeStrategy.PRESERVE_PAYMENT: # FIXME: we should try decreasing change first, # but it requires updating a bunch of unit tests try: @@ -2004,9 +2043,11 @@ class Abstract_Wallet(ABC, Logger, EventListener): except CannotBumpFee as e: tx_new = self._bump_fee_through_decreasing_change( tx=tx, new_fee_rate=new_fee_rate) - else: + elif strategy == BumpFeeStrategy.DECREASE_PAYMENT: tx_new = self._bump_fee_through_decreasing_payment( tx=tx, new_fee_rate=new_fee_rate) + else: + raise Exception(f"unknown strategy: {strategy=}") target_min_fee = new_fee_rate * tx_new.estimated_size() actual_fee = tx_new.get_fee()