Fill in payment computation shortages (#5941)

- Ensures a consistent fixed payment amount for the entire life of the
  loan, except the final payment, which is guaranteed to be the same or
  smaller.
- Convert some Loan structs to compute values that had need manual
  updates to stay consistent.
- Fail the transaction in `LoanPay` if it violates the Vault `assetsAvailable <=
  assetsTotal` invariant.
- Use constexpr to check that min mantissa value for Number and STAmount
  is a power of 10, and compute the max in terms of the min.
- Improve unit tests:
  - Use BrokerParameters and Loan Parameters instead of semi-global
    class values
  - In tests, check that the expected number of loan payments are made.
  - Add LoanBatch manual test to generate a set number of random loans,
    set them up, and pay them off.
  - Add LoanArbitrary manual test to run a single test with specific
    (hard-coded for now) parameters.
  - Add Number support to XRP_t.

include/xrpl/basics/Number.h

@@ -32,6 +32,15 @@ class Number;
 std::string
 to_string(Number const& amount);
 
+template <typename T>
+constexpr bool
+isPowerOfTen(T value)
+{
+    while (value >= 10 && value % 10 == 0)
+        value /= 10;
+    return value == 1;
+}
+
 class Number
 {
     using rep = std::int64_t;
@@ -41,7 +50,9 @@ class Number
 public:
     // The range for the mantissa when normalized
     constexpr static std::int64_t minMantissa = 1'000'000'000'000'000LL;
-    constexpr static std::int64_t maxMantissa = 9'999'999'999'999'999LL;
+    static_assert(isPowerOfTen(minMantissa));
+    constexpr static std::int64_t maxMantissa = minMantissa * 10 - 1;
+    static_assert(maxMantissa == 9'999'999'999'999'999LL);
 
     // The range for the exponent when normalized
     constexpr static int minExponent = -32768;
@@ -58,8 +69,6 @@ public:
     explicit Number(rep mantissa, int exponent);
     explicit constexpr Number(rep mantissa, int exponent, unchecked) noexcept;
 
-    static Number const zero;
-
     constexpr rep
     mantissa() const noexcept;
     constexpr int
@@ -153,22 +162,7 @@ public:
     }
 
     Number
-    truncate() const noexcept
+    truncate() const noexcept;
-    {
-        if (exponent_ >= 0 || mantissa_ == 0)
-            return *this;
-
-        Number ret = *this;
-        while (ret.exponent_ < 0 && ret.mantissa_ != 0)
-        {
-            ret.exponent_ += 1;
-            ret.mantissa_ /= rep(10);
-        }
-        // We are guaranteed that normalize() will never throw an exception
-        // because exponent is either negative or zero at this point.
-        ret.normalize();
-        return ret;
-    }
 
     friend constexpr bool
     operator>(Number const& x, Number const& y) noexcept
@@ -213,6 +207,8 @@ private:
     class Guard;
 };
 
+constexpr static Number numZero{};
+
 inline constexpr Number::Number(rep mantissa, int exponent, unchecked) noexcept
     : mantissa_{mantissa}, exponent_{exponent}
 {

include/xrpl/protocol/Protocol.h

@@ -102,7 +102,9 @@ std::uint16_t constexpr maxTransferFee = 50000;
  * Example: 50% is 0.50 * bipsPerUnity = 5,000 bps.
  */
 Bips32 constexpr bipsPerUnity(100 * 100);
+static_assert(bipsPerUnity == Bips32{10'000});
 TenthBips32 constexpr tenthBipsPerUnity(bipsPerUnity.value() * 10);
+static_assert(tenthBipsPerUnity == TenthBips32(100'000));
 
 constexpr Bips32
 percentageToBips(std::uint32_t percentage)

include/xrpl/protocol/STAmount.h

@@ -66,16 +66,18 @@ public:
     static int const cMaxOffset = 80;
 
     // Maximum native value supported by the code
-    static std::uint64_t const cMinValue = 1'000'000'000'000'000ull;
+    constexpr static std::uint64_t cMinValue = 1'000'000'000'000'000ull;
-    static std::uint64_t const cMaxValue = 9'999'999'999'999'999ull;
+    static_assert(isPowerOfTen(cMinValue));
-    static std::uint64_t const cMaxNative = 9'000'000'000'000'000'000ull;
+    constexpr static std::uint64_t cMaxValue = cMinValue * 10 - 1;
+    static_assert(cMaxValue == 9'999'999'999'999'999ull);
+    constexpr static std::uint64_t cMaxNative = 9'000'000'000'000'000'000ull;
 
     // Max native value on network.
-    static std::uint64_t const cMaxNativeN = 100'000'000'000'000'000ull;
+    constexpr static std::uint64_t cMaxNativeN = 100'000'000'000'000'000ull;
-    static std::uint64_t const cIssuedCurrency = 0x8'000'000'000'000'000ull;
+    constexpr static std::uint64_t cIssuedCurrency = 0x8'000'000'000'000'000ull;
-    static std::uint64_t const cPositive = 0x4'000'000'000'000'000ull;
+    constexpr static std::uint64_t cPositive = 0x4'000'000'000'000'000ull;
-    static std::uint64_t const cMPToken = 0x2'000'000'000'000'000ull;
+    constexpr static std::uint64_t cMPToken = 0x2'000'000'000'000'000ull;
-    static std::uint64_t const cValueMask = ~(cPositive | cMPToken);
+    constexpr static std::uint64_t cValueMask = ~(cPositive | cMPToken);
 
     static std::uint64_t const uRateOne;
 

src/libxrpl/basics/Number.cpp

@@ -43,8 +43,6 @@ namespace ripple {
 
 thread_local Number::rounding_mode Number::mode_ = Number::to_nearest;
 
-Number const Number::zero{};
-
 Number::rounding_mode
 Number::getround()
 {
@@ -523,6 +521,24 @@ Number::operator rep() const
     return drops;
 }
 
+Number
+Number::truncate() const noexcept
+{
+    if (exponent_ >= 0 || mantissa_ == 0)
+        return *this;
+
+    Number ret = *this;
+    while (ret.exponent_ < 0 && ret.mantissa_ != 0)
+    {
+        ret.exponent_ += 1;
+        ret.mantissa_ /= rep(10);
+    }
+    // We are guaranteed that normalize() will never throw an exception
+    // because exponent is either negative or zero at this point.
+    ret.normalize();
+    return ret;
+}
+
 std::string
 to_string(Number const& amount)
 {

src/test/jtx/Env.h

@@ -251,7 +251,9 @@ public:
      *
      * @param suite_ the current unit_test::suite
      */
-    Env(beast::unit_test::suite& suite_) : Env(suite_, envconfig())
+    Env(beast::unit_test::suite& suite_,
+        beast::severities::Severity thresh = beast::severities::kError)
+        : Env(suite_, envconfig(), nullptr, thresh)
     {
     }
 

src/test/jtx/amount.h

@@ -295,6 +295,21 @@ struct XRP_t
         return {TOut{v} * dropsPerXRP};
     }
 
+    /** Returns an amount of XRP as PrettyAmount,
+        which is trivially convertable to STAmount
+
+        @param v The Number of XRP (not drops). May be fractional.
+    */
+    PrettyAmount
+    operator()(Number v) const
+    {
+        auto const c = dropsPerXRP.drops();
+        auto const d = std::int64_t(v * c);
+        if (Number(d) / c != v)
+            Throw<std::domain_error>("unrepresentable");
+        return {d};
+    }
+
     PrettyAmount
     operator()(double v) const
     {

src/xrpld/app/misc/LendingHelpers.h

@@ -98,14 +98,24 @@ struct LoanState
     Number valueOutstanding;
     /// Prinicipal still due to be paid by the borrower.
     Number principalOutstanding;
-    /// Interest still due to be paid by the borrower.
-    Number interestOutstanding;
     /// Interest still due to be paid TO the Vault.
     // This is a portion of interestOutstanding
     Number interestDue;
     /// Management fee still due to be paid TO the broker.
     // This is a portion of interestOutstanding
     Number managementFeeDue;
+
+    /// Interest still due to be paid by the borrower.
+    Number
+    interestOutstanding() const
+    {
+        XRPL_ASSERT_PARTS(
+            interestDue + managementFeeDue ==
+                valueOutstanding - principalOutstanding,
+            "ripple::LoanState::interestOutstanding",
+            "other values add up correctly");
+        return interestDue + managementFeeDue;
+    }
 };
 
 LoanState
@@ -113,7 +123,7 @@ calculateRawLoanState(
     Number const& periodicPayment,
     Number const& periodicRate,
     std::uint32_t const paymentRemaining,
-    TenthBips16 const managementFeeRate);
+    TenthBips32 const managementFeeRate);
 
 LoanState
 calculateRawLoanState(
@@ -121,7 +131,7 @@ calculateRawLoanState(
     TenthBips32 interestRate,
     std::uint32_t paymentInterval,
     std::uint32_t const paymentRemaining,
-    TenthBips16 const managementFeeRate);
+    TenthBips32 const managementFeeRate);
 
 LoanState
 calculateRoundedLoanState(
@@ -136,7 +146,7 @@ Number
 computeFee(
     Asset const& asset,
     Number const& value,
-    TenthBips16 managementFeeRate,
+    TenthBips32 managementFeeRate,
     std::int32_t scale);
 
 Number
@@ -195,10 +205,18 @@ struct PaymentComponents
 
 struct LoanDeltas
 {
-    Number valueDelta;
     Number principalDelta;
     Number interestDueDelta;
     Number managementFeeDueDelta;
+
+    Number
+    valueDelta() const
+    {
+        return principalDelta + interestDueDelta + managementFeeDueDelta;
+    }
+
+    void
+    nonNegative();
 };
 
 PaymentComponents
@@ -218,6 +236,9 @@ computePaymentComponents(
 detail::LoanDeltas
 operator-(LoanState const& lhs, LoanState const& rhs);
 
+LoanState
+operator-(LoanState const& lhs, detail::LoanDeltas const& rhs);
+
 Number
 valueMinusFee(
     Asset const& asset,
@@ -232,7 +253,7 @@ computeLoanProperties(
     TenthBips32 interestRate,
     std::uint32_t paymentInterval,
     std::uint32_t paymentsRemaining,
-    TenthBips16 managementFeeRate);
+    TenthBips32 managementFeeRate);
 
 bool
 isRounded(Asset const& asset, Number const& value, std::int32_t scale);

src/xrpld/app/misc/detail/LendingHelpers.cpp

@@ -467,7 +467,7 @@ struct PaymentComponentsPlus : public PaymentComponents
     PaymentComponentsPlus(
         PaymentComponents const& p,
         Number f,
-        Number v = Number{})
+        Number v = numZero)
         : PaymentComponents(p)
         , untrackedManagementFee(f)
         , untrackedInterest(v)
@@ -673,7 +673,7 @@ tryOverpayment(
     auto const newRounded = calculateRoundedLoanState(
         totalValueOutstanding, principalOutstanding, managementFeeOutstanding);
     auto const valueChange =
-        newRounded.interestOutstanding - rounded.interestOutstanding;
+        newRounded.interestOutstanding() - rounded.interestOutstanding();
     XRPL_ASSERT_PARTS(
         valueChange < beast::zero,
         "ripple::detail::tryOverpayment",
@@ -999,6 +999,17 @@ PaymentComponents::trackedInterestPart() const
         (trackedPrincipalDelta + trackedManagementFeeDelta);
 }
 
+void
+LoanDeltas::nonNegative()
+{
+    if (principalDelta < beast::zero)
+        principalDelta = numZero;
+    if (interestDueDelta < beast::zero)
+        interestDueDelta = numZero;
+    if (managementFeeDueDelta < beast::zero)
+        managementFeeDueDelta = numZero;
+}
+
 PaymentComponents
 computePaymentComponents(
     Asset const& asset,
@@ -1035,8 +1046,6 @@ computePaymentComponents(
             roundToAsset(asset, trueTarget.valueOutstanding, scale),
         .principalOutstanding =
             roundToAsset(asset, trueTarget.principalOutstanding, scale),
-        .interestOutstanding =
-            roundToAsset(asset, trueTarget.interestOutstanding, scale),
         .interestDue = roundToAsset(asset, trueTarget.interestDue, scale),
         .managementFeeDue =
             roundToAsset(asset, trueTarget.managementFeeDue, scale)};
@@ -1044,67 +1053,38 @@ computePaymentComponents(
         totalValueOutstanding, principalOutstanding, managementFeeOutstanding);
 
     LoanDeltas deltas = currentLedgerState - roundedTarget;
-
+    deltas.nonNegative();
-    // It should be impossible for any of the deltas to be negative, but do
-    // defensive checks
-    if (deltas.principalDelta < beast::zero)
-    {
-        // LCOV_EXCL_START
-        UNREACHABLE(
-            "ripple::detail::computePaymentComponents : negative principal "
-            "delta");
-        deltas.principalDelta = Number::zero;
-        // LCOV_EXCL_STOP
-    }
-    if (deltas.interestDueDelta < beast::zero)
-    {
-        // LCOV_EXCL_START
-        UNREACHABLE(
-            "ripple::detail::computePaymentComponents : negative interest "
-            "delta");
-        deltas.interestDueDelta = Number::zero;
-        // LCOV_EXCL_STOP
-    }
-    if (deltas.managementFeeDueDelta < beast::zero)
-    {
-        // LCOV_EXCL_START
-        UNREACHABLE(
-            "ripple::detail::computePaymentComponents : negative management "
-            "fee delta");
-        deltas.managementFeeDueDelta = Number::zero;
-        // LCOV_EXCL_STOP
-    }
 
     // Adjust the deltas if necessary for data integrity
     XRPL_ASSERT_PARTS(
         deltas.principalDelta <= currentLedgerState.principalOutstanding,
         "ripple::detail::computePaymentComponents",
         "principal delta not greater than outstanding");
+
     deltas.principalDelta = std::min(
         deltas.principalDelta, currentLedgerState.principalOutstanding);
+
     XRPL_ASSERT_PARTS(
         deltas.interestDueDelta <= currentLedgerState.interestDue,
         "ripple::detail::computePaymentComponents",
         "interest due delta not greater than outstanding");
+
     deltas.interestDueDelta = std::min(
         {deltas.interestDueDelta,
-         std::max(Number::zero, roundedPeriodicPayment - deltas.principalDelta),
+         std::max(numZero, roundedPeriodicPayment - deltas.principalDelta),
          currentLedgerState.interestDue});
+
     XRPL_ASSERT_PARTS(
         deltas.managementFeeDueDelta <= currentLedgerState.managementFeeDue,
         "ripple::detail::computePaymentComponents",
         "management fee due delta not greater than outstanding");
+
     deltas.managementFeeDueDelta = std::min(
         {deltas.managementFeeDueDelta,
          roundedPeriodicPayment -
              (deltas.principalDelta + deltas.interestDueDelta),
          currentLedgerState.managementFeeDue});
 
-    // In case any adjustments were made (or if the original rounding didn't
-    // quite add up right), recompute the total value delta
-    deltas.valueDelta = deltas.principalDelta + deltas.interestDueDelta +
-        deltas.managementFeeDueDelta;
-
     if (paymentRemaining == 1 ||
         totalValueOutstanding <= roundedPeriodicPayment)
     {
@@ -1112,15 +1092,15 @@ computePaymentComponents(
         // parts.
 
         XRPL_ASSERT_PARTS(
-            deltas.valueDelta == totalValueOutstanding,
+            deltas.valueDelta() <= totalValueOutstanding,
             "ripple::detail::computePaymentComponents",
             "last payment total value agrees");
         XRPL_ASSERT_PARTS(
-            deltas.principalDelta == principalOutstanding,
+            deltas.principalDelta <= principalOutstanding,
             "ripple::detail::computePaymentComponents",
             "last payment principal agrees");
         XRPL_ASSERT_PARTS(
-            deltas.managementFeeDueDelta == managementFeeOutstanding,
+            deltas.managementFeeDueDelta <= managementFeeOutstanding,
             "ripple::detail::computePaymentComponents",
             "last payment management fee agrees");
 
@@ -1231,13 +1211,12 @@ computePaymentComponents(
     // trying to take more than the whole payment. The excess can be positive,
     // which indicates that we're not going to take the whole payment amount,
     // but if so, it must be small.
-    auto takeFrom = [](Number& total, Number& component, Number& excess) {
+    auto takeFrom = [](Number& component, Number& excess) {
         if (excess > beast::zero)
         {
             // Take as much of the excess as we can out of the provided part and
             // the total
             auto part = std::min(component, excess);
-            total -= part;
             component -= part;
             excess -= part;
         }
@@ -1248,14 +1227,41 @@ computePaymentComponents(
             "ripple::detail::computePaymentComponents",
             "excess non-negative");
     };
+    auto giveTo =
+        [](Number& component, Number& shortage, Number const& maximum) {
+            if (shortage > beast::zero)
+            {
+                // Put as much of the shortage as we can into the provided part
+                // and the total
+                auto part = std::min(maximum - component, shortage);
+                component += part;
+                shortage -= part;
+            }
+            // If the shortage goes negative, we put too much, which should be
+            // impossible
+            XRPL_ASSERT_PARTS(
+                shortage >= beast::zero,
+                "ripple::detail::computePaymentComponents",
+                "excess non-negative");
+        };
     auto addressExcess = [&takeFrom](LoanDeltas& deltas, Number& excess) {
-        takeFrom(deltas.valueDelta, deltas.interestDueDelta, excess);
+        // This order is based on where errors are the least problematic
-        takeFrom(deltas.valueDelta, deltas.managementFeeDueDelta, excess);
+        takeFrom(deltas.interestDueDelta, excess);
-        takeFrom(deltas.valueDelta, deltas.principalDelta, excess);
+        takeFrom(deltas.managementFeeDueDelta, excess);
+        takeFrom(deltas.principalDelta, excess);
+    };
+    auto addressShortage = [&giveTo](
+                               LoanDeltas& deltas,
+                               Number& shortage,
+                               LoanState const& current) {
+        giveTo(deltas.interestDueDelta, shortage, current.interestDue);
+        giveTo(deltas.principalDelta, shortage, current.principalOutstanding);
+        giveTo(
+            deltas.managementFeeDueDelta, shortage, current.managementFeeDue);
     };
     Number totalOverpayment =
-        deltas.valueDelta - currentLedgerState.valueOutstanding;
+        deltas.valueDelta() - currentLedgerState.valueOutstanding;
-    if (totalOverpayment > 0)
+    if (totalOverpayment > beast::zero)
     {
         // LCOV_EXCL_START
         UNREACHABLE(
@@ -1264,8 +1270,9 @@ computePaymentComponents(
         addressExcess(deltas, totalOverpayment);
         // LCOV_EXCL_STOP
     }
+
     // Make sure the parts don't add up to too much
-    Number shortage = roundedPeriodicPayment - deltas.valueDelta;
+    Number shortage = roundedPeriodicPayment - deltas.valueDelta();
 
     XRPL_ASSERT_PARTS(
         isRounded(asset, shortage, scale),
@@ -1280,38 +1287,63 @@ computePaymentComponents(
 
         shortage = -excess;
     }
+    else if (shortage > beast::zero && totalOverpayment < beast::zero)
+    {
+        // If there's a shortage, and there's room in the loan itself, we can
+        // top up the parts to make the payment correct.
+        shortage = std::min(-totalOverpayment, shortage);
+        addressShortage(deltas, shortage, currentLedgerState);
+    }
 
     // The shortage should never be negative, which indicates that the parts are
-    // trying to take more than the whole payment. The shortage can be positive,
+    // trying to take more than the whole payment. The shortage should not be
-    // which indicates that we're not going to take the whole payment amount,
+    // positive, either, which indicates that we're not going to take the whole
-    // but if so, it must be small.
+    // payment amount. Only the last payment should be allowed to have a
+    // shortage, and that's handled in a special case above.
     XRPL_ASSERT_PARTS(
-        shortage == beast::zero ||
+        shortage == beast::zero,
+        "ripple::detail::computePaymentComponents",
+        "no shortage or excess");
+#if LOANCOMPLETE
+    /*
+    // This used to be part of the above assert. It will eventually be removed
+    // if proved accurate
+    ||
         (shortage > beast::zero &&
          ((asset.integral() && shortage < 3) ||
-              (scale - shortage.exponent() > 14))),
+          (scale - shortage.exponent() > 14)))
-        "ripple::detail::computePaymentComponents",
+          */
-        "excess is extremely small");
+#endif
 
     XRPL_ASSERT_PARTS(
-        deltas.valueDelta ==
+        deltas.valueDelta() ==
             deltas.principalDelta + deltas.interestDueDelta +
                 deltas.managementFeeDueDelta,
         "ripple::detail::computePaymentComponents",
         "total value adds up");
 
     XRPL_ASSERT_PARTS(
-        deltas.principalDelta >= beast::zero,
+        deltas.principalDelta >= beast::zero &&
+            deltas.principalDelta <= currentLedgerState.principalOutstanding,
         "ripple::detail::computePaymentComponents",
-        "non-negative principal");
+        "valid principal result");
     XRPL_ASSERT_PARTS(
-        deltas.interestDueDelta >= beast::zero,
+        deltas.interestDueDelta >= beast::zero &&
+            deltas.interestDueDelta <= currentLedgerState.interestDue,
         "ripple::detail::computePaymentComponents",
-        "non-negative interest");
+        "valid interest result");
     XRPL_ASSERT_PARTS(
-        deltas.managementFeeDueDelta >= beast::zero,
+        deltas.managementFeeDueDelta >= beast::zero &&
+            deltas.managementFeeDueDelta <= currentLedgerState.managementFeeDue,
         "ripple::detail::computePaymentComponents",
-        "non-negative fee");
+        "valid fee result");
+
+    XRPL_ASSERT_PARTS(
+        deltas.principalDelta + deltas.interestDueDelta +
+                deltas.managementFeeDueDelta >
+            beast::zero,
+        "ripple::detail::computePaymentComponents",
+        "payment parts add to payment");
 
     return PaymentComponents{
 #if LOANCOMPLETE
@@ -1319,11 +1351,18 @@ computePaymentComponents(
         .rawPrincipal = rawPrincipal,
         .rawManagementFee = rawFee,
 #endif
-        // As a final safety check, don't return any negative values
+        // As a final safety check, ensure the value is non-negative, and won't
-        .trackedValueDelta = std::max(deltas.valueDelta, Number::zero),
+        // make the corresponding item negative
-        .trackedPrincipalDelta = std::max(deltas.principalDelta, Number::zero),
+        .trackedValueDelta = std::clamp(
-        .trackedManagementFeeDelta =
+            deltas.valueDelta(), numZero, currentLedgerState.valueOutstanding),
-            std::max(deltas.managementFeeDueDelta, Number::zero),
+        .trackedPrincipalDelta = std::clamp(
+            deltas.principalDelta,
+            numZero,
+            currentLedgerState.principalOutstanding),
+        .trackedManagementFeeDelta = std::clamp(
+            deltas.managementFeeDueDelta,
+            numZero,
+            currentLedgerState.managementFeeDue),
     };
 }
 
@@ -1381,28 +1420,23 @@ detail::LoanDeltas
 operator-(LoanState const& lhs, LoanState const& rhs)
 {
     detail::LoanDeltas result{
-        .valueDelta = lhs.valueOutstanding - rhs.valueOutstanding,
         .principalDelta = lhs.principalOutstanding - rhs.principalOutstanding,
         .interestDueDelta = lhs.interestDue - rhs.interestDue,
         .managementFeeDueDelta = lhs.managementFeeDue - rhs.managementFeeDue,
     };
 
-    XRPL_ASSERT_PARTS(
+    return result;
-        result.valueDelta >= 0,
+}
-        "ripple::operator-(LoanState,LoanState)",
+
-        "valueDelta difference non-negative");
+LoanState
-    XRPL_ASSERT_PARTS(
+operator-(LoanState const& lhs, detail::LoanDeltas const& rhs)
-        result.principalDelta >= 0,
+{
-        "ripple::operator-(LoanState,LoanState)",
+    LoanState result{
-        "principalDelta difference non-negative");
+        .valueOutstanding = lhs.valueOutstanding - rhs.valueDelta(),
-    XRPL_ASSERT_PARTS(
+        .principalOutstanding = lhs.principalOutstanding - rhs.principalDelta,
-        result.interestDueDelta >= 0,
+        .interestDue = lhs.interestDue - rhs.interestDueDelta,
-        "ripple::operator-(LoanState,LoanState)",
+        .managementFeeDue = lhs.managementFeeDue - rhs.managementFeeDueDelta,
-        "interestDueDelta difference non-negative");
+    };
-    XRPL_ASSERT_PARTS(
-        result.managementFeeDueDelta >= 0,
-        "ripple::operator-(LoanState,LoanState)",
-        "managementFeeDueDelta difference non-negative");
 
     return result;
 }
@@ -1470,14 +1504,13 @@ calculateRawLoanState(
     Number const& periodicPayment,
     Number const& periodicRate,
     std::uint32_t const paymentRemaining,
-    TenthBips16 const managementFeeRate)
+    TenthBips32 const managementFeeRate)
 {
     if (paymentRemaining == 0)
     {
         return LoanState{
             .valueOutstanding = 0,
             .principalOutstanding = 0,
-            .interestOutstanding = 0,
             .interestDue = 0,
             .managementFeeDue = 0};
     }
@@ -1493,7 +1526,6 @@ calculateRawLoanState(
     return LoanState{
         .valueOutstanding = rawValueOutstanding,
         .principalOutstanding = rawPrincipalOutstanding,
-        .interestOutstanding = rawInterestOutstanding,
         .interestDue = rawInterestOutstanding - rawManagementFeeOutstanding,
         .managementFeeDue = rawManagementFeeOutstanding};
 };
@@ -1504,7 +1536,7 @@ calculateRawLoanState(
     TenthBips32 interestRate,
     std::uint32_t paymentInterval,
     std::uint32_t const paymentRemaining,
-    TenthBips16 const managementFeeRate)
+    TenthBips32 const managementFeeRate)
 {
     return calculateRawLoanState(
         periodicPayment,
@@ -1521,13 +1553,11 @@ calculateRoundedLoanState(
 {
     // This implementation is pretty trivial, but ensures the calculations are
     // consistent everywhere, and reduces copy/paste errors.
-    Number const interestOutstanding =
-        totalValueOutstanding - principalOutstanding;
     return {
         .valueOutstanding = totalValueOutstanding,
         .principalOutstanding = principalOutstanding,
-        .interestOutstanding = interestOutstanding,
+        .interestDue = totalValueOutstanding - principalOutstanding -
-        .interestDue = interestOutstanding - managementFeeOutstanding,
+            managementFeeOutstanding,
         .managementFeeDue = managementFeeOutstanding};
 }
 
@@ -1544,7 +1574,7 @@ Number
 computeFee(
     Asset const& asset,
     Number const& value,
-    TenthBips16 managementFeeRate,
+    TenthBips32 managementFeeRate,
     std::int32_t scale)
 {
     return roundToAsset(
@@ -1571,7 +1601,7 @@ computeLoanProperties(
     TenthBips32 interestRate,
     std::uint32_t paymentInterval,
     std::uint32_t paymentsRemaining,
-    TenthBips16 managementFeeRate)
+    TenthBips32 managementFeeRate)
 {
     auto const periodicRate = loanPeriodicRate(interestRate, paymentInterval);
     XRPL_ASSERT(

src/xrpld/app/tx/detail/LoanPay.cpp

@@ -374,6 +374,10 @@ LoanPay::doApply()
         paymentParts->principalPaid + paymentParts->interestPaid;
     auto const totalPaidToVaultRounded =
         roundToAsset(asset, totalPaidToVaultRaw, vaultScale, Number::downward);
+    XRPL_ASSERT_PARTS(
+        !asset.integral() || totalPaidToVaultRaw == totalPaidToVaultRounded,
+        "ripple::LoanPay::doApply",
+        "rounding does nothing for integral asset");
     auto const totalPaidToVaultForDebt =
         totalPaidToVaultRaw - paymentParts->valueChange;
 
@@ -405,7 +409,21 @@ LoanPay::doApply()
     // Vault object state changes
     view.update(vaultSle);
 
+    Number const assetsAvailableBefore = *assetsAvailableProxy;
+    Number const pseudoAccountBalanceBefore = accountHolds(
+        view,
+        vaultPseudoAccount,
+        asset,
+        FreezeHandling::fhIGNORE_FREEZE,
+        AuthHandling::ahIGNORE_AUTH,
+        j_);
+
     {
+        XRPL_ASSERT_PARTS(
+            assetsAvailableBefore == pseudoAccountBalanceBefore,
+            "ripple::LoanPay::doApply",
+            "vault pseudo balance agrees before");
+
         auto assetsTotalProxy = vaultSle->at(sfAssetsTotal);
 
         assetsAvailableProxy += totalPaidToVaultRounded;
@@ -415,6 +433,13 @@ LoanPay::doApply()
             *assetsAvailableProxy <= *assetsTotalProxy,
             "ripple::LoanPay::doApply",
             "assets available must not be greater than assets outstanding");
+
+        if (*assetsAvailableProxy > *assetsTotalProxy)
+        {
+            // LCOV_EXCL_START
+            return tecINTERNAL;
+            // LCOV_EXCL_STOP
+        }
     }
 
     // Move funds
@@ -488,6 +513,19 @@ LoanPay::doApply()
             WaiveTransferFee::Yes))
         return ter;
 
+    Number const assetsAvailableAfter = *assetsAvailableProxy;
+    Number const pseudoAccountBalanceAfter = accountHolds(
+        view,
+        vaultPseudoAccount,
+        asset,
+        FreezeHandling::fhIGNORE_FREEZE,
+        AuthHandling::ahIGNORE_AUTH,
+        j_);
+    XRPL_ASSERT_PARTS(
+        assetsAvailableAfter == pseudoAccountBalanceAfter,
+        "ripple::LoanPay::doApply",
+        "vault pseudo balance agrees after");
+
 #if !NDEBUG
     auto const accountBalanceAfter = accountCanSend(
         view, account_, asset, fhIGNORE_FREEZE, ahIGNORE_AUTH, j_);

src/xrpld/app/tx/detail/LoanSet.cpp

@@ -318,6 +318,87 @@ LoanSet::preclaim(PreclaimContext const& ctx)
     return tesSUCCESS;
 }
 
+TER
+LoanSet::checkGuards(
+    Asset const& vaultAsset,
+    Number const& principalRequested,
+    TenthBips32 interestRate,
+    std::uint32_t paymentTotal,
+    LoanProperties const& properties,
+    beast::Journal j)
+{
+    auto const totalInterestOutstanding =
+        properties.totalValueOutstanding - principalRequested;
+    // Guard 1: if there is no computed total interest over the life of the
+    // loan for a non-zero interest rate, we cannot properly amortize the
+    // loan
+    if (interestRate > TenthBips32{0} && totalInterestOutstanding <= 0)
+    {
+        // Unless this is a zero-interest loan, there must be some interest
+        // due on the loan, even if it's (measurable) dust
+        JLOG(j.warn()) << "Loan for " << principalRequested << " with "
+                       << interestRate << "% interest has no interest due";
+        return tecPRECISION_LOSS;
+    }
+    // Guard 1a: If there is any interest computed over the life of the
+    // loan, for a zero interest rate, something went sideways.
+    if (interestRate == TenthBips32{0} && totalInterestOutstanding > 0)
+    {
+        // LCOV_EXCL_START
+        JLOG(j.warn()) << "Loan for " << principalRequested
+                       << " with 0% interest has interest due";
+        return tecINTERNAL;
+        // LCOV_EXCL_STOP
+    }
+
+    // Guard 2: if the principal portion of the first periodic payment is
+    // too small to be accurately represented with the given rounding mode,
+    // raise an error
+    if (properties.firstPaymentPrincipal <= 0)
+    {
+        // Check that some true (unrounded) principal is paid each period.
+        // Since the first payment pays the least principal, if it's good,
+        // they'll all be good. Note that the outstanding principal is
+        // rounded, and may not change right away.
+        JLOG(j.warn()) << "Loan is unable to pay principal.";
+        return tecPRECISION_LOSS;
+    }
+
+    // Guard 3: If the periodic payment is so small that it can't even be
+    // rounded to a representable value, then the loan can't be paid. Also,
+    // avoids dividing by 0.
+    auto const roundedPayment = roundPeriodicPayment(
+        vaultAsset, properties.periodicPayment, properties.loanScale);
+    if (roundedPayment == beast::zero)
+    {
+        JLOG(j.warn()) << "Loan Periodic payment ("
+                       << properties.periodicPayment << ") rounds to 0. ";
+        return tecPRECISION_LOSS;
+    }
+
+    // Guard 4: if the rounded periodic payment is large enough that the
+    // loan can't be amortized in the specified number of payments, raise an
+    // error
+    {
+        NumberRoundModeGuard mg(Number::upward);
+
+        if (std::int64_t const computedPayments{
+                properties.totalValueOutstanding / roundedPayment};
+            computedPayments != paymentTotal)
+        {
+            JLOG(j.warn()) << "Loan Periodic payment ("
+                           << properties.periodicPayment << ") rounding ("
+                           << roundedPayment << ") on a total value of "
+                           << properties.totalValueOutstanding
+                           << " can not complete the loan in the specified "
+                              "number of payments ("
+                           << computedPayments << " != " << paymentTotal << ")";
+            return tecPRECISION_LOSS;
+        }
+    }
+    return tesSUCCESS;
+}
+
 TER
 LoanSet::doApply()
 {
@@ -396,72 +477,14 @@ LoanSet::doApply()
         }
     }
 
-    auto const totalInterestOutstanding =
+    if (auto const ret = checkGuards(
-        properties.totalValueOutstanding - principalRequested;
+            vaultAsset,
-    // Guard 1: if there is no computed total interest over the life of the loan
+            principalRequested,
-    // for a non-zero interest rate, we cannot properly amortize the loan
+            interestRate,
-    if (interestRate > TenthBips32{0} && totalInterestOutstanding <= 0)
+            paymentTotal,
-    {
+            properties,
-        // Unless this is a zero-interest loan, there must be some interest due
+            j_))
-        // on the loan, even if it's (measurable) dust
+        return ret;
-        JLOG(j_.warn()) << "Loan for " << principalRequested << " with "
-                        << interestRate << "% interest has no interest due";
-        return tecPRECISION_LOSS;
-    }
-    // Guard 1a: If there is any interest computed over the life of the loan,
-    // for a zero interest rate, something went sideways.
-    if (interestRate == TenthBips32{0} && totalInterestOutstanding > 0)
-    {
-        // LCOV_EXCL_START
-        JLOG(j_.warn()) << "Loan for " << principalRequested
-                        << " with 0% interest has interest due";
-        return tecINTERNAL;
-        // LCOV_EXCL_STOP
-    }
-
-    // Guard 2: if the principal portion of the first periodic payment is too
-    // small to be accurately represented with the given rounding mode, raise an
-    // error
-    if (properties.firstPaymentPrincipal <= 0)
-    {
-        // Check that some true (unrounded) principal is paid each period. Since
-        // the first payment pays the least principal, if it's good, they'll
-        // all be good. Note that the outstanding principal is rounded, and
-        // may not change right away.
-        JLOG(j_.warn()) << "Loan is unable to pay principal.";
-        return tecPRECISION_LOSS;
-    }
-
-    // Guard 3: If the periodic payment is so small that it can't even be
-    // rounded to a representable value, then the loan can't be paid. Also,
-    // avoids dividing by 0.
-    auto const roundedPayment = roundPeriodicPayment(
-        vaultAsset, properties.periodicPayment, properties.loanScale);
-    if (roundedPayment == beast::zero)
-    {
-        JLOG(j_.warn()) << "Loan Periodic payment ("
-                        << properties.periodicPayment << ") rounds to 0. ";
-        return tecPRECISION_LOSS;
-    }
-
-    // Guard 4: if the rounded periodic payment is large enough that the loan
-    // can't be amortized in the specified number of payments, raise an error
-    {
-        NumberRoundModeGuard mg(Number::upward);
-
-        if (std::int64_t const computedPayments{
-                properties.totalValueOutstanding / roundedPayment};
-            computedPayments != paymentTotal)
-        {
-            JLOG(j_.warn())
-                << "Loan Periodic payment (" << properties.periodicPayment
-                << ") rounding (" << roundedPayment
-                << ") will complete the "
-                   "loan in less than the specified number of payments ("
-                << computedPayments << " < " << paymentTotal << ")";
-            return tecPRECISION_LOSS;
-        }
-    }
 
     // Check that the other computed values are valid
     if (properties.managementFeeOwedToBroker < 0 ||

src/xrpld/app/tx/detail/LoanSet.h

@@ -20,6 +20,7 @@
 #ifndef RIPPLE_TX_LOANSET_H_INCLUDED
 #define RIPPLE_TX_LOANSET_H_INCLUDED
 
+#include <xrpld/app/misc/LendingHelpers.h>
 #include <xrpld/app/tx/detail/Transactor.h>
 
 namespace ripple {
@@ -54,6 +55,15 @@ public:
     static TER
     preclaim(PreclaimContext const& ctx);
 
+    static TER
+    checkGuards(
+        Asset const& vaultAsset,
+        Number const& principalRequested,
+        TenthBips32 interestRate,
+        std::uint32_t paymentTotal,
+        LoanProperties const& properties,
+        beast::Journal j);
+
     TER
     doApply() override;