Skip to content

Commit

Permalink
Change order of checks in amm_info: (XRPLF#4924)
Browse files Browse the repository at this point in the history 
* Change order of checks in amm_info

* Change amm_info error message in API version 3

* Change amm_info error tests
  • Loading branch information
@Bronek
Bronek authored Jun 18, 2024
1 parent 223e6c7 commit c706926
Show file tree
Hide file tree
Showing 4 changed files with 136 additions and 25 deletions.
23 changes: 17 additions & 6 deletions src/ripple/rpc/handlers/AMMInfo.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,15 @@ doAMMInfo(RPC::JsonContext& context)
std::optional<Issue> issue2;
std::optional<uint256> ammID;

if ((params.isMember(jss::asset) != params.isMember(jss::asset2)) ||
(params.isMember(jss::asset) == params.isMember(jss::amm_account)))
constexpr auto invalid = [](Json::Value const& params) -> bool {
return (params.isMember(jss::asset) !=
params.isMember(jss::asset2)) ||
(params.isMember(jss::asset) ==
params.isMember(jss::amm_account));
};

// NOTE, identical check for apVersion >= 3 below
if (context.apiVersion < 3 && invalid(params))
return Unexpected(rpcINVALID_PARAMS);

if (params.isMember(jss::asset))
Expand Down Expand Up @@ -127,17 +134,21 @@ doAMMInfo(RPC::JsonContext& context)
ammID = sle->getFieldH256(sfAMMID);
}

assert(
(issue1.has_value() == issue2.has_value()) &&
(issue1.has_value() != ammID.has_value()));

if (params.isMember(jss::account))
{
accountID = getAccount(params[jss::account], result);
if (!accountID || !ledger->read(keylet::account(*accountID)))
return Unexpected(rpcACT_MALFORMED);
}

// NOTE, identical check for apVersion < 3 above
if (context.apiVersion >= 3 && invalid(params))
return Unexpected(rpcINVALID_PARAMS);

assert(
(issue1.has_value() == issue2.has_value()) &&
(issue1.has_value() != ammID.has_value()));

auto const ammKeylet = [&]() {
if (issue1 && issue2)
return keylet::amm(*issue1, *issue2);
Expand Down
3 changes: 2 additions & 1 deletion src/test/jtx/AMM.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,8 @@ class AMM
std::optional<Issue> issue1 = std::nullopt,
std::optional<Issue> issue2 = std::nullopt,
std::optional<AccountID> const& ammAccount = std::nullopt,
bool ignoreParams = false) const;
bool ignoreParams = false,
unsigned apiVersion = RPC::apiInvalidVersion) const;

