wallet: Return `util::Result` from WalletLoader methods

4 files changed, 47 insertions, 35 deletions

diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h
index 63603c7f7b..1148dc7e4c 100644
--- a/src/interfaces/wallet.h
+++ b/src/interfaces/wallet.h
@@ -88,7 +88,7 @@ public:
     virtual std::string getWalletName() = 0;
 
     // Get a new address.
-    virtual util::Result<CTxDestination> getNewDestination(const OutputType type, const std::string label) = 0;
+    virtual util::Result<CTxDestination> getNewDestination(const OutputType type, const std::string& label) = 0;
 
     //! Get public key.
     virtual bool getPubKey(const CScript& script, const CKeyID& address, CPubKey& pub_key) = 0;
@@ -320,31 +320,31 @@ class WalletLoader : public ChainClient
 {
 public:
     //! Create new wallet.
-    virtual std::unique_ptr<Wallet> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;
+    virtual util::Result<std::unique_ptr<Wallet>> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, std::vector<bilingual_str>& warnings) = 0;
 
-   //! Load existing wallet.
-   virtual std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;
+    //! Load existing wallet.
+    virtual util::Result<std::unique_ptr<Wallet>> loadWallet(const std::string& name, std::vector<bilingual_str>& warnings) = 0;
 
-   //! Return default wallet directory.
-   virtual std::string getWalletDir() = 0;
+    //! Return default wallet directory.
+    virtual std::string getWalletDir() = 0;
 
-   //! Restore backup wallet
-   virtual util::Result<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) = 0;
+    //! Restore backup wallet
+    virtual util::Result<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) = 0;
 
-   //! Return available wallets in wallet directory.
-   virtual std::vector<std::string> listWalletDir() = 0;
+    //! Return available wallets in wallet directory.
+    virtual std::vector<std::string> listWalletDir() = 0;
 
-   //! Return interfaces for accessing wallets (if any).
-   virtual std::vector<std::unique_ptr<Wallet>> getWallets() = 0;
+    //! Return interfaces for accessing wallets (if any).
+    virtual std::vector<std::unique_ptr<Wallet>> getWallets() = 0;
 
-   //! Register handler for load wallet messages. This callback is triggered by
-   //! createWallet and loadWallet above, and also triggered when wallets are
-   //! loaded at startup or by RPC.
-   using LoadWalletFn = std::function<void(std::unique_ptr<Wallet> wallet)>;
-   virtual std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) = 0;
+    //! Register handler for load wallet messages. This callback is triggered by
+    //! createWallet and loadWallet above, and also triggered when wallets are
+    //! loaded at startup or by RPC.
+    using LoadWalletFn = std::function<void(std::unique_ptr<Wallet> wallet)>;
+    virtual std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) = 0;
 
-   //! Return pointer to internal context, useful for testing.
-   virtual wallet::WalletContext* context() { return nullptr; }
+    //! Return pointer to internal context, useful for testing.
+    virtual wallet::WalletContext* context() { return nullptr; }
 };
 
 //! Information about one wallet address.
diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp
index 6e07dc09a0..443941d3c4 100644
--- a/src/qt/walletcontroller.cpp
+++ b/src/qt/walletcontroller.cpp
@@ -262,9 +262,13 @@ void CreateWalletActivity::createWallet()
     }
 
     QTimer::singleShot(500ms, worker(), [this, name, flags] {
-        std::unique_ptr<interfaces::Wallet> wallet = node().walletLoader().createWallet(name, m_passphrase, flags, m_error_message, m_warning_message);
+        auto wallet{node().walletLoader().createWallet(name, m_passphrase, flags, m_warning_message)};
 
-        if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
+        if (wallet) {
+            m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(*wallet));
+        } else {
+            m_error_message = util::ErrorString(wallet);
+        }
 
         QTimer::singleShot(500ms, this, &CreateWalletActivity::finish);
     });
@@ -343,9 +347,13 @@ void OpenWalletActivity::open(const std::string& path)
         tr("Opening Wallet <b>%1</b>…").arg(name.toHtmlEscaped()));
 
     QTimer::singleShot(0, worker(), [this, path] {
-        std::unique_ptr<interfaces::Wallet> wallet = node().walletLoader().loadWallet(path, m_error_message, m_warning_message);
+        auto wallet{node().walletLoader().loadWallet(path, m_warning_message)};
 
-        if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
+        if (wallet) {
+            m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(*wallet));
+        } else {
+            m_error_message = util::ErrorString(wallet);
+        }
 
         QTimer::singleShot(0, this, &OpenWalletActivity::finish);
     });
