From a4adde59653b640323d10082d3e4a8f6074e3650 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki <823856+Bronek@users.noreply.github.com> Date: Tue, 26 Sep 2023 19:43:08 +0100 Subject: [PATCH 01/21] Add DeliverMax to submit, sign, tx etc. --- Builds/CMake/RippledCore.cmake | 1 + src/ripple/app/ledger/impl/LedgerToJson.cpp | 2 + src/ripple/protocol/jss.h | 1 + src/ripple/rpc/DeliverMax.h | 67 ++++++++++++++++++++ src/ripple/rpc/handlers/AccountTx.cpp | 7 +- src/ripple/rpc/handlers/TransactionEntry.cpp | 3 + src/ripple/rpc/handlers/Tx.cpp | 7 +- src/ripple/rpc/handlers/TxHistory.cpp | 7 +- src/ripple/rpc/impl/DeliverMax.cpp | 47 ++++++++++++++ src/ripple/rpc/impl/TransactionSign.cpp | 12 ++++ 10 files changed, 149 insertions(+), 5 deletions(-) create mode 100644 src/ripple/rpc/DeliverMax.h create mode 100644 src/ripple/rpc/impl/DeliverMax.cpp diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index 8d2ff6cbaef..72025f4cfbb 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -731,6 +731,7 @@ target_sources (rippled PRIVATE src/ripple/rpc/handlers/Validators.cpp src/ripple/rpc/handlers/WalletPropose.cpp src/ripple/rpc/impl/DeliveredAmount.cpp + src/ripple/rpc/impl/DeliverMax.cpp src/ripple/rpc/impl/Handler.cpp src/ripple/rpc/impl/LegacyPathFind.cpp src/ripple/rpc/impl/RPCHandler.cpp diff --git a/src/ripple/app/ledger/impl/LedgerToJson.cpp b/src/ripple/app/ledger/impl/LedgerToJson.cpp index 8234ba16f9e..9b9a6d05436 100644 --- a/src/ripple/app/ledger/impl/LedgerToJson.cpp +++ b/src/ripple/app/ledger/impl/LedgerToJson.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include namespace ripple { @@ -123,6 +124,7 @@ fillJsonTx( else { copyFrom(txJson, txn->getJson(JsonOptions::none)); + insertDeliverMax(txJson, *fill.context, txnType); if (stMeta) { txJson[jss::metaData] = stMeta->getJson(JsonOptions::none); diff --git a/src/ripple/protocol/jss.h b/src/ripple/protocol/jss.h index e31a1cb3bf8..685a774c56d 100644 --- a/src/ripple/protocol/jss.h +++ b/src/ripple/protocol/jss.h @@ -69,6 +69,7 @@ JSS(CheckCash); // transaction type. JSS(CheckCreate); // transaction type. JSS(Clawback); // transaction type. JSS(ClearFlag); // field. +JSS(DeliverMax); // out: alias to Amount JSS(DeliverMin); // in: TransactionSign JSS(DepositPreauth); // transaction and ledger type. JSS(Destination); // in: TransactionSign; field. diff --git a/src/ripple/rpc/DeliverMax.h b/src/ripple/rpc/DeliverMax.h new file mode 100644 index 00000000000..46a8425bf76 --- /dev/null +++ b/src/ripple/rpc/DeliverMax.h @@ -0,0 +1,67 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2023 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#ifndef RIPPLE_RPC_DELIVERMAX_H_INCLUDED +#define RIPPLE_RPC_DELIVERMAX_H_INCLUDED + +#include +#include +#include + +#include +#include + +namespace Json { +class Value; +} + +namespace ripple { + +class ReadView; +class Transaction; +class TxMeta; +class STTx; + +namespace RPC { + +struct JsonContext; + +struct Context; + +/** + Copy `Amount` field to `DeliverMax` field in transaction output JSON. + This only applies to Payment transaction type, all others are ignored. + + When context.apiVersion > 1 will also remove `Amount` field, forcing users + to access this value using new `DeliverMax` field only. + @{ + */ + +void +insertDeliverMax( + Json::Value& tx_json, + RPC::Context const& context, + TxType txnType); + +/** @} */ + +} // namespace RPC +} // namespace ripple + +#endif diff --git a/src/ripple/rpc/handlers/AccountTx.cpp b/src/ripple/rpc/handlers/AccountTx.cpp index cee8a629c75..c7dfc5f8655 100644 --- a/src/ripple/rpc/handlers/AccountTx.cpp +++ b/src/ripple/rpc/handlers/AccountTx.cpp @@ -34,6 +34,7 @@ #include #include #include +#include #include #include #include @@ -326,6 +327,9 @@ populateJsonResponse( Json::Value& jvObj = jvTxns.append(Json::objectValue); jvObj[jss::tx] = txn->getJson(JsonOptions::include_date); + auto const& sttx = txn->getSTransaction(); + insertDeliverMax( + jvObj[jss::tx], context, sttx->getTxnType()); if (txnMeta) { jvObj[jss::meta] = @@ -333,8 +337,7 @@ populateJsonResponse( jvObj[jss::validated] = true; insertDeliveredAmount( jvObj[jss::meta], context, txn, *txnMeta); - insertNFTSyntheticInJson( - jvObj, txn->getSTransaction(), *txnMeta); + insertNFTSyntheticInJson(jvObj, sttx, *txnMeta); } } } diff --git a/src/ripple/rpc/handlers/TransactionEntry.cpp b/src/ripple/rpc/handlers/TransactionEntry.cpp index 83c4d18a236..de5141d479b 100644 --- a/src/ripple/rpc/handlers/TransactionEntry.cpp +++ b/src/ripple/rpc/handlers/TransactionEntry.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include namespace ripple { @@ -71,6 +72,8 @@ doTransactionEntry(RPC::JsonContext& context) else { jvResult[jss::tx_json] = sttx->getJson(JsonOptions::none); + insertDeliverMax( + jvResult[jss::tx_json], context, sttx->getTxnType()); if (stobj) jvResult[jss::metadata] = stobj->getJson(JsonOptions::none); // 'accounts' diff --git a/src/ripple/rpc/handlers/Tx.cpp b/src/ripple/rpc/handlers/Tx.cpp index 9416c441209..0fed1e5ae01 100644 --- a/src/ripple/rpc/handlers/Tx.cpp +++ b/src/ripple/rpc/handlers/Tx.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -311,6 +312,9 @@ populateJsonResponse( else if (result.txn) { response = result.txn->getJson(JsonOptions::include_date, args.binary); + auto const& sttx = result.txn->getSTransaction(); + if (!args.binary) + insertDeliverMax(response, context, sttx->getTxnType()); // populate binary metadata if (auto blob = std::get_if(&result.meta)) @@ -327,8 +331,7 @@ populateJsonResponse( response[jss::meta] = meta->getJson(JsonOptions::none); insertDeliveredAmount( response[jss::meta], context, result.txn, *meta); - insertNFTSyntheticInJson( - response, result.txn->getSTransaction(), *meta); + insertNFTSyntheticInJson(response, sttx, *meta); } } response[jss::validated] = result.validated; diff --git a/src/ripple/rpc/handlers/TxHistory.cpp b/src/ripple/rpc/handlers/TxHistory.cpp index 4c76bfac026..5184550e617 100644 --- a/src/ripple/rpc/handlers/TxHistory.cpp +++ b/src/ripple/rpc/handlers/TxHistory.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -63,7 +64,11 @@ doTxHistory(RPC::JsonContext& context) obj["used_postgres"] = true; for (auto const& t : trans) - txs.append(t->getJson(JsonOptions::none)); + { + Json::Value tx_json = t->getJson(JsonOptions::none); + insertDeliverMax(tx_json, context, t->getSTransaction()->getTxnType()); + txs.append(tx_json); + } return obj; } diff --git a/src/ripple/rpc/impl/DeliverMax.cpp b/src/ripple/rpc/impl/DeliverMax.cpp new file mode 100644 index 00000000000..f296b886b4b --- /dev/null +++ b/src/ripple/rpc/impl/DeliverMax.cpp @@ -0,0 +1,47 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2023 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include +#include +#include +#include "ripple/protocol/TxFormats.h" + +namespace ripple { +namespace RPC { + +void +insertDeliverMax( + Json::Value& tx_json, + RPC::Context const& context, + TxType txnType) +{ + if (tx_json.isMember(jss::Amount)) + { + if (txnType == ttPAYMENT) + { + tx_json[jss::DeliverMax] = tx_json[jss::Amount]; + if (context.apiVersion > 1) + tx_json.removeMember(jss::Amount); + } + } +} + +} // namespace RPC +} // namespace ripple diff --git a/src/ripple/rpc/impl/TransactionSign.cpp b/src/ripple/rpc/impl/TransactionSign.cpp index 5cd0c9edee3..f4509bf7afd 100644 --- a/src/ripple/rpc/impl/TransactionSign.cpp +++ b/src/ripple/rpc/impl/TransactionSign.cpp @@ -167,6 +167,18 @@ checkPayment( if (tx_json[jss::TransactionType].asString() != jss::Payment) return Json::Value(); + // DeliverMax is an alias to Amount and we use Amount internally + if (tx_json.isMember(jss::DeliverMax)) + { + if (tx_json.isMember(jss::Amount)) + return RPC::make_error( + rpcINVALID_PARAMS, + "Cannot specify both 'Amount' and 'DeliverMax`"); + + tx_json[jss::Amount] = tx_json[jss::DeliverMax]; + tx_json.removeMember(jss::DeliverMax); + } + if (!tx_json.isMember(jss::Amount)) return RPC::missing_field_error("tx_json.Amount"); From 7d79f8986fbc6e5a54df7bd260246e9a646d2f04 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki <823856+Bronek@users.noreply.github.com> Date: Thu, 28 Sep 2023 15:01:45 +0100 Subject: [PATCH 02/21] Add DeliverMax to subscribe --- src/ripple/app/ledger/BookListeners.cpp | 6 +- src/ripple/app/ledger/BookListeners.h | 2 +- src/ripple/app/ledger/OrderBookDB.cpp | 5 +- src/ripple/app/ledger/OrderBookDB.h | 3 +- src/ripple/app/ledger/impl/LedgerToJson.cpp | 2 +- src/ripple/app/misc/NetworkOPs.cpp | 132 +++++++++++++++++-- src/ripple/net/InfoSub.h | 7 + src/ripple/net/impl/InfoSub.cpp | 12 ++ src/ripple/rpc/DeliverMax.h | 5 +- src/ripple/rpc/handlers/AccountTx.cpp | 4 +- src/ripple/rpc/handlers/PathFind.cpp | 2 + src/ripple/rpc/handlers/Subscribe.cpp | 1 + src/ripple/rpc/handlers/TransactionEntry.cpp | 4 +- src/ripple/rpc/handlers/Tx.cpp | 3 +- src/ripple/rpc/handlers/TxHistory.cpp | 3 +- src/ripple/rpc/impl/DeliverMax.cpp | 7 +- 16 files changed, 163 insertions(+), 35 deletions(-) diff --git a/src/ripple/app/ledger/BookListeners.cpp b/src/ripple/app/ledger/BookListeners.cpp index 885ca4d322a..379e2e32adb 100644 --- a/src/ripple/app/ledger/BookListeners.cpp +++ b/src/ripple/app/ledger/BookListeners.cpp @@ -40,6 +40,7 @@ BookListeners::removeSubscriber(std::uint64_t seq) void BookListeners::publish( Json::Value const& jvObj, + Json::Value const& jvObjApiVer2, hash_set& havePublished) { std::lock_guard sl(mLock); @@ -54,7 +55,10 @@ BookListeners::publish( // Only publish jvObj if this is the first occurence if (havePublished.emplace(p->getSeq()).second) { - p->send(jvObj, true); + if (p->getApiVersion() == 1) + p->send(jvObj, true); + else + p->send(jvObjApiVer2, true); } ++it; } diff --git a/src/ripple/app/ledger/BookListeners.h b/src/ripple/app/ledger/BookListeners.h index 2f668f34042..6e0fed48c07 100644 --- a/src/ripple/app/ledger/BookListeners.h +++ b/src/ripple/app/ledger/BookListeners.h @@ -58,7 +58,7 @@ class BookListeners */ void - publish(Json::Value const& jvObj, hash_set& havePublished); + publish(Json::Value const& jvObj, Json::Value const& jvObjApiVer2, hash_set& havePublished); private: std::recursive_mutex mLock; diff --git a/src/ripple/app/ledger/OrderBookDB.cpp b/src/ripple/app/ledger/OrderBookDB.cpp index 45262c4d8a9..163321f8ed9 100644 --- a/src/ripple/app/ledger/OrderBookDB.cpp +++ b/src/ripple/app/ledger/OrderBookDB.cpp @@ -250,7 +250,8 @@ void OrderBookDB::processTxn( std::shared_ptr const& ledger, const AcceptedLedgerTx& alTx, - Json::Value const& jvObj) + Json::Value const& jvObj, + Json::Value const& jvObjApiVer2) { std::lock_guard sl(mLock); @@ -277,7 +278,7 @@ OrderBookDB::processTxn( {data->getFieldAmount(sfTakerGets).issue(), data->getFieldAmount(sfTakerPays).issue()}); if (listeners) - listeners->publish(jvObj, havePublished); + listeners->publish(jvObj, jvObjApiVer2, havePublished); } }; diff --git a/src/ripple/app/ledger/OrderBookDB.h b/src/ripple/app/ledger/OrderBookDB.h index ea7c60c5f5b..8fd5945072b 100644 --- a/src/ripple/app/ledger/OrderBookDB.h +++ b/src/ripple/app/ledger/OrderBookDB.h @@ -63,7 +63,8 @@ class OrderBookDB processTxn( std::shared_ptr const& ledger, const AcceptedLedgerTx& alTx, - Json::Value const& jvObj); + Json::Value const& jvObj, + Json::Value const& jvObjApiVer2); private: Application& app_; diff --git a/src/ripple/app/ledger/impl/LedgerToJson.cpp b/src/ripple/app/ledger/impl/LedgerToJson.cpp index 9b9a6d05436..7fee8703636 100644 --- a/src/ripple/app/ledger/impl/LedgerToJson.cpp +++ b/src/ripple/app/ledger/impl/LedgerToJson.cpp @@ -124,7 +124,7 @@ fillJsonTx( else { copyFrom(txJson, txn->getJson(JsonOptions::none)); - insertDeliverMax(txJson, *fill.context, txnType); + RPC::insertDeliverMax(txJson, txnType, fill.context->apiVersion); if (stMeta) { txJson[jss::metaData] = stMeta->getJson(JsonOptions::none); diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 541f4f16fe0..b70b2bd00e9 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -64,6 +64,7 @@ #include #include #include +#include #include #include #include @@ -2745,6 +2746,11 @@ NetworkOPsImp::pubProposedTransaction( TER result) { Json::Value jvObj = transJson(*transaction, result, false, ledger); + Json::Value jvObjApi2 = transJson(*transaction, result, false, ledger); + RPC::insertDeliverMax( + jvObj[jss::transaction], transaction->getTxnType(), 1); + RPC::insertDeliverMax( + jvObjApi2[1][jss::transaction], transaction->getTxnType(), 2); { std::lock_guard sl(mSubLock); @@ -2756,7 +2762,10 @@ NetworkOPsImp::pubProposedTransaction( if (p) { - p->send(jvObj, true); + if (p->getApiVersion() == 1) + p->send(jvObj, true); + else + p->send(jvObjApi2, true); ++it; } else @@ -3149,11 +3158,19 @@ NetworkOPsImp::pubValidatedTransaction( Json::Value jvObj = transJson(*stTxn, transaction.getResult(), true, ledger); + Json::Value jvObjApi2 = + transJson(*stTxn, transaction.getResult(), true, ledger); { auto const& meta = transaction.getMeta(); jvObj[jss::meta] = meta.getJson(JsonOptions::none); RPC::insertDeliveredAmount(jvObj[jss::meta], *ledger, stTxn, meta); + RPC::insertDeliverMax(jvObj[jss::transaction], stTxn->getTxnType(), 1); + + jvObjApi2[jss::meta] = meta.getJson(JsonOptions::none); + RPC::insertDeliveredAmount(jvObjApi2[jss::meta], *ledger, stTxn, meta); + RPC::insertDeliverMax( + jvObjApi2[jss::transaction], stTxn->getTxnType(), 2); } { @@ -3166,7 +3183,10 @@ NetworkOPsImp::pubValidatedTransaction( if (p) { - p->send(jvObj, true); + if (p->getApiVersion() == 1) + p->send(jvObj, true); + else + p->send(jvObjApi2, true); ++it; } else @@ -3181,7 +3201,10 @@ NetworkOPsImp::pubValidatedTransaction( if (p) { - p->send(jvObj, true); + if (p->getApiVersion() == 1) + p->send(jvObj, true); + else + p->send(jvObjApi2, true); ++it; } else @@ -3190,7 +3213,7 @@ NetworkOPsImp::pubValidatedTransaction( } if (transaction.getResult() == tesSUCCESS) - app_.getOrderBookDB().processTxn(ledger, transaction, jvObj); + app_.getOrderBookDB().processTxn(ledger, transaction, jvObj, jvObjApi2); pubAccountTransaction(ledger, transaction, last); } @@ -3296,28 +3319,55 @@ NetworkOPsImp::pubAccountTransaction( Json::Value jvObj = transJson(*stTxn, transaction.getResult(), true, ledger); + Json::Value jvObjApi2 = + transJson(*stTxn, transaction.getResult(), true, ledger); { auto const& meta = transaction.getMeta(); jvObj[jss::meta] = meta.getJson(JsonOptions::none); RPC::insertDeliveredAmount(jvObj[jss::meta], *ledger, stTxn, meta); + RPC::insertDeliverMax( + jvObj[jss::transaction], stTxn->getTxnType(), 1); + + jvObjApi2[jss::meta] = meta.getJson(JsonOptions::none); + RPC::insertDeliveredAmount( + jvObjApi2[jss::meta], *ledger, stTxn, meta); + RPC::insertDeliverMax( + jvObjApi2[jss::transaction], stTxn->getTxnType(), 2); } for (InfoSub::ref isrListener : notify) - isrListener->send(jvObj, true); + { + if (isrListener->getApiVersion() == 1) + isrListener->send(jvObj, true); + else + isrListener->send(jvObjApi2, true); + } if (last) + { jvObj[jss::account_history_boundary] = true; + jvObjApi2[jss::account_history_boundary] = true; + } assert(!jvObj.isMember(jss::account_history_tx_stream)); for (auto& info : accountHistoryNotify) { auto& index = info.index_; if (index->forwardTxIndex_ == 0 && !index->haveHistorical_) + { jvObj[jss::account_history_tx_first] = true; - jvObj[jss::account_history_tx_index] = index->forwardTxIndex_++; - info.sink_->send(jvObj, true); + jvObjApi2[jss::account_history_tx_first] = true; + } + jvObj[jss::account_history_tx_index] = index->forwardTxIndex_; + jvObjApi2[jss::account_history_tx_index] = index->forwardTxIndex_; + index->forwardTxIndex_ += 1; + + if (info.sink_->getApiVersion() == 1) + info.sink_->send(jvObj, true); + else + info.sink_->send(jvObjApi2, true); } } } @@ -3372,18 +3422,34 @@ NetworkOPsImp::pubProposedAccountTransaction( if (!notify.empty() || !accountHistoryNotify.empty()) { Json::Value jvObj = transJson(*tx, result, false, ledger); - + Json::Value jvObjApi2 = transJson(*tx, result, false, ledger); + RPC::insertDeliverMax(jvObj[jss::transaction], tx->getTxnType(), 1); + RPC::insertDeliverMax(jvObjApi2[jss::transaction], tx->getTxnType(), 2); for (InfoSub::ref isrListener : notify) - isrListener->send(jvObj, true); + { + if (isrListener->getApiVersion() == 1) + isrListener->send(jvObj, true); + else + isrListener->send(jvObjApi2, true); + } assert(!jvObj.isMember(jss::account_history_tx_stream)); for (auto& info : accountHistoryNotify) { auto& index = info.index_; if (index->forwardTxIndex_ == 0 && !index->haveHistorical_) + { jvObj[jss::account_history_tx_first] = true; - jvObj[jss::account_history_tx_index] = index->forwardTxIndex_++; - info.sink_->send(jvObj, true); + jvObjApi2[jss::account_history_tx_first] = true; + } + jvObj[jss::account_history_tx_index] = index->forwardTxIndex_; + jvObjApi2[jss::account_history_tx_index] = index->forwardTxIndex_; + index->forwardTxIndex_ += 1; + + if (info.sink_->getApiVersion() == 1) + info.sink_->send(jvObj, true); + else + info.sink_->send(jvObjApi2, true); } } } @@ -3585,6 +3651,24 @@ NetworkOPsImp::addAccountHistoryJob(SubAccountHistoryInfoWeak subInfo) return false; }; + auto send2 = [&](Json::Value const& jvObj, + Json::Value const& jvObjApi2, + bool unsubscribe) -> bool { + if (auto sptr = subInfo.sinkWptr_.lock(); sptr) + { + if (sptr->getApiVersion() == 1) + sptr->send(jvObj, true); + else + sptr->send(jvObjApi2, true); + + if (unsubscribe) + unsubAccountHistory(sptr, accountId, false); + return true; + } + + return false; + }; + auto getMoreTxns = [&](std::uint32_t minLedger, std::uint32_t maxLedger, @@ -3743,21 +3827,41 @@ NetworkOPsImp::addAccountHistoryJob(SubAccountHistoryInfoWeak subInfo) send(rpcError(rpcINTERNAL), true); return; } + Json::Value jvTx = transJson( *stTxn, meta->getResultTER(), true, curTxLedger); jvTx[jss::meta] = meta->getJson(JsonOptions::none); - jvTx[jss::account_history_tx_index] = txHistoryIndex--; + jvTx[jss::account_history_tx_index] = txHistoryIndex; + + Json::Value jvTxApi2 = transJson( + *stTxn, meta->getResultTER(), true, curTxLedger); + jvTxApi2[jss::meta] = meta->getJson(JsonOptions::none); + jvTxApi2[jss::account_history_tx_index] = + txHistoryIndex; + txHistoryIndex -= 1; if (i + 1 == num_txns || txns[i + 1].first->getLedger() != tx->getLedger()) + { jvTx[jss::account_history_boundary] = true; + jvTxApi2[jss::account_history_boundary] = true; + } RPC::insertDeliveredAmount( jvTx[jss::meta], *curTxLedger, stTxn, *meta); + RPC::insertDeliverMax( + jvTx[jss::transaction], stTxn->getTxnType(), 1); + + RPC::insertDeliveredAmount( + jvTxApi2[jss::meta], *curTxLedger, stTxn, *meta); + RPC::insertDeliverMax( + jvTxApi2[jss::transaction], stTxn->getTxnType(), 2); + if (isFirstTx(tx, meta)) { jvTx[jss::account_history_tx_first] = true; - send(jvTx, false); + jvTxApi2[jss::account_history_tx_first] = true; + send2(jvTx, jvTxApi2, false); JLOG(m_journal.trace()) << "AccountHistory job for account " @@ -3767,7 +3871,7 @@ NetworkOPsImp::addAccountHistoryJob(SubAccountHistoryInfoWeak subInfo) } else { - send(jvTx, false); + send2(jvTx, jvTxApi2, false); } } diff --git a/src/ripple/net/InfoSub.h b/src/ripple/net/InfoSub.h index fb44e23b720..60ab9faff9c 100644 --- a/src/ripple/net/InfoSub.h +++ b/src/ripple/net/InfoSub.h @@ -229,6 +229,12 @@ class InfoSub : public CountedObject std::shared_ptr const& getRequest(); + void + setApiVersion(int apiVersion); + + int + getApiVersion() const noexcept; + protected: std::mutex mLock; @@ -240,6 +246,7 @@ class InfoSub : public CountedObject std::shared_ptr request_; std::uint64_t mSeq; hash_set accountHistorySubscriptions_; + int apiVersion = 1; static int assign_id() diff --git a/src/ripple/net/impl/InfoSub.cpp b/src/ripple/net/impl/InfoSub.cpp index 9ea5962fa96..c53152ffcf9 100644 --- a/src/ripple/net/impl/InfoSub.cpp +++ b/src/ripple/net/impl/InfoSub.cpp @@ -136,4 +136,16 @@ InfoSub::getRequest() return request_; } +void +InfoSub::setApiVersion(int apiVersion) +{ + this->apiVersion = apiVersion; +} + +int +InfoSub::getApiVersion() const noexcept +{ + return this->apiVersion; +} + } // namespace ripple diff --git a/src/ripple/rpc/DeliverMax.h b/src/ripple/rpc/DeliverMax.h index 46a8425bf76..0798a170b89 100644 --- a/src/ripple/rpc/DeliverMax.h +++ b/src/ripple/rpc/DeliverMax.h @@ -54,10 +54,7 @@ struct Context; */ void -insertDeliverMax( - Json::Value& tx_json, - RPC::Context const& context, - TxType txnType); +insertDeliverMax(Json::Value& tx_json, TxType txnType, int apiVersion); /** @} */ diff --git a/src/ripple/rpc/handlers/AccountTx.cpp b/src/ripple/rpc/handlers/AccountTx.cpp index c7dfc5f8655..25048434a06 100644 --- a/src/ripple/rpc/handlers/AccountTx.cpp +++ b/src/ripple/rpc/handlers/AccountTx.cpp @@ -328,8 +328,8 @@ populateJsonResponse( jvObj[jss::tx] = txn->getJson(JsonOptions::include_date); auto const& sttx = txn->getSTransaction(); - insertDeliverMax( - jvObj[jss::tx], context, sttx->getTxnType()); + RPC::insertDeliverMax( + jvObj[jss::tx], sttx->getTxnType(), context.apiVersion); if (txnMeta) { jvObj[jss::meta] = diff --git a/src/ripple/rpc/handlers/PathFind.cpp b/src/ripple/rpc/handlers/PathFind.cpp index 9d6e0cff1ac..6c3a27302ac 100644 --- a/src/ripple/rpc/handlers/PathFind.cpp +++ b/src/ripple/rpc/handlers/PathFind.cpp @@ -46,6 +46,8 @@ doPathFind(RPC::JsonContext& context) if (!context.infoSub) return rpcError(rpcNO_EVENTS); + context.infoSub->setApiVersion(context.apiVersion); + auto sSubCommand = context.params[jss::subcommand].asString(); if (sSubCommand == "create") diff --git a/src/ripple/rpc/handlers/Subscribe.cpp b/src/ripple/rpc/handlers/Subscribe.cpp index f17aa62b626..e48cfe5049a 100644 --- a/src/ripple/rpc/handlers/Subscribe.cpp +++ b/src/ripple/rpc/handlers/Subscribe.cpp @@ -110,6 +110,7 @@ doSubscribe(RPC::JsonContext& context) { ispSub = context.infoSub; } + ispSub->setApiVersion(context.apiVersion); if (context.params.isMember(jss::streams)) { diff --git a/src/ripple/rpc/handlers/TransactionEntry.cpp b/src/ripple/rpc/handlers/TransactionEntry.cpp index de5141d479b..9a0f68b71a1 100644 --- a/src/ripple/rpc/handlers/TransactionEntry.cpp +++ b/src/ripple/rpc/handlers/TransactionEntry.cpp @@ -72,8 +72,8 @@ doTransactionEntry(RPC::JsonContext& context) else { jvResult[jss::tx_json] = sttx->getJson(JsonOptions::none); - insertDeliverMax( - jvResult[jss::tx_json], context, sttx->getTxnType()); + RPC::insertDeliverMax( + jvResult[jss::tx_json], sttx->getTxnType(), context.apiVersion); if (stobj) jvResult[jss::metadata] = stobj->getJson(JsonOptions::none); // 'accounts' diff --git a/src/ripple/rpc/handlers/Tx.cpp b/src/ripple/rpc/handlers/Tx.cpp index 0fed1e5ae01..e83c0378d23 100644 --- a/src/ripple/rpc/handlers/Tx.cpp +++ b/src/ripple/rpc/handlers/Tx.cpp @@ -314,7 +314,8 @@ populateJsonResponse( response = result.txn->getJson(JsonOptions::include_date, args.binary); auto const& sttx = result.txn->getSTransaction(); if (!args.binary) - insertDeliverMax(response, context, sttx->getTxnType()); + RPC::insertDeliverMax( + response, sttx->getTxnType(), context.apiVersion); // populate binary metadata if (auto blob = std::get_if(&result.meta)) diff --git a/src/ripple/rpc/handlers/TxHistory.cpp b/src/ripple/rpc/handlers/TxHistory.cpp index 5184550e617..d840a3df8ea 100644 --- a/src/ripple/rpc/handlers/TxHistory.cpp +++ b/src/ripple/rpc/handlers/TxHistory.cpp @@ -66,7 +66,8 @@ doTxHistory(RPC::JsonContext& context) for (auto const& t : trans) { Json::Value tx_json = t->getJson(JsonOptions::none); - insertDeliverMax(tx_json, context, t->getSTransaction()->getTxnType()); + RPC::insertDeliverMax( + tx_json, t->getSTransaction()->getTxnType(), context.apiVersion); txs.append(tx_json); } diff --git a/src/ripple/rpc/impl/DeliverMax.cpp b/src/ripple/rpc/impl/DeliverMax.cpp index f296b886b4b..f5b7f1c2f91 100644 --- a/src/ripple/rpc/impl/DeliverMax.cpp +++ b/src/ripple/rpc/impl/DeliverMax.cpp @@ -27,17 +27,14 @@ namespace ripple { namespace RPC { void -insertDeliverMax( - Json::Value& tx_json, - RPC::Context const& context, - TxType txnType) +insertDeliverMax(Json::Value& tx_json, TxType txnType, int apiVersion) { if (tx_json.isMember(jss::Amount)) { if (txnType == ttPAYMENT) { tx_json[jss::DeliverMax] = tx_json[jss::Amount]; - if (context.apiVersion > 1) + if (apiVersion > 1) tx_json.removeMember(jss::Amount); } } From c9f790798f4b83ff98518351b1d0c4fa607ef667 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki <823856+Bronek@users.noreply.github.com> Date: Thu, 28 Sep 2023 20:21:17 +0100 Subject: [PATCH 03/21] Factor out MultivarJson for transaction streams --- src/ripple/app/ledger/BookListeners.cpp | 9 +- src/ripple/app/ledger/BookListeners.h | 4 +- src/ripple/app/ledger/OrderBookDB.cpp | 5 +- src/ripple/app/ledger/OrderBookDB.h | 5 +- src/ripple/app/misc/NetworkOPs.cpp | 188 ++++++++++-------------- src/ripple/basics/MultivarJson.h | 88 +++++++++++ 6 files changed, 176 insertions(+), 123 deletions(-) create mode 100644 src/ripple/basics/MultivarJson.h diff --git a/src/ripple/app/ledger/BookListeners.cpp b/src/ripple/app/ledger/BookListeners.cpp index 379e2e32adb..bbc0058bc76 100644 --- a/src/ripple/app/ledger/BookListeners.cpp +++ b/src/ripple/app/ledger/BookListeners.cpp @@ -39,8 +39,7 @@ BookListeners::removeSubscriber(std::uint64_t seq) void BookListeners::publish( - Json::Value const& jvObj, - Json::Value const& jvObjApiVer2, + MultiApiJson const& jvObj, hash_set& havePublished) { std::lock_guard sl(mLock); @@ -55,10 +54,8 @@ BookListeners::publish( // Only publish jvObj if this is the first occurence if (havePublished.emplace(p->getSeq()).second) { - if (p->getApiVersion() == 1) - p->send(jvObj, true); - else - p->send(jvObjApiVer2, true); + p->send( + jvObj.select(apiVersionSelector(p->getApiVersion())), true); } ++it; } diff --git a/src/ripple/app/ledger/BookListeners.h b/src/ripple/app/ledger/BookListeners.h index 6e0fed48c07..080e15716a6 100644 --- a/src/ripple/app/ledger/BookListeners.h +++ b/src/ripple/app/ledger/BookListeners.h @@ -20,7 +20,9 @@ #ifndef RIPPLE_APP_LEDGER_BOOKLISTENERS_H_INCLUDED #define RIPPLE_APP_LEDGER_BOOKLISTENERS_H_INCLUDED +#include #include + #include #include @@ -58,7 +60,7 @@ class BookListeners */ void - publish(Json::Value const& jvObj, Json::Value const& jvObjApiVer2, hash_set& havePublished); + publish(MultiApiJson const& jvObj, hash_set& havePublished); private: std::recursive_mutex mLock; diff --git a/src/ripple/app/ledger/OrderBookDB.cpp b/src/ripple/app/ledger/OrderBookDB.cpp index 163321f8ed9..faa957203a8 100644 --- a/src/ripple/app/ledger/OrderBookDB.cpp +++ b/src/ripple/app/ledger/OrderBookDB.cpp @@ -250,8 +250,7 @@ void OrderBookDB::processTxn( std::shared_ptr const& ledger, const AcceptedLedgerTx& alTx, - Json::Value const& jvObj, - Json::Value const& jvObjApiVer2) + MultiApiJson const& jvObj) { std::lock_guard sl(mLock); @@ -278,7 +277,7 @@ OrderBookDB::processTxn( {data->getFieldAmount(sfTakerGets).issue(), data->getFieldAmount(sfTakerPays).issue()}); if (listeners) - listeners->publish(jvObj, jvObjApiVer2, havePublished); + listeners->publish(jvObj, havePublished); } }; diff --git a/src/ripple/app/ledger/OrderBookDB.h b/src/ripple/app/ledger/OrderBookDB.h index 8fd5945072b..9b41109e5c6 100644 --- a/src/ripple/app/ledger/OrderBookDB.h +++ b/src/ripple/app/ledger/OrderBookDB.h @@ -23,6 +23,8 @@ #include #include #include +#include + #include namespace ripple { @@ -63,8 +65,7 @@ class OrderBookDB processTxn( std::shared_ptr const& ledger, const AcceptedLedgerTx& alTx, - Json::Value const& jvObj, - Json::Value const& jvObjApiVer2); + MultiApiJson const& jvObj); private: Application& app_; diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index b70b2bd00e9..25021858019 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -41,6 +41,7 @@ #include #include #include +#include #include #include #include @@ -2745,12 +2746,12 @@ NetworkOPsImp::pubProposedTransaction( std::shared_ptr const& transaction, TER result) { - Json::Value jvObj = transJson(*transaction, result, false, ledger); - Json::Value jvObjApi2 = transJson(*transaction, result, false, ledger); + Json::Value jvSeed = transJson(*transaction, result, false, ledger); + MultiApiJson jvObj = MultiApiJson::fill(jvSeed); RPC::insertDeliverMax( - jvObj[jss::transaction], transaction->getTxnType(), 1); + jvObj[0][jss::transaction], transaction->getTxnType(), 1); RPC::insertDeliverMax( - jvObjApi2[1][jss::transaction], transaction->getTxnType(), 2); + jvObj[1][jss::transaction], transaction->getTxnType(), 2); { std::lock_guard sl(mSubLock); @@ -2762,10 +2763,8 @@ NetworkOPsImp::pubProposedTransaction( if (p) { - if (p->getApiVersion() == 1) - p->send(jvObj, true); - else - p->send(jvObjApi2, true); + p->send( + jvObj.select(apiVersionSelector(p->getApiVersion())), true); ++it; } else @@ -3156,23 +3155,20 @@ NetworkOPsImp::pubValidatedTransaction( { auto const& stTxn = transaction.getTxn(); - Json::Value jvObj = - transJson(*stTxn, transaction.getResult(), true, ledger); - Json::Value jvObjApi2 = + Json::Value jvSeed = transJson(*stTxn, transaction.getResult(), true, ledger); { auto const& meta = transaction.getMeta(); - jvObj[jss::meta] = meta.getJson(JsonOptions::none); - RPC::insertDeliveredAmount(jvObj[jss::meta], *ledger, stTxn, meta); - RPC::insertDeliverMax(jvObj[jss::transaction], stTxn->getTxnType(), 1); - - jvObjApi2[jss::meta] = meta.getJson(JsonOptions::none); - RPC::insertDeliveredAmount(jvObjApi2[jss::meta], *ledger, stTxn, meta); - RPC::insertDeliverMax( - jvObjApi2[jss::transaction], stTxn->getTxnType(), 2); + jvSeed[jss::meta] = meta.getJson(JsonOptions::none); + RPC::insertDeliveredAmount(jvSeed[jss::meta], *ledger, stTxn, meta); } + // Create two different Json objects, for different API versions + MultiApiJson jvObj = MultiApiJson::fill(jvSeed); + RPC::insertDeliverMax(jvObj[0][jss::transaction], stTxn->getTxnType(), 1); + RPC::insertDeliverMax(jvObj[1][jss::transaction], stTxn->getTxnType(), 2); + { std::lock_guard sl(mSubLock); @@ -3183,10 +3179,8 @@ NetworkOPsImp::pubValidatedTransaction( if (p) { - if (p->getApiVersion() == 1) - p->send(jvObj, true); - else - p->send(jvObjApi2, true); + p->send( + jvObj.select(apiVersionSelector(p->getApiVersion())), true); ++it; } else @@ -3201,10 +3195,8 @@ NetworkOPsImp::pubValidatedTransaction( if (p) { - if (p->getApiVersion() == 1) - p->send(jvObj, true); - else - p->send(jvObjApi2, true); + p->send( + jvObj.select(apiVersionSelector(p->getApiVersion())), true); ++it; } else @@ -3213,7 +3205,7 @@ NetworkOPsImp::pubValidatedTransaction( } if (transaction.getResult() == tesSUCCESS) - app_.getOrderBookDB().processTxn(ledger, transaction, jvObj, jvObjApi2); + app_.getOrderBookDB().processTxn(ledger, transaction, jvObj); pubAccountTransaction(ledger, transaction, last); } @@ -3317,57 +3309,46 @@ NetworkOPsImp::pubAccountTransaction( { auto const& stTxn = transaction.getTxn(); - Json::Value jvObj = - transJson(*stTxn, transaction.getResult(), true, ledger); - Json::Value jvObjApi2 = + Json::Value jvSeed = transJson(*stTxn, transaction.getResult(), true, ledger); { auto const& meta = transaction.getMeta(); - jvObj[jss::meta] = meta.getJson(JsonOptions::none); - RPC::insertDeliveredAmount(jvObj[jss::meta], *ledger, stTxn, meta); - RPC::insertDeliverMax( - jvObj[jss::transaction], stTxn->getTxnType(), 1); - - jvObjApi2[jss::meta] = meta.getJson(JsonOptions::none); - RPC::insertDeliveredAmount( - jvObjApi2[jss::meta], *ledger, stTxn, meta); - RPC::insertDeliverMax( - jvObjApi2[jss::transaction], stTxn->getTxnType(), 2); + jvSeed[jss::meta] = meta.getJson(JsonOptions::none); + RPC::insertDeliveredAmount(jvSeed[jss::meta], *ledger, stTxn, meta); } + // Create two different Json objects, for different API versions + MultiApiJson jvObj = MultiApiJson::fill(jvSeed); + RPC::insertDeliverMax( + jvObj[0][jss::transaction], stTxn->getTxnType(), 1); + RPC::insertDeliverMax( + jvObj[1][jss::transaction], stTxn->getTxnType(), 2); + for (InfoSub::ref isrListener : notify) { - if (isrListener->getApiVersion() == 1) - isrListener->send(jvObj, true); - else - isrListener->send(jvObjApi2, true); + isrListener->send( + jvObj.select(apiVersionSelector(isrListener->getApiVersion())), + true); } if (last) - { - jvObj[jss::account_history_boundary] = true; - jvObjApi2[jss::account_history_boundary] = true; - } + jvObj.set(jss::account_history_boundary, true); - assert(!jvObj.isMember(jss::account_history_tx_stream)); + assert(!jvObj[0].isMember(jss::account_history_tx_stream)); for (auto& info : accountHistoryNotify) { auto& index = info.index_; if (index->forwardTxIndex_ == 0 && !index->haveHistorical_) - { - jvObj[jss::account_history_tx_first] = true; - jvObjApi2[jss::account_history_tx_first] = true; - } - jvObj[jss::account_history_tx_index] = index->forwardTxIndex_; - jvObjApi2[jss::account_history_tx_index] = index->forwardTxIndex_; + jvObj.set(jss::account_history_tx_first, true); + + jvObj.set(jss::account_history_tx_index, index->forwardTxIndex_); index->forwardTxIndex_ += 1; - if (info.sink_->getApiVersion() == 1) - info.sink_->send(jvObj, true); - else - info.sink_->send(jvObjApi2, true); + info.sink_->send( + jvObj.select(apiVersionSelector(info.sink_->getApiVersion())), + true); } } } @@ -3421,35 +3402,28 @@ NetworkOPsImp::pubProposedAccountTransaction( if (!notify.empty() || !accountHistoryNotify.empty()) { - Json::Value jvObj = transJson(*tx, result, false, ledger); - Json::Value jvObjApi2 = transJson(*tx, result, false, ledger); - RPC::insertDeliverMax(jvObj[jss::transaction], tx->getTxnType(), 1); - RPC::insertDeliverMax(jvObjApi2[jss::transaction], tx->getTxnType(), 2); + Json::Value jvSeed = transJson(*tx, result, false, ledger); + + // Create two different Json objects, for different API versions + MultiApiJson jvObj = MultiApiJson::fill(jvSeed); + RPC::insertDeliverMax(jvObj[0][jss::transaction], tx->getTxnType(), 1); + RPC::insertDeliverMax(jvObj[1][jss::transaction], tx->getTxnType(), 2); for (InfoSub::ref isrListener : notify) - { - if (isrListener->getApiVersion() == 1) - isrListener->send(jvObj, true); - else - isrListener->send(jvObjApi2, true); - } + isrListener->send( + jvObj.select(apiVersionSelector(isrListener->getApiVersion())), + true); assert(!jvObj.isMember(jss::account_history_tx_stream)); for (auto& info : accountHistoryNotify) { auto& index = info.index_; if (index->forwardTxIndex_ == 0 && !index->haveHistorical_) - { - jvObj[jss::account_history_tx_first] = true; - jvObjApi2[jss::account_history_tx_first] = true; - } - jvObj[jss::account_history_tx_index] = index->forwardTxIndex_; - jvObjApi2[jss::account_history_tx_index] = index->forwardTxIndex_; + jvObj.set(jss::account_history_tx_first, true); + jvObj.set(jss::account_history_tx_index, index->forwardTxIndex_); index->forwardTxIndex_ += 1; - - if (info.sink_->getApiVersion() == 1) - info.sink_->send(jvObj, true); - else - info.sink_->send(jvObjApi2, true); + info.sink_->send( + jvObj.select(apiVersionSelector(info.sink_->getApiVersion())), + true); } } } @@ -3651,15 +3625,13 @@ NetworkOPsImp::addAccountHistoryJob(SubAccountHistoryInfoWeak subInfo) return false; }; - auto send2 = [&](Json::Value const& jvObj, - Json::Value const& jvObjApi2, - bool unsubscribe) -> bool { + auto sendMultiApiJson = [&](MultiApiJson const& jvObj, + bool unsubscribe) -> bool { if (auto sptr = subInfo.sinkWptr_.lock(); sptr) { - if (sptr->getApiVersion() == 1) - sptr->send(jvObj, true); - else - sptr->send(jvObjApi2, true); + sptr->send( + jvObj.select(apiVersionSelector(sptr->getApiVersion())), + true); if (unsubscribe) unsubAccountHistory(sptr, accountId, false); @@ -3828,40 +3800,34 @@ NetworkOPsImp::addAccountHistoryJob(SubAccountHistoryInfoWeak subInfo) return; } - Json::Value jvTx = transJson( + Json::Value jvSeed = transJson( *stTxn, meta->getResultTER(), true, curTxLedger); - jvTx[jss::meta] = meta->getJson(JsonOptions::none); - jvTx[jss::account_history_tx_index] = txHistoryIndex; - - Json::Value jvTxApi2 = transJson( - *stTxn, meta->getResultTER(), true, curTxLedger); - jvTxApi2[jss::meta] = meta->getJson(JsonOptions::none); - jvTxApi2[jss::account_history_tx_index] = - txHistoryIndex; + jvSeed[jss::meta] = meta->getJson(JsonOptions::none); + jvSeed[jss::account_history_tx_index] = txHistoryIndex; txHistoryIndex -= 1; if (i + 1 == num_txns || txns[i + 1].first->getLedger() != tx->getLedger()) - { - jvTx[jss::account_history_boundary] = true; - jvTxApi2[jss::account_history_boundary] = true; - } + jvSeed[jss::account_history_boundary] = true; RPC::insertDeliveredAmount( - jvTx[jss::meta], *curTxLedger, stTxn, *meta); - RPC::insertDeliverMax( - jvTx[jss::transaction], stTxn->getTxnType(), 1); + jvSeed[jss::meta], *curTxLedger, stTxn, *meta); - RPC::insertDeliveredAmount( - jvTxApi2[jss::meta], *curTxLedger, stTxn, *meta); + // Create two Json objects, for different API versions + MultiApiJson jvTx = MultiApiJson::fill(jvSeed); + RPC::insertDeliverMax( + jvTx.val[0][jss::transaction], + stTxn->getTxnType(), + 1); RPC::insertDeliverMax( - jvTxApi2[jss::transaction], stTxn->getTxnType(), 2); + jvTx.val[1][jss::transaction], + stTxn->getTxnType(), + 2); if (isFirstTx(tx, meta)) { - jvTx[jss::account_history_tx_first] = true; - jvTxApi2[jss::account_history_tx_first] = true; - send2(jvTx, jvTxApi2, false); + jvTx.set(jss::account_history_tx_first, true); + sendMultiApiJson(jvTx, false); JLOG(m_journal.trace()) << "AccountHistory job for account " @@ -3871,7 +3837,7 @@ NetworkOPsImp::addAccountHistoryJob(SubAccountHistoryInfoWeak subInfo) } else { - send2(jvTx, jvTxApi2, false); + sendMultiApiJson(jvTx, false); } } diff --git a/src/ripple/basics/MultivarJson.h b/src/ripple/basics/MultivarJson.h new file mode 100644 index 00000000000..f328e2bc78f --- /dev/null +++ b/src/ripple/basics/MultivarJson.h @@ -0,0 +1,88 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2023 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#ifndef RIPPLE_BASICS_MULTIVARJSON_H_INCLUDED +#define RIPPLE_BASICS_MULTIVARJSON_H_INCLUDED + +#include + +#include +#include +#include + +namespace ripple { +template +struct MultivarJson +{ + std::array val; + + Json::Value& + operator[](std::size_t i) + { + return val[i]; + } + + Json::Value const& + operator[](std::size_t i) const + { + return val[i]; + } + + Json::Value const& + select(auto selector) const + { + static_assert(std::is_same_v); + return val[selector()]; + } + + static MultivarJson + fill(Json::Value const& v) + { + MultivarJson ret; + for (auto& a : ret.val) + a = v; + return ret; + } + + void + set(const char* key, auto const& v) + { + for (auto& a : this->val) + a[key] = v; + } +}; + +// Wrapper for Json for all supported API versions. +using MultiApiJson = MultivarJson<2>; + +// Helper to create appropriate selector for indexing MultiApiJson by apiVersion +constexpr auto +apiVersionSelector(int apiVersion) noexcept +{ + return [apiVersion]() constexpr + { + // apiVersion <= 1 returns 0 + // apiVersion > 1 returns 1 + return static_cast(apiVersion > 1); + }; +} + +} // namespace ripple + +#endif From 775731bb1f7bd3890291b96d2a4f8cba2a0ff995 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki <823856+Bronek@users.noreply.github.com> Date: Thu, 5 Oct 2023 18:21:31 +0100 Subject: [PATCH 04/21] Refactor transJson to remove repeated code --- src/ripple/app/misc/NetworkOPs.cpp | 119 +++++++++++++---------------- src/ripple/basics/MultivarJson.h | 21 ----- 2 files changed, 52 insertions(+), 88 deletions(-) diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 25021858019..4a248614049 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -601,10 +601,12 @@ class NetworkOPsImp final : public NetworkOPs Json::Value transJson( - const STTx& transaction, + std::shared_ptr const& transaction, TER result, + std::shared_ptr const& ledger, bool validated, - std::shared_ptr const& ledger); + std::optional> meta, + int apiVersion); void pubValidatedTransaction( @@ -2746,12 +2748,9 @@ NetworkOPsImp::pubProposedTransaction( std::shared_ptr const& transaction, TER result) { - Json::Value jvSeed = transJson(*transaction, result, false, ledger); - MultiApiJson jvObj = MultiApiJson::fill(jvSeed); - RPC::insertDeliverMax( - jvObj[0][jss::transaction], transaction->getTxnType(), 1); - RPC::insertDeliverMax( - jvObj[1][jss::transaction], transaction->getTxnType(), 2); + MultiApiJson jvObj( + {transJson(transaction, result, ledger, false, std::nullopt, 1), + transJson(transaction, result, ledger, false, std::nullopt, 2)}); { std::lock_guard sl(mSubLock); @@ -3091,10 +3090,12 @@ NetworkOPsImp::getLocalTxCount() // transactions. Json::Value NetworkOPsImp::transJson( - const STTx& transaction, + std::shared_ptr const& transaction, TER result, + std::shared_ptr const& ledger, bool validated, - std::shared_ptr const& ledger) + std::optional> meta, + int apiVersion) { Json::Value jvObj(Json::objectValue); std::string sToken; @@ -3103,7 +3104,16 @@ NetworkOPsImp::transJson( transResultInfo(result, sToken, sHuman); jvObj[jss::type] = "transaction"; - jvObj[jss::transaction] = transaction.getJson(JsonOptions::none); + jvObj[jss::transaction] = transaction->getJson(JsonOptions::none); + RPC::insertDeliverMax( + jvObj[jss::transaction], transaction->getTxnType(), apiVersion); + + if (meta) + { + jvObj[jss::meta] = meta->get().getJson(JsonOptions::none); + RPC::insertDeliveredAmount( + jvObj[jss::meta], *ledger, transaction, meta->get()); + } if (validated) { @@ -3126,10 +3136,10 @@ NetworkOPsImp::transJson( jvObj[jss::engine_result_code] = result; jvObj[jss::engine_result_message] = sHuman; - if (transaction.getTxnType() == ttOFFER_CREATE) + if (transaction->getTxnType() == ttOFFER_CREATE) { - auto const account = transaction.getAccountID(sfAccount); - auto const amount = transaction.getFieldAmount(sfTakerGets); + auto const account = transaction->getAccountID(sfAccount); + auto const amount = transaction->getFieldAmount(sfTakerGets); // If the offer create is not self funded then add the owner balance if (account != amount.issue().account) @@ -3155,19 +3165,12 @@ NetworkOPsImp::pubValidatedTransaction( { auto const& stTxn = transaction.getTxn(); - Json::Value jvSeed = - transJson(*stTxn, transaction.getResult(), true, ledger); - - { - auto const& meta = transaction.getMeta(); - jvSeed[jss::meta] = meta.getJson(JsonOptions::none); - RPC::insertDeliveredAmount(jvSeed[jss::meta], *ledger, stTxn, meta); - } - // Create two different Json objects, for different API versions - MultiApiJson jvObj = MultiApiJson::fill(jvSeed); - RPC::insertDeliverMax(jvObj[0][jss::transaction], stTxn->getTxnType(), 1); - RPC::insertDeliverMax(jvObj[1][jss::transaction], stTxn->getTxnType(), 2); + auto const metaRef = std::ref(transaction.getMeta()); + auto const trResult = transaction.getResult(); + MultiApiJson jvObj( + {transJson(stTxn, trResult, ledger, true, metaRef, 1), + transJson(stTxn, trResult, ledger, true, metaRef, 2)}); { std::lock_guard sl(mSubLock); @@ -3309,22 +3312,12 @@ NetworkOPsImp::pubAccountTransaction( { auto const& stTxn = transaction.getTxn(); - Json::Value jvSeed = - transJson(*stTxn, transaction.getResult(), true, ledger); - - { - auto const& meta = transaction.getMeta(); - - jvSeed[jss::meta] = meta.getJson(JsonOptions::none); - RPC::insertDeliveredAmount(jvSeed[jss::meta], *ledger, stTxn, meta); - } - // Create two different Json objects, for different API versions - MultiApiJson jvObj = MultiApiJson::fill(jvSeed); - RPC::insertDeliverMax( - jvObj[0][jss::transaction], stTxn->getTxnType(), 1); - RPC::insertDeliverMax( - jvObj[1][jss::transaction], stTxn->getTxnType(), 2); + auto const metaRef = std::ref(transaction.getMeta()); + auto const trResult = transaction.getResult(); + MultiApiJson jvObj( + {transJson(stTxn, trResult, ledger, true, metaRef, 1), + transJson(stTxn, trResult, ledger, true, metaRef, 2)}); for (InfoSub::ref isrListener : notify) { @@ -3336,7 +3329,8 @@ NetworkOPsImp::pubAccountTransaction( if (last) jvObj.set(jss::account_history_boundary, true); - assert(!jvObj[0].isMember(jss::account_history_tx_stream)); + assert(!jvObj.val[0].isMember(jss::account_history_tx_stream)); + assert(!jvObj.val[1].isMember(jss::account_history_tx_stream)); for (auto& info : accountHistoryNotify) { auto& index = info.index_; @@ -3402,18 +3396,19 @@ NetworkOPsImp::pubProposedAccountTransaction( if (!notify.empty() || !accountHistoryNotify.empty()) { - Json::Value jvSeed = transJson(*tx, result, false, ledger); - // Create two different Json objects, for different API versions - MultiApiJson jvObj = MultiApiJson::fill(jvSeed); - RPC::insertDeliverMax(jvObj[0][jss::transaction], tx->getTxnType(), 1); - RPC::insertDeliverMax(jvObj[1][jss::transaction], tx->getTxnType(), 2); + MultiApiJson jvObj({ + transJson(tx, result, ledger, false, std::nullopt, 1), + transJson(tx, result, ledger, false, std::nullopt, 2), + }); + for (InfoSub::ref isrListener : notify) isrListener->send( jvObj.select(apiVersionSelector(isrListener->getApiVersion())), true); - assert(!jvObj.isMember(jss::account_history_tx_stream)); + assert(!jvObj.val[0].isMember(jss::account_history_tx_stream)); + assert(!jvObj.val[1].isMember(jss::account_history_tx_stream)); for (auto& info : accountHistoryNotify) { auto& index = info.index_; @@ -3800,29 +3795,19 @@ NetworkOPsImp::addAccountHistoryJob(SubAccountHistoryInfoWeak subInfo) return; } - Json::Value jvSeed = transJson( - *stTxn, meta->getResultTER(), true, curTxLedger); - jvSeed[jss::meta] = meta->getJson(JsonOptions::none); - jvSeed[jss::account_history_tx_index] = txHistoryIndex; + auto const mRef = std::ref(*meta); + auto const trR = meta->getResultTER(); + MultiApiJson jvTx( + {transJson(stTxn, trR, curTxLedger, true, mRef, 1), + transJson( + stTxn, trR, curTxLedger, true, mRef, 2)}); + + jvTx.set(jss::account_history_tx_index, txHistoryIndex); txHistoryIndex -= 1; if (i + 1 == num_txns || txns[i + 1].first->getLedger() != tx->getLedger()) - jvSeed[jss::account_history_boundary] = true; - - RPC::insertDeliveredAmount( - jvSeed[jss::meta], *curTxLedger, stTxn, *meta); - - // Create two Json objects, for different API versions - MultiApiJson jvTx = MultiApiJson::fill(jvSeed); - RPC::insertDeliverMax( - jvTx.val[0][jss::transaction], - stTxn->getTxnType(), - 1); - RPC::insertDeliverMax( - jvTx.val[1][jss::transaction], - stTxn->getTxnType(), - 2); + jvTx.set(jss::account_history_boundary, true); if (isFirstTx(tx, meta)) { diff --git a/src/ripple/basics/MultivarJson.h b/src/ripple/basics/MultivarJson.h index f328e2bc78f..a631768c2f4 100644 --- a/src/ripple/basics/MultivarJson.h +++ b/src/ripple/basics/MultivarJson.h @@ -32,18 +32,6 @@ struct MultivarJson { std::array val; - Json::Value& - operator[](std::size_t i) - { - return val[i]; - } - - Json::Value const& - operator[](std::size_t i) const - { - return val[i]; - } - Json::Value const& select(auto selector) const { @@ -51,15 +39,6 @@ struct MultivarJson return val[selector()]; } - static MultivarJson - fill(Json::Value const& v) - { - MultivarJson ret; - for (auto& a : ret.val) - a = v; - return ret; - } - void set(const char* key, auto const& v) { From 29593086ce086a0b594858e47322289a968cf8c5 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki <823856+Bronek@users.noreply.github.com> Date: Fri, 6 Oct 2023 14:53:47 +0100 Subject: [PATCH 05/21] Add unit test for MultivarJson --- Builds/CMake/RippledCore.cmake | 1 + src/ripple/basics/MultivarJson.h | 15 +- src/test/basics/MultivarJson_test.cpp | 241 ++++++++++++++++++++++++++ 3 files changed, 251 insertions(+), 6 deletions(-) create mode 100644 src/test/basics/MultivarJson_test.cpp diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index 72025f4cfbb..dc659a2f3a5 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -855,6 +855,7 @@ if (tests) src/test/basics/join_test.cpp src/test/basics/mulDiv_test.cpp src/test/basics/tagged_integer_test.cpp + src/test/basics/MultivarJson_test.cpp #[===============================[ test sources: subdir: beast diff --git a/src/ripple/basics/MultivarJson.h b/src/ripple/basics/MultivarJson.h index a631768c2f4..81aa09ffa48 100644 --- a/src/ripple/basics/MultivarJson.h +++ b/src/ripple/basics/MultivarJson.h @@ -23,24 +23,27 @@ #include #include +#include #include -#include namespace ripple { -template +template struct MultivarJson { - std::array val; + std::array val; + constexpr static std::size_t size = Size; Json::Value const& - select(auto selector) const + select(auto&& selector) const + requires std::same_as { - static_assert(std::is_same_v); return val[selector()]; } void - set(const char* key, auto const& v) + set(const char* key, + auto const& + v) requires std::constructible_from { for (auto& a : this->val) a[key] = v; diff --git a/src/test/basics/MultivarJson_test.cpp b/src/test/basics/MultivarJson_test.cpp new file mode 100644 index 00000000000..cca478f4e8e --- /dev/null +++ b/src/test/basics/MultivarJson_test.cpp @@ -0,0 +1,241 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github0.com/ripple/rippled + Copyright (c) 2023 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include +#include "ripple/beast/unit_test/suite.hpp" +#include "ripple/json/json_value.h" +#include +#include +#include +#include + +namespace ripple { +namespace test { + +struct MultivarJson_test : beast::unit_test::suite +{ + void + run() override + { + constexpr static Json::StaticString string1("string1"); + static Json::Value const str1{string1}; + + static Json::Value const obj1{[]() { + Json::Value obj1; + obj1["one"] = 1; + return obj1; + }()}; + + static Json::Value const jsonNull{}; + + MultivarJson<3> const subject({str1, obj1}); + static_assert(sizeof(subject) == sizeof(subject.val)); + static_assert(subject.size == subject.val.size()); + static_assert( + std::is_same_v>); + + BEAST_EXPECT(subject.val.size() == 3); + BEAST_EXPECT( + (subject.val == std::array{str1, obj1, jsonNull})); + BEAST_EXPECT( + (MultivarJson<3>({obj1, str1}).val == + std::array{obj1, str1, jsonNull})); + BEAST_EXPECT( + (MultivarJson<3>({jsonNull, obj1, str1}).val == + std::array{jsonNull, obj1, str1})); + + { + testcase("default copy construction / assignment"); + + MultivarJson<3> x{subject}; + + BEAST_EXPECT(x.val.size() == subject.val.size()); + BEAST_EXPECT(x.val[0] == subject.val[0]); + BEAST_EXPECT(x.val[1] == subject.val[1]); + BEAST_EXPECT(x.val[2] == subject.val[2]); + BEAST_EXPECT(x.val == subject.val); + BEAST_EXPECT(&x.val[0] != &subject.val[0]); + BEAST_EXPECT(&x.val[1] != &subject.val[1]); + BEAST_EXPECT(&x.val[2] != &subject.val[2]); + + MultivarJson<3> y; + BEAST_EXPECT((y.val == std::array{})); + y = subject; + BEAST_EXPECT(y.val == subject.val); + BEAST_EXPECT(&y.val[0] != &subject.val[0]); + BEAST_EXPECT(&y.val[1] != &subject.val[1]); + BEAST_EXPECT(&y.val[2] != &subject.val[2]); + + y = std::move(x); + BEAST_EXPECT(y.val == subject.val); + BEAST_EXPECT(&y.val[0] != &subject.val[0]); + BEAST_EXPECT(&y.val[1] != &subject.val[1]); + BEAST_EXPECT(&y.val[2] != &subject.val[2]); + } + + { + testcase("select"); + + BEAST_EXPECT( + subject.select([]() -> std::size_t { return 0; }) == str1); + BEAST_EXPECT( + subject.select([]() -> std::size_t { return 1; }) == obj1); + BEAST_EXPECT( + subject.select([]() -> std::size_t { return 2; }) == jsonNull); + + // Tests of requires clause - these are expected to match + static_assert([](auto&& v) { + return requires + { + v.select([]() -> std::size_t {}); + }; + }(subject)); + static_assert([](auto&& v) { + return requires + { + v.select([]() constexpr->std::size_t { return 0; }); + }; + }(subject)); + static_assert([](auto&& v) { + return requires + { + v.select([]() mutable -> std::size_t {}); + }; + }(subject)); + + // Tests of requires clause - these are expected NOT to match + static_assert([](auto&& a) { + return !requires + { + subject.select([]() -> int { return 0; }); + }; + }(subject)); + static_assert([](auto&& v) { + return !requires + { + v.select([]() -> void {}); + }; + }(subject)); + static_assert([](auto&& v) { + return !requires + { + v.select([]() -> bool {}); + }; + }(subject)); + } + + { + struct foo_t final + { + }; + testcase("set"); + + auto x = MultivarJson<2>{{Json::objectValue, Json::objectValue}}; + x.set("name1", 42); + BEAST_EXPECT(x.val[0].isMember("name1")); + BEAST_EXPECT(x.val[1].isMember("name1")); + BEAST_EXPECT(x.val[0]["name1"].isInt()); + BEAST_EXPECT(x.val[1]["name1"].isInt()); + BEAST_EXPECT(x.val[0]["name1"].asInt() == 42); + BEAST_EXPECT(x.val[1]["name1"].asInt() == 42); + + x.set("name2", "bar"); + BEAST_EXPECT(x.val[0].isMember("name2")); + BEAST_EXPECT(x.val[1].isMember("name2")); + BEAST_EXPECT(x.val[0]["name2"].isString()); + BEAST_EXPECT(x.val[1]["name2"].isString()); + BEAST_EXPECT(x.val[0]["name2"].asString() == "bar"); + BEAST_EXPECT(x.val[1]["name2"].asString() == "bar"); + + // Tests of requires clause - these are expected to match + static_assert([](auto&& v) { + return requires + { + v.set("name", Json::nullValue); + }; + }(x)); + static_assert([](auto&& v) { + return requires + { + v.set("name", "value"); + }; + }(x)); + static_assert([](auto&& v) { + return requires + { + v.set("name", true); + }; + }(x)); + static_assert([](auto&& v) { + return requires + { + v.set("name", 42); + }; + }(x)); + + // Tests of requires clause - these are expected NOT to match + static_assert([](auto&& v) { + return !requires + { + v.set("name", foo_t{}); + }; + }(x)); + static_assert([](auto&& v) { + return !requires + { + v.set("name", std::nullopt); + }; + }(x)); + } + + { + testcase("apiVersionSelector"); + + static_assert(MultiApiJson::size == 2); + static MultiApiJson x{{obj1, str1}}; + + static_assert( + std::is_same_v); + static_assert([](auto&& v) { + return requires + { + v.select(apiVersionSelector(1)); + }; + }(x)); + + BEAST_EXPECT(x.select(apiVersionSelector(0)) == obj1); + BEAST_EXPECT(x.select(apiVersionSelector(2)) == str1); + + static_assert( + apiVersionSelector(std::numeric_limits::min())() == 0); + static_assert(apiVersionSelector(0)() == 0); + static_assert(apiVersionSelector(1)() == 0); + static_assert(apiVersionSelector(2)() == 1); + static_assert(apiVersionSelector(3)() == 1); + static_assert( + apiVersionSelector(std::numeric_limits::max())() == 1); + } + } +}; + +BEAST_DEFINE_TESTSUITE(MultivarJson, ripple_basics, ripple); + +} // namespace test +} // namespace ripple From ef6cbc0b9ff1a05b8b709af9df386c502eaafe47 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki <823856+Bronek@users.noreply.github.com> Date: Fri, 6 Oct 2023 19:23:14 +0100 Subject: [PATCH 06/21] Add test for TransactionSign.cpp --- src/ripple/rpc/impl/TransactionSign.cpp | 2 +- src/test/rpc/JSONRPC_test.cpp | 92 +++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-) diff --git a/src/ripple/rpc/impl/TransactionSign.cpp b/src/ripple/rpc/impl/TransactionSign.cpp index f4509bf7afd..45d4026454d 100644 --- a/src/ripple/rpc/impl/TransactionSign.cpp +++ b/src/ripple/rpc/impl/TransactionSign.cpp @@ -173,7 +173,7 @@ checkPayment( if (tx_json.isMember(jss::Amount)) return RPC::make_error( rpcINVALID_PARAMS, - "Cannot specify both 'Amount' and 'DeliverMax`"); + "Cannot specify both 'Amount' and 'DeliverMax'"); tx_json[jss::Amount] = tx_json[jss::DeliverMax]; tx_json.removeMember(jss::DeliverMax); diff --git a/src/test/rpc/JSONRPC_test.cpp b/src/test/rpc/JSONRPC_test.cpp index b6e54967c40..cf0872ca033 100644 --- a/src/test/rpc/JSONRPC_test.cpp +++ b/src/test/rpc/JSONRPC_test.cpp @@ -1035,6 +1035,26 @@ static constexpr TxnTestData txnTestArray[] = { "Missing field 'tx_json.Destination'.", "Missing field 'tx_json.Destination'."}}}, + {"Missing 'Destination' in sign_for.", + __LINE__, + R"({ + "command": "doesnt_matter", + "account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "secret": "masterpassphrase", + "tx_json": { + "Account": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", + "DeliverMax": "1000000000", + "Fee": 50, + "Sequence": 0, + "SigningPubKey": "", + "TransactionType": "Payment" + } +})", + {{"Missing field 'tx_json.Destination'.", + "Missing field 'tx_json.Destination'.", + "Missing field 'tx_json.Destination'.", + "Missing field 'tx_json.Destination'."}}}, + {"Missing 'Fee' in sign_for.", __LINE__, R"({ @@ -1692,6 +1712,34 @@ static constexpr TxnTestData txnTestArray[] = { "Missing field 'account'.", "Invalid field 'tx_json.Amount'."}}}, + {"Invalid DeliverMax in submit_multisigned Payment.", + __LINE__, + R"({ + "command": "submit_multisigned", + "tx_json": { + "Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "DeliverMax": "NotANumber", + "Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", + "Fee": 50, + "Sequence": 0, + "Signers": [ + { + "Signer": { + "Account": "rPcNzota6B8YBokhYtcTNqQVCngtbnWfux", + "TxnSignature": "3045022100F9ED357606932697A4FAB2BE7F222C21DD93CA4CFDD90357AADD07465E8457D6022038173193E3DFFFB5D78DD738CC0905395F885DA65B98FDB9793901FE3FD26ECE", + "SigningPubKey": "02FE36A690D6973D55F88553F5D2C4202DE75F2CF8A6D0E17C70AC223F044501F8" + } + } + ], + "SigningPubKey": "", + "TransactionType": "Payment" + } +})", + {{"Missing field 'secret'.", + "Missing field 'secret'.", + "Missing field 'account'.", + "Invalid field 'tx_json.Amount'."}}}, + {"No build_path in submit_multisigned.", __LINE__, R"({ @@ -1905,6 +1953,50 @@ static constexpr TxnTestData txnTestArray[] = { "A Signer may not be the transaction's Account " "(rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh)."}}}, + {"Empty Signers array in submit_multisigned, use DeliverMax", + __LINE__, + R"({ + "command": "submit_multisigned", + "tx_json": { + "Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "DeliverMax": "10000000", + "Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", + "Fee": 50, + "Sequence": 0, + "Signers": [ + ], + "SigningPubKey": "", + "TransactionType": "Payment" + } +})", + {{"Missing field 'secret'.", + "Missing field 'secret'.", + "Missing field 'account'.", + "tx_json.Signers array may not be empty."}}}, + + {"Payment cannot specify both DeliverMax and Amount.", + __LINE__, + R"({ + "command": "doesnt_matter", + "account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "secret": "masterpassphrase", + "debug_signing": 0, + "tx_json": { + "Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "Amount": "1000000000", + "DeliverMax": "1000000000", + "Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", + "Fee": 50, + "Sequence": 0, + "SigningPubKey": "", + "TransactionType": "Payment" + } +})", + {{"Cannot specify both 'Amount' and 'DeliverMax'", + "Cannot specify both 'Amount' and 'DeliverMax'", + "Cannot specify both 'Amount' and 'DeliverMax'", + "Cannot specify both 'Amount' and 'DeliverMax'"}}}, + }; class JSONRPC_test : public beast::unit_test::suite From 2ec189b92786d8a8d648f897c9c6b6632bf7be1d Mon Sep 17 00:00:00 2001 From: Bronek Kozicki <823856+Bronek@users.noreply.github.com> Date: Mon, 9 Oct 2023 15:07:43 +0100 Subject: [PATCH 07/21] Add DeliverMax to transaction_entry tests --- src/test/rpc/JSONRPC_test.cpp | 2 +- src/test/rpc/TransactionEntry_test.cpp | 123 ++++++++++++++++++++++--- 2 files changed, 111 insertions(+), 14 deletions(-) diff --git a/src/test/rpc/JSONRPC_test.cpp b/src/test/rpc/JSONRPC_test.cpp index cf0872ca033..0de3f113b3b 100644 --- a/src/test/rpc/JSONRPC_test.cpp +++ b/src/test/rpc/JSONRPC_test.cpp @@ -1035,7 +1035,7 @@ static constexpr TxnTestData txnTestArray[] = { "Missing field 'tx_json.Destination'.", "Missing field 'tx_json.Destination'."}}}, - {"Missing 'Destination' in sign_for.", + {"Missing 'Destination' in sign_for, use DeliverMax", __LINE__, R"({ "command": "doesnt_matter", diff --git a/src/test/rpc/TransactionEntry_test.cpp b/src/test/rpc/TransactionEntry_test.cpp index a477a624859..7e80ba00c58 100644 --- a/src/test/rpc/TransactionEntry_test.cpp +++ b/src/test/rpc/TransactionEntry_test.cpp @@ -17,6 +17,8 @@ */ //============================================================================== +#include +#include #include #include #include @@ -150,7 +152,7 @@ class TransactionEntry_test : public beast::unit_test::suite auto check_tx = [this, &env]( int index, std::string const txhash, - std::string const type = "") { + std::string const expected_json = "") { // first request using ledger_index to lookup Json::Value const resIndex{[&env, index, &txhash]() { Json::Value params{Json::objectValue}; @@ -164,13 +166,32 @@ class TransactionEntry_test : public beast::unit_test::suite return; BEAST_EXPECT(resIndex[jss::tx_json][jss::hash] == txhash); - if (!type.empty()) + if (!expected_json.empty()) { - BEAST_EXPECTS( - resIndex[jss::tx_json][jss::TransactionType] == type, - txhash + " is " + - resIndex[jss::tx_json][jss::TransactionType] - .asString()); + Json::Value expected; + Json::Reader().parse(expected_json, expected); + if (RPC::contains_error(expected)) + Throw( + "Internal JSONRPC_test error. Bad test JSON."); + + for (auto memberIt = expected.begin(); + memberIt != expected.end(); + memberIt++) + { + auto const name = memberIt.memberName(); + BEAST_EXPECT(resIndex[jss::tx_json].isMember(name)); + auto const received = resIndex[jss::tx_json][name]; + std::ostringstream ssReceived; + ssReceived << received; + std::ostringstream ssExpected; + ssExpected << *memberIt; + BEAST_EXPECTS( + received == *memberIt, + txhash + " contains \n\"" + name + "\": " // + + ssReceived.str() // + + "but expected " // + + ssExpected.str()); + } } // second request using ledger_hash to lookup and verify @@ -216,8 +237,30 @@ class TransactionEntry_test : public beast::unit_test::suite // these are actually AccountSet txs because fund does two txs and // env.tx only reports the last one - check_tx(env.closed()->seq(), fund_1_tx); - check_tx(env.closed()->seq(), fund_2_tx); + check_tx(env.closed()->seq(), fund_1_tx, R"( +{ + "Account" : "r4nmQNH4Fhjfh6cHDbvVSsBv7KySbj4cBf", + "Fee" : "10", + "Sequence" : 3, + "SetFlag" : 8, + "SigningPubKey" : "0324CAAFA2212D2AEAB9D42D481535614AED486293E1FB1380FF070C3DD7FB4264", + "TransactionType" : "AccountSet", + "TxnSignature" : "3044022007B35E3B99460534FF6BC3A66FBBA03591C355CC38E38588968E87CCD01BE229022071A443026DE45041B55ABB1CC76812A87EA701E475BBB7E165513B4B242D3474", + "hash" : "F4E9DF90D829A9E8B423FF68C34413E240D8D8BB0EFD080DF08114ED398E2506" +} +)"); + check_tx(env.closed()->seq(), fund_2_tx, R"( +{ + "Account" : "rGpeQzUWFu4fMhJHZ1Via5aqFC3A5twZUD", + "Fee" : "10", + "Sequence" : 3, + "SetFlag" : 8, + "SigningPubKey" : "03CFF28E067A2CCE6CC5A598C0B845CBD3F30A7863BE9C0DD55F4960EFABCCF4D0", + "TransactionType" : "AccountSet", + "TxnSignature" : "3045022100C8857FC0759A2AC0D2F320684691A66EAD252EAED9EF88C79791BC58BFCC9D860220421722286487DD0ED6BBA626CE6FCBDD14289F7F4726870C3465A4054C2702D7", + "hash" : "6853CD8226A05068C951CB1F54889FF4E40C5B440DC1C5BA38F114C4E0B1E705" +} +)"); env.trust(A2["USD"](1000), A1); // the trust tx is actually a payment since the trust method @@ -231,16 +274,70 @@ class TransactionEntry_test : public beast::unit_test::suite boost::lexical_cast(env.tx()->getTransactionID()); env.close(); - check_tx(env.closed()->seq(), trust_tx); - check_tx(env.closed()->seq(), pay_tx, jss::Payment.c_str()); + check_tx(env.closed()->seq(), trust_tx, R"( +{ + "Account" : "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "DeliverMax" : "10", + "Destination" : "r4nmQNH4Fhjfh6cHDbvVSsBv7KySbj4cBf", + "Fee" : "10", + "Flags" : 2147483648, + "Sequence" : 3, + "SigningPubKey" : "0330E7FC9D56BB25D6893BA3F317AE5BCF33B3291BD63DB32654A313222F7FD020", + "TransactionType" : "Payment", + "TxnSignature" : "3044022033D9EBF7F02950AF2F6B13C07AEE641C8FEBDD540A338FCB9027A965A4AED35B02206E4E227DCC226A3456C0FEF953449D21645A24EB63CA0BB7C5B62470147FD1D1", + "hash" : "C992D97D88FF444A1AB0C06B27557EC54B7F7DA28254778E60238BEA88E0C101" +} +)"); + + check_tx( + env.closed()->seq(), + pay_tx, + R"( +{ + "Account" : "rGpeQzUWFu4fMhJHZ1Via5aqFC3A5twZUD", + "DeliverMax" : + { + "currency" : "USD", + "issuer" : "rGpeQzUWFu4fMhJHZ1Via5aqFC3A5twZUD", + "value" : "5" + }, + "Destination" : "r4nmQNH4Fhjfh6cHDbvVSsBv7KySbj4cBf", + "Fee" : "10", + "Flags" : 2147483648, + "Sequence" : 4, + "SigningPubKey" : "03CFF28E067A2CCE6CC5A598C0B845CBD3F30A7863BE9C0DD55F4960EFABCCF4D0", + "TransactionType" : "Payment", + "TxnSignature" : "30450221008A722B7F16EDB2348886E88ED4EC682AE9973CC1EE0FF37C93BB2CEC821D3EDF022059E464472031BA5E0D88A93E944B6A8B8DB3E1D5E5D1399A805F615789DB0BED", + "hash" : "988046D484ACE9F5F6A8C792D89C6EA2DB307B5DDA9864AEBA88E6782ABD0865" +} +)"); env(offer(A2, XRP(100), A2["USD"](1))); auto offer_tx = boost::lexical_cast(env.tx()->getTransactionID()); env.close(); - - check_tx(env.closed()->seq(), offer_tx, jss::OfferCreate.c_str()); + check_tx( + env.closed()->seq(), + offer_tx, + R"( +{ + "Account" : "rGpeQzUWFu4fMhJHZ1Via5aqFC3A5twZUD", + "Fee" : "10", + "Sequence" : 5, + "SigningPubKey" : "03CFF28E067A2CCE6CC5A598C0B845CBD3F30A7863BE9C0DD55F4960EFABCCF4D0", + "TakerGets" : + { + "currency" : "USD", + "issuer" : "rGpeQzUWFu4fMhJHZ1Via5aqFC3A5twZUD", + "value" : "1" + }, + "TakerPays" : "100000000", + "TransactionType" : "OfferCreate", + "TxnSignature" : "304502210093FC93ACB77B4E3DE3315441BD010096734859080C1797AB735EB47EBD541BD102205020BB1A7C3B4141279EE4C287C13671E2450EA78914EFD0C6DB2A18344CD4F2", + "hash" : "5FCC1A27A7664F82A0CC4BE5766FBBB7C560D52B93AA7B550CD33B27AEC7EFFB" +} +)"); } public: From aa9f0a9ffe60602847c5f8829edd423dd2e82647 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki <823856+Bronek@users.noreply.github.com> Date: Mon, 9 Oct 2023 15:56:15 +0100 Subject: [PATCH 08/21] Add DeliverMax to subscribe tests --- src/test/rpc/Subscribe_test.cpp | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/src/test/rpc/Subscribe_test.cpp b/src/test/rpc/Subscribe_test.cpp index 5322319907f..7725390f6b6 100644 --- a/src/test/rpc/Subscribe_test.cpp +++ b/src/test/rpc/Subscribe_test.cpp @@ -194,8 +194,16 @@ class Subscribe_test : public beast::unit_test::suite // Check stream update for payment transaction BEAST_EXPECT(wsc->findMsg(5s, [&](auto const& jv) { return jv[jss::meta]["AffectedNodes"][1u]["CreatedNode"] - ["NewFields"][jss::Account] == - Account("alice").human(); + ["NewFields"][jss::Account] // + == Account("alice").human() && + jv[jss::transaction][jss::TransactionType] // + == jss::Payment && + jv[jss::transaction][jss::DeliverMax] // + == "10000000010" && + jv[jss::transaction][jss::Fee] // + == "10" && + jv[jss::transaction][jss::Sequence] // + == 1; })); // Check stream update for accountset transaction @@ -211,7 +219,16 @@ class Subscribe_test : public beast::unit_test::suite // Check stream update for payment transaction BEAST_EXPECT(wsc->findMsg(5s, [&](auto const& jv) { return jv[jss::meta]["AffectedNodes"][1u]["CreatedNode"] - ["NewFields"][jss::Account] == Account("bob").human(); + ["NewFields"][jss::Account] // + == Account("bob").human() && + jv[jss::transaction][jss::TransactionType] // + == jss::Payment && + jv[jss::transaction][jss::DeliverMax] // + == "10000000010" && + jv[jss::transaction][jss::Fee] // + == "10" && + jv[jss::transaction][jss::Sequence] // + == 2; })); // Check stream update for accountset transaction From e35bf625d60f9ce13bab755d1f9dc42ab6b8c870 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki <823856+Bronek@users.noreply.github.com> Date: Mon, 9 Oct 2023 16:16:18 +0100 Subject: [PATCH 09/21] Allow both Amount and DeliverMax if identical --- src/ripple/rpc/impl/TransactionSign.cpp | 12 ++++++++---- src/test/rpc/JSONRPC_test.cpp | 26 +++++++++++++++++++++++-- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/ripple/rpc/impl/TransactionSign.cpp b/src/ripple/rpc/impl/TransactionSign.cpp index 45d4026454d..86d015543f6 100644 --- a/src/ripple/rpc/impl/TransactionSign.cpp +++ b/src/ripple/rpc/impl/TransactionSign.cpp @@ -171,11 +171,15 @@ checkPayment( if (tx_json.isMember(jss::DeliverMax)) { if (tx_json.isMember(jss::Amount)) - return RPC::make_error( - rpcINVALID_PARAMS, - "Cannot specify both 'Amount' and 'DeliverMax'"); + { + if (tx_json[jss::DeliverMax] != tx_json[jss::Amount]) + return RPC::make_error( + rpcINVALID_PARAMS, + "Cannot specify both 'Amount' and 'DeliverMax'"); + } + else + tx_json[jss::Amount] = tx_json[jss::DeliverMax]; - tx_json[jss::Amount] = tx_json[jss::DeliverMax]; tx_json.removeMember(jss::DeliverMax); } diff --git a/src/test/rpc/JSONRPC_test.cpp b/src/test/rpc/JSONRPC_test.cpp index 0de3f113b3b..dc81696fd9e 100644 --- a/src/test/rpc/JSONRPC_test.cpp +++ b/src/test/rpc/JSONRPC_test.cpp @@ -1974,7 +1974,29 @@ static constexpr TxnTestData txnTestArray[] = { "Missing field 'account'.", "tx_json.Signers array may not be empty."}}}, - {"Payment cannot specify both DeliverMax and Amount.", + {"Empty Signers array in submit_multisigned, use DeliverMax and Amount", + __LINE__, + R"({ + "command": "submit_multisigned", + "tx_json": { + "Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "Amount": "10000000", + "DeliverMax": "10000000", + "Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", + "Fee": 50, + "Sequence": 0, + "Signers": [ + ], + "SigningPubKey": "", + "TransactionType": "Payment" + } +})", + {{"Missing field 'secret'.", + "Missing field 'secret'.", + "Missing field 'account'.", + "tx_json.Signers array may not be empty."}}}, + + {"Payment cannot specify different DeliverMax and Amount.", __LINE__, R"({ "command": "doesnt_matter", @@ -1984,7 +2006,7 @@ static constexpr TxnTestData txnTestArray[] = { "tx_json": { "Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", "Amount": "1000000000", - "DeliverMax": "1000000000", + "DeliverMax": "1000000020", "Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", "Fee": 50, "Sequence": 0, From 03edc898fe93cf531b045990cd4dc1a8cea96ee1 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki <823856+Bronek@users.noreply.github.com> Date: Tue, 10 Oct 2023 14:36:05 +0100 Subject: [PATCH 10/21] Add DeliverMax to tx tests --- src/test/rpc/Transaction_test.cpp | 59 +++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/src/test/rpc/Transaction_test.cpp b/src/test/rpc/Transaction_test.cpp index cc3f70a1516..ea99409f67b 100644 --- a/src/test/rpc/Transaction_test.cpp +++ b/src/test/rpc/Transaction_test.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include #include @@ -692,6 +693,63 @@ class Transaction_test : public beast::unit_test::suite } } + void + testRequest(FeatureBitset features) + { + testcase("Test Request"); + + using namespace test::jtx; + using std::to_string; + + const char* COMMAND = jss::tx.c_str(); + // const char* BINARY = jss::binary.c_str(); + + Env env{*this}; + Account const alice{"alice"}; + Account const alie{"alie"}; + Account const gw{"gw"}; + auto const USD{gw["USD"]}; + + env.fund(XRP(1000000), alice, gw); + env.close(); + + // AccountSet + env(noop(alice)); + + // Payment + env(pay(alice, gw, XRP(100))); + + std::shared_ptr txn = env.tx(); + env.close(); + std::shared_ptr meta = + env.closed()->txRead(env.tx()->getTransactionID()).second; + + Json::Value expected = txn->getJson(JsonOptions::none); + expected[jss::DeliverMax] = expected[jss::Amount]; + + auto const result = + env.rpc(COMMAND, to_string(txn->getTransactionID())); + BEAST_EXPECT(result[jss::result][jss::status] == jss::success); + + for (auto memberIt = expected.begin(); memberIt != expected.end(); + memberIt++) + { + std::string const name = memberIt.memberName(); + BEAST_EXPECT(result[jss::result].isMember(name)); + auto const received = result[jss::result][name]; + std::ostringstream ssReceived; + ssReceived << received; + std::ostringstream ssExpected; + ssExpected << *memberIt; + BEAST_EXPECTS( + received == *memberIt, + "Transaction contains \n\"" + name + "\": " // + + ssReceived.str() // + + "but expected " // + + ssExpected.str()); + } + } + public: void run() override @@ -708,6 +766,7 @@ class Transaction_test : public beast::unit_test::suite testRangeCTIDRequest(features); testCTIDValidation(features); testCTIDRPC(features); + testRequest(features); } }; From 9f4206a9e4e9b78e7742610db766baeb9e546195 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki <823856+Bronek@users.noreply.github.com> Date: Tue, 10 Oct 2023 15:19:17 +0100 Subject: [PATCH 11/21] Add DeliverMax to account_tx test --- src/test/rpc/AccountTx_test.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/test/rpc/AccountTx_test.cpp b/src/test/rpc/AccountTx_test.cpp index 24f6737917b..3416819414e 100644 --- a/src/test/rpc/AccountTx_test.cpp +++ b/src/test/rpc/AccountTx_test.cpp @@ -119,14 +119,22 @@ class AccountTx_test : public beast::unit_test::suite // Ledger 3 has the two txs associated with funding the account // All other ledgers have no txs - auto hasTxs = [](Json::Value const& j) { + auto hasTxs = [apiVersion](Json::Value const& j) { return j.isMember(jss::result) && (j[jss::result][jss::status] == "success") && (j[jss::result][jss::transactions].size() == 2) && (j[jss::result][jss::transactions][0u][jss::tx] [jss::TransactionType] == jss::AccountSet) && (j[jss::result][jss::transactions][1u][jss::tx] - [jss::TransactionType] == jss::Payment); + [jss::TransactionType] == jss::Payment) && + (j[jss::result][jss::transactions][1u][jss::tx] + [jss::DeliverMax] == "10000000010") && + ((apiVersion > 1 && + !j[jss::result][jss::transactions][1u][jss::tx].isMember( + jss::Amount)) || + (j[jss::result][jss::transactions][1u][jss::tx][jss::Amount] == + j[jss::result][jss::transactions][1u][jss::tx] + [jss::DeliverMax])); }; auto noTxs = [](Json::Value const& j) { From 432fd163a8a6bbe13aca1de5e955b2c2410026e0 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki <823856+Bronek@users.noreply.github.com> Date: Tue, 10 Oct 2023 17:49:34 +0100 Subject: [PATCH 12/21] Change apiVersion to unsigned --- src/ripple/app/misc/NetworkOPs.cpp | 4 ++-- src/ripple/basics/MultivarJson.h | 2 +- src/ripple/net/InfoSub.h | 6 +++--- src/ripple/net/impl/InfoSub.cpp | 4 ++-- src/ripple/rpc/DeliverMax.h | 6 ++---- src/ripple/rpc/impl/DeliverMax.cpp | 5 +---- src/test/basics/MultivarJson_test.cpp | 5 ++--- 7 files changed, 13 insertions(+), 19 deletions(-) diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 4a248614049..96f433da265 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -606,7 +606,7 @@ class NetworkOPsImp final : public NetworkOPs std::shared_ptr const& ledger, bool validated, std::optional> meta, - int apiVersion); + unsigned int apiVersion); void pubValidatedTransaction( @@ -3095,7 +3095,7 @@ NetworkOPsImp::transJson( std::shared_ptr const& ledger, bool validated, std::optional> meta, - int apiVersion) + unsigned int apiVersion) { Json::Value jvObj(Json::objectValue); std::string sToken; diff --git a/src/ripple/basics/MultivarJson.h b/src/ripple/basics/MultivarJson.h index 81aa09ffa48..400a8058434 100644 --- a/src/ripple/basics/MultivarJson.h +++ b/src/ripple/basics/MultivarJson.h @@ -55,7 +55,7 @@ using MultiApiJson = MultivarJson<2>; // Helper to create appropriate selector for indexing MultiApiJson by apiVersion constexpr auto -apiVersionSelector(int apiVersion) noexcept +apiVersionSelector(unsigned int apiVersion) noexcept { return [apiVersion]() constexpr { diff --git a/src/ripple/net/InfoSub.h b/src/ripple/net/InfoSub.h index 60ab9faff9c..9934b1818db 100644 --- a/src/ripple/net/InfoSub.h +++ b/src/ripple/net/InfoSub.h @@ -230,9 +230,9 @@ class InfoSub : public CountedObject getRequest(); void - setApiVersion(int apiVersion); + setApiVersion(unsigned int apiVersion); - int + unsigned int getApiVersion() const noexcept; protected: @@ -246,7 +246,7 @@ class InfoSub : public CountedObject std::shared_ptr request_; std::uint64_t mSeq; hash_set accountHistorySubscriptions_; - int apiVersion = 1; + unsigned int apiVersion = 1; static int assign_id() diff --git a/src/ripple/net/impl/InfoSub.cpp b/src/ripple/net/impl/InfoSub.cpp index c53152ffcf9..a9b0d954d26 100644 --- a/src/ripple/net/impl/InfoSub.cpp +++ b/src/ripple/net/impl/InfoSub.cpp @@ -137,12 +137,12 @@ InfoSub::getRequest() } void -InfoSub::setApiVersion(int apiVersion) +InfoSub::setApiVersion(unsigned int apiVersion) { this->apiVersion = apiVersion; } -int +unsigned int InfoSub::getApiVersion() const noexcept { return this->apiVersion; diff --git a/src/ripple/rpc/DeliverMax.h b/src/ripple/rpc/DeliverMax.h index 0798a170b89..91ede5bbcb2 100644 --- a/src/ripple/rpc/DeliverMax.h +++ b/src/ripple/rpc/DeliverMax.h @@ -20,8 +20,6 @@ #ifndef RIPPLE_RPC_DELIVERMAX_H_INCLUDED #define RIPPLE_RPC_DELIVERMAX_H_INCLUDED -#include -#include #include #include @@ -48,13 +46,13 @@ struct Context; Copy `Amount` field to `DeliverMax` field in transaction output JSON. This only applies to Payment transaction type, all others are ignored. - When context.apiVersion > 1 will also remove `Amount` field, forcing users + When apiVersion > 1 will also remove `Amount` field, forcing users to access this value using new `DeliverMax` field only. @{ */ void -insertDeliverMax(Json::Value& tx_json, TxType txnType, int apiVersion); +insertDeliverMax(Json::Value& tx_json, TxType txnType, unsigned int apiVersion); /** @} */ diff --git a/src/ripple/rpc/impl/DeliverMax.cpp b/src/ripple/rpc/impl/DeliverMax.cpp index f5b7f1c2f91..7c1412e2e9a 100644 --- a/src/ripple/rpc/impl/DeliverMax.cpp +++ b/src/ripple/rpc/impl/DeliverMax.cpp @@ -18,16 +18,13 @@ //============================================================================== #include -#include #include -#include -#include "ripple/protocol/TxFormats.h" namespace ripple { namespace RPC { void -insertDeliverMax(Json::Value& tx_json, TxType txnType, int apiVersion) +insertDeliverMax(Json::Value& tx_json, TxType txnType, unsigned int apiVersion) { if (tx_json.isMember(jss::Amount)) { diff --git a/src/test/basics/MultivarJson_test.cpp b/src/test/basics/MultivarJson_test.cpp index cca478f4e8e..4a867575be5 100644 --- a/src/test/basics/MultivarJson_test.cpp +++ b/src/test/basics/MultivarJson_test.cpp @@ -223,14 +223,13 @@ struct MultivarJson_test : beast::unit_test::suite BEAST_EXPECT(x.select(apiVersionSelector(0)) == obj1); BEAST_EXPECT(x.select(apiVersionSelector(2)) == str1); - static_assert( - apiVersionSelector(std::numeric_limits::min())() == 0); static_assert(apiVersionSelector(0)() == 0); static_assert(apiVersionSelector(1)() == 0); static_assert(apiVersionSelector(2)() == 1); static_assert(apiVersionSelector(3)() == 1); static_assert( - apiVersionSelector(std::numeric_limits::max())() == 1); + apiVersionSelector( + std::numeric_limits::max())() == 1); } } }; From c99959e4ef4672ae595fec7cdebb140ee4502d7e Mon Sep 17 00:00:00 2001 From: Bronek Kozicki <823856+Bronek@users.noreply.github.com> Date: Wed, 11 Oct 2023 19:22:19 +0100 Subject: [PATCH 13/21] Update API-CHANGELOG.md --- API-CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md index 5115eee0855..9845a15bfb3 100644 --- a/API-CHANGELOG.md +++ b/API-CHANGELOG.md @@ -149,6 +149,15 @@ Since `api_version` 2 is in "beta", breaking changes to V2 can currently be made - Attempting to use a non-boolean value (such as a string) for the `transactions` parameter returns `invalidParams` (`rpcINVALID_PARAMS`). Previously, in API version 1, no error was returned. (https://github.com/XRPLF/rippled/pull/4620) +##### In progress + +- In `Payment` transaction type, JSON RPC field `Amount` is renamed to `DeliverMax`. To enable smooth client transition, `Amount` is still handled, as described below: + - On JSON RPC input (e.g. `submit_multisigned` etc. methods), `Amount` is recognized as an alias to `DeliverMax` for both API version 1 and version 2 clients. + - On JSON RPC input, submitting both `Amount` and `DeliverMax` fields is allowed _only_ if they are identical; otherwise such input is rejected with `rpcINVALID_PARAMS` error. + - On JSON RPC output (e.g. `subscribe`, `account_tx` etc. methods), `DeliverMax` is present in both API version 1 and version 2. + - On JSON RPC output, `Amount` is only present in API version 1 and _not_ in version 2. + + # Unit tests for API changes The following information is useful to developers contributing to this project: From 48697e858e0090ef3df619bdfce18edc96516fec Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Thu, 19 Oct 2023 09:22:11 +0100 Subject: [PATCH 14/21] Minor fixes --- Builds/CMake/RippledCore.cmake | 5 ++-- src/ripple/app/ledger/BookListeners.h | 2 +- src/ripple/app/ledger/OrderBookDB.h | 2 +- src/ripple/app/ledger/impl/LedgerToJson.cpp | 2 +- src/ripple/{rpc => app/misc}/DeliverMax.h | 9 ------ src/ripple/app/misc/NetworkOPs.cpp | 28 +++++++++---------- .../{rpc => app/misc}/impl/DeliverMax.cpp | 3 +- src/ripple/{basics => json}/MultivarJson.h | 25 +++++++++++++++++ src/ripple/rpc/handlers/AccountTx.cpp | 2 +- src/ripple/rpc/handlers/TransactionEntry.cpp | 2 +- src/ripple/rpc/handlers/Tx.cpp | 2 +- src/ripple/rpc/handlers/TxHistory.cpp | 2 +- .../{basics => json}/MultivarJson_test.cpp | 5 ++-- 13 files changed, 54 insertions(+), 35 deletions(-) rename src/ripple/{rpc => app/misc}/DeliverMax.h (94%) rename src/ripple/{rpc => app/misc}/impl/DeliverMax.cpp (97%) rename src/ripple/{basics => json}/MultivarJson.h (68%) rename src/test/{basics => json}/MultivarJson_test.cpp (98%) diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index dc659a2f3a5..d8dfa6c4f40 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -217,6 +217,7 @@ install ( install ( FILES src/ripple/json/JsonPropertyStream.h + src/ripple/json/MultivarJson.h src/ripple/json/Object.h src/ripple/json/Output.h src/ripple/json/Writer.h @@ -467,6 +468,7 @@ target_sources (rippled PRIVATE src/ripple/app/misc/detail/impl/WorkSSL.cpp src/ripple/app/misc/impl/AccountTxPaging.cpp src/ripple/app/misc/impl/AmendmentTable.cpp + src/ripple/app/misc/impl/DeliverMax.cpp src/ripple/app/misc/impl/LoadFeeTrack.cpp src/ripple/app/misc/impl/Manifest.cpp src/ripple/app/misc/impl/Transaction.cpp @@ -731,7 +733,6 @@ target_sources (rippled PRIVATE src/ripple/rpc/handlers/Validators.cpp src/ripple/rpc/handlers/WalletPropose.cpp src/ripple/rpc/impl/DeliveredAmount.cpp - src/ripple/rpc/impl/DeliverMax.cpp src/ripple/rpc/impl/Handler.cpp src/ripple/rpc/impl/LegacyPathFind.cpp src/ripple/rpc/impl/RPCHandler.cpp @@ -855,7 +856,6 @@ if (tests) src/test/basics/join_test.cpp src/test/basics/mulDiv_test.cpp src/test/basics/tagged_integer_test.cpp - src/test/basics/MultivarJson_test.cpp #[===============================[ test sources: subdir: beast @@ -918,6 +918,7 @@ if (tests) src/test/json/Output_test.cpp src/test/json/Writer_test.cpp src/test/json/json_value_test.cpp + src/test/json/MultivarJson_test.cpp #[===============================[ test sources: subdir: jtx diff --git a/src/ripple/app/ledger/BookListeners.h b/src/ripple/app/ledger/BookListeners.h index 080e15716a6..748378a12b1 100644 --- a/src/ripple/app/ledger/BookListeners.h +++ b/src/ripple/app/ledger/BookListeners.h @@ -20,7 +20,7 @@ #ifndef RIPPLE_APP_LEDGER_BOOKLISTENERS_H_INCLUDED #define RIPPLE_APP_LEDGER_BOOKLISTENERS_H_INCLUDED -#include +#include #include #include diff --git a/src/ripple/app/ledger/OrderBookDB.h b/src/ripple/app/ledger/OrderBookDB.h index 9b41109e5c6..b072bafb0c3 100644 --- a/src/ripple/app/ledger/OrderBookDB.h +++ b/src/ripple/app/ledger/OrderBookDB.h @@ -23,7 +23,7 @@ #include #include #include -#include +#include #include diff --git a/src/ripple/app/ledger/impl/LedgerToJson.cpp b/src/ripple/app/ledger/impl/LedgerToJson.cpp index 7fee8703636..55123ba2362 100644 --- a/src/ripple/app/ledger/impl/LedgerToJson.cpp +++ b/src/ripple/app/ledger/impl/LedgerToJson.cpp @@ -19,11 +19,11 @@ #include #include +#include #include #include #include #include -#include #include namespace ripple { diff --git a/src/ripple/rpc/DeliverMax.h b/src/ripple/app/misc/DeliverMax.h similarity index 94% rename from src/ripple/rpc/DeliverMax.h rename to src/ripple/app/misc/DeliverMax.h index 91ede5bbcb2..44e9ffb833f 100644 --- a/src/ripple/rpc/DeliverMax.h +++ b/src/ripple/app/misc/DeliverMax.h @@ -31,17 +31,8 @@ class Value; namespace ripple { -class ReadView; -class Transaction; -class TxMeta; -class STTx; - namespace RPC { -struct JsonContext; - -struct Context; - /** Copy `Amount` field to `DeliverMax` field in transaction output JSON. This only applies to Payment transaction type, all others are ignored. diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 96f433da265..05e71b17f40 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -41,7 +42,7 @@ #include #include #include -#include +#include #include #include #include @@ -65,7 +66,6 @@ #include #include #include -#include #include #include #include @@ -603,8 +603,8 @@ class NetworkOPsImp final : public NetworkOPs transJson( std::shared_ptr const& transaction, TER result, - std::shared_ptr const& ledger, bool validated, + std::shared_ptr const& ledger, std::optional> meta, unsigned int apiVersion); @@ -2749,8 +2749,8 @@ NetworkOPsImp::pubProposedTransaction( TER result) { MultiApiJson jvObj( - {transJson(transaction, result, ledger, false, std::nullopt, 1), - transJson(transaction, result, ledger, false, std::nullopt, 2)}); + {transJson(transaction, result, false, ledger, std::nullopt, 1), + transJson(transaction, result, false, ledger, std::nullopt, 2)}); { std::lock_guard sl(mSubLock); @@ -3092,8 +3092,8 @@ Json::Value NetworkOPsImp::transJson( std::shared_ptr const& transaction, TER result, - std::shared_ptr const& ledger, bool validated, + std::shared_ptr const& ledger, std::optional> meta, unsigned int apiVersion) { @@ -3169,8 +3169,8 @@ NetworkOPsImp::pubValidatedTransaction( auto const metaRef = std::ref(transaction.getMeta()); auto const trResult = transaction.getResult(); MultiApiJson jvObj( - {transJson(stTxn, trResult, ledger, true, metaRef, 1), - transJson(stTxn, trResult, ledger, true, metaRef, 2)}); + {transJson(stTxn, trResult, true, ledger, metaRef, 1), + transJson(stTxn, trResult, true, ledger, metaRef, 2)}); { std::lock_guard sl(mSubLock); @@ -3316,8 +3316,8 @@ NetworkOPsImp::pubAccountTransaction( auto const metaRef = std::ref(transaction.getMeta()); auto const trResult = transaction.getResult(); MultiApiJson jvObj( - {transJson(stTxn, trResult, ledger, true, metaRef, 1), - transJson(stTxn, trResult, ledger, true, metaRef, 2)}); + {transJson(stTxn, trResult, true, ledger, metaRef, 1), + transJson(stTxn, trResult, true, ledger, metaRef, 2)}); for (InfoSub::ref isrListener : notify) { @@ -3398,8 +3398,8 @@ NetworkOPsImp::pubProposedAccountTransaction( { // Create two different Json objects, for different API versions MultiApiJson jvObj({ - transJson(tx, result, ledger, false, std::nullopt, 1), - transJson(tx, result, ledger, false, std::nullopt, 2), + transJson(tx, result, false, ledger, std::nullopt, 1), + transJson(tx, result, false, ledger, std::nullopt, 2), }); for (InfoSub::ref isrListener : notify) @@ -3798,9 +3798,9 @@ NetworkOPsImp::addAccountHistoryJob(SubAccountHistoryInfoWeak subInfo) auto const mRef = std::ref(*meta); auto const trR = meta->getResultTER(); MultiApiJson jvTx( - {transJson(stTxn, trR, curTxLedger, true, mRef, 1), + {transJson(stTxn, trR, true, curTxLedger, mRef, 1), transJson( - stTxn, trR, curTxLedger, true, mRef, 2)}); + stTxn, trR, true, curTxLedger, mRef, 2)}); jvTx.set(jss::account_history_tx_index, txHistoryIndex); txHistoryIndex -= 1; diff --git a/src/ripple/rpc/impl/DeliverMax.cpp b/src/ripple/app/misc/impl/DeliverMax.cpp similarity index 97% rename from src/ripple/rpc/impl/DeliverMax.cpp rename to src/ripple/app/misc/impl/DeliverMax.cpp index 7c1412e2e9a..810b750a355 100644 --- a/src/ripple/rpc/impl/DeliverMax.cpp +++ b/src/ripple/app/misc/impl/DeliverMax.cpp @@ -17,8 +17,9 @@ */ //============================================================================== +#include + #include -#include namespace ripple { namespace RPC { diff --git a/src/ripple/basics/MultivarJson.h b/src/ripple/json/MultivarJson.h similarity index 68% rename from src/ripple/basics/MultivarJson.h rename to src/ripple/json/MultivarJson.h index 400a8058434..18bf407671b 100644 --- a/src/ripple/basics/MultivarJson.h +++ b/src/ripple/json/MultivarJson.h @@ -53,6 +53,31 @@ struct MultivarJson // Wrapper for Json for all supported API versions. using MultiApiJson = MultivarJson<2>; +/* + +NOTE: + +If a future API version change adds another possible format, change the size of +`MultiApiJson`, and update `apiVersionSelector()` to return the appropriate +selection value for the new `apiVersion` and higher. + +e.g. There are 2 formats now, the first, for version one, the second for +versions > 1. Hypothetically, if API version 4 adds a new format, `MultiApiJson` +would be MultivarJson<3>, and `apiVersionSelector` would return +`static_cast(apiVersion < 2 ? 0u : (apiVersion < 4 ? 1u : 2u))` + +NOTE: + +The more different JSON formats we support, the more CPU cycles we need to +pre-build JSON for different API versions e.g. when publishing streams to +`subscribe` clients. Hence it is desirable to keep MultiApiJson small and +instead fully deprecate and remove support for old API versions. For example, if +we removed support for API version 1 and added a different format for API +version 3, the `apiVersionSelector` would change to +`static_cast(apiVersion > 2)` + +*/ + // Helper to create appropriate selector for indexing MultiApiJson by apiVersion constexpr auto apiVersionSelector(unsigned int apiVersion) noexcept diff --git a/src/ripple/rpc/handlers/AccountTx.cpp b/src/ripple/rpc/handlers/AccountTx.cpp index 25048434a06..bd939a92f1c 100644 --- a/src/ripple/rpc/handlers/AccountTx.cpp +++ b/src/ripple/rpc/handlers/AccountTx.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include #include @@ -34,7 +35,6 @@ #include #include #include -#include #include #include #include diff --git a/src/ripple/rpc/handlers/TransactionEntry.cpp b/src/ripple/rpc/handlers/TransactionEntry.cpp index 9a0f68b71a1..677581db6a3 100644 --- a/src/ripple/rpc/handlers/TransactionEntry.cpp +++ b/src/ripple/rpc/handlers/TransactionEntry.cpp @@ -18,10 +18,10 @@ //============================================================================== #include +#include #include #include #include -#include #include namespace ripple { diff --git a/src/ripple/rpc/handlers/Tx.cpp b/src/ripple/rpc/handlers/Tx.cpp index e83c0378d23..92d0e4dd673 100644 --- a/src/ripple/rpc/handlers/Tx.cpp +++ b/src/ripple/rpc/handlers/Tx.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include #include @@ -28,7 +29,6 @@ #include #include #include -#include #include #include #include diff --git a/src/ripple/rpc/handlers/TxHistory.cpp b/src/ripple/rpc/handlers/TxHistory.cpp index d840a3df8ea..0f3e353fcbc 100644 --- a/src/ripple/rpc/handlers/TxHistory.cpp +++ b/src/ripple/rpc/handlers/TxHistory.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include #include @@ -29,7 +30,6 @@ #include #include #include -#include #include #include #include diff --git a/src/test/basics/MultivarJson_test.cpp b/src/test/json/MultivarJson_test.cpp similarity index 98% rename from src/test/basics/MultivarJson_test.cpp rename to src/test/json/MultivarJson_test.cpp index 4a867575be5..041e06ce6f7 100644 --- a/src/test/basics/MultivarJson_test.cpp +++ b/src/test/json/MultivarJson_test.cpp @@ -1,6 +1,6 @@ //------------------------------------------------------------------------------ /* - This file is part of rippled: https://github0.com/ripple/rippled + This file is part of rippled: https://github.com/XRPLF/rippled/ Copyright (c) 2023 Ripple Labs Inc. Permission to use, copy, modify, and/or distribute this software for any @@ -17,7 +17,8 @@ */ //============================================================================== -#include +#include + #include #include "ripple/beast/unit_test/suite.hpp" #include "ripple/json/json_value.h" From ff41b9091bcc52ba0ed3841d8e7ab7dd99d1678a Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Thu, 19 Oct 2023 12:03:33 +0100 Subject: [PATCH 15/21] Add MultivarJson::isMember and small fixes --- src/ripple/app/misc/NetworkOPs.cpp | 25 ++++++++-------- src/ripple/json/MultivarJson.h | 14 +++++++++ src/ripple/net/InfoSub.h | 2 +- src/ripple/net/impl/InfoSub.cpp | 5 ++-- src/ripple/rpc/impl/TransactionSign.cpp | 2 +- src/test/json/MultivarJson_test.cpp | 39 ++++++++++++++++++++++++- src/test/rpc/AccountTx_test.cpp | 7 +++-- src/test/rpc/JSONRPC_test.cpp | 8 ++--- src/test/rpc/Transaction_test.cpp | 27 ++++++++--------- 9 files changed, 91 insertions(+), 38 deletions(-) diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 05e71b17f40..385ccc0abc7 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -3329,16 +3329,16 @@ NetworkOPsImp::pubAccountTransaction( if (last) jvObj.set(jss::account_history_boundary, true); - assert(!jvObj.val[0].isMember(jss::account_history_tx_stream)); - assert(!jvObj.val[1].isMember(jss::account_history_tx_stream)); + assert( + jvObj.isMember(jss::account_history_tx_stream) == + MultiApiJson::none); for (auto& info : accountHistoryNotify) { auto& index = info.index_; if (index->forwardTxIndex_ == 0 && !index->haveHistorical_) jvObj.set(jss::account_history_tx_first, true); - jvObj.set(jss::account_history_tx_index, index->forwardTxIndex_); - index->forwardTxIndex_ += 1; + jvObj.set(jss::account_history_tx_index, index->forwardTxIndex_++); info.sink_->send( jvObj.select(apiVersionSelector(info.sink_->getApiVersion())), @@ -3407,15 +3407,15 @@ NetworkOPsImp::pubProposedAccountTransaction( jvObj.select(apiVersionSelector(isrListener->getApiVersion())), true); - assert(!jvObj.val[0].isMember(jss::account_history_tx_stream)); - assert(!jvObj.val[1].isMember(jss::account_history_tx_stream)); + assert( + jvObj.isMember(jss::account_history_tx_stream) == + MultiApiJson::none); for (auto& info : accountHistoryNotify) { auto& index = info.index_; if (index->forwardTxIndex_ == 0 && !index->haveHistorical_) jvObj.set(jss::account_history_tx_first, true); - jvObj.set(jss::account_history_tx_index, index->forwardTxIndex_); - index->forwardTxIndex_ += 1; + jvObj.set(jss::account_history_tx_index, index->forwardTxIndex_++); info.sink_->send( jvObj.select(apiVersionSelector(info.sink_->getApiVersion())), true); @@ -3609,7 +3609,7 @@ NetworkOPsImp::addAccountHistoryJob(SubAccountHistoryInfoWeak subInfo) auto send = [&](Json::Value const& jvObj, bool unsubscribe) -> bool { - if (auto sptr = subInfo.sinkWptr_.lock(); sptr) + if (auto sptr = subInfo.sinkWptr_.lock()) { sptr->send(jvObj, true); if (unsubscribe) @@ -3622,7 +3622,7 @@ NetworkOPsImp::addAccountHistoryJob(SubAccountHistoryInfoWeak subInfo) auto sendMultiApiJson = [&](MultiApiJson const& jvObj, bool unsubscribe) -> bool { - if (auto sptr = subInfo.sinkWptr_.lock(); sptr) + if (auto sptr = subInfo.sinkWptr_.lock()) { sptr->send( jvObj.select(apiVersionSelector(sptr->getApiVersion())), @@ -3802,9 +3802,8 @@ NetworkOPsImp::addAccountHistoryJob(SubAccountHistoryInfoWeak subInfo) transJson( stTxn, trR, true, curTxLedger, mRef, 2)}); - jvTx.set(jss::account_history_tx_index, txHistoryIndex); - txHistoryIndex -= 1; - + jvTx.set( + jss::account_history_tx_index, txHistoryIndex--); if (i + 1 == num_txns || txns[i + 1].first->getLedger() != tx->getLedger()) jvTx.set(jss::account_history_boundary, true); diff --git a/src/ripple/json/MultivarJson.h b/src/ripple/json/MultivarJson.h index 18bf407671b..c3ccc971104 100644 --- a/src/ripple/json/MultivarJson.h +++ b/src/ripple/json/MultivarJson.h @@ -48,6 +48,20 @@ struct MultivarJson for (auto& a : this->val) a[key] = v; } + + // Intentionally not using class enum here, MultivarJson is scope enough + enum IsMemberResult : int { none = 0, some, all }; + + [[nodiscard]] IsMemberResult + isMember(const char* key) const + { + int count = 0; + for (auto& a : this->val) + if (a.isMember(key)) + count += 1; + + return (count == 0 ? none : (count < size ? some : all)); + } }; // Wrapper for Json for all supported API versions. diff --git a/src/ripple/net/InfoSub.h b/src/ripple/net/InfoSub.h index 9934b1818db..092ba0c0035 100644 --- a/src/ripple/net/InfoSub.h +++ b/src/ripple/net/InfoSub.h @@ -246,7 +246,7 @@ class InfoSub : public CountedObject std::shared_ptr request_; std::uint64_t mSeq; hash_set accountHistorySubscriptions_; - unsigned int apiVersion = 1; + unsigned int apiVersion_ = 0; static int assign_id() diff --git a/src/ripple/net/impl/InfoSub.cpp b/src/ripple/net/impl/InfoSub.cpp index a9b0d954d26..5b37cf0759b 100644 --- a/src/ripple/net/impl/InfoSub.cpp +++ b/src/ripple/net/impl/InfoSub.cpp @@ -139,13 +139,14 @@ InfoSub::getRequest() void InfoSub::setApiVersion(unsigned int apiVersion) { - this->apiVersion = apiVersion; + apiVersion_ = apiVersion; } unsigned int InfoSub::getApiVersion() const noexcept { - return this->apiVersion; + assert(apiVersion_ > 0); + return apiVersion_; } } // namespace ripple diff --git a/src/ripple/rpc/impl/TransactionSign.cpp b/src/ripple/rpc/impl/TransactionSign.cpp index 86d015543f6..5dbfa49aef9 100644 --- a/src/ripple/rpc/impl/TransactionSign.cpp +++ b/src/ripple/rpc/impl/TransactionSign.cpp @@ -175,7 +175,7 @@ checkPayment( if (tx_json[jss::DeliverMax] != tx_json[jss::Amount]) return RPC::make_error( rpcINVALID_PARAMS, - "Cannot specify both 'Amount' and 'DeliverMax'"); + "Cannot specify differing 'Amount' and 'DeliverMax'"); } else tx_json[jss::Amount] = tx_json[jss::DeliverMax]; diff --git a/src/test/json/MultivarJson_test.cpp b/src/test/json/MultivarJson_test.cpp index 041e06ce6f7..f9a45c4cbe1 100644 --- a/src/test/json/MultivarJson_test.cpp +++ b/src/test/json/MultivarJson_test.cpp @@ -39,7 +39,7 @@ struct MultivarJson_test : beast::unit_test::suite static Json::Value const str1{string1}; static Json::Value const obj1{[]() { - Json::Value obj1; + Json::Value obj1(Json::objectValue); obj1["one"] = 1; return obj1; }()}; @@ -206,6 +206,43 @@ struct MultivarJson_test : beast::unit_test::suite }(x)); } + { + testcase("isMember"); + + // Well defined behaviour even if we have different types of members + BEAST_EXPECT(subject.isMember("foo") == decltype(subject)::none); + + auto const makeJson = [](const char* key, int val) { + Json::Value obj1(Json::objectValue); + obj1[key] = val; + return obj1; + }; + + { + // All variants have element "One", none have element "Two" + MultivarJson<2> const s1{ + {makeJson("One", 12), makeJson("One", 42)}}; + BEAST_EXPECT(s1.isMember("One") == decltype(s1)::all); + BEAST_EXPECT(s1.isMember("Two") == decltype(s1)::none); + } + + { + // Some variants have element "One" and some have "Two" + MultivarJson<2> const s2{ + {makeJson("One", 12), makeJson("Two", 42)}}; + BEAST_EXPECT(s2.isMember("One") == decltype(s2)::some); + BEAST_EXPECT(s2.isMember("Two") == decltype(s2)::some); + } + + { + // Not all variants have element "One", because last one is null + MultivarJson<3> const s3{ + {makeJson("One", 12), makeJson("One", 42), {}}}; + BEAST_EXPECT(s3.isMember("One") == decltype(s3)::some); + BEAST_EXPECT(s3.isMember("Two") == decltype(s3)::none); + } + } + { testcase("apiVersionSelector"); diff --git a/src/test/rpc/AccountTx_test.cpp b/src/test/rpc/AccountTx_test.cpp index 3416819414e..8c583ee1254 100644 --- a/src/test/rpc/AccountTx_test.cpp +++ b/src/test/rpc/AccountTx_test.cpp @@ -132,9 +132,10 @@ class AccountTx_test : public beast::unit_test::suite ((apiVersion > 1 && !j[jss::result][jss::transactions][1u][jss::tx].isMember( jss::Amount)) || - (j[jss::result][jss::transactions][1u][jss::tx][jss::Amount] == - j[jss::result][jss::transactions][1u][jss::tx] - [jss::DeliverMax])); + (apiVersion <= 1 && + j[jss::result][jss::transactions][1u][jss::tx][jss::Amount] == + j[jss::result][jss::transactions][1u][jss::tx] + [jss::DeliverMax])); }; auto noTxs = [](Json::Value const& j) { diff --git a/src/test/rpc/JSONRPC_test.cpp b/src/test/rpc/JSONRPC_test.cpp index dc81696fd9e..5d4c09ef8d1 100644 --- a/src/test/rpc/JSONRPC_test.cpp +++ b/src/test/rpc/JSONRPC_test.cpp @@ -2014,10 +2014,10 @@ static constexpr TxnTestData txnTestArray[] = { "TransactionType": "Payment" } })", - {{"Cannot specify both 'Amount' and 'DeliverMax'", - "Cannot specify both 'Amount' and 'DeliverMax'", - "Cannot specify both 'Amount' and 'DeliverMax'", - "Cannot specify both 'Amount' and 'DeliverMax'"}}}, + {{"Cannot specify differing 'Amount' and 'DeliverMax'", + "Cannot specify differing 'Amount' and 'DeliverMax'", + "Cannot specify differing 'Amount' and 'DeliverMax'", + "Cannot specify differing 'Amount' and 'DeliverMax'"}}}, }; diff --git a/src/test/rpc/Transaction_test.cpp b/src/test/rpc/Transaction_test.cpp index ea99409f67b..05d9da73963 100644 --- a/src/test/rpc/Transaction_test.cpp +++ b/src/test/rpc/Transaction_test.cpp @@ -702,7 +702,6 @@ class Transaction_test : public beast::unit_test::suite using std::to_string; const char* COMMAND = jss::tx.c_str(); - // const char* BINARY = jss::binary.c_str(); Env env{*this}; Account const alice{"alice"}; @@ -735,18 +734,20 @@ class Transaction_test : public beast::unit_test::suite memberIt++) { std::string const name = memberIt.memberName(); - BEAST_EXPECT(result[jss::result].isMember(name)); - auto const received = result[jss::result][name]; - std::ostringstream ssReceived; - ssReceived << received; - std::ostringstream ssExpected; - ssExpected << *memberIt; - BEAST_EXPECTS( - received == *memberIt, - "Transaction contains \n\"" + name + "\": " // - + ssReceived.str() // - + "but expected " // - + ssExpected.str()); + if (BEAST_EXPECT(result[jss::result].isMember(name))) + { + auto const received = result[jss::result][name]; + std::ostringstream ssReceived; + ssReceived << received; + std::ostringstream ssExpected; + ssExpected << *memberIt; + BEAST_EXPECTS( + received == *memberIt, + "Transaction contains \n\"" + name + "\": " // + + ssReceived.str() // + + "but expected " // + + ssExpected.str()); + } } } From 5b30a256c7ea6b078cac21b8d884753cec94115e Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Thu, 19 Oct 2023 23:11:50 +0100 Subject: [PATCH 16/21] Fill MultiApiJson in NetworkOPsImp::transJson --- src/ripple/app/misc/NetworkOPs.cpp | 57 ++++++++++++++++-------------- 1 file changed, 31 insertions(+), 26 deletions(-) diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 385ccc0abc7..4d2dcf9b82a 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -599,14 +599,13 @@ class NetworkOPsImp final : public NetworkOPs void processClusterTimer(); - Json::Value + MultiApiJson transJson( std::shared_ptr const& transaction, TER result, bool validated, std::shared_ptr const& ledger, - std::optional> meta, - unsigned int apiVersion); + std::optional> meta); void pubValidatedTransaction( @@ -2748,9 +2747,8 @@ NetworkOPsImp::pubProposedTransaction( std::shared_ptr const& transaction, TER result) { - MultiApiJson jvObj( - {transJson(transaction, result, false, ledger, std::nullopt, 1), - transJson(transaction, result, false, ledger, std::nullopt, 2)}); + MultiApiJson jvObj = + transJson(transaction, result, false, ledger, std::nullopt); { std::lock_guard sl(mSubLock); @@ -3088,14 +3086,13 @@ NetworkOPsImp::getLocalTxCount() // This routine should only be used to publish accepted or validated // transactions. -Json::Value +MultiApiJson NetworkOPsImp::transJson( std::shared_ptr const& transaction, TER result, bool validated, std::shared_ptr const& ledger, - std::optional> meta, - unsigned int apiVersion) + std::optional> meta) { Json::Value jvObj(Json::objectValue); std::string sToken; @@ -3105,8 +3102,6 @@ NetworkOPsImp::transJson( jvObj[jss::type] = "transaction"; jvObj[jss::transaction] = transaction->getJson(JsonOptions::none); - RPC::insertDeliverMax( - jvObj[jss::transaction], transaction->getTxnType(), apiVersion); if (meta) { @@ -3154,7 +3149,26 @@ NetworkOPsImp::transJson( } } - return jvObj; + MultiApiJson multiObj({jvObj, jvObj}); + static_assert(MultiApiJson::size == 2); + static_assert(RPC::apiMinimumSupportedVersion == 1); + for (unsigned apiVersion = 1, lastIndex = MultiApiJson::size; + apiVersion <= 2; + ++apiVersion) + { + unsigned const index = apiVersionSelector(apiVersion)(); + assert(index < MultiApiJson::size); + if (index != lastIndex) + { + RPC::insertDeliverMax( + multiObj.val[index][jss::transaction], + transaction->getTxnType(), + apiVersion); + lastIndex = index; + } + } + + return multiObj; } void @@ -3168,9 +3182,7 @@ NetworkOPsImp::pubValidatedTransaction( // Create two different Json objects, for different API versions auto const metaRef = std::ref(transaction.getMeta()); auto const trResult = transaction.getResult(); - MultiApiJson jvObj( - {transJson(stTxn, trResult, true, ledger, metaRef, 1), - transJson(stTxn, trResult, true, ledger, metaRef, 2)}); + MultiApiJson jvObj = transJson(stTxn, trResult, true, ledger, metaRef); { std::lock_guard sl(mSubLock); @@ -3315,9 +3327,7 @@ NetworkOPsImp::pubAccountTransaction( // Create two different Json objects, for different API versions auto const metaRef = std::ref(transaction.getMeta()); auto const trResult = transaction.getResult(); - MultiApiJson jvObj( - {transJson(stTxn, trResult, true, ledger, metaRef, 1), - transJson(stTxn, trResult, true, ledger, metaRef, 2)}); + MultiApiJson jvObj = transJson(stTxn, trResult, true, ledger, metaRef); for (InfoSub::ref isrListener : notify) { @@ -3397,10 +3407,7 @@ NetworkOPsImp::pubProposedAccountTransaction( if (!notify.empty() || !accountHistoryNotify.empty()) { // Create two different Json objects, for different API versions - MultiApiJson jvObj({ - transJson(tx, result, false, ledger, std::nullopt, 1), - transJson(tx, result, false, ledger, std::nullopt, 2), - }); + MultiApiJson jvObj = transJson(tx, result, false, ledger, std::nullopt); for (InfoSub::ref isrListener : notify) isrListener->send( @@ -3797,10 +3804,8 @@ NetworkOPsImp::addAccountHistoryJob(SubAccountHistoryInfoWeak subInfo) auto const mRef = std::ref(*meta); auto const trR = meta->getResultTER(); - MultiApiJson jvTx( - {transJson(stTxn, trR, true, curTxLedger, mRef, 1), - transJson( - stTxn, trR, true, curTxLedger, mRef, 2)}); + MultiApiJson jvTx = + transJson(stTxn, trR, true, curTxLedger, mRef); jvTx.set( jss::account_history_tx_index, txHistoryIndex--); From bd53535a6e45ba23cc8abff9be672140f2134572 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Fri, 20 Oct 2023 21:19:41 +0100 Subject: [PATCH 17/21] Minor test improvement --- src/test/json/MultivarJson_test.cpp | 6 +++--- src/test/rpc/TransactionEntry_test.cpp | 22 ++++++++++------------ 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/src/test/json/MultivarJson_test.cpp b/src/test/json/MultivarJson_test.cpp index f9a45c4cbe1..1658ea711bf 100644 --- a/src/test/json/MultivarJson_test.cpp +++ b/src/test/json/MultivarJson_test.cpp @@ -105,7 +105,7 @@ struct MultivarJson_test : beast::unit_test::suite static_assert([](auto&& v) { return requires { - v.select([]() -> std::size_t {}); + v.select([]() -> std::size_t { return 0; }); }; }(subject)); static_assert([](auto&& v) { @@ -117,7 +117,7 @@ struct MultivarJson_test : beast::unit_test::suite static_assert([](auto&& v) { return requires { - v.select([]() mutable -> std::size_t {}); + v.select([]() mutable -> std::size_t { return 0; }); }; }(subject)); @@ -137,7 +137,7 @@ struct MultivarJson_test : beast::unit_test::suite static_assert([](auto&& v) { return !requires { - v.select([]() -> bool {}); + v.select([]() -> bool { return false; }); }; }(subject)); } diff --git a/src/test/rpc/TransactionEntry_test.cpp b/src/test/rpc/TransactionEntry_test.cpp index 7e80ba00c58..60225f4621d 100644 --- a/src/test/rpc/TransactionEntry_test.cpp +++ b/src/test/rpc/TransactionEntry_test.cpp @@ -179,18 +179,16 @@ class TransactionEntry_test : public beast::unit_test::suite memberIt++) { auto const name = memberIt.memberName(); - BEAST_EXPECT(resIndex[jss::tx_json].isMember(name)); - auto const received = resIndex[jss::tx_json][name]; - std::ostringstream ssReceived; - ssReceived << received; - std::ostringstream ssExpected; - ssExpected << *memberIt; - BEAST_EXPECTS( - received == *memberIt, - txhash + " contains \n\"" + name + "\": " // - + ssReceived.str() // - + "but expected " // - + ssExpected.str()); + if (BEAST_EXPECT(resIndex[jss::tx_json].isMember(name))) + { + auto const received = resIndex[jss::tx_json][name]; + BEAST_EXPECTS( + received == *memberIt, + txhash + " contains \n\"" + name + "\": " // + + to_string(received) // + + " but expected " // + + to_string(expected)); + } } } From cf0798a0825ac0d33079ef03c7afa7ed98c597cf Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Fri, 20 Oct 2023 21:38:17 +0100 Subject: [PATCH 18/21] Add assert in MultivarJson::select --- src/ripple/json/MultivarJson.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/ripple/json/MultivarJson.h b/src/ripple/json/MultivarJson.h index c3ccc971104..72ec5c95324 100644 --- a/src/ripple/json/MultivarJson.h +++ b/src/ripple/json/MultivarJson.h @@ -23,6 +23,7 @@ #include #include +#include #include #include @@ -37,7 +38,9 @@ struct MultivarJson select(auto&& selector) const requires std::same_as { - return val[selector()]; + auto const index = selector(); + assert(index < size); + return val[index]; } void From efbf39a70c11617393bc04eb0d0851f315d11806 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Mon, 23 Oct 2023 12:51:05 +0000 Subject: [PATCH 19/21] Minor improvement in unit tests --- src/test/rpc/Transaction_test.cpp | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/test/rpc/Transaction_test.cpp b/src/test/rpc/Transaction_test.cpp index 05d9da73963..c16d7bbd004 100644 --- a/src/test/rpc/Transaction_test.cpp +++ b/src/test/rpc/Transaction_test.cpp @@ -737,16 +737,12 @@ class Transaction_test : public beast::unit_test::suite if (BEAST_EXPECT(result[jss::result].isMember(name))) { auto const received = result[jss::result][name]; - std::ostringstream ssReceived; - ssReceived << received; - std::ostringstream ssExpected; - ssExpected << *memberIt; BEAST_EXPECTS( received == *memberIt, "Transaction contains \n\"" + name + "\": " // - + ssReceived.str() // - + "but expected " // - + ssExpected.str()); + + to_string(received) // + + " but expected " // + + to_string(expected)); } } } From 70bd1b707765c55fd21aa9ddb1e1ba1aeaffea74 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Mon, 23 Oct 2023 12:52:10 +0000 Subject: [PATCH 20/21] Minor cleanup in include guards --- src/ripple/app/misc/DeliverMax.h | 4 ++-- src/ripple/json/MultivarJson.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ripple/app/misc/DeliverMax.h b/src/ripple/app/misc/DeliverMax.h index 44e9ffb833f..ddc20bdd7b4 100644 --- a/src/ripple/app/misc/DeliverMax.h +++ b/src/ripple/app/misc/DeliverMax.h @@ -17,8 +17,8 @@ */ //============================================================================== -#ifndef RIPPLE_RPC_DELIVERMAX_H_INCLUDED -#define RIPPLE_RPC_DELIVERMAX_H_INCLUDED +#ifndef RIPPLE_APP_MISC_DELIVERMAX_H_INCLUDED +#define RIPPLE_APP_MISC_DELIVERMAX_H_INCLUDED #include diff --git a/src/ripple/json/MultivarJson.h b/src/ripple/json/MultivarJson.h index 72ec5c95324..94d0090edc4 100644 --- a/src/ripple/json/MultivarJson.h +++ b/src/ripple/json/MultivarJson.h @@ -17,8 +17,8 @@ */ //============================================================================== -#ifndef RIPPLE_BASICS_MULTIVARJSON_H_INCLUDED -#define RIPPLE_BASICS_MULTIVARJSON_H_INCLUDED +#ifndef RIPPLE_JSON_MULTIVARJSON_H_INCLUDED +#define RIPPLE_JSON_MULTIVARJSON_H_INCLUDED #include From 5e7fa7ceb20865e88d32f5b8bab617f3069efc29 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Mon, 23 Oct 2023 13:30:21 +0000 Subject: [PATCH 21/21] Improve version loop and static asserts --- Builds/levelization/results/ordering.txt | 1 + src/ripple/app/misc/NetworkOPs.cpp | 13 +++++++++---- src/test/json/MultivarJson_test.cpp | 15 +++++++++++++++ 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/Builds/levelization/results/ordering.txt b/Builds/levelization/results/ordering.txt index 832b548e5de..5f50dc66118 100644 --- a/Builds/levelization/results/ordering.txt +++ b/Builds/levelization/results/ordering.txt @@ -135,6 +135,7 @@ test.csf > ripple.protocol test.csf > test.jtx test.json > ripple.beast test.json > ripple.json +test.json > ripple.rpc test.json > test.jtx test.jtx > ripple.app test.jtx > ripple.basics diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index e926f786ba1..a431b5562d3 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -3150,10 +3150,15 @@ NetworkOPsImp::transJson( } MultiApiJson multiObj({jvObj, jvObj}); - static_assert(MultiApiJson::size == 2); - static_assert(RPC::apiMinimumSupportedVersion == 1); - for (unsigned apiVersion = 1, lastIndex = MultiApiJson::size; - apiVersion <= 2; + // Minimum supported API version must match index 0 in MultiApiJson + static_assert(apiVersionSelector(RPC::apiMinimumSupportedVersion)() == 0); + // Beta API version must match last index in MultiApiJson + static_assert( + apiVersionSelector(RPC::apiBetaVersion)() + 1 // + == MultiApiJson::size); + for (unsigned apiVersion = RPC::apiMinimumSupportedVersion, + lastIndex = MultiApiJson::size; + apiVersion <= RPC::apiBetaVersion; ++apiVersion) { unsigned const index = apiVersionSelector(apiVersion)(); diff --git a/src/test/json/MultivarJson_test.cpp b/src/test/json/MultivarJson_test.cpp index 1658ea711bf..8cb3a49aff2 100644 --- a/src/test/json/MultivarJson_test.cpp +++ b/src/test/json/MultivarJson_test.cpp @@ -18,6 +18,7 @@ //============================================================================== #include +#include #include #include "ripple/beast/unit_test/suite.hpp" @@ -244,6 +245,7 @@ struct MultivarJson_test : beast::unit_test::suite } { + // NOTE It's fine to change this test when we change API versions testcase("apiVersionSelector"); static_assert(MultiApiJson::size == 2); @@ -269,6 +271,19 @@ struct MultivarJson_test : beast::unit_test::suite apiVersionSelector( std::numeric_limits::max())() == 1); } + + { + // There should be no reson to change this test + testcase("apiVersionSelector invariants"); + + static_assert( + apiVersionSelector(RPC::apiMinimumSupportedVersion)() == 0); + static_assert( + apiVersionSelector(RPC::apiBetaVersion)() + 1 // + == MultiApiJson::size); + + BEAST_EXPECT(MultiApiJson::size >= 1); + } } };