Merge remote-tracking branch 'XRPLF/develop' into ximinez/lending-refactoring-1

* XRPLF/develop:
  Add `Scale` to SingleAssetVault (5652)

include/xrpl/basics/Number.h

@@ -150,6 +150,24 @@ public:
         return (mantissa_ < 0) ? -1 : (mantissa_ ? 1 : 0);
     }
 
+    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;
+    }
+
     friend constexpr bool
     operator>(Number const& x, Number const& y) noexcept
     {

include/xrpl/protocol/Protocol.h

@@ -121,6 +121,13 @@ std::size_t constexpr maxDataPayloadLength = 256;
 /** Vault withdrawal policies */
 std::uint8_t constexpr vaultStrategyFirstComeFirstServe = 1;
 
+/** Default IOU scale factor for a Vault */
+std::uint8_t constexpr vaultDefaultIOUScale = 6;
+/** Maximum scale factor for a Vault. The number is chosen to ensure that
+1 IOU can be always converted to shares.
+10^19 > maxMPTokenAmount (2^64-1) > 10^18 */
+std::uint8_t constexpr vaultMaximumIOUScale = 18;
+
 /** Maximum recursion depth for vault shares being put as an asset inside
  * another vault; counted from 0 */
 std::uint8_t constexpr maxAssetCheckDepth = 5;

include/xrpl/protocol/detail/ledger_entries.macro

@@ -499,6 +499,7 @@ LEDGER_ENTRY(ltVAULT, 0x0084, Vault, vault, ({
     {sfLossUnrealized,       soeREQUIRED},
     {sfShareMPTID,           soeREQUIRED},
     {sfWithdrawalPolicy,     soeREQUIRED},
+    {sfScale,                soeDEFAULT},
     // no SharesTotal ever (use MPTIssuance.sfOutstandingAmount)
     // no PermissionedDomainID ever (use MPTIssuance.sfDomainID)
 }))

include/xrpl/protocol/detail/transactions.macro

@@ -798,6 +798,7 @@ TRANSACTION(ttVAULT_CREATE, 65, VaultCreate,
     {sfDomainID, soeOPTIONAL},
     {sfWithdrawalPolicy, soeOPTIONAL},
     {sfData, soeOPTIONAL},
+    {sfScale, soeOPTIONAL},
 }))
 
 /** This transaction updates a single asset vault. */

src/test/basics/Number_test.cpp

@@ -720,6 +720,30 @@ public:
         BEAST_EXPECT(res2 == STAmount{7518784});
     }
 
+    void
+    test_truncate()
+    {
+        BEAST_EXPECT(Number(25, +1).truncate() == Number(250, 0));
+        BEAST_EXPECT(Number(25, 0).truncate() == Number(25, 0));
+        BEAST_EXPECT(Number(25, -1).truncate() == Number(2, 0));
+        BEAST_EXPECT(Number(25, -2).truncate() == Number(0, 0));
+        BEAST_EXPECT(Number(99, -2).truncate() == Number(0, 0));
+
+        BEAST_EXPECT(Number(-25, +1).truncate() == Number(-250, 0));
+        BEAST_EXPECT(Number(-25, 0).truncate() == Number(-25, 0));
+        BEAST_EXPECT(Number(-25, -1).truncate() == Number(-2, 0));
+        BEAST_EXPECT(Number(-25, -2).truncate() == Number(0, 0));
+        BEAST_EXPECT(Number(-99, -2).truncate() == Number(0, 0));
+
+        BEAST_EXPECT(Number(0, 0).truncate() == Number(0, 0));
+        BEAST_EXPECT(Number(0, 30000).truncate() == Number(0, 0));
+        BEAST_EXPECT(Number(0, -30000).truncate() == Number(0, 0));
+        BEAST_EXPECT(Number(100, -30000).truncate() == Number(0, 0));
+        BEAST_EXPECT(Number(100, -30000).truncate() == Number(0, 0));
+        BEAST_EXPECT(Number(-100, -30000).truncate() == Number(0, 0));
+        BEAST_EXPECT(Number(-100, -30000).truncate() == Number(0, 0));
+    }
+
     void
     run() override
     {
@@ -740,6 +764,7 @@ public:
         test_stream();
         test_inc_dec();
         test_toSTAmount();
+        test_truncate();
     }
 };
 

src/test/jtx/Account.h

@@ -74,6 +74,10 @@ public:
 
     /** @} */
 
+    /** Create an Account from an account ID. Should only be used when the
+     * secret key is unavailable, such as for pseudo-accounts. */
+    explicit Account(std::string name, AccountID const& id);
+
     enum AcctStringType { base58Seed, other };
     /** Create an account from a base58 seed string.  Throws on invalid seed. */
     Account(AcctStringType stringType, std::string base58SeedStr);

src/test/jtx/impl/Account.cpp

@@ -86,6 +86,14 @@ Account::Account(AcctStringType stringType, std::string base58SeedStr)
 {
 }
 
+Account::Account(std::string name, AccountID const& id)
+    : Account(name, randomKeyPair(KeyType::secp256k1), privateCtorTag{})
+{
+    // override the randomly generated values
+    id_ = id;
+    human_ = toBase58(id_);
+}
+
 IOU
 Account::operator[](std::string const& s) const
 {

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

@@ -1553,6 +1553,12 @@ ValidMPTIssuance::finalize(
                 "not escrow finish tx");
             return true;
         }
