fixAMMOfferRounding: generate offer's XRP side first, rounding it down:

A single-path AMM offer with account offer on DEX, is always generated
starting with the takerPays first, which is rounded up, and then
the takerGets, which is rounded down. This rounding ensures that the pool's
product invariant is maintained. However, when one of the offer's side
is XRP, this rounding can result in the AMM offer having a lower
quality, potentially causing offer generation to fail if the quality
is lower than the account's offer quality.

To address this issue, the proposed fix adjusts the offer generation process
to start with the XRP side first and always rounds it down. This results
in a smaller offer size, improving the offer's quality.
This change still ensures the product invariant, as the other generated
side is the exact result of the swap-in or swap-out equations.

src/ripple/app/misc/AMMHelpers.h

@@ -21,11 +21,15 @@
 #define RIPPLE_APP_MISC_AMMHELPERS_H_INCLUDED
 
 #include <ripple/basics/IOUAmount.h>
+#include <ripple/basics/Log.h>
 #include <ripple/basics/Number.h>
+#include <ripple/beast/utility/Journal.h>
 #include <ripple/protocol/AMMCore.h>
 #include <ripple/protocol/AmountConversions.h>
+#include <ripple/protocol/Feature.h>
 #include <ripple/protocol/Issue.h>
 #include <ripple/protocol/Quality.h>
+#include <ripple/protocol/Rules.h>
 #include <ripple/protocol/STAccount.h>
 #include <ripple/protocol/STAmount.h>
 
@@ -145,12 +149,131 @@ withinRelativeDistance(Amt const& calc, Amt const& req, Number const& dist)
 }
 // clang-format on
 
