review fixes

XRPLF · Nov 26, 2024 · 250b36c · 250b36c
1 parent f707d98
commit 250b36c
Show file tree

Hide file tree

Showing 9 changed files with 192 additions and 137 deletions.
diff --git a/src/test/app/PermissionedDomains_test.cpp b/src/test/app/PermissionedDomains_test.cpp
diff --git a/src/test/jtx/impl/permissioned_domains.cpp b/src/test/jtx/impl/permissioned_domains.cpp
@@ -23,7 +23,7 @@
 namespace ripple {
 namespace test {
 namespace jtx {
-namespace pd {
+namespace pdomain {
 
 // helpers
 // Make json for PermissionedDomainSet transaction
@@ -167,7 +167,7 @@ getNewDomain(std::shared_ptr<STObject const> const& meta)
     return ret;
 }
 
-}  // namespace pd
+}  // namespace pdomain
 }  // namespace jtx
 }  // namespace test
 }  // namespace ripple
diff --git a/src/test/jtx/permissioned_domains.h b/src/test/jtx/permissioned_domains.h
@@ -25,7 +25,7 @@
 namespace ripple {
 namespace test {
 namespace jtx {
-namespace pd {
+namespace pdomain {
 
 // Helpers for PermissionedDomains testing
 using Credential = ripple::test::jtx::deposit::AuthorizeCredentials;
@@ -69,7 +69,7 @@ ownerInfo(Account const& account, Env& env);
 uint256
 getNewDomain(std::shared_ptr<STObject const> const& meta);
 
-}  // namespace pd
+}  // namespace pdomain
 }  // namespace jtx
 }  // namespace test
 }  // namespace ripple
diff --git a/src/test/rpc/AccountObjects_test.cpp b/src/test/rpc/AccountObjects_test.cpp
@@ -722,7 +722,7 @@ class AccountObjects_test : public beast::unit_test::suite
             env.fund(XRP(5000), issuer);
 
             // gw creates an PermissionedDomain.
-            env(pd::setTx(gw, {{issuer, credentialType1}}));
+            env(pdomain::setTx(gw, {{issuer, credentialType1}}));
             env.close();
 
             // Find the PermissionedDomain.

diff --git a/src/test/rpc/LedgerRPC_test.cpp b/src/test/rpc/LedgerRPC_test.cpp
@@ -3083,13 +3083,13 @@ class LedgerRPC_test : public beast::unit_test::suite
         env.close();
 
         auto const seq = env.seq(alice);
-        env(pd::setTx(alice, {{alice, "first credential"}}));
+        env(pdomain::setTx(alice, {{alice, "first credential"}}));
         env.close();
-        auto const objects = pd::getObjects(alice, env);
+        auto const objects = pdomain::getObjects(alice, env);
         BEAST_EXPECT(objects.size() == 1);
         if (objects.empty())
             return;
