Improve getJson for Transaction and STTx

XRPLF · Nov 2, 2023 · 95b6055 · 95b6055
1 parent 17e6588
commit 95b6055
Show file tree

Hide file tree

Showing 11 changed files with 78 additions and 57 deletions.
diff --git a/src/ripple/app/ledger/impl/LedgerToJson.cpp b/src/ripple/app/ledger/impl/LedgerToJson.cpp
@@ -133,12 +133,10 @@ fillJsonTx(
     {
         if (fill.context->apiVersion > 1)
         {
-            std::string hash;
             copyFrom(
                 txJson[jss::tx_json],
-                txn->getJson(
-                    JsonOptions::none, false, {std::optional(std::ref(hash))}));
-            txJson[jss::hash] = hash;
+                txn->getJson(JsonOptions::disable_API_prior_V2, false));
+            txJson[jss::hash] = to_string(txn->getTransactionID());
             RPC::insertDeliverMax(
                 txJson[jss::tx_json], txnType, fill.context->apiVersion);
 

diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp
@@ -3106,9 +3106,9 @@ NetworkOPsImp::transJson(
     // NOTE jvObj which is not a finished object for either API version. After
     // it's populated, we need to finish it for a specific API version. This is
     // done in a loop, near the end of this function.
-    std::string hash = {};
+    std::string const hash = to_string(transaction->getTransactionID());
     jvObj[jss::transaction] =
-        transaction->getJson(JsonOptions::none, false, {std::ref(hash)});
+        transaction->getJson(JsonOptions::disable_API_prior_V2, false);
 
     if (meta)
     {

diff --git a/src/ripple/app/misc/Transaction.h b/src/ripple/app/misc/Transaction.h
@@ -24,6 +24,7 @@
 #include <ripple/beast/utility/Journal.h>
 #include <ripple/protocol/ErrorCodes.h>
 #include <ripple/protocol/Protocol.h>
+#include <ripple/protocol/STBase.h>
 #include <ripple/protocol/STTx.h>
 #include <ripple/protocol/TER.h>
 #include <ripple/protocol/TxMeta.h>
@@ -307,11 +308,7 @@ class Transaction : public std::enable_shared_from_this<Transaction>,
 
     /// Same as similar overload for STTx::getJson
     Json::Value
-    getJson(
-        JsonOptions options,
-        bool binary = false,
-        bool showInLedger = false,
-        std::optional<std::reference_wrapper<std::string>> hash = {}) const;
+    getJson(JsonOptions options, bool binary = false) const;
 
     // Information used to locate a transaction.
     // Contains a nodestore hash and ledger sequence pair if the transaction was

diff --git a/src/ripple/app/misc/impl/Transaction.cpp b/src/ripple/app/misc/impl/Transaction.cpp
@@ -165,22 +165,25 @@ Transaction::load(
 
 // options 1 to include the date of the transaction
 Json::Value
-Transaction::getJson(
-    JsonOptions options,
-    bool binary,
-    bool showInLedger,
-    std::optional<std::reference_wrapper<std::string>> hash) const
+Transaction::getJson(JsonOptions options, bool binary) const
 {
-    Json::Value ret(mTransaction->getJson(JsonOptions::none, binary, hash));
+    // Note, we explicitly suppress `include_date` option here
+    Json::Value ret(
+        mTransaction->getJson(options % JsonOptions::include_date, binary));
 
     if (mLedgerIndex)
     {
-        if (showInLedger)
-            ret[jss::inLedger] = mLedgerIndex;  // Deprecated.
+        if (!(options & JsonOptions::disable_API_prior_V2))
+        {
+            // Behaviour before API version 2
+            ret[jss::inLedger] = mLedgerIndex;
+        }
 
+        // TODO: disable_API_prior_V3 to disable output of both `date` and
+        // `ledger_index` elements (taking precedence over include_date)
         ret[jss::ledger_index] = mLedgerIndex;
 
-        if (options == JsonOptions::include_date)
+        if (options & JsonOptions::include_date)
         {
             auto ct = mApp.getLedgerMaster().getCloseTimeBySeq(mLedgerIndex);
             if (ct)

diff --git a/src/ripple/protocol/STBase.h b/src/ripple/protocol/STBase.h
@@ -31,7 +31,35 @@
 #include <utility>
 namespace ripple {
 
-enum class JsonOptions { none = 0, include_date = 1 };
+/// Note, should be treated as flags that can be | and &
+enum class JsonOptions : unsigned int {
+    none = 0,
+    include_date = 1,
+    disable_API_prior_V2 = 2
+};
+
+/// Return: JsonOptions union of lh and rh
+[[nodiscard]] constexpr JsonOptions
+operator|(JsonOptions lh, JsonOptions rh) noexcept
+{
+    return JsonOptions(
+        static_cast<unsigned int>(lh) | static_cast<unsigned int>(rh));
+}
+
+/// Return: JsonOptions similar to lh, but with rh disabled (i.e. "remainder")
+[[nodiscard]] constexpr JsonOptions
+operator%(JsonOptions lh, JsonOptions rh) noexcept
+{
+    return JsonOptions(
+        static_cast<unsigned int>(lh) & ~static_cast<unsigned int>(rh));
+}
+
+/// Return: true if lh contains rh
+[[nodiscard]] constexpr bool
+operator&(JsonOptions lh, JsonOptions rh) noexcept
+{
+    return (static_cast<unsigned int>(lh) & static_cast<unsigned int>(rh)) != 0;
+}
 
 namespace detail {
 class STVar;

diff --git a/src/ripple/protocol/STTx.h b/src/ripple/protocol/STTx.h
@@ -24,11 +24,13 @@
 #include <ripple/protocol/Feature.h>
 #include <ripple/protocol/PublicKey.h>
 #include <ripple/protocol/Rules.h>
+#include <ripple/protocol/STBase.h>
 #include <ripple/protocol/STObject.h>
 #include <ripple/protocol/SecretKey.h>
 #include <ripple/protocol/SeqProxy.h>
 #include <ripple/protocol/TxFormats.h>
 #include <boost/container/flat_set.hpp>
+
 #include <functional>
 
 namespace ripple {
@@ -116,10 +118,7 @@ class STTx final : public STObject, public CountedObject<STTx>
     nested jss::tx for binary hex; instead will return it as JSON string
     */
     Json::Value
-    getJson(
-        JsonOptions options,
-        bool binary,
-        std::optional<std::reference_wrapper<std::string>> hash = {}) const;
+    getJson(JsonOptions options, bool binary) const;
 
     void
     sign(PublicKey const& publicKey, SecretKey const& secretKey);

diff --git a/src/ripple/protocol/impl/STTx.cpp b/src/ripple/protocol/impl/STTx.cpp
@@ -34,6 +34,7 @@
 #include <ripple/protocol/UintTypes.h>
 #include <ripple/protocol/jss.h>
 #include <boost/format.hpp>
+
 #include <array>
 #include <memory>
 #include <type_traits>
@@ -234,13 +235,11 @@ Json::Value STTx::getJson(JsonOptions) const
 }
 
 Json::Value
-STTx::getJson(
-    JsonOptions options,
-    bool binary,
-    std::optional<std::reference_wrapper<std::string>> hash) const
+STTx::getJson(JsonOptions options, bool binary) const
 {
-    if (!hash)  // Old behaviour - default because `hash = {}` in declaration
+    if (!(options & JsonOptions::disable_API_prior_V2))
     {
+        // Behaviour before API version 2
         if (binary)
         {
             Json::Value ret;
@@ -252,8 +251,7 @@ STTx::getJson(
         return getJson(options);
     }
 
-    // Since `hash` is set, do not populate `hash` inside JSON output
-    hash->get() = to_string(getTransactionID());
+    // API version 2 behaviour
     if (binary)
     {
         Serializer s = STObject::getSerializer();

diff --git a/src/ripple/rpc/handlers/AccountTx.cpp b/src/ripple/rpc/handlers/AccountTx.cpp
@@ -329,13 +329,12 @@ populateJsonResponse(
                         (context.apiVersion > 1 ? jss::tx_json : jss::tx);
                     if (context.apiVersion > 1)
                     {
-                        std::string hash;
                         jvObj[json_tx] = txn->getJson(
-                            JsonOptions::include_date,
-                            false,
-                            false,
-                            {std::ref(hash)});
-                        jvObj[jss::hash] = hash;
+                            JsonOptions::include_date |
+                                JsonOptions::disable_API_prior_V2,
+                            false);
+                        jvObj[jss::hash] = to_string(
+                            txn->getSTransaction()->getTransactionID());
                         jvObj[jss::ledger_index] = txn->getLedger();
                         jvObj[jss::ledger_hash] =
                             to_string(context.ledgerMaster.getHashBySeq(
@@ -348,8 +347,8 @@ populateJsonResponse(
                                 to_string_iso(*closeTime);
                     }
                     else
-                        jvObj[json_tx] = txn->getJson(
-                            JsonOptions::include_date, false, true);
+                        jvObj[json_tx] =
+                            txn->getJson(JsonOptions::include_date);
 
                     auto const& sttx = txn->getSTransaction();
                     RPC::insertDeliverMax(

diff --git a/src/ripple/rpc/handlers/TransactionEntry.cpp b/src/ripple/rpc/handlers/TransactionEntry.cpp
@@ -74,10 +74,9 @@ doTransactionEntry(RPC::JsonContext& context)
         {
             if (context.apiVersion > 1)
             {
-                std::string hash;
                 jvResult[jss::tx_json] =
-                    sttx->getJson(JsonOptions::none, false, {std::ref(hash)});
-                jvResult[jss::hash] = hash;
+                    sttx->getJson(JsonOptions::disable_API_prior_V2);
+                jvResult[jss::hash] = to_string(sttx->getTransactionID());
 
                 if (!lpLedger->open())
                     jvResult[jss::ledger_hash] = to_string(

diff --git a/src/ripple/rpc/handlers/Tx.cpp b/src/ripple/rpc/handlers/Tx.cpp
@@ -33,6 +33,7 @@
 #include <ripple/rpc/DeliveredAmount.h>
 #include <ripple/rpc/GRPCHandlers.h>
 #include <ripple/rpc/impl/RPCHelpers.h>
+
 #include <charconv>
 #include <regex>
 
@@ -330,14 +331,13 @@ populateJsonResponse(
         auto const& sttx = result.txn->getSTransaction();
         if (context.apiVersion > 1)
         {
-            std::string hash;
+            constexpr auto optionsJson =
+                JsonOptions::include_date | JsonOptions::disable_API_prior_V2;
             if (args.binary)
-                response[jss::tx_blob] = result.txn->getJson(
-                    JsonOptions::include_date, true, false, {std::ref(hash)});
+                response[jss::tx_blob] = result.txn->getJson(optionsJson, true);
             else
             {
-                response[jss::tx_json] = result.txn->getJson(
-                    JsonOptions::include_date, false, false, {std::ref(hash)});
+                response[jss::tx_json] = result.txn->getJson(optionsJson);
                 RPC::insertDeliverMax(
                     response[jss::tx_json],
                     sttx->getTxnType(),
@@ -347,7 +347,8 @@ populateJsonResponse(
             if (result.ledgerHash)
                 response[jss::ledger_hash] = to_string(*result.ledgerHash);
 
-            response[jss::hash] = hash;
+            response[jss::hash] =
+                to_string(result.txn->getSTransaction()->getTransactionID());
             if (result.validated)
             {
                 response[jss::ledger_index] = result.txn->getLedger();
@@ -358,8 +359,8 @@ populateJsonResponse(
         }
         else
         {
-            response = result.txn->getJson(
-                JsonOptions::include_date, args.binary, true);
+            response =
+                result.txn->getJson(JsonOptions::include_date, args.binary);
             if (!args.binary)
                 RPC::insertDeliverMax(
                     response, sttx->getTxnType(), context.apiVersion);

diff --git a/src/ripple/rpc/impl/TransactionSign.cpp b/src/ripple/rpc/impl/TransactionSign.cpp
@@ -652,14 +652,13 @@ transactionFormatResultImpl(Transaction::pointer tpTrans, unsigned apiVersion)
     {
         if (apiVersion > 1)
         {
-            std::string hash = {};
-            jvResult[jss::tx_json] = tpTrans->getJson(
-                JsonOptions::none, false, false, {std::ref(hash)});
-            jvResult[jss::hash] = hash;
+            jvResult[jss::tx_json] =
+                tpTrans->getJson(JsonOptions::disable_API_prior_V2);
+            jvResult[jss::hash] =
+                to_string(tpTrans->getSTransaction()->getTransactionID());
         }
         else
-            jvResult[jss::tx_json] =
-                tpTrans->getJson(JsonOptions::none, false, true);
+            jvResult[jss::tx_json] = tpTrans->getJson(JsonOptions::none);
 
         jvResult[jss::tx_blob] =
             strHex(tpTrans->getSTransaction()->getSerializer().peekData());