Unify JSON serialization format of transactions (#4775)

* Remove include <ranges>

* Formatting fix

* Output for subscriptions

* Output from sign, submit etc.

* Output from ledger

* Output from account_tx

* Output from transaction_entry

* Output from tx

* Store close_time_iso in API v2 output

* Add small APIv2 unit test for subscribe

* Add unit test for transaction_entry

* Add unit test for tx

* Remove inLedger from API version 2

* Set ledger_hash and ledger_index

* Move isValidated from RPCHelpers to LedgerMaster

* Store closeTime in LedgerFill

* Time formatting fix

* additional tests for Subscribe unit tests

* Improved comments

* Rename mInLedger to mLedgerIndex

* Minor fixes

* Set ledger_hash on closed ledger, even if not validated

* Update API-CHANGELOG.md

* Add ledger_hash, ledger_index to transaction_entry

* Fix validated and close_time_iso in account_tx

* Fix typos

* Improve getJson for Transaction and STTx

* Minor improvements

* Replace class enum JsonOptions with struct

We may consider turning this into a general-purpose template and using it elsewhere

* simplify the extraction of transactionID from Transaction object

* Remove obsolete comments

* Unconditionally set validated in account_tx output

* Minor improvements

* Minor fixes

---------

Co-authored-by: Chenna Keshava <ckeshavabs@gmail.com>

src/ripple/rpc/handlers/AMMInfo.cpp

@@ -16,6 +16,7 @@
     OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 */
 //==============================================================================
+#include <ripple/app/ledger/LedgerMaster.h>
 #include <ripple/app/misc/AMMUtils.h>
 #include <ripple/json/json_value.h>
 #include <ripple/ledger/ReadView.h>
@@ -66,7 +67,7 @@ to_iso8601(NetClock::time_point tp)
     return date::format(
         "%Y-%Om-%dT%H:%M:%OS%z",
         date::sys_time<system_clock::duration>(
-            system_clock::time_point{tp.time_since_epoch() + 946684800s}));
+            system_clock::time_point{tp.time_since_epoch() + epoch_offset}));
 }
 
 Json::Value
@@ -244,8 +245,7 @@ doAMMInfo(RPC::JsonContext& context)
     if (!result.isMember(jss::ledger_index) &&
         !result.isMember(jss::ledger_hash))
         result[jss::ledger_current_index] = ledger->info().seq;
-    result[jss::validated] =
-        RPC::isValidated(context.ledgerMaster, *ledger, context.app);
+    result[jss::validated] = context.ledgerMaster.isValidated(*ledger);
 
     return result;
 }

src/ripple/rpc/handlers/AccountTx.cpp

@@ -37,7 +37,6 @@
 #include <ripple/rpc/Context.h>
 #include <ripple/rpc/DeliveredAmount.h>
 #include <ripple/rpc/Role.h>
-#include <ripple/rpc/impl/RPCHelpers.h>
 
 #include <grpcpp/grpcpp.h>
 
@@ -195,8 +194,8 @@ getLedgerRange(
                         return status;
                     }
 
-                    bool validated = RPC::isValidated(
-                        context.ledgerMaster, *ledgerView, context.app);
+                    bool validated =
+                        context.ledgerMaster.isValidated(*ledgerView);
 
                     if (!validated || ledgerView->info().seq > uValidatedMax ||
                         ledgerView->info().seq < uValidatedMin)
