Merge bitcoin#27217: wallet: Replace use of purpose strings with an enum

18fc71a doc: Release note for purpose string restriction (Andrew Chow)
e83babe wallet: Replace use of purpose strings with an enum (Andrew Chow)
2f80005 wallet: add AddressPurpose enum to replace string values (Ryan Ofsky)
8741522 wallet: Add wallet/types.h for simple public enum and struct types (Ryan Ofsky)

Pull request description:

  Instead of storing and passing around fixed strings for the purpose of an address, use an enum.

ACKs for top commit:
  josibake:
    reACK bitcoin@18fc71a

Tree-SHA512: 82034f020e96b99b29da34dfdd7cfe58f8b7d2afed1409ea4a290c2cac69fc43e449e8b7b2afd874a9facf8f4cd6ebb80d17462317e60a6f011ed8f9eab5d4c5

Author: fanquake

doc/release-notes-27217.md

@@ -0,0 +1,6 @@
+Wallet
+------
+
+* Address Purposes strings are now restricted to the currently known values of "send", "receive", and "refund".
+  Wallets that have unrecognized purpose strings will have loading warnings, and the `listlabels`
+  RPC will raise an error if an unrecognized purpose is requested.

src/Makefile.am

@@ -399,7 +399,6 @@ BITCOIN_CORE_H = \
   wallet/dump.h \
   wallet/fees.h \
   wallet/hdchain.h \
-  wallet/ismine.h \
   wallet/load.h \
   wallet/receive.h \
   wallet/rpcwallet.h \
@@ -409,6 +408,7 @@ BITCOIN_CORE_H = \
   wallet/spend.h \
   wallet/sqlite.h \
   wallet/transaction.h \
+  wallet/types.h \
   wallet/wallet.h \
   wallet/walletdb.h \
   wallet/wallettool.h \

src/interfaces/wallet.h

@@ -33,11 +33,11 @@ class CWallet;
 class UniValue;
 enum class FeeReason;
 enum class TransactionError;
+enum class AddressPurpose;
 enum isminetype : unsigned int;
 struct CRecipient;
 struct NodeContext;
 struct PartiallySignedTransaction;
-struct WalletContext;
 struct bilingual_str;
 using isminefilter = std::underlying_type<isminetype>::type;
 
@@ -122,7 +122,7 @@ class Wallet
     virtual bool haveWatchOnly() = 0;
 
     //! Add or update address.
-    virtual bool setAddressBook(const CTxDestination& dest, const std::string& name, const std::string& purpose) = 0;
+    virtual bool setAddressBook(const CTxDestination& dest, const std::string& name, const std::optional<wallet::AddressPurpose>& purpose) = 0;
 
     // Remove address.
     virtual bool delAddressBook(const CTxDestination& dest) = 0;
@@ -131,7 +131,7 @@ class Wallet
     virtual bool getAddress(const CTxDestination& dest,
         std::string* name,
         isminetype* is_mine,
-        std::string* purpose) = 0;
+        wallet::AddressPurpose* purpose) = 0;
 
     //! Get wallet address list.
     virtual std::vector<WalletAddress> getAddresses() = 0;
@@ -305,7 +305,7 @@ class Wallet
     using AddressBookChangedFn = std::function<void(const CTxDestination& address,
         const std::string& label,
         bool is_mine,
-        const std::string& purpose,
+        wallet::AddressPurpose purpose,
         ChangeType status)>;
     virtual std::unique_ptr<Handler> handleAddressBookChanged(AddressBookChangedFn fn) = 0;
 
@@ -375,11 +375,11 @@ struct WalletAddress
 {
     CTxDestination dest;
     isminetype is_mine;
+    wallet::AddressPurpose purpose;
     std::string name;
-    std::string purpose;
 
-    WalletAddress(CTxDestination dest, isminetype is_mine, std::string name, std::string purpose)
-        : dest(std::move(dest)), is_mine(is_mine), name(std::move(name)), purpose(std::move(purpose))
+    WalletAddress(CTxDestination dest, isminetype is_mine, wallet::AddressPurpose purpose, std::string name)
+        : dest(std::move(dest)), is_mine(is_mine), purpose(std::move(purpose)), name(std::move(name))
     {
     }
 };

src/qt/addresstablemodel.cpp

@@ -9,7 +9,7 @@
 #include <qt/walletmodel.h>
 
 #include <key_io.h>
-
+#include <wallet/types.h>
 #include <algorithm>
 
 #include <QFont>
@@ -52,17 +52,16 @@ struct AddressTableEntryLessThan
 };
 
 /* Determine address type from address purpose */