+
+        if ((tx.getTxnType() == ttVAULT_CLAWBACK ||
+             tx.getTxnType() == ttVAULT_WITHDRAW) &&
+            mptokensDeleted_ == 1 && mptokensCreated_ == 0 &&
+            mptIssuancesCreated_ == 0 && mptIssuancesDeleted_ == 0)
+            return true;
     }
 
     if (mptIssuancesCreated_ != 0)

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

@@ -21,8 +21,10 @@
 #include <xrpld/ledger/View.h>
 
 #include <xrpl/beast/utility/instrumentation.h>
+#include <xrpl/protocol/AccountID.h>
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/MPTIssue.h>
+#include <xrpl/protocol/SField.h>
 #include <xrpl/protocol/STAmount.h>
 #include <xrpl/protocol/STNumber.h>
 #include <xrpl/protocol/TER.h>
@@ -151,7 +153,7 @@ VaultClawback::doApply()
     if (!vault)
         return tefINTERNAL;  // LCOV_EXCL_LINE
 
-    auto const mptIssuanceID = (*vault)[sfShareMPTID];
+    auto const mptIssuanceID = *((*vault)[sfShareMPTID]);
     auto const sleIssuance = view().read(keylet::mptIssuance(mptIssuanceID));
     if (!sleIssuance)
     {
@@ -161,68 +163,169 @@ VaultClawback::doApply()
         // LCOV_EXCL_STOP
     }
 
-    Asset const asset = vault->at(sfAsset);
+    Asset const vaultAsset = vault->at(sfAsset);
     STAmount const amount = [&]() -> STAmount {
         auto const maybeAmount = tx[~sfAmount];
         if (maybeAmount)
             return *maybeAmount;
-        return {sfAmount, asset, 0};
+        return {sfAmount, vaultAsset, 0};
     }();
     XRPL_ASSERT(
-        amount.asset() == asset,
+        amount.asset() == vaultAsset,
         "ripple::VaultClawback::doApply : matching asset");
 
+    auto assetsAvailable = vault->at(sfAssetsAvailable);
+    auto assetsTotal = vault->at(sfAssetsTotal);
+    [[maybe_unused]] auto const lossUnrealized = vault->at(sfLossUnrealized);
+    XRPL_ASSERT(
+        lossUnrealized <= (assetsTotal - assetsAvailable),
+        "ripple::VaultClawback::doApply : loss and assets do balance");
+
     AccountID holder = tx[sfHolder];
-    STAmount assets, shares;
+    MPTIssue const share{mptIssuanceID};
-    if (amount == beast::zero)
+    STAmount sharesDestroyed = {share};
+    STAmount assetsRecovered;
+    try
     {
-        Asset share = *(*vault)[sfShareMPTID];
+        if (amount == beast::zero)
-        shares = accountHolds(
+        {
-            view(),
+            sharesDestroyed = accountHolds(
-            holder,
+                view(),
-            share,
+                holder,
-            FreezeHandling::fhIGNORE_FREEZE,
+                share,
-            AuthHandling::ahIGNORE_AUTH,
+                FreezeHandling::fhIGNORE_FREEZE,
-            j_);
+                AuthHandling::ahIGNORE_AUTH,
-        assets = sharesToAssetsWithdraw(vault, sleIssuance, shares);
+                j_);
+
+            auto const maybeAssets =
+                sharesToAssetsWithdraw(vault, sleIssuance, sharesDestroyed);
+            if (!maybeAssets)
+                return tecINTERNAL;  // LCOV_EXCL_LINE
+            assetsRecovered = *maybeAssets;
+        }
+        else
+        {
+            assetsRecovered = amount;
+            {
+                auto const maybeShares =
+                    assetsToSharesWithdraw(vault, sleIssuance, assetsRecovered);
+                if (!maybeShares)
+                    return tecINTERNAL;  // LCOV_EXCL_LINE
+                sharesDestroyed = *maybeShares;
+            }
+
+            auto const maybeAssets =
+                sharesToAssetsWithdraw(vault, sleIssuance, sharesDestroyed);
+            if (!maybeAssets)
+                return tecINTERNAL;  // LCOV_EXCL_LINE
+            assetsRecovered = *maybeAssets;
+        }
+
+        // Clamp to maximum.
+        if (assetsRecovered > *assetsAvailable)
+        {
+            assetsRecovered = *assetsAvailable;
+            // Note, it is important to truncate the number of shares, otherwise
+            // the corresponding assets might breach the AssetsAvailable
+            {
+                auto const maybeShares = assetsToSharesWithdraw(
+                    vault, sleIssuance, assetsRecovered, TruncateShares::yes);
+                if (!maybeShares)
+                    return tecINTERNAL;  // LCOV_EXCL_LINE
+                sharesDestroyed = *maybeShares;
+            }
+
+            auto const maybeAssets =
+                sharesToAssetsWithdraw(vault, sleIssuance, sharesDestroyed);
+            if (!maybeAssets)
+                return tecINTERNAL;  // LCOV_EXCL_LINE
+            assetsRecovered = *maybeAssets;
+            if (assetsRecovered > *assetsAvailable)
+            {
+                // LCOV_EXCL_START
+                JLOG(j_.error())
+                    << "VaultClawback: invalid rounding of shares.";
+                return tecINTERNAL;
+                // LCOV_EXCL_STOP
+            }
+        }
     }
-    else
+    catch (std::overflow_error const&)
     {
-        assets = amount;
+        // It's easy to hit this exception from Number with large enough Scale
-        shares = assetsToSharesWithdraw(vault, sleIssuance, assets);
+        // so we avoid spamming the log and only use debug here.
+        JLOG(j_.debug())  //
+            << "VaultClawback: overflow error with"
+            << " scale=" << (int)vault->at(sfScale).value()  //
+            << ", assetsTotal=" << vault->at(sfAssetsTotal).value()
+            << ", sharesTotal=" << sleIssuance->at(sfOutstandingAmount)
+            << ", amount=" << amount.value();
+        return tecPATH_DRY;
     }
 