@@ -320,25 +319,51 @@ populateJsonResponse(
         if (auto txnsData = std::get_if<TxnsData>(&result.transactions))
         {
             assert(!args.binary);
+
             for (auto const& [txn, txnMeta] : *txnsData)
             {
                 if (txn)
                 {
                     Json::Value& jvObj = jvTxns.append(Json::objectValue);
+                    jvObj[jss::validated] = true;
+
+                    auto const json_tx =
+                        (context.apiVersion > 1 ? jss::tx_json : jss::tx);
+                    if (context.apiVersion > 1)
+                    {
+                        jvObj[json_tx] = txn->getJson(
+                            JsonOptions::include_date |
+                                JsonOptions::disable_API_prior_V2,
+                            false);
+                        jvObj[jss::hash] = to_string(txn->getID());
+                        jvObj[jss::ledger_index] = txn->getLedger();
+                        jvObj[jss::ledger_hash] =
+                            to_string(context.ledgerMaster.getHashBySeq(
+                                txn->getLedger()));
+
+                        if (auto closeTime =
+                                context.ledgerMaster.getCloseTimeBySeq(
+                                    txn->getLedger()))
+                            jvObj[jss::close_time_iso] =
+                                to_string_iso(*closeTime);
+                    }
+                    else
+                        jvObj[json_tx] =
+                            txn->getJson(JsonOptions::include_date);
 
-                    jvObj[jss::tx] = txn->getJson(JsonOptions::include_date);
                     auto const& sttx = txn->getSTransaction();
                     RPC::insertDeliverMax(
-                        jvObj[jss::tx], sttx->getTxnType(), context.apiVersion);
+                        jvObj[json_tx], sttx->getTxnType(), context.apiVersion);
                     if (txnMeta)
                     {
                         jvObj[jss::meta] =
                             txnMeta->getJson(JsonOptions::include_date);
-                        jvObj[jss::validated] = true;
                         insertDeliveredAmount(
                             jvObj[jss::meta], context, txn, *txnMeta);
                         insertNFTSyntheticInJson(jvObj, sttx, *txnMeta);
                     }
+                    else
+                        assert(false && "Missing transaction medatata");
                 }
             }
         }
@@ -352,7 +377,9 @@ populateJsonResponse(
                 Json::Value& jvObj = jvTxns.append(Json::objectValue);
 
                 jvObj[jss::tx_blob] = strHex(std::get<0>(binaryData));
-                jvObj[jss::meta] = strHex(std::get<1>(binaryData));
+                auto const json_meta =
+                    (context.apiVersion > 1 ? jss::meta_blob : jss::meta);
+                jvObj[json_meta] = strHex(std::get<1>(binaryData));
                 jvObj[jss::ledger_index] = std::get<2>(binaryData);
                 jvObj[jss::validated] = true;
             }

src/ripple/rpc/handlers/LedgerHandler.cpp

@@ -27,7 +27,6 @@
 #include <ripple/rpc/GRPCHandlers.h>
 #include <ripple/rpc/Role.h>
 #include <ripple/rpc/handlers/LedgerHandler.h>
