Skip to content

Commit

Permalink
APIv2: add error messages for account_tx (XRPLF#4571)
Browse files Browse the repository at this point in the history 
Certain inputs for the AccountTx method should return an error. In other
words, an invalid request from a user or client now results in an error
message.

Since this can change the response from the API, it is an API breaking
change. This commit maintains backward compatibility by keeping the
existing behavior for existing requests. When clients specify
"api_version": 2, they will be able to get the updated error messages.

Update unit tests to check the error based on the API version.

* Fix XRPLF#4288
* Fix XRPLF#4545
  • Loading branch information
@PeterChen13579 @ckeshava
PeterChen13579 authored and ckeshava committed Sep 22, 2023
1 parent 384b5ab commit 179ab0a
Show file tree
Hide file tree
Showing 2 changed files with 185 additions and 81 deletions.
29 changes: 27 additions & 2 deletions src/ripple/rpc/handlers/AccountTx.cpp
View file
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)
{
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
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)
{
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

0 comments on commit 179ab0a

Please sign in to comment.