-static AddressTableEntry::Type translateTransactionType(const QString &strPurpose, bool isMine)
+static AddressTableEntry::Type translateTransactionType(wallet::AddressPurpose purpose, bool isMine)
 {
-    AddressTableEntry::Type addressType = AddressTableEntry::Hidden;
     // "refund" addresses aren't shown, and change addresses aren't returned by getAddresses at all.
-    if (strPurpose == "send")
-        addressType = AddressTableEntry::Sending;
-    else if (strPurpose == "receive")
-        addressType = AddressTableEntry::Receiving;
-    else if (strPurpose == "unknown" || strPurpose == "") // if purpose not set, guess
-        addressType = (isMine ? AddressTableEntry::Receiving : AddressTableEntry::Sending);
-    return addressType;
+    switch (purpose) {
+    case wallet::AddressPurpose::SEND: return AddressTableEntry::Sending;
+    case wallet::AddressPurpose::RECEIVE: return AddressTableEntry::Receiving;
+    case wallet::AddressPurpose::REFUND: return AddressTableEntry::Hidden;
+    // No default case to allow for compiler to warn
+    }
+    assert(false);
 }
 
 // Private implementation
@@ -82,7 +81,7 @@ class AddressTablePriv
             for (const auto& address : wallet.getAddresses())
             {
                 AddressTableEntry::Type addressType = translateTransactionType(
-                        QString::fromStdString(address.purpose), address.is_mine);
+                        address.purpose, address.is_mine);
                 cachedAddressTable.append(AddressTableEntry(addressType,
                                   QString::fromStdString(address.name),
                                   QString::fromStdString(EncodeDestination(address.dest))));
@@ -94,7 +93,7 @@ class AddressTablePriv
         std::sort(cachedAddressTable.begin(), cachedAddressTable.end(), AddressTableEntryLessThan());
     }
 
-    void updateEntry(const QString &address, const QString &label, bool isMine, const QString &purpose, int status)
+    void updateEntry(const QString &address, const QString &label, bool isMine, wallet::AddressPurpose purpose, int status)
     {
         // Find address / label in model
         QList<AddressTableEntry>::iterator lower = std::lower_bound(
@@ -236,7 +235,7 @@ bool AddressTableModel::setData(const QModelIndex &index, const QVariant &value,
     if(!index.isValid())
         return false;
     AddressTableEntry *rec = static_cast<AddressTableEntry*>(index.internalPointer());
-    std::string strPurpose = (rec->type == AddressTableEntry::Sending ? "send" : "receive");
+    wallet::AddressPurpose purpose = rec->type == AddressTableEntry::Sending ? wallet::AddressPurpose::SEND : wallet::AddressPurpose::RECEIVE;
     editStatus = OK;
 
     if(role == Qt::EditRole)
@@ -250,7 +249,7 @@ bool AddressTableModel::setData(const QModelIndex &index, const QVariant &value,
                 editStatus = NO_CHANGES;
                 return false;
             }
-            walletModel->wallet().setAddressBook(curAddress, value.toString().toStdString(), strPurpose);
+            walletModel->wallet().setAddressBook(curAddress, value.toString().toStdString(), purpose);
         } else if(index.column() == Address) {
             CTxDestination newAddress = DecodeDestination(value.toString().toStdString());
             // Refuse to set invalid address, set error status and return false
@@ -279,7 +278,7 @@ bool AddressTableModel::setData(const QModelIndex &index, const QVariant &value,
                 // Remove old entry
                 walletModel->wallet().delAddressBook(curAddress);
                 // Add new entry with new address
-                walletModel->wallet().setAddressBook(newAddress, value.toString().toStdString(), strPurpose);
+                walletModel->wallet().setAddressBook(newAddress, value.toString().toStdString(), purpose);
             }
         }
         return true;
@@ -331,7 +330,7 @@ QModelIndex AddressTableModel::index(int row, int column, const QModelIndex &par
 }
 
 void AddressTableModel::updateEntry(const QString &address,
-        const QString &label, bool isMine, const QString &purpose, int status)
+        const QString &label, bool isMine, wallet::AddressPurpose purpose, int status)
 {
     // Update address book model from Dash core
     priv->updateEntry(address, label, isMine, purpose, status);
@@ -361,7 +360,7 @@ QString AddressTableModel::addRow(const QString &type, const QString &label, con
             }
         }
         // Add entry
-        walletModel->wallet().setAddressBook(DecodeDestination(strAddress), strLabel, "send");
+        walletModel->wallet().setAddressBook(DecodeDestination(strAddress), strLabel, wallet::AddressPurpose::SEND);
     }
     else if(type == Receive)
     {
@@ -414,18 +413,18 @@ QString AddressTableModel::labelForAddress(const QString &address) const
     return QString();
 }
 
