Refactor: checking Insufficient reserve to use helper function

include/xrpl/ledger/View.h

@@ -451,6 +451,17 @@ areCompatible(
     beast::Journal::Stream& s,
     char const* reason);
 
+uint32_t
+ownerCount(std::shared_ptr<SLE const> const& accountSle);
+
+TER
+checkInsufficientReserve(
+    ReadView const& view,
+    std::shared_ptr<SLE const> const& accSle,
+    STAmount const& accBalance,
+    std::int32_t ownerCountDelta,
+    std::int32_t accountCountDelta = 0);
+
 //------------------------------------------------------------------------------
 //
 // Modifiers

src/libxrpl/ledger/View.cpp

@@ -1019,6 +1019,29 @@ hashOfSeq(ReadView const& ledger, LedgerIndex seq, beast::Journal journal)
     return std::nullopt;
 }
 
+uint32_t
+ownerCount(std::shared_ptr<SLE const> const& accountSle)
+{
+    return accountSle->getFieldU32(sfOwnerCount);
+}
+
+TER
+checkInsufficientReserve(
+    ReadView const& view,
+    std::shared_ptr<SLE const> const& accSle,
+    STAmount const& accBalance,
+    std::int32_t ownerCountDelta,
+    std::int32_t accountCountDelta)
+{
+    STAmount const reserve{
+        view.fees().accountReserve(ownerCount(accSle) + ownerCountDelta)};
+
+    if (accBalance < reserve)
+        return tecINSUFFICIENT_RESERVE;
+
+    return tesSUCCESS;
+}
+
 //------------------------------------------------------------------------------
 //
 // Modifiers
@@ -1328,13 +1351,13 @@ authorizeMPToken(
         // an account owns, in the case of MPTokens we only
         // *enforce* a reserve if the user owns more than two
         // items. This is similar to the reserve requirements of trust lines.
-        std::uint32_t const uOwnerCount = sleAcct->getFieldU32(sfOwnerCount);
-        XRPAmount const reserveCreate(
-            (uOwnerCount < 2) ? XRPAmount(beast::zero)
-                              : view.fees().accountReserve(uOwnerCount + 1));
-
-        if (priorBalance < reserveCreate)
-            return tecINSUFFICIENT_RESERVE;
+        if (ownerCount(sleAcct) >= 2)
+        {
+            if (auto const ret =
+                    checkInsufficientReserve(view, sleAcct, priorBalance, 1);
+                !isTesSuccess(ret))
+                return ret;
+        }
 
         auto const mptokenKey = keylet::mptoken(mptIssuanceID, account);
         auto mptoken = std::make_shared<SLE>(mptokenKey);

src/test/app/MPToken_test.cpp

@@ -156,6 +156,13 @@ class MPToken_test : public beast::unit_test::suite
                  .metadata = "test",
                  .err = temMALFORMED});
         }
+        // test reserve check of MPTokenIssuanceCreate
+        {
+            Env env{*this, features};
+            env.fund(env.current()->fees().accountReserve(1) - drops(1), alice);
+            MPTTester mptAlice(env, alice, {.fund = false});
+            mptAlice.create({.err = tecINSUFFICIENT_RESERVE});
+        }
     }
 
     void

src/test/app/PayChan_test.cpp

@@ -2321,6 +2321,38 @@ struct PayChan_test : public beast::unit_test::suite
         BEAST_EXPECT(env.seq(bob) == bobSeq);
     }
 
+    void
+    testReserveCheck(FeatureBitset features)
+    {
+        testcase("Reserve check");
+        using namespace jtx;
+        using namespace std::literals::chrono_literals;
+        Env env(*this, features);
+
+        auto const acctReserveForOne = env.current()->fees().accountReserve(1);
+
+        Account const alice{"alice"};
+        Account const bob{"bob"};
+        env.fund(XRP(10000), bob);
+        env.close();
+
+        env.fund(acctReserveForOne - drops(1), alice);
+        env.close();
+
+        BEAST_EXPECT(env.balance(alice) < acctReserveForOne);
+        env(create(alice, bob, XRP(1), 100s, alice.pk()),
+            ter(tecINSUFFICIENT_RESERVE));
+        env.close();
+
+        env(pay(env.master, alice, XRP(1)));
+        env.close();
+
+        BEAST_EXPECT(env.balance(alice) > acctReserveForOne);
+        BEAST_EXPECT(env.balance(alice) < acctReserveForOne + XRP(1));
+        env(create(alice, bob, XRP(1), 100s, alice.pk()), ter(tecUNFUNDED));
+        env.close();
+    }
+
     void
     testWithFeats(FeatureBitset features)
     {
@@ -2345,6 +2377,7 @@ struct PayChan_test : public beast::unit_test::suite
         testMetaAndOwnership(features);
         testAccountDelete(features);
         testUsingTickets(features);
+        testReserveCheck(features);
     }
 
 public:

src/test/app/XChain_test.cpp

@@ -2614,18 +2614,9 @@ struct XChain_test : public beast::unit_test::suite,
 
         testcase("Add Non Batch Account Create Attestation");
 
