Skip to content
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.

Sign up for GitHub

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

Jump to bottom

UI external signer support (e.g. hardware wallet) #4

Merged
merged 7 commits into from
Jun 9, 2021
Merged

UI external signer support (e.g. hardware wallet) #4

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6cdbc83
gui: add external signer path to options dialog
Sjors Sep 16, 2019
eef8d64
gui: create wallet with external signer
Sjors Feb 21, 2020
450cb40
wallet: add displayAddress to interface
Sjors Sep 16, 2019
62ac119
gui: display address on external signer
Sjors Sep 16, 2019
3f845ea
node: add externalSigners to interface
Sjors Oct 29, 2019
24815c6
gui: wallet creation detects external signer
Sjors Nov 7, 2019
1c4b456
gui: send using external signer
Sjors Feb 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 42 additions & 1 deletion src/qt/createwalletdialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,39 @@ CreateWalletDialog::CreateWalletDialog(QWidget* parent) :
});

connect(ui->encrypt_wallet_checkbox, &QCheckBox::toggled, [this](bool checked) {
// Disable the disable_privkeys_checkbox when isEncryptWalletChecked is
// Disable the disable_privkeys_checkbox and external_signer_checkbox when isEncryptWalletChecked is
// set to true, enable it when isEncryptWalletChecked is false.
ui->disable_privkeys_checkbox->setEnabled(!checked);
ui->external_signer_checkbox->setEnabled(!checked);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eef8d64

This should take into account ENABLE_EXTERNAL_SIGNER.

May I suggest adding a private slot update or updateState and move all logic there?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a fix in bitcoin/bitcoin#21935. I agree this whole toggling code needs a refactor.


// When the disable_privkeys_checkbox is disabled, uncheck it.
if (!ui->disable_privkeys_checkbox->isEnabled()) {
ui->disable_privkeys_checkbox->setChecked(false);
}

// When the external_signer_checkbox box is disabled, uncheck it.
if (!ui->external_signer_checkbox->isEnabled()) {
ui->external_signer_checkbox->setChecked(false);
}

});

connect(ui->external_signer_checkbox, &QCheckBox::toggled, [this](bool checked) {
ui->encrypt_wallet_checkbox->setEnabled(!checked);
ui->blank_wallet_checkbox->setEnabled(!checked);
ui->disable_privkeys_checkbox->setEnabled(!checked);
ui->descriptor_checkbox->setEnabled(!checked);

// The external signer checkbox is only enabled when a device is detected.
// In that case it is checked by default. Toggling it restores the other
// options to their default.
ui->descriptor_checkbox->setChecked(checked);
ui->encrypt_wallet_checkbox->setChecked(false);
ui->disable_privkeys_checkbox->setChecked(checked);
// The blank check box is ambiguous. This flag is always true for a
// watch-only wallet, even though we immedidately fetch keys from the
// external signer.
ui->blank_wallet_checkbox->setChecked(checked);
});