-    // Clamp to maximum.
+    if (sharesDestroyed == beast::zero)
-    Number maxAssets = *vault->at(sfAssetsAvailable);
+        return tecPRECISION_LOSS;
-    if (assets > maxAssets)
-    {
-        assets = maxAssets;
-        shares = assetsToSharesWithdraw(vault, sleIssuance, assets);
-    }
 
-    if (shares == beast::zero)
+    assetsTotal -= assetsRecovered;
-        return tecINSUFFICIENT_FUNDS;
+    assetsAvailable -= assetsRecovered;
-
-    vault->at(sfAssetsTotal) -= assets;
-    vault->at(sfAssetsAvailable) -= assets;
     view().update(vault);
 
     auto const& vaultAccount = vault->at(sfAccount);
     // Transfer shares from holder to vault.
-    if (auto ter = accountSend(
+    if (auto const ter = accountSend(
-            view(), holder, vaultAccount, shares, j_, WaiveTransferFee::Yes))
+            view(),
+            holder,
+            vaultAccount,
+            sharesDestroyed,
+            j_,
+            WaiveTransferFee::Yes);
+        !isTesSuccess(ter))
         return ter;
 
+    // Try to remove MPToken for shares, if the holder balance is zero. Vault
+    // pseudo-account will never set lsfMPTAuthorized, so we ignore flags.
+    // Keep MPToken if holder is the vault owner.
+    if (holder != vault->at(sfOwner))
+    {
+        if (auto const ter =
+                removeEmptyHolding(view(), holder, sharesDestroyed.asset(), j_);
+            isTesSuccess(ter))
+        {
+            JLOG(j_.debug())  //
+                << "VaultClawback: removed empty MPToken for vault shares"
+                << " MPTID=" << to_string(mptIssuanceID)  //
+                << " account=" << toBase58(holder);
+        }
+        else if (ter != tecHAS_OBLIGATIONS)
+        {
+            // LCOV_EXCL_START
+            JLOG(j_.error())  //
+                << "VaultClawback: failed to remove MPToken for vault shares"
+                << " MPTID=" << to_string(mptIssuanceID)  //
+                << " account=" << toBase58(holder)        //
+                << " with result: " << transToken(ter);
+            return ter;
+            // LCOV_EXCL_STOP
+        }
+        // else quietly ignore, holder balance is not zero
+    }
+
     // Transfer assets from vault to issuer.
-    if (auto ter = accountSend(
+    if (auto const ter = accountSend(
-            view(), vaultAccount, account_, assets, j_, WaiveTransferFee::Yes))
+            view(),
+            vaultAccount,
+            account_,
+            assetsRecovered,
+            j_,
+            WaiveTransferFee::Yes);
+        !isTesSuccess(ter))
         return ter;
 
     // Sanity check
     if (accountHolds(
             view(),
             vaultAccount,
-            assets.asset(),
+            assetsRecovered.asset(),
             FreezeHandling::fhIGNORE_FREEZE,
             AuthHandling::ahIGNORE_AUTH,
             j_) < beast::zero)

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

@@ -25,8 +25,10 @@
 #include <xrpl/protocol/Asset.h>
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Indexes.h>
+#include <xrpl/protocol/Issue.h>
 #include <xrpl/protocol/MPTIssue.h>
 #include <xrpl/protocol/Protocol.h>
+#include <xrpl/protocol/SField.h>
 #include <xrpl/protocol/STNumber.h>
 #include <xrpl/protocol/TER.h>
 #include <xrpl/protocol/TxFlags.h>
@@ -84,6 +86,16 @@ VaultCreate::preflight(PreflightContext const& ctx)
             return temMALFORMED;
     }
 
+    if (auto const scale = ctx.tx[~sfScale])
+    {
+        auto const vaultAsset = ctx.tx[sfAsset];
+        if (vaultAsset.holds<MPTIssue>() || vaultAsset.native())
+            return temMALFORMED;
+
+        if (scale > vaultMaximumIOUScale)
+            return temMALFORMED;
+    }
+
     return preflight2(ctx);
 }
 
@@ -97,8 +109,8 @@ VaultCreate::calculateBaseFee(ReadView const& view, STTx const& tx)
 TER
 VaultCreate::preclaim(PreclaimContext const& ctx)
 {
-    auto vaultAsset = ctx.tx[sfAsset];
+    auto const vaultAsset = ctx.tx[sfAsset];
-    auto account = ctx.tx[sfAccount];
+    auto const account = ctx.tx[sfAccount];
 
     if (vaultAsset.native())
         ;  // No special checks for XRP
@@ -148,7 +160,7 @@ VaultCreate::preclaim(PreclaimContext const& ctx)
             return tecOBJECT_NOT_FOUND;
     }
 
-    auto sequence = ctx.tx.getSeqValue();
+    auto const sequence = ctx.tx.getSeqValue();
     if (auto const accountId = pseudoAccountAddress(
             ctx.view, keylet::vault(account, sequence).key);
         accountId == beast::zero)
@@ -165,8 +177,8 @@ VaultCreate::doApply()
     // we can consider downgrading them to `tef` or `tem`.
 
     auto const& tx = ctx_.tx;
-    auto sequence = tx.getSeqValue();
+    auto const sequence = tx.getSeqValue();
-    auto owner = view().peek(keylet::account(account_));
+    auto const owner = view().peek(keylet::account(account_));
     if (owner == nullptr)
         return tefINTERNAL;  // LCOV_EXCL_LINE
 
