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

Jump to bottom

APIv2(AccountTx): Added error messages on AccountTx #4571

Merged
merged 8 commits into from
Jun 29, 2023
Merged

APIv2(AccountTx): Added error messages on AccountTx #4571

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4dadab6
Changed error messages on AccountTx
PeterChen13579 Jun 13, 2023
2f3c7e3
Fixed unit test based on comments given
PeterChen13579 Jun 14, 2023
f123188
Added clang format
PeterChen13579 Jun 14, 2023
42ca45f
Merge branch 'XRPLF:develop' into AccountTxErrorFixes
PeterChen13579 Jun 17, 2023
7c838eb
Merge remote-tracking branch 'upstream/develop' into AccountTxErrorFixes
PeterChen13579 Jun 23, 2023
ac88045
changed unit test based on Ed's change
PeterChen13579 Jun 23, 2023
86a9b31
changed unittest based on Arihant's comment
PeterChen13579 Jun 27, 2023
2e1e1a8
Merge branch 'develop' into AccountTxErrorFixes
intelliot Jun 28, 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
29 changes: 27 additions & 2 deletions src/ripple/rpc/handlers/AccountTx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,23 @@ using LedgerSpecifier = RelationalDatabase::LedgerSpecifier;

// parses args into a ledger specifier, or returns a Json object on error
std::variant<std::optional<LedgerSpecifier>, Json::Value>
parseLedgerArgs(Json::Value const& params)
parseLedgerArgs(RPC::Context& context, Json::Value const& params)
{
Json::Value response;
// if ledger_index_min or max is specified, then ledger_hash or ledger_index
// should not be specified. Error out if it is
if (context.apiVersion > 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

not so important, but I'm wondering if it'll be nice to make constants like API_VERSION_1 and API_VERSION_2 instead of using raw numbers 1 and 2 since we are using it in a lot of places ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it's worth it when we're just using consecutive integers to name the versions.

{
if ((params.isMember(jss::ledger_index_min) ||
params.isMember(jss::ledger_index_max)) &&
(params.isMember(jss::ledger_hash) ||
params.isMember(jss::ledger_index)))
{
RPC::Status status{rpcINVALID_PARAMS, "invalidParams"};
status.inject(response);
return response;
}
}
if (params.isMember(jss::ledger_index_min) ||
params.isMember(jss::ledger_index_max))
{
Expand Down Expand Up @@ -145,6 +159,17 @@ getLedgerRange(
using T = std::decay_t<decltype(ls)>;
if constexpr (std::is_same_v<T, LedgerRange>)
{
// if ledger_index_min or ledger_index_max is out of
// valid ledger range, error out. exclude -1 as
// it is a valid input
if (context.apiVersion > 1)
{
if ((ls.max > uValidatedMax && ls.max != -1) ||
(ls.min < uValidatedMin && ls.min != 0))
{
return rpcLGR_IDX_MALFORMED;
}
}
if (ls.min > uValidatedMin)
{
uLedgerMin = ls.min;
Expand Down Expand Up @@ -379,7 +404,7 @@ doAccountTxJson(RPC::JsonContext& context)

args.account = *account;

auto parseRes = parseLedgerArgs(params);
auto parseRes = parseLedgerArgs(context, params);
if (auto jv = std::get_if<Json::Value>(&parseRes))
{
return *jv;
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/rpc/impl/RPCHelpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ extern beast::SemanticVersion const lastVersion;
constexpr unsigned int apiInvalidVersion = 0;
constexpr unsigned int apiVersionIfUnspecified = 1;
constexpr unsigned int apiMinimumSupportedVersion = 1;
constexpr unsigned int apiMaximumSupportedVersion = 1;
constexpr unsigned int apiMaximumSupportedVersion = 2;
constexpr unsigned int apiBetaVersion = 2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't change the apiMaximumSupportedVersion until the updated version is locked in and final and ready for release. We have apiBetaVersion variable right here and the [beta_rpc_api] config option available to allow testing the changes. (It might not be a bad idea to enable BETA_RPC_API = true by default in envconfig.cpp.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wondering what do you mean by "until the updated version is locked in and final and ready for release."? Do you you mean until PR #4522 is merged into develop, so I don't have to change apiMaximumSupportedVersion to 2?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll be popping over to add the same kind of comment to #4568 and #4552 after I'm done here.

"until the updated version is locked in and final and ready for release" means that once a version of rippled is released with apiMaximumSupportedVersion is set to 2, then we are saying that API v2 is Done. Once that happens we can not legitimately make any further changes to API version 2. To the best of my knowledge, API version 2 is still in beta, under active development, and still changing.

Now, if you (and #4568 and #4552) are saying that version 2 is done, let's talk about version 2 being done. This will be the first I've heard about it.

If you want a node to properly handle API v2 requests, the correct way to do that, for now, is with the [beta_rpc_api] config option. And if you want to do it in unit tests, set it up in the config similar to how it's done in https://github.com/XRPLF/rippled/blob/develop/src/test/rpc/AccountInfo_test.cpp#L209-L212, or just by setting cfg.BETA_RPC_API=true; directly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please see also #4573.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to close the loop on this: API v2 will be considered "Done" in the 1.12 release. Any breaking changes in 1.13 will be API v3.


static_assert(apiMinimumSupportedVersion >= apiVersionIfUnspecified);
Expand Down
45 changes: 43 additions & 2 deletions src/test/rpc/AccountTx_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,11 +166,21 @@ class AccountTx_test : public beast::unit_test::suite

p[jss::ledger_index_min] = 0;
p[jss::ledger_index_max] = 100;
BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));
RPC::apiMaximumSupportedVersion == 1
? BEAST_EXPECT(
hasTxs(env.rpc("json", "account_tx", to_string(p))))
: BEAST_EXPECT(isErr(
env.rpc("json", "account_tx", to_string(p)),
rpcLGR_IDX_MALFORMED));
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should copy these tests for an explicit api_version == 1, like this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To follow on @thejohnfreeman's suggestion, loop over both versions for these tests, because we need to keep supporting version 1 indefinitely and ensure it's working correctly.


p[jss::ledger_index_min] = 1;
p[jss::ledger_index_max] = 2;
BEAST_EXPECT(noTxs(env.rpc("json", "account_tx", to_string(p))));
RPC::apiMaximumSupportedVersion == 1
? BEAST_EXPECT(
noTxs(env.rpc("json", "account_tx", to_string(p))))
: BEAST_EXPECT(isErr(
env.rpc("json", "account_tx", to_string(p)),
rpcLGR_IDX_MALFORMED));

p[jss::ledger_index_min] = 2;
p[jss::ledger_index_max] = 1;
Expand All @@ -187,6 +197,14 @@ class AccountTx_test : public beast::unit_test::suite
BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));

p[jss::ledger_index_min] = 1;
RPC::apiMaximumSupportedVersion == 1
? BEAST_EXPECT(
hasTxs(env.rpc("json", "account_tx", to_string(p))))
: BEAST_EXPECT(isErr(
env.rpc("json", "account_tx", to_string(p)),
rpcLGR_IDX_MALFORMED));

p[jss::ledger_index_min] = 2;
BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));

p[jss::ledger_index_min] = env.current()->info().seq;
Expand All @@ -203,6 +221,14 @@ class AccountTx_test : public beast::unit_test::suite
BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));