-#include <ripple/rpc/impl/RPCHelpers.h>
 
 namespace ripple {
 namespace RPC {
@@ -301,8 +300,7 @@ doLedgerGrpc(RPC::GRPCContext<org::xrpl::rpc::v1::GetLedgerRequest>& context)
         response.set_skiplist_included(true);
     }
 
-    response.set_validated(
-        RPC::isValidated(context.ledgerMaster, *ledger, context.app));
+    response.set_validated(context.ledgerMaster.isValidated(*ledger));
 
     auto end = std::chrono::system_clock::now();
     auto duration =

src/ripple/rpc/handlers/SignFor.cpp

@@ -46,6 +46,7 @@ doSignFor(RPC::JsonContext& context)
 
     auto ret = RPC::transactionSignFor(
         context.params,
+        context.apiVersion,
         failType,
         context.role,
         context.ledgerMaster.getValidatedLedgerAge(),

src/ripple/rpc/handlers/SignHandler.cpp

@@ -45,6 +45,7 @@ doSign(RPC::JsonContext& context)
 
     auto ret = RPC::transactionSign(
         context.params,
+        context.apiVersion,
         failType,
         context.role,
         context.ledgerMaster.getValidatedLedgerAge(),

src/ripple/rpc/handlers/Submit.cpp

@@ -63,6 +63,7 @@ doSubmit(RPC::JsonContext& context)
 
         auto ret = RPC::transactionSubmit(
             context.params,
+            context.apiVersion,
             failType,
             context.role,
             context.ledgerMaster.getValidatedLedgerAge(),

src/ripple/rpc/handlers/SubmitMultiSigned.cpp

@@ -45,6 +45,7 @@ doSubmitMultiSigned(RPC::JsonContext& context)
 
     return RPC::transactionSubmitMultiSigned(
         context.params,
+        context.apiVersion,
         failType,
         context.role,
         context.ledgerMaster.getValidatedLedgerAge(),

src/ripple/rpc/handlers/TransactionEntry.cpp

@@ -17,6 +17,7 @@
 */
 //==============================================================================
 
+#include <ripple/app/ledger/LedgerMaster.h>
 #include <ripple/app/main/Application.h>
 #include <ripple/app/misc/DeliverMax.h>
 #include <ripple/ledger/ReadView.h>
@@ -71,11 +72,39 @@ doTransactionEntry(RPC::JsonContext& context)
         }
         else
         {
-            jvResult[jss::tx_json] = sttx->getJson(JsonOptions::none);
+            if (context.apiVersion > 1)
+            {
+                jvResult[jss::tx_json] =
+                    sttx->getJson(JsonOptions::disable_API_prior_V2);
+                jvResult[jss::hash] = to_string(sttx->getTransactionID());
+
+                if (!lpLedger->open())
+                    jvResult[jss::ledger_hash] = to_string(
+                        context.ledgerMaster.getHashBySeq(lpLedger->seq()));
+
+                bool const validated =
+                    context.ledgerMaster.isValidated(*lpLedger);
+
+                jvResult[jss::validated] = validated;
+                if (validated)
+                {
+                    jvResult[jss::ledger_index] = lpLedger->seq();
+                    if (auto closeTime = context.ledgerMaster.getCloseTimeBySeq(
+                            lpLedger->seq()))
+                        jvResult[jss::close_time_iso] =
+                            to_string_iso(*closeTime);
+                }
+            }
+            else
+                jvResult[jss::tx_json] = sttx->getJson(JsonOptions::none);
+
             RPC::insertDeliverMax(
                 jvResult[jss::tx_json], sttx->getTxnType(), context.apiVersion);
+
+            auto const json_meta =
+                (context.apiVersion > 1 ? jss::meta : jss::metadata);
             if (stobj)
-                jvResult[jss::metadata] = stobj->getJson(JsonOptions::none);
+                jvResult[json_meta] = stobj->getJson(JsonOptions::none);
             // 'accounts'
             // 'engine_...'
             // 'ledger_...'

src/ripple/rpc/handlers/Tx.cpp

@@ -22,6 +22,7 @@
 #include <ripple/app/misc/DeliverMax.h>
 #include <ripple/app/misc/NetworkOPs.h>
 #include <ripple/app/misc/Transaction.h>
+#include <ripple/app/rdb/RelationalDatabase.h>
 #include <ripple/basics/ToString.h>
 #include <ripple/net/RPCErr.h>
 #include <ripple/protocol/ErrorCodes.h>
@@ -32,6 +33,7 @@
 #include <ripple/rpc/DeliveredAmount.h>
 #include <ripple/rpc/GRPCHandlers.h>
 #include <ripple/rpc/impl/RPCHelpers.h>
+
 #include <charconv>
 #include <regex>
 
@@ -55,6 +57,8 @@ struct TxResult
     std::variant<std::shared_ptr<TxMeta>, Blob> meta;
     bool validated = false;
     std::optional<std::string> ctid;
+    std::optional<NetClock::time_point> closeTime;
+    std::optional<uint256> ledgerHash;
     TxSearched searchedAll;
 };
 
@@ -140,6 +144,13 @@ doTxPostgres(RPC::Context& context, TxArgs const& args)
                     *(args.hash), res.txn->getLedger(), *meta);
             }
             res.validated = true;
+
+            auto const ledgerInfo =
+                context.app.getRelationalDatabase().getLedgerInfoByIndex(
+                    locator.getLedgerSequence());
+            res.closeTime = ledgerInfo->closeTime;
+            res.ledgerHash = ledgerInfo->hash;
+
             return {res, rpcSUCCESS};
         }
         else
@@ -257,6 +268,9 @@ doTxHelp(RPC::Context& context, TxArgs args)
     std::shared_ptr<Ledger const> ledger =
         context.ledgerMaster.getLedgerBySeq(txn->getLedger());
 
+    if (ledger && !ledger->open())
+        result.ledgerHash = ledger->info().hash;
+
     if (ledger && meta)
     {
         if (args.binary)
@@ -269,6 +283,9 @@ doTxHelp(RPC::Context& context, TxArgs args)
         }
         result.validated = isValidated(
             context.ledgerMaster, ledger->info().seq, ledger->info().hash);
+        if (result.validated)
+            result.closeTime =
+                context.ledgerMaster.getCloseTimeBySeq(txn->getLedger());
 
         // compute outgoing CTID
         uint32_t lgrSeq = ledger->info().seq;
@@ -311,17 +328,52 @@ populateJsonResponse(
     // no errors
     else if (result.txn)
     {
-        response = result.txn->getJson(JsonOptions::include_date, args.binary);
         auto const& sttx = result.txn->getSTransaction();
-        if (!args.binary)
-            RPC::insertDeliverMax(
-                response, sttx->getTxnType(), context.apiVersion);
+        if (context.apiVersion > 1)
+        {
+            constexpr auto optionsJson =
+                JsonOptions::include_date | JsonOptions::disable_API_prior_V2;
+            if (args.binary)
+                response[jss::tx_blob] = result.txn->getJson(optionsJson, true);
+            else
+            {
+                response[jss::tx_json] = result.txn->getJson(optionsJson);
+                RPC::insertDeliverMax(
+                    response[jss::tx_json],
+                    sttx->getTxnType(),
+                    context.apiVersion);
+            }
+
+            // Note, result.ledgerHash is only set in a closed or validated
+            // ledger - as seen in `doTxHelp` and `doTxPostgres`
+            if (result.ledgerHash)
+                response[jss::ledger_hash] = to_string(*result.ledgerHash);
+
+            response[jss::hash] = to_string(result.txn->getID());
+            if (result.validated)
+            {
+                response[jss::ledger_index] = result.txn->getLedger();
+                if (result.closeTime)
+                    response[jss::close_time_iso] =
+                        to_string_iso(*result.closeTime);
+            }
+        }
+        else
+        {
+            response =
+                result.txn->getJson(JsonOptions::include_date, args.binary);
+            if (!args.binary)
+                RPC::insertDeliverMax(
+                    response, sttx->getTxnType(), context.apiVersion);
+        }
 
         // populate binary metadata
         if (auto blob = std::get_if<Blob>(&result.meta))
         {
             assert(args.binary);
-            response[jss::meta] = strHex(makeSlice(*blob));
+            auto json_meta =
+                (context.apiVersion > 1 ? jss::meta_blob : jss::meta);
+            response[json_meta] = strHex(makeSlice(*blob));
         }
         // populate meta data
         else if (auto m = std::get_if<std::shared_ptr<TxMeta>>(&result.meta))

src/ripple/rpc/impl/RPCHelpers.cpp

@@ -600,59 +600,6 @@ getLedger<>(
 template Status
 getLedger<>(std::shared_ptr<ReadView const>&, uint256 const&, Context&);
 
-bool
-isValidated(
-    LedgerMaster& ledgerMaster,
-    ReadView const& ledger,
-    Application& app)
-{
-    if (app.config().reporting())
-        return true;
-
-    if (ledger.open())
-        return false;
-
-    if (ledger.info().validated)
-        return true;
-
-    auto 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 hash =
-            ledgerMaster.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
-                    ledgerMaster.clearLedger(seq);
-                }
-            }
-            return false;
-        }
-    }
-    catch (SHAMapMissingNode const& mn)
-    {
-        auto stream = app.journal("RPCHandler").warn();
-        JLOG(stream) << "Ledger #" << seq << ": " << mn.what();
-        return false;
-    }
-
-    // Mark ledger as validated to save time if we see it again.
-    ledger.info().validated = true;
-    return true;
-}
-
 // The previous version of the lookupLedger command would accept the
 // "ledger_index" argument as a string and silently treat it as a request to
 // return the current ledger which, while not strictly wrong, could cause a lot