-        XEnv mcEnv(*this);
-        XEnv scEnv(*this, true);
-
-        XRPAmount tx_fee = mcEnv.txFee();
-
         Account a{"a"};
         Account doorA{"doorA"};
 
-        STAmount funds{XRP(10000)};
-        mcEnv.fund(funds, a);
-        mcEnv.fund(funds, doorA);
-
         Account ua{"ua"};  // unfunded account we want to create
 
         BridgeDef xrp_b{
@@ -2639,47 +2630,88 @@ struct XChain_test : public beast::unit_test::suite,
             signers,
             Json::nullValue};
 
-        xrp_b.initBridge(mcEnv, scEnv);
-
-        auto const amt = XRP(777);
-        auto const amt_plus_reward = amt + xrp_b.reward;
         {
-            Balance bal_doorA(mcEnv, doorA);
-            Balance bal_a(mcEnv, a);
+            XEnv mcEnv(*this);
+            XEnv scEnv(*this, true);
 
-            mcEnv
-                .tx(sidechain_xchain_account_create(
-                    a, xrp_b.jvb, ua, amt, xrp_b.reward))
-                .close();
+            XRPAmount tx_fee = mcEnv.txFee();
 
-            BEAST_EXPECT(bal_doorA.diff() == amt_plus_reward);
-            BEAST_EXPECT(bal_a.diff() == -(amt_plus_reward + tx_fee));
-        }
+            STAmount funds{XRP(10000)};
+            mcEnv.fund(funds, a);
+            mcEnv.fund(funds, doorA);
+
+            xrp_b.initBridge(mcEnv, scEnv);
+
+            auto const amt = XRP(777);
+            auto const amt_plus_reward = amt + xrp_b.reward;
+            {
+                Balance bal_doorA(mcEnv, doorA);
+                Balance bal_a(mcEnv, a);
+
+                mcEnv
+                    .tx(sidechain_xchain_account_create(
+                        a, xrp_b.jvb, ua, amt, xrp_b.reward))
+                    .close();
 
-        for (int i = 0; i < signers.size(); ++i)
+                BEAST_EXPECT(bal_doorA.diff() == amt_plus_reward);
+                BEAST_EXPECT(bal_a.diff() == -(amt_plus_reward + tx_fee));
+            }
+
+            for (int i = 0; i < signers.size(); ++i)
+            {
+                auto const att = create_account_attestation(
+                    signers[0].account,
+                    xrp_b.jvb,
+                    a,
+                    amt,
+                    xrp_b.reward,
+                    signers[i].account,
+                    true,
+                    1,
+                    ua,
+                    signers[i]);
+                TER const expectedTER = i < xrp_b.quorum
+                    ? tesSUCCESS
+                    : TER{tecXCHAIN_ACCOUNT_CREATE_PAST};
+
+                scEnv.tx(att, ter(expectedTER)).close();
+                if (i + 1 < xrp_b.quorum)
+                    BEAST_EXPECT(!scEnv.env_.le(ua));
+                else
+                    BEAST_EXPECT(scEnv.env_.le(ua));
+            }
+            BEAST_EXPECT(scEnv.env_.le(ua));
+        }
         {
+            XEnv mcEnv(*this);
+            XEnv scEnv(*this, true);
+
+            STAmount funds{XRP(10000)};
+            mcEnv.fund(funds, a);
+            auto const mcBaseFee = mcEnv.env_.current()->fees().base;
+            mcEnv.fund(
+                // for SignerList, Bridge: 2 increments
+                // for SetSignerList, BridgeCreate: 2 base fees
+                mcEnv.env_.current()->fees().accountReserve(3) + mcBaseFee * 2 -
+                    drops(1),
+                doorA);
+
+            xrp_b.initBridge(mcEnv, scEnv);
+
+            auto const amt = XRP(777);
             auto const att = create_account_attestation(
                 signers[0].account,
                 xrp_b.jvb,
                 a,
                 amt,
                 xrp_b.reward,
-                signers[i].account,
-                true,
+                signers[0].account,
+                false,
                 1,
                 ua,
-                signers[i]);
-            TER const expectedTER = i < xrp_b.quorum
-                ? tesSUCCESS
-                : TER{tecXCHAIN_ACCOUNT_CREATE_PAST};
-
-            scEnv.tx(att, ter(expectedTER)).close();
-            if (i + 1 < xrp_b.quorum)
-                BEAST_EXPECT(!scEnv.env_.le(ua));
-            else
-                BEAST_EXPECT(scEnv.env_.le(ua));
+                signers[0]);
+            mcEnv.tx(att, ter(tecINSUFFICIENT_RESERVE)).close();
         }
-        BEAST_EXPECT(scEnv.env_.le(ua));
     }
 
     void

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

