Unify JSON serialization format of transactions (#4775)

Fix #4727 Squashed commit of the following: commit 53e384f Author: Bronek Kozicki <brok@incorrekt.com> Date: Fri Nov 3 13:37:29 2023 +0000 Minor improvements commit c1f2ea4 Merge: 942e209 056255e Author: Bronek Kozicki <brok@incorrekt.com> Date: Thu Nov 2 21:43:11 2023 +0000 Merge branch 'develop' into feature/unify_transaction_json commit 942e209 Author: Bronek Kozicki <brok@incorrekt.com> Date: Thu Nov 2 21:26:05 2023 +0000 Unconditionally set validated in account_tx output commit b5b1118 Author: Bronek Kozicki <brok@incorrekt.com> Date: Thu Nov 2 21:14:37 2023 +0000 Remove obsolete comments commit 7770bc5 Author: Chenna Keshava <ckeshavabs@gmail.com> Date: Thu Nov 2 13:16:44 2023 -0700 simplify the extraction of transactionID from Transaction object commit 94c16d8 Author: Bronek Kozicki <brok@incorrekt.com> Date: Thu Nov 2 20:26:09 2023 +0000 Replace class enum JsonOptions with struct We may consider turning this into a general-purpose template and using it elsewhere commit b2a8a2d Author: Bronek Kozicki <brok@incorrekt.com> Date: Thu Nov 2 17:46:54 2023 +0000 Minor improvements commit 95b6055 Author: Bronek Kozicki <brok@incorrekt.com> Date: Thu Nov 2 17:14:53 2023 +0000 Improve getJson for Transaction and STTx commit 17e6588 Author: Bronek Kozicki <brok@incorrekt.com> Date: Thu Nov 2 12:06:30 2023 +0000 Fix typos commit 5973390 Author: Bronek Kozicki <brok@incorrekt.com> Date: Wed Nov 1 15:55:02 2023 +0000 Fix validated and close_time_iso in account_tx commit cdb3384 Author: Bronek Kozicki <brok@incorrekt.com> Date: Tue Oct 31 17:27:57 2023 +0000 Add ledger_hash, ledger_index to transaction_entry commit 0d8673c Author: Bronek Kozicki <brok@incorrekt.com> Date: Tue Oct 31 17:12:53 2023 +0000 Update API-CHANGELOG.md commit 625c812 Merge: 54f1d60 3b624d8 Author: Bronek Kozicki <brok@incorrekt.com> Date: Mon Oct 30 21:32:35 2023 +0000 Merge branch 'develop' into feature/unify_transaction_json commit 54f1d60 Author: Bronek Kozicki <brok@incorrekt.com> Date: Mon Oct 30 21:19:40 2023 +0000 Set ledger_hash on closed ledger, even if not validated commit ef1276c Author: Bronek Kozicki <brok@incorrekt.com> Date: Mon Oct 30 20:59:58 2023 +0000 Minor fixes commit c81bece Author: Bronek Kozicki <brok@incorrekt.com> Date: Mon Oct 30 17:45:49 2023 +0000 Rename mInLedger to mLedgerIndex commit c477eb4 Author: Bronek Kozicki <brok@incorrekt.com> Date: Mon Oct 30 17:35:33 2023 +0000 Improved comments commit e386093 Author: Chenna Keshava <ckeshavabs@gmail.com> Date: Thu Oct 26 17:51:41 2023 -0700 additional tests for Subscribe unit tests commit 9f1d544 Author: Bronek Kozicki <brok@incorrekt.com> Date: Mon Oct 30 16:09:17 2023 +0000 Time formatting fix commit 4e92ee4 Author: Bronek Kozicki <brok@incorrekt.com> Date: Thu Oct 26 14:04:40 2023 +0000 Store closeTime in LedgerFill commit 1115cef Author: Bronek Kozicki <brok@incorrekt.com> Date: Thu Oct 26 13:22:26 2023 +0100 Move isValidated from RPCHelpers to LedgerMaster commit 2035bbc Author: Bronek Kozicki <brok@incorrekt.com> Date: Tue Oct 24 19:15:50 2023 +0000 Set ledger_hash and ledger_index commit bdf90e5 Author: Bronek Kozicki <brok@incorrekt.com> Date: Tue Oct 24 18:24:30 2023 +0000 Remove inLedger from API version 2 commit c2a3b52 Author: Bronek Kozicki <brok@incorrekt.com> Date: Tue Oct 24 17:39:45 2023 +0000 Add unit test for tx commit 1a96800 Author: Bronek Kozicki <brok@incorrekt.com> Date: Tue Oct 24 16:53:31 2023 +0000 Add unit test for transaction_entry commit 2b397c9 Author: Bronek Kozicki <brok@incorrekt.com> Date: Tue Oct 24 15:45:41 2023 +0000 Add small APIv2 unit test for subscribe commit 4d517e2 Author: Bronek Kozicki <brok@incorrekt.com> Date: Mon Oct 23 15:27:35 2023 +0000 Store close_time_iso in API v2 output commit 910f125 Author: Bronek Kozicki <brok@incorrekt.com> Date: Wed Oct 18 17:24:42 2023 +0100 Output from tx commit 0fb0517 Author: Bronek Kozicki <brok@incorrekt.com> Date: Wed Oct 18 17:24:26 2023 +0100 Output from transaction_entry commit e4ce8b1 Author: Bronek Kozicki <brok@incorrekt.com> Date: Wed Oct 18 17:24:08 2023 +0100 Output from account_tx commit 92635aa Author: Bronek Kozicki <brok@incorrekt.com> Date: Wed Oct 18 17:23:49 2023 +0100 Output from ledger commit 76eee5a Author: Bronek Kozicki <brok@incorrekt.com> Date: Wed Oct 18 17:23:16 2023 +0100 Output from sign, submit etc. commit aa6de60 Author: Bronek Kozicki <brok@incorrekt.com> Date: Mon Oct 23 18:20:21 2023 +0000 Output for subscriptions commit ee53c5d Author: Bronek Kozicki <brok@incorrekt.com> Date: Wed Oct 25 18:17:10 2023 +0000 Formatting fix commit 65346dc Author: Bronek Kozicki <brok@incorrekt.com> Date: Wed Oct 25 18:07:18 2023 +0000 Remove include <ranges>
XRPLF · Nov 3, 2023 · 1b081a5 · 1b081a5
1 parent b28286b
commit 1b081a5
Show file tree

Hide file tree

Showing 33 changed files with 667 additions and 163 deletions.
diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md
@@ -141,6 +141,31 @@ In API version 2, the following methods are no longer available:
 - `tx_history` - Instead, use other methods such as `account_tx` or `ledger` with the `transactions` field set to `true`.
 - `ledger_header` - Instead, use the `ledger` method.
 
+#### Modifications to JSON transaction element in V2
+
+In API version 2, JSON elements for transaction output have been changed and made consistent for all methods which output transactions:
+
+- JSON transaction element is named `tx_json`
+- Binary transaction element is named `tx_blob`
+- JSON transaction metadata element is named `meta`
+- Binary transaction metadata element is named `meta_blob`
+
+Additionally, these elements are now consistently available next to `tx_json` (i.e. sibling elements), where possible:
+
+- `hash` - Transaction ID. This data was stored inside transaction output in API version 1, but in API version 2 is a sibling element.
+- `ledger_index` - Ledger index (only set on validated ledgers)
+- `ledger_hash` - Ledger hash (only set on closed or validated ledgers)
+- `close_time_iso` - Ledger close time expressed in ISO 8601 time format (only set on validated ledgers)
+- `validated` - Bool element set to `true` if the transaction is in a validated ledger, otherwise `false`
+
+This change affects the following methods:
+
+- `tx` - Transaction data moved into element `tx_json` (was inline inside `result`) or, if binary output was requested, moved from `tx` to `tx_blob`. Renamed binary transaction metadata element (if it was requested) from `meta` to `meta_blob`. Changed location of `hash` and added new elements
+- `account_tx` - Renamed transaction element from `tx` to `tx_json`. Renamed binary transaction metadata element (if it was requested) from `meta` to `meta_blob`. Changed location of `hash` and added new elements
+- `transaction_entry` - Renamed transaction metadata element from `metadata` to `meta`. Changed location of `hash` and added new elements
+- `subscribe` - Renamed transaction element from `transaction` to `tx_json`. Changed location of `hash` and added new elements
+- `sign`, `sign_for`, `submit` and `submit_multisigned` - Changed location of `hash` element.
+
 #### Modifications to account_info response in V2
 
 - `signer_lists` is returned in the root of the response. Previously, in API version 1, it was nested under `account_data`. (https://github.com/XRPLF/rippled/pull/3770)

diff --git a/src/ripple/app/ledger/LedgerMaster.h b/src/ripple/app/ledger/LedgerMaster.h
@@ -215,6 +215,8 @@ class LedgerMaster : public AbstractFetchPackContainer
     void
     clearLedger(std::uint32_t seq);
     bool
+    isValidated(ReadView const& ledger);
+    bool
     getValidatedRange(std::uint32_t& minVal, std::uint32_t& maxVal);
     bool
     getFullValidatedRange(std::uint32_t& minVal, std::uint32_t& maxVal);

diff --git a/src/ripple/app/ledger/LedgerToJson.h b/src/ripple/app/ledger/LedgerToJson.h
@@ -21,8 +21,10 @@
 #define RIPPLE_APP_LEDGER_LEDGERTOJSON_H_INCLUDED
 
 #include <ripple/app/ledger/Ledger.h>
+#include <ripple/app/ledger/LedgerMaster.h>
 #include <ripple/app/misc/TxQ.h>
 #include <ripple/basics/StringUtilities.h>
+#include <ripple/basics/chrono.h>
 #include <ripple/json/Object.h>
 #include <ripple/protocol/STTx.h>
 #include <ripple/protocol/jss.h>
@@ -41,6 +43,8 @@ struct LedgerFill
         LedgerEntryType t = ltANY)
         : ledger(l), options(o), txQueue(std::move(q)), type(t), context(ctx)
     {
+        if (context)
+            closeTime = context->ledgerMaster.getCloseTimeBySeq(ledger.seq());
     }
 
     enum Options {
@@ -58,6 +62,7 @@ struct LedgerFill
     std::vector<TxQ::TxDetails> txQueue;
     LedgerEntryType type;
     RPC::Context* context;
+    std::optional<NetClock::time_point> closeTime;
 };
 
 /** Given a Ledger and options, fill a Json::Object or Json::Value with a

diff --git a/src/ripple/app/ledger/impl/LedgerMaster.cpp b/src/ripple/app/ledger/impl/LedgerMaster.cpp
@@ -603,6 +603,54 @@ LedgerMaster::clearLedger(std::uint32_t seq)
     mCompleteLedgers.erase(seq);
 }
 
+bool
+LedgerMaster::isValidated(ReadView const& ledger)
+{
+    if (app_.config().reporting())
+        return true;  // Reporting mode only supports validated ledger
+
+    if (ledger.open())
+        return false;
+
+    if (ledger.info().validated)
+        return true;
+
+    auto const seq = ledger.info().seq;
+    try
+    {
+        // Use the skip list in the last validated ledger to see if ledger
+        // comes before the last validated ledger (and thus has been
+        // validated).
+        auto const hash = walkHashBySeq(seq, InboundLedger::Reason::GENERIC);
+
+        if (!hash || ledger.info().hash != *hash)
+        {
+            // This ledger's hash is not the hash of the validated ledger
+            if (hash)
+            {
+                assert(hash->isNonZero());
+                uint256 valHash =
+                    app_.getRelationalDatabase().getHashByIndex(seq);
+                if (valHash == ledger.info().hash)
+                {
+                    // SQL database doesn't match ledger chain
+                    clearLedger(seq);
+                }
+            }
+            return false;
+        }
+    }
+    catch (SHAMapMissingNode const& mn)
+    {
+        JLOG(m_journal.warn()) << "Ledger #" << seq << ": " << mn.what();
+        return false;
+    }
+
+    // Mark ledger as validated to save time if we see it again.
+    ledger.info().validated = true;
+    return true;
+}
+
 // returns Ledgers we have all the nodes for
 bool
 LedgerMaster::getFullValidatedRange(

diff --git a/src/ripple/app/ledger/impl/LedgerToJson.cpp b/src/ripple/app/ledger/impl/LedgerToJson.cpp
@@ -17,12 +17,14 @@
 */
 //==============================================================================
 
+#include <ripple/app/ledger/LedgerMaster.h>
 #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/protocol/jss.h>
 #include <ripple/rpc/Context.h>
 #include <ripple/rpc/DeliveredAmount.h>
 
@@ -83,6 +85,7 @@ fillJson(Object& json, bool closed, LedgerInfo const& info, bool bFull)
         json[jss::close_time_human] = to_string(info.closeTime);
         if (!getCloseAgree(info))
             json[jss::close_time_estimated] = true;
+        json[jss::close_time_iso] = to_string_iso(info.closeTime);
     }
 }
 