p[jss::ledger_index_max] = env.current()->info().seq;
RPC::apiMaximumSupportedVersion == 1
? BEAST_EXPECT(
hasTxs(env.rpc("json", "account_tx", to_string(p))))
: BEAST_EXPECT(isErr(
env.rpc("json", "account_tx", to_string(p)),
rpcLGR_IDX_MALFORMED));

p[jss::ledger_index_max] = 3;
BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));

p[jss::ledger_index_max] = env.closed()->info().seq;
Expand All @@ -212,6 +238,21 @@ class AccountTx_test : public beast::unit_test::suite
BEAST_EXPECT(noTxs(env.rpc("json", "account_tx", to_string(p))));
}

// Ledger index max/min/index
// ERRORS out with invalid Parenthesis
{
Json::Value p{jParms};
p[jss::ledger_index_max] = -1;
p[jss::ledger_index_min] = -1;
p[jss::ledger_index] = -1;
RPC::apiMaximumSupportedVersion == 1
? BEAST_EXPECT(
hasTxs(env.rpc("json", "account_tx", to_string(p))))
: BEAST_EXPECT(isErr(
env.rpc("json", "account_tx", to_string(p)),
rpcINVALID_PARAMS));
}

// Ledger Sequence
{
Json::Value p{jParms};
Expand Down