-QString AddressTableModel::purposeForAddress(const QString &address) const
+std::optional<wallet::AddressPurpose> AddressTableModel::purposeForAddress(const QString &address) const
 {
-    std::string purpose;
+    wallet::AddressPurpose purpose;
     if (getAddressData(address, /* name= */ nullptr, &purpose)) {
-        return QString::fromStdString(purpose);
+        return purpose;
     }
-    return QString();
+    return std::nullopt;
 }
 
 bool AddressTableModel::getAddressData(const QString &address,
         std::string* name,
-        std::string* purpose) const {
+        wallet::AddressPurpose* purpose) const {
     CTxDestination destination = DecodeDestination(address.toStdString());
     return walletModel->wallet().getAddress(destination, name, /* is_mine= */ nullptr, purpose);
 }

src/qt/addresstablemodel.h

@@ -6,6 +6,7 @@
 #define BITCOIN_QT_ADDRESSTABLEMODEL_H
 
 #include <script/standard.h>
+#include <optional>
 
 #include <QAbstractTableModel>
 #include <QStringList>
@@ -16,6 +17,9 @@ class WalletModel;
 namespace interfaces {
 class Wallet;
 }
+namespace wallet {
+enum class AddressPurpose;
+} // namespace wallet
 
 /**
    Qt model of the address book in the core. This allows views to access and modify the address book.
@@ -71,7 +75,7 @@ class AddressTableModel : public QAbstractTableModel
     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;
+    std::optional<wallet::AddressPurpose> purposeForAddress(const QString &address) const;
 
     /* Look up row index of an address in the model.
        Return -1 if not found.
@@ -87,15 +91,15 @@ class AddressTableModel : public QAbstractTableModel
     EditStatus editStatus = OK;
 
     /** Look up address book data given an address string. */
-    bool getAddressData(const QString &address, std::string* name, std::string* purpose) const;
+    bool getAddressData(const QString &address, std::string* name, wallet::AddressPurpose* purpose) const;
 
     /** Notify listeners that data changed. */
     void emitDataChanged(int index);
 
 public Q_SLOTS:
     /* Update address list from core.
      */
-    void updateEntry(const QString &address, const QString &label, bool isMine, const QString &purpose, int status);
+    void updateEntry(const QString &address, const QString &label, bool isMine, wallet::AddressPurpose purpose, int status);
 
     friend class AddressTablePriv;
 };

src/qt/editaddressdialog.cpp

@@ -9,6 +9,8 @@
 #include <qt/addresstablemodel.h>
 #include <qt/guiutil.h>
 
+#include <wallet/types.h>
+
 #include <QDataWidgetMapper>
 #include <QMessageBox>
 
@@ -140,9 +142,9 @@ QString EditAddressDialog::getDuplicateAddressWarning() const
 {
     QString dup_address = ui->addressEdit->text();
     QString existing_label = model->labelForAddress(dup_address);
-    QString existing_purpose = model->purposeForAddress(dup_address);
+    auto existing_purpose = model->purposeForAddress(dup_address);
 
-    if (existing_purpose == "receive" &&
+    if (existing_purpose == wallet::AddressPurpose::RECEIVE &&
             (mode == NewSendingAddress || mode == EditSendingAddress)) {
         return tr(
             "Address \"%1\" already exists as a receiving address with label "

src/qt/test/addressbooktests.cpp

@@ -99,8 +99,8 @@ void TestAddAddressesToSendBook(interfaces::Node& node)
 
     {
         LOCK(wallet->cs_wallet);
-        wallet->SetAddressBook(r_key_dest, r_label.toStdString(), "receive");
-        wallet->SetAddressBook(s_key_dest, s_label.toStdString(), "send");
+        wallet->SetAddressBook(r_key_dest, r_label.toStdString(), wallet::AddressPurpose::RECEIVE);
+        wallet->SetAddressBook(s_key_dest, s_label.toStdString(), wallet::AddressPurpose::SEND);
     }
 
     auto check_addbook_size = [wallet](int expected_size) {

src/qt/test/wallettests.cpp

@@ -126,7 +126,7 @@ void TestGUI(interfaces::Node& node)
         WalletDescriptor w_desc(std::move(desc), 0, 0, 1, 1);
         if (!wallet->AddWalletDescriptor(w_desc, provider, "", false)) assert(false);
         CTxDestination dest = PKHash(test.coinbaseKey.GetPubKey());
-        wallet->SetAddressBook(dest, "", "receive");
+        wallet->SetAddressBook(dest, "", wallet::AddressPurpose::RECEIVE);
         wallet->SetLastBlockProcessed(105, node.context()->chainman->ActiveChain().Tip()->GetBlockHash());
     }
     {

src/qt/transactiondesc.cpp

@@ -21,7 +21,7 @@
 #include <script/script.h>
 #include <util/system.h>
 #include <validation.h>
-#include <wallet/ismine.h>
+#include <wallet/types.h>
 
 #include <stdint.h>
 #include <string>

src/qt/transactionrecord.cpp

@@ -9,7 +9,7 @@
 #include <interfaces/wallet.h>
 #include <interfaces/node.h>
 
-#include <wallet/ismine.h>
+#include <wallet/types.h>
 
 #include <stdint.h>