Merge branch 'develop' into copilot/remove-non-canonical-fields

Co-authored-by: mvadari <8029314+mvadari@users.noreply.github.com>

API-CHANGELOG.md

@@ -26,10 +26,6 @@ This version is supported by all `xrpld` versions. For WebSocket and HTTP JSON-R
 
 This section contains changes targeting a future version.
 
-### Breaking changes
-
-- `feature`: In admin-mode responses, the `vetoed` field is now always a boolean. Disabled obsolete amendments now have `"vetoed": true` and a new `"obsolete": true` field, instead of the previous `"vetoed": "Obsolete"` string value. This change improves type safety for API clients. Both `vetoed` and `obsolete` fields are only present in admin-mode responses for disabled amendments.
-
 ### Additions
 
 - `ledger_entry`, `account_objects`: The `Delegate` ledger entry now includes an optional `DestinationNode` field, which stores the index into the authorized account's owner directory. This field is present on entries created after bidirectional directory tracking was introduced and may appear in RPC responses for those entries. ([#6681](https://github.com/XRPLF/rippled/pull/6681))

API-VERSION-3.md

@@ -6,6 +6,13 @@ For info about how [API versioning](https://xrpl.org/request-formatting.html#api
 
 ## Breaking Changes
 
+### Modifications to `tx` and `account_tx`
+
+In API version 2, the `tx_json` field in `tx` and `account_tx` responses includes server-added lower-case fields (`date`, `ledger_index`, and `ctid`) that are not part of the canonical signed transaction. In API version 3, these fields are removed from `tx_json` and are only present at the top-level result object.
+
+- **Before (API v2)**: The `tx_json` object in the response contained `date`, `ledger_index`, and `ctid` fields alongside the canonical PascalCase transaction fields.
+- **After (API v3)**: The `tx_json` object contains only the canonical signed transaction fields. The `date`, `ledger_index`, and `ctid` fields appear exclusively at the top-level result object.
+
 ### Modifications to `amm_info`
 
 The order of error checks has been changed to provide more specific error messages. ([#4924](https://github.com/XRPLF/rippled/pull/4924))

include/xrpl/protocol/STBase.h

@@ -22,9 +22,10 @@ struct JsonOptions
         None = 0b0000'0000,
         IncludeDate = 0b0000'0001,
         DisableApiPriorV2 = 0b0000'0010,
+        DisableApiPriorV3 = 0b0000'0100,
 
         // IMPORTANT `All` must be union of all of the above; see also operator~
-        All = IncludeDate | DisableApiPriorV2  // 0b0000'0011
+        All = IncludeDate | DisableApiPriorV2 | DisableApiPriorV3  // 0b0000'0111
     };
 
     constexpr JsonOptions(underlying_t v) noexcept : value(v)

include/xrpl/protocol/jss.h

@@ -435,7 +435,6 @@ JSS(node_write_retries);          // out: GetCounts
 JSS(node_writes_delayed);         // out::GetCounts
 JSS(nth);                         // out: RPC server_definitions
 JSS(obligations);                 // out: GatewayBalances
-JSS(obsolete);                    // out: AmendmentTableImpl
 JSS(offers);                      // out: NetworkOPs, AccountOffers, Subscribe
 JSS(offer_id);                    // out: insertNFTokenOfferID
 JSS(offline);                     // in: TransactionSign

src/test/rpc/AccountTx_test.cpp

@@ -164,20 +164,52 @@ class AccountTx_test : public beast::unit_test::Suite
                     {
                         auto const& payment = j[jss::result][jss::transactions][1u];
 
-                        return (payment.isMember(jss::tx_json)) &&
-                            (payment[jss::tx_json][jss::TransactionType] == jss::Payment) &&
-                            (payment[jss::tx_json][jss::DeliverMax] == "10000000010") &&
-                            (!payment[jss::tx_json].isMember(jss::Amount)) &&
-                            (!payment[jss::tx_json].isMember(jss::hash)) &&
-                            (payment[jss::hash] ==
-                             "9F3085D85F472D1CC29627F260DF68EDE59D42D1D0C33E345"
-                             "ECF0D4CE981D0A8") &&
-                            (payment[jss::validated] == true) &&
-                            (payment[jss::ledger_index] == 3) &&
-                            (payment[jss::ledger_hash] ==
-                             "5476DCD816EA04CBBA57D47BBF1FC58A5217CC93A5ADD79CB"
-                             "580A5AFDD727E33") &&
-                            (payment[jss::close_time_iso] == "2000-01-01T00:00:10Z");
+                        if (apiVersion >= 3)
+                        {
+                            // In API v3, server-added lower-case fields must
+                            // not be in tx_json, but must be at result level
+                            return (payment.isMember(jss::tx_json)) &&
+                                (payment[jss::tx_json][jss::TransactionType] == jss::Payment) &&
+                                (payment[jss::tx_json][jss::DeliverMax] == "10000000010") &&
+                                (!payment[jss::tx_json].isMember(jss::Amount)) &&
+                                (!payment[jss::tx_json].isMember(jss::hash)) &&
+                                (!payment[jss::tx_json].isMember(jss::date)) &&
+                                (!payment[jss::tx_json].isMember(jss::ledger_index)) &&
+                                (!payment[jss::tx_json].isMember(jss::ctid)) &&
+                                // date and ctid must be at the transaction
+                                // object level (outside tx_json) in API v3
+                                (payment.isMember(jss::date)) && (payment.isMember(jss::ctid)) &&
+                                (payment[jss::hash] ==
+                                 "9F3085D85F472D1CC29627F260DF68EDE59D42D1D0C33E345"
+                                 "ECF0D4CE981D0A8") &&
+                                (payment[jss::validated] == true) &&
+                                (payment[jss::ledger_index] == 3) &&
+                                (payment[jss::ledger_hash] ==
+                                 "5476DCD816EA04CBBA57D47BBF1FC58A5217CC93A5ADD79CB"
+                                 "580A5AFDD727E33") &&
+                                (payment[jss::close_time_iso] == "2000-01-01T00:00:10Z");
+                        }
+                        else
+                        {
+                            // In API v2, date and ledger_index are still in
+                            // tx_json for backwards compatibility
+                            return (payment.isMember(jss::tx_json)) &&
+                                (payment[jss::tx_json][jss::TransactionType] == jss::Payment) &&
+                                (payment[jss::tx_json][jss::DeliverMax] == "10000000010") &&
+                                (!payment[jss::tx_json].isMember(jss::Amount)) &&
+                                (!payment[jss::tx_json].isMember(jss::hash)) &&
+                                (payment[jss::tx_json].isMember(jss::date)) &&
+                                (payment[jss::tx_json].isMember(jss::ledger_index)) &&
+                                (payment[jss::hash] ==
+                                 "9F3085D85F472D1CC29627F260DF68EDE59D42D1D0C33E345"
+                                 "ECF0D4CE981D0A8") &&
+                                (payment[jss::validated] == true) &&
+                                (payment[jss::ledger_index] == 3) &&
+                                (payment[jss::ledger_hash] ==
+                                 "5476DCD816EA04CBBA57D47BBF1FC58A5217CC93A5ADD79CB"
+                                 "580A5AFDD727E33") &&
+                                (payment[jss::close_time_iso] == "2000-01-01T00:00:10Z");
+                        }
                     }
                     else
                     {

src/test/rpc/Feature_test.cpp

@@ -166,15 +166,12 @@ class Feature_test : public beast::unit_test::Suite
                 feature.isMember(jss::enabled) && !feature[jss::enabled].asBool(),
                 feature[jss::name].asString() + " enabled");
             BEAST_EXPECTS(
-                feature.isMember(jss::vetoed) && feature[jss::vetoed].isBool(),
-                feature[jss::name].asString() + " vetoed is bool");
-            BEAST_EXPECTS(
-                feature[jss::vetoed].asBool() == (expectVeto || expectObsolete),
-                feature[jss::name].asString() + " vetoed value");
-            BEAST_EXPECTS(
-                feature.isMember(jss::obsolete) && feature[jss::obsolete].isBool() &&
-                    feature[jss::obsolete].asBool() == expectObsolete,
-                feature[jss::name].asString() + " obsolete");
+                feature.isMember(jss::vetoed) && feature[jss::vetoed].isBool() == !expectObsolete &&
+                    (!feature[jss::vetoed].isBool() ||
+                     feature[jss::vetoed].asBool() == expectVeto) &&
+                    (feature[jss::vetoed].isBool() ||
+                     feature[jss::vetoed].asString() == "Obsolete"),
+                feature[jss::name].asString() + " vetoed");
             BEAST_EXPECTS(
                 feature.isMember(jss::supported) && feature[jss::supported].asBool(),
                 feature[jss::name].asString() + " supported");
@@ -306,9 +303,6 @@ class Feature_test : public beast::unit_test::Suite
                     (*it).isMember(jss::supported) &&
                         (*it)[jss::supported].asBool() == expectSupported,
                     (*it)[jss::name].asString() + " supported");
