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>
 
@@ -227,12 +229,64 @@ swapAssetIn(
     TAmounts<TIn, TOut> const& pool,
     TIn const& assetIn,
     std::uint16_t tfee)
+{
+    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)),
+            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
  * of the other asset. Implements AMM Swap out.
@@ -249,6 +303,56 @@ swapAssetOut(
     TAmounts<TIn, TOut> const& pool,
     TOut const& assetOut,
     std::uint16_t tfee)
+{
+    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),
@@ -256,6 +360,7 @@ swapAssetOut(
                 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
+ * Amount type keeps 16 digits. Maintaining the LP balance by adding
- * tokens or subtracting withdrawn LP tokens from LP balance results in
+ * deposited tokens or subtracting withdrawn LP tokens from LP balance
- * losing precision in LP balance. I.e. the resulting 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
+ * old tokens balance from the new one for deposit or vice versa for
- * to cancel out the precision loss.
+ * 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 @@ public:
         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

@@ -1146,8 +1146,18 @@ accountSend(
     STAmount const& saAmount,
     beast::Journal j,
     WaiveTransferFee waiveFee)
+{
+    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()
 {

src/test/app/AMMExtended_test.cpp

@@ -24,6 +24,7 @@
 #include <ripple/app/paths/impl/StrandFlow.h>
 #include <ripple/ledger/PaymentSandbox.h>
 #include <ripple/protocol/AMMCore.h>
+#include <ripple/protocol/Feature.h>
 #include <ripple/protocol/STParsedJSON.h>
 #include <ripple/resource/Fees.h>
 #include <ripple/rpc/RPCHandler.h>
@@ -93,10 +94,20 @@ private:
             sendmax(BTC(1'000)),
             txflags(tfPartialPayment));
 
+        if (!features[fixAMMRounding])
+        {
             BEAST_EXPECT(ammCarol.expectBalances(
                 STAmount{BTC, UINT64_C(1'001'000000374812), -12},
                 USD(100'000),
                 ammCarol.tokens()));
+        }
+        else
+        {
+            BEAST_EXPECT(ammCarol.expectBalances(
+                STAmount{BTC, UINT64_C(1'001'000000374815), -12},
+                USD(100'000),
+                ammCarol.tokens()));
+        }
 
         env.require(balance(bob, USD(200'100)));
         BEAST_EXPECT(isOffer(env, carol, BTC(49), XRP(49)));
@@ -709,12 +720,24 @@ private:
         auto const jrr = env.rpc("json", "submit", to_string(payment));
         BEAST_EXPECT(jrr[jss::result][jss::status] == "success");
         BEAST_EXPECT(jrr[jss::result][jss::engine_result] == "tesSUCCESS");
+        if (!features[fixAMMRounding])
+        {
             BEAST_EXPECT(ammAlice.expectBalances(
                 STAmount(XTS, UINT64_C(101'010101010101), -12),
                 XXX(99),
                 ammAlice.tokens()));
             BEAST_EXPECT(expectLine(
                 env, bob, STAmount{XTS, UINT64_C(98'989898989899), -12}));
+        }
+        else
+        {
+            BEAST_EXPECT(ammAlice.expectBalances(
+                STAmount(XTS, UINT64_C(101'0101010101011), -13),
+                XXX(99),
+                ammAlice.tokens()));
+            BEAST_EXPECT(expectLine(
+                env, bob, STAmount{XTS, UINT64_C(98'9898989898989), -13}));
+        }
         BEAST_EXPECT(expectLine(env, bob, XXX(101)));
     }
 
@@ -1404,6 +1427,7 @@ private:
         using namespace jtx;
         FeatureBitset const all{supported_amendments()};
         testRmFundedOffer(all);
+        testRmFundedOffer(all - fixAMMRounding);
         testEnforceNoRipple(all);
         testFillModes(all);
         testOfferCrossWithXRP(all);
@@ -1417,6 +1441,7 @@ private:
         testOfferCreateThenCross(all);
         testSellFlagExceedLimit(all);
         testGatewayCrossCurrency(all);
+        testGatewayCrossCurrency(all - fixAMMRounding);
         // testPartialCross
         // testXRPDirectCross
         // testDirectCross
@@ -2292,16 +2317,36 @@ private:
                 txflags(tfNoRippleDirect | tfPartialPayment | tfLimitQuality));
             env.close();
 
+            if (!features[fixAMMRounding])
+            {
                 // alice buys 77.2727USD with 75.5555GBP and pays 25% tr fee
                 // on 75.5555GBP
                 // 1,200 - 75.55555*1.25 = 1200 - 94.4444 = 1105.55555GBP
                 BEAST_EXPECT(expectLine(
-                env, alice, STAmount{GBP, UINT64_C(1'105'555555555555), -12}));
+                    env,
+                    alice,
+                    STAmount{GBP, UINT64_C(1'105'555555555555), -12}));
                 // 75.5555GBP is swapped in for 77.7272USD
                 BEAST_EXPECT(amm.expectBalances(
                     STAmount{GBP, UINT64_C(1'075'555555555556), -12},
                     STAmount{USD, UINT64_C(1'022'727272727272), -12},
                     amm.tokens()));
+            }
+            else
+            {
+                // alice buys 77.2727USD with 75.5555GBP and pays 25% tr fee
+                // on 75.5555GBP
+                // 1,200 - 75.55555*1.25 = 1200 - 94.4444 = 1105.55555GBP
+                BEAST_EXPECT(expectLine(
+                    env,
+                    alice,
+                    STAmount{GBP, UINT64_C(1'105'555555555554), -12}));
+                // 75.5555GBP is swapped in for 77.7272USD
+                BEAST_EXPECT(amm.expectBalances(
+                    STAmount{GBP, UINT64_C(1'075'555555555557), -12},
+                    STAmount{USD, UINT64_C(1'022'727272727272), -12},
+                    amm.tokens()));
+            }
             BEAST_EXPECT(expectLine(
                 env, carol, STAmount{USD, UINT64_C(1'277'272727272728), -12}));
         }
@@ -2319,6 +2364,8 @@ private:
             env(offer(alice, EUR(100), USD(100)));
             env.close();
 
+            if (!features[fixAMMRounding])
+            {
                 // 95.2380USD is swapped in for 100EUR
                 BEAST_EXPECT(amm.expectBalances(
                     STAmount{USD, UINT64_C(1'095'238095238095), -12},
@@ -2331,6 +2378,22 @@ private:
                     alice,
                     STAmount{USD, UINT64_C(1'080'952380952381), -12},
                     EUR(1'300)));
+            }
+            else
+            {
+                // 95.2380USD is swapped in for 100EUR
+                BEAST_EXPECT(amm.expectBalances(
+                    STAmount{USD, UINT64_C(1'095'238095238096), -12},
+                    EUR(1'050),
+                    amm.tokens()));
+                // alice pays 25% tr fee on 95.2380USD
+                // 1200-95.2380*1.25 = 1200 - 119.0477 = 1080.9523USD
+                BEAST_EXPECT(expectLine(
+                    env,
+                    alice,
+                    STAmount{USD, UINT64_C(1'080'95238095238), -11},
+                    EUR(1'300)));
+            }
             BEAST_EXPECT(expectOffers(env, alice, 0));
         }
 
@@ -2354,21 +2417,43 @@ private:
             env(pay(gw, dan, USD(1'000)));
             AMM ammDan(env, dan, USD(1'000), EUR(1'050));
 
+            if (!features[fixAMMRounding])
+            {
                 // alice -> bob -> gw -> carol. $50 should have transfer fee;
                 // $10, no fee
                 env(pay(alice, carol, EUR(50)),
                     path(bob, gw, ~EUR),
                     sendmax(USDA(60)),
                     txflags(tfNoRippleDirect));
-
+                BEAST_EXPECT(ammDan.expectBalances(
-            BEAST_EXPECT(
+                    USD(1'050), EUR(1'000), ammDan.tokens()));
-                ammDan.expectBalances(USD(1'050), EUR(1'000), ammDan.tokens()));
                 BEAST_EXPECT(expectLine(env, dan, USD(0)));
                 BEAST_EXPECT(expectLine(env, dan, EUR(0)));
                 BEAST_EXPECT(expectLine(env, bob, USD(-10)));
                 BEAST_EXPECT(expectLine(env, bob, USDA(60)));
                 BEAST_EXPECT(expectLine(env, carol, EUR(50)));
             }
+            else
+            {
+                // alice -> bob -> gw -> carol. $50 should have transfer fee;
+                // $10, no fee
+                env(pay(alice, carol, EUR(50)),
+                    path(bob, gw, ~EUR),
+                    sendmax(USDA(60.1)),
+                    txflags(tfNoRippleDirect));
+                BEAST_EXPECT(ammDan.expectBalances(
+                    STAmount{USD, UINT64_C(1'050'000000000001), -12},
+                    EUR(1'000),
+                    ammDan.tokens()));
+                BEAST_EXPECT(expectLine(env, dan, USD(0)));
+                BEAST_EXPECT(expectLine(env, dan, EUR(0)));
+                BEAST_EXPECT(expectLine(
+                    env, bob, STAmount{USD, INT64_C(-10'000000000001), -12}));
+                BEAST_EXPECT(expectLine(
+                    env, bob, STAmount{USDA, UINT64_C(60'000000000001), -12}));
+                BEAST_EXPECT(expectLine(env, carol, EUR(50)));
+            }
+        }
     }
 
     void
@@ -2401,11 +2486,21 @@ private:
             // alice buys 107.1428USD with 120GBP and pays 25% tr fee on 120GBP
             // 1,000 - 120*1.25 = 850GBP
             BEAST_EXPECT(expectLine(env, alice, GBP(850)));
+            if (!features[fixAMMRounding])
+            {
                 // 120GBP is swapped in for 107.1428USD
                 BEAST_EXPECT(amm.expectBalances(
                     GBP(1'120),
                     STAmount{USD, UINT64_C(892'8571428571428), -13},
                     amm.tokens()));
+            }
+            else
+            {
+                BEAST_EXPECT(amm.expectBalances(
+                    GBP(1'120),
+                    STAmount{USD, UINT64_C(892'8571428571429), -13},
+                    amm.tokens()));
+            }
             // 25% of 85.7142USD is paid in tr fee
             // 85.7142*1.25 = 107.1428USD
             BEAST_EXPECT(expectLine(
@@ -2479,20 +2574,39 @@ private:
                 txflags(tfNoRippleDirect | tfPartialPayment));
             env.close();
 
-            // alice buys 107.1428EUR with 120GBP and pays 25% tr fee on 120GBP
-            // 1,000 - 120*1.25 = 850GBP
             BEAST_EXPECT(expectLine(env, alice, GBP(850)));
-            // 120GBP is swapped in for 107.1428EUR
+            if (!features[fixAMMRounding])
+            {
+                // alice buys 107.1428EUR with 120GBP and pays 25% tr fee on
+                // 120GBP 1,000 - 120*1.25 = 850GBP 120GBP is swapped in for
+                // 107.1428EUR
                 BEAST_EXPECT(amm1.expectBalances(
                     GBP(1'120),
                     STAmount{EUR, UINT64_C(892'8571428571428), -13},
                     amm1.tokens()));
-            // 25% on 85.7142EUR is paid in tr fee 85.7142*1.25 = 107.1428EUR
+                // 25% on 85.7142EUR is paid in tr fee 85.7142*1.25 =
-            // 85.7142EUR is swapped in for 78.9473USD
+                // 107.1428EUR 85.7142EUR is swapped in for 78.9473USD
                 BEAST_EXPECT(amm2.expectBalances(
                     STAmount(EUR, UINT64_C(1'085'714285714286), -12),
                     STAmount{USD, UINT64_C(921'0526315789471), -13},
                     amm2.tokens()));
+            }
+            else
+            {
+                // alice buys 107.1428EUR with 120GBP and pays 25% tr fee on
+                // 120GBP 1,000 - 120*1.25 = 850GBP 120GBP is swapped in for
+                // 107.1428EUR
+                BEAST_EXPECT(amm1.expectBalances(
+                    GBP(1'120),
+                    STAmount{EUR, UINT64_C(892'8571428571429), -13},
+                    amm1.tokens()));
+                // 25% on 85.7142EUR is paid in tr fee 85.7142*1.25 =
+                // 107.1428EUR 85.7142EUR is swapped in for 78.9473USD
+                BEAST_EXPECT(amm2.expectBalances(
+                    STAmount(EUR, UINT64_C(1'085'714285714286), -12),
+                    STAmount{USD, UINT64_C(921'052631578948), -12},
+                    amm2.tokens()));
+            }
             // 25% on 63.1578USD is paid in tr fee 63.1578*1.25 = 78.9473USD
             BEAST_EXPECT(expectLine(
                 env, carol, STAmount(USD, UINT64_C(1'063'157894736842), -12)));
@@ -2578,13 +2692,31 @@ private:
                 txflags(tfNoRippleDirect | tfPartialPayment | tfLimitQuality));
             env.close();
 
+            if (!features[fixAMMRounding])
+            {
                 // alice buys 28.125USD with 24GBP and pays 25% tr fee
                 // on 24GBP
                 // 1,200 - 24*1.25 = 1,170GBP
                 BEAST_EXPECT(expectLine(env, alice, GBP(1'170)));
                 // 24GBP is swapped in for 28.125USD
-            BEAST_EXPECT(
+                BEAST_EXPECT(amm.expectBalances(
-                amm.expectBalances(GBP(1'024), USD(1'171.875), amm.tokens()));
+                    GBP(1'024), USD(1'171.875), amm.tokens()));
+            }
+            else
+            {
+                // alice buys 28.125USD with 24GBP and pays 25% tr fee
+                // on 24GBP
+                // 1,200 - 24*1.25 =~ 1,170GBP
+                BEAST_EXPECT(expectLine(
+                    env,
+                    alice,
+                    STAmount{GBP, UINT64_C(1'169'999999999999), -12}));
+                // 24GBP is swapped in for 28.125USD
+                BEAST_EXPECT(amm.expectBalances(
+                    STAmount{GBP, UINT64_C(1'024'000000000001), -12},
+                    USD(1'171.875),
+                    amm.tokens()));
+            }
             // 25% on 22.5USD is paid in tr fee
             // 22.5*1.25 = 28.125USD
             BEAST_EXPECT(expectLine(env, carol, USD(1'222.5)));
@@ -2617,11 +2749,15 @@ private:
                 txflags(tfNoRippleDirect | tfPartialPayment | tfLimitQuality));
             env.close();
 
+            if (!features[fixAMMRounding])
+            {
                 // alice buys 70.4210EUR with 70.4210GBP via the offer
                 // and pays 25% tr fee on 70.4210GBP
                 // 1,400 - 70.4210*1.25 = 1400 - 88.0262 = 1311.9736GBP
                 BEAST_EXPECT(expectLine(
-                env, alice, STAmount{GBP, UINT64_C(1'311'973684210527), -12}));
+                    env,
+                    alice,
+                    STAmount{GBP, UINT64_C(1'311'973684210527), -12}));
                 // ed doesn't pay tr fee, the balances reflect consumed offer
                 // 70.4210GBP/70.4210EUR
                 BEAST_EXPECT(expectLine(
@@ -2642,6 +2778,37 @@ private:
                     STAmount{EUR, UINT64_C(1'056'336842105263), -12},
                     STAmount{USD, UINT64_C(1'325'334821428571), -12},
                     amm.tokens()));
+            }
+            else
+            {
+                // alice buys 70.4210EUR with 70.4210GBP via the offer
+                // and pays 25% tr fee on 70.4210GBP
+                // 1,400 - 70.4210*1.25 = 1400 - 88.0262 = 1311.9736GBP
+                BEAST_EXPECT(expectLine(
+                    env,
+                    alice,
+                    STAmount{GBP, UINT64_C(1'311'973684210525), -12}));
+                // ed doesn't pay tr fee, the balances reflect consumed offer
+                // 70.4210GBP/70.4210EUR
+                BEAST_EXPECT(expectLine(
+                    env,
+                    ed,
+                    STAmount{EUR, UINT64_C(1'329'57894736842), -11},
+                    STAmount{GBP, UINT64_C(1'470'42105263158), -11}));
+                BEAST_EXPECT(expectOffers(
+                    env,
+                    ed,
+                    1,
+                    {Amounts{
+                        STAmount{GBP, UINT64_C(929'57894736842), -11},
+                        STAmount{EUR, UINT64_C(929'57894736842), -11}}}));
+                // 25% on 56.3368EUR is paid in tr fee 56.3368*1.25 = 70.4210EUR
+                // 56.3368EUR is swapped in for 74.6651USD
+                BEAST_EXPECT(amm.expectBalances(
+                    STAmount{EUR, UINT64_C(1'056'336842105264), -12},
+                    STAmount{USD, UINT64_C(1'325'334821428571), -12},
+                    amm.tokens()));
+            }
             // 25% on 59.7321USD is paid in tr fee 59.7321*1.25 = 74.6651USD
             BEAST_EXPECT(expectLine(
                 env, carol, STAmount(USD, UINT64_C(1'459'732142857143), -12)));
@@ -2674,17 +2841,40 @@ private:
                 txflags(tfNoRippleDirect | tfPartialPayment | tfLimitQuality));
             env.close();
 
+            if (!features[fixAMMRounding])
+            {
                 // alice buys 53.3322EUR with 56.3368GBP via the amm
                 // and pays 25% tr fee on 56.3368GBP
                 // 1,400 - 56.3368*1.25 = 1400 - 70.4210 = 1329.5789GBP
                 BEAST_EXPECT(expectLine(
-                env, alice, STAmount{GBP, UINT64_C(1'329'578947368421), -12}));
+                    env,
-            //// 25% on 56.3368EUR is paid in tr fee 56.3368*1.25 = 70.4210EUR
+                    alice,
+                    STAmount{GBP, UINT64_C(1'329'578947368421), -12}));
+                //// 25% on 56.3368EUR is paid in tr fee 56.3368*1.25
+                ///= 70.4210EUR
                 // 56.3368GBP is swapped in for 53.3322EUR
                 BEAST_EXPECT(amm.expectBalances(
                     STAmount{GBP, UINT64_C(1'056'336842105263), -12},
                     STAmount{EUR, UINT64_C(946'6677295918366), -13},
                     amm.tokens()));
+            }
+            else
+            {
+                // alice buys 53.3322EUR with 56.3368GBP via the amm
+                // and pays 25% tr fee on 56.3368GBP
+                // 1,400 - 56.3368*1.25 = 1400 - 70.4210 = 1329.5789GBP
+                BEAST_EXPECT(expectLine(
+                    env,
+                    alice,
+                    STAmount{GBP, UINT64_C(1'329'57894736842), -11}));
+                //// 25% on 56.3368EUR is paid in tr fee 56.3368*1.25
+                ///= 70.4210EUR
+                // 56.3368GBP is swapped in for 53.3322EUR
+                BEAST_EXPECT(amm.expectBalances(
+                    STAmount{GBP, UINT64_C(1'056'336842105264), -12},
+                    STAmount{EUR, UINT64_C(946'6677295918366), -13},
+                    amm.tokens()));
+            }
             // 25% on 42.6658EUR is paid in tr fee 42.6658*1.25 = 53.3322EUR
             // 42.6658EUR/59.7321USD
             BEAST_EXPECT(expectLine(
@@ -2729,11 +2919,15 @@ private:
                 txflags(tfNoRippleDirect | tfPartialPayment | tfLimitQuality));
             env.close();
 
+            if (!features[fixAMMRounding])
+            {
                 // alice buys 53.3322EUR with 107.5308GBP
                 // 25% on 86.0246GBP is paid in tr fee
                 // 1,400 - 86.0246*1.25 = 1400 - 107.5308 = 1229.4691GBP
                 BEAST_EXPECT(expectLine(
-                env, alice, STAmount{GBP, UINT64_C(1'292'469135802469), -12}));
+                    env,
+                    alice,
+                    STAmount{GBP, UINT64_C(1'292'469135802469), -12}));
                 // 86.0246GBP is swapped in for 79.2106EUR
                 BEAST_EXPECT(amm1.expectBalances(
                     STAmount{GBP, UINT64_C(1'086'024691358025), -12},
@@ -2745,6 +2939,28 @@ private:
                     STAmount{EUR, UINT64_C(1'063'368497635504), -12},
                     STAmount{USD, UINT64_C(1'316'570881226053), -12},
                     amm2.tokens()));
+            }
+            else
+            {
+                // alice buys 53.3322EUR with 107.5308GBP
+                // 25% on 86.0246GBP is paid in tr fee
+                // 1,400 - 86.0246*1.25 = 1400 - 107.5308 = 1229.4691GBP
+                BEAST_EXPECT(expectLine(
+                    env,
+                    alice,
+                    STAmount{GBP, UINT64_C(1'292'469135802466), -12}));
+                // 86.0246GBP is swapped in for 79.2106EUR
+                BEAST_EXPECT(amm1.expectBalances(
+                    STAmount{GBP, UINT64_C(1'086'024691358027), -12},
+                    STAmount{EUR, UINT64_C(920'7893779556188), -13},
+                    amm1.tokens()));
+                // 25% on 63.3684EUR is paid in tr fee 63.3684*1.25 = 79.2106EUR
+                // 63.3684EUR is swapped in for 83.4291USD
+                BEAST_EXPECT(amm2.expectBalances(
+                    STAmount{EUR, UINT64_C(1'063'368497635505), -12},
+                    STAmount{USD, UINT64_C(1'316'570881226053), -12},
+                    amm2.tokens()));
+            }
             // 25% on 66.7432USD is paid in tr fee 66.7432*1.25 = 83.4291USD
             BEAST_EXPECT(expectLine(
                 env, carol, STAmount(USD, UINT64_C(1'466'743295019157), -12)));
@@ -2774,6 +2990,8 @@ private:
                 txflags(tfNoRippleDirect | tfPartialPayment | tfLimitQuality));
             env.close();
 
+            if (!features[fixAMMRounding])
+            {
                 // 108.1481GBP is swapped in for 97.5935EUR
                 BEAST_EXPECT(amm1.expectBalances(
                     STAmount{GBP, UINT64_C(1'108'148148148149), -12},
@@ -2785,6 +3003,21 @@ private:
                     STAmount{EUR, UINT64_C(1'078'074866310161), -12},
                     STAmount{USD, UINT64_C(1'298'611111111111), -12},
                     amm2.tokens()));
+            }
+            else
+            {
+                // 108.1481GBP is swapped in for 97.5935EUR
+                BEAST_EXPECT(amm1.expectBalances(
+                    STAmount{GBP, UINT64_C(1'108'148148148151), -12},
+                    STAmount{EUR, UINT64_C(902'4064171122975), -13},
+                    amm1.tokens()));
+                // 25% on 78.0748EUR is paid in tr fee 78.0748*1.25 = 97.5935EUR
+                // 78.0748EUR is swapped in for 101.3888USD
+                BEAST_EXPECT(amm2.expectBalances(
+                    STAmount{EUR, UINT64_C(1'078'074866310162), -12},
+                    STAmount{USD, UINT64_C(1'298'611111111111), -12},
+                    amm2.tokens()));
+            }
             // 25% on 81.1111USD is paid in tr fee 81.1111*1.25 = 101.3888USD
             BEAST_EXPECT(expectLine(
                 env, carol, STAmount{USD, UINT64_C(1'481'111111111111), -12}));
@@ -3037,6 +3270,8 @@ private:
             env(offer(bob, XRP(100), USD(100)));
             env(offer(bob, XRP(1'000), USD(100)));
             AMM ammDan(env, dan, XRP(1'000), USD(1'100));
+            if (!features[fixAMMRounding])
+            {
                 env(pay(alice, carol, USD(10'000)),
                     paths(XRP),
                     delivermin(USD(200)),
@@ -3044,8 +3279,24 @@ private:
                     sendmax(XRP(200)));
                 env.require(balance(bob, USD(0)));
                 env.require(balance(carol, USD(200)));
-            BEAST_EXPECT(
+                BEAST_EXPECT(ammDan.expectBalances(
-                ammDan.expectBalances(XRP(1'100), USD(1'000), ammDan.tokens()));
+                    XRP(1'100), USD(1'000), ammDan.tokens()));
+            }
+            else
+            {
+                env(pay(alice, carol, USD(10'000)),
+                    paths(XRP),
+                    delivermin(USD(200)),
+                    txflags(tfPartialPayment),
+                    sendmax(XRPAmount(200'000'001)));
+                env.require(balance(bob, USD(0)));
+                env.require(balance(
+                    carol, STAmount{USD, UINT64_C(200'00000090909), -11}));
+                BEAST_EXPECT(ammDan.expectBalances(
+                    XRPAmount{1'100'000'001},
+                    STAmount{USD, UINT64_C(999'99999909091), -11},
+                    ammDan.tokens()));
+            }
         }
     }
 
@@ -3829,7 +4080,9 @@ private:
         testBookStep(all);
         testBookStep(all | ownerPaysFee);
         testTransferRate(all | ownerPaysFee);
+        testTransferRate((all - fixAMMRounding) | ownerPaysFee);
         testTransferRateNoOwnerFee(all);
+        testTransferRateNoOwnerFee(all - fixAMMRounding);
         testLimitQuality();
         testXRPPathLoop();
     }
@@ -3848,6 +4101,7 @@ private:
         using namespace jtx;
         FeatureBitset const all{supported_amendments()};
         test_convert_all_of_an_asset(all);
+        test_convert_all_of_an_asset(all - fixAMMRounding);
     }
 
     void

src/test/jtx/impl/AMM.cpp

@@ -234,7 +234,6 @@ AMM::expectBalances(
         balances(asset1.issue(), asset2.issue(), account);
     return asset1 == asset1Balance && asset2 == asset2Balance &&
         lptAMMBalance == STAmount{lpt, lptIssue_};
-    return false;
 }
 
 IOUAmount