-        // env(pd::deleteTx(alice, domain));
+        // env(pdomain::deleteTx(alice, domain));
 
         {
             // Succeed

diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp
@@ -1131,27 +1131,45 @@ ValidPermissionedDomain::visitEntry(
     std::shared_ptr<SLE const> const& before,
     std::shared_ptr<SLE const> const& after)
 {
-    if (after->getType() != ltPERMISSIONED_DOMAIN)
+    if (before && before->getType() != ltPERMISSIONED_DOMAIN)
+        return;
+    if (after && after->getType() != ltPERMISSIONED_DOMAIN)
         return;
-    auto const& credentials = after->getFieldArray(sfAcceptedCredentials);
-    credentialsSize_ = credentials.size();
-    auto const sorted = credentials::makeSorted(credentials);
-    isUnique_ = !sorted.empty();
 
-    // If array have duplicates then all the other checks are invalid
-    isSorted_ = false;
+    auto check = [](SleStatus& sleStatus,
+                    std::shared_ptr<SLE const> const& sle) {
+        auto const& credentials = sle->getFieldArray(sfAcceptedCredentials);
+        sleStatus.credentialsSize_ = credentials.size();
+        auto const sorted = credentials::makeSorted(credentials);
+        sleStatus.isUnique_ = !sorted.empty();
 
-    if (isUnique_)
-    {
-        unsigned i = 0;
-        for (auto const& cred : sorted)
+        // If array have duplicates then all the other checks are invalid
+        sleStatus.isSorted_ = false;
+
+        if (sleStatus.isUnique_)
         {
-            auto const& credTx = credentials[i++];
-            isSorted_ = (cred.first == credTx[sfIssuer]) &&
-                (cred.second == credTx[sfCredentialType]);
-            if (!isSorted_)
-                break;
+            unsigned i = 0;
+            for (auto const& cred : sorted)
+            {
+                auto const& credTx = credentials[i++];
+                sleStatus.isSorted_ = (cred.first == credTx[sfIssuer]) &&
+                    (cred.second == credTx[sfCredentialType]);
+                if (!sleStatus.isSorted_)
+                    break;
+            }
         }
+    };
+
+    if (before)
+    {
+        sleStatus_[0] = SleStatus();
+        check(*sleStatus_[0], after);
+    }
+
+    if (after)
+    {
+        sleStatus_[1] = SleStatus();
+        check(*sleStatus_[1], after);
     }
 }
 
@@ -1166,36 +1184,44 @@ ValidPermissionedDomain::finalize(
     if (tx.getTxnType() != ttPERMISSIONED_DOMAIN_SET || result != tesSUCCESS)
         return true;
 
-    if (!credentialsSize_)
-    {
-        JLOG(j.fatal()) << "Invariant failed: permissioned domain with "
-                           "no rules.";
-        return false;
-    }
+    auto check = [](SleStatus const& sleStatus, beast::Journal const& j) {
+        if (!sleStatus.credentialsSize_)
+        {
+            JLOG(j.fatal()) << "Invariant failed: permissioned domain with "
+                               "no rules.";
+            return false;
+        }
 
-    if (credentialsSize_ > maxPermissionedDomainCredentialsArraySize)
-    {
-        JLOG(j.fatal()) << "Invariant failed: permissioned domain bad "
-                           "credentials size "
-                        << credentialsSize_;
-        return false;
-    }
+        if (sleStatus.credentialsSize_ >
+            maxPermissionedDomainCredentialsArraySize)
+        {
+            JLOG(j.fatal()) << "Invariant failed: permissioned domain bad "
+                               "credentials size "
+                            << sleStatus.credentialsSize_;
+            return false;
+        }
 
-    if (!isUnique_)
-    {
-        JLOG(j.fatal()) << "Invariant failed: permissioned domain credentials "
-                           "aren't unique";
-        return false;
-    }
+        if (!sleStatus.isUnique_)
+        {
+            JLOG(j.fatal())
+                << "Invariant failed: permissioned domain credentials "
+                   "aren't unique";
+            return false;
+        }
 
-    if (!isSorted_)
-    {
-        JLOG(j.fatal()) << "Invariant failed: permissioned domain credentials "
-                           "aren't sorted";
-        return false;
-    }
+        if (!sleStatus.isSorted_)
+        {
+            JLOG(j.fatal())
+                << "Invariant failed: permissioned domain credentials "
+                   "aren't sorted";
+            return false;
+        }
 
-    return true;
+        return true;
+    };
+
+    return (sleStatus_[0] ? check(*sleStatus_[0], j) : true) &&
+        (sleStatus_[1] ? check(*sleStatus_[1], j) : true);
 }
 
 }  // namespace ripple
diff --git a/src/xrpld/app/tx/detail/InvariantCheck.h b/src/xrpld/app/tx/detail/InvariantCheck.h
@@ -482,8 +482,12 @@ class ValidMPTIssuance
  */
 class ValidPermissionedDomain
 {
-    std::size_t credentialsSize_{0};
-    bool isSorted_ = false, isUnique_ = false;
+    struct SleStatus
+    {
+        std::size_t credentialsSize_{0};
+        bool isSorted_ = false, isUnique_ = false;
+    };
+    std::optional<SleStatus> sleStatus_[2];
 
 public:
     void

diff --git a/src/xrpld/app/tx/detail/PermissionedDomainDelete.cpp b/src/xrpld/app/tx/detail/PermissionedDomainDelete.cpp
@@ -27,8 +27,10 @@ PermissionedDomainDelete::preflight(PreflightContext const& ctx)
 {
     if (!ctx.rules.enabled(featurePermissionedDomains))
         return temDISABLED;
+
     if (auto const ret = preflight1(ctx); !isTesSuccess(ret))
         return ret;
+
     auto const domain = ctx.tx.getFieldH256(sfDomainID);
     if (domain == beast::zero)
         return temMALFORMED;
@@ -41,12 +43,15 @@ PermissionedDomainDelete::preclaim(PreclaimContext const& ctx)
 {
     auto const domain = ctx.tx.getFieldH256(sfDomainID);
     auto const sleDomain = ctx.view.read({ltPERMISSIONED_DOMAIN, domain});
+
     if (!sleDomain)
         return tecNO_ENTRY;
+
     assert(
         sleDomain->isFieldPresent(sfOwner) && ctx.tx.isFieldPresent(sfAccount));
     if (sleDomain->getAccountID(sfOwner) != ctx.tx.getAccountID(sfAccount))
         return tecNO_PERMISSION;
+
     return tesSUCCESS;
 }
 
@@ -55,19 +60,23 @@ TER
 PermissionedDomainDelete::doApply()
 {
     assert(ctx_.tx.isFieldPresent(sfDomainID));
+
     auto const slePd =
         view().peek({ltPERMISSIONED_DOMAIN, ctx_.tx.at(sfDomainID)});
-    auto const page{(*slePd)[sfOwnerNode]};
+    auto const page = (*slePd)[sfOwnerNode];
+
     if (!view().dirRemove(keylet::ownerDir(account_), page, slePd->key(), true))
     {
         JLOG(j_.fatal())
             << "Unable to delete permissioned domain directory entry.";
         return tefBAD_LEDGER;
     }
+
     auto const ownerSle = view().peek(keylet::account(account_));
     assert(ownerSle && ownerSle->getFieldU32(sfOwnerCount) > 0);
     adjustOwnerCount(view(), ownerSle, -1, ctx_.journal);
     view().erase(slePd);
+
     return tesSUCCESS;
 }
 

diff --git a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp
@@ -86,10 +86,10 @@ PermissionedDomainSet::doApply()
     if (!ownerSle)
         return tefINTERNAL;
 
-    auto const sortedTX =
+    auto const sortedTxCredentials =
         credentials::makeSorted(ctx_.tx.getFieldArray(sfAcceptedCredentials));
-    STArray sortedLE(sfAcceptedCredentials, sortedTX.size());
-    for (auto const& p : sortedTX)
+    STArray sortedLE(sfAcceptedCredentials, sortedTxCredentials.size());
+    for (auto const& p : sortedTxCredentials)
     {
         auto cred = STObject::makeInnerObject(sfCredential);
         cred.setAccountID(sfIssuer, p.first);