-/** Finds takerPays (i) and takerGets (o) such that given pool composition
- * poolGets(I) and poolPays(O): (O - o) / (I + i) = quality.
- * Where takerGets is calculated as the swapAssetIn (see below).
- * The above equation produces the quadratic equation:
- * i^2*(1-fee) + i*I*(2-fee) + I^2 - I*O/quality,
- * which is solved for i, and o is found with swapAssetIn().
+std::optional<Number>
+solveQuadraticEqSmallest(Number const& a, Number const& b, Number const& c);
+
+/** Generate AMM offer starting with takerGets when AMM pool
+ * from the payment perspective is IOU(in)/XRP(out)
+ * Equations:
+ * Spot Price Quality after the offer is consumed:
+ *     Qsp = (O - o) / (I + i) (1)
+ *  where O is poolPays, I is poolGets, o is takerGets, i is takerPays
+ * Swap out:
+ *     i = (I * o) / (O - o) * f (2)
+ *  where f is (1 - tfee/100000), tfee is in basis points
+ * Effective price targetQuality:
+ *     Qep = o / i (3)
+ * There are two scenarios to consider
+ * A) Qsp = Qep. Substitute i in (1) with (2) and solve for o
+ *    and Qsp = targetQuality(Qt):
+ *     o**2 + o * (I * Qt * (1 - 1 / f) - 2 * O) + O**2 - Qt * I * O = 0
+ * B) Qep = Qsp. Substitute i in (3) with (2) and solve for o
+ *    and Qep = targetQuality(Qt):
+ *     o = O - I * Qt / f
+ * Since the scenario is not know a priori, both A and B are solved and
+ * the lowest value of o is takerGets. takerPays is calculated with
+ * swap out eq (2). If o is less or equal to 0 then the offer can't
+ * be generated.
+ */
+template <typename TIn, typename TOut>
+std::optional<TAmounts<TIn, TOut>>
+getAMMOfferStartWithTakerGets(
+    TAmounts<TIn, TOut> const& pool,
+    Quality const& targetQuality,
+    std::uint16_t const& tfee)
+{
+    assert(targetQuality.rate() != beast::zero);
+    auto const f = feeMult(tfee);
+    auto const a = 1;
+    auto const b = pool.in * (1 - 1 / f) / targetQuality.rate() - 2 * pool.out;
+    auto const c =
+        pool.out * pool.out - pool.in * pool.out / targetQuality.rate();
+
+    auto nTakerGets = solveQuadraticEqSmallest(a, b, c);
+    if (!nTakerGets || *nTakerGets <= 0)
+        return std::nullopt;
+
+    auto const nTakerGetsConstraint =
+        pool.out - pool.in / (targetQuality.rate() * f);
+    if (nTakerGetsConstraint <= 0)
+        return std::nullopt;
+
+    // Pick the smallest to make the quality better
+    if (nTakerGetsConstraint < nTakerGets)
+        nTakerGets = nTakerGetsConstraint;
+
+    auto const takerGets = toAmount<TOut>(
+        getIssue(pool.out), *nTakerGets, Number::rounding_mode::downward);
+
+    return TAmounts<TIn, TOut>{swapAssetOut(pool, takerGets, tfee), takerGets};
+}
+
+/** Generate AMM offer starting with takerPays when AMM pool
+ * from the payment perspective is XRP(in)/IOU(out) or IOU(in)/IOU(out).
+ * Equations:
+ * Spot Price Quality after the offer is consumed:
+ *     Qsp = (O - o) / (I + i) (1)
+ *  where O is poolPays, I is poolGets, o is takerGets, i is takerPays
+ * Swap in:
+ *     o = (O * i * f) / (I + i * f) (2)
+ *  where f is (1 - tfee/100000), tfee is in basis points
+ * Effective price quality:
+ *     Qep = o / i (3)
+ * There are two scenarios to consider
+ * A) Qsp = Qep. Substitute o in (1) with (2) and solve for i
+ *    and Qsp = targetQuality(Qt):
+ *     i**2 * f + i * I * (1 + f) + I**2 - I * O / Qt = 0
+ * B) Qep = Qsp. Substitute i in (3) with (2) and solve for i
+ *    and Qep = targetQuality(Qt):
+ *     i = O / Qt - I / f
+ * Since the scenario is not know a priori, both A and B are solved and
+ * the lowest value of i is takerPays. takerGets is calculated with
+ * swap in eq (2). If i is less or equal to 0 then the offer can't
+ * be generated.
+ */
+template <typename TIn, typename TOut>
+std::optional<TAmounts<TIn, TOut>>
+getAMMOfferStartWithTakerPays(
+    TAmounts<TIn, TOut> const& pool,
+    Quality const& targetQuality,
+    std::uint16_t tfee)
+{
+    auto const f = feeMult(tfee);
+    auto const& a = f;
+    auto const b = pool.in * (1 + f);
+    auto const c =
+        pool.in * pool.in - pool.in * pool.out * targetQuality.rate();
+
+    auto nTakerPays = solveQuadraticEqSmallest(a, b, c);
+    if (!nTakerPays || nTakerPays <= 0)
+        return std::nullopt;
+
+    auto const nTakerPaysConstraint =
+        pool.out * targetQuality.rate() - pool.in / f;
+    if (nTakerPaysConstraint <= 0)
+        return std::nullopt;
+
+    // Pick the smallest to make the quality better
+    if (nTakerPaysConstraint < nTakerPays)
+        nTakerPays = nTakerPaysConstraint;
+
+    auto const takerPays = toAmount<TIn>(
+        getIssue(pool.in), *nTakerPays, Number::rounding_mode::downward);
+
+    return TAmounts<TIn, TOut>{takerPays, swapAssetIn(pool, takerPays, tfee)};
+}
+
+/**   Generate AMM offer so that either updated Spot Price Quality (SPQ)
+ * is equal to LOB quality (in this case AMM offer quality is
+ * better than LOB quality) or AMM offer is equal to LOB quality
+ * (in this case SPQ is better than LOB quality).
+ * Pre-amendment code calculates takerPays first. If takerGets is XRP,
+ * it is rounded down, which results in worse offer quality than
+ * LOB quality, and the offer might fail to generate.
+ * Post-amendment code calculates the XRP offer side first. The result
+ * is rounded down, which makes the offer quality better.
+ *   It might not be possible to match either SPQ or AMM offer to LOB
+ * quality. This generally happens at higher fees.
  * @param pool AMM pool balances
  * @param quality requested quality
  * @param tfee trading fee in basis points
@@ -161,43 +284,114 @@ std::optional<TAmounts<TIn, TOut>>
 changeSpotPriceQuality(
     TAmounts<TIn, TOut> const& pool,
     Quality const& quality,
-    std::uint16_t tfee)
+    std::uint16_t tfee,
+    Rules const& rules,
+    beast::Journal j)
 {
-    auto const f = feeMult(tfee);  // 1 - fee
-    auto const& a = f;
-    auto const b = pool.in * (1 + f);
-    Number const c = pool.in * pool.in - pool.in * pool.out * quality.rate();
-    if (auto const res = b * b - 4 * a * c; res < 0)
-        return std::nullopt;
-    else if (auto const nTakerPaysPropose = (-b + root2(res)) / (2 * a);
-             nTakerPaysPropose > 0)
+    if (!rules.enabled(fixAMMOfferRounding))
     {
-        auto const nTakerPays = [&]() {
-            // The fee might make the AMM offer quality less than CLOB quality.
-            // Therefore, AMM offer has to satisfy this constraint: o / i >= q.
-            // Substituting o with swapAssetIn() gives:
-            // i <= O / q - I / (1 - fee).
-            auto const nTakerPaysConstraint =
-                pool.out * quality.rate() - pool.in / f;
-            if (nTakerPaysPropose > nTakerPaysConstraint)
-                return nTakerPaysConstraint;
-            return nTakerPaysPropose;
-        }();
-        if (nTakerPays <= 0)
+        // Finds takerPays (i) and takerGets (o) such that given pool
+        // composition poolGets(I) and poolPays(O): (O - o) / (I + i) = quality.
+        // Where takerGets is calculated as the swapAssetIn (see below).
+        // The above equation produces the quadratic equation:
+        // i^2*(1-fee) + i*I*(2-fee) + I^2 - I*O/quality,
+        // which is solved for i, and o is found with swapAssetIn().
+        auto const f = feeMult(tfee);  // 1 - fee
+        auto const& a = f;
+        auto const b = pool.in * (1 + f);
+        Number const c =
+            pool.in * pool.in - pool.in * pool.out * quality.rate();
+        if (auto const res = b * b - 4 * a * c; res < 0)
             return std::nullopt;
-        auto const takerPays = toAmount<TIn>(
-            getIssue(pool.in), nTakerPays, Number::rounding_mode::upward);
-        // should not fail
-        if (auto const amounts =
-                TAmounts<TIn, TOut>{
-                    takerPays, swapAssetIn(pool, takerPays, tfee)};
-            Quality{amounts} < quality &&
-            !withinRelativeDistance(Quality{amounts}, quality, Number(1, -7)))
-            Throw<std::runtime_error>("changeSpotPriceQuality failed");
-        else
-            return amounts;
+        else if (auto const nTakerPaysPropose = (-b + root2(res)) / (2 * a);
+                 nTakerPaysPropose > 0)
+        {
+            auto const nTakerPays = [&]() {
+                // The fee might make the AMM offer quality less than CLOB
+                // quality. Therefore, AMM offer has to satisfy this constraint:
+                // o / i >= q. Substituting o with swapAssetIn() gives: i <= O /
+                // q - I / (1 - fee).
+                auto const nTakerPaysConstraint =
+                    pool.out * quality.rate() - pool.in / f;
+                if (nTakerPaysPropose > nTakerPaysConstraint)
+                    return nTakerPaysConstraint;
+                return nTakerPaysPropose;
+            }();
+            if (nTakerPays <= 0)
+            {
+                JLOG(j.trace())
+                    << "changeSpotPriceQuality negative: " << to_string(pool.in)
+                    << " " << to_string(pool.out) << " " << quality << " "
+                    << tfee;
+                return std::nullopt;
+            }
+            auto const takerPays = toAmount<TIn>(
+                getIssue(pool.in), nTakerPays, Number::rounding_mode::upward);
+            // should not fail
+            if (auto const amounts =
+                    TAmounts<TIn, TOut>{
+                        takerPays, swapAssetIn(pool, takerPays, tfee)};
+                Quality{amounts} < quality &&
+                !withinRelativeDistance(
+                    Quality{amounts}, quality, Number(1, -7)))
+            {
+                JLOG(j.error())
+                    << "changeSpotPriceQuality failed: " << to_string(pool.in)
+                    << " " << to_string(pool.out) << " "
+                    << to_string(amounts.in) << " " << to_string(amounts.out)
+                    << " " << quality << " " << tfee;
+                Throw<std::runtime_error>("changeSpotPriceQuality failed");
+            }
+            else
+            {
+                JLOG(j.trace())
+                    << "changeSpotPriceQuality succeeded: "
+                    << to_string(pool.in) << " " << to_string(pool.out) << " "
+                    << to_string(amounts.in) << " " << to_string(amounts.out)
+                    << " " << quality << " " << tfee;
+                return amounts;
+            }
+        }
+        JLOG(j.trace()) << "changeSpotPriceQuality negative: "
+                        << to_string(pool.in) << " " << to_string(pool.out)
+                        << " " << quality << " " << tfee;
+        return std::nullopt;
+    }
+
+    // Generate the offer starting with XRP side. Return seated offer amounts
+    // if the offer can be generated, otherwise nullopt.
+    auto const amounts = [&]() {
+        if (isXRP(getIssue(pool.out)))
+            return getAMMOfferStartWithTakerGets(pool, quality, tfee);
+        return getAMMOfferStartWithTakerPays(pool, quality, tfee);
+    }();
+    if (!amounts)
+    {
+        JLOG(j.trace()) << "changeSpotPrice negative: " << to_string(pool.in)
+                        << " " << to_string(pool.out) << " "
+                        << Number{1} / quality.rate() << " " << tfee
+                        << std::endl;
+        return std::nullopt;
     }
-    return std::nullopt;
+
+    // Might fail due to finite precision. Should the relative difference be
+    // allowed?
+    if (Quality{*amounts} < quality)
+    {
+        JLOG(j.error()) << "changeSpotPriceQuality failed: "
+                        << to_string(pool.in) << " " << to_string(pool.out)
+                        << " " << to_string(amounts->in) << " "
+                        << to_string(amounts->out) << " " << quality << " "
+                        << tfee;
+        return std::nullopt;
+    }
+
+    JLOG(j.trace()) << "changeSpotPriceQuality succeeded: "
+                    << to_string(pool.in) << " " << to_string(pool.out) << " "
+                    << to_string(amounts->in) << " " << to_string(amounts->out)
+                    << " " << quality << " " << tfee;
+
+    return amounts;
 }
 
 /** AMM pool invariant - the product (A * B) after swap in/out has to remain

src/ripple/app/misc/impl/AMMHelpers.cpp

@@ -203,4 +203,15 @@ solveQuadraticEq(Number const& a, Number const& b, Number const& c)
     return (-b + root2(b * b - 4 * a * c)) / (2 * a);
 }
 
+std::optional<Number>
+solveQuadraticEqSmallest(Number const& a, Number const& b, Number const& c)
+{
+    if (auto const r = b * b - 4 * a * c; r < 0)
+        return std::nullopt;
+    else if (b > 0)
+        return (-b + root2(r)) / (2 * a);
+    else
+        return (-b - root2(r)) / (2 * a);
+}
+
 }  // namespace ripple

src/ripple/app/paths/impl/AMMLiquidity.cpp

@@ -205,8 +205,8 @@ AMMLiquidity<TIn, TOut>::getOffer(
                 return maxOffer(balances, view.rules());
             }
             else if (
-                auto const amounts =
-                    changeSpotPriceQuality(balances, *clobQuality, tradingFee_))
+                auto const amounts = changeSpotPriceQuality(
+                    balances, *clobQuality, tradingFee_, view.rules(), j_))
             {
                 return AMMOffer<TIn, TOut>(
                     *this, *amounts, balances, Quality{*amounts});
@@ -239,7 +239,12 @@ AMMLiquidity<TIn, TOut>::getOffer(
             return offer;
         }
 
-        JLOG(j_.error()) << "AMMLiquidity::getOffer, failed";
+        JLOG(j_.error()) << "AMMLiquidity::getOffer, failed "
+                         << ammContext_.multiPath() << " "
+                         << ammContext_.curIters() << " "
+                         << (clobQuality ? clobQuality->rate() : STAmount{})
+                         << " " << to_string(balances.in) << " "
+                         << to_string(balances.out);
     }
 
     return std::nullopt;

src/ripple/app/paths/impl/BookStep.cpp

@@ -853,11 +853,18 @@ BookStep<TIn, TOut, TDerived>::tip(ReadView const& view) const
     BookTip bt(sb, book_);
     auto const clobQuality =
         bt.step(j_) ? std::optional<Quality>(bt.quality()) : std::nullopt;
-    // Don't pass in clobQuality. For one-path it returns the offer as
-    // the pool balances and the resulting quality is Spot Price Quality.
-    // For multi-path it returns the actual offer.
+    // Pass in clobQuality if single path. It might not be possible
+    // to generate AMM offer at this quality, in which case should use
+    // clobQuality. Multi-path generates actual offer with the quality
+    // calculated from the offer size.
+    auto const targetQuality = [&]() -> std::optional<Quality> {
+        if (view.rules().enabled(fixAMMOfferRounding) && ammLiquidity_ &&
+            !ammLiquidity_->context().multiPath())
+            return clobQuality;
+        return std::nullopt;
+    }();
     // AMM quality is better or no CLOB offer
-    if (auto const ammOffer = getAMMOffer(view, std::nullopt); ammOffer &&
+    if (auto const ammOffer = getAMMOffer(view, targetQuality); ammOffer &&
         ((clobQuality && ammOffer->quality() > clobQuality) || !clobQuality))
         return ammOffer;
     // CLOB quality is better or nullopt

src/ripple/app/tx/impl/AMMCreate.cpp

@@ -252,8 +252,8 @@ applyCreate(
             : 1};
     sleAMMRoot->setFieldU32(sfSequence, seqno);
     // Ignore reserves requirement, disable the master key, allow default
-    // rippling (AMM LPToken can be used as a token in another AMM, which must
-    // support payments and offer crossing), and enable deposit authorization to
+    // rippling (AMM LPToken can be used in payments and offer crossing but
+    // not as a token in another AMM), and enable deposit authorization to
     // prevent payments into AMM.
     // Note, that the trustlines created by AMM have 0 credit limit.
     // This prevents shifting the balance between accounts via AMM,

src/ripple/protocol/Feature.h

@@ -74,7 +74,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 = 71;
+static constexpr std::size_t numFeatures = 72;
 
 /** Amendments that this server supports and the default voting behavior.
    Whether they are enabled depends on the Rules defined in the validated
@@ -358,7 +358,7 @@ extern uint256 const fixAMMOverflowOffer;
 extern uint256 const featurePriceOracle;
 extern uint256 const fixEmptyDID;
 extern uint256 const fixXChainRewardRounding;
-extern uint256 const fixAMMOverflowOffer;
+extern uint256 const fixAMMOfferRounding;
 
 }  // namespace ripple