diff --git a/include/xrpl/protocol/STAmount.h b/include/xrpl/protocol/STAmount.h index dadeec096f3..88a46421599 100644 --- a/include/xrpl/protocol/STAmount.h +++ b/include/xrpl/protocol/STAmount.h @@ -42,8 +42,8 @@ class STAmount final : public STBase, public CountedObject public: using value_type = STAmount; - static int const cMinOffset = -96; - static int const cMaxOffset = 80; + static int constexpr cMinOffset = -96; + static int constexpr cMaxOffset = 80; // Maximum native value supported by the code constexpr static std::uint64_t cMinValue = 1'000'000'000'000'000ull; @@ -739,6 +739,21 @@ canAdd(STAmount const& amt1, STAmount const& amt2); bool canSubtract(STAmount const& amt1, STAmount const& amt2); +/** Get the scale of a Number for a given asset. + * + * "scale" is similar to "exponent", but from the perspective of STAmount, which has different rules + * and mantissa ranges for determining the exponent than Number. + * + * @param number The Number to get the scale of. + * @param asset The asset to use for determining the scale. + * @return The scale of this Number for the given asset. + */ +inline int +scale(Number const& number, Asset const& asset) +{ + return STAmount{asset, number}.exponent(); +} + } // namespace xrpl //------------------------------------------------------------------------------ diff --git a/include/xrpl/tx/invariants/VaultInvariant.h b/include/xrpl/tx/invariants/VaultInvariant.h index ded9e4618bd..d04cb2998b9 100644 --- a/include/xrpl/tx/invariants/VaultInvariant.h +++ b/include/xrpl/tx/invariants/VaultInvariant.h @@ -8,6 +8,7 @@ #include #include +#include #include #include @@ -60,11 +61,19 @@ class ValidVault Shares static make(SLE const&); }; +public: + struct DeltaInfo final + { + Number delta = numZero; + std::optional scale; + }; + +private: std::vector afterVault_ = {}; std::vector afterMPTs_ = {}; std::vector beforeVault_ = {}; std::vector beforeMPTs_ = {}; - std::unordered_map deltas_ = {}; + std::unordered_map deltas_ = {}; public: void @@ -72,6 +81,10 @@ class ValidVault bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&); + + // Compute the coarsest scale required to represent all numbers + [[nodiscard]] static std::int32_t + computeCoarsestScale(std::vector const& numbers); }; } // namespace xrpl diff --git a/include/xrpl/tx/transactors/lending/LendingHelpers.h b/include/xrpl/tx/transactors/lending/LendingHelpers.h index 4057c9c1738..8dd6866ac34 100644 --- a/include/xrpl/tx/transactors/lending/LendingHelpers.h +++ b/include/xrpl/tx/transactors/lending/LendingHelpers.h @@ -171,7 +171,7 @@ getAssetsTotalScale(SLE::const_ref vaultSle) { if (!vaultSle) return Number::minExponent - 1; // LCOV_EXCL_LINE - return STAmount{vaultSle->at(sfAsset), vaultSle->at(sfAssetsTotal)}.exponent(); + return scale(vaultSle->at(sfAssetsTotal), vaultSle->at(sfAsset)); } TER diff --git a/src/libxrpl/tx/invariants/VaultInvariant.cpp b/src/libxrpl/tx/invariants/VaultInvariant.cpp index f0dd82c2f88..b45d4046c8d 100644 --- a/src/libxrpl/tx/invariants/VaultInvariant.cpp +++ b/src/libxrpl/tx/invariants/VaultInvariant.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -61,10 +62,12 @@ ValidVault::visitEntry( "xrpl::ValidVault::visitEntry : some object is available"); // Number balanceDelta will capture the difference (delta) between "before" - // state (zero if created) and "after" state (zero if destroyed), so the - // invariants can validate that the change in account balances matches the - // change in vault balances, stored to deltas_ at the end of this function. - Number balanceDelta{}; + // state (zero if created) and "after" state (zero if destroyed), and + // preserves value scale (exponent) to round values to the same scale during + // validation. It is used to validate that the change in account + // balances matches the change in vault balances, stored to deltas_ at the + // end of this function. + DeltaInfo balanceDelta{numZero, std::nullopt}; std::int8_t sign = 0; if (before) @@ -78,18 +81,34 @@ ValidVault::visitEntry( // At this moment we have no way of telling if this object holds // vault shares or something else. Save it for finalize. beforeMPTs_.push_back(Shares::make(*before)); - balanceDelta = static_cast(before->getFieldU64(sfOutstandingAmount)); + balanceDelta.delta = + static_cast(before->getFieldU64(sfOutstandingAmount)); + // MPTs are ints, so the scale is always 0. + balanceDelta.scale = 0; sign = 1; break; case ltMPTOKEN: - balanceDelta = static_cast(before->getFieldU64(sfMPTAmount)); + balanceDelta.delta = static_cast(before->getFieldU64(sfMPTAmount)); + // MPTs are ints, so the scale is always 0. + balanceDelta.scale = 0; sign = -1; break; case ltACCOUNT_ROOT: - case ltRIPPLE_STATE: - balanceDelta = before->getFieldAmount(sfBalance); + balanceDelta.delta = before->getFieldAmount(sfBalance); + // Account balance is XRP, which is an int, so the scale is + // always 0. + balanceDelta.scale = 0; sign = -1; break; + case ltRIPPLE_STATE: { + auto const amount = before->getFieldAmount(sfBalance); + balanceDelta.delta = amount; + // Trust Line balances are STAmounts, so we can use the exponent + // directly to get the scale. + balanceDelta.scale = amount.exponent(); + sign = -1; + break; + } default:; } } @@ -105,19 +124,36 @@ ValidVault::visitEntry( // At this moment we have no way of telling if this object holds // vault shares or something else. Save it for finalize. afterMPTs_.push_back(Shares::make(*after)); - balanceDelta -= + balanceDelta.delta -= Number(static_cast(after->getFieldU64(sfOutstandingAmount))); + // MPTs are ints, so the scale is always 0. + balanceDelta.scale = 0; sign = 1; break; case ltMPTOKEN: - balanceDelta -= Number(static_cast(after->getFieldU64(sfMPTAmount))); + balanceDelta.delta -= + Number(static_cast(after->getFieldU64(sfMPTAmount))); + // MPTs are ints, so the scale is always 0. + balanceDelta.scale = 0; sign = -1; break; case ltACCOUNT_ROOT: - case ltRIPPLE_STATE: - balanceDelta -= Number(after->getFieldAmount(sfBalance)); + balanceDelta.delta -= Number(after->getFieldAmount(sfBalance)); + // Account balance is XRP, which is an int, so the scale is + // always 0. + balanceDelta.scale = 0; sign = -1; break; + case ltRIPPLE_STATE: { + auto const amount = after->getFieldAmount(sfBalance); + balanceDelta.delta -= Number(amount); + // Trust Line balances are STAmounts, so we can use the exponent + // directly to get the scale. + if (amount.exponent() > balanceDelta.scale) + balanceDelta.scale = amount.exponent(); + sign = -1; + break; + } default:; } } @@ -129,7 +165,11 @@ ValidVault::visitEntry( // transferred to the account. We intentionally do not compare balanceDelta // against zero, to avoid missing such updates. if (sign != 0) - deltas_[key] = balanceDelta * sign; + { + XRPL_ASSERT_PARTS(balanceDelta.scale, "xrpl::ValidVault::visitEntry", "scale initialized"); + balanceDelta.delta *= sign; + deltas_[key] = balanceDelta; + } } bool @@ -391,13 +431,13 @@ ValidVault::finalize( } auto const& vaultAsset = afterVault.asset; - auto const deltaAssets = [&](AccountID const& id) -> std::optional { + auto const deltaAssets = [&](AccountID const& id) -> std::optional { auto const get = // - [&](auto const& it, std::int8_t sign = 1) -> std::optional { + [&](auto const& it, std::int8_t sign = 1) -> std::optional { if (it == deltas_.end()) return std::nullopt; - return it->second * sign; + return DeltaInfo{it->second.delta * sign, it->second.scale}; }; return std::visit( @@ -416,7 +456,7 @@ ValidVault::finalize( }, vaultAsset.value()); }; - auto const deltaAssetsTxAccount = [&]() -> std::optional { + auto const deltaAssetsTxAccount = [&]() -> std::optional { auto ret = deltaAssets(tx[sfAccount]); // Nothing returned or not XRP transaction if (!ret.has_value() || !vaultAsset.native()) @@ -427,20 +467,20 @@ ValidVault::finalize( delegate.has_value() && *delegate != tx[sfAccount]) return ret; - *ret += fee.drops(); - if (*ret == zero) + ret->delta += fee.drops(); + if (ret->delta == zero) return std::nullopt; return ret; }; - auto const deltaShares = [&](AccountID const& id) -> std::optional { + auto const deltaShares = [&](AccountID const& id) -> std::optional { auto const it = [&]() { if (id == afterVault.pseudoId) return deltas_.find(keylet::mptIssuance(afterVault.shareMPTID).key); return deltas_.find(keylet::mptoken(afterVault.shareMPTID, id).key); }(); - return it != deltas_.end() ? std::optional(it->second) : std::nullopt; + return it != deltas_.end() ? std::optional(it->second) : std::nullopt; }; auto const vaultHoldsNoAssets = [&](Vault const& vault) { @@ -567,16 +607,36 @@ ValidVault::finalize( !beforeVault_.empty(), "xrpl::ValidVault::finalize : deposit updated a vault"); auto const& beforeVault = beforeVault_[0]; - auto const vaultDeltaAssets = deltaAssets(afterVault.pseudoId); - - if (!vaultDeltaAssets) + auto const maybeVaultDeltaAssets = deltaAssets(afterVault.pseudoId); + if (!maybeVaultDeltaAssets) { JLOG(j.fatal()) << // "Invariant failed: deposit must change vault balance"; return false; // That's all we can do } - if (*vaultDeltaAssets > tx[sfAmount]) + // Get the coarsest scale to round calculations to + DeltaInfo totalDelta{ + afterVault.assetsTotal - beforeVault.assetsTotal, + std::max( + scale(afterVault.assetsTotal, vaultAsset), + scale(beforeVault.assetsTotal, vaultAsset))}; + DeltaInfo availableDelta{ + afterVault.assetsAvailable - beforeVault.assetsAvailable, + std::max( + scale(afterVault.assetsAvailable, vaultAsset), + scale(beforeVault.assetsAvailable, vaultAsset))}; + auto const minScale = computeCoarsestScale({ + *maybeVaultDeltaAssets, + totalDelta, + availableDelta, + }); + + auto const vaultDeltaAssets = + roundToAsset(vaultAsset, maybeVaultDeltaAssets->delta, minScale); + auto const txAmount = roundToAsset(vaultAsset, tx[sfAmount], minScale); + + if (vaultDeltaAssets > txAmount) { JLOG(j.fatal()) << // "Invariant failed: deposit must not change vault " @@ -584,7 +644,7 @@ ValidVault::finalize( result = false; } - if (*vaultDeltaAssets <= zero) + if (vaultDeltaAssets <= zero) { JLOG(j.fatal()) << // "Invariant failed: deposit must increase vault balance"; @@ -601,16 +661,23 @@ ValidVault::finalize( if (!issuerDeposit) { - auto const accountDeltaAssets = deltaAssetsTxAccount(); - if (!accountDeltaAssets) + auto const maybeAccDeltaAssets = deltaAssetsTxAccount(); + if (!maybeAccDeltaAssets) { JLOG(j.fatal()) << // "Invariant failed: deposit must change depositor " "balance"; return false; } + auto const localMinScale = + std::max(minScale, computeCoarsestScale({*maybeAccDeltaAssets})); + + auto const accountDeltaAssets = + roundToAsset(vaultAsset, maybeAccDeltaAssets->delta, localMinScale); + auto const localVaultDeltaAssets = + roundToAsset(vaultAsset, vaultDeltaAssets, localMinScale); - if (*accountDeltaAssets >= zero) + if (accountDeltaAssets >= zero) { JLOG(j.fatal()) << // "Invariant failed: deposit must decrease depositor " @@ -618,7 +685,7 @@ ValidVault::finalize( result = false; } - if (*accountDeltaAssets * -1 != *vaultDeltaAssets) + if (localVaultDeltaAssets * -1 != accountDeltaAssets) { JLOG(j.fatal()) << // "Invariant failed: deposit must change vault and " @@ -636,16 +703,17 @@ ValidVault::finalize( result = false; } - auto const accountDeltaShares = deltaShares(tx[sfAccount]); - if (!accountDeltaShares) + auto const maybeAccDeltaShares = deltaShares(tx[sfAccount]); + if (!maybeAccDeltaShares) { JLOG(j.fatal()) << // "Invariant failed: deposit must change depositor " "shares"; return false; // That's all we can do } - - if (*accountDeltaShares <= zero) + // We don't need to round shares, they are integral MPT + auto const& accountDeltaShares = *maybeAccDeltaShares; + if (accountDeltaShares.delta <= zero) { JLOG(j.fatal()) << // "Invariant failed: deposit must increase depositor " @@ -653,15 +721,17 @@ ValidVault::finalize( result = false; } - auto const vaultDeltaShares = deltaShares(afterVault.pseudoId); - if (!vaultDeltaShares || *vaultDeltaShares == zero) + auto const maybeVaultDeltaShares = deltaShares(afterVault.pseudoId); + if (!maybeVaultDeltaShares || maybeVaultDeltaShares->delta == zero) { JLOG(j.fatal()) << // "Invariant failed: deposit must change vault shares"; return false; // That's all we can do } - if (*vaultDeltaShares * -1 != *accountDeltaShares) + // We don't need to round shares, they are integral MPT + auto const& vaultDeltaShares = *maybeVaultDeltaShares; + if (vaultDeltaShares.delta * -1 != accountDeltaShares.delta) { JLOG(j.fatal()) << // "Invariant failed: deposit must change depositor and " @@ -669,13 +739,18 @@ ValidVault::finalize( result = false; } - if (beforeVault.assetsTotal + *vaultDeltaAssets != afterVault.assetsTotal) + auto const assetTotalDelta = roundToAsset( + vaultAsset, afterVault.assetsTotal - beforeVault.assetsTotal, minScale); + if (assetTotalDelta != vaultDeltaAssets) { JLOG(j.fatal()) << "Invariant failed: deposit and assets " "outstanding must add up"; result = false; } - if (beforeVault.assetsAvailable + *vaultDeltaAssets != afterVault.assetsAvailable) + + auto const assetAvailableDelta = roundToAsset( + vaultAsset, afterVault.assetsAvailable - beforeVault.assetsAvailable, minScale); + if (assetAvailableDelta != vaultDeltaAssets) { JLOG(j.fatal()) << "Invariant failed: deposit and assets " "available must add up"; @@ -693,16 +768,33 @@ ValidVault::finalize( "vault"); auto const& beforeVault = beforeVault_[0]; - auto const vaultDeltaAssets = deltaAssets(afterVault.pseudoId); + auto const maybeVaultDeltaAssets = deltaAssets(afterVault.pseudoId); - if (!vaultDeltaAssets) + if (!maybeVaultDeltaAssets) { JLOG(j.fatal()) << "Invariant failed: withdrawal must " "change vault balance"; return false; // That's all we can do } - if (*vaultDeltaAssets >= zero) + // Get the most coarse scale to round calculations to + auto const totalDelta = DeltaInfo{ + afterVault.assetsTotal - beforeVault.assetsTotal, + std::max( + scale(afterVault.assetsTotal, vaultAsset), + scale(beforeVault.assetsTotal, vaultAsset))}; + auto const availableDelta = DeltaInfo{ + afterVault.assetsAvailable - beforeVault.assetsAvailable, + std::max( + scale(afterVault.assetsAvailable, vaultAsset), + scale(beforeVault.assetsAvailable, vaultAsset))}; + auto const minScale = + computeCoarsestScale({*maybeVaultDeltaAssets, totalDelta, availableDelta}); + + auto const vaultPseudoDeltaAssets = + roundToAsset(vaultAsset, maybeVaultDeltaAssets->delta, minScale); + + if (vaultPseudoDeltaAssets >= zero) { JLOG(j.fatal()) << "Invariant failed: withdrawal must " "decrease vault balance"; @@ -720,15 +812,15 @@ ValidVault::finalize( if (!issuerWithdrawal) { - auto const accountDeltaAssets = deltaAssetsTxAccount(); - auto const otherAccountDelta = [&]() -> std::optional { + auto const maybeAccDelta = deltaAssetsTxAccount(); + auto const maybeOtherAccDelta = [&]() -> std::optional { if (auto const destination = tx[~sfDestination]; destination && *destination != tx[sfAccount]) return deltaAssets(*destination); return std::nullopt; }(); - if (accountDeltaAssets.has_value() == otherAccountDelta.has_value()) + if (maybeAccDelta.has_value() == maybeOtherAccDelta.has_value()) { JLOG(j.fatal()) << // "Invariant failed: withdrawal must change one " @@ -737,9 +829,17 @@ ValidVault::finalize( } auto const destinationDelta = // - accountDeltaAssets ? *accountDeltaAssets : *otherAccountDelta; + maybeAccDelta ? *maybeAccDelta : *maybeOtherAccDelta; + + // the scale of destinationDelta can be coarser than + // minScale, so we take that into account when rounding + auto const localMinScale = + std::max(minScale, computeCoarsestScale({destinationDelta})); - if (destinationDelta <= zero) + auto const roundedDestinationDelta = + roundToAsset(vaultAsset, destinationDelta.delta, localMinScale); + + if (roundedDestinationDelta <= zero) { JLOG(j.fatal()) << // "Invariant failed: withdrawal must increase " @@ -747,7 +847,9 @@ ValidVault::finalize( result = false; } - if (*vaultDeltaAssets * -1 != destinationDelta) + auto const localPseudoDeltaAssets = + roundToAsset(vaultAsset, vaultPseudoDeltaAssets, localMinScale); + if (localPseudoDeltaAssets * -1 != roundedDestinationDelta) { JLOG(j.fatal()) << // "Invariant failed: withdrawal must change vault " @@ -756,6 +858,7 @@ ValidVault::finalize( } } + // We don't need to round shares, they are integral MPT auto const accountDeltaShares = deltaShares(tx[sfAccount]); if (!accountDeltaShares) { @@ -765,7 +868,7 @@ ValidVault::finalize( return false; } - if (*accountDeltaShares >= zero) + if (accountDeltaShares->delta >= zero) { JLOG(j.fatal()) << // "Invariant failed: withdrawal must decrease depositor " @@ -773,15 +876,16 @@ ValidVault::finalize( result = false; } + // We don't need to round shares, they are integral MPT auto const vaultDeltaShares = deltaShares(afterVault.pseudoId); - if (!vaultDeltaShares || *vaultDeltaShares == zero) + if (!vaultDeltaShares || vaultDeltaShares->delta == zero) { JLOG(j.fatal()) << // "Invariant failed: withdrawal must change vault shares"; return false; // That's all we can do } - if (*vaultDeltaShares * -1 != *accountDeltaShares) + if (vaultDeltaShares->delta * -1 != accountDeltaShares->delta) { JLOG(j.fatal()) << // "Invariant failed: withdrawal must change depositor " @@ -789,15 +893,20 @@ ValidVault::finalize( result = false; } + auto const assetTotalDelta = roundToAsset( + vaultAsset, afterVault.assetsTotal - beforeVault.assetsTotal, minScale); // Note, vaultBalance is negative (see check above) - if (beforeVault.assetsTotal + *vaultDeltaAssets != afterVault.assetsTotal) + if (assetTotalDelta != vaultPseudoDeltaAssets) { JLOG(j.fatal()) << "Invariant failed: withdrawal and " "assets outstanding must add up"; result = false; } - if (beforeVault.assetsAvailable + *vaultDeltaAssets != afterVault.assetsAvailable) + auto const assetAvailableDelta = roundToAsset( + vaultAsset, afterVault.assetsAvailable - beforeVault.assetsAvailable, minScale); + + if (assetAvailableDelta != vaultPseudoDeltaAssets) { JLOG(j.fatal()) << "Invariant failed: withdrawal and " "assets available must add up"; @@ -828,10 +937,24 @@ ValidVault::finalize( } } - auto const vaultDeltaAssets = deltaAssets(afterVault.pseudoId); - if (vaultDeltaAssets) + auto const maybeVaultDeltaAssets = deltaAssets(afterVault.pseudoId); + if (maybeVaultDeltaAssets) { - if (*vaultDeltaAssets >= zero) + auto const totalDelta = DeltaInfo{ + afterVault.assetsTotal - beforeVault.assetsTotal, + std::max( + scale(afterVault.assetsTotal, vaultAsset), + scale(beforeVault.assetsTotal, vaultAsset))}; + auto const availableDelta = DeltaInfo{ + afterVault.assetsAvailable - beforeVault.assetsAvailable, + std::max( + scale(afterVault.assetsAvailable, vaultAsset), + scale(beforeVault.assetsAvailable, vaultAsset))}; + auto const minScale = + computeCoarsestScale({*maybeVaultDeltaAssets, totalDelta, availableDelta}); + auto const vaultDeltaAssets = + roundToAsset(vaultAsset, maybeVaultDeltaAssets->delta, minScale); + if (vaultDeltaAssets >= zero) { JLOG(j.fatal()) << // "Invariant failed: clawback must decrease vault " @@ -839,7 +962,9 @@ ValidVault::finalize( result = false; } - if (beforeVault.assetsTotal + *vaultDeltaAssets != afterVault.assetsTotal) + auto const assetsTotalDelta = roundToAsset( + vaultAsset, afterVault.assetsTotal - beforeVault.assetsTotal, minScale); + if (assetsTotalDelta != vaultDeltaAssets) { JLOG(j.fatal()) << // "Invariant failed: clawback and assets outstanding " @@ -847,8 +972,11 @@ ValidVault::finalize( result = false; } - if (beforeVault.assetsAvailable + *vaultDeltaAssets != - afterVault.assetsAvailable) + auto const assetAvailableDelta = roundToAsset( + vaultAsset, + afterVault.assetsAvailable - beforeVault.assetsAvailable, + minScale); + if (assetAvailableDelta != vaultDeltaAssets) { JLOG(j.fatal()) << // "Invariant failed: clawback and assets available " @@ -863,15 +991,15 @@ ValidVault::finalize( return false; // That's all we can do } - auto const accountDeltaShares = deltaShares(tx[sfHolder]); - if (!accountDeltaShares) + // We don't need to round shares, they are integral MPT + auto const maybeAccountDeltaShares = deltaShares(tx[sfHolder]); + if (!maybeAccountDeltaShares) { JLOG(j.fatal()) << // "Invariant failed: clawback must change holder shares"; return false; // That's all we can do } - - if (*accountDeltaShares >= zero) + if (maybeAccountDeltaShares->delta >= zero) { JLOG(j.fatal()) << // "Invariant failed: clawback must decrease holder " @@ -879,15 +1007,16 @@ ValidVault::finalize( result = false; } + // We don't need to round shares, they are integral MPT auto const vaultDeltaShares = deltaShares(afterVault.pseudoId); - if (!vaultDeltaShares || *vaultDeltaShares == zero) + if (!vaultDeltaShares || vaultDeltaShares->delta == zero) { JLOG(j.fatal()) << // "Invariant failed: clawback must change vault shares"; return false; // That's all we can do } - if (*vaultDeltaShares * -1 != *accountDeltaShares) + if (vaultDeltaShares->delta * -1 != maybeAccountDeltaShares->delta) { JLOG(j.fatal()) << // "Invariant failed: clawback must change holder and " @@ -924,4 +1053,19 @@ ValidVault::finalize( return true; } +[[nodiscard]] std::int32_t +ValidVault::computeCoarsestScale(std::vector const& numbers) +{ + if (numbers.empty()) + return 0; + + auto const max = + std::max_element(numbers.begin(), numbers.end(), [](auto const& a, auto const& b) -> bool { + return a.scale < b.scale; + }); + XRPL_ASSERT_PARTS( + max->scale, "xrpl::ValidVault::computeCoarsestScale", "scale set for destinationDelta"); + return max->scale.value_or(STAmount::cMaxOffset); +} + } // namespace xrpl diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp index 69469923761..96d178fd0e2 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp @@ -123,7 +123,7 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx) return roundToAsset( vaultAsset, tenthBipsOfValue(currentDebtTotal, TenthBips32(sleBroker->at(sfCoverRateMinimum))), - currentDebtTotal.exponent()); + scale(currentDebtTotal, vaultAsset)); }(); if (coverAvail < amount) return tecINSUFFICIENT_FUNDS; diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 9653a36edfe..edbbd3800bf 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -19,9 +19,14 @@ #include #include #include +#include #include +#include +#include +#include + namespace xrpl { namespace test { @@ -3805,6 +3810,128 @@ class Invariants_test : public beast::unit_test::suite precloseMpt); } + void + testVaultComputeCoarsestScale() + { + using namespace jtx; + + Account const issuer{"issuer"}; + PrettyAsset const vaultAsset = issuer["IOU"]; + + struct TestCase + { + std::string name; + std::int32_t expectedMinScale; + std::vector values; + }; + + NumberMantissaScaleGuard g{MantissaRange::large}; + + auto makeDelta = [&vaultAsset](Number const& n) -> ValidVault::DeltaInfo { + return {n, scale(n, vaultAsset.raw())}; + }; + + auto const testCases = std::vector{ + { + .name = "No values", + .expectedMinScale = 0, + .values = {}, + }, + { + .name = "Mixed integer and Number values", + .expectedMinScale = -15, + .values = {makeDelta(1), makeDelta(-1), makeDelta(Number{10, -1})}, + }, + { + .name = "Mixed scales", + .expectedMinScale = -17, + .values = + {makeDelta(Number{1, -2}), makeDelta(Number{5, -3}), makeDelta(Number{3, -2})}, + }, + { + .name = "Equal scales", + .expectedMinScale = -16, + .values = + {makeDelta(Number{1, -1}), makeDelta(Number{5, -1}), makeDelta(Number{1, -1})}, + }, + { + .name = "Mixed mantissa sizes", + .expectedMinScale = -12, + .values = + {makeDelta(Number{1}), + makeDelta(Number{1234, -3}), + makeDelta(Number{12345, -6}), + makeDelta(Number{123, 1})}, + }, + }; + + for (auto const& tc : testCases) + { + testcase("vault computeCoarsestScale: " + tc.name); + + auto const actualScale = ValidVault::computeCoarsestScale(tc.values); + + BEAST_EXPECTS( + actualScale == tc.expectedMinScale, + "expected: " + std::to_string(tc.expectedMinScale) + + ", actual: " + std::to_string(actualScale)); + for (auto const& num : tc.values) + { + // None of these scales are far enough apart that rounding the + // values would lose information, so check that the rounded + // value matches the original. + auto const actualRounded = roundToAsset(vaultAsset, num.delta, actualScale); + BEAST_EXPECTS( + actualRounded == num.delta, + "number " + to_string(num.delta) + " rounded to scale " + + std::to_string(actualScale) + " is " + to_string(actualRounded)); + } + } + + auto const testCases2 = std::vector{ + { + .name = "False equivalence", + .expectedMinScale = -15, + .values = + { + makeDelta(Number{1234567890123456789, -18}), + makeDelta(Number{12345, -4}), + makeDelta(Number{1}), + }, + }, + }; + + // Unlike the first set of test cases, the values in these test could + // look equivalent if using the wrong scale. + for (auto const& tc : testCases2) + { + testcase("vault computeCoarsestScale: " + tc.name); + + auto const actualScale = ValidVault::computeCoarsestScale(tc.values); + + BEAST_EXPECTS( + actualScale == tc.expectedMinScale, + "expected: " + std::to_string(tc.expectedMinScale) + + ", actual: " + std::to_string(actualScale)); + std::optional first; + Number firstRounded; + for (auto const& num : tc.values) + { + if (!first) + { + first = num.delta; + firstRounded = roundToAsset(vaultAsset, num.delta, actualScale); + continue; + } + auto const numRounded = roundToAsset(vaultAsset, num.delta, actualScale); + BEAST_EXPECTS( + numRounded != firstRounded, + "at a scale of " + std::to_string(actualScale) + " " + to_string(num.delta) + + " == " + to_string(*first)); + } + } + } + public: void run() override @@ -3830,6 +3957,7 @@ class Invariants_test : public beast::unit_test::suite testValidPseudoAccounts(); testValidLoanBroker(); testVault(); + testVaultComputeCoarsestScale(); } }; diff --git a/src/test/app/LendingHelpers_test.cpp b/src/test/app/LendingHelpers_test.cpp index 24661c18fba..3b6fd73e327 100644 --- a/src/test/app/LendingHelpers_test.cpp +++ b/src/test/app/LendingHelpers_test.cpp @@ -3,7 +3,6 @@ #include #include #include -#include #include #include diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index fa7befd39ef..d40fcd61f2e 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -2665,7 +2665,7 @@ class Loan_test : public beast::unit_test::suite env(manage(lender, loanKeylet.key, tfLoanDefault), ter(tecNO_PERMISSION)); }); -#if LOANTODO +#if LOAN_TODO // TODO /* @@ -5326,7 +5326,7 @@ class Loan_test : public beast::unit_test::suite } } -#if LOANTODO +#if LOAN_TODO void testLoanPayLateFullPaymentBypassesPenalties() { @@ -6981,14 +6981,145 @@ class Loan_test : public beast::unit_test::suite BEAST_EXPECT(afterSecondCoverAvailable == 0); } + // Tests that vault withdrawals work correctly when the vault has unrealized + // loss from an impaired loan, ensuring the invariant check properly + // accounts for the loss. + void + testWithdrawReflectsUnrealizedLoss() + { + using namespace jtx; + using namespace loan; + using namespace std::chrono_literals; + + testcase("Vault withdraw reflects sfLossUnrealized"); + + // Test constants + static constexpr std::int64_t INITIAL_FUNDING = 1'000'000; + static constexpr std::int64_t LENDER_INITIAL_IOU = 5'000'000; + static constexpr std::int64_t DEPOSITOR_INITIAL_IOU = 1'000'000; + static constexpr std::int64_t BORROWER_INITIAL_IOU = 100'000; + static constexpr std::int64_t DEPOSIT_AMOUNT = 5'000; + static constexpr std::int64_t PRINCIPAL_AMOUNT = 99; + static constexpr std::uint64_t EXPECTED_SHARES_PER_DEPOSITOR = 5'000'000'000; + static constexpr std::uint32_t PAYMENT_INTERVAL = 600; + static constexpr std::uint32_t PAYMENT_TOTAL = 2; + + Env env(*this, all); + + // Setup accounts + Account const issuer{"issuer"}; + Account const lender{"lender"}; + Account const depositorA{"lpA"}; + Account const depositorB{"lpB"}; + Account const borrower{"borrowerA"}; + + env.fund(XRP(INITIAL_FUNDING), issuer, lender, depositorA, depositorB, borrower); + env.close(); + + // Setup trust lines + PrettyAsset const iouAsset = issuer[iouCurrency]; + env(trust(lender, iouAsset(10'000'000))); + env(trust(depositorA, iouAsset(10'000'000))); + env(trust(depositorB, iouAsset(10'000'000))); + env(trust(borrower, iouAsset(10'000'000))); + env.close(); + + // Fund accounts with IOUs + env(pay(issuer, lender, iouAsset(LENDER_INITIAL_IOU))); + env(pay(issuer, depositorA, iouAsset(DEPOSITOR_INITIAL_IOU))); + env(pay(issuer, depositorB, iouAsset(DEPOSITOR_INITIAL_IOU))); + env(pay(issuer, borrower, iouAsset(BORROWER_INITIAL_IOU))); + env.close(); + + // Create vault and broker, then add deposits from two depositors + auto const broker = createVaultAndBroker(env, iouAsset, lender); + Vault v{env}; + + env(v.deposit({ + .depositor = depositorA, + .id = broker.vaultKeylet().key, + .amount = iouAsset(DEPOSIT_AMOUNT), + }), + ter(tesSUCCESS)); + env(v.deposit({ + .depositor = depositorB, + .id = broker.vaultKeylet().key, + .amount = iouAsset(DEPOSIT_AMOUNT), + }), + ter(tesSUCCESS)); + env.close(); + + // Create a loan + auto const sleBroker = env.le(keylet::loanbroker(broker.brokerID)); + if (!BEAST_EXPECT(sleBroker)) + return; + + auto const loanKeylet = keylet::loan(broker.brokerID, sleBroker->at(sfLoanSequence)); + + env(set(borrower, broker.brokerID, PRINCIPAL_AMOUNT), + sig(sfCounterpartySignature, lender), + paymentTotal(PAYMENT_TOTAL), + paymentInterval(PAYMENT_INTERVAL), + fee(env.current()->fees().base * 2), + ter(tesSUCCESS)); + env.close(); + + // Impair the loan to create unrealized loss + env(manage(lender, loanKeylet.key, tfLoanImpair), ter(tesSUCCESS)); + env.close(); + + // Verify unrealized loss is recorded in the vault + auto const vaultAfterImpair = env.le(broker.vaultKeylet()); + if (!BEAST_EXPECT(vaultAfterImpair)) + return; + + BEAST_EXPECT( + vaultAfterImpair->at(sfLossUnrealized) == broker.asset(PRINCIPAL_AMOUNT).value()); + + // Helper to get share balance for a depositor + auto const shareAsset = vaultAfterImpair->at(sfShareMPTID); + auto const getShareBalance = [&](Account const& depositor) -> std::uint64_t { + auto const token = env.le(keylet::mptoken(shareAsset, depositor.id())); + return token ? token->getFieldU64(sfMPTAmount) : 0; + }; + + // Verify both depositors have equal shares + auto const sharesLpA = getShareBalance(depositorA); + auto const sharesLpB = getShareBalance(depositorB); + BEAST_EXPECT(sharesLpA == EXPECTED_SHARES_PER_DEPOSITOR); + BEAST_EXPECT(sharesLpB == EXPECTED_SHARES_PER_DEPOSITOR); + BEAST_EXPECT(sharesLpA == sharesLpB); + + // Helper to attempt withdrawal + auto const attemptWithdrawShares = [&](Account const& depositor, + std::uint64_t shareAmount, + TER expected) { + STAmount const shareAmt{MPTIssue{shareAsset}, Number(shareAmount)}; + env(v.withdraw( + {.depositor = depositor, .id = broker.vaultKeylet().key, .amount = shareAmt}), + ter(expected)); + env.close(); + }; + + // Regression test: Both depositors should successfully withdraw despite + // unrealized loss. Previously failed with invariant violation: + // "withdrawal must change vault and destination balance by equal + // amount". This was caused by sharesToAssetsWithdraw rounding down, + // creating a mismatch where vaultDeltaAssets * -1 != destinationDelta + // when unrealized loss exists. + attemptWithdrawShares(depositorA, sharesLpA, tesSUCCESS); + attemptWithdrawShares(depositorB, sharesLpB, tesSUCCESS); + } + public: void run() override { -#if LOANTODO +#if LOAN_TODO testLoanPayLateFullPaymentBypassesPenalties(); testLoanCoverMinimumRoundingExploit(); #endif + testWithdrawReflectsUnrealizedLoss(); testInvalidLoanSet(); testCoverDepositWithdrawNonTransferableMPT();