rpc: Adjust RPCTypeCheckObj error string #26240

leo-aa88 · 2022-10-03T21:40:29Z

Unifies the JSON type error strings as mentioned in #26214. Also refer to #25737.

test/functional/wallet_signrawtransactionwithwallet.py

test/functional/rpc_mempool_info.py

test/functional/wallet_send.py

maflcko

I don't follow the changes. The changes shouldn't change any logic, but only adjust a format string

leo-aa88 · 2022-10-04T12:56:19Z

I don't follow the changes. The changes shouldn't change any logic, but only adjust a format string

I might have misunderstood the issue, but it mentions the unification of the error string.

src/rpc/util.cpp

maflcko

Thanks. Lgtm, feel free to ignore the nit

test/functional/rpc_fundrawtransaction.py

maflcko · 2022-10-04T17:59:24Z

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

src/rpc/util.cpp

furszy

Code ACK db218795

Need to cleanup the commit message, it's currently saying "rpc: Adjust RPCTypeCheckObj error string" three times.

src/rpc/util.cpp

furszy

Something that #26214 isn't stating is that, with this change, we will remove the name of the invalid arg in the returned error description.

e.g. the user will receive a general "JSON value of type string is not of expected type number". When before, was receiving the name of the invalid parameter: "Expected type number for conf_target, got string".

I'm more inclined to add the invalid parameter name into the RPCTypeCheckArgument error string rather than remove it from RPCTypeCheckObj. It's useful to quickly detect which arg type is wrong when you type a long command.

This refactor could be done in a follow-up PR, but if we are agree over this point, then instead of calling RPCTypeCheckArgument inside RPCTypeCheckObj, we can just change the returned string to be:

strprintf("JSON value of type %s for arg %s is not of expected type %s", uvTypeName(v.type()),  t.first, uvTypeName(t.second.type)));

leo-aa88 · 2022-10-07T15:00:27Z

Something that #26214 isn't stating is that, with this change, we will remove the name of the invalid arg in the returned error description.

e.g. the user will receive a general "JSON value of type string is not of expected type number". When before, was receiving the name of the invalid parameter: "Expected type number for conf_target, got string".

I'm more inclined to add the invalid parameter name into the RPCTypeCheckArgument error string rather than remove it from RPCTypeCheckObj. It's useful to quickly detect which arg type is wrong when you type a long command.

This refactor could be done in a follow-up PR, but if we are agree over this point, then instead of calling RPCTypeCheckArgument inside RPCTypeCheckObj, we can just change the returned string to be:
strprintf("JSON value of type %s for arg %s is not of expected type %s", uvTypeName(v.type()),  t.first, uvTypeName(t.second.type)));

This modification would modify the signature of the function void RPCTypeCheckArgument(const UniValue& value, const UniValueType& typeExpected) to void RPCTypeCheckArgument(const UniValue& value, const std::pair<const std::string, const UniValueType> & typeExpected) and this would conflict with its use in RPCTypeCheck. I can visualize two ways of handling this: either by modifying RPCTypeCheckArgument to accept different argument types and handle them internally or modifying the RPCTypeCheck function itself. The latter would involve bigger changes in the code.

maflcko · 2022-10-07T15:43:10Z

I'd say to just leave the code logic as it was before and only change the string

leo-aa88 · 2022-10-08T03:18:23Z

The file feature_backwards_compatibility.py raised the following error while running the ci test. I'm not sure about how to handle and interpret it. Locally on my machine all tests have passed.

131/252 - feature_backwards_compatibility.py --descriptors
 failed, Duration: 15 s
stdout:
2022-10-08T01:49:20.923000Z TestFramework (INFO): Initializing test directory /tmp/cirrus-build/ci/scratch/test_runner/test_runner_₿_🏃_20221008_014101/feature_backwards_compatibility_117
2022-10-08T01:49:26.955000Z TestFramework (INFO): Test wallet backwards compatibility...
2022-10-08T01:49:32.888000Z TestFramework (INFO): Test wallet upgrade path...
2022-10-08T01:49:33.602000Z TestFramework (INFO): Stopping nodes
stderr:
Traceback (most recent call last):
  File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_backwards_compatibility.py", line 329, in <module>
    BackwardsCompatibilityTest().main()
  File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 156, in main
    exit_code = self.shutdown()
  File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 311, in shutdown
    self.stop_nodes()
  File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 571, in stop_nodes
    node.wait_until_stopped()
  File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 384, in wait_until_stopped
    wait_until_helper(self.is_node_stopped, timeout=timeout, timeout_factor=self.timeout_factor)
  File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 270, in wait_until_helper
    if predicate():
  File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 375, in is_node_stopped
    "Node returned non-zero exit code (%d) when stopping" % return_code)
AssertionError: [node 9] Node returned non-zero exit code (-6) when stopping

maflcko · 2022-10-10T07:42:47Z

