fix: Add description for terLOCKED error (#6811)

include/xrpl/protocol/TER.h

@@ -344,10 +344,6 @@ enum TECcodes : TERUnderlyingType {
     tecLIMIT_EXCEEDED = 195,
     tecPSEUDO_ACCOUNT = 196,
     tecPRECISION_LOSS = 197,
-    // DEPRECATED: This error code tecNO_DELEGATE_PERMISSION is reserved for
-    // backward compatibility with historical data on non-prod networks, can be
-    // reclaimed after those networks reset.
-    tecNO_DELEGATE_PERMISSION = 198,
 };
 
 //------------------------------------------------------------------------------

include/xrpl/tx/Transactor.h

@@ -299,13 +299,8 @@ private:
 
     TER
     consumeSeqProxy(SLE::pointer const& sleAccount);
-
     TER
     payFee();
-
-    void
-    processPersistentChanges(TER& result, XRPAmount& fee, bool& applied);
-
     static NotTEC
     checkSingleSign(
         ReadView const& view,
@@ -313,7 +308,6 @@ private:
         AccountID const& idAccount,
         std::shared_ptr<SLE const> sleAccount,
         beast::Journal const j);
-
     static NotTEC
     checkMultiSign(
         ReadView const& view,

src/libxrpl/protocol/Permissions.cpp

@@ -67,6 +67,11 @@ Permission::Permission()
 #pragma pop_macro("PERMISSION")
     };
 
+    XRPL_ASSERT(
+        txFeatureMap_.size() == delegableTx_.size(),
+        "xrpl::Permission : txFeatureMap_ and delegableTx_ must have same "
+        "size");
+
     for ([[maybe_unused]] auto const& permission : granularPermissionMap_)
     {
         XRPL_ASSERT(

src/libxrpl/protocol/TER.cpp

@@ -215,6 +215,7 @@ transResults()
         MAKE_ERROR(terNO_AMM,                 "AMM doesn't exist for the asset pair."),
         MAKE_ERROR(terADDRESS_COLLISION,      "Failed to allocate an unique account address."),
         MAKE_ERROR(terNO_DELEGATE_PERMISSION, "Delegated account lacks permission to perform this transaction."),
+        MAKE_ERROR(terLOCKED,                 "Fund is locked."),
 
         MAKE_ERROR(tesSUCCESS,                "The transaction was applied. Only final in a validated ledger."),
     };

src/libxrpl/tx/Transactor.cpp

@@ -19,9 +19,6 @@
 #include <xrpl/tx/Transactor.h>
 #include <xrpl/tx/apply.h>
 
-#include <map>
-#include <unordered_set>
-
 namespace xrpl {
 
 /** Performs early sanity checks on the txid */
@@ -1091,110 +1088,6 @@ Transactor::trapTransaction(uint256 txHash) const
     JLOG(j_.debug()) << "Transaction trapped: " << txHash;
 }
 
-void
-Transactor::processPersistentChanges(TER& result, XRPAmount& fee, bool& applied)
-{
-    JLOG(j_.trace()) << "reapplying because of " << transToken(result);
-
-    // FIXME: This mechanism for doing work while returning a `tec` is
-    //        awkward and very limiting. A more general purpose approach
-    //        should be used, making it possible to do more useful work
-    //        when transactions fail with a `tec` code.
-
-    // Build a list of ledger entry types to collect, based on the
-    // result code. Only deleted objects of these types will be
-    // re-applied after the context is reset.
-    std::unordered_set<LedgerEntryType> typesToCollect;
-    if ((result == tecOVERSIZE) || (result == tecKILLED))
-        typesToCollect.insert(ltOFFER);
-    if (result == tecINCOMPLETE)
-        typesToCollect.insert(ltRIPPLE_STATE);
-    typesToCollect.insert(ltMPTOKEN);
-    if (result == tecEXPIRED)
-    {
-        typesToCollect.insert(ltNFTOKEN_OFFER);
-        typesToCollect.insert(ltCREDENTIAL);
-    }
-
-    std::map<LedgerEntryType, std::vector<uint256>> deletedObjects;
-    if (!typesToCollect.empty())
-    {
-        ctx_.visit([&typesToCollect, &deletedObjects](
-                       uint256 const& index,
-                       bool isDelete,
-                       std::shared_ptr<SLE const> const& before,
-                       std::shared_ptr<SLE const> const& after) {
-            if (isDelete)
-            {
-                XRPL_ASSERT(
-                    before && after,
-                    "xrpl::Transactor::operator()::visit : non-null SLE "
-                    "inputs");
-                if (before && after)
-                {
-                    auto const type = before->getType();
-                    if (typesToCollect.contains(type))
-                    {
-                        // For offers, only collect unfunded removals
-                        // (where TakerPays is unchanged)
-                        if (type == ltOFFER &&
-                            before->getFieldAmount(sfTakerPays) !=
-                                after->getFieldAmount(sfTakerPays))
-                            return;
-
-                        deletedObjects[type].push_back(index);
-                    }
-                }
-            }
-        });
-    }
-
-    // Reset the context, potentially adjusting the fee.
-    {
-        auto const resetResult = reset(fee);
-        if (!isTesSuccess(resetResult.first))
-            result = resetResult.first;
-
-        fee = resetResult.second;
-    }
-
-    // Re-apply the collected deletions
-    auto const viewJ = ctx_.registry.get().getJournal("View");
-    for (auto const& [type, ids] : deletedObjects)
-    {
-        if (ids.empty())
-            continue;
-
-        switch (type)
-        {
-            case ltOFFER:
-                removeUnfundedOffers(view(), ids, viewJ);
-                break;
-            case ltNFTOKEN_OFFER:
-                removeExpiredNFTokenOffers(view(), ids, viewJ);
-                break;
-            case ltRIPPLE_STATE:
-                removeDeletedTrustLines(view(), ids, viewJ);
-                break;
-            case ltMPTOKEN:
-                removeDeletedMPTs(view(), ids, viewJ);
-                break;
-            case ltCREDENTIAL:
-                removeExpiredCredentials(view(), ids, viewJ);
-                break;
-            // LCOV_EXCL_START
-            default:
-                UNREACHABLE(
-                    "xrpl::Transactor::processPersistentChanges() : "
-                    "unexpected type");
-                break;
-                // LCOV_EXCL_STOP
-        }
-    }
-
-    applied = isTecClaim(result);
-}
-
 //------------------------------------------------------------------------------
 ApplyResult
 Transactor::operator()()
@@ -1263,7 +1156,108 @@ Transactor::operator()()
         (result == tecOVERSIZE) || (result == tecKILLED) || (result == tecINCOMPLETE) ||
         (result == tecEXPIRED) || (isTecClaimHardFail(result, view().flags())))
     {
-        processPersistentChanges(result, fee, applied);
+        JLOG(j_.trace()) << "reapplying because of " << transToken(result);
+
+        // FIXME: This mechanism for doing work while returning a `tec` is
+        //        awkward and very limiting. A more general purpose approach
+        //        should be used, making it possible to do more useful work
+        //        when transactions fail with a `tec` code.
+        std::vector<uint256> removedOffers;
+        std::vector<uint256> removedTrustLines;
+        std::vector<uint256> removedMPTs;
+        std::vector<uint256> expiredNFTokenOffers;
+        std::vector<uint256> expiredCredentials;
+
+        bool const doOffers = ((result == tecOVERSIZE) || (result == tecKILLED));
+        bool const doLinesOrMPTs = (result == tecINCOMPLETE);
+        bool const doNFTokenOffers = (result == tecEXPIRED);
+        bool const doCredentials = (result == tecEXPIRED);
+        if (doOffers || doLinesOrMPTs || doNFTokenOffers || doCredentials)
+        {
+            ctx_.visit([doOffers,
+                        &removedOffers,
+                        doLinesOrMPTs,
+                        &removedTrustLines,
+                        &removedMPTs,
+                        doNFTokenOffers,
+                        &expiredNFTokenOffers,
+                        doCredentials,
+                        &expiredCredentials](
+                           uint256 const& index,
+                           bool isDelete,
+                           std::shared_ptr<SLE const> const& before,
+                           std::shared_ptr<SLE const> const& after) {
+                if (isDelete)
+                {
+                    XRPL_ASSERT(
+                        before && after,
+                        "xrpl::Transactor::operator()::visit : non-null SLE "
+                        "inputs");
+                    if (doOffers && before && after && (before->getType() == ltOFFER) &&
+                        (before->getFieldAmount(sfTakerPays) == after->getFieldAmount(sfTakerPays)))
+                    {
+                        // Removal of offer found or made unfunded
+                        removedOffers.push_back(index);
+                    }
+
+                    if (doLinesOrMPTs && before && after)
+                    {
+                        // Removal of obsolete AMM trust line
+                        if (before->getType() == ltRIPPLE_STATE)
+                        {
+                            removedTrustLines.push_back(index);
+                        }
+                        else if (before->getType() == ltMPTOKEN)
+                        {
+                            removedMPTs.push_back(index);
+                        }
+                    }
+
+                    if (doNFTokenOffers && before && after &&
+                        (before->getType() == ltNFTOKEN_OFFER))
+                        expiredNFTokenOffers.push_back(index);
+
+                    if (doCredentials && before && after && (before->getType() == ltCREDENTIAL))
+                        expiredCredentials.push_back(index);
+                }
+            });
+        }
+
+        // Reset the context, potentially adjusting the fee.
+        {
+            auto const resetResult = reset(fee);
+            if (!isTesSuccess(resetResult.first))
+                result = resetResult.first;
+
+            fee = resetResult.second;
+        }
+
+        // If necessary, remove any offers found unfunded during processing
+        if ((result == tecOVERSIZE) || (result == tecKILLED))
+        {
+            removeUnfundedOffers(view(), removedOffers, ctx_.registry.get().getJournal("View"));
+        }
+
+        if (result == tecEXPIRED)
+        {
+            removeExpiredNFTokenOffers(
+                view(), expiredNFTokenOffers, ctx_.registry.get().getJournal("View"));
+        }
+
+        if (result == tecINCOMPLETE)
+        {
+            removeDeletedTrustLines(
+                view(), removedTrustLines, ctx_.registry.get().getJournal("View"));
+            removeDeletedMPTs(view(), removedMPTs, ctx_.registry.get().getJournal("View"));
+        }
+
+        if (result == tecEXPIRED)
+        {
+            removeExpiredCredentials(
+                view(), expiredCredentials, ctx_.registry.get().getJournal("View"));
+        }
+
+        applied = isTecClaim(result);
     }
 
     if (applied)

src/libxrpl/tx/transactors/delegate/DelegateUtils.cpp

@@ -6,7 +6,7 @@ NotTEC
 checkTxPermission(std::shared_ptr<SLE const> const& delegate, STTx const& tx)
 {
     if (!delegate)
-        return terNO_DELEGATE_PERMISSION;  // LCOV_EXCL_LINE
+        return terNO_DELEGATE_PERMISSION;
 
     auto const permissionArray = delegate->getFieldArray(sfPermissions);
     auto const txPermission = tx.getTxnType() + 1;
@@ -28,7 +28,7 @@ loadGranularPermission(
     std::unordered_set<GranularPermissionType>& granularPermissions)
 {
     if (!delegate)
-        return;  // LCOV_EXCL_LINE
+        return;
 
     auto const permissionArray = delegate->getFieldArray(sfPermissions);
     for (auto const& permission : permissionArray)

src/libxrpl/tx/transactors/payment/Payment.cpp

@@ -265,6 +265,7 @@ Payment::checkPermission(ReadView const& view, STTx const& tx)
         tx.isFieldPresent(sfPaths))
         return terNO_DELEGATE_PERMISSION;
 
+    // PaymentMint and PaymentBurn apply to both IOU and MPT direct payments.
     if (granularPermissions.contains(PaymentMint) && !isXRP(amountAsset) &&
         amountAsset.getIssuer() == tx[sfAccount])
         return tesSUCCESS;

src/test/app/Batch_test.cpp

@@ -4155,8 +4155,12 @@ class Batch_test : public beast::unit_test::suite
             std::vector<TestLedgerData> const testCases = {
                 {0, "Batch", "tesSUCCESS", batchID, std::nullopt},
                 {1, "TrustSet", "tesSUCCESS", txIDs[0], batchID},
+                // jv2 fails with terNO_DELEGATE_PERMISSION.
             };
             validateClosedLedger(env, testCases);
+
+            // verify jv2 is not present in the closed ledger.
+            BEAST_EXPECT(env.rpc("tx", txIDs[1])[jss::result][jss::error] == "txnNotFound");
         }
     }
 

