Refactor: checking Insufficient reserve to use helper function

include/xrpl/ledger/View.h

@@ -550,6 +550,16 @@ 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);
+
 //------------------------------------------------------------------------------
 //
 // Modifiers

src/libxrpl/ledger/View.cpp

@@ -1124,6 +1124,28 @@ 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)
+{
+    STAmount const reserve{
+        view.fees().accountReserve(ownerCount(accSle) + ownerCountDelta)};
+
+    if (accBalance < reserve)
+        return tecINSUFFICIENT_RESERVE;
+
+    return tesSUCCESS;
+}
+
 //------------------------------------------------------------------------------
 //
 // Modifiers
@@ -1467,8 +1489,9 @@ addEmptyHolding(
         return tecDUPLICATE;
 
     // Can the account cover the trust line reserve ?
-    std::uint32_t const ownerCount = sleDst->at(sfOwnerCount);
-    if (priorBalance < view.fees().accountReserve(ownerCount + 1))
+    if (auto const ret =
+            checkInsufficientReserve(view, sleDst, priorBalance, 1);
+        !isTesSuccess(ret))
         return tecNO_LINE_INSUF_RESERVE;
 
     return trustCreate(
@@ -1562,13 +1585,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;
+        }
 
         // Defensive check before we attempt to create MPToken for the issuer
         auto const mpt = view.read(keylet::mptIssuance(mptIssuanceID));

src/test/app/MPToken_test.cpp

@@ -137,6 +137,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

@@ -2081,6 +2081,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 - drops(1));
+        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)
     {
@@ -2105,6 +2137,7 @@ struct PayChan_test : public beast::unit_test::suite
         testMetaAndOwnership(features);
         testAccountDelete(features);
         testUsingTickets(features);
+        testReserveCheck(features);
     }
 
 public:

src/test/app/XChain_test.cpp

@@ -2595,18 +2595,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{
@@ -2620,47 +2611,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/AMMCreate.cpp

@@ -117,14 +117,17 @@ AMMCreate::preclaim(PreclaimContext const& ctx)
     }
 
     // Check the reserve for LPToken trustline
-    STAmount const xrpBalance = xrpLiquid(ctx.view, accountID, 1, ctx.j);
     // Insufficient reserve
-    if (xrpBalance <= beast::zero)
+    auto const accountSle = ctx.view.read(keylet::account(accountID));
+    if (auto const ret = checkInsufficientReserve(
+            ctx.view, accountSle, accountSle->getFieldAmount(sfBalance), 1);
+        !isTesSuccess(ret))
     {
         JLOG(ctx.j.debug()) << "AMM Instance: insufficient reserves";
         return tecINSUF_RESERVE_LINE;
     }
 
+    STAmount const xrpBalance = xrpLiquid(ctx.view, accountID, 1, ctx.j);
     auto insufficientBalance = [&](STAmount const& asset) {
         if (isXRP(asset))
             return xrpBalance < asset;

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

@@ -207,7 +207,13 @@ AMMDeposit::preclaim(PreclaimContext const& ctx)
             // Adjust the reserve if LP doesn't have LPToken trustline
             auto const sle = ctx.view.read(
                 keylet::line(accountID, lpIssue.account, lpIssue.currency));
-            if (xrpLiquid(ctx.view, accountID, !sle, ctx.j) >= deposit)
+            auto const accountSle = ctx.view.read(keylet::account(accountID));
+            if (auto const ret = checkInsufficientReserve(
+                    ctx.view,
+                    accountSle,
+                    accountSle->getFieldAmount(sfBalance) - deposit,
+                    !sle);
+                isTesSuccess(ret))
                 return TER(tesSUCCESS);
             if (sle)
                 return tecUNFUNDED_AMM;
@@ -335,9 +341,11 @@ AMMDeposit::preclaim(PreclaimContext const& ctx)
     // We checked above but need to check again if depositing IOU only.
     if (ammLPHolds(ctx.view, *ammSle, accountID, ctx.j) == beast::zero)
     {
-        STAmount const xrpBalance = xrpLiquid(ctx.view, accountID, 1, ctx.j);
+        auto const accountSle = ctx.view.read(keylet::account(accountID));
         // Insufficient reserve
-        if (xrpBalance <= beast::zero)
+        if (auto const ret = checkInsufficientReserve(
+                ctx.view, accountSle, accountSle->getFieldAmount(sfBalance), 1);
+            !isTesSuccess(ret))
         {
             JLOG(ctx.j.debug()) << "AMM Instance: insufficient reserves";
             return tecINSUF_RESERVE_LINE;

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

@@ -573,15 +573,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/CashCheck.cpp

@@ -329,8 +329,9 @@ CashCheck::doApply()
                 auto const sleDst = psb.peek(keylet::account(account_));
 
                 // Can the account cover the trust line's reserve?
-                if (std::uint32_t const ownerCount = {sleDst->at(sfOwnerCount)};
-                    mPriorBalance < psb.fees().accountReserve(ownerCount + 1))
+                if (auto const ret =
+                        checkInsufficientReserve(psb, sleDst, mPriorBalance, 1);
+                    !isTesSuccess(ret))
                 {
                     JLOG(j_.trace()) << "Trust line does not exist. "
                                         "Insufficent reserve to create line.";

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

@@ -144,13 +144,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

@@ -63,13 +63,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")};