fix: Improve ValidAMM invariant (#7295)

include/xrpl/tx/invariants/AMMInvariant.h

@@ -15,7 +15,9 @@ class ValidAMM
     std::optional<AccountID> ammAccount_;
     std::optional<STAmount> lptAMMBalanceAfter_;
     std::optional<STAmount> lptAMMBalanceBefore_;
+    std::optional<STAmount> lptAMMBalanceBeforeDeletion_;
     bool ammPoolChanged_{false};
+    bool ammDeleted_{false};
 
 public:
     enum class ZeroAllowed : bool { No = false, Yes = true };
@@ -35,12 +37,17 @@ private:
     [[nodiscard]] bool
     finalizeCreate(STTx const&, ReadView const&, bool enforce, beast::Journal const&) const;
     [[nodiscard]] bool
-    finalizeDelete(bool enforce, TER res, beast::Journal const&) const;
+    finalizeDelete(bool enforce, bool enforceAMMDelete, TER res, beast::Journal const&) const;
     [[nodiscard]] bool
     finalizeDeposit(STTx const&, ReadView const&, bool enforce, beast::Journal const&) const;
     // Includes clawback
     [[nodiscard]] bool
-    finalizeWithdraw(STTx const&, ReadView const&, bool enforce, beast::Journal const&) const;
+    finalizeWithdraw(
+        STTx const&,
+        ReadView const&,
+        bool enforce,
+        bool enforceAMMDelete,
+        beast::Journal const&) const;
     [[nodiscard]] bool
     finalizeDEX(bool enforce, beast::Journal const&) const;
     [[nodiscard]] bool

src/libxrpl/tx/invariants/AMMInvariant.cpp

@@ -27,7 +27,14 @@ void
 ValidAMM::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after)
 {
     if (isDelete)
+    {
+        if (before && before->getType() == ltAMM)
+        {
+            ammDeleted_ = true;
+            lptAMMBalanceBeforeDeletion_ = before->getFieldAmount(sfLPTokenBalance);
+        }
         return;
+    }
 
     if (after)
     {
@@ -166,18 +173,60 @@ ValidAMM::finalizeCreate(
 }
 
 bool
-ValidAMM::finalizeDelete(bool enforce, TER res, beast::Journal const& j) const
+ValidAMM::finalizeDelete(bool enforce, bool enforceAMMDelete, TER res, beast::Journal const& j)
+    const
 {
     if (ammAccount_)
     {
         // LCOV_EXCL_START
-        std::string const msg = (isTesSuccess(res)) ? "AMM object is not deleted on tesSUCCESS"
-                                                    : "AMM object is changed on tecINCOMPLETE";
+        std::string const msg = (isTesSuccess(res)) ? "AMM object remained on tesSUCCESS"
+                                                    : "AMM object changed on tecINCOMPLETE";
         JLOG(j.error()) << "Invariant failed: AMMDelete failed, " << msg;
         if (enforce)
             return false;
         // LCOV_EXCL_STOP
     }
+    if (enforceAMMDelete)
+    {
+        if (isTesSuccess(res))
+        {
+            if (!ammDeleted_)
+            {
+                // LCOV_EXCL_START
+                JLOG(j.error())
+                    << "Invariant failed: AMMDelete failed, AMM object remained on tesSUCCESS";
+                return false;
+                // LCOV_EXCL_STOP
+            }
+            if (!lptAMMBalanceBeforeDeletion_)
+            {
+                // LCOV_EXCL_START
+                JLOG(j.error())
+                    << "Invariant failed: AMMDelete failed, AMM object deleted without LP balance";
+                return false;
+                // LCOV_EXCL_STOP
+            }
+            if (*lptAMMBalanceBeforeDeletion_ != beast::kZero)
+            {
+                // LCOV_EXCL_START
+                JLOG(j.error())
+                    << "Invariant failed: AMMDelete failed, AMM object deleted with non-zero LP "
+                       "balance: "
+                    << *lptAMMBalanceBeforeDeletion_;
+                return false;
+                // LCOV_EXCL_STOP
+            }
+        }
+        else if (ammDeleted_)
+        {
+            // AMM should only be fully deleted when AMMDelete returns tesSUCCESS.
+            // LCOV_EXCL_START
+            JLOG(j.error()) << "Invariant failed: AMMDelete failed, AMM object deleted when result "
+                               "is not tesSUCCESS";
+            return false;
+            // LCOV_EXCL_STOP
+        }
+    }
 
     return true;
 }
@@ -271,16 +320,20 @@ ValidAMM::finalizeWithdraw(
     xrpl::STTx const& tx,
     xrpl::ReadView const& view,
     bool enforce,
+    bool enforceAMMDelete,
     beast::Journal const& j) const
 {
-    if (!ammAccount_)
+    if (enforceAMMDelete && ammDeleted_)
     {
-        // Last Withdraw or Clawback deleted AMM
+        // Last Withdraw or Clawback can delete the AMM. We don't have to check
+        // the LPToken balance because a final AMMWithdraw or AMMClawback can
+        // redeem the remaining LP tokens and delete the AMM entry in the same
+        // transaction.
+        return true;
     }
-    else if (!generalInvariant(tx, view, ZeroAllowed::Yes, j))
+    if (ammAccount_ && !generalInvariant(tx, view, ZeroAllowed::Yes, j) && enforce)
     {
-        if (enforce)
-            return false;
+        return false;
     }
 
     return true;
@@ -300,6 +353,25 @@ ValidAMM::finalize(
         return true;
 
     bool const enforce = view.rules().enabled(fixAMMv1_3);
+    bool const enforceAMMDelete = view.rules().enabled(fixCleanup3_3_0);
+
+    // AMM can only be deleted by AMMWithdraw, AMMClawback, and AMMDelete
+    if (enforceAMMDelete && ammDeleted_)
+    {
+        switch (tx.getTxnType())
+        {
+            case ttAMM_WITHDRAW:
+            case ttAMM_CLAWBACK:
+            case ttAMM_DELETE:
+                break;
+            default:
+                // LCOV_EXCL_START
+                JLOG(j.error()) << "Invariant failed: AMM failed, unexpected AMM deletion by "
+                                << tx.getTxnType();
+                return false;
+                // LCOV_EXCL_STOP
+        }
+    }
 
     switch (tx.getTxnType())
     {
@@ -309,13 +381,13 @@ ValidAMM::finalize(
             return finalizeDeposit(tx, view, enforce, j);
         case ttAMM_CLAWBACK:
         case ttAMM_WITHDRAW:
-            return finalizeWithdraw(tx, view, enforce, j);
+            return finalizeWithdraw(tx, view, enforce, enforceAMMDelete, j);
         case ttAMM_BID:
             return finalizeBid(enforce, j);
         case ttAMM_VOTE:
             return finalizeVote(enforce, j);
         case ttAMM_DELETE:
-            return finalizeDelete(enforce, result, j);
+            return finalizeDelete(enforce, enforceAMMDelete, result, j);
         case ttCHECK_CASH:
         case ttOFFER_CREATE:
         case ttPAYMENT:

src/test/app/Invariants_test.cpp

@@ -51,6 +51,7 @@
 #include <xrpl/tx/ApplyContext.h>
 #include <xrpl/tx/Transactor.h>
 #include <xrpl/tx/applySteps.h>
+#include <xrpl/tx/invariants/AMMInvariant.h>
 #include <xrpl/tx/invariants/DirectoryInvariant.h>
 #include <xrpl/tx/invariants/VaultInvariant.h>
 
@@ -1277,6 +1278,87 @@ class Invariants_test : public beast::unit_test::Suite
             });
     }
 
+    void
+    testAMMDeleteInvariants(FeatureBitset features)
+    {
+        using namespace test::jtx;
+
+        bool const enforceAMMDelete = features[fixCleanup3_3_0];
+        testcase << "AMM delete invariants" + std::string(enforceAMMDelete ? " fix" : "");
+
+        Env env(*this, features);
+        Account const issuer{"issuer"};
+        Issue const lptIssue{Currency(0x4c50540000000000), issuer.id()};
+        STAmount const zeroLP{lptIssue, 0};
+        STAmount const nonZeroLP{lptIssue, 1};
+
+        auto const makeAMM = [](STAmount const& lptBalance) {
+            auto sleAMM = std::make_shared<SLE>(keylet::amm(uint256(1)));
+            sleAMM->setFieldAmount(sfLPTokenBalance, lptBalance);
+            return sleAMM;
+        };
+
+        auto const checkInvariant = [&](TxType txType,
+                                        TER result,
+                                        std::optional<STAmount> const& deletedLPBalance,
+                                        bool expected,
+                                        std::string const& expectedLog) {
+            test::StreamSink sink{beast::Severity::Warning};
+            beast::Journal const jlog{sink};
+            ValidAMM invariant;
+
+            if (deletedLPBalance)
+                invariant.visitEntry(true, makeAMM(*deletedLPBalance), nullptr);
+
+            bool const actual = invariant.finalize(
+                STTx{txType, [](STObject&) {}}, result, XRPAmount{}, *env.current(), jlog);
+
+            BEAST_EXPECTS(actual == expected, "unexpected AMM delete invariant result");
+            auto const messages = sink.messages().str();
+            auto const expectedLogWhenEnforced = enforceAMMDelete ? expectedLog : "";
+            if (!expectedLogWhenEnforced.empty())
+            {
+                BEAST_EXPECTS(messages.contains(expectedLogWhenEnforced), expectedLogWhenEnforced);
+            }
+            else
+            {
+                BEAST_EXPECTS(messages.empty(), messages);
+            }
+        };
+
+        checkInvariant(
+            ttPAYMENT,
+            tesSUCCESS,
+            nonZeroLP,
+            !enforceAMMDelete,
+            "Invariant failed: AMM failed, unexpected AMM deletion by");
+        checkInvariant(
+            ttAMM_DELETE,
+            tesSUCCESS,
+            std::nullopt,
+            !enforceAMMDelete,
+            "Invariant failed: AMMDelete failed, AMM object remained on tesSUCCESS");
+        checkInvariant(
+            ttAMM_DELETE,
+            tesSUCCESS,
+            nonZeroLP,
+            !enforceAMMDelete,
+            "Invariant failed: AMMDelete failed, AMM object deleted with non-zero LP balance");
+        checkInvariant(
+            ttAMM_DELETE,
+            tecINCOMPLETE,
+            zeroLP,
+            !enforceAMMDelete,
+            "Invariant failed: AMMDelete failed, AMM object deleted when result is not tesSUCCESS");
+
+        checkInvariant(ttAMM_WITHDRAW, tesSUCCESS, nonZeroLP, true, "");
+        checkInvariant(ttAMM_CLAWBACK, tesSUCCESS, nonZeroLP, true, "");
+
+        checkInvariant(ttAMM_DELETE, tesSUCCESS, zeroLP, true, "");
+        checkInvariant(ttAMM_WITHDRAW, tesSUCCESS, zeroLP, true, "");
+        checkInvariant(ttAMM_CLAWBACK, tesSUCCESS, zeroLP, true, "");
+    }
+
     static SLE::pointer
     createPermissionedDomain(
         ApplyContext& ac,
@@ -4900,6 +4982,8 @@ public:
         testNoZeroEscrow();
         testValidNewAccountRoot();
         testNFTokenPageInvariants();
+        testAMMDeleteInvariants(defaultAmendments());
+        testAMMDeleteInvariants(defaultAmendments() - fixCleanup3_3_0);
         testPermissionedDomainInvariants(defaultAmendments() | fixCleanup3_1_3);
         testPermissionedDomainInvariants(defaultAmendments() - fixCleanup3_1_3);
         testPermissionedDEX(defaultAmendments() | fixCleanup3_1_3);