/** Verify the AMM balances.
*/
Expand Down
8 changes: 6 additions & 2 deletions src/test/jtx/impl/AMM.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,8 @@ AMM::ammRpcInfo(
std::optional<Issue> issue1,
std::optional<Issue> issue2,
std::optional<AccountID> const& ammAccount,
bool ignoreParams) const
bool ignoreParams,
unsigned apiVersion) const
{
Json::Value jv;
if (account)
Expand Down Expand Up @@ -191,7 +192,10 @@ AMM::ammRpcInfo(
if (ammAccount)
jv[jss::amm_account] = to_string(*ammAccount);
}
auto jr = env_.rpc("json", "amm_info", to_string(jv));
auto jr =
(apiVersion == RPC::apiInvalidVersion
? env_.rpc("json", "amm_info", to_string(jv))
: env_.rpc(apiVersion, "json", "amm_info", to_string(jv)));
if (jr.isObject() && jr.isMember(jss::result) &&
jr[jss::result].isMember(jss::status))
return jr[jss::result];
Expand Down
127 changes: 111 additions & 16 deletions src/test/rpc/AMMInfo_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,19 @@ class AMMInfo_test : public jtx::AMMTestBase
testcase("Errors");

using namespace jtx;

Account const bogie("bogie");
enum TestAccount { None, Alice, Bogie };
auto accountId = [&](AMM const& ammAlice,
TestAccount v) -> std::optional<AccountID> {
if (v == Alice)
return ammAlice.ammAccount();
else if (v == Bogie)
return bogie;
else
return std::nullopt;
};

// Invalid tokens pair
testAMM([&](AMM& ammAlice, Env&) {
Account const gw("gw");
Expand All @@ -48,36 +61,70 @@ class AMMInfo_test : public jtx::AMMTestBase

// Invalid LP account id
testAMM([&](AMM& ammAlice, Env&) {
Account bogie("bogie");
auto const jv = ammAlice.ammRpcInfo(bogie.id());
BEAST_EXPECT(jv[jss::error_message] == "Account malformed.");
});

std::vector<std::tuple<
std::optional<Issue>,
std::optional<Issue>,
TestAccount,
bool>> const invalidParams = {
{xrpIssue(), std::nullopt, None, false},
{std::nullopt, USD.issue(), None, false},
{xrpIssue(), std::nullopt, Alice, false},
{std::nullopt, USD.issue(), Alice, false},
{xrpIssue(), USD.issue(), Alice, false},
{std::nullopt, std::nullopt, None, true}};

// Invalid parameters
testAMM([&](AMM& ammAlice, Env&) {
std::vector<std::tuple<
std::optional<Issue>,
std::optional<Issue>,
std::optional<AccountID>,
bool>>
vals = {
{xrpIssue(), std::nullopt, std::nullopt, false},
{std::nullopt, USD.issue(), std::nullopt, false},
{xrpIssue(), std::nullopt, ammAlice.ammAccount(), false},
{std::nullopt, USD.issue(), ammAlice.ammAccount(), false},
{xrpIssue(), USD.issue(), ammAlice.ammAccount(), false},
{std::nullopt, std::nullopt, std::nullopt, true}};
for (auto const& [iss1, iss2, acct, ignoreParams] : vals)
for (auto const& [iss1, iss2, acct, ignoreParams] : invalidParams)
{
auto const jv = ammAlice.ammRpcInfo(
std::nullopt,
std::nullopt,
iss1,
iss2,
accountId(ammAlice, acct),
ignoreParams);
BEAST_EXPECT(jv[jss::error_message] == "Invalid parameters.");
}
});

// Invalid parameters *and* invalid LP account, default API version
testAMM([&](AMM& ammAlice, Env&) {
for (auto const& [iss1, iss2, acct, ignoreParams] : invalidParams)
{
auto const jv = ammAlice.ammRpcInfo(
std::nullopt, std::nullopt, iss1, iss2, acct, ignoreParams);
bogie, //
std::nullopt,
iss1,
iss2,
accountId(ammAlice, acct),
ignoreParams);
BEAST_EXPECT(jv[jss::error_message] == "Invalid parameters.");
}
});

// Invalid parameters *and* invalid LP account, API version 3
testAMM([&](AMM& ammAlice, Env&) {
for (auto const& [iss1, iss2, acct, ignoreParams] : invalidParams)
{
auto const jv = ammAlice.ammRpcInfo(
bogie, //
std::nullopt,
iss1,
iss2,
accountId(ammAlice, acct),
ignoreParams,
3);
BEAST_EXPECT(jv[jss::error_message] == "Account malformed.");
}
});

// Invalid AMM account id
testAMM([&](AMM& ammAlice, Env&) {
Account bogie("bogie");
auto const jv = ammAlice.ammRpcInfo(
std::nullopt,
std::nullopt,
Expand All @@ -86,6 +133,54 @@ class AMMInfo_test : public jtx::AMMTestBase
bogie.id());
BEAST_EXPECT(jv[jss::error_message] == "Account malformed.");
});

std::vector<std::tuple<
std::optional<Issue>,
std::optional<Issue>,
TestAccount,
bool>> const invalidParamsBadAccount = {
{xrpIssue(), std::nullopt, None, false},
{std::nullopt, USD.issue(), None, false},
{xrpIssue(), std::nullopt, Bogie, false},
{std::nullopt, USD.issue(), Bogie, false},
{xrpIssue(), USD.issue(), Bogie, false},
{std::nullopt, std::nullopt, None, true}};

// Invalid parameters *and* invalid AMM account, default API version
testAMM([&](AMM& ammAlice, Env&) {
for (auto const& [iss1, iss2, acct, ignoreParams] :
invalidParamsBadAccount)
{
auto const jv = ammAlice.ammRpcInfo(
std::nullopt,
std::nullopt,
iss1,
iss2,
accountId(ammAlice, acct),
ignoreParams);
BEAST_EXPECT(jv[jss::error_message] == "Invalid parameters.");
}
});

// Invalid parameters *and* invalid AMM account, API version 3
testAMM([&](AMM& ammAlice, Env&) {
for (auto const& [iss1, iss2, acct, ignoreParams] :
invalidParamsBadAccount)
{
auto const jv = ammAlice.ammRpcInfo(
std::nullopt,
std::nullopt,
iss1,
iss2,
accountId(ammAlice, acct),
ignoreParams,
3);
BEAST_EXPECT(
jv[jss::error_message] ==
(acct == Bogie ? std::string("Account malformed.")
: std::string("Invalid parameters.")));
}
});
}

void
Expand Down

0 comments on commit c706926

Please sign in to comment.