refactor: Combine GetWalletForJSONRPCRequest and EnsureWalletIsAvailable functions #19100

pull ryanofsky wants to merge 1 commits into bitcoin:master from ryanofsky:pr/ensa changing 3 files +58 −212

ryanofsky commented at 9:35 PM on May 28, 2020: member

This simplifies control flow and also helps get rid of the ::vpwallets variable in #19101 since EnsureWalletIsAvailable doesn't have access to the request context.
MarcoFalke commented at 10:21 PM on May 28, 2020: member

Concept ACK removing all that copy-paste boilerplate
DrahtBot added the label Refactoring on May 28, 2020
DrahtBot added the label RPC/REST/ZMQ on May 28, 2020
DrahtBot added the label Wallet on May 28, 2020
DrahtBot commented at 6:47 AM on May 29, 2020: 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:
- #18654 (rpc: separate bumpfee's psbt creation function into psbtbumpfee by achow101)
- #18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)
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.
promag commented at 12:40 AM on June 1, 2020: member

Code review ACK 5a431c350e343e3c416ee8c2fadd36d999ff2518.

refactor: Combine GetWalletForJSONRPCRequest and EnsureWalletIsAvailable functions

This simplifies control flow and also helps get rid of the ::vpwallets
variable, because EnsureWalletIsAvailable doesn't have access to the request
context.

f42f5e58f5

in src/wallet/rpcwallet.cpp:2565 in 5a431c350e outdated
```
2462 | @@ -2550,12 +2463,7 @@ static UniValue listwallets(const JSONRPCRequest& request)
2463 |      UniValue obj(UniValue::VARR);
2464 |  
2465 |      for (const std::shared_ptr<CWallet>& wallet : GetWallets()) {
2466 | -        if (!EnsureWalletIsAvailable(wallet.get(), request.fHelp)) {
2467 | -            return NullUniValue;
2468 | -        }
```
ryanofsky commented at 3:29 PM on June 1, 2020:

Note: PR is just dropping the Ensure check here which has been around since 9508761ed69a30f4af24fbc9274176056431abfb. It seems like it never did anything because the wallet pointer would never be null, and it was probably added originally as a precaution

promag commented at 11:05 AM on June 11, 2020:

Agree.
DrahtBot added the label Needs rebase on Jun 5, 2020
ryanofsky force-pushed on Jun 5, 2020
ryanofsky commented at 12:41 PM on June 5, 2020: member

Rebased 5a431c350e343e3c416ee8c2fadd36d999ff2518 -> f42f5e58f5fd063d5feec3eadf4a4040a941d4af (pr/ensa.1 -> pr/ensa.2, compare) due to conflict with #19096
DrahtBot removed the label Needs rebase on Jun 5, 2020
promag commented at 11:43 AM on June 11, 2020: member

Tested ACK f42f5e58f5fd063d5feec3eadf4a4040a941d4af.

MarcoFalke commented at 1:51 PM on June 11, 2020: member

ACK f42f5e58f5fd063d5feec3eadf4a4040a941d4af (reviewed code to check that this is a refactor) 💢

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 91abc4cab5ad750fa39568a9cdd77f33c2cfdb2f05ec0fcf18b5c862634b598f -

</details>

MarcoFalke merged this on Jun 11, 2020
MarcoFalke closed this on Jun 11, 2020
sidhujag referenced this in commit 4085e64c21 on Jun 11, 2020
Fabcien referenced this in commit 51c94b782d on Apr 29, 2021
cryptapus referenced this in commit 72b476a0cf on May 3, 2021
cryptapus referenced this in commit d481525cc9 on May 3, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Refactoring Wallet RPC/REST/ZMQ

Linked (view graph)

#19101 refactor: remove ::vpwallets and related global variables

refactor: Combine GetWalletForJSONRPCRequest and EnsureWalletIsAvailable functions #19100

Conflicts