wallet: Add missing check for -descriptors wallet tool option #20687

1. 
   MarcoFalke commented at 6:42 pm on December 17, 2020: member

   Also, fix a test failure when compiled without sqlite
2. wallet: Pass ArgsManager into ExecuteWalletToolFunc instead of using global fa7dde1c41
3. MarcoFalke added the label Wallet on Dec 17, 2020
4. test: Add missing check for is_sqlite_compiled faf8f61368
5. wallet: Add missing check for -descriptors wallet tool option fae32f295c
6. MarcoFalke force-pushed on Dec 17, 2020
7. ryanofsky approved
8. 
   ryanofsky commented at 7:42 pm on December 17, 2020: member

   Code review ACK fae32f295cc5b57c1cb95090bb60cddb42f9778a. Thanks for implementing the -descriptors check and dealing with the test failure!
9. in test/functional/tool_wallet.py:347 in fae32f295c

   343@@ -344,11 +344,13 @@ def test_dump_createfromdump(self):
   344         self.assert_raises_tool_error('Dump file {} does not exist.'.format(non_exist_dump), '-wallet=todump', '-dumpfile={}'.format(non_exist_dump), 'createfromdump')
   345         wallet_path = os.path.join(self.nodes[0].datadir, 'regtest/wallets/todump2')
   346         self.assert_raises_tool_error('Failed to create database path \'{}\'. Database already exists.'.format(wallet_path), '-wallet=todump2', '-dumpfile={}'.format(wallet_dump), 'createfromdump')
   347+        self.assert_raises_tool_error("The -descriptors option can only be used with the 'create' command.", '-descriptors', '-wallet=todump2', '-dumpfile={}'.format(wallet_dump), 'createfromdump')
   

   ---

   ---

   

   jonatack commented at 8:23 pm on December 17, 2020:

   style nit, can use f-strings if you retouch
10. in src/wallet/wallettool.cpp:119 in fae32f295c

    119+    if (args.IsArgSet("-dumpfile") && command != "dump" && command != "createfromdump") {
    120         tfm::format(std::cerr, "The -dumpfile option can only be used with the \"dump\" and \"createfromdump\" commands.\n");
    121         return false;
    122     }
    123+    if (args.IsArgSet("-descriptors") && command != "create") {
    124+        tfm::format(std::cerr, "The -descriptors option can only be used with the 'create' command.\n");
    

    ---

    ---

    

    jonatack commented at 8:26 pm on December 17, 2020:

    the other error messages use double quotes around the command names (lines 112, 117)

    0        tfm::format(std::cerr, "The -descriptors option can only be used with the \"create\" command.\n");
    

    ---

    

    MarcoFalke commented at 8:35 pm on December 17, 2020:

    the help text uses single quotes, so I used that for consistency

    ---

    

    jonatack commented at 8:39 pm on December 17, 2020:

    yup, i noticed the help follows a different format than the errors
11. 
    jonatack commented at 8:28 pm on December 17, 2020: member

    Code review utACK fae32f295cc5b57c1cb95090bb60cddb42f9778a
12. 
    DrahtBot commented at 6:34 am on December 18, 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:

    • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option 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.

13. MarcoFalke merged this on Dec 18, 2020
14. MarcoFalke closed this on Dec 18, 2020

    ---

15. MarcoFalke deleted the branch on Dec 18, 2020
16. sidhujag referenced this in commit 776e695a8a on Dec 18, 2020
17. DrahtBot locked this on Feb 15, 2022

Contributors
MarcoFalke ryanofsky jonatack DrahtBot

Labels
Wallet