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. 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

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

    ACK c370e82bce3640d0ed99cff16e815b66853a8320

  9. 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)

  10. 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?

  11. stackman27 force-pushed on Oct 19, 2020
  12. 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!

  13. S3RK commented at 3:06 AM on October 22, 2020: member

    Code review ACK d218f9e5ca0a39902abd55d0bb1a5e9d7829761d

  14. 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

  15. 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.

  16. 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.

  17. 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.

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

    Concept ACK removing unused code.

  19. 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.

  20. 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.

  21. 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.

  22. jnewbery renamed this:
    Removed unused warning and formatted RPC result
    Wallet: Change upgradewallet return type to be an object
    on Oct 28, 2020
  23. 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 ?

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

    Both commits in this PR is fine

  25. stackman27 force-pushed on Oct 29, 2020
  26. 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!

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

    utACK c93f2ac1054ec07dbe5f3c80bfda3ca8cc2e48c0

  29. 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

  30. 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.

  31. fanquake removed this from the milestone 0.21.0 on Nov 2, 2020
  32. MarcoFalke added the label Waiting for author on Nov 2, 2020
  33. jnewbery renamed this:
    Wallet: Change upgradewallet return type to be an object
    Wallet: do not return warnings from UpgradeWallet()
    on Nov 2, 2020
  34. DrahtBot commented at 3:49 PM on November 2, 2020: member

    <!--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.

  35. stackman27 force-pushed on Nov 2, 2020
  36. stackman27 force-pushed on Nov 2, 2020
  37. 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?

  38. 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

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

    review ACK bca54204b54e4ce82b59daa6ebac60020a810f76

  43. stackman27 force-pushed on Nov 9, 2020
  44. stackman27 force-pushed on Nov 9, 2020
  45. stackman27 force-pushed on Nov 9, 2020
  46. stackman27 force-pushed on Nov 13, 2020
  47. 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?

  48. stackman27 force-pushed on Nov 13, 2020
  49. 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

  50. jonatack commented at 6:55 PM on November 16, 2020: member

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

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

    ACK 963696288955dc31b3a4fd136bfb791a9d99755b: diff looks correct

    Thanks for removing unused stuff.

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

    review ACK 963696288955dc31b3a4fd136bfb791a9d99755b

  55. jonatack commented at 11:05 AM on November 17, 2020: member

    ACK 963696288955dc31b3a4fd136bfb791a9d99755b

  56. MarcoFalke merged this on Nov 17, 2020
  57. MarcoFalke closed this on Nov 17, 2020
  58. sidhujag referenced this in commit 8412b5d478 on Nov 17, 2020
  59. Fabcien referenced this in commit dbfb1067ae on Dec 23, 2021
  60. DrahtBot locked this on Feb 15, 2022
Contributors
stackman27MarcoFalkeachow101laanwjS3RKluke-jrhebastofanquakejnewberymeshcolliderDrahtBotjonatackpracticalswift
Labels
RefactoringWallet
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-02 18:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me