Replace CWalletDB::ReadKeyValue with CWallet::LoadKeyValue #8831

pull pstratem wants to merge 4 commits into bitcoin:master from pstratem:2016-09-28-cwallet-loadkeyvalue changing 5 files +506 −432

pstratem commented at 6:06 PM on September 28, 2016: contributor

This moves logic out of CWalletDB
MarcoFalke added the label Wallet on Sep 28, 2016
laanwj added the label Refactoring on Sep 29, 2016
Replace CWalletDB::ReadKeyValue with CWallet::LoadKeyValue 1a305b5a23
in src/wallet/walletdb.h:None in b671cbba98 outdated
```
  40 | @@ -41,6 +41,24 @@ enum DBErrors
  41 |      DB_NEED_REWRITE
  42 |  };
  43 |  
  44 | +class CWalletScanState {
```
laanwj commented at 1:31 PM on October 19, 2016:

I'd say this belongs in wallet.h, not walletdb.h, as it's not used in walletdb anymore?

pstratem commented at 12:58 AM on November 1, 2016:

It's still used in CWalletDB::LoadWallet

sipa commented at 10:49 PM on November 7, 2016:

Splitting it up across files is pretty ugly in that case. Is there a next step PR in the pipeline to fix that?

pstratem commented at 11:04 PM on November 7, 2016:

there isn't currently but there will be
pstratem force-pushed on Nov 7, 2016
Make IsKeyType accessible outside walletdb.cpp f08415586e
pstratem force-pushed on Nov 8, 2016
Introduce and use CWalletDB::ReadMinVerion e5587046c4
Move CWalletDB::LoadWallet logic to CWallet::LoadWallet dc127cac69
pstratem commented at 1:00 AM on November 8, 2016: contributor

@sipa latest commit completely removes CWalletDB::LoadWallet

Still cant move CWalletScanState because of CWalletDB::Recover
sipa commented at 3:31 AM on November 15, 2016: member

Wouldn't it be possible to create a simple CWalletLoader class in walletdb.h, with only virtual functions, have CWallet implement CWalletLoader and then let CWalletDB::LoadWallet take a CWalletLoader* argument?
pstratem commented at 4:13 AM on November 15, 2016: contributor

@sipa I guess, but why?

CWalletDB really should just be doing serialization
sipa commented at 6:20 PM on November 15, 2016: member

@pstratem Serialization is in wallet.h (by virtue of the SerializationOp methods in the data structure). Walletdb's responsibility is knowing the file format (what keys correspond to what, interacting with BDB, ...). It seems that your pull here is partially moving the knowledge of the file format to CWallet itself.

By introducing a CWalletLoader interface, you can just move the knowledge for how to load entries to the wallet itself, while leaving the format definition in walletdb.
laanwj commented at 10:02 AM on February 23, 2017: member

I agree with @sipa here. The goal is not to move everything related to the wallet and knowledge of everything into CWallet. This appears to be happening lately: part of initialization was moved into CWallet, argument parsing was moved into CWallet, and so on.

Splitting up a "god class" later is quite difficult, so I'd also prefer not going through that and introducing a CWalletLoader which is concerned with the file format.
laanwj commented at 3:43 PM on June 22, 2017: member

Closing this - would need extensive rebasing, and it seems from above discussion that there was no agreement on this approach. Let me know if it should be re-openened.
laanwj closed this on Jun 22, 2017
DrahtBot locked this on Sep 8, 2021

Contributors

Labels