Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APIv2(ledger_entry) : check error #4630

Merged
merged 9 commits into from
Sep 8, 2023
42 changes: 38 additions & 4 deletions src/ripple/rpc/handlers/LedgerEntry.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,15 @@ doLedgerEntry(RPC::JsonContext& context)

if (context.params.isMember(jss::index))
{
if (!uNodeIndex.parseHex(context.params[jss::index].asString()))
// XRPL Doc states "index" field is a string. check for this apiVersion
// 2 onwards. invalidParam error is thrown if the above condition is
// false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be precise, the error is not thrown, it's returned. I think it's worth rewriting this and all the other comments to say, "invalidParam error is returned if the above condition is false.", or even simpler, "invalidParam error is returned if not."

if (context.apiVersion > 1u && !context.params[jss::index].isString())
{
uNodeIndex = beast::zero;
jvResult[jss::error] = "invalidParams";
}
else if (!uNodeIndex.parseHex(context.params[jss::index].asString()))
{
uNodeIndex = beast::zero;
jvResult[jss::error] = "malformedRequest";
Expand All @@ -70,8 +78,15 @@ doLedgerEntry(RPC::JsonContext& context)
else if (context.params.isMember(jss::check))
{
expectedType = ltCHECK;

if (!uNodeIndex.parseHex(context.params[jss::check].asString()))
// XRPL Doc states "check" field is a string. Check for this apiVersion
// 2 onwards. invalidParam error is thrown if the above condition is
// false
if (context.apiVersion > 1u && !context.params[jss::check].isString())
{
uNodeIndex = beast::zero;
jvResult[jss::error] = "invalidParams";
}
else if (!uNodeIndex.parseHex(context.params[jss::check].asString()))
{
uNodeIndex = beast::zero;
jvResult[jss::error] = "malformedRequest";
Expand Down Expand Up @@ -125,7 +140,17 @@ doLedgerEntry(RPC::JsonContext& context)
}
else if (!context.params[jss::directory].isObject())
{
if (!uNodeIndex.parseHex(context.params[jss::directory].asString()))
// XRPL Doc states "directory" field is a string. check for this
// apiVersion 2 onwards. invalidParam error is thrown if the above
// condition is false
if (context.apiVersion > 1u &&
!context.params[jss::directory].isString())
{
uNodeIndex = beast::zero;
jvResult[jss::error] = "invalidParams";
}
else if (!uNodeIndex.parseHex(
context.params[jss::directory].asString()))
{
uNodeIndex = beast::zero;
jvResult[jss::error] = "malformedRequest";
Expand Down Expand Up @@ -333,6 +358,15 @@ doLedgerEntry(RPC::JsonContext& context)

if (context.params[jss::nft_page].isString())
{
// XRPL Doc states "nft_page" field is a string. check for this
// apiVersion 2 onwards. invalidParam error is thrown if the above
// condition is false
if (context.apiVersion > 1u &&
!context.params[jss::nft_page].isString())
{
uNodeIndex = beast::zero;
jvResult[jss::error] = "invalidParams";
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 359 is checking whether nft_page is a string, so the check on line 365 that it's a string can never fail. In other words, this block is unnecessary.

if (!uNodeIndex.parseHex(context.params[jss::nft_page].asString()))
{
uNodeIndex = beast::zero;
Expand Down
33 changes: 25 additions & 8 deletions src/test/rpc/LedgerRPC_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1226,15 +1226,32 @@ class LedgerRPC_test : public beast::unit_test::suite
// "features" is not an option supported by ledger_entry.
Json::Value jvParams;
jvParams[jss::api_version] = apiVersion;
jvParams[jss::features] = ledgerHash;
jvParams[jss::ledger_hash] = ledgerHash;
Json::Value const jrr =
env.rpc("json", "ledger_entry", to_string(jvParams))[jss::result];
{
jvParams[jss::features] = ledgerHash;
jvParams[jss::ledger_hash] = ledgerHash;
Json::Value const jrr = env.rpc(
"json", "ledger_entry", to_string(jvParams))[jss::result];

if (apiVersion < 2u)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: why is apiVersion checked with < 2u in this file compared > 1u? seems better to keep consistent throughout codebase (if most files have already established some standard)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the test file, it is specifically checking results for apiVersion < 2u and apiVersion other than that (>=2) , while in non-test files, the new error codes only execute if apiVersion is greater than 1. There is a guideline already established here:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't have access to that doc, but thanks for elaboration. If you don't mind sharing it, my email is [email protected]

Is > 1u in non-test file equivalent to >= 2u in test file? These seem to always be equivalent expressions since it is impossible to have decimal (1.5) here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea > 1u in non-test file is equivalent to >= 2u in test file.

checkErrorValue(jrr, "unknownOption", "");
else
checkErrorValue(jrr, "invalidParams", "");
}
// invalid check input
{
Json::Value checkParams;
checkParams[jss::owner] = "rhigTLJJyXXSRUyRCQtqi1NoAZZzZnS4KU";
checkParams[jss::ledger_index] = "validated";
jvParams[jss::check] = checkParams;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're reusing the jvParams from the previous test here, so that contains features, etc. To guarantee that you're testing what you expect to be testing, start with a new jvParams.


Json::Value const jrr = env.rpc(
"json", "ledger_entry", to_string(jvParams))[jss::result];

if (apiVersion < 2u)
checkErrorValue(jrr, "unknownOption", "");
else
checkErrorValue(jrr, "invalidParams", "");
if (apiVersion < 2u)
checkErrorValue(jrr, "internal", "Internal error.");
else
checkErrorValue(jrr, "invalidParams", "");
}
}

/// @brief ledger RPC requests as a way to drive
Expand Down