From 899661e05ebf087b03e6b339309bcd98f5c51789 Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Fri, 11 Sep 2026 21:45:21 +0100 Subject: [PATCH] refactor: Migrate first handlers to rpc-spec (#3199) --- conan.lock | 2 +- conanfile.py | 2 +- src/rpc/CMakeLists.txt | 3 + src/rpc/handlers/AccountCurrencies.cpp | 36 ++------ src/rpc/handlers/AccountCurrencies.hpp | 49 +---------- src/rpc/handlers/AccountInfo.cpp | 44 ++-------- src/rpc/handlers/AccountInfo.hpp | 58 +------------ .../rpc/handlers/AccountCurrenciesTests.cpp | 84 ++++++++++++++++++- tests/unit/rpc/handlers/AccountInfoTests.cpp | 9 +- tests/unit/rpc/handlers/AllHandlerTests.cpp | 3 +- 10 files changed, 111 insertions(+), 179 deletions(-) diff --git a/conan.lock b/conan.lock index 1e7c54e73..7c9957108 100644 --- a/conan.lock +++ b/conan.lock @@ -3,7 +3,7 @@ "requires": [ "zlib/1.3.2#1cb806da49011867778ffb6ac7190fcb%1782392402.122708", "xxhash/0.8.3#681d36a0a6111fc56e5e45ea182c19cc%1782392402.420688", - "xrpl-rpc-spec/0.1.7#774d2f93c4b48a1523d8d5a94a2b082a%1787768955.574105", + "xrpl-rpc-spec/0.1.10#38d3a69d1802fbb7af5796fed8326888%1789129776.315907", "xrpl/3.4.0-rc1#19678cbb46117ef8a669558ad19d6a1f%1789050823.813662", "sqlite3/3.53.0#324ada52333108388a9a6108bfa96734%1782392403.185447", "spdlog/1.17.0#bcbaaf7147bda6ad24ffbd1ac3d7142c%1782736610.443882", diff --git a/conanfile.py b/conanfile.py index 0404b34bb..a055c9f3f 100644 --- a/conanfile.py +++ b/conanfile.py @@ -17,7 +17,7 @@ class ClioConan(ConanFile): "fmt/12.1.0", "libbacktrace/cci.20210118", "spdlog/1.17.0", - "xrpl-rpc-spec/0.1.7", + "xrpl-rpc-spec/0.1.10", "xrpl/3.4.0-rc1", ] diff --git a/src/rpc/CMakeLists.txt b/src/rpc/CMakeLists.txt index 73dbcade0..28cd5e098 100644 --- a/src/rpc/CMakeLists.txt +++ b/src/rpc/CMakeLists.txt @@ -61,4 +61,7 @@ target_sources( handlers/VaultInfo.cpp ) +rpcspec_generate_instantiations(OUT_VAR rpcspec_instantiations) +target_sources(clio_rpc PRIVATE ${rpcspec_instantiations}) + target_link_libraries(clio_rpc PUBLIC clio_util clio_data rpcspec::rpcspec) diff --git a/src/rpc/handlers/AccountCurrencies.cpp b/src/rpc/handlers/AccountCurrencies.cpp index a72ebc3e9..913fb373c 100644 --- a/src/rpc/handlers/AccountCurrencies.cpp +++ b/src/rpc/handlers/AccountCurrencies.cpp @@ -4,11 +4,9 @@ #include "rpc/RPCHelpers.hpp" #include "rpc/common/Types.hpp" #include "util/Assert.hpp" -#include "util/JsonUtils.hpp" #include #include -#include #include #include #include @@ -33,11 +31,10 @@ AccountCurrenciesHandler::process( { auto const range = sharedPtrBackend_->fetchLedgerRange(); ASSERT(range.has_value(), "AccountCurrencies' ledger range must be available"); - auto const expectedLgrInfo = getLedgerHeaderFromHashOrSeq( + auto const expectedLgrInfo = getLedgerHeaderFromLedgerSpecifier( *sharedPtrBackend_, ctx.yield, - input.ledgerHash, - input.ledgerIndex, + input.ledger, range->maxSequence // NOLINT(bugprone-unchecked-optional-access) ); @@ -45,13 +42,10 @@ AccountCurrenciesHandler::process( return Error{expectedLgrInfo.error()}; auto const& lgrInfo = *expectedLgrInfo; - auto const accountID = accountFromStringStrict(input.account); + auto const& accountID = input.account; auto const accountLedgerObject = sharedPtrBackend_->fetchLedgerObject( - // NOLINTNEXTLINE(bugprone-unchecked-optional-access) - xrpl::keylet::account(*accountID).key, - lgrInfo.seq, - ctx.yield + xrpl::keylet::account(accountID).key, lgrInfo.seq, ctx.yield ); if (!accountLedgerObject) return Error{Status{RippledError::RpcActNotFound}}; @@ -88,7 +82,7 @@ AccountCurrenciesHandler::process( // traverse all owned nodes, limit->max, marker->empty traverseOwnedNodes( *sharedPtrBackend_, - *accountID, // NOLINT(bugprone-unchecked-optional-access) + accountID, lgrInfo.seq, std::numeric_limits::max(), {}, @@ -120,24 +114,4 @@ tag_invoke( }; } -AccountCurrenciesHandler::Input -tag_invoke(boost::json::value_to_tag, boost::json::value const& jv) -{ - auto input = AccountCurrenciesHandler::Input{}; - auto const& jsonObject = jv.as_object(); - - input.account = boost::json::value_to(jv.at(JS(account))); - - if (jsonObject.contains(JS(ledger_hash))) - input.ledgerHash = boost::json::value_to(jv.at(JS(ledger_hash))); - - if (jsonObject.contains(JS(ledger_index))) { - auto const expectedLedgerIndex = util::getLedgerIndex(jv.at(JS(ledger_index))); - if (expectedLedgerIndex.has_value()) - input.ledgerIndex = *expectedLedgerIndex; - } - - return input; -} - } // namespace rpc diff --git a/src/rpc/handlers/AccountCurrencies.hpp b/src/rpc/handlers/AccountCurrencies.hpp index f8e4441f2..eecac4fff 100644 --- a/src/rpc/handlers/AccountCurrencies.hpp +++ b/src/rpc/handlers/AccountCurrencies.hpp @@ -1,19 +1,15 @@ #pragma once #include "data/BackendInterface.hpp" -#include "rpc/JS.hpp" -#include "rpc/common/Checkers.hpp" -#include "rpc/common/Specs.hpp" #include "rpc/common/Types.hpp" -#include "rpc/common/Validators.hpp" #include #include -#include +#include +#include #include #include -#include #include #include @@ -25,7 +21,8 @@ namespace rpc { * * For more details see: https://xrpl.org/account_currencies.html */ -class AccountCurrenciesHandler { +class AccountCurrenciesHandler + : public rpc::spec::HandlerFor { // dependencies std::shared_ptr sharedPtrBackend_; @@ -42,15 +39,6 @@ public: bool validated = true; }; - /** - * @brief A struct to hold the input data for the command - */ - struct Input { - std::string account; - std::optional ledgerHash; - std::optional ledgerIndex; - }; - using Result = HandlerReturnType; /** @@ -63,26 +51,6 @@ public: { } - /** - * @brief Returns the API specification for the command - * - * @param apiVersion The api version to return the spec for - * @return The spec for the given apiVersion - */ - static RpcSpecConstRef - spec([[maybe_unused]] uint32_t apiVersion) - { - static auto const kRpcSpec = RpcSpec{ - {JS(account), validation::Required{}, validation::CustomValidators::accountValidator}, - {JS(ledger_hash), validation::CustomValidators::uint256HexStringValidator}, - {JS(ledger_index), validation::CustomValidators::ledgerIndexValidator}, - {"account_index", check::Deprecated{}}, - {JS(strict), check::Deprecated{}} - }; - - return kRpcSpec; - } - /** * @brief Process the AccountCurrencies command * @@ -102,15 +70,6 @@ private: */ friend void tag_invoke(boost::json::value_from_tag, boost::json::value& jv, Output const& output); - - /** - * @brief Convert a JSON object to Input type - * - * @param jv The JSON object to convert - * @return Input parsed from the JSON object - */ - friend Input - tag_invoke(boost::json::value_to_tag, boost::json::value const& jv); }; } // namespace rpc diff --git a/src/rpc/handlers/AccountInfo.cpp b/src/rpc/handlers/AccountInfo.cpp index 6c246568e..5b7951f38 100644 --- a/src/rpc/handlers/AccountInfo.cpp +++ b/src/rpc/handlers/AccountInfo.cpp @@ -3,16 +3,13 @@ #include "data/AmendmentCenter.hpp" #include "rpc/JS.hpp" #include "rpc/RPCHelpers.hpp" -#include "rpc/common/JsonBool.hpp" #include "rpc/common/Types.hpp" #include "util/Assert.hpp" -#include "util/JsonUtils.hpp" #include #include #include #include -#include #include #include #include @@ -46,11 +43,10 @@ AccountInfoHandler::process(AccountInfoHandler::Input const& input, Context cons auto const range = sharedPtrBackend_->fetchLedgerRange(); ASSERT(range.has_value(), "AccountInfo's ledger range must be available"); - auto const expectedLgrInfo = getLedgerHeaderFromHashOrSeq( + auto const expectedLgrInfo = getLedgerHeaderFromLedgerSpecifier( *sharedPtrBackend_, ctx.yield, - input.ledgerHash, - input.ledgerIndex, + input.ledger, range->maxSequence // NOLINT(bugprone-unchecked-optional-access) ); @@ -58,10 +54,8 @@ AccountInfoHandler::process(AccountInfoHandler::Input const& input, Context cons return Error{expectedLgrInfo.error()}; auto const& lgrInfo = *expectedLgrInfo; - auto const accountStr = input.account.value_or(input.ident.value_or("")); - auto const accountID = accountFromStringStrict(accountStr); - auto const accountKeylet = - xrpl::keylet::account(*accountID); // NOLINT(bugprone-unchecked-optional-access) + auto const accountID = input.account ? *input.account : *input.ident; + auto const accountKeylet = xrpl::keylet::account(accountID); auto const accountLedgerObject = sharedPtrBackend_->fetchLedgerObject(accountKeylet.key, lgrInfo.seq, ctx.yield); @@ -99,8 +93,7 @@ AccountInfoHandler::process(AccountInfoHandler::Input const& input, Context cons if (input.signerLists) { // We put the SignerList in an array because of an anticipated // future when we support multiple signer lists on one account. - auto const signersKey = - xrpl::keylet::signerList(*accountID); // NOLINT(bugprone-unchecked-optional-access) + auto const signersKey = xrpl::keylet::signerList(accountID); // This code will need to be revisited if in the future we // support multiple SignerLists on one account. @@ -203,31 +196,4 @@ tag_invoke( } } -AccountInfoHandler::Input -tag_invoke(boost::json::value_to_tag, boost::json::value const& jv) -{ - auto input = AccountInfoHandler::Input{}; - auto const& jsonObject = jv.as_object(); - - if (jsonObject.contains(JS(ident))) - input.ident = boost::json::value_to(jsonObject.at(JS(ident))); - - if (jsonObject.contains(JS(account))) - input.account = boost::json::value_to(jsonObject.at(JS(account))); - - if (jsonObject.contains(JS(ledger_hash))) - input.ledgerHash = boost::json::value_to(jsonObject.at(JS(ledger_hash))); - - if (jsonObject.contains(JS(ledger_index))) { - auto const expectedLedgerIndex = util::getLedgerIndex(jsonObject.at(JS(ledger_index))); - if (expectedLedgerIndex.has_value()) - input.ledgerIndex = *expectedLedgerIndex; - } - - if (jsonObject.contains(JS(signer_lists))) - input.signerLists = boost::json::value_to(jsonObject.at(JS(signer_lists))); - - return input; -} - } // namespace rpc diff --git a/src/rpc/handlers/AccountInfo.hpp b/src/rpc/handlers/AccountInfo.hpp index 091859b0e..08196c054 100644 --- a/src/rpc/handlers/AccountInfo.hpp +++ b/src/rpc/handlers/AccountInfo.hpp @@ -2,17 +2,13 @@ #include "data/AmendmentCenterInterface.hpp" #include "data/BackendInterface.hpp" -#include "rpc/JS.hpp" -#include "rpc/common/Checkers.hpp" -#include "rpc/common/JsonBool.hpp" -#include "rpc/common/Specs.hpp" #include "rpc/common/Types.hpp" -#include "rpc/common/Validators.hpp" #include #include +#include +#include #include -#include #include #include @@ -28,7 +24,7 @@ namespace rpc { * * For more details see: https://xrpl.org/account_info.html */ -class AccountInfoHandler { +class AccountInfoHandler : public rpc::spec::HandlerFor { std::shared_ptr sharedPtrBackend_; std::shared_ptr amendmentCenter_; @@ -49,20 +45,6 @@ public: bool validated = true; }; - /** - * @brief A struct to hold the input data for the command - * - * `queue` is not available in Reporting mode - * `ident` is deprecated, keep it for now, in line with rippled - */ - struct Input { - std::optional account; - std::optional ident; - std::optional ledgerHash; - std::optional ledgerIndex; - JsonBool signerLists{false}; - }; - using Result = HandlerReturnType; /** @@ -79,31 +61,6 @@ public: { } - /** - * @brief Returns the API specification for the command - * - * @param apiVersion The api version to return the spec for - * @return The spec for the given apiVersion - */ - static RpcSpecConstRef - spec([[maybe_unused]] uint32_t apiVersion) - { - static auto const kRpcSpecV1 = RpcSpec{ - {JS(account), validation::CustomValidators::accountValidator}, - {JS(ident), validation::CustomValidators::accountValidator}, - {JS(ident), check::Deprecated{}}, - {JS(ledger_hash), validation::CustomValidators::uint256HexStringValidator}, - {JS(ledger_index), validation::CustomValidators::ledgerIndexValidator}, - {JS(ledger), check::Deprecated{}}, - {JS(strict), check::Deprecated{}} - }; - - static auto const kRpcSpec = - RpcSpec{kRpcSpecV1, {{JS(signer_lists), validation::Type{}}}}; - - return apiVersion == 1 ? kRpcSpecV1 : kRpcSpec; - } - /** * @brief Process the AccountInfo command * @@ -123,15 +80,6 @@ private: */ friend void tag_invoke(boost::json::value_from_tag, boost::json::value& jv, Output const& output); - - /** - * @brief Convert a JSON object to Input type - * - * @param jv The JSON object to convert - * @return Input parsed from the JSON object - */ - friend Input - tag_invoke(boost::json::value_to_tag, boost::json::value const& jv); }; } // namespace rpc diff --git a/tests/unit/rpc/handlers/AccountCurrenciesTests.cpp b/tests/unit/rpc/handlers/AccountCurrenciesTests.cpp index 909a75279..a7d7f9317 100644 --- a/tests/unit/rpc/handlers/AccountCurrenciesTests.cpp +++ b/tests/unit/rpc/handlers/AccountCurrenciesTests.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -152,6 +153,87 @@ TEST_F(RPCAccountCurrenciesHandlerTest, LedgerNonExistViaHash) }); } +TEST_F(RPCAccountCurrenciesHandlerTest, LedgerHashMalformed) +{ + static auto const kInput = boost::json::parse( + fmt::format(R"JSON({{ "account": "{}", "ledger_hash": "1" }})JSON", kAccount) + ); + auto const handler = AnyHandler{AccountCurrenciesHandler{backend_}}; + runSpawn([&](auto yield) { + auto const output = handler.process(kInput, Context{yield}); + ASSERT_FALSE(output); + auto const err = rpc::makeError(output.result.error()); + EXPECT_EQ(err.at("error").as_string(), "invalidParams"); + EXPECT_EQ( + err.at("error_message").as_string(), "Invalid field 'ledger_hash', not hex string." + ); + }); +} + +TEST_F(RPCAccountCurrenciesHandlerTest, LedgerIndexMalformed) +{ + static auto const kInput = boost::json::parse( + fmt::format(R"JSON({{ "account": "{}", "ledger_index": "a" }})JSON", kAccount) + ); + auto const handler = AnyHandler{AccountCurrenciesHandler{backend_}}; + runSpawn([&](auto yield) { + auto const output = handler.process(kInput, Context{yield}); + ASSERT_FALSE(output); + auto const err = rpc::makeError(output.result.error()); + EXPECT_EQ(err.at("error").as_string(), "invalidParams"); + EXPECT_EQ( + err.at("error_message").as_string(), + "Invalid field 'ledger_index', not string or number." + ); + }); +} + +TEST_F(RPCAccountCurrenciesHandlerTest, LedgerIndexEmptyStringMalformed) +{ + static auto const kInput = boost::json::parse( + fmt::format(R"JSON({{ "account": "{}", "ledger_index": "" }})JSON", kAccount) + ); + auto const handler = AnyHandler{AccountCurrenciesHandler{backend_}}; + runSpawn([&](auto yield) { + auto const output = handler.process(kInput, Context{yield}); + ASSERT_FALSE(output); + EXPECT_EQ(rpc::makeError(output.result.error()).at("error").as_string(), "invalidParams"); + }); +} + +TEST_F(RPCAccountCurrenciesHandlerTest, LedgerIndexOutOfRangeMalformed) +{ + static auto const kInput = boost::json::parse( + fmt::format(R"JSON({{ "account": "{}", "ledger_index": 4294967296 }})JSON", kAccount) + ); + auto const handler = AnyHandler{AccountCurrenciesHandler{backend_}}; + runSpawn([&](auto yield) { + auto const output = handler.process(kInput, Context{yield}); + ASSERT_FALSE(output); + EXPECT_EQ(rpc::makeError(output.result.error()).at("error").as_string(), "invalidParams"); + }); +} + +TEST_F(RPCAccountCurrenciesHandlerTest, ValidLedgerHashWithMalformedLedgerIndex) +{ + static auto const kInput = boost::json::parse( + fmt::format( + R"JSON({{ "account": "{}", "ledger_hash": "{}", "ledger_index": "a" }})JSON", + kAccount, + kLedgerHash + ) + ); + auto const handler = AnyHandler{AccountCurrenciesHandler{backend_}}; + runSpawn([&](auto yield) { + auto const output = handler.process(kInput, Context{yield}); + ASSERT_FALSE(output); + EXPECT_EQ( + rpc::makeError(output.result.error()).at("error_message").as_string(), + "Invalid field 'ledger_index', not string or number." + ); + }); +} + TEST_F(RPCAccountCurrenciesHandlerTest, DefaultParameter) { static constexpr auto kOutput = R"JSON({ @@ -318,7 +400,7 @@ TEST(RPCAccountCurrenciesHandlerSpecTest, DeprecatedFields) {"strict", true} }; auto const spec = AccountCurrenciesHandler::spec(2); - auto const warnings = spec.check(json); + auto const warnings = rpc::spec::toJsonArray(spec.check(json)); ASSERT_EQ(warnings.size(), 1); ASSERT_TRUE(warnings[0].is_object()); auto const& warning = warnings[0].as_object(); diff --git a/tests/unit/rpc/handlers/AccountInfoTests.cpp b/tests/unit/rpc/handlers/AccountInfoTests.cpp index e28bceb96..46a237b2e 100644 --- a/tests/unit/rpc/handlers/AccountInfoTests.cpp +++ b/tests/unit/rpc/handlers/AccountInfoTests.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -106,21 +107,21 @@ generateTestValuesForParametersTest() .testJson = R"JSON({"ident": "rLEsXccBGNR3UPuPu2hUXPjziKC3qKSBun", "ledger_hash": "1"})JSON", .expectedError = "invalidParams", - .expectedErrorMessage = "ledger_hashMalformed" + .expectedErrorMessage = "Invalid field 'ledger_hash', not hex string." }, AccountInfoParamTestCaseBundle{ .testName = "LedgerHashNotString", .testJson = R"JSON({"ident": "rLEsXccBGNR3UPuPu2hUXPjziKC3qKSBun", "ledger_hash": 1})JSON", .expectedError = "invalidParams", - .expectedErrorMessage = "ledger_hashNotString" + .expectedErrorMessage = "Invalid field 'ledger_hash', not hex string." }, AccountInfoParamTestCaseBundle{ .testName = "LedgerIndexInvalid", .testJson = R"JSON({"ident": "rLEsXccBGNR3UPuPu2hUXPjziKC3qKSBun", "ledger_index": "a"})JSON", .expectedError = "invalidParams", - .expectedErrorMessage = "ledgerIndexMalformed" + .expectedErrorMessage = "Invalid field 'ledger_index', not string or number." }, }; } @@ -952,7 +953,7 @@ TEST(RPCAccountInfoHandlerSpecTest, DeprecatedFields) {"strict", true} }; auto const spec = AccountInfoHandler::spec(2); - auto const warnings = spec.check(json); + auto const warnings = rpc::spec::toJsonArray(spec.check(json)); ASSERT_EQ(warnings.size(), 1); auto const& warning = warnings[0]; ASSERT_TRUE(warning.is_object()); diff --git a/tests/unit/rpc/handlers/AllHandlerTests.cpp b/tests/unit/rpc/handlers/AllHandlerTests.cpp index 8f739c596..7816bde59 100644 --- a/tests/unit/rpc/handlers/AllHandlerTests.cpp +++ b/tests/unit/rpc/handlers/AllHandlerTests.cpp @@ -168,8 +168,7 @@ AccountInfoHandler::Input createInput() { AccountInfoHandler::Input input{}; - input.account = kAccount; - input.ident = "asdf"; + input.account = getAccountIdWithString(kAccount); return input; }