APIv2: consistently return ledger_index as integer (XRPLF#4820)

Bronek · sophiax851 · commit aff75e7cf674 · 2024-06-12T14:43:28.000-07:00
For api_version 2, always return ledger_index as integer in JSON output.

api_version 1 retains prior behavior.
diff --git a/src/ripple/app/ledger/impl/LedgerToJson.cpp b/src/ripple/app/ledger/impl/LedgerToJson.cpp
@@ -27,6 +27,7 @@
 #include <ripple/protocol/jss.h>
 #include <ripple/rpc/Context.h>
 #include <ripple/rpc/DeliveredAmount.h>
+#include <ripple/rpc/impl/RPCHelpers.h>
 
 namespace ripple {
 
@@ -52,10 +53,17 @@ isBinary(LedgerFill const& fill)
 
 template <class Object>
 void
-fillJson(Object& json, bool closed, LedgerInfo const& info, bool bFull)
+fillJson(
+    Object& json,
+    bool closed,
+    LedgerInfo const& info,
+    bool bFull,
+    unsigned apiVersion)
 {
     json[jss::parent_hash] = to_string(info.parentHash);
-    json[jss::ledger_index] = to_string(info.seq);
+    json[jss::ledger_index] = (apiVersion > 1)
+        ? Json::Value(info.seq)
+        : Json::Value(std::to_string(info.seq));
 
     if (closed)
     {
@@ -159,7 +167,10 @@ fillJsonTx(
         txJson[jss::validated] = validated;
         if (validated)
         {
-            txJson[jss::ledger_index] = to_string(fill.ledger.seq());
+            auto const seq = fill.ledger.seq();
+            txJson[jss::ledger_index] = (fill.context->apiVersion > 1)
+                ? Json::Value(seq)
+                : Json::Value(std::to_string(seq));
             if (fill.closeTime)
                 txJson[jss::close_time_iso] = to_string_iso(*fill.closeTime);
         }
@@ -315,7 +326,13 @@ fillJson(Object& json, LedgerFill const& fill)
     if (isBinary(fill))
         fillJsonBinary(json, !fill.ledger.open(), fill.ledger.info());
     else
-        fillJson(json, !fill.ledger.open(), fill.ledger.info(), bFull);
+        fillJson(
+            json,
+            !fill.ledger.open(),
+            fill.ledger.info(),
+            bFull,
+            (fill.context ? fill.context->apiVersion
+                          : RPC::apiMaximumSupportedVersion));
 
     if (bFull || fill.options & LedgerFill::dumpTxrp)
         fillJsonTx(json, fill);
diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp
@@ -2181,8 +2181,10 @@ NetworkOPsImp::pubValidation(std::shared_ptr<STValidation> const& val)
         if (masterKey != signerPublic)
             jvObj[jss::master_key] = toBase58(TokenType::NodePublic, masterKey);
 
+        // NOTE *seq is a number, but old API versions used string. We replace
+        // number with a string using MultiApiJson near end of this function
         if (auto const seq = (*val)[~sfLedgerSequence])
-            jvObj[jss::ledger_index] = to_string(*seq);
+            jvObj[jss::ledger_index] = *seq;
 
         if (val->isFieldPresent(sfAmendments))
         {
@@ -2220,12 +2222,28 @@ NetworkOPsImp::pubValidation(std::shared_ptr<STValidation> const& val)
             reserveIncXRP && reserveIncXRP->native())
             jvObj[jss::reserve_inc] = reserveIncXRP->xrp().jsonClipped();
 
+        // NOTE Use MultiApiJson to publish two slightly different JSON objects
+        // for consumers supporting different API versions
+        MultiApiJson multiObj{jvObj};
+        visit<RPC::apiMinimumSupportedVersion, RPC::apiMaximumValidVersion>(
+            multiObj,  //
+            [](Json::Value& jvTx, unsigned int apiVersion) {
+                // Type conversion for older API versions to string
+                if (jvTx.isMember(jss::ledger_index) && apiVersion < 2)
+                {
+                    jvTx[jss::ledger_index] =
+                        std::to_string(jvTx[jss::ledger_index].asUInt());
+                }
+            });
+
         for (auto i = mStreamMaps[sValidations].begin();
              i != mStreamMaps[sValidations].end();)
         {
             if (auto p = i->second.lock())
             {
-                p->send(jvObj, true);
+                p->send(
+                    multiObj.select(apiVersionSelector(p->getApiVersion())),
+                    true);
                 ++i;
             }
             else
@@ -3159,25 +3177,10 @@ 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);
-    // Last valid (possibly beta) API ver. must match last index in MultiApiJson
-    static_assert(
-        apiVersionSelector(RPC::apiMaximumValidVersion)() + 1  //
-        == MultiApiJson::size);
-    for (unsigned apiVersion = RPC::apiMinimumSupportedVersion,
-                  lastIndex = MultiApiJson::size;
-         apiVersion <= RPC::apiMaximumValidVersion;
-         ++apiVersion)
-    {
-        unsigned const index = apiVersionSelector(apiVersion)();
-        assert(index < MultiApiJson::size);
-        if (index != lastIndex)
-        {
-            lastIndex = index;
-
-            Json::Value& jvTx = multiObj.val[index];
+    MultiApiJson multiObj{jvObj};
+    visit<RPC::apiMinimumSupportedVersion, RPC::apiMaximumValidVersion>(
+        multiObj,  //
+        [&](Json::Value& jvTx, unsigned int apiVersion) {
             RPC::insertDeliverMax(
                 jvTx[jss::transaction], transaction->getTxnType(), apiVersion);
 
@@ -3190,8 +3193,7 @@ NetworkOPsImp::transJson(
             {
                 jvTx[jss::transaction][jss::hash] = hash;
             }
-        }
-    }
+        });
 
     return multiObj;
 }
diff --git a/src/ripple/json/MultivarJson.h b/src/ripple/json/MultivarJson.h
@@ -26,14 +26,24 @@
 #include <cassert>
 #include <concepts>
 #include <cstdlib>
+#include <type_traits>
+#include <utility>
 
 namespace ripple {
 template <std::size_t Size>
 struct MultivarJson
 {
-    std::array<Json::Value, Size> val;
+    std::array<Json::Value, Size> val = {};
     constexpr static std::size_t size = Size;
 
+    explicit MultivarJson(Json::Value const& init = {})
+    {
+        if (init == Json::Value{})
+            return;  // All elements are already default-initialized
+        for (auto& v : val)
+            v = init;
+    }
+
     Json::Value const&
     select(auto&& selector) const
         requires std::same_as<std::size_t, decltype(selector())>
@@ -68,7 +78,7 @@ struct MultivarJson
 };
 
 // Wrapper for Json for all supported API versions.
-using MultiApiJson = MultivarJson<2>;
+using MultiApiJson = MultivarJson<3>;
 
 /*
 
@@ -78,21 +88,17 @@ 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
+prepare 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)`
 
+Such hypothetical change should correspond with change in RPCHelpers.h
+`apiMinimumSupportedVersion = 2;`
+
 */
 
 // Helper to create appropriate selector for indexing MultiApiJson by apiVersion
@@ -101,12 +107,44 @@ apiVersionSelector(unsigned int apiVersion) noexcept
 {
     return [apiVersion]() constexpr
     {
-        // apiVersion <= 1 returns 0
-        // apiVersion > 1  returns 1
-        return static_cast<std::size_t>(apiVersion > 1);
+        return static_cast<std::size_t>(
+            apiVersion <= 1         //
+                ? 0                 //
+                : (apiVersion <= 2  //
+                       ? 1          //
+                       : 2));
     };
 }
 
+// Helper to execute a callback for every version. Want both min and max version
+// provided explicitly, so user will know to do update `size` when they change
+template <
+    unsigned int minVer,
+    unsigned int maxVer,
+    std::size_t size,
+    typename Fn>
+    requires                                       //
+    (maxVer >= minVer) &&                          //
+    (size == maxVer + 1 - minVer) &&               //
+    (apiVersionSelector(minVer)() == 0) &&         //
+    (apiVersionSelector(maxVer)() + 1 == size) &&  //
+    requires(Json::Value& json, Fn fn)
+{
+    fn(json, static_cast<unsigned int>(1));
+}
+void
+visit(MultivarJson<size>& json, Fn fn)
+{
+    [&]<std::size_t... offset>(std::index_sequence<offset...>)
+    {
+        static_assert(((apiVersionSelector(minVer + offset)() >= 0) && ...));
+        static_assert(((apiVersionSelector(minVer + offset)() < size) && ...));
+        (fn(json.val[apiVersionSelector(minVer + offset)()], minVer + offset),
+         ...);
+    }
+    (std::make_index_sequence<size>{});
+}
+
 }  // namespace ripple
 
 #endif
diff --git a/src/test/json/MultivarJson_test.cpp b/src/test/json/MultivarJson_test.cpp