@@ -118,8 +121,48 @@ fillJsonTx(
     if (bBinary)
     {
         txJson[jss::tx_blob] = serializeHex(*txn);
+        if (fill.context->apiVersion > 1)
+            txJson[jss::hash] = to_string(txn->getTransactionID());
+
+        auto const json_meta =
+            (fill.context->apiVersion > 1 ? jss::meta_blob : jss::meta);
         if (stMeta)
-            txJson[jss::meta] = serializeHex(*stMeta);
+            txJson[json_meta] = serializeHex(*stMeta);
+    }
+    else if (fill.context->apiVersion > 1)
+    {
+        copyFrom(
+            txJson[jss::tx_json],
+            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);
+
+        if (stMeta)
+        {
+            txJson[jss::meta] = stMeta->getJson(JsonOptions::none);
+
+            // If applicable, insert delivered amount
+            if (txnType == ttPAYMENT || txnType == ttCHECK_CASH)
+                RPC::insertDeliveredAmount(
+                    txJson[jss::meta],
+                    fill.ledger,
+                    txn,
+                    {txn->getTransactionID(), fill.ledger.seq(), *stMeta});
+        }
+
+        if (!fill.ledger.open())
+            txJson[jss::ledger_hash] = to_string(fill.ledger.info().hash);
+
+        const bool validated =
+            fill.context->ledgerMaster.isValidated(fill.ledger);
+        txJson[jss::validated] = validated;
+        if (validated)
+        {
+            txJson[jss::ledger_index] = to_string(fill.ledger.seq());
+            if (fill.closeTime)
+                txJson[jss::close_time_iso] = to_string_iso(*fill.closeTime);
+        }
     }
     else
     {
@@ -254,7 +297,11 @@ fillJsonQueue(Object& json, LedgerFill const& fill)
         if (tx.lastResult)
             txJson["last_result"] = transToken(*tx.lastResult);
 
-        txJson[jss::tx] = fillJsonTx(fill, bBinary, bExpanded, tx.txn, nullptr);
+        auto&& temp = fillJsonTx(fill, bBinary, bExpanded, tx.txn, nullptr);
+        if (fill.context->apiVersion > 1)
+            copyFrom(txJson, temp);
+        else
+            copyFrom(txJson[jss::tx], temp);
     }
 }
 

diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp
@@ -63,6 +63,7 @@
 #include <ripple/protocol/BuildInfo.h>
 #include <ripple/protocol/Feature.h>
 #include <ripple/protocol/STParsedJSON.h>
+#include <ripple/protocol/jss.h>
 #include <ripple/resource/Fees.h>
 #include <ripple/resource/ResourceManager.h>
 #include <ripple/rpc/BookChanges.h>
@@ -74,6 +75,7 @@
 
 #include <algorithm>
 #include <mutex>
+#include <optional>
 #include <string>
 #include <tuple>
 #include <unordered_map>
@@ -3096,7 +3098,11 @@ NetworkOPsImp::transJson(
     transResultInfo(result, sToken, sHuman);
 
     jvObj[jss::type] = "transaction";
-    jvObj[jss::transaction] = transaction->getJson(JsonOptions::none);
+    // 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.
+    jvObj[jss::transaction] =
+        transaction->getJson(JsonOptions::disable_API_prior_V2, false);
 
     if (meta)
     {
@@ -3105,13 +3111,16 @@ NetworkOPsImp::transJson(
             jvObj[jss::meta], *ledger, transaction, meta->get());
     }
 
+    if (!ledger->open())
+        jvObj[jss::ledger_hash] = to_string(ledger->info().hash);
+
     if (validated)
     {
         jvObj[jss::ledger_index] = ledger->info().seq;
-        jvObj[jss::ledger_hash] = to_string(ledger->info().hash);
         jvObj[jss::transaction][jss::date] =
             ledger->info().closeTime.time_since_epoch().count();
         jvObj[jss::validated] = true;
+        jvObj[jss::close_time_iso] = to_string_iso(ledger->info().closeTime);
 
         // WRITEME: Put the account next seq here
     }
@@ -3144,6 +3153,7 @@ NetworkOPsImp::transJson(
         }
     }
 
