From dddd5c7f06562be684206e0e684b4014dfc91b29 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Sun, 23 May 2021 01:24:26 +0300 Subject: [PATCH] Merge #12830: [qt] [tests] Clarify address book error messages, add tests 5109fc4 [tests] [qt] Add tests for address book manipulation via EditAddressDialog (James O'Beirne) 9c01be1 [tests] [qt] Introduce qt/test/util with a generalized ConfirmMessage (James O'Beirne) 8cdcaee [qt] Display more helpful message when adding a send address has failed (James O'Beirne) c5b2770 Add purpose arg to Wallet::getAddress (James O'Beirne) Pull request description: Addresses https://github.com/bitcoin/bitcoin/issues/12796. When a user attempts to add to the address book a sending address which is already present as a receiving address, they're presented with a confusing error indicating the address is already present in the book, despite the fact that this row is currently invisible. ![selection_011](https://user-images.githubusercontent.com/73197/38096704-8a2948d2-3341-11e8-9632-7d563201f28c.jpg) This change adds a more specific error message indicating its existence as a receiving address (as discussed in the linked issue). ![selection_016](https://user-images.githubusercontent.com/73197/38198467-fa26164e-365a-11e8-8fc5-ddab9caf2fbd.jpg) This change also adds some tests exercising use of the address book via QT. Adding so much test code for such a trivial change may seem weird, but it's my hope that this will make further test-writing for address book usage (and other QT features) more approachable. Tree-SHA512: fbdd5564f7a9a2380bbe437f3378e8d4d5fd9201efff4879b72bc23f2cc1c2eecaf2b811994c25070ee052422e48e47901787c2e62cc584774a997fe6a2a327a --- src/Makefile.qttest.include | 7 +- src/interfaces/wallet.cpp | 8 +- src/interfaces/wallet.h | 5 +- src/qt/addressbookpage.h | 2 +- src/qt/addresstablemodel.cpp | 40 +++++---- src/qt/addresstablemodel.h | 10 ++- src/qt/editaddressdialog.cpp | 21 ++++- src/qt/editaddressdialog.h | 5 +- src/qt/test/addressbooktests.cpp | 140 +++++++++++++++++++++++++++++++ src/qt/test/addressbooktests.h | 15 ++++ src/qt/test/test_main.cpp | 9 +- src/qt/test/util.cpp | 22 +++++ src/qt/test/util.h | 12 +++ src/qt/test/wallettests.cpp | 18 +--- src/qt/transactiondesc.cpp | 10 ++- src/qt/transactionrecord.cpp | 2 +- src/qt/walletmodel.cpp | 3 +- src/qt/walletmodel.h | 2 + 18 files changed, 280 insertions(+), 51 deletions(-) create mode 100644 src/qt/test/addressbooktests.cpp create mode 100644 src/qt/test/addressbooktests.h create mode 100644 src/qt/test/util.cpp create mode 100644 src/qt/test/util.h diff --git a/src/Makefile.qttest.include b/src/Makefile.qttest.include index c01ac37d2e..5de268eb51 100644 --- a/src/Makefile.qttest.include +++ b/src/Makefile.qttest.include @@ -14,14 +14,17 @@ TEST_QT_MOC_CPP = \ if ENABLE_WALLET TEST_QT_MOC_CPP += \ + qt/test/moc_addressbooktests.cpp \ qt/test/moc_paymentservertests.cpp \ qt/test/moc_wallettests.cpp endif TEST_QT_H = \ + qt/test/addressbooktests.h \ qt/test/compattests.h \ qt/test/rpcnestedtests.h \ qt/test/uritests.h \ + qt/test/util.h \ qt/test/paymentrequestdata.h \ qt/test/paymentservertests.h \ qt/test/trafficgraphdatatests.h \ @@ -39,13 +42,15 @@ qt_test_test_dash_qt_SOURCES = \ qt/test/compattests.cpp \ qt/test/rpcnestedtests.cpp \ qt/test/test_main.cpp \ - qt/test/uritests.cpp \ qt/test/trafficgraphdatatests.cpp \ + qt/test/uritests.cpp \ + qt/test/util.cpp \ $(TEST_QT_H) \ $(TEST_BITCOIN_CPP) \ $(TEST_BITCOIN_H) if ENABLE_WALLET qt_test_test_dash_qt_SOURCES += \ + qt/test/addressbooktests.cpp \ qt/test/paymentservertests.cpp \ qt/test/wallettests.cpp \ wallet/test/wallet_test_fixture.cpp diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index ecbff4b5f8..359b975407 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -222,7 +222,10 @@ public: { return m_wallet.DelAddressBook(dest); } - bool getAddress(const CTxDestination& dest, std::string* name, isminetype* is_mine) override + bool getAddress(const CTxDestination& dest, + std::string* name, + isminetype* is_mine, + std::string* purpose) override { LOCK(m_wallet.cs_wallet); auto it = m_wallet.mapAddressBook.find(dest); @@ -235,6 +238,9 @@ public: if (is_mine) { *is_mine = IsMine(m_wallet, dest); } + if (purpose) { + *purpose = it->second.purpose; + } return true; } std::vector getAddresses() override diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 17ebb7c110..bde3f7ccba 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -127,8 +127,9 @@ public: //! Look up address in wallet, return whether exists. virtual bool getAddress(const CTxDestination& dest, - std::string* name = nullptr, - isminetype* is_mine = nullptr) = 0; + std::string* name, + isminetype* is_mine, + std::string* purpose) = 0; //! Get wallet address list. virtual std::vector getAddresses() = 0; diff --git a/src/qt/addressbookpage.h b/src/qt/addressbookpage.h index a8a8242a59..4d536f33f7 100644 --- a/src/qt/addressbookpage.h +++ b/src/qt/addressbookpage.h @@ -37,7 +37,7 @@ public: ForEditing /**< Open address book for editing */ }; - explicit AddressBookPage(Mode mode, Tabs tab, QWidget* parent); + explicit AddressBookPage(Mode mode, Tabs tab, QWidget* parent = 0); ~AddressBookPage(); void setModel(AddressTableModel *model); diff --git a/src/qt/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp index cb164227a2..29cf7806f3 100644 --- a/src/qt/addresstablemodel.cpp +++ b/src/qt/addresstablemodel.cpp @@ -266,7 +266,8 @@ bool AddressTableModel::setData(const QModelIndex &index, const QVariant &value, } // Check for duplicate addresses to prevent accidental deletion of addresses, if you try // to paste an existing address over another address (with a different label) - if (walletModel->wallet().getAddress(newAddress)) + if (walletModel->wallet().getAddress( + newAddress, /* name= */ nullptr, /* is_mine= */ nullptr, /* purpose= */ nullptr)) { editStatus = DUPLICATE_ADDRESS; return false; @@ -351,7 +352,8 @@ QString AddressTableModel::addRow(const QString &type, const QString &label, con } // Check for duplicate addresses { - if(walletModel->wallet().getAddress(DecodeDestination(strAddress))) + if (walletModel->wallet().getAddress( + DecodeDestination(strAddress), /* name= */ nullptr, /* is_mine= */ nullptr, /* purpose= */ nullptr)) { editStatus = DUPLICATE_ADDRESS; return QString(); @@ -404,27 +406,31 @@ bool AddressTableModel::removeRows(int row, int count, const QModelIndex &parent return true; } -/* Look up label for address in address book, if not found return empty string. - */ QString AddressTableModel::labelForAddress(const QString &address) const { - CTxDestination dest = DecodeDestination(address.toStdString()); - return labelForDestination(dest); -} - - -QString AddressTableModel::labelForDestination(const CTxDestination &dest) const -{ - { - std::string name; - if (walletModel->wallet().getAddress(dest, &name)) - { - return QString::fromStdString(name); - } + std::string name; + if (getAddressData(address, &name, /* purpose= */ nullptr)) { + return QString::fromStdString(name); } return QString(); } +QString AddressTableModel::purposeForAddress(const QString &address) const +{ + std::string purpose; + if (getAddressData(address, /* name= */ nullptr, &purpose)) { + return QString::fromStdString(purpose); + } + return QString(); +} + +bool AddressTableModel::getAddressData(const QString &address, + std::string* name, + std::string* purpose) const { + CTxDestination destination = DecodeDestination(address.toStdString()); + return walletModel->wallet().getAddress(destination, name, /* is_mine= */ nullptr, purpose); +} + int AddressTableModel::lookupAddress(const QString &address) const { QModelIndexList lst = match(index(0, Address, QModelIndex()), diff --git a/src/qt/addresstablemodel.h b/src/qt/addresstablemodel.h index 32c46b57e0..39cef1514e 100644 --- a/src/qt/addresstablemodel.h +++ b/src/qt/addresstablemodel.h @@ -67,10 +67,11 @@ public: */ QString addRow(const QString &type, const QString &label, const QString &address); - /* Look up label for address in address book, if not found return empty string. - */ + /** Look up label for address in address book, if not found return empty string. */ QString labelForAddress(const QString &address) const; - QString labelForDestination(const CTxDestination &dest) const; + + /** Look up purpose for address in address book, if not found return empty string. */ + QString purposeForAddress(const QString &address) const; /* Look up row index of an address in the model. Return -1 if not found. @@ -85,6 +86,9 @@ private: QStringList columns; EditStatus editStatus; + /** Look up address book data given an address string. */ + bool getAddressData(const QString &address, std::string* name, std::string* purpose) const; + /** Notify listeners that data changed. */ void emitDataChanged(int index); diff --git a/src/qt/editaddressdialog.cpp b/src/qt/editaddressdialog.cpp index 9811894cae..78293d25d1 100644 --- a/src/qt/editaddressdialog.cpp +++ b/src/qt/editaddressdialog.cpp @@ -114,7 +114,7 @@ void EditAddressDialog::accept() break; case AddressTableModel::DUPLICATE_ADDRESS: QMessageBox::warning(this, windowTitle(), - tr("The entered address \"%1\" is already in the address book.").arg(ui->addressEdit->text()), + getDuplicateAddressWarning(), QMessageBox::Ok, QMessageBox::Ok); break; case AddressTableModel::WALLET_UNLOCK_FAILURE: @@ -134,6 +134,25 @@ void EditAddressDialog::accept() QDialog::accept(); } +QString EditAddressDialog::getDuplicateAddressWarning() const +{ + QString dup_address = ui->addressEdit->text(); + QString existing_label = model->labelForAddress(dup_address); + QString existing_purpose = model->purposeForAddress(dup_address); + + if (existing_purpose == "receive" && + (mode == NewSendingAddress || mode == EditSendingAddress)) { + return tr( + "Address \"%1\" already exists as a receiving address with label " + "\"%2\" and so cannot be added as a sending address." + ).arg(dup_address).arg(existing_label); + } + return tr( + "The entered address \"%1\" is already in the address book with " + "label \"%2\"." + ).arg(dup_address).arg(existing_label); +} + QString EditAddressDialog::getAddress() const { return address; diff --git a/src/qt/editaddressdialog.h b/src/qt/editaddressdialog.h index 41c5d1708a..3aba74bf08 100644 --- a/src/qt/editaddressdialog.h +++ b/src/qt/editaddressdialog.h @@ -30,7 +30,7 @@ public: EditSendingAddress }; - explicit EditAddressDialog(Mode mode, QWidget *parent); + explicit EditAddressDialog(Mode mode, QWidget *parent = 0); ~EditAddressDialog(); void setModel(AddressTableModel *model); @@ -45,6 +45,9 @@ public Q_SLOTS: private: bool saveCurrentRow(); + /** Return a descriptive string when adding an already-existing address fails. */ + QString getDuplicateAddressWarning() const; + Ui::EditAddressDialog *ui; QDataWidgetMapper *mapper; Mode mode; diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp new file mode 100644 index 0000000000..5880a02040 --- /dev/null +++ b/src/qt/test/addressbooktests.cpp @@ -0,0 +1,140 @@ +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include + +#include +#include + +namespace +{ + +/** + * Fill the edit address dialog box with data, submit it, and ensure that + * the resulting message meets expectations. + */ +void EditAddressAndSubmit( + EditAddressDialog* dialog, + const QString& label, const QString& address, QString expected_msg) +{ + QString warning_text; + + dialog->findChild("labelEdit")->setText(label); + dialog->findChild("addressEdit")->setText(address); + + ConfirmMessage(&warning_text, 5); + dialog->accept(); + QCOMPARE(warning_text, expected_msg); +} + +/** + * Test adding various send addresses to the address book. + * + * There are three cases tested: + * + * - new_address: a new address which should add as a send address successfully. + * - existing_s_address: an existing sending address which won't add successfully. + * - existing_r_address: an existing receiving address which won't add successfully. + * + * In each case, verify the resulting state of the address book and optionally + * the warning message presented to the user. + */ +void TestAddAddressesToSendBook() +{ + TestChain100Setup test; + std::shared_ptr wallet = std::make_shared(WalletLocation(), WalletDatabase::CreateMock()); + bool firstRun; + wallet->LoadWallet(firstRun); + + auto build_address = [wallet]() { + CKey key; + key.MakeNewKey(true); + CTxDestination dest = key.GetPubKey().GetID(); + + return std::make_pair(dest, QString::fromStdString(EncodeDestination(dest))); + }; + + CTxDestination r_key_dest, s_key_dest; + + // Add a preexisting "receive" entry in the address book. + QString preexisting_r_address; + QString r_label("already here (r)"); + + // Add a preexisting "send" entry in the address book. + QString preexisting_s_address; + QString s_label("already here (s)"); + + // Define a new address (which should add to the address book successfully). + QString new_address; + + std::tie(r_key_dest, preexisting_r_address) = build_address(); + std::tie(s_key_dest, preexisting_s_address) = build_address(); + std::tie(std::ignore, new_address) = build_address(); + + { + LOCK(wallet->cs_wallet); + wallet->SetAddressBook(r_key_dest, r_label.toStdString(), "receive"); + wallet->SetAddressBook(s_key_dest, s_label.toStdString(), "send"); + } + + auto check_addbook_size = [wallet](int expected_size) { + QCOMPARE(static_cast(wallet->mapAddressBook.size()), expected_size); + }; + + // We should start with the two addresses we added earlier and nothing else. + check_addbook_size(2); + + // Initialize relevant QT models. + auto node = interfaces::MakeNode(); + OptionsModel optionsModel(*node); + AddWallet(wallet); + WalletModel walletModel(std::move(node->getWallets()[0]), *node, &optionsModel); + RemoveWallet(wallet); + EditAddressDialog editAddressDialog(EditAddressDialog::NewSendingAddress); + editAddressDialog.setModel(walletModel.getAddressTableModel()); + + EditAddressAndSubmit( + &editAddressDialog, QString("uhoh"), preexisting_r_address, + QString( + "Address \"%1\" already exists as a receiving address with label " + "\"%2\" and so cannot be added as a sending address." + ).arg(preexisting_r_address).arg(r_label)); + + check_addbook_size(2); + + EditAddressAndSubmit( + &editAddressDialog, QString("uhoh, different"), preexisting_s_address, + QString( + "The entered address \"%1\" is already in the address book with " + "label \"%2\"." + ).arg(preexisting_s_address).arg(s_label)); + + check_addbook_size(2); + + // Submit a new address which should add successfully - we expect the + // warning message to be blank. + EditAddressAndSubmit( + &editAddressDialog, QString("new"), new_address, QString("")); + + check_addbook_size(3); +} + +} // namespace + +void AddressBookTests::addressBookTests() +{ + TestAddAddressesToSendBook(); +} diff --git a/src/qt/test/addressbooktests.h b/src/qt/test/addressbooktests.h new file mode 100644 index 0000000000..beeb9e76a9 --- /dev/null +++ b/src/qt/test/addressbooktests.h @@ -0,0 +1,15 @@ +#ifndef BITCOIN_QT_TEST_ADDRESSBOOKTESTS_H +#define BITCOIN_QT_TEST_ADDRESSBOOKTESTS_H + +#include +#include + +class AddressBookTests : public QObject +{ + Q_OBJECT + +private Q_SLOTS: + void addressBookTests(); +}; + +#endif // BITCOIN_QT_TEST_ADDRESSBOOKTESTS_H diff --git a/src/qt/test/test_main.cpp b/src/qt/test/test_main.cpp index c29669dc36..2e9750ce81 100644 --- a/src/qt/test/test_main.cpp +++ b/src/qt/test/test_main.cpp @@ -15,6 +15,7 @@ #include #ifdef ENABLE_WALLET +#include #include #include #endif @@ -94,10 +95,14 @@ int main(int argc, char *argv[]) if (QTest::qExec(&test5) != 0) { fInvalid = true; } + AddressBookTests test6; + if (QTest::qExec(&test6) != 0) { + fInvalid = true; + } #endif - TrafficGraphDataTests test6; - if (QTest::qExec(&test6) != 0) + TrafficGraphDataTests test7; + if (QTest::qExec(&test7) != 0) fInvalid = true; return fInvalid; } diff --git a/src/qt/test/util.cpp b/src/qt/test/util.cpp new file mode 100644 index 0000000000..261caaaee5 --- /dev/null +++ b/src/qt/test/util.cpp @@ -0,0 +1,22 @@ +#include + +#include +#include +#include +#include +#include +#include + +void ConfirmMessage(QString* text, int msec) +{ + QTimer::singleShot(msec, makeCallback([text](Callback* callback) { + for (QWidget* widget : QApplication::topLevelWidgets()) { + if (widget->inherits("QMessageBox")) { + QMessageBox* messageBox = qobject_cast(widget); + if (text) *text = messageBox->text(); + messageBox->defaultButton()->click(); + } + } + delete callback; + }), SLOT(call())); +} diff --git a/src/qt/test/util.h b/src/qt/test/util.h new file mode 100644 index 0000000000..324386c139 --- /dev/null +++ b/src/qt/test/util.h @@ -0,0 +1,12 @@ +#ifndef BITCOIN_QT_TEST_UTIL_H +#define BITCOIN_QT_TEST_UTIL_H + +/** + * Press "Ok" button in message box dialog. + * + * @param text - Optionally store dialog text. + * @param msec - Number of miliseconds to pause before triggering the callback. + */ +void ConfirmMessage(QString* text = nullptr, int msec = 0); + +#endif // BITCOIN_QT_TEST_UTIL_H diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 6c624e935b..c370e90f1c 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -1,4 +1,5 @@ #include +#include #include #include @@ -36,21 +37,6 @@ namespace { -//! Press "Ok" button in message box dialog. -void ConfirmMessage(QString* text = nullptr) -{ - QTimer::singleShot(0, makeCallback([text](Callback* callback) { - for (QWidget* widget : QApplication::topLevelWidgets()) { - if (widget->inherits("QMessageBox")) { - QMessageBox* messageBox = qobject_cast(widget); - if (text) *text = messageBox->text(); - messageBox->defaultButton()->click(); - } - } - delete callback; - }), SLOT(call())); -} - //! Press "Yes" or "Cancel" buttons in modal send confirmation dialog. void ConfirmSend(QString* text = nullptr, bool cancel = false) { @@ -219,7 +205,7 @@ void TestGUI() RemoveWallet(wallet); } -} +} // namespace void WalletTests::walletTests() { diff --git a/src/qt/transactiondesc.cpp b/src/qt/transactiondesc.cpp index 2c49f0dead..92837103b4 100644 --- a/src/qt/transactiondesc.cpp +++ b/src/qt/transactiondesc.cpp @@ -104,7 +104,7 @@ QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wall if (IsValidDestination(address)) { std::string name; isminetype ismine; - if (wallet.getAddress(address, &name, &ismine)) + if (wallet.getAddress(address, &name, &ismine, /* purpose= */ nullptr)) { strHTML += "" + tr("From") + ": " + tr("unknown") + "
"; strHTML += "" + tr("To") + ": "; @@ -130,7 +130,8 @@ QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wall strHTML += "" + tr("To") + ": "; CTxDestination dest = DecodeDestination(strAddress); std::string name; - if (wallet.getAddress(dest, &name) && !name.empty()) + if (wallet.getAddress( + dest, &name, /* is_mine= */ nullptr, /* purpose= */ nullptr) && !name.empty()) strHTML += GUIUtil::HtmlEscape(name) + " "; strHTML += GUIUtil::HtmlEscape(strAddress) + "
"; } @@ -198,7 +199,8 @@ QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wall { strHTML += "" + tr("To") + ": "; std::string name; - if (wallet.getAddress(address, &name) && !name.empty()) + if (wallet.getAddress( + address, &name, /* is_mine= */ nullptr, /* purpose= */ nullptr) && !name.empty()) strHTML += GUIUtil::HtmlEscape(name) + " "; strHTML += GUIUtil::HtmlEscape(EncodeDestination(address)); if(toSelf == ISMINE_SPENDABLE) @@ -320,7 +322,7 @@ QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wall if (ExtractDestination(txout.scriptPubKey, address)) { std::string name; - if (wallet.getAddress(address, &name) && !name.empty()) + if (wallet.getAddress(address, &name, /* is_mine= */ nullptr, /* purpose= */ nullptr) && !name.empty()) strHTML += GUIUtil::HtmlEscape(name) + " "; strHTML += QString::fromStdString(EncodeDestination(address)); } diff --git a/src/qt/transactionrecord.cpp b/src/qt/transactionrecord.cpp index 4a98143ffb..9f3e3e14ed 100644 --- a/src/qt/transactionrecord.cpp +++ b/src/qt/transactionrecord.cpp @@ -337,7 +337,7 @@ void TransactionRecord::updateLabel(interfaces::Wallet& wallet) { if (IsValidDestination(txDest)) { std::string name; - if (wallet.getAddress(txDest, &name)) { + if (wallet.getAddress(txDest, &name, /* is_mine= */ nullptr, /* purpose= */ nullptr)) { label = QString::fromStdString(name); } else { label = ""; diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 0b7a5cd804..774e375548 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -319,7 +319,8 @@ WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction &tran { // Check if we have a new address or an updated label std::string name; - if (!m_wallet->getAddress(dest, &name)) + if (!m_wallet->getAddress( + dest, &name, /* is_mine= */ nullptr, /* purpose= */ nullptr)) { m_wallet->setAddressBook(dest, strLabel, "send"); } diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index 32be8f665c..ab40489b82 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -212,6 +212,8 @@ public: QString getWalletName() const; bool isMultiwallet(); + + AddressTableModel* getAddressTableModel() const { return addressTableModel; } private: std::unique_ptr m_wallet; std::unique_ptr m_handler_unload;