-                BEAST_EXPECTS(
-                    (*it).isMember(jss::obsolete) && (*it)[jss::obsolete].isBool(),
-                    (*it)[jss::name].asString() + " unknown obsolete");
                 BEAST_EXPECT(!(*it).isMember(jss::vetoed));
                 BEAST_EXPECT(!(*it).isMember(jss::majority));
                 BEAST_EXPECT(!(*it).isMember(jss::count));
@@ -378,15 +372,12 @@ class Feature_test : public beast::unit_test::Suite
             else
             {
                 BEAST_EXPECTS(
-                    (*it).isMember(jss::vetoed) && (*it)[jss::vetoed].isBool(),
-                    (*it)[jss::name].asString() + " vetoed is bool");
-                BEAST_EXPECTS(
-                    (*it)[jss::vetoed].asBool() == (expectVeto || expectObsolete),
-                    (*it)[jss::name].asString() + " vetoed value");
-                BEAST_EXPECTS(
-                    (*it).isMember(jss::obsolete) && (*it)[jss::obsolete].isBool() &&
-                        (*it)[jss::obsolete].asBool() == expectObsolete,
-                    (*it)[jss::name].asString() + " obsolete");
+                    (*it).isMember(jss::vetoed) && (*it)[jss::vetoed].isBool() == !expectObsolete &&
+                        (!(*it)[jss::vetoed].isBool() ||
+                         (*it)[jss::vetoed].asBool() == expectVeto) &&
+                        ((*it)[jss::vetoed].isBool() ||
+                         (*it)[jss::vetoed].asString() == "Obsolete"),
+                    (*it)[jss::name].asString() + " vetoed");
             }
             BEAST_EXPECTS(
                 (*it).isMember(jss::supported) && (*it)[jss::supported].asBool() == expectSupported,
@@ -456,15 +447,12 @@ class Feature_test : public beast::unit_test::Suite
                 (expectVeto || expectObsolete) ^ feature.isMember(jss::majority),
                 feature[jss::name].asString() + " majority");
             BEAST_EXPECTS(
-                feature.isMember(jss::vetoed) && feature[jss::vetoed].isBool(),
-                feature[jss::name].asString() + " vetoed is bool");
-            BEAST_EXPECTS(
-                feature[jss::vetoed].asBool() == (expectVeto || expectObsolete),
-                feature[jss::name].asString() + " vetoed value");
-            BEAST_EXPECTS(
-                feature.isMember(jss::obsolete) && feature[jss::obsolete].isBool() &&
-                    feature[jss::obsolete].asBool() == expectObsolete,
-                feature[jss::name].asString() + " obsolete");
+                feature.isMember(jss::vetoed) && feature[jss::vetoed].isBool() == !expectObsolete &&
+                    (!feature[jss::vetoed].isBool() ||
+                     feature[jss::vetoed].asBool() == expectVeto) &&
+                    (feature[jss::vetoed].isBool() ||
+                     feature[jss::vetoed].asString() == "Obsolete"),
+                feature[jss::name].asString() + " vetoed");
             BEAST_EXPECTS(feature.isMember(jss::count), feature[jss::name].asString() + " count");
             BEAST_EXPECTS(
                 feature.isMember(jss::threshold), feature[jss::name].asString() + " threshold");
