Done in 948651d.

Done in 3c96b25.

Below I used m_error_message = wallet ? bilingual_str{} : wallet.GetError();

It would be good to use the same code consistently everywhere.

Done in 7cbfa13. Thanks for the review.

I think the code change was not correctly pushed.
The commit 7cbfa1335bd89d5831e916672e57aa8601b12bb4 still displays the line:

else m_error_message = wallet.GetError();

and not:

m_error_message = wallet ? bilingual_str{} : wallet.GetError();

@shaavan I said the suggestion was addressed because I understand it's about keeping all code consistent across all commits.

I had originally defined all m_error_message same way as done in #25594: m_error_message = wallet ? bilingual_str{} : wallet.GetError();.

So #25616 (comment) and #25616 (comment) suggested changing them to else m_error_message = res_wallet.GetError(); but I had forgotten to change it to restoreWallet(). 7cbfa13 fixed this.

If I understand the BResult interface correctly, it seems to be the clearest option, as BResult<T>::GetError() will return the default bilingual_str{} : value if T is not set.

However, both approaches work. If the reviewers have a strong opinion about one of them, I can change it.

BResult is essentially an std::variant wrapper. It stores one object OR the other (succeed or failure). Not both.

So, it is redundant to ask m_error_message = wallet ? bilingual_str{} : wallet.GetError(); first and then ask if succeeded in another if block.

m_error_message = wallet ? bilingual_str{} : wallet.GetError(); was intentional to reset the member that stores the error, instead of leaving the previous value, if no error occurred. However, I am fine changing it to something else.

Yeah, I double checked it before comment. The error message is only set here after calling the backend method. So, the string will always be empty if no error occurs.

Is it possible to load a wallet with an error and then load a wallet without error on the same walletcontroller?

Oh, I missed the comment, sorry Marko.

Is it possible to load a wallet with an error and then load a wallet without error on the same walletcontroller?

You mean, restoring or opening a wallet?

Because if that is the case, there shouldn't be any problem. Every time that we restore or open a wallet we create a new WalletControllerActivity subclass which encapsulates the error and warning messages (it's a single shot class. It gets deleted as soon as it finishes processing the action).

can makeWallet be renamed to makeWalletResult or something else? Having both createWallet and makeWallet is a bit confusing.

Done in 087959d. Thanks.

-        if (res_wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(res_wallet.ReleaseObj());
-        else m_error_message = res_wallet.GetError();
+        if (res_wallet) {
+            m_wallet_model = m_wallet_controller->getOrCreateWallet(res_wallet.ReleaseObj());
+        } else {
+            m_error_message = res_wallet.GetError();
+        }

Any reason why you've changed the variable name from wallet to res_wallet here? It's wallet in the identical code sections below.

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

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);

nit if you repush, here and in createWallet() above, while touching this line can use Clang-tidy named arg format (/*load_on_start=*/true) like in restoreWallet() below.

nit, could save 9 lines by replacing the above 4 lines in each of the 3 methods with identical logic in one line: return wallet ? wallet : util::Error{error}; (feel free to ignore)

That would be good, but applying this change will result in the error below.
I'm not sure about the reason for the error.

wallet/interfaces.cpp:577:25: error: call to implicitly-deleted copy constructor of 'util::Result<std::unique_ptr<Wallet>>'
        return wallet ? wallet : util::Error{error};
                        ^~~~~~
./util/result.h:38:36: note: copy constructor of 'Result<std::unique_ptr<interfaces::Wallet>>' is implicitly deleted because field 'm_variant' has a deleted copy constructor
    std::variant<bilingual_str, T> m_variant;

Ah, std::unique_ptr is move constructible and move assignable but not copy constructible or copy assignable. So this would work: return wallet ? std::move(wallet) : util::Error{error};

Thanks. Done in 466374f.

So return wallet moves the object and return wallet ? wallet : util::Error{error}; copies the object. Interesting.

It might be that return with a ternary doesn't benefit from RVO (return value optimization) by the compiler in the same way as with an if statement.

https://en.cppreference.com/w/cpp/language/copy_elision

Edit: benefitting from RVO could (possibly, unsure) be an argument for using the more verbose if statement.

Interesting discussion about this:

https://stackoverflow.com/questions/48250917/should-we-write-stdmove-in-the-cases-when-rvo-can-not-be-done

https://stackoverflow.com/questions/22078029/why-does-the-ternary-operator-prevent-return-value-optimization

Thanks, not a blocker but this is a topic I plan to go deeper into in order to improve my understanding.

Looks like this file invited itself into the last push, ACK 466374f6c1a032d7540b1c9307631f56fe555fcf otherwise :)

Removed. Thanks.

Thanks for picking up #25721 (comment) here.

Thanks for picking up the suggestion #25721 (comment) here.

why the revert here? (and in the others as well).
the result could be an object or an error, not both.

if (wallet) m_wallet_model = something;
else m_error_message = util::ErrorString(wallet);

Thinking that might be good to add an assertion inside util::ErrorString, callers should always check that the result is an error prior to convert it.

Maybe assert(std::holds_alternative<bilingual_str>(result.m_variant)); inside util::ErrorString ?

However this assertion cause a compile-time error error: static_assert failed due to requirement '__detail::__variant::__exactly_once<bilingual_str, bilingual_str, bilingual_str>' "T must occur exactly once in alternatives"

yep, that was where I was pointing to.
assert(!result.has_value()); there should work fine.

I applied the suggestion above (if (wallet) m_wallet_model = something;) in ac74799.

But regarding the assertion, I think a follow-up PR is better because it also requires changing the src/test/result_tests.cpp which expects the result of util::ErrorString be {} if the object has value This would increase the scope of this PR.

Missing brackets in the conditionals again (e.g. #25616 (comment))

shouldn't be needed:
https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md?plain=1#L77-L80

"If an if only has a single-statement then-clause, it can appear on the same line as the if, without braces. In every other case, braces are required, and the then and else clauses must appear correctly indented on a new line."

yep, aren't we having a single line statement here?

if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(*wallet));
else m_error_message = util::ErrorString(wallet);

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

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

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

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