From 7e4049a47f5ec3c2114a770a2caab68df119a81f Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Tue, 20 Feb 2024 19:30:21 +0000 Subject: [PATCH 1/3] Change order of checks in amm_info --- src/ripple/rpc/handlers/AMMInfo.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/ripple/rpc/handlers/AMMInfo.cpp b/src/ripple/rpc/handlers/AMMInfo.cpp index c6711fa7b82..534bc106ba7 100644 --- a/src/ripple/rpc/handlers/AMMInfo.cpp +++ b/src/ripple/rpc/handlers/AMMInfo.cpp @@ -96,10 +96,6 @@ 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))) - return Unexpected(rpcINVALID_PARAMS); - if (params.isMember(jss::asset)) { if (auto const i = getIssue(params[jss::asset], context.j)) @@ -127,10 +123,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 +130,14 @@ doAMMInfo(RPC::JsonContext& context) return Unexpected(rpcACT_MALFORMED); } + if ((params.isMember(jss::asset) != params.isMember(jss::asset2)) || + (params.isMember(jss::asset) == params.isMember(jss::amm_account))) + 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); From 99ea4fe591e4a9b268ad59165271b255684c85a4 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Wed, 21 Feb 2024 12:48:47 +0000 Subject: [PATCH 2/3] Change amm_info error message in API version 3 --- src/ripple/rpc/handlers/AMMInfo.cpp | 15 ++++- src/test/jtx/AMM.h | 3 +- src/test/jtx/impl/AMM.cpp | 8 ++- src/test/rpc/AMMInfo_test.cpp | 101 ++++++++++++++++++++++++++++ 4 files changed, 122 insertions(+), 5 deletions(-) diff --git a/src/ripple/rpc/handlers/AMMInfo.cpp b/src/ripple/rpc/handlers/AMMInfo.cpp index 534bc106ba7..5e994f22cef 100644 --- a/src/ripple/rpc/handlers/AMMInfo.cpp +++ b/src/ripple/rpc/handlers/AMMInfo.cpp @@ -96,6 +96,17 @@ doAMMInfo(RPC::JsonContext& context) std::optional issue2; std::optional ammID; + 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)) { if (auto const i = getIssue(params[jss::asset], context.j)) @@ -130,8 +141,8 @@ doAMMInfo(RPC::JsonContext& context) return Unexpected(rpcACT_MALFORMED); } - if ((params.isMember(jss::asset) != params.isMember(jss::asset2)) || - (params.isMember(jss::asset) == params.isMember(jss::amm_account))) + // NOTE, identical check for apVersion < 3 above + if (context.apiVersion >= 3 && invalid(params)) return Unexpected(rpcINVALID_PARAMS); assert( diff --git a/src/test/jtx/AMM.h b/src/test/jtx/AMM.h index 4c6e8d78a4e..3b222412685 100644 --- a/src/test/jtx/AMM.h +++ b/src/test/jtx/AMM.h @@ -176,7 +176,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 2d5ce90d306..ad6e1301b35 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 1d9642539a1..dfeae88d85f 100644 --- a/src/test/rpc/AMMInfo_test.cpp +++ b/src/test/rpc/AMMInfo_test.cpp @@ -75,6 +75,52 @@ class AMMInfo_test : public jtx::AMMTestBase } }); + // Invalid parameters *and* invalid LP account, default API version + testAMM([&](AMM& ammAlice, Env&) { + Account bogie("bogie"); + 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) + { + auto const jv = ammAlice.ammRpcInfo( + bogie, std::nullopt, iss1, iss2, acct, ignoreParams); + BEAST_EXPECT(jv[jss::error_message] == "Invalid parameters."); + } + }); + + // Invalid parameters *and* invalid LP account, API version 3 + testAMM([&](AMM& ammAlice, Env&) { + Account bogie("bogie"); + 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) + { + auto const jv = ammAlice.ammRpcInfo( + bogie, std::nullopt, iss1, iss2, acct, ignoreParams, 3); + BEAST_EXPECT(jv[jss::error_message] == "Account malformed."); + } + }); + // Invalid AMM account id testAMM([&](AMM& ammAlice, Env&) { Account bogie("bogie"); @@ -86,6 +132,61 @@ class AMMInfo_test : public jtx::AMMTestBase bogie.id()); BEAST_EXPECT(jv[jss::error_message] == "Account malformed."); }); + + // Invalid parameters *and* invalid AMM account, default API version + testAMM([&](AMM& ammAlice, Env&) { + Account bogie("bogie"); + std::vector, + std::optional, + std::optional, + bool>> + vals = { + {xrpIssue(), std::nullopt, std::nullopt, false}, + {std::nullopt, USD.issue(), std::nullopt, false}, + {xrpIssue(), std::nullopt, bogie, false}, + {std::nullopt, USD.issue(), bogie, false}, + {xrpIssue(), USD.issue(), bogie, false}, + {std::nullopt, std::nullopt, std::nullopt, true}}; + for (auto const& [iss1, iss2, acct, ignoreParams] : vals) + { + auto const jv = ammAlice.ammRpcInfo( + std::nullopt, std::nullopt, iss1, iss2, acct, ignoreParams); + BEAST_EXPECT(jv[jss::error_message] == "Invalid parameters."); + } + }); + + // Invalid parameters *and* invalid AMM account, API version 3 + testAMM([&](AMM& ammAlice, Env&) { + Account bogie("bogie"); + std::vector, + std::optional, + std::optional, + bool>> + vals = { + {xrpIssue(), std::nullopt, std::nullopt, false}, + {std::nullopt, USD.issue(), std::nullopt, false}, + {xrpIssue(), std::nullopt, bogie, false}, + {std::nullopt, USD.issue(), bogie, false}, + {xrpIssue(), USD.issue(), bogie, false}, + {std::nullopt, std::nullopt, std::nullopt, true}}; + for (auto const& [iss1, iss2, acct, ignoreParams] : vals) + { + auto const jv = ammAlice.ammRpcInfo( + std::nullopt, + std::nullopt, + iss1, + iss2, + acct, + ignoreParams, + 3); + BEAST_EXPECT( + jv[jss::error_message] == + (acct ? std::string("Account malformed.") + : std::string("Invalid parameters."))); + } + }); } void From 0ac0c1b1d3974dc315caeb15f9f365169704f64e Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Thu, 22 Feb 2024 21:23:03 +0000 Subject: [PATCH 3/3] Change amm_info error tests --- src/test/rpc/AMMInfo_test.cpp | 150 ++++++++++++++++------------------ 1 file changed, 72 insertions(+), 78 deletions(-) diff --git a/src/test/rpc/AMMInfo_test.cpp b/src/test/rpc/AMMInfo_test.cpp index dfeae88d85f..cf016b4a2c8 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,82 +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, acct, ignoreParams); + 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&) { - Account bogie("bogie"); - 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( - bogie, 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&) { - Account bogie("bogie"); - 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( - bogie, std::nullopt, iss1, iss2, acct, ignoreParams, 3); + 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, @@ -133,58 +134,51 @@ class AMMInfo_test : public jtx::AMMTestBase 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&) { - Account bogie("bogie"); - std::vector, - std::optional, - std::optional, - bool>> - vals = { - {xrpIssue(), std::nullopt, std::nullopt, false}, - {std::nullopt, USD.issue(), std::nullopt, false}, - {xrpIssue(), std::nullopt, bogie, false}, - {std::nullopt, USD.issue(), bogie, false}, - {xrpIssue(), USD.issue(), bogie, false}, - {std::nullopt, std::nullopt, std::nullopt, true}}; - for (auto const& [iss1, iss2, acct, ignoreParams] : vals) + for (auto const& [iss1, iss2, acct, ignoreParams] : + invalidParamsBadAccount) { auto const jv = ammAlice.ammRpcInfo( - std::nullopt, std::nullopt, iss1, iss2, acct, ignoreParams); + 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&) { - Account bogie("bogie"); - std::vector, - std::optional, - std::optional, - bool>> - vals = { - {xrpIssue(), std::nullopt, std::nullopt, false}, - {std::nullopt, USD.issue(), std::nullopt, false}, - {xrpIssue(), std::nullopt, bogie, false}, - {std::nullopt, USD.issue(), bogie, false}, - {xrpIssue(), USD.issue(), bogie, false}, - {std::nullopt, std::nullopt, std::nullopt, true}}; - for (auto const& [iss1, iss2, acct, ignoreParams] : vals) + for (auto const& [iss1, iss2, acct, ignoreParams] : + invalidParamsBadAccount) { auto const jv = ammAlice.ammRpcInfo( std::nullopt, std::nullopt, iss1, iss2, - acct, + accountId(ammAlice, acct), ignoreParams, 3); BEAST_EXPECT( jv[jss::error_message] == - (acct ? std::string("Account malformed.") - : std::string("Invalid parameters."))); + (acct == Bogie ? std::string("Account malformed.") + : std::string("Invalid parameters."))); } }); }