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 4 commits
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
82 changes: 82 additions & 0 deletions src/test/rpc/AccountTx_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ class AccountTx_test : public beast::unit_test::suite
};

Json::Value jParms;
jParms[jss::api_version] = 1;

BEAST_EXPECT(isErr(
env.rpc("json", "account_tx", to_string(jParms)),
Expand Down Expand Up @@ -205,6 +206,9 @@ class AccountTx_test : public beast::unit_test::suite
p[jss::ledger_index_max] = env.current()->info().seq;
BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));

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;
BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));

Expand Down Expand Up @@ -244,6 +248,83 @@ class AccountTx_test : public beast::unit_test::suite
}
}

void
testParametersApiV2()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might wanna:

  • Add a parameter for api_version.
  • Call this method in a loop over versions from apiMinimumSupportedVersion to apiBetaVersion.
  • Remove the other method.
  • Make the assertion on the error value conditional on the api_version parameter, using condition < 2 for the old behavior and else for the new behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

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

#4552 (comment)

{
using namespace test::jtx;
// test for API version 2
Env env{*this, envconfig([](std::unique_ptr<Config> c) {
c->loadFromString("\n[beta_rpc_api]\n1\n");
return c;
})};
PeterChen13579 marked this conversation as resolved.
Show resolved Hide resolved
Account A1{"A1"};
env.fund(XRP(10000), A1);
env.close();

auto hasTxs = [](Json::Value const& j) {
return j.isMember(jss::result) &&
(j[jss::result][jss::status] == "success") &&
(j[jss::result][jss::transactions].size() == 2) &&
(j[jss::result][jss::transactions][0u][jss::tx]
[jss::TransactionType] == jss::AccountSet) &&
(j[jss::result][jss::transactions][1u][jss::tx]
[jss::TransactionType] == jss::Payment);
};

auto isErr = [](Json::Value const& j, error_code_i code) {
return j.isMember(jss::result) &&
j[jss::result].isMember(jss::error) &&
j[jss::result][jss::error] == RPC::get_error_info(code).token;
};

Json::Value jParms;
jParms[jss::api_version] = 2;
jParms[jss::account] = A1.human();

// Ledger index max/min/index all specified
// 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;

BEAST_EXPECT(isErr(
env.rpc("json", "account_tx", to_string(p)),
rpcINVALID_PARAMS));
}

// Ledger index min/max only
{
Json::Value p{jParms};
p[jss::ledger_index_max] = 100;
p[jss::ledger_index_min] = 0;
BEAST_EXPECT(isErr(
env.rpc("json", "account_tx", to_string(p)),
rpcLGR_IDX_MALFORMED));

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

p[jss::ledger_index_min] = 2;
p[jss::ledger_index_max] = 1;
BEAST_EXPECT(isErr(
env.rpc("json", "account_tx", to_string(p)),
rpcINVALID_LGR_RANGE));
}

// Ledger index max only
{
Json::Value p{jParms};
p[jss::ledger_index_max] = env.current()->info().seq;
BEAST_EXPECT(isErr(
env.rpc("json", "account_tx", to_string(p)),
rpcLGR_IDX_MALFORMED));
}
}

void
testContents()
{
Expand Down Expand Up @@ -594,6 +675,7 @@ class AccountTx_test : public beast::unit_test::suite
run() override
{
testParameters();
testParametersApiV2();
testContents();
testAccountDelete();
}
Expand Down