Add the fixAMMOfferRounding amendment: (#4983)

* Fix AMM offer rounding and low quality LOB offer blocking AMM:

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. Regardless if the offer
has XRP or not, the rounding is done so that the offer size is minimized.
This change still ensures the product invariant, as the other generated
side is the exact result of the swap-in or swap-out equations.

If a liquidity can be provided by both AMM and LOB offer on offer crossing
then AMM offer is generated so that it matches LOB offer quality. If LOB
offer quality is less than limit quality then generated AMM offer quality
is also less than limit quality and the offer doesn't cross. To address
this issue, if LOB quality is better than limit quality then use LOB
quality to generate AMM offer. Otherwise, don't use the quality to generate
AMM offer. In this case, limitOut() function in StrandFlow limits
the out amount to match strand's quality to limit quality and consume
maximum AMM liquidity.

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

@@ -203,4 +203,19 @@ solveQuadraticEq(Number const& a, Number const& b, Number const& c)
     return (-b + root2(b * b - 4 * a * c)) / (2 * a);
 }
 
+// Minimize takerGets or takerPays
+std::optional<Number>
+solveQuadraticEqSmallest(Number const& a, Number const& b, Number const& c)
+{
+    auto const d = b * b - 4 * a * c;
+    if (d < 0)
+        return std::nullopt;
+    // use numerically stable citardauq formula for quadratic equation solution
+    // https://people.csail.mit.edu/bkph/articles/Quadratics.pdf
+    if (b > 0)
+        return (2 * c) / (-b - root2(d));
+    else
+        return (2 * c) / (-b + root2(d));
+}
+
 }  // 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

@@ -297,6 +297,14 @@ class BookPaymentStep : public BookStep<TIn, TOut, BookPaymentStep<TIn, TOut>>
         return true;
     }
 
+    // A payment doesn't use quality threshold (limitQuality)
+    // since the strand's quality doesn't directly relate to the step's quality.
+    std::optional<Quality>
+    qualityThreshold(Quality const& lobQuality) const
+    {
+        return lobQuality;
+    }
+
     // For a payment ofrInRate is always the same as trIn.
     std::uint32_t
     getOfrInRate(Step const*, AccountID const&, std::uint32_t trIn) const
@@ -450,6 +458,25 @@ class BookOfferCrossingStep
         return !defaultPath_ || quality >= qualityThreshold_;
     }
 
+    // Return quality threshold or nullopt to use when generating AMM offer.
+    // AMM synthetic offer is generated to match LOB offer quality.
+    // If LOB tip offer quality is less than qualityThreshold
+    // then generated AMM offer quality is also less than qualityThreshold and
+    // the offer is not crossed even though AMM might generate a better quality
+    // offer. To address this, if qualityThreshold is greater than lobQuality
+    // then don't use quality to generate the AMM offer. The limit out value
+    // generates the maximum AMM offer in this case, which matches
+    // the quality threshold. This only applies to single path scenario.
+    // Multi-path AMM offers work the same as LOB offers.
+    std::optional<Quality>
+    qualityThreshold(Quality const& lobQuality) const
+    {
+        if (this->ammLiquidity_ && !this->ammLiquidity_->multiPath() &&
+            qualityThreshold_ > lobQuality)
+            return std::nullopt;
+        return lobQuality;
+    }
+
     // For offer crossing don't pay the transfer fee if alice is paying alice.
     // A regular (non-offer-crossing) payment does not apply this rule.
     std::uint32_t
@@ -758,8 +785,16 @@ BookStep<TIn, TOut, TDerived>::forEachOffer(
     };
 
     // At any payment engine iteration, AMM offer can only be consumed once.
-    auto tryAMM = [&](std::optional<Quality> const& quality) -> bool {
-        auto ammOffer = getAMMOffer(sb, quality);
+    auto tryAMM = [&](std::optional<Quality> const& lobQuality) -> bool {
+        // If offer crossing then use either LOB quality or nullopt
+        // to prevent AMM being blocked by a lower quality LOB.
+        auto const qualityThreshold = [&]() -> std::optional<Quality> {
+            if (sb.rules().enabled(fixAMMv1_1) && lobQuality)
+                return static_cast<TDerived const*>(this)->qualityThreshold(
+                    *lobQuality);
+            return lobQuality;
+        }();
+        auto ammOffer = getAMMOffer(sb, qualityThreshold);
         return !ammOffer || execOffer(*ammOffer);
     };
 
@@ -776,7 +811,7 @@ BookStep<TIn, TOut, TDerived>::forEachOffer(
     }
     else
     {
-        // Might have AMM offer if there is no CLOB offers.
+        // Might have AMM offer if there are no LOB offers.
         tryAMM(std::nullopt);
     }
 
@@ -851,17 +886,37 @@ BookStep<TIn, TOut, TDerived>::tip(ReadView const& view) const
     // This can be simplified (and sped up) if directories are never empty.
     Sandbox sb(&view, tapNONE);
     BookTip bt(sb, book_);
-    auto const clobQuality =
+    auto const lobQuality =
         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.
-    // AMM quality is better or no CLOB offer
-    if (auto const ammOffer = getAMMOffer(view, std::nullopt); ammOffer &&
-        ((clobQuality && ammOffer->quality() > clobQuality) || !clobQuality))
+    // Multi-path offer generates an offer with the quality
+    // calculated from the offer size and the quality is constant in this case.
+    // Single path offer quality changes with the offer size. Spot price quality
+    // (SPQ) can't be used in this case as the upper bound quality because
+    // even if SPQ quality is better than LOB quality, it might not be possible
+    // to generate AMM offer at or better quality than LOB quality. Another
+    // factor to consider is limit quality on offer crossing. If LOB quality
+    // is greater than limit quality then use LOB quality when generating AMM
+    // offer, otherwise don't use quality threshold when generating AMM offer.
+    // AMM or LOB offer, whether multi-path or single path then can be selected
+    // based on the best offer quality. Using the quality to generate AMM offer
+    // in this case also prevents the payment engine from going into multiple
+    // iterations to cross a LOB offer. This happens when AMM changes
+    // the out amount at the start of iteration to match the limitQuality
+    // on offer crossing but AMM can't generate the offer at this quality,
+    // as the result a LOB offer is partially crossed, and it might take a few
+    // iterations to fully cross the offer.
+    auto const qualityThreshold = [&]() -> std::optional<Quality> {
+        if (view.rules().enabled(fixAMMv1_1) && lobQuality)
+            return static_cast<TDerived const*>(this)->qualityThreshold(
+                *lobQuality);
+        return std::nullopt;
+    }();
+    // AMM quality is better or no LOB offer
+    if (auto const ammOffer = getAMMOffer(view, qualityThreshold); ammOffer &&
+        ((lobQuality && ammOffer->quality() > lobQuality) || !lobQuality))
         return ammOffer;
-    // CLOB quality is better or nullopt
-    return clobQuality;
+    // LOB quality is better or nullopt
+    return lobQuality;
 }
 
 template <class TIn, class TOut, class TDerived>

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/basics/Number.h

