APIv2(account_tx, noripple_check): return error for invalid input (fix #4543) #4620
Conversation
There was a problem hiding this comment.
I have similar comments that I had in previous PRs (#4577 (comment)). Better to keep it consistent.
…9/rippled into malformedOptParams
|
@ximinez if you mind reviewing this PR when you have the time. Thanks 😃 |
| @@ -137,7 +137,8 @@ doAccountInfo(RPC::JsonContext& context) | |||
| // The document states that signer_lists is a bool, however | |||
| // assigning any string value works. Do not allow this. | |||
| // This check is for api Version 2 onwards only | |||
| if (!params[jss::signer_lists].isBool() && context.apiVersion > 1) | |||
| if (params.isMember(jss::signer_lists) && | |||
There was a problem hiding this comment.
You don't have to explicitly check if jss::signer_lists is a member of params, because otherwise a NULL member is returned as a default. (Source:
rippled/src/ripple/json/json_value.h
Line 341 in aded4a7
I'm not saying that what you did is wrong, I am making sure that the existing code is not incorrect.
Does it make a difference that you are using 1u instead of 1, given that the left-hand-side context.apiVersion is an unsigned int?
There was a problem hiding this comment.
Would "1" not do an implicit type conversion from signed to unsigned ? I think "1u" avoids that extra step. No ?
There was a problem hiding this comment.
Checking that the field exists is generally best practice for readability, and because if the Json::Value is not const (which is the case for params), referencing the field will create the field if it doesn't exist. That can have negative side effects, from just making the object look ugly and wrong, to actually changing behavior.
| @@ -389,6 +389,20 @@ doAccountTxJson(RPC::JsonContext& context) | |||
| AccountTxArgs args; | |||
| Json::Value response; | |||
|
|
|||
| // The document states that binary and forward params is a boolean value, | |||
There was a problem hiding this comment.
what do you mean by "document" here? Is it the XRPL documentation website? Can you include a link for the same?
| @@ -137,7 +137,8 @@ doAccountInfo(RPC::JsonContext& context) | |||
| // The document states that signer_lists is a bool, however | |||
| // assigning any string value works. Do not allow this. | |||
| // This check is for api Version 2 onwards only | |||
| if (!params[jss::signer_lists].isBool() && context.apiVersion > 1) | |||
| if (params.isMember(jss::signer_lists) && | |||
There was a problem hiding this comment.
Checking that the field exists is generally best practice for readability, and because if the Json::Value is not const (which is the case for params), referencing the field will create the field if it doesn't exist. That can have negative side effects, from just making the object look ugly and wrong, to actually changing behavior.
src/test/rpc/NoRipple_test.cpp
Outdated
|
|
||
| if (params[jss::api_version] < 2) |
There was a problem hiding this comment.
There's a bunch more overhead to look up params[jss::api_version] and compare it to 2 than there would be to just compare apiVersion < 2u.
But more importantly, this check makes no sense. It makes more sense to run every test, then compare the results based on version. Yes, that will sometimes lead to repetition, but what happens if version 1 is eventually deprecated? Do you want these tests to be deleted? Of course not. You also want to run that version-2-only test under version 1 to demonstrate that the "asdf" is accepted as a valid value.
intelliot
changed the title
|
note: updates |
|
@arihantkothari - could you check whether all of your comments have been addressed? |
There was a problem hiding this comment.
Hey @intelliot , yes all my comments have been addressed. Thanks! 🚀
…LF#4620) For the `account_tx` and `noripple_check` methods, perform input validation for optional parameters such as "binary", "forward", "strict", "transactions". Previously, when these parameters had invalid values (e.g. not a bool), no error would be returned. Now, it returns an `invalidParams` error. * This updates the behavior to match Clio (https://github.com/XRPLF/clio). * Since this is potentially a breaking change, it only applies to requests specifying api_version: 2. * Fix XRPLF#4543.
…LF#4620) For the `account_tx` and `noripple_check` methods, perform input validation for optional parameters such as "binary", "forward", "strict", "transactions". Previously, when these parameters had invalid values (e.g. not a bool), no error would be returned. Now, it returns an `invalidParams` error. * This updates the behavior to match Clio (https://github.com/XRPLF/clio). * Since this is potentially a breaking change, it only applies to requests specifying api_version: 2. * Fix XRPLF#4543.
…LF#4620) For the `account_tx` and `noripple_check` methods, perform input validation for optional parameters such as "binary", "forward", "strict", "transactions". Previously, when these parameters had invalid values (e.g. not a bool), no error would be returned. Now, it returns an `invalidParams` error. * This updates the behavior to match Clio (https://github.com/XRPLF/clio). * Since this is potentially a breaking change, it only applies to requests specifying api_version: 2. * Fix XRPLF#4543.
High Level Overview of Change
Fixed issue #4543 resolving malformed input on certain parameters
Context of Change
Certain inputs for AccountTx and noRippleCheck should of error'ed out
Type of Change
Added own unit tests for API version 2