@@ -190,6 +202,10 @@ VaultCreate::doApply()
         !isTesSuccess(ter))
         return ter;
 
+    std::uint8_t const scale = (asset.holds<MPTIssue>() || asset.native())
+        ? 0
+        : ctx_.tx[~sfScale].value_or(vaultDefaultIOUScale);
+
     auto txFlags = tx.getFlags();
     std::uint32_t mptFlags = 0;
     if ((txFlags & tfVaultShareNonTransferable) == 0)
@@ -209,12 +225,13 @@ VaultCreate::doApply()
             .account = pseudoId->value(),
             .sequence = 1,
             .flags = mptFlags,
+            .assetScale = scale,
             .metadata = tx[~sfMPTokenMetadata],
             .domainId = tx[~sfDomainID],
         });
     if (!maybeShare)
         return maybeShare.error();  // LCOV_EXCL_LINE
-    auto& share = *maybeShare;
+    auto const& mptIssuanceID = *maybeShare;
 
     vault->setFieldIssue(sfAsset, STIssue{sfAsset, asset});
     vault->at(sfFlags) = txFlags & tfVaultPrivate;
@@ -227,7 +244,7 @@ VaultCreate::doApply()
     // Leave default values for AssetTotal and AssetAvailable, both zero.
     if (auto value = tx[~sfAssetsMaximum])
         vault->at(sfAssetsMaximum) = *value;
-    vault->at(sfShareMPTID) = share;
+    vault->at(sfShareMPTID) = mptIssuanceID;
     if (auto value = tx[~sfData])
         vault->at(sfData) = *value;
     // Required field, default to vaultStrategyFirstComeFirstServe
@@ -235,9 +252,31 @@ VaultCreate::doApply()
         vault->at(sfWithdrawalPolicy) = *value;
     else
         vault->at(sfWithdrawalPolicy) = vaultStrategyFirstComeFirstServe;
-    // No `LossUnrealized`.
+    if (scale)
+        vault->at(sfScale) = scale;
     view().insert(vault);
 
+    // Explicitly create MPToken for the vault owner
+    if (auto const err = authorizeMPToken(
+            view(), mPriorBalance, mptIssuanceID, account_, ctx_.journal);
+        !isTesSuccess(err))
+        return err;
+
+    // If the vault is private, set the authorized flag for the vault owner
+    if (txFlags & tfVaultPrivate)
+    {
+        if (auto const err = authorizeMPToken(
+                view(),
+                mPriorBalance,
+                mptIssuanceID,
+                pseudoId,
+                ctx_.journal,
+                {},
+                account_);
+            !isTesSuccess(err))
+            return err;
+    }
+
     return tesSUCCESS;
 }
 

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

@@ -21,6 +21,7 @@
 #include <xrpld/ledger/View.h>
 
 #include <xrpl/protocol/Feature.h>
+#include <xrpl/protocol/MPTIssue.h>
 #include <xrpl/protocol/STNumber.h>
 #include <xrpl/protocol/TER.h>
 #include <xrpl/protocol/TxFlags.h>
@@ -128,7 +129,8 @@ VaultDelete::doApply()
 
     // Destroy the share issuance. Do not use MPTokenIssuanceDestroy for this,
     // no special logic needed. First run few checks, duplicated from preclaim.
-    auto const mpt = view().peek(keylet::mptIssuance(vault->at(sfShareMPTID)));
+    auto const shareMPTID = *vault->at(sfShareMPTID);
+    auto const mpt = view().peek(keylet::mptIssuance(shareMPTID));
     if (!mpt)
     {
         // LCOV_EXCL_START
@@ -137,6 +139,24 @@ VaultDelete::doApply()
         // LCOV_EXCL_STOP
     }
 
