From c5b277033a72650c221084ec0f1326623a810fd0 Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Tue, 10 Apr 2018 11:50:10 -0400 Subject: [PATCH 1/4] Add purpose arg to Wallet::getAddress Also make all arguments to getAddress required and document args at call sites. --- src/interfaces/wallet.cpp | 8 +++++++- src/interfaces/wallet.h | 5 +++-- src/qt/addresstablemodel.cpp | 6 ++++-- src/qt/transactiondesc.cpp | 10 ++++++---- src/qt/walletmodel.cpp | 3 ++- 5 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index f4dfdae8a..63b9d80a9 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -152,7 +152,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); @@ -165,6 +168,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 2a03f6c60..ff779cd0a 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -99,8 +99,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/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp index 1e3acd75c..d85a13cd5 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(); diff --git a/src/qt/transactiondesc.cpp b/src/qt/transactiondesc.cpp index f316c3ca4..2cb446c45 100644 --- a/src/qt/transactiondesc.cpp +++ b/src/qt/transactiondesc.cpp @@ -102,7 +102,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") + ": "; @@ -128,7 +128,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) + "
"; } @@ -196,7 +197,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) @@ -319,7 +321,7 @@ QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wall if (ExtractDestination(vout.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/walletmodel.cpp b/src/qt/walletmodel.cpp index 8f30a2a87..3418b1f1a 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -274,7 +274,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"); } From 8cdcaee4c7b256c5c3b70f1cfb04a5fb547311cd Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Tue, 10 Apr 2018 16:27:40 -0400 Subject: [PATCH 2/4] [qt] Display more helpful message when adding a send address has failed Addresses #12796. When we're unable to add a sending address to the address book because it already exists as a receiving address, display a message indicating as much. This should help avoid confusion about an address supposedly already in the book but which isn't currently visible in the interface. --- src/qt/addresstablemodel.cpp | 28 +++++++++++++++++++--------- src/qt/addresstablemodel.h | 9 +++++++-- src/qt/editaddressdialog.cpp | 21 ++++++++++++++++++++- src/qt/editaddressdialog.h | 3 +++ 4 files changed, 49 insertions(+), 12 deletions(-) diff --git a/src/qt/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp index d85a13cd5..f38e864b4 100644 --- a/src/qt/addresstablemodel.cpp +++ b/src/qt/addresstablemodel.cpp @@ -407,21 +407,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 destination = DecodeDestination(address.toStdString()); - std::string name; - if (walletModel->wallet().getAddress(destination, &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 d7aeda9d8..979f861fe 100644 --- a/src/qt/addresstablemodel.h +++ b/src/qt/addresstablemodel.h @@ -67,10 +67,12 @@ public: */ QString addRow(const QString &type, const QString &label, const QString &address, const OutputType address_type); - /* 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; + /** 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. */ @@ -86,6 +88,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 38411c499..f26a31158 100644 --- a/src/qt/editaddressdialog.cpp +++ b/src/qt/editaddressdialog.cpp @@ -109,7 +109,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: @@ -129,6 +129,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 41c5d1708..fb3d90077 100644 --- a/src/qt/editaddressdialog.h +++ b/src/qt/editaddressdialog.h @@ -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; From 9c01be1b85f8ad11a8a0826a4a2bcdc2668a5c1f Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Thu, 29 Mar 2018 10:56:04 -0400 Subject: [PATCH 3/4] [tests] [qt] Introduce qt/test/util with a generalized ConfirmMessage ConfirmMessage is reused in future tests apart from its single usage here. --- src/Makefile.qttest.include | 2 ++ src/qt/test/util.cpp | 22 ++++++++++++++++++++++ src/qt/test/util.h | 12 ++++++++++++ src/qt/test/wallettests.cpp | 18 ++---------------- 4 files changed, 38 insertions(+), 16 deletions(-) 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 127915219..473693faa 100644 --- a/src/Makefile.qttest.include +++ b/src/Makefile.qttest.include @@ -20,6 +20,7 @@ TEST_QT_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/wallettests.h @@ -38,6 +39,7 @@ qt_test_test_bitcoin_qt_SOURCES = \ qt/test/rpcnestedtests.cpp \ qt/test/test_main.cpp \ qt/test/uritests.cpp \ + qt/test/util.cpp \ $(TEST_QT_H) \ $(TEST_BITCOIN_CPP) \ $(TEST_BITCOIN_H) diff --git a/src/qt/test/util.cpp b/src/qt/test/util.cpp new file mode 100644 index 000000000..261caaaee --- /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 000000000..324386c13 --- /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 56d2d3819..a09d98dfe 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -1,4 +1,5 @@ #include +#include #include #include @@ -35,21 +36,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) { @@ -264,7 +250,7 @@ void TestGUI() QCOMPARE(requestTableModel->rowCount({}), currentRowCount-1); } -} +} // namespace void WalletTests::walletTests() { From 5109fc4a9cb2cbd73c33197fb9129e1413ab051b Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Thu, 29 Mar 2018 10:59:57 -0400 Subject: [PATCH 4/4] [tests] [qt] Add tests for address book manipulation via EditAddressDialog Also modifies corresponding QT code to allow for use within test cases. --- src/Makefile.qttest.include | 3 + src/qt/addressbookpage.h | 2 +- src/qt/editaddressdialog.h | 2 +- src/qt/test/addressbooktests.cpp | 143 +++++++++++++++++++++++++++++++ src/qt/test/addressbooktests.h | 15 ++++ src/qt/test/test_main.cpp | 5 ++ src/qt/walletmodel.h | 2 + 7 files changed, 170 insertions(+), 2 deletions(-) create mode 100644 src/qt/test/addressbooktests.cpp create mode 100644 src/qt/test/addressbooktests.h diff --git a/src/Makefile.qttest.include b/src/Makefile.qttest.include index 473693faa..4b14212b2 100644 --- a/src/Makefile.qttest.include +++ b/src/Makefile.qttest.include @@ -12,11 +12,13 @@ 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 \ @@ -45,6 +47,7 @@ qt_test_test_bitcoin_qt_SOURCES = \ $(TEST_BITCOIN_H) if ENABLE_WALLET qt_test_test_bitcoin_qt_SOURCES += \ + qt/test/addressbooktests.cpp \ qt/test/paymentservertests.cpp \ qt/test/wallettests.cpp \ wallet/test/wallet_test_fixture.cpp diff --git a/src/qt/addressbookpage.h b/src/qt/addressbookpage.h index 8877d0733..ba420c5e1 100644 --- a/src/qt/addressbookpage.h +++ b/src/qt/addressbookpage.h @@ -38,7 +38,7 @@ public: ForEditing /**< Open address book for editing */ }; - explicit AddressBookPage(const PlatformStyle *platformStyle, Mode mode, Tabs tab, QWidget *parent); + explicit AddressBookPage(const PlatformStyle *platformStyle, Mode mode, Tabs tab, QWidget *parent = 0); ~AddressBookPage(); void setModel(AddressTableModel *model); diff --git a/src/qt/editaddressdialog.h b/src/qt/editaddressdialog.h index fb3d90077..3aba74bf0 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); diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp new file mode 100644 index 000000000..0c2e7ae71 --- /dev/null +++ b/src/qt/test/addressbooktests.cpp @@ -0,0 +1,143 @@ +#include +#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; + CWallet wallet("mock", WalletDatabase::CreateMock()); + bool firstRun; + wallet.LoadWallet(firstRun); + + auto build_address = [&wallet]() { + CKey key; + key.MakeNewKey(true); + CTxDestination dest(GetDestinationForKey( + key.GetPubKey(), wallet.m_default_address_type)); + + 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. + std::unique_ptr platformStyle(PlatformStyle::instantiate("other")); + auto node = interfaces::MakeNode(); + OptionsModel optionsModel(*node); + AddWallet(&wallet); + WalletModel walletModel(std::move(node->getWallets()[0]), *node, platformStyle.get(), &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 000000000..beeb9e76a --- /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 25ee66dc6..56d4d3e45 100644 --- a/src/qt/test/test_main.cpp +++ b/src/qt/test/test_main.cpp @@ -13,6 +13,7 @@ #include #ifdef ENABLE_WALLET +#include #include #include #endif @@ -99,6 +100,10 @@ int main(int argc, char *argv[]) if (QTest::qExec(&test5) != 0) { fInvalid = true; } + AddressBookTests test6; + if (QTest::qExec(&test6) != 0) { + fInvalid = true; + } #endif fs::remove_all(pathTemp); diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index e5ed5b4e8..9173fcae5 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -204,6 +204,8 @@ public: QString getWalletName() const; bool isMultiwallet(); + + AddressTableModel* getAddressTableModel() const { return addressTableModel; } private: std::unique_ptr m_wallet; std::unique_ptr m_handler_status_changed;