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

@@ -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

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<WalletAddress> getAddresses() override

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<WalletAddress> getAddresses() = 0;

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);

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()),

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);
 

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;

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;

src/qt/test/addressbooktests.cpp

@@ -0,0 +1,140 @@
+#include <qt/test/addressbooktests.h>
+#include <qt/test/util.h>
+#include <test/test_dash.h>
+
+#include <interfaces/node.h>
+#include <qt/addressbookpage.h>
+#include <qt/addresstablemodel.h>
+#include <qt/editaddressdialog.h>
+#include <qt/callback.h>
+#include <qt/optionsmodel.h>
+#include <qt/qvalidatedlineedit.h>
+#include <qt/walletmodel.h>
+
+#include <key.h>
+#include <pubkey.h>
+#include <key_io.h>
+#include <wallet/wallet.h>
+
+#include <QTimer>
+#include <QMessageBox>
+
+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<QLineEdit*>("labelEdit")->setText(label);
+    dialog->findChild<QValidatedLineEdit*>("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<CWallet> wallet = std::make_shared<CWallet>(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<int>(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();
+}

src/qt/test/addressbooktests.h

@@ -0,0 +1,15 @@
+#ifndef BITCOIN_QT_TEST_ADDRESSBOOKTESTS_H
+#define BITCOIN_QT_TEST_ADDRESSBOOKTESTS_H
+
+#include <QObject>
+#include <QTest>
+
+class AddressBookTests : public QObject
+{
+    Q_OBJECT
+
+private Q_SLOTS:
+    void addressBookTests();
+};
+
+#endif // BITCOIN_QT_TEST_ADDRESSBOOKTESTS_H

src/qt/test/test_main.cpp

@@ -15,6 +15,7 @@
 #include <qt/test/trafficgraphdatatests.h>
 
 #ifdef ENABLE_WALLET
+#include <qt/test/addressbooktests.h>
 #include <qt/test/paymentservertests.h>
 #include <qt/test/wallettests.h>
 #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;
 }

src/qt/test/util.cpp

@@ -0,0 +1,22 @@
+#include <qt/callback.h>
+
+#include <QApplication>
+#include <QMessageBox>
+#include <QTimer>
+#include <QString>
+#include <QPushButton>
+#include <QWidget>
+
+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<QMessageBox*>(widget);
+                if (text) *text = messageBox->text();
+                messageBox->defaultButton()->click();
+            }
+        }
+        delete callback;
+    }), SLOT(call()));
+}

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

src/qt/test/wallettests.cpp

@@ -1,4 +1,5 @@
 #include <qt/test/wallettests.h>
+#include <qt/test/util.h>
 
 #include <coinjoin/coinjoin-client.h>
 #include <interfaces/node.h>
@@ -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<QMessageBox*>(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()
 {

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 += "<b>" + tr("From") + ":</b> " + tr("unknown") + "<br>";
                     strHTML += "<b>" + tr("To") + ":</b> ";
@@ -130,7 +130,8 @@ QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wall
         strHTML += "<b>" + tr("To") + ":</b> ";
         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) + "<br>";
     }
@@ -198,7 +199,8 @@ QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wall
                     {
                         strHTML += "<b>" + tr("To") + ":</b> ";
                         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));
                     }

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 = "";

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");
                 }

src/qt/walletmodel.h

@@ -212,6 +212,8 @@ public:
     QString getWalletName() const;
 
     bool isMultiwallet();
+
+    AddressTableModel* getAddressTableModel() const { return addressTableModel; }
 private:
     std::unique_ptr<interfaces::Wallet> m_wallet;
     std::unique_ptr<interfaces::Handler> m_handler_unload;