+    // Try to remove MPToken for vault shares for the vault owner if it exists.
+    if (auto const mptoken = view().peek(keylet::mptoken(shareMPTID, account_)))
+    {
+        if (auto const ter =
+                removeEmptyHolding(view(), account_, MPTIssue(shareMPTID), j_);
+            !isTesSuccess(ter))
+        {
+            // LCOV_EXCL_START
+            JLOG(j_.error())  //
+                << "VaultDelete: failed to remove vault owner's MPToken"
+                << " MPTID=" << to_string(shareMPTID)  //
+                << " account=" << toBase58(account_)   //
+                << " with result: " << transToken(ter);
+            return ter;
+            // LCOV_EXCL_STOP
+        }
+    }
+
     if (!view().dirRemove(
             keylet::ownerDir(pseudoID), (*mpt)[sfOwnerNode], mpt->key(), false))
     {

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

@@ -26,6 +26,7 @@
 #include <xrpl/protocol/Indexes.h>
 #include <xrpl/protocol/LedgerFormats.h>
 #include <xrpl/protocol/MPTIssue.h>
+#include <xrpl/protocol/SField.h>
 #include <xrpl/protocol/STNumber.h>
 #include <xrpl/protocol/TER.h>
 #include <xrpl/protocol/TxFlags.h>
@@ -138,7 +139,7 @@ VaultDeposit::preclaim(PreclaimContext const& ctx)
     if (isFrozen(ctx.view, account, vaultShare))
         return tecLOCKED;
 
-    if (vault->isFlag(tfVaultPrivate) && account != vault->at(sfOwner))
+    if (vault->isFlag(lsfVaultPrivate) && account != vault->at(sfOwner))
     {
         auto const maybeDomainID = sleIssuance->at(~sfDomainID);
         // Since this is a private vault and the account is not its owner, we
@@ -183,7 +184,7 @@ VaultDeposit::doApply()
     if (!vault)
         return tefINTERNAL;  // LCOV_EXCL_LINE
 
-    auto const assets = ctx_.tx[sfAmount];
+    auto const amount = ctx_.tx[sfAmount];
     // Make sure the depositor can hold shares.
     auto const mptIssuanceID = (*vault)[sfShareMPTID];
     auto const sleIssuance = view().read(keylet::mptIssuance(mptIssuanceID));
@@ -197,14 +198,14 @@ VaultDeposit::doApply()
 
     auto const& vaultAccount = vault->at(sfAccount);
     // Note, vault owner is always authorized
-    if ((vault->getFlags() & tfVaultPrivate) && account_ != vault->at(sfOwner))
+    if (vault->isFlag(lsfVaultPrivate) && account_ != vault->at(sfOwner))
     {
         if (auto const err = enforceMPTokenAuthorization(
                 ctx_.view(), mptIssuanceID, account_, mPriorBalance, j_);
             !isTesSuccess(err))
             return err;
     }
-    else
+    else  // !vault->isFlag(lsfVaultPrivate) || account_ == vault->at(sfOwner)
     {
         // No authorization needed, but must ensure there is MPToken
         auto sleMpt = view().read(keylet::mptoken(mptIssuanceID, account_));
@@ -221,8 +222,12 @@ VaultDeposit::doApply()
         }
 
         // If the vault is private, set the authorized flag for the vault owner
-        if (vault->isFlag(tfVaultPrivate))
+        if (vault->isFlag(lsfVaultPrivate))
         {
+            // This follows from the reverse of the outer enclosing if condition
+            XRPL_ASSERT(
+                account_ == vault->at(sfOwner),
+                "ripple::VaultDeposit::doApply : account is owner");
             if (auto const err = authorizeMPToken(
                     view(),
                     mPriorBalance,              // priorBalance
@@ -237,14 +242,52 @@ VaultDeposit::doApply()
         }
     }
 
-    // Compute exchange before transferring any amounts.
+    STAmount sharesCreated = {vault->at(sfShareMPTID)}, assetsDeposited;
-    auto const shares = assetsToSharesDeposit(vault, sleIssuance, assets);
+    try
+    {
+        // Compute exchange before transferring any amounts.
+        {
+            auto const maybeShares =
+                assetsToSharesDeposit(vault, sleIssuance, amount);
+            if (!maybeShares)
+                return tecINTERNAL;  // LCOV_EXCL_LINE
+            sharesCreated = *maybeShares;
+        }
+        if (sharesCreated == beast::zero)
+            return tecPRECISION_LOSS;
+
+        auto const maybeAssets =
+            sharesToAssetsDeposit(vault, sleIssuance, sharesCreated);
+        if (!maybeAssets)
+            return tecINTERNAL;  // LCOV_EXCL_LINE
+        else if (*maybeAssets > amount)
+        {
+            // LCOV_EXCL_START
+            JLOG(j_.error()) << "VaultDeposit: would take more than offered.";
+            return tecINTERNAL;
+            // LCOV_EXCL_STOP
+        }
+        assetsDeposited = *maybeAssets;
+    }
+    catch (std::overflow_error const&)
+    {
+        // It's easy to hit this exception from Number with large enough Scale
+        // so we avoid spamming the log and only use debug here.
+        JLOG(j_.debug())  //
+            << "VaultDeposit: overflow error with"
+            << " scale=" << (int)vault->at(sfScale).value()  //
+            << ", assetsTotal=" << vault->at(sfAssetsTotal).value()
+            << ", sharesTotal=" << sleIssuance->at(sfOutstandingAmount)
+            << ", amount=" << amount;
+        return tecPATH_DRY;
+    }
+
     XRPL_ASSERT(
-        shares.asset() != assets.asset(),
+        sharesCreated.asset() != assetsDeposited.asset(),
         "ripple::VaultDeposit::doApply : assets are not shares");
 
-    vault->at(sfAssetsTotal) += assets;
+    vault->at(sfAssetsTotal) += assetsDeposited;
-    vault->at(sfAssetsAvailable) += assets;
+    vault->at(sfAssetsAvailable) += assetsDeposited;
     view().update(vault);
 
     // A deposit must not push the vault over its limit.
@@ -253,15 +296,21 @@ VaultDeposit::doApply()
         return tecLIMIT_EXCEEDED;
 
     // Transfer assets from depositor to vault.
-    if (auto ter = accountSend(
+    if (auto const ter = accountSend(
-            view(), account_, vaultAccount, assets, j_, WaiveTransferFee::Yes))
+            view(),
+            account_,
+            vaultAccount,
+            assetsDeposited,
+            j_,
+            WaiveTransferFee::Yes);
+        !isTesSuccess(ter))
         return ter;
 
     // Sanity check
     if (accountHolds(
             view(),
             account_,
-            assets.asset(),
+            assetsDeposited.asset(),
             FreezeHandling::fhIGNORE_FREEZE,
             AuthHandling::ahIGNORE_AUTH,
             j_) < beast::zero)
@@ -273,8 +322,14 @@ VaultDeposit::doApply()
     }
 
     // Transfer shares from vault to depositor.
-    if (auto ter = accountSend(
+    if (auto const ter = accountSend(
-            view(), vaultAccount, account_, shares, j_, WaiveTransferFee::Yes))
+            view(),
+            vaultAccount,
+            account_,
+            sharesCreated,
+            j_,
+            WaiveTransferFee::Yes);
+        !isTesSuccess(ter))
         return ter;
 
     return tesSUCCESS;

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

@@ -108,7 +108,7 @@ VaultSet::preclaim(PreclaimContext const& ctx)
     if (auto const domain = ctx.tx[~sfDomainID])
     {
         // We can only set domain if private flag was originally set
-        if ((vault->getFlags() & tfVaultPrivate) == 0)
+        if (!vault->isFlag(lsfVaultPrivate))
         {
             JLOG(ctx.j.debug()) << "VaultSet: vault is not private";
             return tecNO_PERMISSION;
@@ -175,9 +175,9 @@ VaultSet::doApply()
     {
         if (*domainId != beast::zero)
         {
-            // In VaultSet::preclaim we enforce that tfVaultPrivate must have
+            // In VaultSet::preclaim we enforce that lsfVaultPrivate must have
             // been set in the vault. We currently do not support making such a
-            // vault public (i.e. removal of tfVaultPrivate flag). The
+            // vault public (i.e. removal of lsfVaultPrivate flag). The
             // sfDomainID flag must be set in the MPTokenIssuance object and can
             // be freely updated.
             sleIssuance->setFieldH256(sfDomainID, *domainId);

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

@@ -177,7 +177,7 @@ VaultWithdraw::doApply()
     if (!vault)
         return tefINTERNAL;  // LCOV_EXCL_LINE
 
-    auto const mptIssuanceID = (*vault)[sfShareMPTID];
+    auto const mptIssuanceID = *((*vault)[sfShareMPTID]);
     auto const sleIssuance = view().read(keylet::mptIssuance(mptIssuanceID));
     if (!sleIssuance)
     {
@@ -192,24 +192,57 @@ VaultWithdraw::doApply()
     // to deposit into it, and this means you are also indefinitely authorized
     // to withdraw from it.
 
-    auto amount = ctx_.tx[sfAmount];
+    auto const amount = ctx_.tx[sfAmount];
-    auto const asset = vault->at(sfAsset);
+    Asset const vaultAsset = vault->at(sfAsset);
-    auto const share = MPTIssue(mptIssuanceID);
+    MPTIssue const share{mptIssuanceID};
-    STAmount shares, assets;
+    STAmount sharesRedeemed = {share};
-    if (amount.asset() == asset)
+    STAmount assetsWithdrawn;
+    try
     {
-        // Fixed assets, variable shares.
+        if (amount.asset() == vaultAsset)
-        assets = amount;
+        {
-        shares = assetsToSharesWithdraw(vault, sleIssuance, assets);
+            // Fixed assets, variable shares.
+            {
+                auto const maybeShares =
+                    assetsToSharesWithdraw(vault, sleIssuance, amount);
+                if (!maybeShares)
+                    return tecINTERNAL;  // LCOV_EXCL_LINE
+                sharesRedeemed = *maybeShares;
+            }
+
+            if (sharesRedeemed == beast::zero)
+                return tecPRECISION_LOSS;
+            auto const maybeAssets =
+                sharesToAssetsWithdraw(vault, sleIssuance, sharesRedeemed);
+            if (!maybeAssets)
+                return tecINTERNAL;  // LCOV_EXCL_LINE
+            assetsWithdrawn = *maybeAssets;
+        }
+        else if (amount.asset() == share)
+        {
+            // Fixed shares, variable assets.
+            sharesRedeemed = amount;
+            auto const maybeAssets =
+                sharesToAssetsWithdraw(vault, sleIssuance, sharesRedeemed);
+            if (!maybeAssets)
+                return tecINTERNAL;  // LCOV_EXCL_LINE
+            assetsWithdrawn = *maybeAssets;
+        }
+        else
+            return tefINTERNAL;  // LCOV_EXCL_LINE
     }
-    else if (amount.asset() == share)
+    catch (std::overflow_error const&)
     {
-        // Fixed shares, variable assets.
+        // It's easy to hit this exception from Number with large enough Scale
-        shares = amount;
+        // so we avoid spamming the log and only use debug here.
-        assets = sharesToAssetsWithdraw(vault, sleIssuance, shares);
+        JLOG(j_.debug())  //
+            << "VaultWithdraw: overflow error with"
+            << " scale=" << (int)vault->at(sfScale).value()  //
+            << ", assetsTotal=" << vault->at(sfAssetsTotal).value()
+            << ", sharesTotal=" << sleIssuance->at(sfOutstandingAmount)
+            << ", amount=" << amount.value();
+        return tecPATH_DRY;
     }
-    else
-        return tefINTERNAL;  // LCOV_EXCL_LINE
 
     if (accountHolds(
             view(),
@@ -217,31 +250,72 @@ VaultWithdraw::doApply()
             share,
             FreezeHandling::fhZERO_IF_FROZEN,
             AuthHandling::ahIGNORE_AUTH,
-            j_) < shares)
+            j_) < sharesRedeemed)
     {
         JLOG(j_.debug()) << "VaultWithdraw: account doesn't hold enough shares";
         return tecINSUFFICIENT_FUNDS;
     }
 
-    // The vault must have enough assets on hand. The vault may hold assets that
+    auto assetsAvailable = vault->at(sfAssetsAvailable);
-    // it has already pledged. That is why we look at AssetAvailable instead of
+    auto assetsTotal = vault->at(sfAssetsTotal);
-    // the pseudo-account balance.
+    [[maybe_unused]] auto const lossUnrealized = vault->at(sfLossUnrealized);
-    if (*vault->at(sfAssetsAvailable) < assets)
+    XRPL_ASSERT(
+        lossUnrealized <= (assetsTotal - assetsAvailable),
+        "ripple::VaultWithdraw::doApply : loss and assets do balance");
+
+    // The vault must have enough assets on hand. The vault may hold assets
+    // that it has already pledged. That is why we look at AssetAvailable
+    // instead of the pseudo-account balance.
+    if (*assetsAvailable < assetsWithdrawn)
     {
         JLOG(j_.debug()) << "VaultWithdraw: vault doesn't hold enough assets";
         return tecINSUFFICIENT_FUNDS;
     }
 
-    vault->at(sfAssetsTotal) -= assets;
+    assetsTotal -= assetsWithdrawn;
-    vault->at(sfAssetsAvailable) -= assets;
+    assetsAvailable -= assetsWithdrawn;
     view().update(vault);
 
     auto const& vaultAccount = vault->at(sfAccount);
     // Transfer shares from depositor to vault.
-    if (auto ter = accountSend(
+    if (auto const ter = accountSend(
-            view(), account_, vaultAccount, shares, j_, WaiveTransferFee::Yes))
+            view(),
+            account_,
+            vaultAccount,
+            sharesRedeemed,
+            j_,
+            WaiveTransferFee::Yes);
+        !isTesSuccess(ter))
         return ter;
 
+    // Try to remove MPToken for shares, if the account balance is zero. Vault
+    // pseudo-account will never set lsfMPTAuthorized, so we ignore flags.
+    // Keep MPToken if holder is the vault owner.
+    if (account_ != vault->at(sfOwner))
+    {
+        if (auto const ter = removeEmptyHolding(
+                view(), account_, sharesRedeemed.asset(), j_);
+            isTesSuccess(ter))
+        {
+            JLOG(j_.debug())  //
+                << "VaultWithdraw: removed empty MPToken for vault shares"
+                << " MPTID=" << to_string(mptIssuanceID)  //
+                << " account=" << toBase58(account_);
+        }
+        else if (ter != tecHAS_OBLIGATIONS)
+        {
+            // LCOV_EXCL_START
+            JLOG(j_.error())  //
+                << "VaultWithdraw: failed to remove MPToken for vault shares"
+                << " MPTID=" << to_string(mptIssuanceID)  //
+                << " account=" << toBase58(account_)      //
+                << " with result: " << transToken(ter);
+            return ter;
+            // LCOV_EXCL_STOP
+        }
+        // else quietly ignore, account balance is not zero
+    }
+
     auto const dstAcct = [&]() -> AccountID {
         if (ctx_.tx.isFieldPresent(sfDestination))
             return ctx_.tx.getAccountID(sfDestination);
@@ -249,15 +323,21 @@ VaultWithdraw::doApply()
     }();
 
     // Transfer assets from vault to depositor or destination account.
-    if (auto ter = accountSend(
+    if (auto const ter = accountSend(
-            view(), vaultAccount, dstAcct, assets, j_, WaiveTransferFee::Yes))
+            view(),
+            vaultAccount,
+            dstAcct,
+            assetsWithdrawn,
+            j_,
+            WaiveTransferFee::Yes);
+        !isTesSuccess(ter))
         return ter;
 
     // Sanity check
     if (accountHolds(
             view(),
             vaultAccount,
-            assets.asset(),
+            assetsWithdrawn.asset(),
             FreezeHandling::fhIGNORE_FREEZE,
             AuthHandling::ahIGNORE_AUTH,
             j_) < beast::zero)

src/xrpld/ledger/View.h

@@ -928,28 +928,41 @@ deleteAMMTrustLine(
     std::optional<AccountID> const& ammAccountID,
     beast::Journal j);
 
-// From the perspective of a vault,
+// From the perspective of a vault, return the number of shares to give the
-// return the number of shares to give the depositor
+// depositor when they deposit a fixed amount of assets. Since shares are MPT
-// when they deposit a fixed amount of assets.
+// this number is integral and always truncated in this calculation.
-[[nodiscard]] STAmount
+[[nodiscard]] std::optional<STAmount>
 assetsToSharesDeposit(
     std::shared_ptr<SLE const> const& vault,
     std::shared_ptr<SLE const> const& issuance,
     STAmount const& assets);
 
-// From the perspective of a vault,
+// From the perspective of a vault, return the number of assets to take from
-// return the number of shares to demand from the depositor
+// depositor when they receive a fixed amount of shares. Note, since shares are
-// when they ask to withdraw a fixed amount of assets.
+// MPT, they are always an integral number.
-[[nodiscard]] STAmount
+[[nodiscard]] std::optional<STAmount>
+sharesToAssetsDeposit(
+    std::shared_ptr<SLE const> const& vault,
+    std::shared_ptr<SLE const> const& issuance,
+    STAmount const& shares);
+
+enum class TruncateShares : bool { no = false, yes = true };
+
+// From the perspective of a vault, return the number of shares to demand from
+// the depositor when they ask to withdraw a fixed amount of assets. Since
+// shares are MPT this number is integral, and it will be rounded to nearest
+// unless explicitly requested to be truncated instead.
+[[nodiscard]] std::optional<STAmount>
 assetsToSharesWithdraw(
     std::shared_ptr<SLE const> const& vault,
     std::shared_ptr<SLE const> const& issuance,
-    STAmount const& assets);
+    STAmount const& assets,
+    TruncateShares truncate = TruncateShares::no);
 
-// From the perspective of a vault,
+// From the perspective of a vault, return the number of assets to give the
-// return the number of assets to give the depositor
+// depositor when they redeem a fixed amount of shares. Note, since shares are
-// when they redeem a fixed amount of shares.
+// MPT, they are always an integral number.
-[[nodiscard]] STAmount
+[[nodiscard]] std::optional<STAmount>
 sharesToAssetsWithdraw(
     std::shared_ptr<SLE const> const& vault,
     std::shared_ptr<SLE const> const& issuance,

src/xrpld/ledger/detail/View.cpp

@@ -2854,58 +2854,113 @@ rippleCredit(
         saAmount.asset().value());
 }
 
-[[nodiscard]] STAmount
+[[nodiscard]] std::optional<STAmount>
 assetsToSharesDeposit(
     std::shared_ptr<SLE const> const& vault,
     std::shared_ptr<SLE const> const& issuance,
     STAmount const& assets)
 {
+    XRPL_ASSERT(
+        !assets.negative(),
+        "ripple::assetsToSharesDeposit : non-negative assets");
     XRPL_ASSERT(
         assets.asset() == vault->at(sfAsset),
         "ripple::assetsToSharesDeposit : assets and vault match");
-    Number assetTotal = vault->at(sfAssetsTotal);
+    if (assets.negative() || assets.asset() != vault->at(sfAsset))
-    STAmount shares{vault->at(sfShareMPTID), static_cast<Number>(assets)};
+        return std::nullopt;  // LCOV_EXCL_LINE
+
+    Number const assetTotal = vault->at(sfAssetsTotal);
+    STAmount shares{vault->at(sfShareMPTID)};
     if (assetTotal == 0)
-        return shares;
+        return STAmount{
-    Number shareTotal = issuance->at(sfOutstandingAmount);
+            shares.asset(),
-    shares = shareTotal * (assets / assetTotal);
+            Number(assets.mantissa(), assets.exponent() + vault->at(sfScale))
+                .truncate()};
+
+    Number const shareTotal = issuance->at(sfOutstandingAmount);
+    shares = (shareTotal * (assets / assetTotal)).truncate();
     return shares;
 }
 
-[[nodiscard]] STAmount
+[[nodiscard]] std::optional<STAmount>
+sharesToAssetsDeposit(
+    std::shared_ptr<SLE const> const& vault,
+    std::shared_ptr<SLE const> const& issuance,
+    STAmount const& shares)
+{
+    XRPL_ASSERT(
+        !shares.negative(),
+        "ripple::sharesToAssetsDeposit : non-negative shares");
+    XRPL_ASSERT(
+        shares.asset() == vault->at(sfShareMPTID),
+        "ripple::sharesToAssetsDeposit : shares and vault match");
+    if (shares.negative() || shares.asset() != vault->at(sfShareMPTID))
+        return std::nullopt;  // LCOV_EXCL_LINE
+
+    Number const assetTotal = vault->at(sfAssetsTotal);
+    STAmount assets{vault->at(sfAsset)};
+    if (assetTotal == 0)
+        return STAmount{
+            assets.asset(),
+            shares.mantissa(),
+            shares.exponent() - vault->at(sfScale),
+            false};
+
+    Number const shareTotal = issuance->at(sfOutstandingAmount);
+    assets = assetTotal * (shares / shareTotal);
+    return assets;
+}
+
+[[nodiscard]] std::optional<STAmount>
 assetsToSharesWithdraw(
     std::shared_ptr<SLE const> const& vault,
     std::shared_ptr<SLE const> const& issuance,
-    STAmount const& assets)
+    STAmount const& assets,
+    TruncateShares truncate)
 {
+    XRPL_ASSERT(
+        !assets.negative(),
+        "ripple::assetsToSharesDeposit : non-negative assets");
     XRPL_ASSERT(
         assets.asset() == vault->at(sfAsset),
         "ripple::assetsToSharesWithdraw : assets and vault match");
+    if (assets.negative() || assets.asset() != vault->at(sfAsset))
+        return std::nullopt;  // LCOV_EXCL_LINE
+
     Number assetTotal = vault->at(sfAssetsTotal);
     assetTotal -= vault->at(sfLossUnrealized);
     STAmount shares{vault->at(sfShareMPTID)};
     if (assetTotal == 0)
         return shares;
-    Number shareTotal = issuance->at(sfOutstandingAmount);
+    Number const shareTotal = issuance->at(sfOutstandingAmount);
-    shares = shareTotal * (assets / assetTotal);
+    Number result = shareTotal * (assets / assetTotal);
+    if (truncate == TruncateShares::yes)
+        result = result.truncate();
+    shares = result;
     return shares;
 }
 
-[[nodiscard]] STAmount
+[[nodiscard]] std::optional<STAmount>
 sharesToAssetsWithdraw(
     std::shared_ptr<SLE const> const& vault,
     std::shared_ptr<SLE const> const& issuance,
     STAmount const& shares)
 {
+    XRPL_ASSERT(
+        !shares.negative(),
+        "ripple::sharesToAssetsDeposit : non-negative shares");
     XRPL_ASSERT(
         shares.asset() == vault->at(sfShareMPTID),
         "ripple::sharesToAssetsWithdraw : shares and vault match");
+    if (shares.negative() || shares.asset() != vault->at(sfShareMPTID))
+        return std::nullopt;  // LCOV_EXCL_LINE
+
     Number assetTotal = vault->at(sfAssetsTotal);
     assetTotal -= vault->at(sfLossUnrealized);
     STAmount assets{vault->at(sfAsset)};
     if (assetTotal == 0)
         return assets;
-    Number shareTotal = issuance->at(sfOutstandingAmount);
+    Number const shareTotal = issuance->at(sfOutstandingAmount);
     assets = assetTotal * (shares / shareTotal);
     return assets;
 }