From adb567b20f4f72c74873a7400e07ef2f805ab15b Mon Sep 17 00:00:00 2001 From: Sander van Grieken Date: Thu, 24 Oct 2024 13:05:57 +0200 Subject: [PATCH 1/2] qt: wizard: implement user feedback TODOs, consistently use wizard.check_multisig_constraints --- electrum/gui/qt/util.py | 13 ++++-- electrum/gui/qt/wizard/wallet.py | 76 ++++++++++++++++++-------------- 2 files changed, 53 insertions(+), 36 deletions(-) diff --git a/electrum/gui/qt/util.py b/electrum/gui/qt/util.py index 79f5c1bce..52da25554 100644 --- a/electrum/gui/qt/util.py +++ b/electrum/gui/qt/util.py @@ -1259,10 +1259,12 @@ def read_QIcon_from_bytes(b: bytes) -> QIcon: qp = read_QPixmap_from_bytes(b) return QIcon(qp) + class IconLabel(QWidget): HorizontalSpacing = 2 - def __init__(self, *, text='', final_stretch=True): + def __init__(self, *, text='', final_stretch=True, reverse=False, hide_if_empty=False): super(QWidget, self).__init__() + self.hide_if_empty = hide_if_empty size = max(16, font_height()) self.icon_size = QSize(size, size) layout = QHBoxLayout() @@ -1271,13 +1273,18 @@ class IconLabel(QWidget): self.icon = QLabel() self.label = QLabel(text) self.label.setTextInteractionFlags(Qt.TextInteractionFlag.TextSelectableByMouse) - layout.addWidget(self.label) + layout.addWidget(self.icon if reverse else self.label) layout.addSpacing(self.HorizontalSpacing) - layout.addWidget(self.icon) + layout.addWidget(self.label if reverse else self.icon) if final_stretch: layout.addStretch() + self.setText(text) + def setText(self, text): self.label.setText(text) + if self.hide_if_empty: + self.setVisible(bool(text)) + def setIcon(self, icon): self.icon.setPixmap(icon.pixmap(self.icon_size)) self.icon.repaint() # macOS hack for #6269 diff --git a/electrum/gui/qt/wizard/wallet.py b/electrum/gui/qt/wizard/wallet.py index 4437f5c24..6202f848a 100644 --- a/electrum/gui/qt/wizard/wallet.py +++ b/electrum/gui/qt/wizard/wallet.py @@ -29,7 +29,7 @@ from electrum.gui.qt.bip39_recovery_dialog import Bip39RecoveryDialog from electrum.gui.qt.password_dialog import PasswordLayout, PW_NEW, MSG_ENTER_PASSWORD, PasswordLayoutForHW from electrum.gui.qt.seed_dialog import SeedWidget, MSG_PASSPHRASE_WARN_ISSUE4566, KeysWidget from electrum.gui.qt.util import (PasswordLineEdit, char_width_in_lineedit, WWLabel, InfoButton, font_height, - ChoiceWidget, MessageBoxMixin, icon_path) + ChoiceWidget, MessageBoxMixin, icon_path, IconLabel, read_QIcon) if TYPE_CHECKING: from electrum.simple_config import SimpleConfig @@ -516,6 +516,12 @@ class WCEnterExt(WalletWizardComponent, Logger): self.ext_edit.textEdited.connect(self.on_text_edited) self.layout().addWidget(self.ext_edit) self.layout().addStretch(1) + self.warn_label = IconLabel(reverse=True, hide_if_empty=True) + self.warn_label.setIcon(read_QIcon('warning.png')) + self.layout().addWidget(self.warn_label) + + def on_ready(self): + self.validate() def on_text_edited(self, text): # TODO also for cosigners? @@ -525,30 +531,10 @@ class WCEnterExt(WalletWizardComponent, Logger): def validate(self): self.apply() - text = self.ext_edit.text() - if len(text) == 0: - self.valid = False - return - - cosigner_data = self.wizard.current_cosigner(self.wizard_data) - if self.wizard_data['wallet_type'] == 'multisig': - if 'seed_variant' in cosigner_data and cosigner_data['seed_variant'] in ['bip39', 'slip39']: - # defer validation to when derivation path is known - self.valid = True - else: - if self.wizard.has_duplicate_masterkeys(self.wizard_data): - self.logger.debug('Duplicate master keys!') - # TODO: user feedback - self.valid = False - elif self.wizard.has_heterogeneous_masterkeys(self.wizard_data): - self.logger.debug('Heterogenous master keys!') - # TODO: user feedback - self.valid = False - else: - self.valid = True - else: - self.valid = True + musig_valid, errortext = self.wizard.check_multisig_constraints(self.wizard_data) + self.valid = musig_valid + self.warn_label.setText(errortext) def apply(self): cosigner_data = self.wizard.current_cosigner(self.wizard_data) @@ -567,8 +553,14 @@ class WCConfirmExt(WalletWizardComponent): self.layout().addWidget(self.ext_edit) self.layout().addStretch(1) - def on_text_edited(self, text): - self.valid = text == self.wizard_data['seed_extra_words'] + def on_ready(self): + self.validate() + + def on_text_edited(self, *args): + self.validate() + + def validate(self): + self.valid = self.ext_edit.text() == self.wizard_data['seed_extra_words'] def apply(self): pass @@ -580,6 +572,8 @@ class WCHaveSeed(WalletWizardComponent, Logger): Logger.__init__(self) self.layout().addWidget(WWLabel(_('Please enter your seed phrase in order to restore your wallet.'))) + self.warn_label = IconLabel(reverse=True, hide_if_empty=True) + self.warn_label.setIcon(read_QIcon('warning.png')) self.seed_widget = None self.can_passphrase = True @@ -610,6 +604,8 @@ class WCHaveSeed(WalletWizardComponent, Logger): self.layout().addWidget(self.seed_widget) self.layout().addStretch(1) + self.layout().addWidget(self.warn_label) + def is_seed(self, x): # really only used for electrum seeds. bip39 and slip39 are validated in SeedWidget t = mnemonic.calc_seed_type(x) @@ -635,10 +631,11 @@ class WCHaveSeed(WalletWizardComponent, Logger): return self.apply() - if not self.wizard.check_multisig_constraints(self.wizard_data)[0]: - # TODO: user feedback + musig_valid, errortext = self.wizard.check_multisig_constraints(self.wizard_data) + if not musig_valid: seed_valid = False + self.warn_label.setText(errortext) self.valid = seed_valid def apply(self): @@ -662,6 +659,9 @@ class WCScriptAndDerivation(WalletWizardComponent, Logger): self.choice_w = None self.derivation_path_edit = None + self.warn_label = IconLabel(reverse=True, hide_if_empty=True) + self.warn_label.setIcon(read_QIcon('warning.png')) + def on_ready(self): message1 = _('Choose the type of addresses in your wallet.') message2 = ' '.join([ @@ -736,6 +736,7 @@ class WCScriptAndDerivation(WalletWizardComponent, Logger): on_choice_click(self.choice_w.selected_index) # set default value for derivation path self.layout().addStretch(1) + self.layout().addWidget(self.warn_label) def validate(self): self.apply() @@ -744,10 +745,12 @@ class WCScriptAndDerivation(WalletWizardComponent, Logger): valid = is_bip32_derivation(cosigner_data['derivation_path']) if valid: - valid, error = self.wizard.check_multisig_constraints(self.wizard_data) + valid, errortext = self.wizard.check_multisig_constraints(self.wizard_data) if not valid: - # TODO: user feedback - self.logger.error(error) + self.logger.error(errortext) + self.warn_label.setText(errortext) + else: + self.warn_label.setText(_('Invalid derivation path')) self.valid = valid @@ -825,6 +828,9 @@ class WCHaveMasterKey(WalletWizardComponent): self.label.setMinimumWidth(400) self.header_layout.addWidget(self.label) + self.warn_label = IconLabel(reverse=True, hide_if_empty=True) + self.warn_label.setIcon(read_QIcon('warning.png')) + def on_ready(self): if self.wizard_data['wallet_type'] == 'standard': self.label.setText(self.message_create) @@ -840,10 +846,12 @@ class WCHaveMasterKey(WalletWizardComponent): def is_valid(x) -> bool: if not keystore.is_bip32_key(x): + self.warn_label.setText(_('Invalid key')) return False self.apply() - if not self.wizard.check_multisig_constraints(self.wizard_data)[0]: - # TODO: user feedback + musig_valid, errortext = self.wizard.check_multisig_constraints(self.wizard_data) + self.warn_label.setText(errortext) + if not musig_valid: return False return True else: @@ -858,6 +866,8 @@ class WCHaveMasterKey(WalletWizardComponent): self.keys_widget.validChanged.connect(key_valid_changed) self.layout().addWidget(self.keys_widget) + self.layout().addStretch() + self.layout().addWidget(self.warn_label) def apply(self): text = self.keys_widget.get_text() From 4a37668b01f23c2a1fc46844fc19c7c556f3ec6f Mon Sep 17 00:00:00 2001 From: Sander van Grieken Date: Thu, 24 Oct 2024 15:57:57 +0200 Subject: [PATCH 2/2] wizard: don't require seed extension to be set early. this also fixes deferring multisig constraint validation when seed is same as another cosigner, but still can have different seed extension --- electrum/gui/qt/wizard/wallet.py | 2 -- electrum/wizard.py | 14 ++++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/electrum/gui/qt/wizard/wallet.py b/electrum/gui/qt/wizard/wallet.py index 6202f848a..6135b4195 100644 --- a/electrum/gui/qt/wizard/wallet.py +++ b/electrum/gui/qt/wizard/wallet.py @@ -450,7 +450,6 @@ class WCCreateSeed(WalletWizardComponent): self.wizard_data['seed_type'] = self.seed_type self.wizard_data['seed_extend'] = self.seed_widget.is_ext self.wizard_data['seed_variant'] = 'electrum' - self.wizard_data['seed_extra_words'] = '' # empty default def create_seed(self): self.busy = True @@ -648,7 +647,6 @@ class WCHaveSeed(WalletWizardComponent, Logger): else: cosigner_data['seed_type'] = self.seed_widget.seed_type cosigner_data['seed_extend'] = self.seed_widget.is_ext if self.can_passphrase else False - cosigner_data['seed_extra_words'] = '' # empty default class WCScriptAndDerivation(WalletWizardComponent, Logger): diff --git a/electrum/wizard.py b/electrum/wizard.py index 3c7277539..0506fa1e4 100644 --- a/electrum/wizard.py +++ b/electrum/wizard.py @@ -414,10 +414,11 @@ class NewWalletWizard(AbstractWizard): def keystore_from_data(self, wallet_type: str, data: dict): if data['keystore_type'] in ['createseed', 'haveseed'] and 'seed' in data: + seed_extension = data['seed_extra_words'] if data['seed_extend'] else '' if data['seed_variant'] == 'electrum': - return keystore.from_seed(data['seed'], passphrase=data['seed_extra_words'], for_multisig=True) + return keystore.from_seed(data['seed'], passphrase=seed_extension, for_multisig=True) elif data['seed_variant'] == 'bip39': - root_seed = keystore.bip39_to_seed(data['seed'], passphrase=data['seed_extra_words']) + root_seed = keystore.bip39_to_seed(data['seed'], passphrase=seed_extension) derivation = normalize_bip32_derivation(data['derivation_path']) if wallet_type == 'multisig': script = data['script_type'] if data['script_type'] != 'p2sh' else 'standard' @@ -425,7 +426,7 @@ class NewWalletWizard(AbstractWizard): script = data['script_type'] if data['script_type'] != 'p2pkh' else 'standard' return keystore.from_bip43_rootseed(root_seed, derivation=derivation, xtype=script) elif data['seed_variant'] == 'slip39': - root_seed = data['seed'].decrypt(data['seed_extra_words']) + root_seed = data['seed'].decrypt(seed_extension) derivation = normalize_bip32_derivation(data['derivation_path']) if wallet_type == 'multisig': script = data['script_type'] if data['script_type'] != 'p2sh' else 'standard' @@ -548,15 +549,16 @@ class NewWalletWizard(AbstractWizard): for addr in data['address_list'].split(): addresses[addr] = {} elif data['keystore_type'] in ['createseed', 'haveseed']: + seed_extension = data['seed_extra_words'] if data['seed_extend'] else '' if data['seed_type'] in ['old', 'standard', 'segwit']: self._logger.debug('creating keystore from electrum seed') - k = keystore.from_seed(data['seed'], passphrase=data['seed_extra_words'], for_multisig=data['wallet_type'] == 'multisig') + k = keystore.from_seed(data['seed'], passphrase=seed_extension, for_multisig=data['wallet_type'] == 'multisig') elif data['seed_type'] in ['bip39', 'slip39']: self._logger.debug('creating keystore from %s seed' % data['seed_type']) if data['seed_type'] == 'bip39': - root_seed = keystore.bip39_to_seed(data['seed'], passphrase=data['seed_extra_words']) + root_seed = keystore.bip39_to_seed(data['seed'], passphrase=seed_extension) else: - root_seed = data['seed'].decrypt(data['seed_extra_words']) + root_seed = data['seed'].decrypt(seed_extension) derivation = normalize_bip32_derivation(data['derivation_path']) if data['wallet_type'] == 'multisig': script = data['script_type'] if data['script_type'] != 'p2sh' else 'standard'