This is another step to move the wallet code from init.cpp to wallet.cpp while keeping the behavior identical. Behavior simplification can be done once all code is available in wallet.cpp.
This is also a basic step into the direction of cloning the wallet, add BIP32 & multi-wallet in the second wallet implementation without introducing bugs in the current wallet implementation.
New warning:
0wallet/wallet.cpp:3222:1: warning: control may reach end of non-void function [-Wreturn-type]
1}
2^
31 warning generated.
Could CWallet::ParameterInteraction() take a const std::map<std::string, std::string>& mapArgs as parameter instead of using the mapArgs global from util.h like in https://github.com/jtimon/bitcoin/commit/3d03f15126b79bd9f22ccd27f1d8ca43d9e16bf5 ?
In fact, I believe I got the idea from one of your older versions of this (and then @luke-jr suggested some improvements to std::vector<std::pair<std::string, std::string> > CStandardPolicy::GetOptionsHelp() that would allow him to create generic forms in qt that automatically extend themselves when new options are added).
Concept ACK
876@@ -875,8 +877,11 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
877 /* Returns the wallets help message */
878 static std::string GetWalletHelpString(bool showDebug);
879
880- /* initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */
881- static CWallet* InitLoadWallet(bool fDisableWallet, const std::string& strWalletFile, std::string& warningString, std::string& errorString);
882+ /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */
883+ static bool InitLoadWallet();
Why static instead of calling it from the pwalletMain global (which could be moved to wallet)? Or are we turning the global into a singleton?
Right. This is the direction where I’m heading to. But I work in very tiny steps (as you can see). For now it just moves “static code” to a static function in CWallet. Later it should be within the pwalletMain context.
991- nTxConfirmTarget = GetArg("-txconfirmtarget", DEFAULT_TX_CONFIRM_TARGET);
992- bSpendZeroConfChange = GetBoolArg("-spendzeroconfchange", DEFAULT_SPEND_ZEROCONF_CHANGE);
993- fSendFreeTransactions = GetBoolArg("-sendfreetransactions", DEFAULT_SEND_FREE_TRANSACTIONS);
994-
995- std::string strWalletFile = GetArg("-wallet", DEFAULT_WALLET_DAT);
996+ CWallet::ParameterInteraction();
You forgot to remove AmountErrMsg from init as well. Maybe it would help to create two separate commits or even pulls: One addressing the move-only and the other the refactoring Warning/Error messages?
I really want to get this merged quick but I am concerned that reviewers are turned off by the diff containing both moves and refactors.
375+bool static UIWarning(const std::string &str)
376+{
377+ uiInterface.ThreadSafeMessageBox(str, "", CClientUIInterface::MSG_WARNING);
378+ return true;
379+}
380+
bool static InitWarning(), so probably fine. Still, the code duplication could be avoided somehow.
I have thought about that and if we want to decouple the wallet, we probably need to accept short term duplications of some code. In that case, the factored out shared code is uiInterface.ThreadSafeMessageBox. A duplicated entry point only used by CWallet should be okay.
But I agree with @mrbandrews: the return code should be switched to void (for UIWarning, keep it for UIError for now).
Will fix that.
3218+ nTxConfirmTarget = GetArg("-txconfirmtarget", DEFAULT_TX_CONFIRM_TARGET);
3219+ bSpendZeroConfChange = GetBoolArg("-spendzeroconfchange", DEFAULT_SPEND_ZEROCONF_CHANGE);
3220+ fSendFreeTransactions = GetBoolArg("-sendfreetransactions", DEFAULT_SEND_FREE_TRANSACTIONS);
3221+
3222+ std::string strWalletFile = GetArg("-wallet", DEFAULT_WALLET_DAT);
3223+
3016@@ -2992,8 +3017,10 @@ std::string CWallet::GetWalletHelpString(bool showDebug)
3017 return strUsage;
3018 }
3019
3020-CWallet* CWallet::InitLoadWallet(bool fDisableWallet, const std::string& strWalletFile, std::string& warningString, std::string& errorString)
3021+bool CWallet::InitLoadWallet()
3022 {
3023+ std::string strWalletFile = GetArg("-wallet", DEFAULT_WALLET_DAT);
3024+
Okay. Fair enough. Added two commits. First added commit will refactor the static methods to take them into the CWallet instance context. This looks much cleaner now.
Second commit is a cosmetic BDB version logprint change.
A migration from the global pwalletMain (called from everywhere) to a singleton is out-of-scope for now.
Thanks for the reviews!
Great, thanks for addressing the other nits.
utACK 25340b7
3161@@ -3150,7 +3162,62 @@ CWallet* CWallet::InitLoadWallet(bool fDisableWallet, const std::string& strWall
3162 }
3163 walletInstance->SetBroadcastTransactions(GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST));
3164
3165- return walletInstance;
3166+ pwalletMain = walletInstance;
3167+ return true;
3168+}
3169+
3170+bool CWallet::ParameterInteraction()
utACK 25340b7
A migration from the global pwalletMain (called from everywhere) to a singleton is out-of-scope for now.
Seems like a useless intermediate step to me. Singletons are just nicely dressed-up globals. But we’d like to get rid of a single, global wallet, in any form. Just pass the thing around when you need it.