diff --git a/src/ripple/rpc/handlers/AMMInfo.cpp b/src/ripple/rpc/handlers/AMMInfo.cpp index b240c8c2ae6..b4f56ffba21 100644 --- a/src/ripple/rpc/handlers/AMMInfo.cpp +++ b/src/ripple/rpc/handlers/AMMInfo.cpp @@ -96,8 +96,15 @@ doAMMInfo(RPC::JsonContext& context) std::optional issue2; std::optional 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)) @@ -127,10 +134,6 @@ 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); @@ -138,6 +141,14 @@ doAMMInfo(RPC::JsonContext& context) 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); diff --git a/src/test/jtx/AMM.h b/src/test/jtx/AMM.h index b98065a077e..bb1032e006d 100644 --- a/src/test/jtx/AMM.h +++ b/src/test/jtx/AMM.h @@ -174,7 +174,8 @@ class AMM std::optional issue1 = std::nullopt, std::optional issue2 = std::nullopt, std::optional const& ammAccount = std::nullopt, - bool ignoreParams = false) const; + bool ignoreParams = false, + unsigned apiVersion = RPC::apiInvalidVersion) const; /** Verify the AMM balances. */ diff --git a/src/test/jtx/impl/AMM.cpp b/src/test/jtx/impl/AMM.cpp index 38cdb201854..a36384f4a29 100644 --- a/src/test/jtx/impl/AMM.cpp +++ b/src/test/jtx/impl/AMM.cpp @@ -163,7 +163,8 @@ AMM::ammRpcInfo( std::optional issue1, std::optional issue2, std::optional const& ammAccount, - bool ignoreParams) const + bool ignoreParams, + unsigned apiVersion) const { Json::Value jv; if (account) @@ -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]; diff --git a/src/test/rpc/AMMInfo_test.cpp b/src/test/rpc/AMMInfo_test.cpp index cfcb672c032..664a0c04cd5 100644 --- a/src/test/rpc/AMMInfo_test.cpp +++ b/src/test/rpc/AMMInfo_test.cpp @@ -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 { + 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"); @@ -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::optional, + 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::optional, - std::optional, - 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, @@ -86,6 +133,54 @@ class AMMInfo_test : public jtx::AMMTestBase bogie.id()); BEAST_EXPECT(jv[jss::error_message] == "Account malformed."); }); + + std::vector, + std::optional, + 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