@@ -590,15 +590,15 @@ AMMWithdraw::withdraw(
             if (!sleAccount)
                 return tecINTERNAL;  // LCOV_EXCL_LINE
             auto const balance = (*sleAccount)[sfBalance].xrp();
-            std::uint32_t const ownerCount = sleAccount->at(sfOwnerCount);
 
-            // See also SetTrust::doApply()
-            XRPAmount const reserve(
-                (ownerCount < 2) ? XRPAmount(beast::zero)
-                                 : view.fees().accountReserve(ownerCount + 1));
-
-            if (std::max(priorBalance, balance) < reserve)
-                return tecINSUFFICIENT_RESERVE;
+            std::uint32_t const count = ownerCount(sleAccount);
+            if (count >= 2)
+            {
+                if (auto const ret = checkInsufficientReserve(
+                        view, sleAccount, std::max(priorBalance, balance), 1);
+                    !isTesSuccess(ret))
+                    return ret;
+            }
         }
         return tesSUCCESS;
     };

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

@@ -164,13 +164,10 @@ CreateCheck::doApply()
     // A check counts against the reserve of the issuing account, but we
     // check the starting balance because we want to allow dipping into the
     // reserve to pay fees.
-    {
-        STAmount const reserve{
-            view().fees().accountReserve(sle->getFieldU32(sfOwnerCount) + 1)};
-
-        if (mPriorBalance < reserve)
-            return tecINSUFFICIENT_RESERVE;
-    }
+    if (auto const ret =
+            checkInsufficientReserve(view(), sle, mPriorBalance, 1);
+        !isTesSuccess(ret))
+        return ret;
 
     // Note that we use the value from the sequence or ticket as the
     // Check sequence.  For more explanation see comments in SeqProxy.h.

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

@@ -82,13 +82,10 @@ CreateTicket::doApply()
     // check the starting balance because we want to allow dipping into the
     // reserve to pay fees.
     std::uint32_t const ticketCount = ctx_.tx[sfTicketCount];
-    {
-        XRPAmount const reserve = view().fees().accountReserve(
-            sleAccountRoot->getFieldU32(sfOwnerCount) + ticketCount);
-
-        if (mPriorBalance < reserve)
-            return tecINSUFFICIENT_RESERVE;
-    }
+    if (auto const ret = checkInsufficientReserve(
+            view(), sleAccountRoot, mPriorBalance, ticketCount);
+        !isTesSuccess(ret))
+        return ret;
 
     beast::Journal viewJ{ctx_.app.journal("View")};
 

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

@@ -139,12 +139,10 @@ CredentialCreate::doApply()
     if (!sleIssuer)
         return tefINTERNAL;
 
-    {
-        STAmount const reserve{view().fees().accountReserve(
-            sleIssuer->getFieldU32(sfOwnerCount) + 1)};
-        if (mPriorBalance < reserve)
-            return tecINSUFFICIENT_RESERVE;
-    }
+    if (auto const ret =
+            checkInsufficientReserve(view(), sleIssuer, mPriorBalance, 1);
+        !isTesSuccess(ret))
+        return ret;
 
     sleCred->setAccountID(sfSubject, subject);
     sleCred->setAccountID(sfIssuer, account_);
@@ -344,12 +342,10 @@ CredentialAccept::doApply()
     if (!sleSubject || !sleIssuer)
         return tefINTERNAL;
 
-    {
-        STAmount const reserve{view().fees().accountReserve(
-            sleSubject->getFieldU32(sfOwnerCount) + 1)};
-        if (mPriorBalance < reserve)
-            return tecINSUFFICIENT_RESERVE;
-    }
+    if (auto const ret =
+            checkInsufficientReserve(view(), sleSubject, mPriorBalance, 1);
+        !isTesSuccess(ret))
+        return ret;
 
     auto const credType(ctx_.tx[sfCredentialType]);
     Keylet const credentialKey = keylet::credential(account_, issuer, credType);

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

@@ -79,14 +79,11 @@ addSLE(
         return tefINTERNAL;
 
     // Check reserve availability for new object creation
-    {
-        auto const balance = STAmount((*sleAccount)[sfBalance]).xrp();
-        auto const reserve =
-            ctx.view().fees().accountReserve((*sleAccount)[sfOwnerCount] + 1);
-
-        if (balance < reserve)
-            return tecINSUFFICIENT_RESERVE;
-    }
+    auto const balance = STAmount((*sleAccount)[sfBalance]).xrp();
+    if (auto const ret =
+            checkInsufficientReserve(ctx.view(), sleAccount, balance, 1);
+        !isTesSuccess(ret))
+        return ret;
 
     // Add ledger object to ledger
     ctx.view().insert(sle);

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

@@ -109,11 +109,10 @@ DelegateSet::doApply()
         return tesSUCCESS;
     }
 
-    STAmount const reserve{ctx_.view().fees().accountReserve(
-        sleOwner->getFieldU32(sfOwnerCount) + 1)};
-
-    if (mPriorBalance < reserve)
-        return tecINSUFFICIENT_RESERVE;
+    if (auto const ret =
+            checkInsufficientReserve(view(), sleOwner, mPriorBalance, 1);
+        !isTesSuccess(ret))
+        return ret;
 
     auto const& permissions = ctx_.tx.getFieldArray(sfPermissions);
     if (!permissions.empty())