@@ -393,8 +401,11 @@ void RestoreWalletActivity::restore(const fs::path& backup_file, const std::stri
     QTimer::singleShot(0, worker(), [this, backup_file, wallet_name] {
         auto wallet{node().walletLoader().restoreWallet(backup_file, wallet_name, m_warning_message)};
 
-        m_error_message = util::ErrorString(wallet);
-        if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(*wallet));
+        if (wallet) {
+            m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(*wallet));
+        } else {
+            m_error_message = util::ErrorString(wallet);
+        }
 
         QTimer::singleShot(0, this, &RestoreWalletActivity::finish);
     });
diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp
index aea6d5534e..4fbc519e39 100644
--- a/src/wallet/interfaces.cpp
+++ b/src/wallet/interfaces.cpp
@@ -148,7 +148,7 @@ public:
     void abortRescan() override { m_wallet->AbortRescan(); }
     bool backupWallet(const std::string& filename) override { return m_wallet->BackupWallet(filename); }
     std::string getWalletName() override { return m_wallet->GetName(); }
-    util::Result<CTxDestination> getNewDestination(const OutputType type, const std::string label) override
+    util::Result<CTxDestination> getNewDestination(const OutputType type, const std::string& label) override
     {
         LOCK(m_wallet->cs_wallet);
         return m_wallet->GetNewDestination(type, label);
@@ -551,32 +551,34 @@ public:
     void setMockTime(int64_t time) override { return SetMockTime(time); }
 
     //! WalletLoader methods
-    std::unique_ptr<Wallet> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, bilingual_str& error, std::vector<bilingual_str>& warnings) override
+    util::Result<std::unique_ptr<Wallet>> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, std::vector<bilingual_str>& warnings) override
     {
-        std::shared_ptr<CWallet> wallet;
         DatabaseOptions options;
         DatabaseStatus status;
         ReadDatabaseArgs(*m_context.args, options);
         options.require_create = true;
         options.create_flags = wallet_creation_flags;
         options.create_passphrase = passphrase;
-        return MakeWallet(m_context, CreateWallet(m_context, name, true /* load_on_start */, options, status, error, warnings));
+        bilingual_str error;
+        util::Result<std::unique_ptr<Wallet>> wallet{MakeWallet(m_context, CreateWallet(m_context, name, /*load_on_start=*/true, options, status, error, warnings))};
+        return wallet ? std::move(wallet) : util::Error{error};
     }
-    std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) override
+    util::Result<std::unique_ptr<Wallet>> loadWallet(const std::string& name, std::vector<bilingual_str>& warnings) override
     {
         DatabaseOptions options;
         DatabaseStatus status;
         ReadDatabaseArgs(*m_context.args, options);
         options.require_existing = true;
-        return MakeWallet(m_context, LoadWallet(m_context, name, true /* load_on_start */, options, status, error, warnings));
+        bilingual_str error;
+        util::Result<std::unique_ptr<Wallet>> wallet{MakeWallet(m_context, LoadWallet(m_context, name, /*load_on_start=*/true, options, status, error, warnings))};
+        return wallet ? std::move(wallet) : util::Error{error};
     }
     util::Result<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) override
     {
         DatabaseStatus status;
         bilingual_str error;
         util::Result<std::unique_ptr<Wallet>> wallet{MakeWallet(m_context, RestoreWallet(m_context, backup_file, wallet_name, /*load_on_start=*/true, status, error, warnings))};
-        if (!wallet) return util::Error{error};
-        return wallet;
+        return wallet ? std::move(wallet) : util::Error{error};
     }
     std::string getWalletDir() override
     {
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 199925b4b1..093f510945 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -919,10 +919,9 @@ BOOST_FIXTURE_TEST_CASE(wallet_sync_tx_invalid_state_test, TestingSetup)
     // Add tx to wallet
     const auto& op_dest = wallet.GetNewDestination(OutputType::BECH32M, "");
     BOOST_ASSERT(op_dest);
-    const CTxDestination& dest = *op_dest;
 
     CMutableTransaction mtx;
-    mtx.vout.push_back({COIN, GetScriptForDestination(dest)});
+    mtx.vout.push_back({COIN, GetScriptForDestination(*op_dest)});
     mtx.vin.push_back(CTxIn(g_insecure_rand_ctx.rand256(), 0));
     const auto& tx_id_to_spend = wallet.AddToWallet(MakeTransactionRef(mtx), TxStateInMempool{})->GetHash();