respond to comments

XRPLF · Apr 12, 2024 · c8af202 · c8af202
1 parent 792d185
commit c8af202
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 33 deletions.
diff --git a/src/ripple/ledger/impl/ApplyStateTable.cpp b/src/ripple/ledger/impl/ApplyStateTable.cpp
@@ -21,10 +21,8 @@
 #include <ripple/json/to_string.h>
 #include <ripple/ledger/detail/ApplyStateTable.h>
 #include <ripple/protocol/Feature.h>
-#include <ripple/protocol/LedgerFormats.h>
 #include <ripple/protocol/st.h>
 #include <cassert>
-#include <set>
 
 namespace ripple {
 namespace detail {
@@ -615,6 +613,7 @@ ApplyStateTable::threadTx(
         return;
     }
     // threadItem only applied to AccountRoot
+    assert(sle->isThreadedType(base.rules()));
     threadItem(meta, sle);
 }
 

diff --git a/src/ripple/protocol/STLedgerEntry.h b/src/ripple/protocol/STLedgerEntry.h
@@ -21,11 +21,11 @@
 #define RIPPLE_PROTOCOL_STLEDGERENTRY_H_INCLUDED
 
 #include <ripple/protocol/Indexes.h>
-#include <ripple/protocol/Rules.h>
 #include <ripple/protocol/STObject.h>
 
 namespace ripple {
 
+class Rules;
 class Invariants_test;
 
 class STLedgerEntry final : public STObject, public CountedObject<STLedgerEntry>

diff --git a/src/ripple/protocol/impl/STLedgerEntry.cpp b/src/ripple/protocol/impl/STLedgerEntry.cpp
@@ -17,13 +17,15 @@
 */
 //==============================================================================
 
+#include <ripple/protocol/STLedgerEntry.h>
+
 #include <ripple/basics/Log.h>
 #include <ripple/basics/contract.h>
 #include <ripple/basics/safe_cast.h>
 #include <ripple/json/to_string.h>
 #include <ripple/protocol/Feature.h>
 #include <ripple/protocol/Indexes.h>
-#include <ripple/protocol/STLedgerEntry.h>
+#include <ripple/protocol/Rules.h>
 #include <ripple/protocol/jss.h>
 #include <boost/format.hpp>
 #include <algorithm>
@@ -138,7 +140,7 @@ STLedgerEntry::isThreadedType(Rules const& rules) const
             newPreviousTxnIDTypes.cbegin(),
             newPreviousTxnIDTypes.cend(),
             type_);
-    return getFieldIndex(sfPreviousTxnID) != -1 && !excludePrevTxnID;
+    return !excludePrevTxnID && getFieldIndex(sfPreviousTxnID) != -1;
 }
 
 bool

diff --git a/src/test/ledger/Directory_test.cpp b/src/test/ledger/Directory_test.cpp
@@ -406,28 +406,30 @@ struct Directory_test : public beast::unit_test::suite
         auto const alice = Account{"alice"};
         auto const USD = gw["USD"];
 
-        auto ledger_data = [](Env& env) {
+        auto ledger_data = [this](Env& env) {
             Json::Value params;
             params[jss::type] = jss::directory;
             params[jss::ledger_index] = "validated";
-            return env.rpc(
-                "json", "ledger_data", to_string(params))[jss::result];
+            auto const result =
+                env.rpc("json", "ledger_data", to_string(params))[jss::result];
+            BEAST_EXPECT(!result.isMember(jss::marker));
+            return result;
         };
 
-        {
-            // fixPreviousTxnID is disabled.
-            Env env(*this, supported_amendments() - fixPreviousTxnID);
-            env.fund(XRP(10000), alice, gw);
-            env.close();
-            env.trust(USD(1000), alice);
-            env(pay(gw, alice, USD(1000)));
-            env.close();
+        // fixPreviousTxnID is disabled.
+        Env env(*this, supported_amendments() - fixPreviousTxnID);
+        env.fund(XRP(10000), alice, gw);
+        env.close();
+        env.trust(USD(1000), alice);
+        env(pay(gw, alice, USD(1000)));
+        env.close();
 
+        {
+            auto const jrr = ledger_data(env);
+            auto const& jstate = jrr[jss::state];
+            BEAST_EXPECTS(checkArraySize(jstate, 2), jrr.toStyledString());
+            for (auto const& directory : jstate)
             {
-                auto const jrr = ledger_data(env);
-                BEAST_EXPECTS(
-                    checkArraySize(jrr[jss::state], 2), jrr.toStyledString());
-                auto const directory = jrr[jss::state][0u];
                 BEAST_EXPECT(
                     directory["LedgerEntryType"] ==
                     jss::DirectoryNode);  // sanity check
@@ -436,26 +438,50 @@ struct Directory_test : public beast::unit_test::suite
                 BEAST_EXPECT(!directory.isMember("PreviousTxnID"));
                 BEAST_EXPECT(!directory.isMember("PreviousTxnLgrSeq"));
             }
+        }
 
-            // Now enable the amendment so the directory node is updated.
-            env.enableFeature(fixPreviousTxnID);
-            env.close();
+        // Now enable the amendment so the directory node is updated.
+        env.enableFeature(fixPreviousTxnID);
+        env.close();
 
-            // Make sure the `PreviousTxnID` and `PreviousTxnLgrSeq` fields now
-            // exist
-            env(offer(alice, XRP(1), USD(1)));
-            env.close();
+        // Make sure the `PreviousTxnID` and `PreviousTxnLgrSeq` fields now
+        // exist
+        env(offer(alice, XRP(1), USD(1)));
+        auto const txID = to_string(env.tx()->getTransactionID());
+        auto const ledgerSeq = env.current()->info().seq;
+        env.close();
+        // Make sure the fields only exist if the object is touched
+        env(noop(gw));
+        env.close();
 
+        {
+            auto const jrr = ledger_data(env);
+            auto const& jstate = jrr[jss::state];
+            BEAST_EXPECTS(checkArraySize(jstate, 3), jrr.toStyledString());
+            for (auto const& directory : jstate)
             {
-                auto const jrr = ledger_data(env);
-                BEAST_EXPECTS(
-                    checkArraySize(jrr[jss::state], 3), jrr.toStyledString());
-                auto const directory = jrr[jss::state][0u];
                 BEAST_EXPECT(
                     directory["LedgerEntryType"] ==
                     jss::DirectoryNode);  // sanity check
-                BEAST_EXPECT(directory.isMember("PreviousTxnID"));
-                BEAST_EXPECT(directory.isMember("PreviousTxnLgrSeq"));
+                if (directory[jss::Owner] == gw.human())
+                {
+                    // gw's directory did not get touched, so it
+                    // should not have those fields populated
+                    BEAST_EXPECT(!directory.isMember("PreviousTxnID"));
+                    BEAST_EXPECT(!directory.isMember("PreviousTxnLgrSeq"));
+                }
+                else
+                {
+                    // All of the other directories, including the order
+                    // book, did get touched, so they should have those
+                    // fields
+                    BEAST_EXPECT(
+                        directory.isMember("PreviousTxnID") &&
+                        directory["PreviousTxnID"].asString() == txID);
+                    BEAST_EXPECT(
+                        directory.isMember("PreviousTxnLgrSeq") &&
+                        directory["PreviousTxnLgrSeq"].asUInt() == ledgerSeq);
+                }
             }
         }
     }