From c97337578de16d740cc05377543b234a3b2396dc Mon Sep 17 00:00:00 2001 From: ledhed2222 Date: Wed, 17 Aug 2022 15:31:50 +0000 Subject: [PATCH 1/7] add nft_tx and mark certain APIs as clio-only to improve error messaging --- .gitignore | 1 + CMakeLists.txt | 4 +- src/backend/CassandraBackend.cpp | 4 + src/rpc/Handlers.h | 3 + src/rpc/RPC.cpp | 19 ++- src/rpc/RPC.h | 3 + src/rpc/RPCHelpers.cpp | 206 ++++++++++++++++++++++++++++ src/rpc/RPCHelpers.h | 14 ++ src/rpc/handlers/AccountTx.cpp | 226 +++++-------------------------- src/rpc/handlers/NFTInfo.cpp | 25 ++-- src/rpc/handlers/NFTOffers.cpp | 18 +-- src/rpc/handlers/NFTTx.cpp | 51 +++++++ 12 files changed, 345 insertions(+), 229 deletions(-) create mode 100644 src/rpc/handlers/NFTTx.cpp diff --git a/.gitignore b/.gitignore index f8bcd4899..bee14b839 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ *clio*.log build/ .python-version +config.json diff --git a/CMakeLists.txt b/CMakeLists.txt index 0c0d699fa..cc3e8aa00 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -75,6 +75,8 @@ target_sources(clio PRIVATE src/rpc/handlers/NoRippleCheck.cpp # NFT src/rpc/handlers/NFTInfo.cpp + src/rpc/handlers/NFTTx.cpp + src/rpc/handlers/NFTOffers.cpp # Ledger src/rpc/handlers/Ledger.cpp src/rpc/handlers/LedgerData.cpp @@ -86,8 +88,6 @@ target_sources(clio PRIVATE src/rpc/handlers/AccountTx.cpp # Dex src/rpc/handlers/BookOffers.cpp - # NFT - src/rpc/handlers/NFTOffers.cpp # Payment Channel src/rpc/handlers/ChannelAuthorize.cpp src/rpc/handlers/ChannelVerify.cpp diff --git a/src/backend/CassandraBackend.cpp b/src/backend/CassandraBackend.cpp index f6d7c4c2a..1a4a87994 100644 --- a/src/backend/CassandraBackend.cpp +++ b/src/backend/CassandraBackend.cpp @@ -651,6 +651,8 @@ CassandraBackend::fetchNFTTransactions( static_cast(lgrSeq), static_cast(txnIdx)}; + // Only modify if forward because forward query is >= seq_idx, + // while reverse query handles this via < seq_idx if (forward) ++cursor->transactionIndex; } @@ -734,6 +736,8 @@ CassandraBackend::fetchAccountTransactions( static_cast(lgrSeq), static_cast(txnIdx)}; + // Only modify if forward because forward query is >= seq_idx, + // while reverse query handles this via < seq_idx if (forward) ++cursor->transactionIndex; } diff --git a/src/rpc/Handlers.h b/src/rpc/Handlers.h index 6207243e5..672371647 100644 --- a/src/rpc/Handlers.h +++ b/src/rpc/Handlers.h @@ -58,6 +58,9 @@ doNFTSellOffers(Context const& context); Result doNFTInfo(Context const& context); +Result +doNFTTx(Context const& context); + // ledger methods Result doLedger(Context const& context); diff --git a/src/rpc/RPC.cpp b/src/rpc/RPC.cpp index e350b91ff..e145f3bc5 100644 --- a/src/rpc/RPC.cpp +++ b/src/rpc/RPC.cpp @@ -168,6 +168,7 @@ struct Handler std::string method; std::function handler; std::optional limit; + bool isClioOnly = false; }; class HandlerTable @@ -206,6 +207,12 @@ class HandlerTable return handlerMap_[command].handler; } + + bool + getIsClioOnly(std::string const& command) + { + return handlerMap_.contains(command) && handlerMap_[command].isClioOnly; + } }; static HandlerTable handlerTable{ @@ -223,8 +230,9 @@ static HandlerTable handlerTable{ {"ledger", &doLedger, {}}, {"ledger_data", &doLedgerData, LimitRange{1, 100, 2048}}, {"nft_buy_offers", &doNFTBuyOffers, LimitRange{1, 50, 100}}, - {"nft_info", &doNFTInfo}, + {"nft_info", &doNFTInfo, {}, true}, {"nft_sell_offers", &doNFTSellOffers, LimitRange{1, 50, 100}}, + {"nft_tx", &doNFTTx, LimitRange{1, 50, 100}, true}, {"ledger_entry", &doLedgerEntry, {}}, {"ledger_range", &doLedgerRange, {}}, {"subscribe", &doSubscribe, {}}, @@ -251,6 +259,12 @@ validHandler(std::string const& method) return handlerTable.contains(method) || forwardCommands.contains(method); } +bool +isClioOnly(std::string const& method) +{ + return handlerTable.getIsClioOnly(method); +} + Status getLimit(RPC::Context const& context, std::uint32_t& limit) { @@ -286,6 +300,9 @@ shouldForwardToRippled(Context const& ctx) { auto request = ctx.params; + if (isClioOnly(ctx.method)) + return false; + if (forwardCommands.find(ctx.method) != forwardCommands.end()) return true; diff --git a/src/rpc/RPC.h b/src/rpc/RPC.h index 29154734e..eb1e00152 100644 --- a/src/rpc/RPC.h +++ b/src/rpc/RPC.h @@ -226,6 +226,9 @@ buildResponse(Context const& ctx); bool validHandler(std::string const& method); +bool +isClioOnly(std::string const& method); + Status getLimit(RPC::Context const& context, std::uint32_t& limit); diff --git a/src/rpc/RPCHelpers.cpp b/src/rpc/RPCHelpers.cpp index cdd19e1a0..127670c86 100644 --- a/src/rpc/RPCHelpers.cpp +++ b/src/rpc/RPCHelpers.cpp @@ -1478,4 +1478,210 @@ specifiesCurrentOrClosedLedger(boost::json::object const& request) return false; } +std::variant +getNFTID(boost::json::object const& request) +{ + if (!request.contains(JS(nft_id))) + return Status{Error::rpcINVALID_PARAMS, "missingTokenid"}; + + if (!request.at(JS(nft_id)).is_string()) + return Status{Error::rpcINVALID_PARAMS, "tokenidNotString"}; + + ripple::uint256 tokenid; + if (!tokenid.parseHex(request.at(JS(nft_id)).as_string().c_str())) + return Status{Error::rpcINVALID_PARAMS, "malformedCursor"}; + + return tokenid; +} + +// TODO - this function is long and shouldn't be responsible for as much as it +// is. Split it out into some helper functions. +std::variant +traverseTransactions( + Context const& context, + std::function const&)> getter) +{ + auto request = context.params; + boost::json::object response = {}; + + bool const binary = getBool(request, JS(binary), false); + bool const forward = getBool(request, JS(forward), false); + + std::optional cursor; + + if (request.contains(JS(marker))) + { + auto const& obj = request.at(JS(marker)).as_object(); + + std::optional transactionIndex = {}; + if (obj.contains(JS(seq))) + { + if (!obj.at(JS(seq)).is_int64()) + return Status{ + Error::rpcINVALID_PARAMS, "transactionIndexNotInt"}; + + transactionIndex = + boost::json::value_to(obj.at(JS(seq))); + } + + std::optional ledgerIndex = {}; + if (obj.contains(JS(ledger))) + { + if (!obj.at(JS(ledger)).is_int64()) + return Status{Error::rpcINVALID_PARAMS, "ledgerIndexNotInt"}; + + ledgerIndex = + boost::json::value_to(obj.at(JS(ledger))); + } + + if (!transactionIndex || !ledgerIndex) + return Status{Error::rpcINVALID_PARAMS, "missingLedgerOrSeq"}; + + cursor = {*ledgerIndex, *transactionIndex}; + } + + auto minIndex = context.range.minSequence; + if (request.contains(JS(ledger_index_min))) + { + auto& min = request.at(JS(ledger_index_min)); + + if (!min.is_int64()) + return Status{Error::rpcINVALID_PARAMS, "ledgerSeqMinNotNumber"}; + + if (min.as_int64() != -1) + { + if (context.range.maxSequence < min.as_int64() || + context.range.minSequence > min.as_int64()) + return Status{ + Error::rpcINVALID_PARAMS, "ledgerSeqMinOutOfRange"}; + else + minIndex = boost::json::value_to(min); + } + + if (forward && !cursor) + cursor = {minIndex, 0}; + } + + auto maxIndex = context.range.maxSequence; + if (request.contains(JS(ledger_index_max))) + { + auto& max = request.at(JS(ledger_index_max)); + + if (!max.is_int64()) + return Status{Error::rpcINVALID_PARAMS, "ledgerSeqMaxNotNumber"}; + + if (max.as_int64() != -1) + { + if (context.range.maxSequence < max.as_int64() || + context.range.minSequence > max.as_int64()) + return Status{ + Error::rpcINVALID_PARAMS, "ledgerSeqMaxOutOfRange"}; + else + maxIndex = boost::json::value_to(max); + } + + if (minIndex > maxIndex) + return Status{Error::rpcINVALID_PARAMS, "invalidIndex"}; + + if (!forward && !cursor) + cursor = {maxIndex, INT32_MAX}; + } + + if (request.contains(JS(ledger_index)) || request.contains(JS(ledger_hash))) + { + if (request.contains(JS(ledger_index_max)) || + request.contains(JS(ledger_index_min))) + return Status{ + Error::rpcINVALID_PARAMS, "containsLedgerSpecifierAndRange"}; + + auto v = ledgerInfoFromRequest(context); + if (auto status = std::get_if(&v)) + return *status; + + maxIndex = minIndex = std::get(v).seq; + } + + if (!cursor) + { + if (forward) + cursor = {minIndex, 0}; + else + cursor = {maxIndex, INT32_MAX}; + } + + std::uint32_t limit; + if (auto const status = getLimit(context, limit); status) + return status; + + if (request.contains(JS(limit))) + response[JS(limit)] = limit; + + boost::json::array txns; + auto [blobs, retCursor] = getter(limit, forward, cursor); + auto dbFetchEnd = std::chrono::system_clock::now(); + + if (retCursor) + { + boost::json::object cursorJson; + cursorJson[JS(ledger)] = retCursor->ledgerSequence; + cursorJson[JS(seq)] = retCursor->transactionIndex; + response[JS(marker)] = cursorJson; + } + + std::optional maxReturnedIndex; + std::optional minReturnedIndex; + for (auto const& txnPlusMeta : blobs) + { + if (txnPlusMeta.ledgerSequence < minIndex || + txnPlusMeta.ledgerSequence > maxIndex) + { + BOOST_LOG_TRIVIAL(debug) + << __func__ + << " skipping over transactions from incomplete ledger"; + continue; + } + + boost::json::object obj; + + if (!binary) + { + auto [txn, meta] = toExpandedJson(txnPlusMeta); + obj[JS(meta)] = meta; + obj[JS(tx)] = txn; + obj[JS(tx)].as_object()[JS(ledger_index)] = + txnPlusMeta.ledgerSequence; + obj[JS(tx)].as_object()[JS(date)] = txnPlusMeta.date; + } + else + { + obj[JS(meta)] = ripple::strHex(txnPlusMeta.metadata); + obj[JS(tx_blob)] = ripple::strHex(txnPlusMeta.transaction); + obj[JS(ledger_index)] = txnPlusMeta.ledgerSequence; + obj[JS(date)] = txnPlusMeta.date; + } + obj[JS(validated)] = true; + + txns.push_back(obj); + if (!minReturnedIndex || txnPlusMeta.ledgerSequence < *minReturnedIndex) + minReturnedIndex = txnPlusMeta.ledgerSequence; + if (!maxReturnedIndex || txnPlusMeta.ledgerSequence > *maxReturnedIndex) + maxReturnedIndex = txnPlusMeta.ledgerSequence; + } + + response[JS(ledger_index_min)] = minIndex; + response[JS(ledger_index_max)] = maxIndex; + + response[JS(transactions)] = txns; + + auto serializationEnd = std::chrono::system_clock::now(); + BOOST_LOG_TRIVIAL(info) + << __func__ << " serialization took " + << ((serializationEnd - dbFetchEnd).count() / 1000000000.0); + + return response; +} + } // namespace RPC diff --git a/src/rpc/RPCHelpers.h b/src/rpc/RPCHelpers.h index 50823bd04..f7dc559c8 100644 --- a/src/rpc/RPCHelpers.h +++ b/src/rpc/RPCHelpers.h @@ -24,6 +24,7 @@ namespace RPC { std::optional accountFromStringStrict(std::string const& account); + std::optional accountFromSeed(std::string const& account); @@ -253,5 +254,18 @@ getChannelId(boost::json::object const& request, ripple::uint256& channelId); bool specifiesCurrentOrClosedLedger(boost::json::object const& request); +std::variant +getNFTID(boost::json::object const& request); + +// This function is the driver for both `account_tx` and `nft_tx` and should +// be used for any future transaction enumeration APIs. +std::variant +traverseTransactions( + Context const& context, + std::function const&)> getter); + } // namespace RPC #endif diff --git a/src/rpc/handlers/AccountTx.cpp b/src/rpc/handlers/AccountTx.cpp index ff1ef6247..807def312 100644 --- a/src/rpc/handlers/AccountTx.cpp +++ b/src/rpc/handlers/AccountTx.cpp @@ -2,211 +2,53 @@ #include #include -namespace RPC { +// { +// account: +// ledger_hash: +// ledger_index: +// ledger_index_min: +// ledger_index_max: +// binary: +// forward: +// limit: +// marker: +// } -using boost::json::value_to; +namespace RPC { Result doAccountTx(Context const& context) { auto request = context.params; - boost::json::object response = {}; ripple::AccountID accountID; if (auto const status = getAccount(request, accountID); status) return status; - bool const binary = getBool(request, JS(binary), false); - bool const forward = getBool(request, JS(forward), false); - - std::optional cursor; - - if (request.contains(JS(marker))) - { - auto const& obj = request.at(JS(marker)).as_object(); - - std::optional transactionIndex = {}; - if (obj.contains(JS(seq))) - { - if (!obj.at(JS(seq)).is_int64()) - return Status{ - Error::rpcINVALID_PARAMS, "transactionIndexNotInt"}; - - transactionIndex = - boost::json::value_to(obj.at(JS(seq))); - } - - std::optional ledgerIndex = {}; - if (obj.contains(JS(ledger))) - { - if (!obj.at(JS(ledger)).is_int64()) - return Status{Error::rpcINVALID_PARAMS, "ledgerIndexNotInt"}; - - ledgerIndex = - boost::json::value_to(obj.at(JS(ledger))); - } - - if (!transactionIndex || !ledgerIndex) - return Status{Error::rpcINVALID_PARAMS, "missingLedgerOrSeq"}; - - cursor = {*ledgerIndex, *transactionIndex}; - } - - auto minIndex = context.range.minSequence; - if (request.contains(JS(ledger_index_min))) - { - auto& min = request.at(JS(ledger_index_min)); - - if (!min.is_int64()) - return Status{Error::rpcINVALID_PARAMS, "ledgerSeqMinNotNumber"}; - - if (min.as_int64() != -1) - { - if (context.range.maxSequence < min.as_int64() || - context.range.minSequence > min.as_int64()) - return Status{ - Error::rpcINVALID_PARAMS, "ledgerSeqMinOutOfRange"}; - else - minIndex = value_to(min); - } - - if (forward && !cursor) - cursor = {minIndex, 0}; - } - - auto maxIndex = context.range.maxSequence; - if (request.contains(JS(ledger_index_max))) - { - auto& max = request.at(JS(ledger_index_max)); - - if (!max.is_int64()) - return Status{Error::rpcINVALID_PARAMS, "ledgerSeqMaxNotNumber"}; - - if (max.as_int64() != -1) - { - if (context.range.maxSequence < max.as_int64() || - context.range.minSequence > max.as_int64()) - return Status{ - Error::rpcINVALID_PARAMS, "ledgerSeqMaxOutOfRange"}; - else - maxIndex = value_to(max); - } - - if (minIndex > maxIndex) - return Status{Error::rpcINVALID_PARAMS, "invalidIndex"}; - - if (!forward && !cursor) - cursor = {maxIndex, INT32_MAX}; - } - - if (request.contains(JS(ledger_index)) || request.contains(JS(ledger_hash))) - { - if (request.contains(JS(ledger_index_max)) || - request.contains(JS(ledger_index_min))) - return Status{ - Error::rpcINVALID_PARAMS, "containsLedgerSpecifierAndRange"}; - - auto v = ledgerInfoFromRequest(context); - if (auto status = std::get_if(&v)) - return *status; - - maxIndex = minIndex = std::get(v).seq; - } - - if (!cursor) - { - if (forward) - cursor = {minIndex, 0}; - else - cursor = {maxIndex, INT32_MAX}; - } - - std::uint32_t limit; - if (auto const status = getLimit(context, limit); status) - return status; - - if (request.contains(JS(limit))) - response[JS(limit)] = limit; - - boost::json::array txns; - auto start = std::chrono::system_clock::now(); - auto [blobs, retCursor] = context.backend->fetchAccountTransactions( - accountID, limit, forward, cursor, context.yield); - - auto end = std::chrono::system_clock::now(); - BOOST_LOG_TRIVIAL(info) << __func__ << " db fetch took " - << ((end - start).count() / 1000000000.0) - << " num blobs = " << blobs.size(); + auto const maybeResponse = traverseTransactions( + context, + [&accountID, &context]( + std::uint32_t const limit, + bool const forward, + std::optional const& cursorIn) { + auto const start = std::chrono::system_clock::now(); + auto const txnsAndCursor = + context.backend->fetchAccountTransactions( + accountID, limit, forward, cursorIn, context.yield); + BOOST_LOG_TRIVIAL(info) + << __func__ << " db fetch took " + << ((std::chrono::system_clock::now() - start).count() / + 1000000000.0) + << " num blobs = " << txnsAndCursor.txns.size(); + return txnsAndCursor; + }); + + if (auto const status = std::get_if(&maybeResponse)) + return *status; + auto response = std::get(maybeResponse); response[JS(account)] = ripple::to_string(accountID); - - if (retCursor) - { - boost::json::object cursorJson; - cursorJson[JS(ledger)] = retCursor->ledgerSequence; - cursorJson[JS(seq)] = retCursor->transactionIndex; - response[JS(marker)] = cursorJson; - } - - std::optional maxReturnedIndex; - std::optional minReturnedIndex; - for (auto const& txnPlusMeta : blobs) - { - if (txnPlusMeta.ledgerSequence < minIndex || - txnPlusMeta.ledgerSequence > maxIndex) - { - BOOST_LOG_TRIVIAL(debug) - << __func__ - << " skipping over transactions from incomplete ledger"; - continue; - } - - boost::json::object obj; - - if (!binary) - { - auto [txn, meta] = toExpandedJson(txnPlusMeta); - obj[JS(meta)] = meta; - obj[JS(tx)] = txn; - obj[JS(tx)].as_object()[JS(ledger_index)] = - txnPlusMeta.ledgerSequence; - obj[JS(tx)].as_object()[JS(date)] = txnPlusMeta.date; - } - else - { - obj[JS(meta)] = ripple::strHex(txnPlusMeta.metadata); - obj[JS(tx_blob)] = ripple::strHex(txnPlusMeta.transaction); - obj[JS(ledger_index)] = txnPlusMeta.ledgerSequence; - obj[JS(date)] = txnPlusMeta.date; - } - obj[JS(validated)] = true; - - txns.push_back(obj); - if (!minReturnedIndex || txnPlusMeta.ledgerSequence < *minReturnedIndex) - minReturnedIndex = txnPlusMeta.ledgerSequence; - if (!maxReturnedIndex || txnPlusMeta.ledgerSequence > *maxReturnedIndex) - maxReturnedIndex = txnPlusMeta.ledgerSequence; - } - - assert(cursor); - if (!forward) - { - response[JS(ledger_index_min)] = cursor->ledgerSequence; - response[JS(ledger_index_max)] = maxIndex; - } - else - { - response[JS(ledger_index_max)] = cursor->ledgerSequence; - response[JS(ledger_index_min)] = minIndex; - } - - response[JS(transactions)] = txns; - - auto end2 = std::chrono::system_clock::now(); - BOOST_LOG_TRIVIAL(info) << __func__ << " serialization took " - << ((end2 - end).count() / 1000000000.0); - return response; -} // namespace RPC +} } // namespace RPC diff --git a/src/rpc/handlers/NFTInfo.cpp b/src/rpc/handlers/NFTInfo.cpp index f185c9e02..689093c20 100644 --- a/src/rpc/handlers/NFTInfo.cpp +++ b/src/rpc/handlers/NFTInfo.cpp @@ -89,24 +89,15 @@ doNFTInfo(Context const& context) auto request = context.params; boost::json::object response = {}; - if (!request.contains("nft_id")) - return Status{Error::rpcINVALID_PARAMS, "Missing nft_id"}; - - auto const& jsonTokenID = request.at("nft_id"); - if (!jsonTokenID.is_string()) - return Status{Error::rpcINVALID_PARAMS, "nft_id is not a string"}; - - ripple::uint256 tokenID; - if (!tokenID.parseHex(jsonTokenID.as_string().c_str())) - return Status{Error::rpcINVALID_PARAMS, "Malformed nft_id"}; - - // We only need to fetch the ledger header because the ledger hash is - // supposed to be included in the response. The ledger sequence is specified - // in the request - auto v = ledgerInfoFromRequest(context); - if (auto status = std::get_if(&v)) + auto const maybeTokenID = getNFTID(request); + if (auto const status = std::get_if(&maybeTokenID)) return *status; - ripple::LedgerInfo lgrInfo = std::get(v); + auto const tokenID = std::get(maybeTokenID); + + auto const maybeLedgerInfo = ledgerInfoFromRequest(context); + if (auto status = std::get_if(&maybeLedgerInfo)) + return *status; + auto const lgrInfo = std::get(maybeLedgerInfo); std::optional dbResponse = context.backend->fetchNFT(tokenID, lgrInfo.seq, context.yield); diff --git a/src/rpc/handlers/NFTOffers.cpp b/src/rpc/handlers/NFTOffers.cpp index adf513ff3..0500fdb98 100644 --- a/src/rpc/handlers/NFTOffers.cpp +++ b/src/rpc/handlers/NFTOffers.cpp @@ -129,26 +129,10 @@ enumerateNFTOffers( return response; } -std::variant -getTokenid(boost::json::object const& request) -{ - if (!request.contains(JS(nft_id))) - return Status{Error::rpcINVALID_PARAMS, "missingTokenid"}; - - if (!request.at(JS(nft_id)).is_string()) - return Status{Error::rpcINVALID_PARAMS, "tokenidNotString"}; - - ripple::uint256 tokenid; - if (!tokenid.parseHex(request.at(JS(nft_id)).as_string().c_str())) - return Status{Error::rpcINVALID_PARAMS, "malformedCursor"}; - - return tokenid; -} - Result doNFTOffers(Context const& context, bool sells) { - auto const v = getTokenid(context.params); + auto const v = getNFTID(context.params); if (auto const status = std::get_if(&v)) return *status; diff --git a/src/rpc/handlers/NFTTx.cpp b/src/rpc/handlers/NFTTx.cpp new file mode 100644 index 000000000..62cf8ba6b --- /dev/null +++ b/src/rpc/handlers/NFTTx.cpp @@ -0,0 +1,51 @@ +#include + +// { +// nft_id: +// ledger_hash: +// ledger_index: +// ledger_index_min: +// ledger_index_max: +// binary: +// forward: +// limit: +// marker: +// } + +namespace RPC { + +Result +doNFTTx(Context const& context) +{ + auto const maybeTokenID = getNFTID(context.params); + if (auto const status = std::get_if(&maybeTokenID)) + return *status; + auto const tokenID = std::get(maybeTokenID); + + auto const maybeResponse = traverseTransactions( + context, + [&tokenID, &context]( + std::uint32_t const limit, + bool const forward, + std::optional const& cursorIn) + -> Backend::TransactionsAndCursor { + auto const start = std::chrono::system_clock::now(); + auto const txnsAndCursor = context.backend->fetchNFTTransactions( + tokenID, limit, forward, cursorIn, context.yield); + BOOST_LOG_TRIVIAL(info) + << __func__ << " db fetch took " + << ((std::chrono::system_clock::now() - start).count() / + 1000000000.0) + << " num blobs = " << txnsAndCursor.txns.size(); + return txnsAndCursor; + }); + + if (auto const status = std::get_if(&maybeResponse)) + return *status; + auto response = std::get(maybeResponse); + + response[JS(nft_id)] = ripple::to_string(tokenID); + return response; +} + +} // namespace RPC From 25680788f56323a625a585373db7e356cfea8b31 Mon Sep 17 00:00:00 2001 From: ledhed2222 Date: Thu, 18 Aug 2022 19:37:13 +0000 Subject: [PATCH 2/7] address some comments --- src/rpc/RPC.cpp | 4 ++-- src/rpc/RPCHelpers.cpp | 26 ++++++++++++++++---------- src/rpc/RPCHelpers.h | 2 +- src/rpc/handlers/AccountTx.cpp | 5 +++-- src/rpc/handlers/NFTTx.cpp | 5 +++-- 5 files changed, 25 insertions(+), 17 deletions(-) diff --git a/src/rpc/RPC.cpp b/src/rpc/RPC.cpp index e145f3bc5..e9e3d629e 100644 --- a/src/rpc/RPC.cpp +++ b/src/rpc/RPC.cpp @@ -209,7 +209,7 @@ class HandlerTable } bool - getIsClioOnly(std::string const& command) + isClioOnly(std::string const& command) { return handlerMap_.contains(command) && handlerMap_[command].isClioOnly; } @@ -262,7 +262,7 @@ validHandler(std::string const& method) bool isClioOnly(std::string const& method) { - return handlerTable.getIsClioOnly(method); + return handlerTable.isClioOnly(method); } Status diff --git a/src/rpc/RPCHelpers.cpp b/src/rpc/RPCHelpers.cpp index 127670c86..d43a4b064 100644 --- a/src/rpc/RPCHelpers.cpp +++ b/src/rpc/RPCHelpers.cpp @@ -806,15 +806,19 @@ traverseOwnedNodes( } auto end = std::chrono::system_clock::now(); - BOOST_LOG_TRIVIAL(debug) << "Time loading owned directories: " - << ((end - start).count() / 1000000000.0); + BOOST_LOG_TRIVIAL(debug) + << "Time loading owned directories: " + << std::chrono::duration_cast(end - start) + .count(); start = std::chrono::system_clock::now(); auto objects = backend.fetchLedgerObjects(keys, sequence, yield); end = std::chrono::system_clock::now(); - BOOST_LOG_TRIVIAL(debug) << "Time loading owned entries: " - << ((end - start).count() / 1000000000.0); + BOOST_LOG_TRIVIAL(debug) + << "Time loading owned entries: " + << std::chrono::duration_cast(end - start) + .count(); for (auto i = 0; i < objects.size(); ++i) { @@ -1482,14 +1486,14 @@ std::variant getNFTID(boost::json::object const& request) { if (!request.contains(JS(nft_id))) - return Status{Error::rpcINVALID_PARAMS, "missingTokenid"}; + return Status{Error::rpcINVALID_PARAMS, "missingTokenID"}; if (!request.at(JS(nft_id)).is_string()) - return Status{Error::rpcINVALID_PARAMS, "tokenidNotString"}; + return Status{Error::rpcINVALID_PARAMS, "tokenIDNotString"}; ripple::uint256 tokenid; if (!tokenid.parseHex(request.at(JS(nft_id)).as_string().c_str())) - return Status{Error::rpcINVALID_PARAMS, "malformedCursor"}; + return Status{Error::rpcINVALID_PARAMS, "malformedTokenID"}; return tokenid; } @@ -1502,7 +1506,7 @@ traverseTransactions( std::function const&)> getter) + std::optional const&)> transactionFetcher) { auto request = context.params; boost::json::object response = {}; @@ -1620,7 +1624,7 @@ traverseTransactions( response[JS(limit)] = limit; boost::json::array txns; - auto [blobs, retCursor] = getter(limit, forward, cursor); + auto [blobs, retCursor] = transactionFetcher(limit, forward, cursor); auto dbFetchEnd = std::chrono::system_clock::now(); if (retCursor) @@ -1679,7 +1683,9 @@ traverseTransactions( auto serializationEnd = std::chrono::system_clock::now(); BOOST_LOG_TRIVIAL(info) << __func__ << " serialization took " - << ((serializationEnd - dbFetchEnd).count() / 1000000000.0); + << std::chrono::duration_cast( + serializationEnd - dbFetchEnd) + .count(); return response; } diff --git a/src/rpc/RPCHelpers.h b/src/rpc/RPCHelpers.h index f7dc559c8..12104d9b6 100644 --- a/src/rpc/RPCHelpers.h +++ b/src/rpc/RPCHelpers.h @@ -265,7 +265,7 @@ traverseTransactions( std::function const&)> getter); + std::optional const&)> transactionFetcher); } // namespace RPC #endif diff --git a/src/rpc/handlers/AccountTx.cpp b/src/rpc/handlers/AccountTx.cpp index 807def312..4586a92bd 100644 --- a/src/rpc/handlers/AccountTx.cpp +++ b/src/rpc/handlers/AccountTx.cpp @@ -37,8 +37,9 @@ doAccountTx(Context const& context) accountID, limit, forward, cursorIn, context.yield); BOOST_LOG_TRIVIAL(info) << __func__ << " db fetch took " - << ((std::chrono::system_clock::now() - start).count() / - 1000000000.0) + << std::chrono::duration_cast( + std::chrono::system_clock::now() - start) + .count() << " num blobs = " << txnsAndCursor.txns.size(); return txnsAndCursor; }); diff --git a/src/rpc/handlers/NFTTx.cpp b/src/rpc/handlers/NFTTx.cpp index 62cf8ba6b..70dd0ad47 100644 --- a/src/rpc/handlers/NFTTx.cpp +++ b/src/rpc/handlers/NFTTx.cpp @@ -34,8 +34,9 @@ doNFTTx(Context const& context) tokenID, limit, forward, cursorIn, context.yield); BOOST_LOG_TRIVIAL(info) << __func__ << " db fetch took " - << ((std::chrono::system_clock::now() - start).count() / - 1000000000.0) + << std::chrono::duration_cast( + std::chrono::system_clock::now() - start) + .count() << " num blobs = " << txnsAndCursor.txns.size(); return txnsAndCursor; }); From d2ac0d58c777cf74a78ae55a0711e6f5ec294d55 Mon Sep 17 00:00:00 2001 From: ledhed2222 Date: Fri, 19 Aug 2022 03:22:55 +0000 Subject: [PATCH 3/7] respond to comments --- src/rpc/RPCHelpers.cpp | 20 ++++++++------------ src/rpc/RPCHelpers.h | 4 +++- src/rpc/handlers/AccountTx.cpp | 25 +++++++------------------ src/rpc/handlers/NFTInfo.cpp | 8 ++++---- src/rpc/handlers/NFTTx.cpp | 26 ++++++++------------------ 5 files changed, 30 insertions(+), 53 deletions(-) diff --git a/src/rpc/RPCHelpers.cpp b/src/rpc/RPCHelpers.cpp index d43a4b064..7b70172d0 100644 --- a/src/rpc/RPCHelpers.cpp +++ b/src/rpc/RPCHelpers.cpp @@ -268,7 +268,7 @@ getTaker(boost::json::object const& request, ripple::AccountID& takerID) if (request.contains(JS(taker))) { auto parsed = parseTaker(request.at(JS(taker))); - if (auto status = std::get_if(&parsed)) + if (auto status = std::get_if(&parsed); status) return *status; else takerID = std::get(parsed); @@ -1504,9 +1504,11 @@ std::variant traverseTransactions( Context const& context, std::function const& backend, std::uint32_t const, bool const, - std::optional const&)> transactionFetcher) + std::optional const&, + boost::asio::yield_context& yield)> transactionFetcher) { auto request = context.params; boost::json::object response = {}; @@ -1602,7 +1604,7 @@ traverseTransactions( Error::rpcINVALID_PARAMS, "containsLedgerSpecifierAndRange"}; auto v = ledgerInfoFromRequest(context); - if (auto status = std::get_if(&v)) + if (auto status = std::get_if(&v); status) return *status; maxIndex = minIndex = std::get(v).seq; @@ -1624,7 +1626,8 @@ traverseTransactions( response[JS(limit)] = limit; boost::json::array txns; - auto [blobs, retCursor] = transactionFetcher(limit, forward, cursor); + auto [blobs, retCursor] = transactionFetcher( + context.backend, limit, forward, cursor, context.yield); auto dbFetchEnd = std::chrono::system_clock::now(); if (retCursor) @@ -1635,8 +1638,6 @@ traverseTransactions( response[JS(marker)] = cursorJson; } - std::optional maxReturnedIndex; - std::optional minReturnedIndex; for (auto const& txnPlusMeta : blobs) { if (txnPlusMeta.ledgerSequence < minIndex || @@ -1669,10 +1670,6 @@ traverseTransactions( obj[JS(validated)] = true; txns.push_back(obj); - if (!minReturnedIndex || txnPlusMeta.ledgerSequence < *minReturnedIndex) - minReturnedIndex = txnPlusMeta.ledgerSequence; - if (!maxReturnedIndex || txnPlusMeta.ledgerSequence > *maxReturnedIndex) - maxReturnedIndex = txnPlusMeta.ledgerSequence; } response[JS(ledger_index_min)] = minIndex; @@ -1680,11 +1677,10 @@ traverseTransactions( response[JS(transactions)] = txns; - auto serializationEnd = std::chrono::system_clock::now(); BOOST_LOG_TRIVIAL(info) << __func__ << " serialization took " << std::chrono::duration_cast( - serializationEnd - dbFetchEnd) + std::chrono::system_clock::now() - dbFetchEnd) .count(); return response; diff --git a/src/rpc/RPCHelpers.h b/src/rpc/RPCHelpers.h index 12104d9b6..2da191a0c 100644 --- a/src/rpc/RPCHelpers.h +++ b/src/rpc/RPCHelpers.h @@ -263,9 +263,11 @@ std::variant traverseTransactions( Context const& context, std::function const& backend, std::uint32_t const, bool const, - std::optional const&)> transactionFetcher); + std::optional const&, + boost::asio::yield_context& yield)> transactionFetcher); } // namespace RPC #endif diff --git a/src/rpc/handlers/AccountTx.cpp b/src/rpc/handlers/AccountTx.cpp index 4586a92bd..9349c5620 100644 --- a/src/rpc/handlers/AccountTx.cpp +++ b/src/rpc/handlers/AccountTx.cpp @@ -2,18 +2,6 @@ #include #include -// { -// account: -// ledger_hash: -// ledger_index: -// ledger_index_min: -// ledger_index_max: -// binary: -// forward: -// limit: -// marker: -// } - namespace RPC { Result @@ -27,14 +15,15 @@ doAccountTx(Context const& context) auto const maybeResponse = traverseTransactions( context, - [&accountID, &context]( + [&accountID]( + std::shared_ptr const& backend, std::uint32_t const limit, bool const forward, - std::optional const& cursorIn) { + std::optional const& cursorIn, + boost::asio::yield_context& yield) { auto const start = std::chrono::system_clock::now(); - auto const txnsAndCursor = - context.backend->fetchAccountTransactions( - accountID, limit, forward, cursorIn, context.yield); + auto const txnsAndCursor = backend->fetchAccountTransactions( + accountID, limit, forward, cursorIn, yield); BOOST_LOG_TRIVIAL(info) << __func__ << " db fetch took " << std::chrono::duration_cast( @@ -44,7 +33,7 @@ doAccountTx(Context const& context) return txnsAndCursor; }); - if (auto const status = std::get_if(&maybeResponse)) + if (auto const status = std::get_if(&maybeResponse); status) return *status; auto response = std::get(maybeResponse); diff --git a/src/rpc/handlers/NFTInfo.cpp b/src/rpc/handlers/NFTInfo.cpp index 689093c20..da234f7c1 100644 --- a/src/rpc/handlers/NFTInfo.cpp +++ b/src/rpc/handlers/NFTInfo.cpp @@ -90,12 +90,12 @@ doNFTInfo(Context const& context) boost::json::object response = {}; auto const maybeTokenID = getNFTID(request); - if (auto const status = std::get_if(&maybeTokenID)) + if (auto const status = std::get_if(&maybeTokenID); status) return *status; auto const tokenID = std::get(maybeTokenID); auto const maybeLedgerInfo = ledgerInfoFromRequest(context); - if (auto status = std::get_if(&maybeLedgerInfo)) + if (auto status = std::get_if(&maybeLedgerInfo); status) return *status; auto const lgrInfo = std::get(maybeLedgerInfo); @@ -121,10 +121,10 @@ doNFTInfo(Context const& context) { auto const maybeURI = getURI(*dbResponse, context); // An error occurred - if (Status const* status = std::get_if(&maybeURI)) + if (Status const* status = std::get_if(&maybeURI); status) return *status; // A URI was found - if (std::string const* uri = std::get_if(&maybeURI)) + if (std::string const* uri = std::get_if(&maybeURI); uri) response["uri"] = *uri; // A URI was not found, explicitly set to null else diff --git a/src/rpc/handlers/NFTTx.cpp b/src/rpc/handlers/NFTTx.cpp index 70dd0ad47..de28d2122 100644 --- a/src/rpc/handlers/NFTTx.cpp +++ b/src/rpc/handlers/NFTTx.cpp @@ -1,37 +1,27 @@ #include -// { -// nft_id: -// ledger_hash: -// ledger_index: -// ledger_index_min: -// ledger_index_max: -// binary: -// forward: -// limit: -// marker: -// } - namespace RPC { Result doNFTTx(Context const& context) { auto const maybeTokenID = getNFTID(context.params); - if (auto const status = std::get_if(&maybeTokenID)) + if (auto const status = std::get_if(&maybeTokenID); status) return *status; auto const tokenID = std::get(maybeTokenID); auto const maybeResponse = traverseTransactions( context, - [&tokenID, &context]( + [&tokenID]( + std::shared_ptr const& backend, std::uint32_t const limit, bool const forward, - std::optional const& cursorIn) + std::optional const& cursorIn, + boost::asio::yield_context& yield) -> Backend::TransactionsAndCursor { auto const start = std::chrono::system_clock::now(); - auto const txnsAndCursor = context.backend->fetchNFTTransactions( - tokenID, limit, forward, cursorIn, context.yield); + auto const txnsAndCursor = backend->fetchNFTTransactions( + tokenID, limit, forward, cursorIn, yield); BOOST_LOG_TRIVIAL(info) << __func__ << " db fetch took " << std::chrono::duration_cast( @@ -41,7 +31,7 @@ doNFTTx(Context const& context) return txnsAndCursor; }); - if (auto const status = std::get_if(&maybeResponse)) + if (auto const status = std::get_if(&maybeResponse); status) return *status; auto response = std::get(maybeResponse); From ec799a94509859195880c0126e9fae79a1f8209c Mon Sep 17 00:00:00 2001 From: ledhed2222 Date: Fri, 19 Aug 2022 23:50:33 +0000 Subject: [PATCH 4/7] address more comments --- CMakeLists.txt | 2 +- src/backend/CassandraBackend.cpp | 10 ++++++---- src/rpc/RPCHelpers.cpp | 2 ++ 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index cc3e8aa00..cf55332d4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -75,8 +75,8 @@ target_sources(clio PRIVATE src/rpc/handlers/NoRippleCheck.cpp # NFT src/rpc/handlers/NFTInfo.cpp - src/rpc/handlers/NFTTx.cpp src/rpc/handlers/NFTOffers.cpp + src/rpc/handlers/NFTTx.cpp # Ledger src/rpc/handlers/Ledger.cpp src/rpc/handlers/LedgerData.cpp diff --git a/src/backend/CassandraBackend.cpp b/src/backend/CassandraBackend.cpp index 1a4a87994..0dfc63184 100644 --- a/src/backend/CassandraBackend.cpp +++ b/src/backend/CassandraBackend.cpp @@ -651,8 +651,9 @@ CassandraBackend::fetchNFTTransactions( static_cast(lgrSeq), static_cast(txnIdx)}; - // Only modify if forward because forward query is >= seq_idx, - // while reverse query handles this via < seq_idx + // Only modify if forward because forward query + // (selectNFTTxForward_) orders by ledger/tx sequence >= whereas + // reverse query (selectNFTTx_) orders by ledger/tx sequence <. if (forward) ++cursor->transactionIndex; } @@ -736,8 +737,9 @@ CassandraBackend::fetchAccountTransactions( static_cast(lgrSeq), static_cast(txnIdx)}; - // Only modify if forward because forward query is >= seq_idx, - // while reverse query handles this via < seq_idx + // Only modify if forward because forward query + // (selectAccountTxForward_) orders by ledger/tx sequence >= whereas + // reverse query (selectAccountTx_) orders by ledger/tx sequence <. if (forward) ++cursor->transactionIndex; } diff --git a/src/rpc/RPCHelpers.cpp b/src/rpc/RPCHelpers.cpp index 7b70172d0..f3da67319 100644 --- a/src/rpc/RPCHelpers.cpp +++ b/src/rpc/RPCHelpers.cpp @@ -1520,6 +1520,8 @@ traverseTransactions( if (request.contains(JS(marker))) { + if (!request.at(JS(marker)).is_object()) + return Status{Error::rpcINVALID_PARAMS, "invalidMarker"}; auto const& obj = request.at(JS(marker)).as_object(); std::optional transactionIndex = {}; From 940b110a2f8ce1852776379d9c38c81f91d3393f Mon Sep 17 00:00:00 2001 From: ledhed2222 Date: Sat, 20 Aug 2022 00:30:42 +0000 Subject: [PATCH 5/7] improves logging clarity --- src/rpc/RPCHelpers.cpp | 9 ++++++--- src/rpc/handlers/AccountTx.cpp | 4 ++-- src/rpc/handlers/NFTTx.cpp | 4 ++-- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/rpc/RPCHelpers.cpp b/src/rpc/RPCHelpers.cpp index f3da67319..6909464f8 100644 --- a/src/rpc/RPCHelpers.cpp +++ b/src/rpc/RPCHelpers.cpp @@ -809,7 +809,8 @@ traverseOwnedNodes( BOOST_LOG_TRIVIAL(debug) << "Time loading owned directories: " << std::chrono::duration_cast(end - start) - .count(); + .count() + << " milliseconds"; start = std::chrono::system_clock::now(); auto objects = backend.fetchLedgerObjects(keys, sequence, yield); @@ -818,7 +819,8 @@ traverseOwnedNodes( BOOST_LOG_TRIVIAL(debug) << "Time loading owned entries: " << std::chrono::duration_cast(end - start) - .count(); + .count() + << " milliseconds"; for (auto i = 0; i < objects.size(); ++i) { @@ -1683,7 +1685,8 @@ traverseTransactions( << __func__ << " serialization took " << std::chrono::duration_cast( std::chrono::system_clock::now() - dbFetchEnd) - .count(); + .count() + << " milliseconds"; return response; } diff --git a/src/rpc/handlers/AccountTx.cpp b/src/rpc/handlers/AccountTx.cpp index 9349c5620..581d164b9 100644 --- a/src/rpc/handlers/AccountTx.cpp +++ b/src/rpc/handlers/AccountTx.cpp @@ -25,11 +25,11 @@ doAccountTx(Context const& context) auto const txnsAndCursor = backend->fetchAccountTransactions( accountID, limit, forward, cursorIn, yield); BOOST_LOG_TRIVIAL(info) - << __func__ << " db fetch took " + << "doAccountTx db fetch took " << std::chrono::duration_cast( std::chrono::system_clock::now() - start) .count() - << " num blobs = " << txnsAndCursor.txns.size(); + << " milliseconds - num blobs = " << txnsAndCursor.txns.size(); return txnsAndCursor; }); diff --git a/src/rpc/handlers/NFTTx.cpp b/src/rpc/handlers/NFTTx.cpp index de28d2122..db0c1c028 100644 --- a/src/rpc/handlers/NFTTx.cpp +++ b/src/rpc/handlers/NFTTx.cpp @@ -23,11 +23,11 @@ doNFTTx(Context const& context) auto const txnsAndCursor = backend->fetchNFTTransactions( tokenID, limit, forward, cursorIn, yield); BOOST_LOG_TRIVIAL(info) - << __func__ << " db fetch took " + << "doNFTTx db fetch took " << std::chrono::duration_cast( std::chrono::system_clock::now() - start) .count() - << " num blobs = " << txnsAndCursor.txns.size(); + << " milliseconds - num blobs = " << txnsAndCursor.txns.size(); return txnsAndCursor; }); From 79f230b363de0542a5fe7de343a6004dd7d4a3a7 Mon Sep 17 00:00:00 2001 From: ledhed2222 Date: Sat, 20 Aug 2022 00:42:39 +0000 Subject: [PATCH 6/7] further improve logging --- src/rpc/handlers/AccountTx.cpp | 11 ++++------- src/rpc/handlers/NFTTx.cpp | 5 +++-- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/rpc/handlers/AccountTx.cpp b/src/rpc/handlers/AccountTx.cpp index 581d164b9..8fdd3da2a 100644 --- a/src/rpc/handlers/AccountTx.cpp +++ b/src/rpc/handlers/AccountTx.cpp @@ -1,5 +1,3 @@ -#include -#include #include namespace RPC { @@ -7,15 +5,14 @@ namespace RPC { Result doAccountTx(Context const& context) { - auto request = context.params; - ripple::AccountID accountID; - if (auto const status = getAccount(request, accountID); status) + if (auto const status = getAccount(context.params, accountID); status) return status; + constexpr std::string_view outerFuncName = __func__; auto const maybeResponse = traverseTransactions( context, - [&accountID]( + [&accountID, &outerFuncName]( std::shared_ptr const& backend, std::uint32_t const limit, bool const forward, @@ -25,7 +22,7 @@ doAccountTx(Context const& context) auto const txnsAndCursor = backend->fetchAccountTransactions( accountID, limit, forward, cursorIn, yield); BOOST_LOG_TRIVIAL(info) - << "doAccountTx db fetch took " + << outerFuncName << " db fetch took " << std::chrono::duration_cast( std::chrono::system_clock::now() - start) .count() diff --git a/src/rpc/handlers/NFTTx.cpp b/src/rpc/handlers/NFTTx.cpp index db0c1c028..5e4f49623 100644 --- a/src/rpc/handlers/NFTTx.cpp +++ b/src/rpc/handlers/NFTTx.cpp @@ -10,9 +10,10 @@ doNFTTx(Context const& context) return *status; auto const tokenID = std::get(maybeTokenID); + constexpr std::string_view outerFuncName = __func__; auto const maybeResponse = traverseTransactions( context, - [&tokenID]( + [&tokenID, &outerFuncName]( std::shared_ptr const& backend, std::uint32_t const limit, bool const forward, @@ -23,7 +24,7 @@ doNFTTx(Context const& context) auto const txnsAndCursor = backend->fetchNFTTransactions( tokenID, limit, forward, cursorIn, yield); BOOST_LOG_TRIVIAL(info) - << "doNFTTx db fetch took " + << outerFuncName << " db fetch took " << std::chrono::duration_cast( std::chrono::system_clock::now() - start) .count() From 98105282c62f6748e733413f8a44b2a3694d8777 Mon Sep 17 00:00:00 2001 From: ledhed2222 Date: Thu, 1 Sep 2022 17:32:04 +0000 Subject: [PATCH 7/7] comments --- src/rpc/RPCHelpers.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rpc/RPCHelpers.cpp b/src/rpc/RPCHelpers.cpp index 6909464f8..8179a2e22 100644 --- a/src/rpc/RPCHelpers.cpp +++ b/src/rpc/RPCHelpers.cpp @@ -1632,7 +1632,7 @@ traverseTransactions( boost::json::array txns; auto [blobs, retCursor] = transactionFetcher( context.backend, limit, forward, cursor, context.yield); - auto dbFetchEnd = std::chrono::system_clock::now(); + auto serializationStart = std::chrono::system_clock::now(); if (retCursor) { @@ -1684,7 +1684,7 @@ traverseTransactions( BOOST_LOG_TRIVIAL(info) << __func__ << " serialization took " << std::chrono::duration_cast( - std::chrono::system_clock::now() - dbFetchEnd) + std::chrono::system_clock::now() - serializationStart) .count() << " milliseconds";