From b195011effe54b703abc91e0a69a00607efb2c40 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Fri, 14 Nov 2025 17:30:56 +0000 Subject: [PATCH] fix: Apply object reserve for Vault pseudo-account (#5954) --- src/test/app/Vault_test.cpp | 18 ++++++++----- src/test/jtx/impl/vault.cpp | 1 - src/xrpld/app/tx/detail/VaultCreate.cpp | 12 +++------ src/xrpld/app/tx/detail/VaultCreate.h | 3 --- src/xrpld/app/tx/detail/VaultDelete.cpp | 34 +++++++++++++++++++++++-- 5 files changed, 47 insertions(+), 21 deletions(-) diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 3488ccf458..a46168c774 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -1348,7 +1348,7 @@ class Vault_test : public beast::unit_test::suite Vault& vault) { auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); testcase("insufficient fee"); - env(tx, fee(env.current()->fees().base), ter(telINSUF_FEE_P)); + env(tx, fee(env.current()->fees().base - 1), ter(telINSUF_FEE_P)); }); testCase([this]( @@ -2093,6 +2093,10 @@ class Vault_test : public beast::unit_test::suite auto const sleMPT = env.le(mptoken); BEAST_EXPECT(sleMPT == nullptr); + // Use one reserve so the next transaction fails + env(ticket::create(owner, 1)); + env.close(); + // No reserve to create MPToken for asset in VaultWithdraw tx = vault.withdraw( {.depositor = owner, @@ -2110,7 +2114,7 @@ class Vault_test : public beast::unit_test::suite } }, {.requireAuth = false, - .initialXRP = acctReserve + incReserve * 4 - 1}); + .initialXRP = acctReserve + incReserve * 4 + 1}); testCase([this]( Env& env, @@ -2999,6 +3003,9 @@ class Vault_test : public beast::unit_test::suite env.le(keylet::line(owner, asset.raw().get())); BEAST_EXPECT(trustline == nullptr); + env(ticket::create(owner, 1)); + env.close(); + // Fail because not enough reserve to create trust line tx = vault.withdraw( {.depositor = owner, @@ -3014,7 +3021,7 @@ class Vault_test : public beast::unit_test::suite env(tx); env.close(); }, - CaseArgs{.initialXRP = acctReserve + incReserve * 4 - 1}); + CaseArgs{.initialXRP = acctReserve + incReserve * 4 + 1}); testCase( [&, this]( @@ -3035,8 +3042,7 @@ class Vault_test : public beast::unit_test::suite env(pay(owner, charlie, asset(100))); env.close(); - // Use up some reserve on tickets - env(ticket::create(charlie, 2)); + env(ticket::create(charlie, 3)); env.close(); // Fail because not enough reserve to create MPToken for shares @@ -3054,7 +3060,7 @@ class Vault_test : public beast::unit_test::suite env(tx); env.close(); }, - CaseArgs{.initialXRP = acctReserve + incReserve * 4 - 1}); + CaseArgs{.initialXRP = acctReserve + incReserve * 4 + 1}); testCase([&, this]( Env& env, diff --git a/src/test/jtx/impl/vault.cpp b/src/test/jtx/impl/vault.cpp index 663c42c6ee..13c8578146 100644 --- a/src/test/jtx/impl/vault.cpp +++ b/src/test/jtx/impl/vault.cpp @@ -38,7 +38,6 @@ Vault::create(CreateArgs const& args) jv[jss::TransactionType] = jss::VaultCreate; jv[jss::Account] = args.owner.human(); jv[jss::Asset] = to_json(args.asset); - jv[jss::Fee] = STAmount(env.current()->fees().increment).getJson(); if (args.flags) jv[jss::Flags] = *args.flags; return {jv, keylet}; diff --git a/src/xrpld/app/tx/detail/VaultCreate.cpp b/src/xrpld/app/tx/detail/VaultCreate.cpp index 9447976a32..bd761528d5 100644 --- a/src/xrpld/app/tx/detail/VaultCreate.cpp +++ b/src/xrpld/app/tx/detail/VaultCreate.cpp @@ -98,13 +98,6 @@ VaultCreate::preflight(PreflightContext const& ctx) return tesSUCCESS; } -XRPAmount -VaultCreate::calculateBaseFee(ReadView const& view, STTx const& tx) -{ - // One reserve increment is typically much greater than one base fee. - return calculateOwnerReserveFee(view, tx); -} - TER VaultCreate::preclaim(PreclaimContext const& ctx) { @@ -161,8 +154,9 @@ VaultCreate::doApply() if (auto ter = dirLink(view(), account_, vault)) return ter; - adjustOwnerCount(view(), owner, 1, j_); - auto ownerCount = owner->at(sfOwnerCount); + // We will create Vault and PseudoAccount, hence increase OwnerCount by 2 + adjustOwnerCount(view(), owner, 2, j_); + auto const ownerCount = owner->at(sfOwnerCount); if (mPriorBalance < view().fees().accountReserve(ownerCount)) return tecINSUFFICIENT_RESERVE; diff --git a/src/xrpld/app/tx/detail/VaultCreate.h b/src/xrpld/app/tx/detail/VaultCreate.h index 3f952d540a..6ebab2f5f5 100644 --- a/src/xrpld/app/tx/detail/VaultCreate.h +++ b/src/xrpld/app/tx/detail/VaultCreate.h @@ -42,9 +42,6 @@ public: static NotTEC preflight(PreflightContext const& ctx); - static XRPAmount - calculateBaseFee(ReadView const& view, STTx const& tx); - static TER preclaim(PreclaimContext const& ctx); diff --git a/src/xrpld/app/tx/detail/VaultDelete.cpp b/src/xrpld/app/tx/detail/VaultDelete.cpp index ab7db78956..da8500c513 100644 --- a/src/xrpld/app/tx/detail/VaultDelete.cpp +++ b/src/xrpld/app/tx/detail/VaultDelete.cpp @@ -165,7 +165,35 @@ VaultDelete::doApply() return tecHAS_OBLIGATIONS; // LCOV_EXCL_LINE // Destroy the pseudo-account. - view().erase(view().peek(keylet::account(pseudoID))); + auto vaultPseudoSLE = view().peek(keylet::account(pseudoID)); + if (!vaultPseudoSLE || vaultPseudoSLE->at(~sfVaultID) != vault->key()) + return tefBAD_LEDGER; // LCOV_EXCL_LINE + + // Making the payment and removing the empty holding should have deleted any + // obligations associated with the vault or vault pseudo-account. + if (*vaultPseudoSLE->at(sfBalance)) + { + // LCOV_EXCL_START + JLOG(j_.error()) << "VaultDelete: pseudo-account has a balance"; + return tecHAS_OBLIGATIONS; + // LCOV_EXCL_STOP + } + if (vaultPseudoSLE->at(sfOwnerCount) != 0) + { + // LCOV_EXCL_START + JLOG(j_.error()) << "VaultDelete: pseudo-account still owns objects"; + return tecHAS_OBLIGATIONS; + // LCOV_EXCL_STOP + } + if (view().exists(keylet::ownerDir(pseudoID))) + { + // LCOV_EXCL_START + JLOG(j_.error()) << "VaultDelete: pseudo-account has a directory"; + return tecHAS_OBLIGATIONS; + // LCOV_EXCL_STOP + } + + view().erase(vaultPseudoSLE); // Remove the vault from its owner's directory. auto const ownerID = vault->at(sfOwner); @@ -189,7 +217,9 @@ VaultDelete::doApply() return tefBAD_LEDGER; // LCOV_EXCL_STOP } - adjustOwnerCount(view(), owner, -1, j_); + + // We are destroying Vault and PseudoAccount, hence decrease by 2 + adjustOwnerCount(view(), owner, -2, j_); // Destroy the vault. view().erase(vault);