@@ -554,9 +542,8 @@ class Feature_test : public beast::unit_test::Suite
         auto feature = *(jrr.begin());
         BEAST_EXPECTS(feature[jss::name] == featureName, "name");
         BEAST_EXPECTS(
-            feature[jss::vetoed].isBool() && feature[jss::vetoed].asBool() == true, "vetoed");
-        BEAST_EXPECTS(
-            feature[jss::obsolete].isBool() && feature[jss::obsolete].asBool() == true, "obsolete");
+            feature[jss::vetoed].isString() && feature[jss::vetoed].asString() == "Obsolete",
+            "vetoed");
 
         jrr = env.rpc("feature", featureName, "reject")[jss::result];
         if (!BEAST_EXPECTS(jrr[jss::status] == jss::success, "status"))
@@ -567,9 +554,8 @@ class Feature_test : public beast::unit_test::Suite
         feature = *(jrr.begin());
         BEAST_EXPECTS(feature[jss::name] == featureName, "name");
         BEAST_EXPECTS(
-            feature[jss::vetoed].isBool() && feature[jss::vetoed].asBool() == true, "vetoed");
-        BEAST_EXPECTS(
-            feature[jss::obsolete].isBool() && feature[jss::obsolete].asBool() == true, "obsolete");
+            feature[jss::vetoed].isString() && feature[jss::vetoed].asString() == "Obsolete",
+            "vetoed");
 
         jrr = env.rpc("feature", featureName, "accept")[jss::result];
         if (!BEAST_EXPECTS(jrr[jss::status] == jss::success, "status"))