@@ -387,6 +387,26 @@ class saveNumberRoundMode
     operator=(saveNumberRoundMode const&) = delete;
 };
 
+// saveNumberRoundMode doesn't do quite enough for us.  What we want is a
+// Number::RoundModeGuard that sets the new mode and restores the old mode
+// when it leaves scope.  Since Number doesn't have that facility, we'll
+// build it here.
+class NumberRoundModeGuard
+{
+    saveNumberRoundMode saved_;
+
+public:
+    explicit NumberRoundModeGuard(Number::rounding_mode mode) noexcept
+        : saved_{Number::setround(mode)}
+    {
+    }
+
+    NumberRoundModeGuard(NumberRoundModeGuard const&) = delete;
+
+    NumberRoundModeGuard&
+    operator=(NumberRoundModeGuard const&) = delete;
+};
+
 }  // namespace ripple
 
 #endif  // RIPPLE_BASICS_NUMBER_H_INCLUDED

src/ripple/ledger/impl/View.cpp

@@ -1147,7 +1147,7 @@ accountSend(
     beast::Journal j,
     WaiveTransferFee waiveFee)
 {
-    if (view.rules().enabled(fixAMMRounding))
+    if (view.rules().enabled(fixAMMv1_1))
     {
         if (saAmount < beast::zero)
         {

src/ripple/protocol/Feature.h

@@ -359,7 +359,7 @@ extern uint256 const featurePriceOracle;
 extern uint256 const fixEmptyDID;
 extern uint256 const fixXChainRewardRounding;
 extern uint256 const fixPreviousTxnID;
-extern uint256 const fixAMMRounding;
+extern uint256 const fixAMMv1_1;
 
 }  // namespace ripple
 

src/ripple/protocol/impl/Feature.cpp

@@ -466,7 +466,7 @@ REGISTER_FEATURE(PriceOracle,                   Supported::yes, VoteBehavior::De
 REGISTER_FIX    (fixEmptyDID,                   Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FIX    (fixXChainRewardRounding,       Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FIX    (fixPreviousTxnID,              Supported::yes, VoteBehavior::DefaultNo);
-REGISTER_FIX    (fixAMMRounding,                Supported::yes, VoteBehavior::DefaultNo);
+REGISTER_FIX    (fixAMMv1_1,                Supported::yes, VoteBehavior::DefaultNo);
 
 // The following amendments are obsolete, but must remain supported
 // because they could potentially get enabled.

src/ripple/protocol/impl/STAmount.cpp

@@ -1367,26 +1367,6 @@ canonicalizeRoundStrict(
 
 namespace {
 
-// saveNumberRoundMode doesn't do quite enough for us.  What we want is a
-// Number::RoundModeGuard that sets the new mode and restores the old mode
-// when it leaves scope.  Since Number doesn't have that facility, we'll
-// build it here.
-class NumberRoundModeGuard
-{
-    saveNumberRoundMode saved_;
-
-public:
-    explicit NumberRoundModeGuard(Number::rounding_mode mode) noexcept
-        : saved_{Number::setround(mode)}
-    {
-    }
-
-    NumberRoundModeGuard(NumberRoundModeGuard const&) = delete;
-
-    NumberRoundModeGuard&
-    operator=(NumberRoundModeGuard const&) = delete;
-};
-
 // We need a class that has an interface similar to NumberRoundModeGuard
 // but does nothing.
 class DontAffectNumberRoundMode

src/test/app/AMMCalc_test.cpp

@@ -413,13 +413,18 @@ class AMMCalc_test : public beast::unit_test::suite
             //     10 is AMM trading fee
             else if (*p == "changespq")
             {
+                Env env(*this);
                 if (auto const pool = getAmounts(++p))
                 {
                     if (auto const offer = getAmounts(p))
                     {
                         auto const fee = getFee(p);
                         if (auto const ammOffer = changeSpotPriceQuality(
-                                pool->first, Quality{offer->first}, fee);
+                                pool->first,
+                                Quality{offer->first},
+                                fee,
+                                env.current()->rules(),
+                                beast::Journal(beast::Journal::getNullSink()));
                             ammOffer)
                             std::cout
                                 << "amm offer: " << toString(ammOffer->in)