Add fixAMMv1_2 amendment (#5176)

* Add reserve check on AMM Withdraw
* Try AMM max offer if changeSpotPriceQuality() fails

include/xrpl/protocol/Feature.h

@@ -80,7 +80,7 @@ namespace detail {
 // Feature.cpp. Because it's only used to reserve storage, and determine how
 // large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than
 // the actual number of amendments. A LogicError on startup will verify this.
-static constexpr std::size_t numFeatures = 104;
+static constexpr std::size_t numFeatures = 105;
 
 /** Amendments that this server supports and the default voting behavior.
    Whether they are enabled depends on the Rules defined in the validated

include/xrpl/protocol/detail/features.macro

@@ -29,6 +29,7 @@
 // If you add an amendment here, then do not forget to increment `numFeatures`
 // in include/xrpl/protocol/Feature.h.
 
+XRPL_FIX    (AMMv1_2,                    Supported::yes, VoteBehavior::DefaultNo)
 // InvariantsV1_1 will be changes to Supported::yes when all the
 // invariants expected to be included under it are complete.
 XRPL_FEATURE(MPTokensV1,                 Supported::yes, VoteBehavior::DefaultNo)

src/test/app/AMM_test.cpp

@@ -7064,6 +7064,55 @@ private:
         }
     }
 
+    void
+    testFixReserveCheckOnWithdrawal(FeatureBitset features)
+    {
+        testcase("Fix Reserve Check On Withdrawal");
+        using namespace jtx;
+
+        auto const err = features[fixAMMv1_2] ? ter(tecINSUFFICIENT_RESERVE)
+                                              : ter(tesSUCCESS);
+
+        auto test = [&](auto&& cb) {
+            Env env(*this, features);
+            auto const starting_xrp =
+                reserve(env, 2) + env.current()->fees().base * 5;
+            env.fund(starting_xrp, gw);
+            env.fund(starting_xrp, alice);
+            env.trust(USD(2'000), alice);
+            env.close();
+            env(pay(gw, alice, USD(2'000)));
+            env.close();
+            AMM amm(env, gw, EUR(1'000), USD(1'000));
+            amm.deposit(alice, USD(1));
+            cb(amm);
+        };
+
+        // Equal withdraw
+        test([&](AMM& amm) { amm.withdrawAll(alice, std::nullopt, err); });
+
+        // Equal withdraw with a limit
+        test([&](AMM& amm) {
+            amm.withdraw(WithdrawArg{
+                .account = alice,
+                .asset1Out = EUR(0.1),
+                .asset2Out = USD(0.1),
+                .err = err});
+            amm.withdraw(WithdrawArg{
+                .account = alice,
+                .asset1Out = USD(0.1),
+                .asset2Out = EUR(0.1),
+                .err = err});
+        });
+
+        // Single withdraw
+        test([&](AMM& amm) {
+            amm.withdraw(WithdrawArg{
+                .account = alice, .asset1Out = EUR(0.1), .err = err});
+            amm.withdraw(WithdrawArg{.account = alice, .asset1Out = USD(0.1)});
+        });
+    }
+
     void
     run() override
     {
@@ -7117,6 +7166,8 @@ private:
         testAMMDepositWithFrozenAssets(all);
         testAMMDepositWithFrozenAssets(all - featureAMMClawback);
         testAMMDepositWithFrozenAssets(all - fixAMMv1_1 - featureAMMClawback);
+        testFixReserveCheckOnWithdrawal(all);
+        testFixReserveCheckOnWithdrawal(all - fixAMMv1_2);
     }
 };
 

src/xrpld/app/paths/detail/AMMLiquidity.cpp

@@ -211,6 +211,13 @@ AMMLiquidity<TIn, TOut>::getOffer(
                 return AMMOffer<TIn, TOut>(
                     *this, *amounts, balances, Quality{*amounts});
             }
+            else if (view.rules().enabled(fixAMMv1_2))
+            {
+                if (auto const maxAMMOffer = maxOffer(balances, view.rules());
+                    maxAMMOffer &&
+                    Quality{maxAMMOffer->amount()} > *clobQuality)
+                    return maxAMMOffer;
+            }
         }
         catch (std::overflow_error const& e)
         {

src/xrpld/app/tx/detail/AMMClawback.cpp

@@ -188,6 +188,7 @@ AMMClawback::applyGuts(Sandbox& sb)
                 0,
                 FreezeHandling::fhIGNORE_FREEZE,
                 WithdrawAll::Yes,
+                mPriorBalance,
                 ctx_.journal);
     else
         std::tie(result, newLPTokenBalance, amountWithdraw, amount2Withdraw) =
@@ -267,6 +268,7 @@ AMMClawback::equalWithdrawMatchingOneAmount(
             0,
             FreezeHandling::fhIGNORE_FREEZE,
             WithdrawAll::Yes,
+            mPriorBalance,
             ctx_.journal);
 
     // Because we are doing a two-asset withdrawal,
@@ -284,6 +286,7 @@ AMMClawback::equalWithdrawMatchingOneAmount(
         0,
         FreezeHandling::fhIGNORE_FREEZE,
         WithdrawAll::No,
+        mPriorBalance,
         ctx_.journal);
 }
 

src/xrpld/app/tx/detail/AMMWithdraw.cpp

@@ -472,6 +472,7 @@ AMMWithdraw::withdraw(
         tfee,
         FreezeHandling::fhZERO_IF_FROZEN,
         isWithdrawAll(ctx_.tx),
+        mPriorBalance,
         j_);
     return {ter, newLPTokenBalance};
 }
@@ -490,6 +491,7 @@ AMMWithdraw::withdraw(
     std::uint16_t tfee,
     FreezeHandling freezeHandling,
     WithdrawAll withdrawAll,
+    XRPAmount const& priorBalance,
     beast::Journal const& journal)
 {
     auto const lpTokens = ammLPHolds(view, ammSle, account, journal);
@@ -589,6 +591,33 @@ AMMWithdraw::withdraw(
         return {tecAMM_BALANCE, STAmount{}, STAmount{}, STAmount{}};
     }
 
+    // Check the reserve in case a trustline has to be created
+    bool const enabledFixAMMv1_2 = view.rules().enabled(fixAMMv1_2);
+    auto sufficientReserve = [&](Issue const& issue) -> TER {
+        if (!enabledFixAMMv1_2 || isXRP(issue))
+            return tesSUCCESS;
+        if (!view.exists(keylet::line(account, issue)))
+        {
+            auto const sleAccount = view.read(keylet::account(account));
+            if (!sleAccount)
+                return tecINTERNAL;  // LCOV_EXCL_LINE
+            auto const balance = (*sleAccount)[sfBalance].xrp();
+            std::uint32_t const ownerCount = sleAccount->at(sfOwnerCount);
+
+            // See also SetTrust::doApply()
+            XRPAmount const reserve(
+                (ownerCount < 2) ? XRPAmount(beast::zero)
+                                 : view.fees().accountReserve(ownerCount + 1));
+
+            if (std::max(priorBalance, balance) < reserve)
+                return tecINSUFFICIENT_RESERVE;
+        }
+        return tesSUCCESS;
+    };
+
+    if (auto const err = sufficientReserve(amountWithdrawActual.issue()))
+        return {err, STAmount{}, STAmount{}, STAmount{}};
+
     // Withdraw amountWithdraw
     auto res = accountSend(
         view,
@@ -609,6 +638,10 @@ AMMWithdraw::withdraw(
     // Withdraw amount2Withdraw
     if (amount2WithdrawActual)
     {
+        if (auto const err = sufficientReserve(amount2WithdrawActual->issue());
+            err != tesSUCCESS)
+            return {err, STAmount{}, STAmount{}, STAmount{}};
+
         res = accountSend(
             view,
             ammAccount,
@@ -676,6 +709,7 @@ AMMWithdraw::equalWithdrawTokens(
             tfee,
             FreezeHandling::fhZERO_IF_FROZEN,
             isWithdrawAll(ctx_.tx),
+            mPriorBalance,
             ctx_.journal);
     return {ter, newLPTokenBalance};
 }
@@ -725,6 +759,7 @@ AMMWithdraw::equalWithdrawTokens(
     std::uint16_t tfee,
     FreezeHandling freezeHanding,
     WithdrawAll withdrawAll,
+    XRPAmount const& priorBalance,
     beast::Journal const& journal)
 {
     try
@@ -745,6 +780,7 @@ AMMWithdraw::equalWithdrawTokens(
                 tfee,
                 freezeHanding,
                 WithdrawAll::Yes,
+                priorBalance,
                 journal);
         }
 
@@ -773,6 +809,7 @@ AMMWithdraw::equalWithdrawTokens(
             tfee,
             freezeHanding,
             withdrawAll,
+            priorBalance,
             journal);
     }
     // LCOV_EXCL_START

src/xrpld/app/tx/detail/AMMWithdraw.h

@@ -96,6 +96,7 @@ public:
      * @param lpTokensWithdraw amount of tokens to withdraw
      * @param tfee trading fee in basis points
      * @param withdrawAll if withdrawing all lptokens
+     * @param priorBalance balance before fees
      * @return
      */
     static std::tuple<TER, STAmount, STAmount, std::optional<STAmount>>
@@ -112,6 +113,7 @@ public:
         std::uint16_t tfee,
         FreezeHandling freezeHanding,
         WithdrawAll withdrawAll,
+        XRPAmount const& priorBalance,
         beast::Journal const& journal);
 
     /** Withdraw requested assets and token from AMM into LP account.
@@ -127,6 +129,7 @@ public:
      * @param lpTokensWithdraw amount of lptokens to withdraw
      * @param tfee trading fee in basis points
      * @param withdrawAll if withdraw all lptokens
+     * @param priorBalance balance before fees
      * @return
      */
     static std::tuple<TER, STAmount, STAmount, std::optional<STAmount>>
@@ -143,6 +146,7 @@ public:
         std::uint16_t tfee,
         FreezeHandling freezeHandling,
         WithdrawAll withdrawAll,
+        XRPAmount const& priorBalance,
         beast::Journal const& journal);
 
     static std::pair<TER, bool>