@@ -580,9 +566,8 @@ class Feature_test : public beast::unit_test::Suite
         feature = *(jrr.begin());
         BEAST_EXPECTS(feature[jss::name] == featureName, "name");
         BEAST_EXPECTS(
-            feature[jss::vetoed].isBool() && feature[jss::vetoed].asBool() == true, "vetoed");
-        BEAST_EXPECTS(
-            feature[jss::obsolete].isBool() && feature[jss::obsolete].asBool() == true, "obsolete");
+            feature[jss::vetoed].isString() && feature[jss::vetoed].asString() == "Obsolete",
+            "vetoed");
 
         // anything other than accept or reject is an error
         jrr = env.rpc("feature", featureName, "maybe");

src/test/rpc/Transaction_test.cpp

@@ -798,6 +798,25 @@ class Transaction_test : public beast::unit_test::Suite
                 result[jss::result][jss::ledger_hash] ==
                 "B41882E20F0EC6228417D28B9AE0F33833645D35F6799DFB782AC97FC4BB51"
                 "D2");
+
+            auto const& tx_json = result[jss::result][jss::tx_json];
+            if (apiVersion >= 3)
+            {
+                // In API v3, server-added lower-case fields must not appear
+                // inside tx_json; they are at the result level.
+                BEAST_EXPECT(!tx_json.isMember(jss::date));
+                BEAST_EXPECT(!tx_json.isMember(jss::ledger_index));
+                BEAST_EXPECT(!tx_json.isMember(jss::ctid));
+                // date must be at result level in API v3
+                BEAST_EXPECT(result[jss::result].isMember(jss::date));
+            }
+            else
+            {
+                // In API v2, date and ledger_index are still included in
+                // tx_json for backwards compatibility.
+                BEAST_EXPECT(tx_json.isMember(jss::date));
+                BEAST_EXPECT(tx_json.isMember(jss::ledger_index));
+            }
         }
 
         for (auto memberIt = expected.begin(); memberIt != expected.end(); memberIt++)

src/xrpld/app/misc/detail/AmendmentTable.cpp

