Add DeliverMax (alias for Amount) in Payment transactions

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
@@ -732,7 +734,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
@@ -857,7 +858,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
@@ -920,6 +920,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

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 <ripple/basics/MultivarJson.h>
+#include <ripple/json/MultivarJson.h>
 #include <ripple/net/InfoSub.h>
 
 #include <memory>

src/ripple/app/ledger/OrderBookDB.h

@@ -23,7 +23,7 @@
 #include <ripple/app/ledger/AcceptedLedgerTx.h>
 #include <ripple/app/ledger/BookListeners.h>
 #include <ripple/app/main/Application.h>
-#include <ripple/basics/MultivarJson.h>
+#include <ripple/json/MultivarJson.h>
 
 #include <mutex>
 

src/ripple/app/ledger/impl/LedgerToJson.cpp

@@ -19,11 +19,11 @@
 
 #include <ripple/app/ledger/LedgerToJson.h>
 #include <ripple/app/main/Application.h>
+#include <ripple/app/misc/DeliverMax.h>
 #include <ripple/app/misc/TxQ.h>
 #include <ripple/basics/base_uint.h>
 #include <ripple/core/Pg.h>
 #include <ripple/rpc/Context.h>
-#include <ripple/rpc/DeliverMax.h>
 #include <ripple/rpc/DeliveredAmount.h>
 
 namespace ripple {

src/ripple/rpc/DeliverMax.h → 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.

src/ripple/app/misc/NetworkOPs.cpp

@@ -29,6 +29,7 @@
 #include <ripple/app/ledger/TransactionMaster.h>
 #include <ripple/app/main/LoadManager.h>
 #include <ripple/app/misc/AmendmentTable.h>
+#include <ripple/app/misc/DeliverMax.h>
 #include <ripple/app/misc/HashRouter.h>
 #include <ripple/app/misc/LoadFeeTrack.h>
 #include <ripple/app/misc/NetworkOPs.h>
@@ -41,7 +42,6 @@
 #include <ripple/app/rdb/backend/SQLiteDatabase.h>
 #include <ripple/app/reporting/ReportingETL.h>
 #include <ripple/app/tx/apply.h>
-#include <ripple/basics/MultivarJson.h>
 #include <ripple/basics/PerfLog.h>
 #include <ripple/basics/SubmitSync.h>
 #include <ripple/basics/UptimeClock.h>
@@ -53,6 +53,7 @@
 #include <ripple/consensus/ConsensusParms.h>
 #include <ripple/crypto/RFC1751.h>
 #include <ripple/crypto/csprng.h>
+#include <ripple/json/MultivarJson.h>
 #include <ripple/json/to_string.h>
 #include <ripple/net/RPCErr.h>
 #include <ripple/nodestore/DatabaseShard.h>
@@ -65,7 +66,6 @@
 #include <ripple/resource/Fees.h>
 #include <ripple/resource/ResourceManager.h>
 #include <ripple/rpc/BookChanges.h>
-#include <ripple/rpc/DeliverMax.h>
 #include <ripple/rpc/DeliveredAmount.h>
 #include <ripple/rpc/ServerHandler.h>
 #include <ripple/rpc/impl/RPCHelpers.h>
@@ -599,14 +599,13 @@ class NetworkOPsImp final : public NetworkOPs
     void
     processClusterTimer();
 
-    Json::Value
+    MultiApiJson
     transJson(
         std::shared_ptr<STTx const> const& transaction,
         TER result,
-        std::shared_ptr<ReadView const> const& ledger,
         bool validated,
-        std::optional<std::reference_wrapper<TxMeta const>> meta,
-        unsigned int apiVersion);
+        std::shared_ptr<ReadView const> const& ledger,
+        std::optional<std::reference_wrapper<TxMeta const>> meta);
 
     void
     pubValidatedTransaction(
@@ -2748,9 +2747,8 @@ NetworkOPsImp::pubProposedTransaction(
     std::shared_ptr<STTx const> const& transaction,
     TER result)
 {
-    MultiApiJson jvObj(
-        {transJson(transaction, result, ledger, false, std::nullopt, 1),
-         transJson(transaction, result, ledger, false, 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<STTx const> const& transaction,
     TER result,
-    std::shared_ptr<ReadView const> const& ledger,
     bool validated,
-    std::optional<std::reference_wrapper<TxMeta const>> meta,
-    unsigned int apiVersion)
+    std::shared_ptr<ReadView const> const& ledger,
+    std::optional<std::reference_wrapper<TxMeta const>> 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, ledger, true, metaRef, 1),
-         transJson(stTxn, trResult, ledger, true, 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, ledger, true, metaRef, 1),
-             transJson(stTxn, trResult, ledger, true, metaRef, 2)});
+        MultiApiJson jvObj = transJson(stTxn, trResult, true, ledger, metaRef);
 
         for (InfoSub::ref isrListener : notify)
         {
@@ -3329,16 +3339,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())),
@@ -3397,25 +3407,22 @@ NetworkOPsImp::pubProposedAccountTransaction(
     if (!notify.empty() || !accountHistoryNotify.empty())
     {
         // 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),
-        });
+        MultiApiJson jvObj = transJson(tx, result, false, ledger, std::nullopt);
 
         for (InfoSub::ref isrListener : notify)
             isrListener->send(
                 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 +3616,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 +3629,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())),
@@ -3797,14 +3804,11 @@ 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, curTxLedger, true, mRef, 2)});
-
-                        jvTx.set(jss::account_history_tx_index, txHistoryIndex);
-                        txHistoryIndex -= 1;
+                        MultiApiJson jvTx =
+                            transJson(stTxn, trR, true, curTxLedger, mRef);
 
+                        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);

src/ripple/rpc/impl/DeliverMax.cpp → src/ripple/app/misc/impl/DeliverMax.cpp

@@ -17,8 +17,9 @@
 */
 //==============================================================================
 
+#include <ripple/app/misc/DeliverMax.h>
+
 #include <ripple/protocol/jss.h>
-#include <ripple/rpc/DeliverMax.h>
 
 namespace ripple {
 namespace RPC {

src/ripple/basics/MultivarJson.h → src/ripple/json/MultivarJson.h

@@ -48,11 +48,50 @@ 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.
 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<std::size_t>(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<std::size_t>(apiVersion > 2)`
+
+*/
+
 // Helper to create appropriate selector for indexing MultiApiJson by apiVersion
 constexpr auto
 apiVersionSelector(unsigned int apiVersion) noexcept

src/ripple/net/InfoSub.h

@@ -246,7 +246,7 @@ class InfoSub : public CountedObject<InfoSub>
     std::shared_ptr<InfoSubRequest> request_;
     std::uint64_t mSeq;
     hash_set<AccountID> accountHistorySubscriptions_;
-    unsigned int apiVersion = 1;
+    unsigned int apiVersion_ = 0;
 
     static int
     assign_id()