fix clang-tidy

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

include/xrpl/tx/Transactor.h

@@ -7,6 +7,7 @@
 #include <xrpl/tx/ApplyContext.h>
 #include <xrpl/tx/applySteps.h>
 
+#include <tuple>
 #include <utility>
 
 namespace xrpl {
@@ -358,8 +359,13 @@ private:
 
     TER
     consumeSeqProxy(SLE::pointer const& sleAccount);
+
     TER
     payFee();
+
+    std::tuple<TER, XRPAmount, bool>
+    processPersistentChanges(TER result, XRPAmount fee);
+
     static NotTEC
     checkSingleSign(
         ReadView const& view,
@@ -367,6 +373,7 @@ private:
         AccountID const& idAccount,
         SLE::const_pointer sleAccount,
         beast::Journal const j);
+
     static NotTEC
     checkMultiSign(
         ReadView const& view,

src/libxrpl/tx/Transactor.cpp

@@ -44,8 +44,11 @@
 #include <cstddef>
 #include <cstdint>
 #include <exception>
+#include <map>
 #include <optional>
 #include <stdexcept>
+#include <tuple>
+#include <unordered_set>
 #include <utility>
 #include <vector>
 
@@ -1065,7 +1068,7 @@ removeDeletedMPTs(ApplyView& view, std::vector<uint256> const& mpts, beast::Jour
     for (auto const& index : mpts)
     {
         if (auto const sleState = view.peek({ltMPTOKEN, index}); sleState &&
-            deleteAMMMPToken(view, sleState, (*sleState)[sfIssuer], viewJ) != tesSUCCESS)
+            deleteAMMMPToken(view, sleState, (*sleState)[sfAccount], viewJ) != tesSUCCESS)
         {
             JLOG(viewJ.error()) << "removeDeletedMPTs: failed to delete AMM MPT";
         }
@@ -1134,6 +1137,120 @@ Transactor::trapTransaction(uint256 txHash) const
     JLOG(j_.debug()) << "Transaction trapped: " << txHash;
 }
 
+std::tuple<TER, XRPAmount, bool>
+Transactor::processPersistentChanges(TER result, XRPAmount fee)
+{
+    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.
+
+    auto typesForResult = [](TER const ter) {
+        std::unordered_set<LedgerEntryType> types;
+        if ((ter == tecOVERSIZE) || (ter == tecKILLED))
+            types.insert(ltOFFER);
+        if (ter == tecINCOMPLETE)
+        {
+            types.insert(ltRIPPLE_STATE);
+            types.insert(ltMPTOKEN);
+        }
+        if (ter == tecEXPIRED)
+        {
+            types.insert(ltNFTOKEN_OFFER);
+            types.insert(ltCREDENTIAL);
+        }
+        return types;
+    };
+
+    // 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.
+    auto const typesToCollect = typesForResult(result);
+
+    std::map<LedgerEntryType, std::vector<uint256>> deletedObjects;
+    if (!typesToCollect.empty())
+    {
+        ctx_.visit(
+            [&typesToCollect, &deletedObjects](
+                uint256 const& index, bool isDelete, SLE::const_ref before, SLE::const_ref after) {
+                if (isDelete)
+                {
+                    XRPL_ASSERT(
+                        before && after,
+                        "xrpl::Transactor::processPersistentChanges : 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, but only if the reset succeeded
+    // and the post-reset result still allows the same deletion type.
+    auto const typesToApply = typesForResult(result);
+    if (isTecClaim(result) && !typesToApply.empty())
+    {
+        auto const viewJ = ctx_.registry.get().getJournal("View");
+        for (auto const& [type, ids] : deletedObjects)
+        {
+            if (ids.empty() || !typesToApply.contains(type))
+                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
+            }
+        }
+    }
+
+    return {result, fee, isTecClaim(result)};
+}
+
 [[nodiscard]] TER
 Transactor::checkTransactionInvariants(TER result, XRPAmount fee)
 {
@@ -1183,6 +1300,7 @@ Transactor::checkInvariants(TER result, XRPAmount fee)
      */
     return ctx_.checkInvariants(result, fee);
 }
+
 //------------------------------------------------------------------------------
 ApplyResult
 Transactor::operator()()
@@ -1249,108 +1367,7 @@ Transactor::operator()()
         (result == tecOVERSIZE) || (result == tecKILLED) || (result == tecINCOMPLETE) ||
         (result == tecEXPIRED) || (isTecClaimHardFail(result, view().flags())))
     {
-        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,
-                           SLE::const_ref before,
-                           SLE::const_ref 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);
+        std::tie(result, fee, applied) = processPersistentChanges(result, fee);
     }
 
     if (applied)

src/test/app/NFToken_test.cpp

@@ -1120,6 +1120,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::Suite
         if (features[fixCleanup3_1_3])
         {
             buyerCount--;
+            BEAST_EXPECT(!env.closed()->exists(keylet::nftoffer(buyerExpOfferIndex)));
         }
         BEAST_EXPECT(ownerCount(env, buyer) == buyerCount);
 
@@ -1143,6 +1144,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::Suite
         if (features[fixCleanup3_1_3])
         {
             aliceCount--;
+            BEAST_EXPECT(!env.closed()->exists(keylet::nftoffer(aliceExpOfferIndex)));
         }
         BEAST_EXPECT(ownerCount(env, alice) == aliceCount);
         BEAST_EXPECT(ownerCount(env, buyer) == buyerCount);

src/test/app/Offer_test.cpp

@@ -797,11 +797,13 @@ public:
             // The offer expires (it's not removed yet).
             env.close();
             env.require(Owners(bob, 1), offers(bob, 1));
+            auto const expiredBobOffer = keylet::offer(bob, env.seq(bob) - 1);
 
             // bob creates the offer that will be crossed.
             env(offer(bob, usd(500), XRP(500)), Ter(tesSUCCESS));
             env.close();
             env.require(Owners(bob, 2), offers(bob, 2));
+            auto const crossedBobOffer = keylet::offer(bob, env.seq(bob) - 1);
 
             env(trust(alice, usd(1000)), Ter(tesSUCCESS));
             env(pay(gw, alice, usd(1000)), Ter(tesSUCCESS));
@@ -820,6 +822,8 @@ public:
                 Balance(bob, usd(kNone)),
                 Owners(bob, 1),
                 offers(bob, 1));
+            BEAST_EXPECT(!env.current()->exists(expiredBobOffer));
+            BEAST_EXPECT(env.current()->exists(crossedBobOffer));
 
             // Order that can be filled
             env(offer(alice, XRP(500), usd(500)), Txflags(tfFillOrKill), Ter(tesSUCCESS));
@@ -835,6 +839,27 @@ public:
                 offers(bob, 0));
         }
 
+        // A failed Fill-or-Kill may tentatively consume a funded offer before
+        // the transaction is reset. That offer must not be treated as an
+        // unfunded offer cleanup.
+        {
+            Env env{*this, features};
+
+            env.fund(startBalance, gw, alice, bob);
+            env.close();
+
+            env(offer(bob, usd(500), XRP(500)), Ter(tesSUCCESS));
+            env.close();
+            auto const bobOffer = keylet::offer(bob, env.seq(bob) - 1);
+
+            env(trust(alice, usd(1000)), Ter(tesSUCCESS));
+            env(pay(gw, alice, usd(1000)), Ter(tesSUCCESS));
+            env(offer(alice, XRP(1000), usd(1000)), Txflags(tfFillOrKill), Ter(tecKILLED));
+
+            env.require(offers(alice, 0), offers(bob, 1), Balance(alice, usd(1000)));
+            BEAST_EXPECT(env.current()->exists(bobOffer));
+        }
+
         // Immediate or Cancel - cross as much as possible
         // and add nothing on the books:
         {

src/test/app/SetRegularKey_test.cpp

@@ -72,6 +72,27 @@ public:
         env(regkey(alice, alice), Ter(temBAD_REGKEY));
     }
 
+    void
+    testNoAlternativeKey()
+    {
+        using namespace test::jtx;
+
+        testcase("Cannot remove last signing method");
+        Env env{*this, testableAmendments()};
+        Account const alice("alice");
+        Account const bob("bob");
+        env.fund(XRP(10000), alice);
+
+        env(regkey(alice, bob));
+        env(fset(alice, asfDisableMaster), Sig(alice));
+
+        env(regkey(alice, kDisabled), Sig(bob), Ter(tecNO_ALTERNATIVE_KEY));
+
+        auto const sle = env.le(alice);
+        BEAST_EXPECT(
+            sle && sle->isFlag(lsfDisableMaster) && sle->getAccountID(sfRegularKey) == bob.id());
+    }
+
     void
     testPasswordSpent()
     {
@@ -169,6 +190,7 @@ public:
     {
         testDisabledMasterKey();
         testDisabledRegularKey();
+        testNoAlternativeKey();
         testPasswordSpent();
         testUniversalMask();
         testTicketRegularKey();