refactor: wallet: return util::Result from `GetReservedDestination` methods #25656

Can remove the bottom scriptChange = GetScriptForDestination(dest); call if GetReservedDestination failed (no valid destination).

auto op_dest = reservedest.GetReservedDestination(true);
if (!op_dest) {
    error = _("Transaction needs a change address, but we can't generate it.") + Untranslated(" ") + op_dest.GetError();
} else {
    scriptChange = GetScriptForDestination(op_dest.GetObj());
}

Thanks, done. Scrictly speaking moving the scriptChange assignment into the conditional is a bit more than a refactor (it could have been done also before the BResult change), but it also makes sense to not unnecessarily call GetScriptForDestination in the failure case.

Because CTxDestination default value is CNoDestination, this will return a succeed BResult object. Would be better to do:

return Untranslated("Not supported");

(same as we are doing for the base GetNewDestination)

I would have expected for {} to return an empty error message. Is there any way to avoid this confusion at compile time?

I would have expected for {} to return an empty error message. Is there any way to avoid this confusion at compile time?

I think the silent conversion of bilingual_str into errors is an anti-feature. Not every translated string is necessarily an error string, and the conversion makes it not possible to use a BResult<bilingual_str> type that returns a normal translated string. #25665 improves this by adding an explicit error contructor

I would have expected for {} to return an empty error message. Is there any way to avoid this confusion at compile time?

Yeah @MarcoFalke, my bad here, I shouldn't had reviewed the PR before sleep...

As we internally use an std::variant, and the first element is the bilingual_str. The {} will indeed initialize the variant with an empty error message.

What I tried to say on the message is that I don't see how empty error strings are useful. The caller cannot know where/why the function failed (which is the main purpose of returning an error string in the first place).

Thinking out loud without test it: Probably we could prevent this behavior by removing the empty constructor or by asserting that an error string cannot be empty.

I think the silent conversion of bilingual_str into errors is an anti-feature. Not every translated string is necessarily an error string, and the conversion makes it not possible to use a BResult<bilingual_str> type that returns a normal translated string. #25665 improves this by adding an explicit error contructor

You know, the initial version that did for the generic CallResult was actually implemented in that way. I was using an OperationResult<T, E> ErrorOut(string) helper function instead of the silent conversion.

I have 25665 on my review list 👍🏼 . The last time that I checked it (brief check, thus why I haven't commented yet), I wasn't so sure about the internal std:.variant removal because every result was going to have the succeed object initialized AND the errors AND warnings in memory. And conceptually, a function can succeed OR fail. Not both.

But.. will comment there soon :). Need to finish up few other reviews first. Been focused on the nice, and long, #24914 lately.

Changed to return return Untranslated("Not supported"); for the sake of being consistent with other methods in the same class (GetNewDestination) right now, as suggested by furszy. The discussion here about general improvements to BResults seem to better fit to #25665, this PR is merely a refactor.

What about this:

const auto& op_address = m_spk_man->GetReservedDestination(type, internal, nIndex, keypool);
if (!op_address) return op_address;
fInternal = keypool.fInternal;
address = op_address.ReleaseObj();

Thanks, done.