Merge remote-tracking branch 'upstream/develop' into invariant-acctde…

…lete

* upstream/develop:
  Set version to 2.2.0-rc1
  fix amendment: AMM swap should honor invariants: (5002)
  Add global access to the current ledger rules:
  chore: fix typos (4958)
  test: Add RPC error checking support to unit tests (4987)

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/app/misc/TxQ.h

@@ -322,7 +322,7 @@ class TxQ
 
     /**
      * @brief Returns minimum required fee for tx and two sequences:
-     *        first vaild sequence for this account in current ledger
+     *        first valid sequence for this account in current ledger
      *        and first available sequence for transaction
      * @param view current open ledger
      * @param tx the transaction

src/ripple/app/paths/impl/Steps.h

@@ -417,7 +417,7 @@ toStrand(
                        quality of the tip of the book drops below this value,
                        then evaluating the strand can stop.
    @param sendMax Optional asset to send.
-   @param paths Paths to use to fullfill the payment. Each path in the pathset
+   @param paths Paths to use to fulfill the payment. Each path in the pathset
                 contains an ordered collection of the offer books to use and
                 accounts to ripple through.
    @param addDefaultPath Determines if the default path should be included

src/ripple/app/reporting/ETLSource.cpp

@@ -27,7 +27,7 @@ namespace ripple {
 
 // Create ETL source without grpc endpoint
 // Fetch ledger and load initial ledger will fail for this source
-// Primarly used in read-only mode, to monitor when ledgers are validated
+// Primarily used in read-only mode, to monitor when ledgers are validated
 ETLSource::ETLSource(std::string ip, std::string wsPort, ReportingETL& etl)
     : ip_(ip)
     , wsPort_(wsPort)

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

@@ -259,7 +259,7 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx)
             // We also _must not_ check the tx submitter in brokered
             // mode, because then we are confirming that the broker can
             // cover what the buyer will pay, which doesn't make sense, causes
-            // an unncessary tec, and is also resolved with this amendment.
+            // an unnecessary tec, and is also resolved with this amendment.
             if (accountFunds(
                     ctx.view,
                     ctx.tx[sfAccount],

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

@@ -831,8 +831,11 @@ Transactor::operator()()
 {
     JLOG(j_.trace()) << "apply: " << ctx_.tx.getTransactionID();
 
+    // raii classes for the current ledger rules. fixSTAmountCanonicalize and
+    // fixSTAmountCanonicalize predate the rulesGuard and should be replaced.
     STAmountSO stAmountSO{view().rules().enabled(fixSTAmountCanonicalize)};
     NumberSO stNumberSO{view().rules().enabled(fixUniversalNumber)};
+    CurrentTransactionRulesGuard currentTransctionRulesGuard(view().rules());
 
 #ifdef DEBUG
     {

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 = 73;
+static constexpr std::size_t numFeatures = 74;
 
 /** 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;
 extern uint256 const featureInvariantsV1_1;
 
 }  // namespace ripple