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.

Sign up for GitHub

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): return "invalidParams" when fields missing #4552

Merged
merged 16 commits into from
Jul 6, 2023
Merged

APIv2(ledger_entry): return "invalidParams" when fields missing #4552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
e770f0f
Error Code Change: unknownOption -> invalidParams
arihantkothari May 29, 2023
0961be3
Updated the tests
arihantkothari May 30, 2023
42ca435
Enable version 2 by setting ApiMaximumSupportedVersion as 2, and inva…
arihantkothari Jun 9, 2023
3687956
Tests for v1 and v2
arihantkothari Jun 9, 2023
973e2cb
making it consistent with account_info versioning support
arihantkothari Jun 16, 2023
744b419
Merge branch 'XRPLF:develop' into ledger_entry-invalidParams
arihantkothari Jun 22, 2023
50cbc1c
updating based on #4573 PR
arihantkothari Jun 22, 2023
c095bfb
followed guideline, version specific tests loop from min to beta, met…
arihantkothari Jun 27, 2023
2da51ed
Merge branch 'develop' into ledger_entry-invalidParams
arihantkothari Jun 28, 2023
4cf4856
2 (signed) -> 2u (unsigned)
arihantkothari Jun 28, 2023
13bf9cf
Merge branch 'develop' into ledger_entry-invalidParams
intelliot Jun 29, 2023
b486afc
Merge branch 'develop' into ledger_entry-invalidParams
intelliot Jun 30, 2023
0ed7cde
Merge branch 'develop' into ledger_entry-invalidParams
intelliot Jul 1, 2023
8298953
Merge branch 'develop' into ledger_entry-invalidParams
intelliot Jul 3, 2023
90b04cb
APIv2(ledger_entry): return "invalidParams" when fields missing (#4552)
arihantkothari Jul 4, 2023
eb61c13
Merge branch 'develop' into ledger_entry-invalidParams
intelliot Jul 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/ripple/rpc/handlers/LedgerEntry.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,12 @@ doLedgerEntry(RPC::JsonContext& context)
}
}
else
jvResult[jss::error] = "unknownOption";
{
if (context.apiVersion < 2u)
jvResult[jss::error] = "unknownOption";
else
jvResult[jss::error] = "invalidParams";
}
}

if (uNodeIndex.isNonZero())
Expand Down
23 changes: 19 additions & 4 deletions src/test/rpc/LedgerRPC_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <ripple/beast/unit_test.h>
#include <ripple/protocol/ErrorCodes.h>
#include <ripple/protocol/jss.h>
#include <ripple/rpc/impl/RPCHelpers.h>
#include <test/jtx.h>

namespace ripple {
Expand Down Expand Up @@ -1212,21 +1213,28 @@ class LedgerRPC_test : public beast::unit_test::suite
}

void
testLedgerEntryUnknownOption()
testLedgerEntryInvalidParams(unsigned int apiVersion)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what the convention is in rippled but i'd use uint32_t or perhaps uint16_t. Fine as it is if others are happy though

{
testcase("ledger_entry Request Unknown Option");
testcase(
"ledger_entry Request With Invalid Parameters v" +
std::to_string(apiVersion));
using namespace test::jtx;
Env env{*this};

std::string const ledgerHash{to_string(env.closed()->info().hash)};

// "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];
checkErrorValue(jrr, "unknownOption", "");

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

/// @brief ledger RPC requests as a way to drive
Expand Down Expand Up @@ -1724,11 +1732,18 @@ class LedgerRPC_test : public beast::unit_test::suite
testLedgerEntryPayChan();
testLedgerEntryRippleState();
testLedgerEntryTicket();
testLedgerEntryUnknownOption();
testLookupLedger();
testNoQueue();
testQueue();
testLedgerAccountsOption();

// version specific tests
for (auto testVersion = RPC::apiMinimumSupportedVersion;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It could be beneficial to extract this into some sort of helper function (maybe in the next pr) as i imagine this will be copy-pasted all over the place very soon. Better do it sooner than later. I'd go with a variadic template accepting callables (functions expecting api version as their input).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you are right, it will get copied. I'll try this out in the next PR. Thank you for pointing it out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

any idea what would be the most appropriate file to put this helper in ? RPCHelpers ? (cc: @ximinez )

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since it's only going to be used in tests, it should be under src/test. Env.h already has a lot of stuff, including the functions to make RPC calls, so maybe a static member or standalone function in there... Alternately, just create a new RPCUtils.h under src/test/jtx.

Another alternative if you feel like rewriting this test again is to create a member function of Env, so you can loop over each test case individually without the overhead of creating a new Env every time.

I don't know for sure, though.

testVersion <= RPC::apiBetaVersion;
arihantkothari marked this conversation as resolved.
Show resolved Hide resolved
++testVersion)
{
testLedgerEntryInvalidParams(testVersion);
}
intelliot marked this conversation as resolved.
Show resolved Hide resolved
}
};

Expand Down