MOVEONLY: Move wallet backup and encryption RPCs out of rpcwallet #23647

pull meshcollider wants to merge 4 commits into bitcoin:master from meshcollider:202111_split_walletrpc_total changing 6 files +390 −370

meshcollider commented at 8:10 AM on December 2, 2021: contributor

As part of an effort to split rpcwallet as per #23622, this moves rpcdump.cpp into the new wallet/rpc directory as well as moving backup and encryption RPCs out of rpcwallet.
meshcollider added the label Refactoring on Dec 2, 2021
meshcollider added the label Wallet on Dec 2, 2021
DrahtBot commented at 12:01 PM on December 2, 2021: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #23497 (Add src/node/ and src/wallet/ code to node:: and wallet:: namespaces 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.
meshcollider commented at 12:12 PM on December 2, 2021: contributor

Too many conflicts, going to try just the first three commits.
MarcoFalke commented at 12:42 PM on December 2, 2021: member

No conflicts as of last run.

:partying_face:
MarcoFalke marked this as ready for review on Dec 2, 2021
in src/wallet/rpcwallet.h:20 in 0892bea014 outdated
```
  15 | +class CWallet;
  16 | +struct WalletContext;
  17 |  
  18 |  Span<const CRPCCommand> GetWalletRPCCommands();
  19 |  
  20 | +std::tuple<std::shared_ptr<CWallet>, std::vector<bilingual_str>> LoadWalletHelper(WalletContext& context, UniValue load_on_start_param, const std::string wallet_name);
```
MarcoFalke commented at 2:50 PM on December 2, 2021:

What would happen if this was moved to wallet/rpc/util?

meshcollider commented at 7:13 PM on December 2, 2021:

Yeah I was thinking about it but also conscious of @ryanofsky's idea to potentially split further into load.cpp and thought it would be better there. I can move it in a follow-up if preferred.

in src/wallet/rpc/encrypt.cpp:6 in acc8d3286b outdated

   0 | @@ -0,0 +1,249 @@
   1 | +// Copyright (c) 2011-2021 The Bitcoin Core developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +#include <rpc/util.h>
   6 | +#include <util/translation.h>

MarcoFalke commented at 2:51 PM on December 2, 2021:

why?

diff --git a/src/wallet/rpc/encrypt.cpp b/src/wallet/rpc/encrypt.cpp
index bd145624b7..e659f434a3 100644
--- a/src/wallet/rpc/encrypt.cpp
+++ b/src/wallet/rpc/encrypt.cpp
@@ -3,7 +3,6 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <rpc/util.h>
-#include <util/translation.h>
 #include <wallet/rpc/util.h>
 #include <wallet/wallet.h>

MarcoFalke approved

MarcoFalke commented at 2:51 PM on December 2, 2021: member

ACK acc8d3286b823c4c7117ed0a96955473e9531231 🌐

<details><summary>Show signature</summary>

Signature:

</details>

ryanofsky commented at 5:22 PM on December 2, 2021: member

Code review ACK acc8d3286b823c4c7117ed0a96955473e9531231. Confirmed this is move-only (other than making RPC methods non-static)
ryanofsky approved
meshcollider renamed this:
~~Split rpcwallet into multiple smaller parts~~
MOVEONLY: Move wallet backup and encryption RPCs out of rpcwallet
on Dec 2, 2021
DrahtBot added the label Needs rebase on Dec 2, 2021
MOVEONLY: Move rpcdump.cpp to wallet/rpc/backup.cpp 3a9d39324e
MOVEONLY: Move backupwallet and restorewallet to rpc/backup.cpp 803b30502b
MOVEONLY: Move wallet encryption RPCs to encrypt.cpp 8b73640152
meshcollider commented at 11:40 PM on December 2, 2021: contributor

Rebased and removed the unnecessary #include <util/translation.h>

Also added a new commit moving LoadWalletHelper to util.
DrahtBot removed the label Needs rebase on Dec 2, 2021
MOVEONLY: Move LoadWalletHelper to wallet/rpc/util 5b2167fd30
MarcoFalke approved

MarcoFalke commented at 8:16 AM on December 3, 2021: member

ACK 5b2167fd30ea4384b93a0226e9fbef4650aa9438 🎭

<details><summary>Show signature</summary>

Signature:

</details>

MarcoFalke referenced this in commit 8b4d53e4d6 on Dec 3, 2021
DrahtBot added the label Needs rebase on Dec 3, 2021
DrahtBot commented at 8:37 AM on December 3, 2021: member



🐙 This pull request conflicts with the target branch and needs rebase.

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
fanquake commented at 9:17 AM on December 3, 2021: member

This was merged.
fanquake closed this on Dec 3, 2021
sidhujag referenced this in commit cb105948cc on Dec 3, 2021
RandyMcMillan referenced this in commit ecc33dfa53 on Dec 23, 2021
DrahtBot locked this on Dec 3, 2022

Contributors

Labels

Refactoring Wallet Needs rebase

Linked (view graph)

#23622 [RFC] Splitting up rpcwallet

MOVEONLY: Move wallet backup and encryption RPCs out of rpcwallet #23647

Conflicts