GetReservedDestination methods
#25656
ReserveDestination::GetReservedDestination, {Legacy,Descriptor,}ScriptPubKeyMan::GetReservedDestination are improved by returning util::Result<CTxDestination> instead of bool in order to get rid of the two CTxDestination& and bilingual_str& out-parameters.
713+ if (op_dest.HasRes()) {
714+ dest = op_dest.ReleaseObj();
715+ } else {
716+ error = _("Transaction needs a change address, but we can't generate it.") + Untranslated(" ") + op_dest.GetError();
717 }
718 scriptChange = GetScriptForDestination(dest);
Can remove the bottom scriptChange = GetScriptForDestination(dest); call if GetReservedDestination failed (no valid destination).
0auto op_dest = reservedest.GetReservedDestination(true);
1if (!op_dest) {
2 error = _("Transaction needs a change address, but we can't generate it.") + Untranslated(" ") + op_dest.GetError();
3} else {
4 scriptChange = GetScriptForDestination(op_dest.GetObj());
5}
GetScriptForDestination in the failure case.
178@@ -179,7 +179,7 @@ class ScriptPubKeyMan
179 virtual bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) { return false; }
180 virtual bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) { return false; }
181
182- virtual bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool, bilingual_str& error) { return false; }
183+ virtual BResult<CTxDestination> GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) { return {}; }
Because CTxDestination default value is CNoDestination, this will return a succeed BResult object.
Would be better to do:
0return Untranslated("Not supported");
(same as we are doing for the base GetNewDestination)
{} 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.
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.
2442+ if (op_address.HasRes()) {
2443+ address = op_address.ReleaseObj();
2444+ } else {
2445+ return op_address.GetError();
2446 }
2447 fInternal = keypool.fInternal;
What about this:
0const auto& op_address = m_spk_man->GetReservedDestination(type, internal, nIndex, keypool);
1if (!op_address) return op_address;
2fInternal = keypool.fInternal;
3address = op_address.ReleaseObj();
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
util::Result from WalletLoader methods by w0xlt)If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
util::Result is used instead of BResult now accordingly. Also squashed the two commits into one as I think it’s easier to review.
