From dfbd145676255f82bad529976928dff1939c5e48 Mon Sep 17 00:00:00 2001
From: furszy <matiasfurszyfer@protonmail.com>
Date: Fri, 22 Mar 2024 09:34:04 -0300
Subject: [PATCH] refactor: interfaces, make 'createTransaction' less
 error-prone

Bundle all function's outputs inside the util::Result returned object.

Reasons for the refactoring:
- The 'change_pos' ref argument has been a source of bugs in the past.
- The 'fee' ref argument is currently only set when the transaction creation process succeeds.
---
 src/interfaces/wallet.h   |  6 +++---
 src/qt/walletmodel.cpp    | 23 ++++++++++++-----------
 src/wallet/interfaces.cpp | 15 ++++-----------
 3 files changed, 19 insertions(+), 25 deletions(-)

diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h
index 61142366234..808efaa3e35 100644
--- a/src/interfaces/wallet.h
+++ b/src/interfaces/wallet.h
@@ -26,6 +26,7 @@
 #include <utility>
 #include <vector>
 
+struct CreatedTransactionResult;
 class CFeeRate;
 class CKey;
 enum class FeeReason;
@@ -142,11 +143,10 @@ class Wallet
     virtual void listLockedCoins(std::vector<COutPoint>& outputs) = 0;
 
     //! Create transaction.
-    virtual util::Result<CTransactionRef> createTransaction(const std::vector<wallet::CRecipient>& recipients,
+    virtual util::Result<CreatedTransactionResult> createTransaction(const std::vector<wallet::CRecipient>& recipients,
         const wallet::CCoinControl& coin_control,
         bool sign,
-        int& change_pos,
-        CAmount& fee) = 0;
+        std::optional<unsigned int> change_pos) = 0;
 
     //! Commit transaction.
     virtual void commitTransaction(CTransactionRef tx,
diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp
index 82e5204c640..19f4f862e61 100644
--- a/src/qt/walletmodel.cpp
+++ b/src/qt/walletmodel.cpp
@@ -22,6 +22,7 @@
 #include <psbt.h>
 #include <util/translation.h>
 #include <wallet/coincontrol.h>
+#include <wallet/util_spend.h>
 #include <wallet/wallet.h> // for CRecipient
 
 #include <stdint.h>
@@ -153,6 +154,8 @@ bool WalletModel::validateAddress(const QString& address) const
 
 WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransaction &transaction, const CCoinControl& coinControl)
 {
+    transaction.getWtx() = nullptr; // reset tx output
+
     CAmount total = 0;
     bool fSubtractFeeFromAmount = false;
     QList<SendCoinsRecipient> recipients = transaction.getRecipients();
@@ -204,22 +207,20 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
     }
 
     try {
-        CAmount nFeeRequired = 0;
-        int nChangePosRet = -1;
-
         auto& newTx = transaction.getWtx();
-        const auto& res = m_wallet->createTransaction(vecSend, coinControl, /*sign=*/!wallet().privateKeysDisabled(), nChangePosRet, nFeeRequired);
-        newTx = res ? *res : nullptr;
-        transaction.setTransactionFee(nFeeRequired);
-        if (fSubtractFeeFromAmount && newTx)
-            transaction.reassignAmounts(nChangePosRet);
-
-        if (!newTx) {
+        const auto& res = m_wallet->createTransaction(vecSend, coinControl, /*sign=*/!wallet().privateKeysDisabled(), /*change_pos=*/std::nullopt);
+        if (!res) {
             Q_EMIT message(tr("Send Coins"), QString::fromStdString(util::ErrorString(res).translated),
-                CClientUIInterface::MSG_ERROR);
+                           CClientUIInterface::MSG_ERROR);
             return TransactionCreationFailed;
         }
 
+        newTx = res->tx;
+        CAmount nFeeRequired = res->fee;
+        transaction.setTransactionFee(nFeeRequired);
+        if (fSubtractFeeFromAmount && newTx)
+            transaction.reassignAmounts(res->change_pos ? int(*res->change_pos) : -1);
+
         // Reject absurdly high fee. (This can never happen because the
         // wallet never creates transactions with fee greater than
         // m_default_max_tx_fee. This merely a belt-and-suspenders check).
diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp
index d15273dfc9b..99198c0225c 100644
--- a/src/wallet/interfaces.cpp
+++ b/src/wallet/interfaces.cpp
@@ -27,6 +27,7 @@
 #include <wallet/receive.h>
 #include <wallet/rpc/wallet.h>
 #include <wallet/spend.h>
+#include <wallet/util_spend.h>
 #include <wallet/wallet.h>
 
 #include <memory>
@@ -274,21 +275,13 @@ class WalletImpl : public Wallet
         LOCK(m_wallet->cs_wallet);
         return m_wallet->ListLockedCoins(outputs);
     }
-    util::Result<CTransactionRef> createTransaction(const std::vector<CRecipient>& recipients,
+    util::Result<CreatedTransactionResult> createTransaction(const std::vector<CRecipient>& recipients,
         const CCoinControl& coin_control,
         bool sign,
-        int& change_pos,
-        CAmount& fee) override
+        std::optional<unsigned int> change_pos) override
     {
         LOCK(m_wallet->cs_wallet);
-        auto res = CreateTransaction(*m_wallet, recipients, change_pos == -1 ? std::nullopt : std::make_optional(change_pos),
-                                     coin_control, sign);
-        if (!res) return util::Error{util::ErrorString(res)};
-        const auto& txr = *res;
-        fee = txr.fee;
-        change_pos = txr.change_pos ? *txr.change_pos : -1;
-
-        return txr.tx;
+        return CreateTransaction(*m_wallet, recipients, change_pos, coin_control, sign);
     }
     void commitTransaction(CTransactionRef tx,
         WalletValueMap value_map,