+    std::string const hash = to_string(transaction->getTransactionID());
     MultiApiJson multiObj({jvObj, jvObj});
     // Minimum supported API version must match index 0 in MultiApiJson
     static_assert(apiVersionSelector(RPC::apiMinimumSupportedVersion)() == 0);
@@ -3160,11 +3170,21 @@ NetworkOPsImp::transJson(
         assert(index < MultiApiJson::size);
         if (index != lastIndex)
         {
-            RPC::insertDeliverMax(
-                multiObj.val[index][jss::transaction],
-                transaction->getTxnType(),
-                apiVersion);
             lastIndex = index;
+
+            Json::Value& jvTx = multiObj.val[index];
+            RPC::insertDeliverMax(
+                jvTx[jss::transaction], transaction->getTxnType(), apiVersion);
+
+            if (apiVersion > 1)
+            {
+                jvTx[jss::tx_json] = jvTx.removeMember(jss::transaction);
+                jvTx[jss::hash] = hash;
+            }
+            else
+            {
+                jvTx[jss::transaction][jss::hash] = hash;
+            }
         }
     }
 

diff --git a/src/ripple/app/misc/Transaction.h b/src/ripple/app/misc/Transaction.h
@@ -24,10 +24,11 @@
 #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>
-#include <boost/optional.hpp>
+
 #include <optional>
 #include <variant>
 