connect(ui->disable_privkeys_checkbox, &QCheckBox::toggled, [this](bool checked) {
Expand Down Expand Up @@ -63,11 +88,22 @@ CreateWalletDialog::CreateWalletDialog(QWidget* parent) :
ui->descriptor_checkbox->setToolTip(tr("Compiled without sqlite support (required for descriptor wallets)"));
ui->descriptor_checkbox->setEnabled(false);
ui->descriptor_checkbox->setChecked(false);
ui->external_signer_checkbox->setEnabled(false);
ui->external_signer_checkbox->setChecked(false);
#endif

#ifndef USE_BDB
ui->descriptor_checkbox->setEnabled(false);
ui->descriptor_checkbox->setChecked(true);
#endif

#ifndef ENABLE_EXTERNAL_SIGNER
//: "External signing" means using devices such as hardware wallets.
ui->external_signer_checkbox->setToolTip(tr("Compiled without external signing support (required for external signing)"));
Sjors marked this conversation as resolved.
Show resolved Hide resolved
ui->external_signer_checkbox->setEnabled(false);
ui->external_signer_checkbox->setChecked(false);
#endif

}

CreateWalletDialog::~CreateWalletDialog()
Expand Down Expand Up @@ -99,3 +135,8 @@ bool CreateWalletDialog::isDescriptorWalletChecked() const
{
return ui->descriptor_checkbox->isChecked();
}

bool CreateWalletDialog::isExternalSignerChecked() const
{
return ui->external_signer_checkbox->isChecked();
}
1 change: 1 addition & 0 deletions src/qt/createwalletdialog.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class CreateWalletDialog : public QDialog
bool isDisablePrivateKeysChecked() const;
bool isMakeBlankWalletChecked() const;
bool isDescriptorWalletChecked() const;
bool isExternalSignerChecked() const;

private:
Ui::CreateWalletDialog *ui;
Expand Down
11 changes: 11 additions & 0 deletions src/qt/forms/createwalletdialog.ui
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,16 @@
</property>
</widget>
</item>
<item>
<widget class="QCheckBox" name="external_signer_checkbox">
<property name="toolTip">
<string>Use an external signing device such as a hardware wallet. Configure the external signer script in wallet preferences first.</string>
</property>
<property name="text">
<string>External signer</string>
</property>
</widget>
</item>
</layout>
</widget>
</item>
Expand Down Expand Up @@ -143,6 +153,7 @@
<tabstop>disable_privkeys_checkbox</tabstop>
<tabstop>blank_wallet_checkbox</tabstop>
<tabstop>descriptor_checkbox</tabstop>
<tabstop>external_signer_checkbox</tabstop>
</tabstops>
<resources/>
<connections>
Expand Down
30 changes: 30 additions & 0 deletions src/qt/forms/optionsdialog.ui
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,36 @@
</layout>
</widget>
</item>
<item>
<widget class="QGroupBox" name="groupBoxHww">
<property name="title">
<string>External Signer (e.g. hardware wallet)</string>
</property>
<layout class="QVBoxLayout" name="verticalLayoutHww">
<item>
<layout class="QHBoxLayout" name="horizontalLayoutHww">
<item>
<widget class="QLabel" name="externalSignerPathLabel">
<property name="text">
<string>&amp;External signer script path</string>
</property>
<property name="buddy">
<cstring>externalSignerPath</cstring>
</property>
</widget>
</item>
<item>
<widget class="QLineEdit" name="externalSignerPath">
<property name="toolTip">
<string>Full path to a Bitcoin Core compatible script (e.g. C:\Downloads\hwi.exe or /Users/you/Downloads/hwi.py). Beware: malware can steal your coins!</string>
</property>
</widget>
</item>
</layout>
</item>
</layout>
</widget>
Sjors marked this conversation as resolved.
Show resolved Hide resolved
</item>
<item>
<spacer name="verticalSpacer_Wallet">
<property name="orientation">
Expand Down
2 changes: 2 additions & 0 deletions src/qt/optionsdialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ void OptionsDialog::setModel(OptionsModel *_model)
connect(ui->prune, &QCheckBox::clicked, this, &OptionsDialog::togglePruneWarning);
connect(ui->pruneSize, qOverload<int>(&QSpinBox::valueChanged), this, &OptionsDialog::showRestartWarning);
connect(ui->databaseCache, qOverload<int>(&QSpinBox::valueChanged), this, &OptionsDialog::showRestartWarning);
connect(ui->externalSignerPath, &QLineEdit::textChanged, [this]{ showRestartWarning(); });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

28cd645

Note to other reviewers. The QLineEdit::textChanged signal has the QString parameter that actually is a new text. Using a lambda instead of a slot makes discarding of this parameter explicit. I think this is a good practice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6cdbc83

Should disable externalSignerPath if not defined ENABLE_EXTERNAL_SIGNER?

connect(ui->threadsScriptVerif, qOverload<int>(&QSpinBox::valueChanged), this, &OptionsDialog::showRestartWarning);
/* Wallet */
connect(ui->spendZeroConfChange, &QCheckBox::clicked, this, &OptionsDialog::showRestartWarning);
Expand Down Expand Up @@ -233,6 +234,7 @@ void OptionsDialog::setMapper()
/* Wallet */
mapper->addMapping(ui->spendZeroConfChange, OptionsModel::SpendZeroConfChange);
mapper->addMapping(ui->coinControlFeatures, OptionsModel::CoinControlFeatures);
mapper->addMapping(ui->externalSignerPath, OptionsModel::ExternalSignerPath);

/* Network */
mapper->addMapping(ui->mapPortUpnp, OptionsModel::MapPortUPnP);
Expand Down
15 changes: 15 additions & 0 deletions src/qt/optionsmodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,13 @@ void OptionsModel::Init(bool resetSettings)
settings.setValue("bSpendZeroConfChange", true);
if (!gArgs.SoftSetBoolArg("-spendzeroconfchange", settings.value("bSpendZeroConfChange").toBool()))
addOverriddenOption("-spendzeroconfchange");

if (!settings.contains("external_signer_path"))
settings.setValue("external_signer_path", "");

if (!gArgs.SoftSetArg("-signer", settings.value("external_signer_path").toString().toStdString())) {
addOverriddenOption("-signer");
}
#endif

// Network
Expand Down Expand Up @@ -326,6 +333,8 @@ QVariant OptionsModel::data(const QModelIndex & index, int role) const
#ifdef ENABLE_WALLET
case SpendZeroConfChange:
return settings.value("bSpendZeroConfChange");
case ExternalSignerPath:
return settings.value("external_signer_path");
#endif
case DisplayUnit:
return nDisplayUnit;
Expand Down Expand Up @@ -445,6 +454,12 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in
setRestartRequired(true);
}
break;
case ExternalSignerPath:
if (settings.value("external_signer_path") != value.toString()) {
settings.setValue("external_signer_path", value.toString());
setRestartRequired(true);
}
break;
#endif
case DisplayUnit:
setDisplayUnit(value);
Expand Down
1 change: 1 addition & 0 deletions src/qt/optionsmodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ class OptionsModel : public QAbstractListModel
Prune, // bool
PruneSize, // int
DatabaseCache, // int
ExternalSignerPath, // QString
SpendZeroConfChange, // bool
Listen, // bool
OptionIDRowCount,
Expand Down
3 changes: 3 additions & 0 deletions src/qt/walletcontroller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,9 @@ void CreateWalletActivity::createWallet()
if (m_create_wallet_dialog->isDescriptorWalletChecked()) {
flags |= WALLET_FLAG_DESCRIPTORS;
}
if (m_create_wallet_dialog->isExternalSignerChecked()) {
flags |= WALLET_FLAG_EXTERNAL_SIGNER;
}

QTimer::singleShot(500, worker(), [this, name, flags] {
std::unique_ptr<interfaces::Wallet> wallet = node().walletClient().createWallet(name, m_passphrase, flags, m_error_message, m_warning_message);
Expand Down