RPC: dumphdinfo should throw an error when wallet isn't HD

[PastaPastaPasta]

Throws an RPCError RPC_WALLET_ERROR (text = "This wallet is not a HD (Hierarchical Deterministic) wallet.") when the command dumphdinfo is called and the wallet is not HD.

This is in response to #2069

[nmarley]

@paulied Thanks for the PR! Not sure if we want to throw an error here, hence the return NullUniValue instead just below.

Nonetheless, can you please ensure the syntax is clean by properly aligning the closing brace for your else branch?

[PastaPastaPasta]

@nmarley Formatting should be fixed
The only problem with relying on return NullUniValue is that it gives no feedback to the user and results in confusion like #2069

[UdjinM6]

IMO throwing an error is perfectly fine here and fixes #2069 , though return NullUniValue; will never be reached in this case and should be removed.

This whole thing needs refactoring actually, smth like:

diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
index 0eebe5c9a..70dd72c07 100644
--- a/src/wallet/rpcdump.cpp
+++ b/src/wallet/rpcdump.cpp
@@ -705,26 +705,23 @@ UniValue dumphdinfo(const JSONRPCRequest& request)
 
     EnsureWalletIsUnlocked();
 
-    // add the base58check encoded extended master if the wallet uses HD
     CHDChain hdChainCurrent;
-    if (pwalletMain->GetHDChain(hdChainCurrent))
-    {
-        if (!pwalletMain->GetDecryptedHDChain(hdChainCurrent))
-            throw JSONRPCError(RPC_INTERNAL_ERROR, "Cannot decrypt HD seed");
+    if (!pwalletMain->GetHDChain(hdChainCurrent))
+        throw JSONRPCError(RPC_WALLET_ERROR, "This wallet is not a HD wallet.");
 
-        SecureString ssMnemonic;
-        SecureString ssMnemonicPassphrase;
-        hdChainCurrent.GetMnemonic(ssMnemonic, ssMnemonicPassphrase);
+    if (!pwalletMain->GetDecryptedHDChain(hdChainCurrent))
+        throw JSONRPCError(RPC_INTERNAL_ERROR, "Cannot decrypt HD seed");
 
-        UniValue obj(UniValue::VOBJ);
-        obj.push_back(Pair("hdseed", HexStr(hdChainCurrent.GetSeed())));
-        obj.push_back(Pair("mnemonic", ssMnemonic.c_str()));
-        obj.push_back(Pair("mnemonicpassphrase", ssMnemonicPassphrase.c_str()));
+    SecureString ssMnemonic;
+    SecureString ssMnemonicPassphrase;
+    hdChainCurrent.GetMnemonic(ssMnemonic, ssMnemonicPassphrase);
 
-        return obj;
-    }
+    UniValue obj(UniValue::VOBJ);
+    obj.push_back(Pair("hdseed", HexStr(hdChainCurrent.GetSeed())));
+    obj.push_back(Pair("mnemonic", ssMnemonic.c_str()));
+    obj.push_back(Pair("mnemonicpassphrase", ssMnemonicPassphrase.c_str()));
 
-    return NullUniValue;
+    return obj;
 }
 
 UniValue dumpwallet(const JSONRPCRequest& request)

[nmarley]

I agree that some feedback would be better than none.

Personally I would prefer if we could move to JSONRPC 2.0, avoiding JSONRPCErrors (or refactoring) and returning valid JSON even if an error is encountered, but I guess that's another discussion entirely.

[PastaPastaPasta]

Based on @UdjinM6's comment, the code has been refactored.
Relevant section:

    CHDChain hdChainCurrent;
    if (!pwalletMain->GetHDChain(hdChainCurrent))
        throw JSONRPCError(RPC_WALLET_ERROR, "This wallet is not a HD wallet.");

    if (!pwalletMain->GetDecryptedHDChain(hdChainCurrent))
        throw JSONRPCError(RPC_INTERNAL_ERROR, "Cannot decrypt HD seed");

    SecureString ssMnemonic;
    SecureString ssMnemonicPassphrase;
    hdChainCurrent.GetMnemonic(ssMnemonic, ssMnemonicPassphrase);

    UniValue obj(UniValue::VOBJ);
    obj.push_back(Pair("hdseed", HexStr(hdChainCurrent.GetSeed())));
    obj.push_back(Pair("mnemonic", ssMnemonic.c_str()));
    obj.push_back(Pair("mnemonicpassphrase", ssMnemonicPassphrase.c_str()));

    return obj;

[UdjinM6]

ACK

btw, diff without spaces https://github.com/dashpay/dash/pull/2134/files?w=1