Refactor wallet API to use nano::result (boost outcome) and direct return values by pwojcikdev · Pull Request #5023 · nanocurrency/nano-node

pwojcikdev · 2026-02-05T13:58:22Z

Refactors key wallet functions to improve error handling and API ergonomics:

Refactors wallet::insert_adhoc and wallet::deterministic_insert to return nano::result instead of out-parameters
Refactors wallet::get_seed and wallet_store::seed to return values directly
Adds stricter runtime assertions for wallet operations
Adds tests for wallet representative detection and locked wallet scenarios

…sult

…et::get_work` to return `nano::result`

…ectly

gr0vity-dev-bot · 2026-02-05T15:52:16Z

Test Results for Commit `1af9ede`

Pull Request 5023: Results
Overall Status: ✅

Test Case Results

✅ 5n4pr_conf_10k_bintree: PASS (Duration: 118s)
✅ 5n4pr_conf_10k_change: PASS (Duration: 132s)
✅ 5n4pr_conf_change_dependant: PASS (Duration: 133s)
✅ 5n4pr_conf_change_independant: PASS (Duration: 133s)
✅ 5n4pr_conf_send_dependant: PASS (Duration: 126s)
✅ 5n4pr_conf_send_independant: PASS (Duration: 131s)
✅ 5n4pr_rocks_10k_bintree: PASS (Duration: 128s)
✅ 5n4pr_rocks_10k_change: PASS (Duration: 174s)

Last updated: 2026-02-06 15:53:43 UTC

Copilot

Pull request overview

This pull request refactors the wallet API to improve error handling by replacing out-parameters with direct return values using nano::result (boost::outcome). The changes affect key wallet operations including insert_adhoc, deterministic_insert, get_seed, and fetch_prv.

Changes:

Refactored wallet functions to return nano::result<T> instead of using out-parameters
Changed wallet_store::seed to return nano::raw_key directly
Added runtime assertions for wallet operations and improved error handling
Added test coverage for locked wallet scenarios

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
nano/node/wallet.hpp	Updated function signatures to return `nano::result` types
nano/node/wallet.cpp	Refactored implementations to use result types and added release assertions
nano/node/json_handler.cpp	Updated RPC handlers to work with new result-based API
nano/node/cli.cpp	Updated CLI commands to check results from wallet operations
nano/qt/qt.cpp	Updated Qt GUI code to handle result types
nano/nano_wallet/entry.cpp	Updated wallet initialization to use new API
nano/lib/errors.hpp	Made `std::error_code` conversion operator implicit
nano/boost/outcome.hpp	Disabled nodiscard attribute for outcome results
nano/test_common/system.cpp	Updated tests to assert on wallet operation results
nano/slow_test/node.cpp	Updated tests to handle new result types
nano/rpc_test/rpc.cpp	Added locked wallet test and updated existing tests
nano/qt_test/qt.cpp	Updated Qt tests to verify error handling
nano/core_test/wallet.cpp	Updated wallet tests to verify error codes
nano/core_test/wallets.cpp	Updated wallet collection tests
nano/core_test/node.cpp	Updated node tests for new API

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

nano/boost/outcome.hpp

Copilot · 2026-02-06T08:26:44Z

nano/node/json_handler.cpp

+			for (decltype (count) i = 0; i < count; ++i)
 			{
-				nano::account new_key (wallet->deterministic_insert (generate_work));
-				if (!new_key.is_zero ())
+				auto key_result = wallet->deterministic_insert (generate_work);
+				if (key_result)
 				{
 					boost::property_tree::ptree entry;
-					entry.put ("", new_key.to_account ());
+					entry.put ("", key_result.value ().to_account ());
 					accounts.push_back (std::make_pair ("", entry));
 				}
 			}


When the wallet is locked, this loop will attempt to insert count accounts, but every single attempt will fail. This is inefficient - it would be better to check if the wallet is locked before the loop and either return an error or break out early after the first failure. Consider adding an early exit: if !key_result on the first iteration, break from the loop to avoid unnecessary repeated failures.

Copilot · 2026-02-06T08:26:45Z

nano/node/wallet.cpp

-			insert_adhoc (transaction_a, prv);
-			other_a.erase (transaction_a, *i);
+			result = true;
+			break;


This changes the behavior of the move function. Previously, if a key failed to fetch, the function would set the result to true (indicating an error) but continue processing the remaining keys. Now it breaks immediately on the first error. This could leave the wallet in an inconsistent state where only some keys have been moved. The old behavior was more resilient - it would attempt to move all keys that could be moved, even if some failed. Consider restoring the original behavior by removing the break statement and continuing to process all keys, accumulating errors along the way.

Suggested change

break;

continue;

Copilot · 2026-02-06T08:26:45Z

nano/node/wallet.cpp

+			break;
+		}
+		if (!prv_result.value ().is_zero ())
+		{
+			insert_adhoc (transaction_a, prv_result.value ());
+		}
+		else
+		{
+			insert_watch (transaction_a, i->first);
 		}
+		other_a.erase (transaction_a, i->first);


Similar to the move function, this changes the behavior of the import function. Previously, if a key failed to fetch, the function would set the result to true but continue processing the remaining keys. Now it breaks immediately on the first error. This could leave the wallet in an inconsistent state where only some keys have been imported. Consider restoring the original behavior by removing the break statement and continuing to process all keys.

Suggested change

break;

}

if (!prv_result.value ().is_zero ())

{

insert_adhoc (transaction_a, prv_result.value ());

}

else

{

insert_watch (transaction_a, i->first);

}

other_a.erase (transaction_a, i->first);

}

else if (!prv_result.value ().is_zero ())

{

insert_adhoc (transaction_a, prv_result.value ());

other_a.erase (transaction_a, i->first);

}

else

{

insert_watch (transaction_a, i->first);

other_a.erase (transaction_a, i->first);

}

nano/node/wallet.cpp

Copilot · 2026-02-06T08:26:46Z

nano/qt/qt.cpp

+			auto seed_result = this->wallet.wallet_m->get_seed ();
+			release_assert (seed_result);
+			this->wallet.application.clipboard ()->setText (QString (seed_result.value ().to_string ().c_str ()));


This code uses release_assert on the get_seed result after checking is_locked(). However, get_seed could still fail in edge cases even when the wallet appears unlocked (e.g., concurrent lock operations). The check should be defensive and handle the error case properly rather than using a release assertion that will crash the application. Consider checking the result and showing an appropriate error message to the user if it fails.

Copilot · 2026-02-06T08:26:46Z

nano/node/cli.cpp

+						auto seed_result = existing->second->get_seed ();
+						release_assert (seed_result);
+						std::cout << boost::str (boost::format ("Seed: %1%\n") % seed_result.value ().to_string ());


Similar to the Qt code, this uses release_assert on get_seed result after checking that enter_password succeeded. While this is safer than the Qt case, it's still possible (though unlikely) for the wallet to become locked between the password check and the seed retrieval due to concurrent operations. Consider handling the error case gracefully by checking the result and printing an error message instead of crashing the application.

Copilot · 2026-02-06T08:26:46Z

nano/nano_wallet/entry.cpp

 					{
-						wallet_config.account = wallet->deterministic_insert ();
+						auto insert_result = wallet->deterministic_insert ();
+						release_assert (insert_result);


Using release_assert here will crash the entire wallet application if deterministic_insert fails (e.g., due to a locked wallet). This is inappropriate for a GUI application startup path. The code should handle the error gracefully, perhaps by prompting the user to unlock the wallet or creating a new wallet with a password. Consider checking the result and handling the locked wallet scenario explicitly.

Suggested change

release_assert (insert_result);

if (!insert_result)

{

if (splash)

{

splash->hide ();

}

show_error ("Unable to create a new deterministic account. The wallet may be locked or otherwise unavailable.");

std::exit (1);

}

…urn nano::result

…cked

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

nano/node/wallet.cpp

Copilot · 2026-02-07T15:45:41Z

nano/node/wallet.cpp

+nano::result<nano::public_key> nano::wallet::insert_adhoc (nano::raw_key const & prv, bool generate_work)
+{
+	auto transaction = wallets.tx_begin_write ();

-		// Makes sure that the representatives container will
-		// be in sync with any added keys.
-		transaction.commit ();
+	if (!store.valid_password (transaction))
+	{
+		return nano::error (nano::error_common::wallet_locked);
+	}

-		if (wallets.check_rep (key))
-		{
-			logger.info (nano::log::type::wallet, "New account qualified as a representative: {}", key.to_account ());
-			representatives.lock ()->insert (key);
-		}
+	auto key = store.insert_adhoc (transaction, prv);
+
+	logger.info (nano::log::type::wallet, "Ad-hoc inserted new account: {}", key.to_account ());
+
+	auto ledger_txn = wallets.ledger.tx_begin_read ();
+	if (generate_work)
+	{
+		work_ensure (key, wallets.ledger.latest_root (ledger_txn, key));
 	}
+
+	if (wallets.check_rep (key))
+	{
+		logger.info (nano::log::type::wallet, "New account qualified as a representative: {}", key.to_account ());
+		representatives.lock ()->insert (key);
+	}


wallet::insert_adhoc updates the in-memory representatives set before the LMDB write transaction is committed (commit happens on write_transaction destruction). Previously this path explicitly committed before inserting into representatives, which avoids a race where other threads can observe the representative key but fail to fetch it from the store (uncommitted). Consider committing the transaction before mutating representatives, or restructuring the scope so the transaction is committed before representatives is updated (and ideally apply the same pattern to deterministic inserts too).

nano/lib/errors.hpp

nano/boost/outcome.hpp

pwojcikdev added 4 commits February 5, 2026 14:51

Disable [[nodiscard]] warning for boost::outcome results

c735e86

Refactor wallet_store::fetch and wallet::fetch_prv to return nano::re…

4d7b8cf

…sult

Refactor wallet_store::work_get to return std::optional and `wall…

