Skip to content

refactor: Return util::Result from WalletLoader methods #25616

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
maflcko merged 1 commit into from
Aug 10, 2022
Merged

refactor: Return util::Result from WalletLoader methods #25616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 19 additions & 19 deletions src/interfaces/wallet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ class Wallet
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;

@jonatack jonatack Aug 7, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking up #25721 (comment) here and fixing up indentation in this file.

@maflcko maflcko Aug 10, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wallet method still creates a copy?

$ git grep GetNewDestination -- '*wallet.h'
src/wallet/wallet.h:    util::Result<CTxDestination> GetNewDestination(const OutputType type, const std::string label);


//! Get public key.
virtual bool getPubKey(const CScript& script, const CKeyID& address, CPubKey& pub_key) = 0;
Expand Down Expand Up @@ -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;

@maflcko maflcko Aug 10, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would probably be best to also wrap the warnings in the result? Are you working on this @ryanofsky ?

@ryanofsky ryanofsky Aug 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #25616 (comment)

Would probably be best to also wrap the warnings in the result? Are you working on this @ryanofsky ?

It needs rebase, but yes I did this in #25722

@maflcko maflcko Aug 15, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I meant a minimal extract without the other C++ bloat (for example Result<void>) that isn't needed for simply passing warnings


//! 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.
Expand Down
23 changes: 17 additions & 6 deletions src/qt/walletcontroller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Expand Down Expand Up @@ -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);
});
Expand Down Expand Up @@ -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));

@maflcko maflcko Aug 10, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I preferred the previous code, as it is shorter and less ambiguous

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);
});
Expand Down
18 changes: 10 additions & 8 deletions src/wallet/interfaces.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ class WalletImpl : public Wallet
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);
Expand Down Expand Up @@ -551,32 +551,34 @@ class WalletLoaderImpl : public WalletLoader
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
{
Expand Down
3 changes: 1 addition & 2 deletions src/wallet/test/wallet_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)});

@jonatack jonatack Aug 9, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking up #25721 (comment) here.

mtx.vin.push_back(CTxIn(g_insecure_rand_ctx.rand256(), 0));
const auto& tx_id_to_spend = wallet.AddToWallet(MakeTransactionRef(mtx), TxStateInMempool{})->GetHash();

Expand Down