@@ -693,8 +640,7 @@ lookupLedger(
         result[jss::ledger_current_index] = info.seq;
     }
 
-    result[jss::validated] =
-        isValidated(context.ledgerMaster, *ledger, context.app);
+    result[jss::validated] = context.ledgerMaster.isValidated(*ledger);
     return Status::OK;
 }
 

src/ripple/rpc/impl/RPCHelpers.h

@@ -168,12 +168,6 @@ ledgerFromSpecifier(
     org::xrpl::rpc::v1::LedgerSpecifier const& specifier,
     Context& context);
 
-bool
-isValidated(
-    LedgerMaster& ledgerMaster,
-    ReadView const& ledger,
-    Application& app);
-
 hash_set<AccountID>
 parseAccountIds(Json::Value const& jvArray);
 

src/ripple/rpc/impl/TransactionSign.cpp

@@ -645,12 +645,20 @@ transactionConstructImpl(
 }
 
 static Json::Value
-transactionFormatResultImpl(Transaction::pointer tpTrans)
+transactionFormatResultImpl(Transaction::pointer tpTrans, unsigned apiVersion)
 {
     Json::Value jvResult;
     try
     {
-        jvResult[jss::tx_json] = tpTrans->getJson(JsonOptions::none);
+        if (apiVersion > 1)
+        {
+            jvResult[jss::tx_json] =
+                tpTrans->getJson(JsonOptions::disable_API_prior_V2);
+            jvResult[jss::hash] = to_string(tpTrans->getID());
+        }
+        else
+            jvResult[jss::tx_json] = tpTrans->getJson(JsonOptions::none);
+
         jvResult[jss::tx_blob] =
             strHex(tpTrans->getSTransaction()->getSerializer().peekData());
 
@@ -777,6 +785,7 @@ checkFee(
 Json::Value
 transactionSign(
     Json::Value jvRequest,
+    unsigned apiVersion,
     NetworkOPs::FailHard failType,
     Role role,
     std::chrono::seconds validatedLedgerAge,
@@ -807,13 +816,14 @@ transactionSign(
     if (!txn.second)
         return txn.first;
 
-    return transactionFormatResultImpl(txn.second);
+    return transactionFormatResultImpl(txn.second, apiVersion);
 }
 
 /** Returns a Json::objectValue. */
 Json::Value
 transactionSubmit(
     Json::Value jvRequest,
+    unsigned apiVersion,
     NetworkOPs::FailHard failType,
     Role role,
     std::chrono::seconds validatedLedgerAge,
@@ -853,7 +863,7 @@ transactionSubmit(
             rpcINTERNAL, "Exception occurred during transaction submission.");
     }
 
-    return transactionFormatResultImpl(txn.second);
+    return transactionFormatResultImpl(txn.second, apiVersion);
 }
 
 namespace detail {
@@ -943,6 +953,7 @@ sortAndValidateSigners(STArray& signers, AccountID const& signingForID)
 Json::Value
 transactionSignFor(
     Json::Value jvRequest,
+    unsigned apiVersion,
     NetworkOPs::FailHard failType,
     Role role,
     std::chrono::seconds validatedLedgerAge,
@@ -1043,13 +1054,14 @@ transactionSignFor(
     if (!txn.second)
         return txn.first;
 
-    return transactionFormatResultImpl(txn.second);
+    return transactionFormatResultImpl(txn.second, apiVersion);
 }
 
 /** Returns a Json::objectValue. */
 Json::Value
 transactionSubmitMultiSigned(
     Json::Value jvRequest,
+    unsigned apiVersion,
     NetworkOPs::FailHard failType,
     Role role,
     std::chrono::seconds validatedLedgerAge,
@@ -1236,7 +1248,7 @@ transactionSubmitMultiSigned(
             rpcINTERNAL, "Exception occurred during transaction submission.");
     }
 
-    return transactionFormatResultImpl(txn.second);
+    return transactionFormatResultImpl(txn.second, apiVersion);
 }
 
 }  // namespace RPC

src/ripple/rpc/impl/TransactionSign.h

@@ -96,6 +96,7 @@ getProcessTxnFn(NetworkOPs& netOPs)
 Json::Value
 transactionSign(
     Json::Value params,  // Passed by value so it can be modified locally.
+    unsigned apiVersion,
     NetworkOPs::FailHard failType,
     Role role,
     std::chrono::seconds validatedLedgerAge,
@@ -105,6 +106,7 @@ transactionSign(
 Json::Value
 transactionSubmit(
     Json::Value params,  // Passed by value so it can be modified locally.
+    unsigned apiVersion,
     NetworkOPs::FailHard failType,
     Role role,
     std::chrono::seconds validatedLedgerAge,
@@ -116,6 +118,7 @@ transactionSubmit(
 Json::Value
 transactionSignFor(
     Json::Value params,  // Passed by value so it can be modified locally.
+    unsigned apiVersion,
     NetworkOPs::FailHard failType,
     Role role,
     std::chrono::seconds validatedLedgerAge,
@@ -125,6 +128,7 @@ transactionSignFor(
 Json::Value
 transactionSubmitMultiSigned(
     Json::Value params,  // Passed by value so it can be modified locally.
+    unsigned apiVersion,
     NetworkOPs::FailHard failType,
     Role role,
     std::chrono::seconds validatedLedgerAge,