refactor: Return BResult from restoreWallet #25594

maflcko · 2022-07-12T13:37:47Z

This avoids the error in-out param (and if warnings is added to BResult, it will avoid passing that in-out param as well).

Also, as it is needed for this change, prepare BResult for non-copyable types.

w0xlt

Tested ACK fa869c6

Perhaps this change can be split into two commits: one for BResult and one for restoreWallet.

ryanofsky

Code review ACK fa869c6

ryanofsky · 2022-07-12T18:52:42Z

src/util/result.h

@@ -18,9 +19,9 @@ class BResult {
    std::variant<bilingual_str, T> m_variant;

 public:
-    BResult() : m_variant(Untranslated("")) {}
-    BResult(const T& _obj) : m_variant(_obj) {}


In commit "refactor: Return BResult from restoreWallet" (fa869c6)

I think dropping const T& constructor drops support for noncopyable and nonmovable objects.

Maybe want:

BResult(const T& obj) : m_variant{obj} {} BResult(T&& obj) : m_variant{std::move(obj)} {}

drops support

This was never supported, unless I am missing something?

Regardless, it seems wrong to return memory that is not owned. Though, if you really want to, it might be better to make the type T explicit. That is, make it a a raw pointer/std::ref/span/string_view/whatever.

drops support

This was never supported, unless I am missing something?

You're right. It didn't work previously or in the version I suggested either. It would be possible, but would have to call one of the std::in_place_* variant constructors here.

Regardless, it seems wrong to return memory that is not owned.

The idea would not to have BResult point to an outside value but for BResult to contain a value that is constructed in-place. But regardless, this is an edge case not worth considering here. I might support with it in my followup for #25308 and add a test if I do

w0xlt

reACK fa475e9

The previous commit, although it failed some CI checks, had worked on Ubuntu 21.10 clang version 13.0.0-2.

Maybe shared_ptr<CWallet> src/wallet/wallet.h:std::RestoreWallet(..,bilingual_str& error,...) might also be changed to BResult<CWallet> src/wallet/wallet.h:std::RestoreWallet(..)

w0xlt · 2022-07-13T02:49:23Z

I proposed some changes to BResult in #25601 that might make it easier to use in src/wallet/wallet.h:{RestoreWallet(...),CreateWallet(...),LoadWallet(...)}.

maflcko · 2022-07-13T07:24:20Z

Maybe shared_ptr src/wallet/wallet.h:std::RestoreWallet(..,bilingual_str& error,...) might also be changed to BResult src/wallet/wallet.h:std::RestoreWallet(..)

Good idea. Feel free to ping me for review on a follow-up. For now I am trying to keep the changes here to a minimum.

ryanofsky

Code review ACK fa475e9. Changes since last review were replacing auto with explicit type and splitting commits

ryanofsky · 2022-07-13T12:46:22Z

src/util/result.h

@@ -18,9 +19,9 @@ class BResult {
    std::variant<bilingual_str, T> m_variant;

 public:
-    BResult() : m_variant(Untranslated("")) {}
-    BResult(const T& _obj) : m_variant(_obj) {}


drops support

This was never supported, unless I am missing something?

You're right. It didn't work previously or in the version I suggested either. It would be possible, but would have to call one of the std::in_place_* variant constructors here.

Regardless, it seems wrong to return memory that is not owned.

The idea would not to have BResult point to an outside value but for BResult to contain a value that is constructed in-place. But regardless, this is an edge case not worth considering here. I might support with it in my followup for #25308 and add a test if I do

DrahtBot · 2022-07-14T04:07:24Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25608 (BResult improvements by ryanofsky)

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.

fa475e9 refactor: Return BResult from restoreWallet (MacroFake) fa8de09 Prepare BResult for non-copyable types (MacroFake) Pull request description: This avoids the `error` in-out param (and if `warnings` is added to `BResult`, it will avoid passing that in-out param as well). Also, as it is needed for this change, prepare `BResult` for non-copyable types. ACKs for top commit: w0xlt: reACK bitcoin@fa475e9 ryanofsky: Code review ACK fa475e9. Changes since last review were replacing auto with explicit type and splitting commits Tree-SHA512: 46350883572f13721ddd198f5dfb88d2fa58ebcbda416f74da3563ea15c920fb1e6ff30558526a4ac91c36c21e6afe27751a4e51b7b8bcbcbe805209f4e9014b

07df6cd wallet: Return `util::Result` from WalletLoader methods (w0xlt) Pull request description: This PR adds a method that implement common logic to WalletLoader methods and change them to return `BResult<std::unique_ptr<Wallet>>`. Motivation: #25594 changed `restoreWallet` to return `BResult` but this method shares a common pattern with `createWallet` and `loadWallet`. This PR keeps the same pattern to all WalletLoader methods. ACKs for top commit: jonatack: Review ACK 07df6cd theStack: Code-review ACK 07df6cd Tree-SHA512: 2fe321134883f7cce60206888113800afef0fa168dab184e1a8692cd21a231970eb9c25c7220ea39e5d457134002d47f0974463925db76abbf8dfcd421629c63

…r methods 07df6cd wallet: Return `util::Result` from WalletLoader methods (w0xlt) Pull request description: This PR adds a method that implement common logic to WalletLoader methods and change them to return `BResult<std::unique_ptr<Wallet>>`. Motivation: bitcoin#25594 changed `restoreWallet` to return `BResult` but this method shares a common pattern with `createWallet` and `loadWallet`. This PR keeps the same pattern to all WalletLoader methods. ACKs for top commit: jonatack: Review ACK 07df6cd theStack: Code-review ACK 07df6cd Tree-SHA512: 2fe321134883f7cce60206888113800afef0fa168dab184e1a8692cd21a231970eb9c25c7220ea39e5d457134002d47f0974463925db76abbf8dfcd421629c63

maflcko force-pushed the 2207-BResult-restoreWallet- branch from 29f67ee to fa869c6 Compare July 12, 2022 13:38

fanquake added the Wallet label Jul 12, 2022

maflcko mentioned this pull request Jul 12, 2022

refactor: introduce generic 'Result' class and connect it to CreateTransaction and GetNewDestination #25218

Merged

w0xlt approved these changes Jul 12, 2022

View reviewed changes

maflcko force-pushed the 2207-BResult-restoreWallet- branch from fa869c6 to f38b370 Compare July 12, 2022 17:17

MacroFake added 2 commits July 12, 2022 19:19

Prepare BResult for non-copyable types

fa8de09

refactor: Return BResult from restoreWallet

fa475e9

maflcko force-pushed the 2207-BResult-restoreWallet- branch from f38b370 to fa475e9 Compare July 12, 2022 17:20

ryanofsky approved these changes Jul 12, 2022

View reviewed changes

w0xlt approved these changes Jul 12, 2022

View reviewed changes

w0xlt mentioned this pull request Jul 13, 2022

util: Make BResult error a generic type instead of only bilingual_str #25601

Closed

ryanofsky approved these changes Jul 13, 2022

View reviewed changes

DrahtBot mentioned this pull request Jul 14, 2022

BResult improvements, allow returning values on failure #25608

Closed

maflcko merged commit 062b9db into bitcoin:master Jul 14, 2022

bitcoin deleted a comment Jul 14, 2022

maflcko deleted the 2207-BResult-restoreWallet-🍌 branch July 14, 2022 10:37

w0xlt mentioned this pull request Jul 14, 2022

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

Merged

bitcoin locked and limited conversation to collaborators Jul 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Return BResult from restoreWallet #25594

refactor: Return BResult from restoreWallet #25594

Uh oh!

maflcko commented Jul 12, 2022

Uh oh!

w0xlt left a comment

Uh oh!

ryanofsky left a comment

Uh oh!

ryanofsky Jul 12, 2022

Uh oh!

maflcko Jul 13, 2022

Uh oh!

ryanofsky Jul 13, 2022

Uh oh!

w0xlt left a comment •

edited

Loading

Uh oh!

w0xlt commented Jul 13, 2022

Uh oh!

maflcko commented Jul 13, 2022

Uh oh!

ryanofsky left a comment

Uh oh!

ryanofsky Jul 13, 2022

Uh oh!

DrahtBot commented Jul 14, 2022

Uh oh!

Uh oh!

refactor: Return BResult from restoreWallet #25594

refactor: Return BResult from restoreWallet #25594

Uh oh!

Conversation

maflcko commented Jul 12, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 13, 2022

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 13, 2022

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w0xlt commented Jul 13, 2022

Uh oh!

maflcko commented Jul 13, 2022

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 13, 2022

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jul 14, 2022

Conflicts

Uh oh!

Uh oh!

w0xlt left a comment •

edited

Loading