@@ -957,8 +957,7 @@ AmendmentTableImpl::injectJson(
     {
         if (fs.vote == AmendmentVote::Obsolete)
         {
-            v[jss::vetoed] = true;
-            v[jss::obsolete] = true;
+            v[jss::vetoed] = "Obsolete";
         }
         else
         {

src/xrpld/app/misc/detail/Transaction.cpp

@@ -163,28 +163,30 @@ Transaction::getJson(JsonOptions options, bool binary) const
             ret[jss::inLedger] = ledgerIndex_;
         }
 
-        // TODO: disable_API_prior_V3 to disable output of both `date` and
-        // `ledger_index` elements (taking precedence over include_date)
-        ret[jss::ledger_index] = ledgerIndex_;
-
-        if (options & JsonOptions::Values::IncludeDate)
+        if (!(options & JsonOptions::Values::DisableApiPriorV3))
         {
-            auto ct = app_.getLedgerMaster().getCloseTimeBySeq(ledgerIndex_);
-            if (ct)
-                ret[jss::date] = ct->time_since_epoch().count();
-        }
+            ret[jss::ledger_index] = ledgerIndex_;
 
-        // compute outgoing CTID
-        // override local network id if it's explicitly in the txn
-        std::optional netID = networkID_;
-        if (transaction_->isFieldPresent(sfNetworkID))
-            netID = transaction_->getFieldU32(sfNetworkID);
+            if (options & JsonOptions::Values::IncludeDate)
+            {
+                auto ct = app_.getLedgerMaster().getCloseTimeBySeq(ledgerIndex_);
+                if (ct)
+                    ret[jss::date] = ct->time_since_epoch().count();
+            }
 
-        if (txnSeq_ && netID)
-        {
-            std::optional<std::string> const ctid = RPC::encodeCTID(ledgerIndex_, *txnSeq_, *netID);
-            if (ctid)
-                ret[jss::ctid] = *ctid;
+            // compute outgoing CTID
+            // override local network id if it's explicitly in the txn
+            std::optional netID = networkID_;
+            if (transaction_->isFieldPresent(sfNetworkID))
+                netID = transaction_->getFieldU32(sfNetworkID);
+
+            if (txnSeq_ && netID)
+            {
+                std::optional<std::string> const ctid =
+                    RPC::encodeCTID(ledgerIndex_, *txnSeq_, *netID);
+                if (ctid)
+                    ret[jss::ctid] = *ctid;
+            }
         }
     }
 

src/xrpld/rpc/handlers/account/AccountTx.cpp

@@ -3,6 +3,7 @@
 #include <xrpld/app/misc/DeliverMax.h>
 #include <xrpld/app/misc/Transaction.h>
 #include <xrpld/app/rdb/backend/SQLiteDatabase.h>
+#include <xrpld/rpc/CTID.h>
 #include <xrpld/rpc/Context.h>
 #include <xrpld/rpc/DeliveredAmount.h>
 #include <xrpld/rpc/MPTokenIssuanceID.h>
@@ -17,6 +18,7 @@
 #include <xrpl/basics/chrono.h>
 #include <xrpl/basics/strHex.h>
 #include <xrpl/beast/utility/instrumentation.h>
+#include <xrpl/core/NetworkIDService.h>
 #include <xrpl/json/json_value.h>
 #include <xrpl/ledger/ReadView.h>
 #include <xrpl/protocol/AccountID.h>
@@ -309,12 +311,18 @@ populateJsonResponse(
                     auto const jsonTx = (context.apiVersion > 1 ? jss::tx_json : jss::tx);
                     if (context.apiVersion > 1)
                     {
-                        jvObj[jsonTx] = txn->getJson(
+                        static constexpr auto kOptionsJsonV2 =
                             static_cast<JsonOptions::underlying_t>(
                                 JsonOptions::Values::IncludeDate) |
-                                static_cast<JsonOptions::underlying_t>(
-                                    JsonOptions::Values::DisableApiPriorV2),
-                            false);
+                            static_cast<JsonOptions::underlying_t>(
+                                JsonOptions::Values::DisableApiPriorV2);
+                        static constexpr auto kOptionsJsonV3 =
+                            static_cast<JsonOptions::underlying_t>(
+                                JsonOptions::Values::DisableApiPriorV2) |
+                            static_cast<JsonOptions::underlying_t>(
+                                JsonOptions::Values::DisableApiPriorV3);
+                        auto const opts = context.apiVersion >= 3 ? kOptionsJsonV3 : kOptionsJsonV2;
+                        jvObj[jsonTx] = txn->getJson(opts, false);
                         jvObj[jss::hash] = to_string(txn->getID());
                         jvObj[jss::ledger_index] = txn->getLedger();
                         jvObj[jss::ledger_hash] =
@@ -322,7 +330,20 @@ populateJsonResponse(
 
                         if (auto closeTime =
                                 context.ledgerMaster.getCloseTimeBySeq(txn->getLedger()))
+                        {
                             jvObj[jss::close_time_iso] = toStringIso(*closeTime);
+                            if (context.apiVersion >= 3)
+                                jvObj[jss::date] = closeTime->time_since_epoch().count();
+                        }
+
+                        if (context.apiVersion >= 3 && txnMeta)
+                        {
+                            uint32_t const lgrSeq = txn->getLedger();
+                            uint32_t const txnIdx = txnMeta->getIndex();
+                            uint32_t const netID = context.app.getNetworkIDService().getNetworkID();
+                            if (auto const ctid = RPC::encodeCTID(lgrSeq, txnIdx, netID))
+                                jvObj[jss::ctid] = *ctid;
+                        }
                     }
                     else
                     {

src/xrpld/rpc/handlers/transaction/Tx.cpp

@@ -205,9 +205,13 @@ populateJsonResponse(
         auto const& sttx = result.txn->getSTransaction();
         if (context.apiVersion > 1)
         {
-            static constexpr auto kOptionsJson =
+            static constexpr auto kOptionsJsonV2 =
                 static_cast<JsonOptions::underlying_t>(JsonOptions::Values::IncludeDate) |
                 static_cast<JsonOptions::underlying_t>(JsonOptions::Values::DisableApiPriorV2);
+            static constexpr auto kOptionsJsonV3 =
+                static_cast<JsonOptions::underlying_t>(JsonOptions::Values::DisableApiPriorV2) |
+                static_cast<JsonOptions::underlying_t>(JsonOptions::Values::DisableApiPriorV3);
+            auto const kOptionsJson = context.apiVersion >= 3 ? kOptionsJsonV3 : kOptionsJsonV2;
             if (args.binary)
             {
                 response[jss::tx_blob] = result.txn->getJson(kOptionsJson, true);
@@ -229,7 +233,11 @@ populateJsonResponse(
             {
                 response[jss::ledger_index] = result.txn->getLedger();
                 if (result.closeTime)
+                {
                     response[jss::close_time_iso] = toStringIso(*result.closeTime);
+                    if (context.apiVersion >= 3)
+                        response[jss::date] = result.closeTime->time_since_epoch().count();
+                }
             }
         }
         else