Skip to content

Address type dropdown, add Taproot (Receive tab) #459

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

Merged
merged 2 commits into from
Dec 22, 2021
Merged

Address type dropdown, add Taproot (Receive tab) #459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
56113da
wallet: add taprootEnabled() to interface
Sjors Oct 29, 2021
c9a77e2
gui: address type dropdown, add bech32m
Sjors Dec 8, 2021
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
4 changes: 4 additions & 0 deletions doc/release-notes-gui-459.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
GUI changes
-----------

- The Bech32 checkbox has been replaced with a dropdown for all address types, including the new Bech32m (BIP-350) standard for Taproot enabled wallets.
3 changes: 3 additions & 0 deletions src/interfaces/wallet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,9 @@ class Wallet
// Return whether private keys enabled.
virtual bool privateKeysDisabled() = 0;

// Return whether the wallet contains a Taproot scriptPubKeyMan
virtual bool taprootEnabled() = 0;

// Return whether wallet uses an external signer.
virtual bool hasExternalSigner() = 0;

Expand Down
10 changes: 2 additions & 8 deletions src/qt/forms/receivecoinsdialog.ui
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@
</widget>
</item>
<item>
<widget class="QCheckBox" name="useBech32">
<widget class="QComboBox" name="addressType">
<property name="sizePolicy">
<sizepolicy hsizetype="Fixed" vsizetype="Fixed">
<horstretch>0</horstretch>
Expand All @@ -211,12 +211,6 @@
<property name="focusPolicy">
<enum>Qt::StrongFocus</enum>
</property>
<property name="toolTip">
<string>Native segwit addresses (aka Bech32 or BIP-173) reduce your transaction fees later on and offer better protection against typos, but old wallets don't support them. When unchecked, an address compatible with older wallets will be created instead.</string>
</property>
<property name="text">
<string>Generate native segwit (Bech32) address</string>
</property>
</widget>
</item>
<item>
Expand Down Expand Up @@ -366,7 +360,7 @@
<tabstops>
<tabstop>reqLabel</tabstop>
<tabstop>reqAmount</tabstop>
<tabstop>useBech32</tabstop>
<tabstop>addressType</tabstop>
<tabstop>reqMessage</tabstop>
<tabstop>receiveButton</tabstop>
<tabstop>clearButton</tabstop>
Expand Down
26 changes: 13 additions & 13 deletions src/qt/receivecoinsdialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,18 @@ void ReceiveCoinsDialog::setModel(WalletModel *_model)
&QItemSelectionModel::selectionChanged, this,
&ReceiveCoinsDialog::recentRequestsView_selectionChanged);

if (model->wallet().getDefaultAddressType() == OutputType::BECH32) {
ui->useBech32->setCheckState(Qt::Checked);
} else {
ui->useBech32->setCheckState(Qt::Unchecked);
// Populate address type dropdown and select default
auto add_address_type = [&](OutputType type, const QString& text, const QString& tooltip) {
const auto index = ui->addressType->count();
ui->addressType->addItem(text, (int) type);
ui->addressType->setItemData(index, tooltip, Qt::ToolTipRole);
if (model->wallet().getDefaultAddressType() == type) ui->addressType->setCurrentIndex(index);
};
add_address_type(OutputType::LEGACY, "Base58 (Legacy)", "Not recommended due to higher fees and less protection against typos.");
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing a check (like the one for taproot)?

Locally, I am getting:

Screenshot from 2021-12-22 12-26-46

Copy link
Member Author

Choose a reason for hiding this comment

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

I assume you manually created a wallet without a pkh() descriptor? We could add checks for the existence of each output type to the interface and then remove them from the dropdown. However, if someone manually creates a wallet, they would also understand the error message, and it might even better than a missing item?

add_address_type(OutputType::P2SH_SEGWIT, "Base58 (P2SH-SegWit)", "Generates an address compatible with older wallets.");
add_address_type(OutputType::BECH32, "Bech32 (SegWit)", "Generates a native segwit address (BIP-173). Some old wallets don't support it.");
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe replace "wallets" with "services" or "third-party services" or "third-party wallets".

Otherwise the user might get confused into thinking that an old wallet.dat can't generate this, which is not true?

Copy link
Member Author

@Sjors Sjors Dec 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if they're more likely to think of an upgrade scenario than about receiving coins from another wallet they have? "third-party" is a bit of an tech industry term, and probably doesn't translate well. Paging @jonatack.

("other, old, wallets" is more clear perhaps, though maybe not pretty)

if (model->wallet().taprootEnabled()) {
add_address_type(OutputType::BECH32M, "Bech32m (Taproot)", "Bech32m (BIP-350) is an upgrade to Bech32, wallet support is still limited.");
}

// Set the button to be enabled or disabled based on whether the wallet can give out new addresses.
Expand Down Expand Up @@ -144,15 +152,7 @@ void ReceiveCoinsDialog::on_receiveButton_clicked()
QString address;
QString label = ui->reqLabel->text();
/* Generate new receiving address */
OutputType address_type;
if (ui->useBech32->isChecked()) {
address_type = OutputType::BECH32;
} else {
address_type = model->wallet().getDefaultAddressType();
if (address_type == OutputType::BECH32) {
address_type = OutputType::P2SH_SEGWIT;
}
}
const OutputType address_type = (OutputType)ui->addressType->currentData().toInt();
address = model->getAddressTableModel()->addRow(AddressTableModel::Receive, label, "", address_type);

switch(model->getAddressTableModel()->getEditStatus())
Expand Down
5 changes: 5 additions & 0 deletions src/wallet/interfaces.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,11 @@ class WalletImpl : public Wallet
bool canGetAddresses() override { return m_wallet->CanGetAddresses(); }
bool hasExternalSigner() override { return m_wallet->IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER); }
bool privateKeysDisabled() override { return m_wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS); }
bool taprootEnabled() override {
if (m_wallet->IsLegacy()) return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this special case needed? Wouldn't it return nullptr below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe not, but since legacy wallets don't have descriptors, it does seem cleaner.

auto spk_man = m_wallet->GetScriptPubKeyMan(OutputType::BECH32M, /*internal=*/false);
return spk_man != nullptr;
}
OutputType getDefaultAddressType() override { return m_wallet->m_default_address_type; }
CAmount getDefaultMaxTxFee() override { return m_wallet->m_default_max_tx_fee; }
void remove() override
Expand Down