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(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 all 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
237 changes: 158 additions & 79 deletions src/test/rpc/AccountTx_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ class AccountTx_test : public beast::unit_test::suite
};

void
testParameters()
testParameters(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.

Not related to this PR, but wondering why the test cases in this file do not have testcase(testCaseDesctiption) ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My guess is that we don't have any written, enforced guidelines for tests.

Copy link
Collaborator

@arihantkothari arihantkothari Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think it might be useful to add testcase(testCaseDescription + v<apiVersion>) so that users can see if the unit test is actually running on different versions when they run it. what do you think ? (maybe out of scope for this PR ?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think our test framework and tests are long overdue for a redesign with firm and written guidelines, but I won't worry about the inconsistency in this PR.

{
using namespace test::jtx;

Expand Down Expand Up @@ -143,104 +143,178 @@ class AccountTx_test : public beast::unit_test::suite
};

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

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

jParms[jss::account] = "0xDEADBEEF";

BEAST_EXPECT(isErr(
env.rpc("json", "account_tx", to_string(jParms)),
rpcACT_MALFORMED));

jParms[jss::account] = A1.human();
BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(jParms))));

// Ledger min/max index
if (apiVersion < 2)
{
Json::Value p{jParms};
p[jss::ledger_index_min] = -1;
p[jss::ledger_index_max] = -1;
BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));

p[jss::ledger_index_min] = 0;
p[jss::ledger_index_max] = 100;
BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));
BEAST_EXPECT(isErr(
env.rpc("json", "account_tx", to_string(jParms)),
rpcINVALID_PARAMS));

p[jss::ledger_index_min] = 1;
p[jss::ledger_index_max] = 2;
BEAST_EXPECT(noTxs(env.rpc("json", "account_tx", to_string(p))));
jParms[jss::account] = "0xDEADBEEF";

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

// Ledger index min only
{
Json::Value p{jParms};
p[jss::ledger_index_min] = -1;
BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));
jParms[jss::account] = A1.human();
BEAST_EXPECT(
hasTxs(env.rpc("json", "account_tx", to_string(jParms))));

p[jss::ledger_index_min] = 1;
BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));
// Ledger min/max index
{
Json::Value p{jParms};
p[jss::ledger_index_min] = -1;
p[jss::ledger_index_max] = -1;
BEAST_EXPECT(
hasTxs(env.rpc("json", "account_tx", to_string(p))));

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

p[jss::ledger_index_min] = 1;
p[jss::ledger_index_max] = 2;
BEAST_EXPECT(
noTxs(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)),
(RPC::apiMaximumSupportedVersion == 1
? rpcLGR_IDXS_INVALID
: rpcINVALID_LGR_RANGE)));
}

p[jss::ledger_index_min] = env.current()->info().seq;
BEAST_EXPECT(isErr(
env.rpc("json", "account_tx", to_string(p)),
(RPC::apiMaximumSupportedVersion == 1 ? rpcLGR_IDXS_INVALID
: rpcINVALID_LGR_RANGE)));
}
// Ledger index min only
{
Json::Value p{jParms};
p[jss::ledger_index_min] = -1;
BEAST_EXPECT(
hasTxs(env.rpc("json", "account_tx", to_string(p))));

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

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

// Ledger index max only
{
Json::Value p{jParms};
p[jss::ledger_index_max] = -1;
BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));
// Ledger index max only
{
Json::Value p{jParms};
p[jss::ledger_index_max] = -1;
BEAST_EXPECT(
hasTxs(env.rpc("json", "account_tx", to_string(p))));

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))));

p[jss::ledger_index_max] = env.closed()->info().seq - 1;
BEAST_EXPECT(
noTxs(env.rpc("json", "account_tx", to_string(p))));
}

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

p[jss::ledger_index_max] = env.closed()->info().seq;
BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));
p[jss::ledger_index] = env.closed()->info().seq;
BEAST_EXPECT(
hasTxs(env.rpc("json", "account_tx", to_string(p))));

p[jss::ledger_index_max] = env.closed()->info().seq - 1;
BEAST_EXPECT(noTxs(env.rpc("json", "account_tx", to_string(p))));
}
p[jss::ledger_index] = env.closed()->info().seq - 1;
BEAST_EXPECT(
noTxs(env.rpc("json", "account_tx", to_string(p))));

// Ledger Sequence
{
Json::Value p{jParms};
p[jss::ledger_index] = env.current()->info().seq;
BEAST_EXPECT(isErr(
env.rpc("json", "account_tx", to_string(p)),
rpcLGR_NOT_VALIDATED));

p[jss::ledger_index] = env.closed()->info().seq;
BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));
p[jss::ledger_index] = env.current()->info().seq + 1;
BEAST_EXPECT(isErr(
env.rpc("json", "account_tx", to_string(p)),
rpcLGR_NOT_FOUND));
}

p[jss::ledger_index] = env.closed()->info().seq - 1;
BEAST_EXPECT(noTxs(env.rpc("json", "account_tx", to_string(p))));
// Ledger Hash
{
Json::Value p{jParms};

p[jss::ledger_index] = env.current()->info().seq;
BEAST_EXPECT(isErr(
env.rpc("json", "account_tx", to_string(p)),
rpcLGR_NOT_VALIDATED));
p[jss::ledger_hash] = to_string(env.closed()->info().hash);
BEAST_EXPECT(
hasTxs(env.rpc("json", "account_tx", to_string(p))));

p[jss::ledger_index] = env.current()->info().seq + 1;
BEAST_EXPECT(isErr(
env.rpc("json", "account_tx", to_string(p)), rpcLGR_NOT_FOUND));
p[jss::ledger_hash] =
to_string(env.closed()->info().parentHash);
BEAST_EXPECT(
noTxs(env.rpc("json", "account_tx", to_string(p))));
}
}

// Ledger Hash
else
{
Json::Value p{jParms};
// Ledger index max/min/index all specified
// ERRORS out with invalid Parenthesis
{
jParms[jss::account] = "0xDEADBEEF";
jParms[jss::account] = A1.human();
Json::Value p{jParms};

p[jss::ledger_hash] = to_string(env.closed()->info().hash);
BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));
p[jss::ledger_index_max] = -1;
p[jss::ledger_index_min] = -1;
p[jss::ledger_index] = -1;

p[jss::ledger_hash] = to_string(env.closed()->info().parentHash);
BEAST_EXPECT(noTxs(env.rpc("json", "account_tx", to_string(p))));
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));
}
}
}

Expand Down Expand Up @@ -593,7 +667,12 @@ class AccountTx_test : public beast::unit_test::suite
void
run() override
{
testParameters();
for (auto testVersion = RPC::apiMinimumSupportedVersion;
testVersion <= RPC::apiBetaVersion;
++testVersion)
{
testParameters(testVersion);
}
testContents();
testAccountDelete();
}
Expand Down