src/test/app/Delegate_test.cpp

@@ -2,6 +2,7 @@
 #include <test/jtx/CaptureLogs.h>
 #include <test/jtx/delegate.h>
 
+#include <xrpl/ledger/helpers/DelegateHelpers.h>
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Permissions.h>
 
@@ -1856,6 +1857,21 @@ class Delegate_test : public beast::unit_test::suite
                 "\n  Action: Verify security requirements to interact with Delegation feature");
     }
 
+    void
+    testDelegateUtilsNullptrCheck()
+    {
+        testcase("DelegateUtils nullptr check");
+
+        // checkTxPermission nullptr check
+        STTx const tx{ttPAYMENT, [](STObject&) {}};
+        BEAST_EXPECT(checkTxPermission(nullptr, tx) == terNO_DELEGATE_PERMISSION);
+
+        // loadGranularPermission nullptr check
+        std::unordered_set<GranularPermissionType> granularPermissions;
+        loadGranularPermission(nullptr, ttPAYMENT, granularPermissions);
+        BEAST_EXPECT(granularPermissions.empty());
+    }
+
     void
     run() override
     {
@@ -1881,6 +1897,7 @@ class Delegate_test : public beast::unit_test::suite
         testPermissionValue(all);
         testTxRequireFeatures(all);
         testTxDelegableCount();
+        testDelegateUtilsNullptrCheck();
     }
 };
 BEAST_DEFINE_TESTSUITE(Delegate, app, xrpl);