From a8914da2755f67a60913c84e1cad1816668c0905 Mon Sep 17 00:00:00 2001
From: Vito <5780819+Tapanito@users.noreply.github.com>
Date: Thu, 19 Mar 2026 17:48:08 +0100
Subject: [PATCH] refactor: document rounding behaviour, consolidate deposit
 NAV check

---
 include/xrpl/ledger/VaultHelpers.h  | 20 ++++++++++++++++++
 src/libxrpl/ledger/VaultHelpers.cpp | 32 +++++++++++++++++++----------
 2 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/include/xrpl/ledger/VaultHelpers.h b/include/xrpl/ledger/VaultHelpers.h
index 19c5a8f38e..0a72de1810 100644
--- a/include/xrpl/ledger/VaultHelpers.h
+++ b/include/xrpl/ledger/VaultHelpers.h
@@ -50,6 +50,26 @@ computeDeposit(
     STAmount const& assets,
     beast::Journal j);
 
+/** Compute a withdrawal given a fixed asset amount.
+ *
+ *  Converts assets → shares → assets (round-trip) to determine the
+ *  exact shares to redeem and assets to return.
+ *
+ *  The intermediate shares value is stored as an MPT (integer), which
+ *  uses banker's rounding (round-to-nearest, even on tie). When shares
+ *  round up, the back-calculated assets may exceed the requested amount.
+ *
+ *  Example (scale=1, vault=87.5 IOU, 875 shares):
+ *    assetsToSharesWithdraw(3.75) → round(37.5) = 38 shares
+ *    sharesToAssetsWithdraw(38)   → 3.8 IOU  (> 3.75 requested)
+ *
+ *  This matches v1 behaviour. See XLS-0065 §3.1.7.1.
+ *
+ *  @return ExchangeResult{assets, shares} on success.
+ *  @return tecPRECISION_LOSS if computed shares are zero.
+ *  @return tecPATH_DRY on arithmetic overflow.
+ *  @return tecINTERNAL on invalid input or vault state.
+ */
 [[nodiscard]] Expected<ExchangeResult, TER>
 computeWithdrawByAssets(
     Rules const& rules,
diff --git a/src/libxrpl/ledger/VaultHelpers.cpp b/src/libxrpl/ledger/VaultHelpers.cpp
index 6a232db4e9..5e1bca2448 100644
--- a/src/libxrpl/ledger/VaultHelpers.cpp
+++ b/src/libxrpl/ledger/VaultHelpers.cpp
@@ -10,6 +10,10 @@ namespace xrpl::vault {
 
 namespace detail {
 
+// Deposit uses NAV = assetsTotal - interestUnrealized (excludes loss).
+// Withdraw uses NAV = assetsTotal - interestUnrealized - lossUnrealized.
+// See XLS-0065 §3.1.7.1 "Share Valuation" for rationale.
+
 STAmount
 assetsToSharesDeposit(SLE::const_ref vault, SLE::const_ref issuance, STAmount const& assets)
 {
@@ -100,9 +104,6 @@ sharesToAssetsWithdraw(SLE::const_ref vault, SLE::const_ref issuance, STAmount c
 
 }  // namespace detail
 
-// v1 math is intentionally not factored out like v2. Since Single Asset Vault
-// is already released, refactoring v1 risks introducing behavioral changes in
-// production code that we cannot gate behind an amendment.
 namespace {
 namespace v1 {
 
@@ -181,10 +182,22 @@ validateVaultState(SLE::const_ref vault, SLE::const_ref issuance, beast::Journal
 
     Number const interestUnrealized = vault->at(sfInterestUnrealized);
     Number const lossUnrealized = vault->at(sfLossUnrealized);
-    Number const nav = assetTotal - interestUnrealized - lossUnrealized;
-    if (nav < 0)
+
+    // Deposit NAV excludes loss; withdrawal NAV excludes both.
+    // Deposit NAV <= 0 means all vault value is unrealized interest, which should be impossible.
+    Number const depositNAV = assetTotal - interestUnrealized;
+    if (depositNAV <= 0)
     {
-        JLOG(j.error()) << "vault state: NAV < 0"
+        JLOG(j.error()) << "vault state: deposit NAV <= 0"
+                        << " (assetsTotal=" << assetTotal
+                        << ", interestUnrealized=" << interestUnrealized << ")";
+        return tecINTERNAL;
+    }
+
+    Number const withdrawNAV = depositNAV - lossUnrealized;
+    if (withdrawNAV < 0)
+    {
+        JLOG(j.error()) << "vault state: withdrawal NAV < 0"
                         << " (assetsTotal=" << assetTotal
                         << ", interestUnrealized=" << interestUnrealized
                         << ", lossUnrealized=" << lossUnrealized << ")";
@@ -192,19 +205,16 @@ validateVaultState(SLE::const_ref vault, SLE::const_ref issuance, beast::Journal
     }
 
     Number const shareTotal = issuance->at(sfOutstandingAmount);
-    if (nav > 0 && shareTotal <= 0)
+    if (withdrawNAV > 0 && shareTotal <= 0)
     {
         JLOG(j.error()) << "vault state: no outstanding shares with positive NAV"
-                        << " (NAV=" << nav << ", shareTotal=" << shareTotal << ")";
+                        << " (NAV=" << withdrawNAV << ", shareTotal=" << shareTotal << ")";
         return tecINTERNAL;
     }
 
     return tesSUCCESS;
 }
 
-// Dispatch to v1 or v2 math based on amendment.
-// v1 is frozen — no refactoring to preserve existing behaviour.
-
 STAmount
 assetsToSharesDeposit(
     Rules const& rules,