Wallet: do not return warnings from UpgradeWallet() #20139

pull stackman27 wants to merge 1 commits into bitcoin:master from stackman27:upgradewallet_rpc_cleanup changing 3 files +3 −5
  1. stackman27 commented at 9:07 PM on October 12, 2020: contributor

    The warning variable was unused in upgradewallet so I removed it

  2. stackman27 cross-referenced this on Oct 12, 2020 from issue upgradewallet rpc warnings and error by MarcoFalke
  3. in src/wallet/rpcwallet.cpp:4442 in 13952e451f outdated 
    4437 | @@ -4438,7 +4438,9 @@ static RPCHelpMan upgradewallet()
4438 |          {
4439 |              {"version", RPCArg::Type::NUM, /* default */ strprintf("%d", FEATURE_LATEST), "The version number to upgrade to. Default is the latest wallet version"}
4440 |          },
4441 | -        RPCResults{},
4442 | +        RPCResult{
4443 | +            RPCResult::Type::STR, "Error", "error description during upgrading wallet"
    MarcoFalke commented at 9:10 PM on October 12, 2020:

    Would be good to return an object instead. See

    doc/developer-notes.md:- Try to make the RPC response a JSON object.
    stackman27 commented at 5:49 AM on October 14, 2020:

    RPC Result changed from str to object

  4. MarcoFalke added this to the milestone 0.21.0 on Oct 12, 2020
  5. stackman27 force-pushed on Oct 12, 2020
  6. stackman27 force-pushed on Oct 12, 2020
  7. DrahtBot added the label RPC/REST/ZMQ on Oct 12, 2020
  8. DrahtBot added the label Wallet on Oct 12, 2020
  9. achow101 commented at 6:03 PM on October 13, 2020: member

    ACK c370e82bce3640d0ed99cff16e815b66853a8320

  10. laanwj commented at 9:01 AM on October 15, 2020: member

    I think this is a fairly confusing combination of changes in one commit. There's the doc update and the warnings unused field removal that make sense individually, but don't have much in common. (you could split it into two commits within this same PR maybe)

  11. stackman27 commented at 4:40 PM on October 15, 2020: contributor

    I think this is a fairly confusing combination of changes in one commit. There's the doc update and the warnings unused field removal that make sense individually, but don't have much in common. (you could split it into two commits within this same PR maybe)

    Do you suggest one commit as doc update and another as removal of unused field?

  12. stackman27 force-pushed on Oct 19, 2020
  13. stackman27 commented at 5:51 AM on October 19, 2020: contributor

    I think this is a fairly confusing combination of changes in one commit. There's the doc update and the warnings unused field removal that make sense individually, but don't have much in common. (you could split it into two commits within this same PR maybe)

    Fixed!

  14. S3RK commented at 3:06 AM on October 22, 2020: contributor

    Code review ACK d218f9e5ca0a39902abd55d0bb1a5e9d7829761d

  15. in src/wallet/rpcwallet.cpp:4464 in d218f9e5ca outdated 
    4467 | -    std::vector<bilingual_str> warnings;
4468 | -    if (!pwallet->UpgradeWallet(version, error, warnings)) {
4469 | +    if (!pwallet->UpgradeWallet(version, error)) {
4470 |          throw JSONRPCError(RPC_WALLET_ERROR, error.original);
4471 |      }
4472 |      return error.original;
    MarcoFalke commented at 7:25 AM on October 22, 2020:

    This is not an object what is returned here

    stackman27 commented at 7:16 AM on October 25, 2020:

    so should I have UpgradeWallet return an object? I'm a little confused

  16. luke-jr commented at 6:53 PM on October 24, 2020: member

    NACK, this doesn't make sense to me as-is.

    And even if we don't have any warnings today, it might be nice to keep the interface in case we do later.

  17. hebasto commented at 12:59 PM on October 25, 2020: member

    Weak Concept NACK. I lean to agree with @luke-jr about keeping (yet unused) warnings in the interface.

  18. MarcoFalke commented at 9:22 PM on October 25, 2020: member

    Why? If they are kept you'd have to return an always empty warnings array. Otherwise, it will get used in the future and surely it will be forgotten to update the RPC method to return the warnings.

  19. fanquake commented at 2:05 PM on October 27, 2020: member

    Concept ACK removing unused code.

  20. jnewbery commented at 9:10 AM on October 28, 2020: member

    There's the doc update and the warnings unused field removal that make sense individually, but don't have much in common.

    I agree with this. This should be split into two PRs.

    I think we can remove the 0.21 milestone. This isn't a bugfix and doesn't need to included in the next release.

  21. MarcoFalke commented at 9:16 AM on October 28, 2020: member

    Changing the return type is a breaking change if done after the 0.21 release. I don't see why such a trivial fix should pushed back.

  22. jnewbery commented at 10:03 AM on October 28, 2020: member

    Ah. For some reason I thought that upgradewallet had been in a previous release. I agree that we should fix the return format now to avoid a breaking RPC interface change. @stackman27 - I've fixed up the return value and tests (as well as cleaned up the commit logs) here: https://github.com/jnewbery/bitcoin/tree/pr20139.1. Feel free to take that branch.

  23. jnewbery renamed this:
    Removed unused warning and formatted RPC result
    Wallet: Change upgradewallet return type to be an object
    on Oct 28, 2020
  24. stackman27 commented at 6:52 PM on October 28, 2020: contributor

    Ah. For some reason I thought that upgradewallet had been in a previous release. I agree that we should fix the return format now to avoid a breaking RPC interface change.

    @stackman27 - I've fixed up the return value and tests (as well as cleaned up the commit logs) here: https://github.com/jnewbery/bitcoin/tree/pr20139.1. Feel free to take that branch.

    Should I have 2 different commits or 2 different PR's one for documentation and one for changing the return type ?

  25. jnewbery commented at 7:45 PM on October 28, 2020: member

    Both commits in this PR is fine

  26. stackman27 force-pushed on Oct 29, 2020
  27. in src/wallet/rpcwallet.cpp:4444 in 32a1aea7b2 outdated 
    4437 | @@ -4438,7 +4438,12 @@ static RPCHelpMan upgradewallet()
4438 |          {
4439 |              {"version", RPCArg::Type::NUM, /* default */ strprintf("%d", FEATURE_LATEST), "The version number to upgrade to. Default is the latest wallet version"}
4440 |          },
4441 | -        RPCResults{},
4442 | +        RPCResult{
4443 | +            RPCResult::Type::OBJ, "", "",
4444 | +            {
4445 | +                {RPCResult::Type::STR, "Error", "error description during upgrading wallet"}
    MarcoFalke commented at 6:43 AM on October 30, 2020:

    No need to return an error if there is none. Please make this optional like in analyzepsbt

    stackman27 commented at 4:31 AM on October 31, 2020:

    Done!

  28. stackman27 force-pushed on Oct 30, 2020
  29. meshcollider commented at 11:03 PM on November 1, 2020: contributor

    utACK c93f2ac1054ec07dbe5f3c80bfda3ca8cc2e48c0

  30. in src/wallet/rpcwallet.cpp:4470 in c93f2ac105 outdated 
    4469 | +    if (!pwallet->UpgradeWallet(version, error)) {
4470 |          throw JSONRPCError(RPC_WALLET_ERROR, error.original);
4471 |      }
4472 | -    return error.original;
4473 | +    UniValue obj(UniValue::VOBJ);
4474 | +    obj.pushKV("error", error.original);
    MarcoFalke commented at 7:19 AM on November 2, 2020:

    This is not optionally pushed

  31. jnewbery commented at 8:42 AM on November 2, 2020: member

    @stackman27, since we need the Return object from upgradewallet RPC for 0.21, I've pulled it into a separate PR #20282 and addressed Marco's review comment. You can use this PR for the Remove unused warning parameter in UpgradeWallet() commit, which doesn't require the 0.21 milestone.

  32. fanquake removed this from the milestone 0.21.0 on Nov 2, 2020
  33. MarcoFalke added the label Waiting for author on Nov 2, 2020
  34. jnewbery renamed this:
    Wallet: Change upgradewallet return type to be an object
    Wallet: do not return warnings from UpgradeWallet()
    on Nov 2, 2020
  35. DrahtBot cross-referenced this on Nov 2, 2020 from issue wallet: change upgradewallet return type to be an object by jnewbery
  36. DrahtBot commented at 3:49 PM on November 2, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #20403 (wallet: upgradewallet fixes, improvements, test coverage by jonatack)

    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.

  37. stackman27 force-pushed on Nov 2, 2020
  38. stackman27 force-pushed on Nov 2, 2020
  39. stackman27 commented at 10:33 PM on November 2, 2020: contributor

    @stackman27, since we need the Return object from upgradewallet RPC for 0.21, I've pulled it into a separate PR #20282 and addressed Marco's review comment. You can use this PR for the Remove unused warning parameter in UpgradeWallet() commit, which doesn't require the 0.21 milestone.

    Done! Could you please check and verify?

  40. meshcollider commented at 1:53 AM on November 4, 2020: contributor

    @stackman27 you may like to include the suggested changes from #20282#pullrequestreview-521620062 in this PR too

  41. MarcoFalke removed the label Waiting for author on Nov 4, 2020
  42. MarcoFalke removed the label RPC/REST/ZMQ on Nov 4, 2020
  43. MarcoFalke added the label Refactoring on Nov 4, 2020
  44. MarcoFalke commented at 8:05 AM on November 4, 2020: member

    review ACK bca54204b54e4ce82b59daa6ebac60020a810f76

  45. stackman27 force-pushed on Nov 9, 2020
  46. stackman27 force-pushed on Nov 9, 2020
  47. stackman27 force-pushed on Nov 9, 2020
  48. stackman27 force-pushed on Nov 13, 2020
  49. stackman27 commented at 7:20 PM on November 13, 2020: contributor

    @stackman27 you may like to include the suggested changes from #20282 (review) in this PR too

    Done! Could you please verify?

  50. stackman27 force-pushed on Nov 13, 2020
  51. DrahtBot cross-referenced this on Nov 16, 2020 from issue wallet: upgradewallet fixes, improvements, test coverage by jonatack
  52. in src/wallet/rpcwallet.cpp:4486 in 481f640495 outdated 
    4484 |      if (!error.empty()) {
4485 |          obj.pushKV("error", error.original);
4486 | +    } else {
4487 | +        if(!request.params[0].isNull()) {
4488 | +            obj.pushKV("result", "The wallet was upgraded successfully to version 169900.");
4489 | +        }
    jonatack commented at 6:53 PM on November 16, 2020:

    I think this addition can be dropped along with the test change (see also #20403, and apologies--I didn't realize you were working on this until DrahtBot mentioned it).

    stackman27 commented at 8:50 PM on November 16, 2020:

    okay so I'll just take the result object out

  53. jonatack commented at 6:55 PM on November 16, 2020: contributor

    Concept ACK on just removing the unused warnings. Will re-review after update.

  54. [upgradewallet] removed unused warning param 9636962889
  55. stackman27 force-pushed on Nov 16, 2020
  56. practicalswift commented at 9:56 AM on November 17, 2020: contributor

    ACK 963696288955dc31b3a4fd136bfb791a9d99755b: diff looks correct

    Thanks for removing unused stuff.

  57. MarcoFalke commented at 10:11 AM on November 17, 2020: member

    review ACK 963696288955dc31b3a4fd136bfb791a9d99755b

  58. jonatack commented at 11:05 AM on November 17, 2020: contributor

    ACK 963696288955dc31b3a4fd136bfb791a9d99755b

  59. MarcoFalke merged this on Nov 17, 2020
  60. MarcoFalke closed this on Nov 17, 2020
  61. sidhujag referenced this in commit 8412b5d478 on Nov 17, 2020
  62. Fabcien referenced this in commit dbfb1067ae on Dec 23, 2021
  63. bitcoin locked this on Feb 15, 2022
Contributors
stackman27MarcoFalkeachow101laanwjS3RKluke-jrhebastofanquakejnewberymeshcolliderDrahtBotjonatackpracticalswift
Labels
RefactoringWallet
Linked (view graph)
#20282 wallet: change upgradewallet return type to be an object#20403 wallet: upgradewallet fixes, improvements, test coverage
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:54 UTC