cf1d782

…et::get_work` to return `nano::result`

Refactor wallet::get_seed and wallet_store::seed to return values dir…

19e9e6d

…ectly

pwojcikdev force-pushed the wallet-use-result branch from 0601408 to 193b1ce Compare February 5, 2026 22:50

pwojcikdev requested a review from Copilot February 6, 2026 08:21

Copilot started reviewing on behalf of pwojcikdev February 6, 2026 08:21 View session

Copilot AI reviewed Feb 6, 2026

View reviewed changes

pwojcikdev added 9 commits February 6, 2026 15:06

Add test for accounts_create RPC with locked wallet

55f8974

Refactor wallet::insert_adhoc and wallet::deterministic_insert to ret…

2762b24

…urn nano::result

Stricter runtime assertions for the wallet component

455cf26

Report error and break early in accounts_create RPC when wallet is lo…

52f0924

…cked

Continue processing remaining keys on error in wallet_store::move

6d9c774

Continue processing remaining keys on error in wallet_store::import

57b8ec0

Handle get_seed error gracefully in Qt wallet backup

ada892f

Handle get_seed error gracefully in CLI wallet_decrypt_unsafe

efb001e

Handle deterministic_insert error gracefully in wallet GUI startup

1af9ede

pwojcikdev force-pushed the wallet-use-result branch from dfdd62a to 1af9ede Compare February 6, 2026 14:06

pwojcikdev requested a review from Copilot February 7, 2026 15:40

Copilot started reviewing on behalf of pwojcikdev February 7, 2026 15:40 View session

Copilot AI reviewed Feb 7, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor wallet API to use nano::result (boost outcome) and direct return values#5023

Refactor wallet API to use nano::result (boost outcome) and direct return values#5023
pwojcikdev wants to merge 13 commits intonanocurrency:developfrom
pwojcikdev:wallet-use-result

pwojcikdev commented Feb 5, 2026

Uh oh!

gr0vity-dev-bot commented Feb 5, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Feb 7, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-						release_assert (insert_result);
+						if (!insert_result)
+						{
+							if (splash)
+							{
+								splash->hide ();
+							}
+							show_error ("Unable to create a new deterministic account. The wallet may be locked or otherwise unavailable.");
+							std::exit (1);
+						}

Conversation

pwojcikdev commented Feb 5, 2026

Uh oh!

gr0vity-dev-bot commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results for Commit 1af9ede

Test Case Results

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gr0vity-dev-bot commented Feb 5, 2026 •

edited

Loading

Test Results for Commit `1af9ede`