@@ -99,13 +100,13 @@ class Transaction : public std::enable_shared_from_this<Transaction>,
     LedgerIndex
     getLedger() const
     {
-        return mInLedger;
+        return mLedgerIndex;
     }
 
     bool
     isValidated() const
     {
-        return mInLedger != 0;
+        return mLedgerIndex != 0;
     }
 
     TransStatus
@@ -138,7 +139,7 @@ class Transaction : public std::enable_shared_from_this<Transaction>,
     void
     setLedger(LedgerIndex ledger)
     {
-        mInLedger = ledger;
+        mLedgerIndex = ledger;
     }
 
     /**
@@ -386,7 +387,7 @@ class Transaction : public std::enable_shared_from_this<Transaction>,
 
     uint256 mTransactionID;
 
-    LedgerIndex mInLedger = 0;
+    LedgerIndex mLedgerIndex = 0;
     TransStatus mStatus = INVALID;
     TER mResult = temUNCERTAIN;
     bool mApplying = false;

diff --git a/src/ripple/app/misc/impl/Transaction.cpp b/src/ripple/app/misc/impl/Transaction.cpp
@@ -62,7 +62,7 @@ void
 Transaction::setStatus(TransStatus ts, std::uint32_t lseq)
 {
     mStatus = ts;
-    mInLedger = lseq;
+    mLedgerIndex = lseq;
 }
 
 TransStatus
@@ -167,16 +167,25 @@ Transaction::load(
 Json::Value
 Transaction::getJson(JsonOptions options, bool binary) const
 {
-    Json::Value ret(mTransaction->getJson(JsonOptions::none, binary));
+    // Note, we explicitly suppress `include_date` option here
+    Json::Value ret(
+        mTransaction->getJson(options & ~JsonOptions::include_date, binary));
 
-    if (mInLedger)
+    if (mLedgerIndex)
     {
-        ret[jss::inLedger] = mInLedger;  // Deprecated.
-        ret[jss::ledger_index] = mInLedger;
+        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(mInLedger);
+            auto ct = mApp.getLedgerMaster().getCloseTimeBySeq(mLedgerIndex);
             if (ct)
                 ret[jss::date] = ct->time_since_epoch().count();
         }