Skip to content

Commit 42f4f7d

Browse files
committed
refactor: Replace BResult with util::Result
Rename BResult class to util::Result and update the class interface to be more compatible with std::optional and with a full-featured result class implemented in #25665. Motivation for this change is to update existing BResult usages now so they don't have to change later when more features are added to the Result class.
1 parent 6dc3084 commit 42f4f7d

23 files changed

+201
-110
lines changed

src/Makefile.test.include

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,7 @@ BITCOIN_TESTS =\
118118
test/raii_event_tests.cpp \
119119
test/random_tests.cpp \
120120
test/rest_tests.cpp \
121+
test/result_tests.cpp \
121122
test/reverselock_tests.cpp \
122123
test/rpc_tests.cpp \
123124
test/sanity_tests.cpp \

src/bench/wallet_loading.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,10 @@ static void BenchUnloadWallet(std::shared_ptr<CWallet>&& wallet)
4646
static void AddTx(CWallet& wallet)
4747
{
4848
const auto& dest = wallet.GetNewDestination(OutputType::BECH32, "");
49-
assert(dest.HasRes());
49+
assert(dest);
5050

5151
CMutableTransaction mtx;
52-
mtx.vout.push_back({COIN, GetScriptForDestination(dest.GetObj())});
52+
mtx.vout.push_back({COIN, GetScriptForDestination(*dest)});
5353
mtx.vin.push_back(CTxIn());
5454

5555
wallet.AddToWallet(MakeTransactionRef(mtx), TxStateInactive{});

src/interfaces/wallet.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ class Wallet
8888
virtual std::string getWalletName() = 0;
8989

9090
// Get a new address.
91-
virtual BResult<CTxDestination> getNewDestination(const OutputType type, const std::string label) = 0;
91+
virtual util::Result<CTxDestination> getNewDestination(const OutputType type, const std::string label) = 0;
9292

9393
//! Get public key.
9494
virtual bool getPubKey(const CScript& script, const CKeyID& address, CPubKey& pub_key) = 0;
@@ -139,7 +139,7 @@ class Wallet
139139
virtual void listLockedCoins(std::vector<COutPoint>& outputs) = 0;
140140

141141
//! Create transaction.
142-
virtual BResult<CTransactionRef> createTransaction(const std::vector<wallet::CRecipient>& recipients,
142+
virtual util::Result<CTransactionRef> createTransaction(const std::vector<wallet::CRecipient>& recipients,
143143
const wallet::CCoinControl& coin_control,
144144
bool sign,
145145
int& change_pos,
@@ -329,7 +329,7 @@ class WalletLoader : public ChainClient
329329
virtual std::string getWalletDir() = 0;
330330

331331
//! Restore backup wallet
332-
virtual BResult<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) = 0;
332+
virtual util::Result<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) = 0;
333333

334334
//! Return available wallets in wallet directory.
335335
virtual std::vector<std::string> listWalletDir() = 0;

src/qt/addresstablemodel.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -384,7 +384,7 @@ QString AddressTableModel::addRow(const QString &type, const QString &label, con
384384
return QString();
385385
}
386386
}
387-
strAddress = EncodeDestination(op_dest.GetObj());
387+
strAddress = EncodeDestination(*op_dest);
388388
}
389389
else
390390
{

src/qt/walletcontroller.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -393,8 +393,8 @@ void RestoreWalletActivity::restore(const fs::path& backup_file, const std::stri
393393
QTimer::singleShot(0, worker(), [this, backup_file, wallet_name] {
394394
auto wallet{node().walletLoader().restoreWallet(backup_file, wallet_name, m_warning_message)};
395395

396-
m_error_message = wallet ? bilingual_str{} : wallet.GetError();
397-
if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(wallet.ReleaseObj());
396+
m_error_message = wallet ? bilingual_str{} : util::ErrorString(wallet);
397+
if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(*wallet));
398398

399399
QTimer::singleShot(0, this, &RestoreWalletActivity::finish);
400400
});

src/qt/walletmodel.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
207207

208208
auto& newTx = transaction.getWtx();
209209
const auto& res = m_wallet->createTransaction(vecSend, coinControl, !wallet().privateKeysDisabled() /* sign */, nChangePosRet, nFeeRequired);
210-
newTx = res ? res.GetObj() : nullptr;
210+
newTx = res ? *res : nullptr;
211211
transaction.setTransactionFee(nFeeRequired);
212212
if (fSubtractFeeFromAmount && newTx)
213213
transaction.reassignAmounts(nChangePosRet);
@@ -218,7 +218,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
218218
{
219219
return SendCoinsReturn(AmountWithFeeExceedsBalance);
220220
}
221-
Q_EMIT message(tr("Send Coins"), QString::fromStdString(res.GetError().translated),
221+
Q_EMIT message(tr("Send Coins"), QString::fromStdString(util::ErrorString(res).translated),
222222
CClientUIInterface::MSG_ERROR);
223223
return TransactionCreationFailed;
224224
}

src/test/result_tests.cpp

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
// Copyright (c) 2022 The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#include <util/result.h>
6+
7+
#include <boost/test/unit_test.hpp>
8+
9+
BOOST_AUTO_TEST_SUITE(result_tests)
10+
11+
struct NoCopy
12+
{
13+
NoCopy(int n) : m_n{std::make_unique<int>(n)} {}
14+
std::unique_ptr<int> m_n;
15+
};
16+
17+
bool operator==(const NoCopy& a, const NoCopy& b)
18+
{
19+
return *a.m_n == *b.m_n;
20+
}
21+
22+
std::ostream& operator<<(std::ostream& os, const NoCopy& o)
23+
{
24+
if (o.m_n) os << "NoCopy(" << *o.m_n << ")"; else os << "NoCopy(nullptr)";
25+
return os;
26+
}
27+
28+
util::Result<int> IntFn(int i, bool success)
29+
{
30+
if (success) return i;
31+
return {util::Error{Untranslated(strprintf("int %i error.", i))}};
32+
}
33+
34+
util::Result<NoCopy> NoCopyFn(int i, bool success)
35+
{
36+
if (success) return {i};
37+
return {util::Error{Untranslated(strprintf("nocopy %i error.", i))}};
38+
}
39+
40+
template<typename T>
41+
void ExpectResult(const util::Result<T>& result, bool success, const bilingual_str& str)
42+
{
43+
BOOST_CHECK_EQUAL(bool(result), success);
44+
BOOST_CHECK_EQUAL(ErrorString(result).original, str.original);
45+
BOOST_CHECK_EQUAL(ErrorString(result).translated, str.translated);
46+
}
47+
48+
template<typename T, typename... Args>
49+
void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, Args&&... args)
50+
{
51+
ExpectResult(result, true, str);
52+
BOOST_CHECK_EQUAL(result.has_value(), true);
53+
BOOST_CHECK_EQUAL(result.value(), T{std::forward<Args>(args)...});
54+
BOOST_CHECK_EQUAL(&result.value(), &*result);
55+
}
56+
57+
template<typename T, typename... Args>
58+
void ExpectFail(const util::Result<T>& result, const bilingual_str& str)
59+
{
60+
ExpectResult(result, false, str);
61+
}
62+
63+
BOOST_AUTO_TEST_CASE(check_returned)
64+
{
65+
ExpectSuccess(IntFn(5, true), {}, 5);
66+
ExpectFail(IntFn(5, false), Untranslated("int 5 error."));
67+
ExpectSuccess(NoCopyFn(5, true), {}, 5);
68+
ExpectFail(NoCopyFn(5, false), Untranslated("nocopy 5 error."));
69+
}
70+
71+
BOOST_AUTO_TEST_SUITE_END()

