fix amendment: AMM swap should honor invariants: (#5002)

The AMM has an invariant for swaps where:
new_balance_1*new_balance_2 >= old_balance_1*old_balance_2

Due to rounding, this invariant could sometimes be violated (although by
very small amounts).

This patch introduces an amendment `fixAMMRounding` that changes the
rounding to always favor the AMM. Doing this should maintain the
invariant.

Co-authored-by: Bronek Kozicki
Co-authored-by: thejohnfreeman

src/ripple/app/misc/AMMHelpers.h

@@ -24,8 +24,10 @@
 #include <ripple/basics/Number.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>
 
@@ -228,10 +230,62 @@ swapAssetIn(
     TIn const& assetIn,
     std::uint16_t tfee)
 {
-    return toAmount<TOut>(
-        getIssue(pool.out),
-        pool.out - (pool.in * pool.out) / (pool.in + assetIn * feeMult(tfee)),
-        Number::rounding_mode::downward);
+    if (auto const& rules = getCurrentTransactionRules();
+        rules && rules->enabled(fixAMMRounding))
+    {
+        // set rounding to always favor the amm. Clip to zero.
+        // calculate:
+        // pool.out -
+        // (pool.in * pool.out) / (pool.in + assetIn * feeMult(tfee)),
+        // and explicitly set the rounding modes
+        // Favoring the amm means we should:
+        // minimize:
+        // pool.out -
+        // (pool.in * pool.out) / (pool.in + assetIn * feeMult(tfee)),
+        // maximize:
+        // (pool.in * pool.out) / (pool.in + assetIn * feeMult(tfee)),
+        // (pool.in * pool.out)
+        // minimize:
+        // (pool.in + assetIn * feeMult(tfee)),
+        // minimize:
+        // assetIn * feeMult(tfee)
+        // feeMult is: (1-fee), fee is tfee/100000
+        // minimize:
+        // 1-fee
+        // maximize:
+        // fee
+        saveNumberRoundMode _{Number::getround()};
+
+        Number::setround(Number::upward);
+        auto const numerator = pool.in * pool.out;
+        auto const fee = getFee(tfee);
+
+        Number::setround(Number::downward);
+        auto const denom = pool.in + assetIn * (1 - fee);
+
+        if (denom.signum() <= 0)
+            return toAmount<TOut>(getIssue(pool.out), 0);
+
+        Number::setround(Number::upward);
+        auto const ratio = numerator / denom;
+
+        Number::setround(Number::downward);
+        auto const swapOut = pool.out - ratio;
+
+        if (swapOut.signum() < 0)
+            return toAmount<TOut>(getIssue(pool.out), 0);
+
+        return toAmount<TOut>(
+            getIssue(pool.out), swapOut, Number::rounding_mode::downward);
+    }
+    else
+    {
+        return toAmount<TOut>(
+            getIssue(pool.out),
+            pool.out -
+                (pool.in * pool.out) / (pool.in + assetIn * feeMult(tfee)),
+            Number::rounding_mode::downward);
+    }
 }
 
 /** Swap assetOut out of the pool and swap in a proportional amount
@@ -250,11 +304,62 @@ swapAssetOut(
     TOut const& assetOut,
     std::uint16_t tfee)
 {
-    return toAmount<TIn>(
-        getIssue(pool.in),
-        ((pool.in * pool.out) / (pool.out - assetOut) - pool.in) /
-            feeMult(tfee),
-        Number::rounding_mode::upward);
+    if (auto const& rules = getCurrentTransactionRules();
+        rules && rules->enabled(fixAMMRounding))
+    {
+        // set rounding to always favor the amm. Clip to zero.
+        // calculate:
+        // ((pool.in * pool.out) / (pool.out - assetOut) - pool.in) /
+        // (1-tfee/100000)
+        // maximize:
+        // ((pool.in * pool.out) / (pool.out - assetOut) - pool.in)
+        // maximize:
+        // (pool.in * pool.out) / (pool.out - assetOut)
+        // maximize:
+        // (pool.in * pool.out)
+        // minimize
+        // (pool.out - assetOut)
+        // minimize:
+        // (1-tfee/100000)
+        // maximize:
+        // tfee/100000
+
+        saveNumberRoundMode _{Number::getround()};
+
+        Number::setround(Number::upward);
+        auto const numerator = pool.in * pool.out;
+
+        Number::setround(Number::downward);
+        auto const denom = pool.out - assetOut;
+        if (denom.signum() <= 0)
+        {
+            return toMaxAmount<TIn>(getIssue(pool.in));
+        }
+
+        Number::setround(Number::upward);
+        auto const ratio = numerator / denom;
+        auto const numerator2 = ratio - pool.in;
+        auto const fee = getFee(tfee);
+
+        Number::setround(Number::downward);
+        auto const feeMult = 1 - fee;
+
+        Number::setround(Number::upward);
+        auto const swapIn = numerator2 / feeMult;
+        if (swapIn.signum() < 0)
+            return toAmount<TIn>(getIssue(pool.in), 0);
+
+        return toAmount<TIn>(
+            getIssue(pool.in), swapIn, Number::rounding_mode::upward);
+    }
+    else
+    {
+        return toAmount<TIn>(
+            getIssue(pool.in),
+            ((pool.in * pool.out) / (pool.out - assetOut) - pool.in) /
+                feeMult(tfee),
+            Number::rounding_mode::upward);
+    }
 }
 
 /** Return square of n.
@@ -263,12 +368,12 @@ Number
 square(Number const& n);
 
 /** Adjust LP tokens to deposit/withdraw.
- * Amount type keeps 16 digits. Maintaining the LP balance by adding deposited
- * tokens or subtracting withdrawn LP tokens from LP balance results in
- * losing precision in LP balance. I.e. the resulting LP balance
+ * Amount type keeps 16 digits. Maintaining the LP balance by adding
+ * deposited tokens or subtracting withdrawn LP tokens from LP balance
+ * results in losing precision in LP balance. I.e. the resulting LP balance
  * is less than the actual sum of LP tokens. To adjust for this, subtract
- * old tokens balance from the new one for deposit or vice versa for withdraw
- * to cancel out the precision loss.
+ * old tokens balance from the new one for deposit or vice versa for
+ * withdraw to cancel out the precision loss.
  * @param lptAMMBalance LPT AMM Balance
  * @param lpTokens LP tokens to deposit or withdraw
  * @param isDeposit true if deposit, false if withdraw

src/ripple/basics/Number.h

@@ -133,6 +133,13 @@ class Number
         return x.mantissa_ < y.mantissa_;
     }
 
+    /** Return the sign of the amount */
+    constexpr int
+    signum() const noexcept
+    {
+        return (mantissa_ < 0) ? -1 : (mantissa_ ? 1 : 0);
+    }
+
     friend constexpr bool
     operator>(Number const& x, Number const& y) noexcept
     {

src/ripple/ledger/impl/View.cpp

@@ -1147,7 +1147,17 @@ accountSend(
     beast::Journal j,
     WaiveTransferFee waiveFee)
 {
-    assert(saAmount >= beast::zero);
+    if (view.rules().enabled(fixAMMRounding))
+    {
+        if (saAmount < beast::zero)
+        {
+            return tecINTERNAL;
+        }
+    }
+    else
+    {
+        assert(saAmount >= beast::zero);
+    }
 
     /* If we aren't sending anything or if the sender is the same as the
      * receiver then we don't need to do anything.

src/ripple/protocol/AmountConversions.h

@@ -24,6 +24,8 @@
 #include <ripple/basics/XRPAmount.h>
 #include <ripple/protocol/STAmount.h>
 
+#include <type_traits>
+
 namespace ripple {
 
 inline STAmount
@@ -130,16 +132,44 @@ toAmount(
     saveNumberRoundMode rm(Number::getround());
     if (isXRP(issue))
         Number::setround(mode);
+
     if constexpr (std::is_same_v<IOUAmount, T>)
         return IOUAmount(n);
-    if constexpr (std::is_same_v<XRPAmount, T>)
+    else if constexpr (std::is_same_v<XRPAmount, T>)
         return XRPAmount(static_cast<std::int64_t>(n));
-    if constexpr (std::is_same_v<STAmount, T>)
+    else if constexpr (std::is_same_v<STAmount, T>)
     {
         if (isXRP(issue))
             return STAmount(issue, static_cast<std::int64_t>(n));
         return STAmount(issue, n.mantissa(), n.exponent());
     }
+    else
+    {
+        constexpr bool alwaysFalse = !std::is_same_v<T, T>;
+        static_assert(alwaysFalse, "Unsupported type for toAmount");
+    }
+}
+
+template <typename T>
+T
+toMaxAmount(Issue const& issue)
+{
+    if constexpr (std::is_same_v<IOUAmount, T>)
+        return IOUAmount(STAmount::cMaxValue, STAmount::cMaxOffset);
+    else if constexpr (std::is_same_v<XRPAmount, T>)
+        return XRPAmount(static_cast<std::int64_t>(STAmount::cMaxNativeN));
+    else if constexpr (std::is_same_v<STAmount, T>)
+    {
+        if (isXRP(issue))
+            return STAmount(
+                issue, static_cast<std::int64_t>(STAmount::cMaxNativeN));
+        return STAmount(issue, STAmount::cMaxValue, STAmount::cMaxOffset);
+    }
+    else
+    {
+        constexpr bool alwaysFalse = !std::is_same_v<T, T>;
+        static_assert(alwaysFalse, "Unsupported type for toMaxAmount");
+    }
 }
 
 inline STAmount
@@ -157,10 +187,15 @@ getIssue(T const& amt)
 {
     if constexpr (std::is_same_v<IOUAmount, T>)
         return noIssue();
-    if constexpr (std::is_same_v<XRPAmount, T>)
+    else if constexpr (std::is_same_v<XRPAmount, T>)
         return xrpIssue();
-    if constexpr (std::is_same_v<STAmount, T>)
+    else if constexpr (std::is_same_v<STAmount, T>)
         return amt.issue();
+    else
+    {
+        constexpr bool alwaysFalse = !std::is_same_v<T, T>;
+        static_assert(alwaysFalse, "Unsupported type for getIssue");
+    }
 }
 
 template <typename T>
@@ -169,10 +204,15 @@ get(STAmount const& a)
 {
     if constexpr (std::is_same_v<IOUAmount, T>)
         return a.iou();
-    if constexpr (std::is_same_v<XRPAmount, T>)
+    else if constexpr (std::is_same_v<XRPAmount, T>)
         return a.xrp();
-    if constexpr (std::is_same_v<STAmount, T>)
+    else if constexpr (std::is_same_v<STAmount, T>)
         return a;
+    else
+    {
+        constexpr bool alwaysFalse = !std::is_same_v<T, T>;
+        static_assert(alwaysFalse, "Unsupported type for get");
+    }
 }
 
 }  // namespace ripple

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

src/ripple/protocol/impl/Feature.cpp

@@ -466,6 +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);
 
 // The following amendments are obsolete, but must remain supported
 // because they could potentially get enabled.

src/ripple/protocol/impl/Rules.cpp

@@ -26,7 +26,7 @@
 namespace ripple {
 
 namespace {
-// Use a static inisde a function to help prevent order-of-initialization issues
+// Use a static inside a function to help prevent order-of-initialization issues
 LocalValue<std::optional<Rules>>&
 getCurrentTransactionRulesRef()
 {