src/rpc/util.cpp

Any reason to remove the {}? According to the dev notes we want them

Any reason to remove the {}? According to the dev notes we want them

Yes, this would also reduce the diff.

src/rpc/util.cpp

aureleoules

ACK e0a2cbe40ce51079472593330f8de39c78397a23 LGTM

aureleoules · 2022-10-10T08:08:37Z

src/rpc/util.cpp

Any reason to remove the {}? According to the dev notes we want them

Yes, this would also reduce the diff.

furszy

ACK 2dede9f

DrahtBot · 2022-11-12T05:20:35Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26485 (RPC: Add support for name-only parameters 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.

2dede9f Adjust RPCTypeCheckObj error string (Leonardo Araujo) Pull request description: Unifies the JSON type error strings as mentioned in bitcoin#26214. Also refer to bitcoin#25737. ACKs for top commit: furszy: ACK 2dede9f Tree-SHA512: c918889e347ba32cb6d0e33c0de5956c2077dd40c996151e16741b0c4983ff098c60258206ded76ad7bbec4876c780c6abb494a97e4f1e05717d28a59b9167a6

fanquake requested review from furszy and maflcko October 3, 2022 21:49

DrahtBot added the RPC/REST/ZMQ label Oct 3, 2022

maflcko reviewed Oct 4, 2022

View reviewed changes

test/functional/wallet_signrawtransactionwithwallet.py Outdated Show resolved Hide resolved

maflcko reviewed Oct 4, 2022

View reviewed changes

test/functional/rpc_mempool_info.py Outdated Show resolved Hide resolved

maflcko reviewed Oct 4, 2022

View reviewed changes

test/functional/wallet_send.py Outdated Show resolved Hide resolved

maflcko reviewed Oct 4, 2022

View reviewed changes

src/rpc/util.cpp Outdated Show resolved Hide resolved

furszy reviewed Oct 4, 2022

View reviewed changes

src/rpc/util.cpp Outdated Show resolved Hide resolved

leo-aa88 force-pushed the rpc/adjust-rpc_type_check_obj-error-string branch from edf89d2 to 8a495b9 Compare October 4, 2022 16:52

maflcko approved these changes Oct 4, 2022

View reviewed changes

test/functional/rpc_fundrawtransaction.py Outdated Show resolved Hide resolved

leo-aa88 force-pushed the rpc/adjust-rpc_type_check_obj-error-string branch from 7a80633 to 1640a5e Compare October 4, 2022 18:06

furszy reviewed Oct 5, 2022

View reviewed changes

src/rpc/util.cpp Outdated Show resolved Hide resolved

leo-aa88 force-pushed the rpc/adjust-rpc_type_check_obj-error-string branch from 1640a5e to db21879 Compare October 5, 2022 02:48

furszy reviewed Oct 6, 2022

View reviewed changes

src/rpc/util.cpp Outdated Show resolved Hide resolved

leo-aa88 force-pushed the rpc/adjust-rpc_type_check_obj-error-string branch from db21879 to 2b5fcc2 Compare October 7, 2022 01:39

furszy reviewed Oct 7, 2022

View reviewed changes

leo-aa88 force-pushed the rpc/adjust-rpc_type_check_obj-error-string branch from 2b5fcc2 to e0a2cbe Compare October 8, 2022 01:30

maflcko approved these changes Oct 10, 2022

View reviewed changes

aureleoules approved these changes Oct 10, 2022

View reviewed changes

Adjust RPCTypeCheckObj error string

2dede9f

leo-aa88 force-pushed the rpc/adjust-rpc_type_check_obj-error-string branch from e0a2cbe to 2dede9f Compare October 10, 2022 21:08

furszy approved these changes Oct 11, 2022

View reviewed changes

DrahtBot mentioned this pull request Nov 12, 2022

RPC: Accept options as named-only parameters #26485

Merged

maflcko merged commit 48174c0 into bitcoin:master Nov 14, 2022

fanquake mentioned this pull request Nov 14, 2022

Adjust RPCTypeCheckObj error string #26214

Closed

bitcoin locked and limited conversation to collaborators Nov 14, 2023

leo-aa88 deleted the rpc/adjust-rpc_type_check_obj-error-string branch January 22, 2024 03:42

rpc: Adjust RPCTypeCheckObj error string #26240

rpc: Adjust RPCTypeCheckObj error string #26240

Uh oh!

Conversation

leo-aa88 commented Oct 3, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

leo-aa88 commented Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Oct 4, 2022

Uh oh!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

leo-aa88 commented Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Oct 7, 2022

Uh oh!

leo-aa88 commented Oct 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

aureleoules Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

aureleoules Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Nov 12, 2022

Conflicts

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

leo-aa88 commented Oct 4, 2022 •

edited

Loading

leo-aa88 commented Oct 7, 2022 •

edited

Loading

leo-aa88 commented Oct 8, 2022 •

edited

Loading