src/test/util/wallet.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,9 @@ std::string getnewaddress(CWallet& w)
2121
{
2222
constexpr auto output_type = OutputType::BECH32;
2323
auto op_dest = w.GetNewDestination(output_type, "");
24-
assert(op_dest.HasRes());
24+
assert(op_dest);
2525

26-
return EncodeDestination(op_dest.GetObj());
26+
return EncodeDestination(*op_dest);
2727
}
2828

2929
#endif // ENABLE_WALLET

src/util/result.h

Lines changed: 49 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -9,41 +9,60 @@
99

1010
#include <variant>
1111

12-
/*
13-
* 'BResult' is a generic class useful for wrapping a return object
14-
* (in case of success) or propagating the error cause.
15-
*/
12+
namespace util {
13+
14+
struct Error { bilingual_str message; };
15+
16+
//! The util::Result class provides a standard way for functions to return error
17+
//! messages in addition to optional result values.
18+
//!
19+
//! It is intended for high-level functions that need to report error strings to
20+
//! end users. Lower-level functions that don't need this error-reporting and
21+
//! only need error-handling should avoid util::Result and instead use standard
22+
//! classes like std::optional, std::variant, and std::tuple, or custom structs
23+
//! and enum types to return function results.
24+
//!
25+
//! Usage examples can be found in \example ../test/result_tests.cpp, but in
26+
//! general code returning `util::Result<T>` values is very similar to code
27+
//! returning `std::optional<T>` values. Existing functions returning
28+
//! `std::optional<T>` can be updated to return `util::Result<T>` and return
29+
//! error strings usually just replacing `return std::nullopt;` with `return
30+
//! util::Error{error_string};`.
1631
template<class T>
17-
class BResult {
32+
class Result {
1833
private:
1934
std::variant<bilingual_str, T> m_variant;
2035

36+
template <typename FT>
37+
friend class Result;
38+
template <typename FT>
39+
friend bilingual_str ErrorString(const Result<FT>& result);
40+
2141
public:
22-
BResult() : m_variant{Untranslated("")} {}
23-
BResult(T obj) : m_variant{std::move(obj)} {}
24-
BResult(bilingual_str error) : m_variant{std::move(error)} {}
25-
26-
/* Whether the function succeeded or not */
27-
bool HasRes() const { return std::holds_alternative<T>(m_variant); }
28-
29-
/* In case of success, the result object */
30-
const T& GetObj() const {
31-
assert(HasRes());
32-
return std::get<T>(m_variant);
33-
}
34-
T ReleaseObj()
35-
{
36-
assert(HasRes());
37-
return std::move(std::get<T>(m_variant));
38-
}
39-
40-
/* In case of failure, the error cause */
41-
const bilingual_str& GetError() const {
42-
assert(!HasRes());
43-
return std::get<bilingual_str>(m_variant);
44-
}
45-
46-
explicit operator bool() const { return HasRes(); }
42+
Result(T obj) : m_variant{std::move(obj)} {}
43+
Result(Error error) : m_variant{std::move(error.message)} {}
44+
template <typename OT>
45+
Result(Error error, Result<OT>&& other) : m_variant{std::move(std::get<0>(other.m_variant))} {}
46+
47+
//! std::optional methods, so functions returning optional<T> can change to
48+
//! return Result<T> with minimal changes to existing code, and vice versa.
49+
bool has_value() const { return m_variant.index() == 1; }
50+
const T& value() const { assert(*this); return std::get<1>(m_variant); }
51+
T& value() { assert(*this); return std::get<1>(m_variant); }
52+
template <typename U>
53+
T value_or(const U& default_value) const { return has_value() ? value() : default_value; }
54+
operator bool() const { return has_value(); }
55+
const T* operator->() const { return &value(); }
56+
const T& operator*() const { return value(); }
57+
T* operator->() { return &value(); }
58+
T& operator*() { return value(); }
4759
};
4860

61+
template <typename T>
62+
bilingual_str ErrorString(const Result<T>& result)
63+
{
64+
return result ? bilingual_str{} : std::get<0>(result.m_variant);
65+
}
66+
} // namespace util
67+
4968
#endif // BITCOIN_UTIL_RESULT_H

src/wallet/feebumper.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -221,11 +221,11 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
221221
constexpr int RANDOM_CHANGE_POSITION = -1;
222222
auto res = CreateTransaction(wallet, recipients, RANDOM_CHANGE_POSITION, new_coin_control, false);
223223
if (!res) {
224-
errors.push_back(Untranslated("Unable to create transaction.") + Untranslated(" ") + res.GetError());
224+
errors.push_back(Untranslated("Unable to create transaction.") + Untranslated(" ") + util::ErrorString(res));
225225
return Result::WALLET_ERROR;
226226
}
227227

228-
const auto& txr = res.GetObj();
228+
const auto& txr = *res;
229229
// Write back new fee if successful
230230
new_fee = txr.fee;
231231

0 commit comments

Comments
 (0)