Increase test coverage

Builds/CMake/RippledCore.cmake

@@ -806,6 +806,7 @@ if (tests)
     src/test/app/AMM_test.cpp
     src/test/app/AMMCalc_test.cpp
     src/test/app/AMMExtended_test.cpp
+    src/test/app/AMMWithdraw_test.cpp
     src/test/app/Check_test.cpp
     src/test/app/Clawback_test.cpp
     src/test/app/CrossingLimits_test.cpp

src/ripple/app/misc/impl/AMMUtils.cpp

@@ -60,9 +60,12 @@ ammHolds(
                     *optIssue2,
                     std::make_optional(std::make_pair(issue1, issue2))))
             {
+                // This error can only be hit if the AMM is corrupted
+                // LCOV_EXCL_START
                 JLOG(j.debug()) << "ammHolds: Invalid optIssue1 or optIssue2 "
                                 << *optIssue1 << " " << *optIssue2;
                 return std::nullopt;
+                // LCOV_EXCL_STOP
             }
             return std::make_optional(std::make_pair(*optIssue1, *optIssue2));
         }
@@ -74,17 +77,22 @@ ammHolds(
                 return std::make_optional(std::make_pair(issue1, issue2));
             else if (checkIssue == issue2)
                 return std::make_optional(std::make_pair(issue2, issue1));
+            // This error can only be hit if the AMM is corrupted
+            // LCOV_EXCL_START
             JLOG(j.debug())
                 << "ammHolds: Invalid " << label << " " << checkIssue;
             return std::nullopt;
+            // LCOV_EXCL_STOP
         };
         if (optIssue1)
         {
             return singleIssue(*optIssue1, "optIssue1");
         }
         else if (optIssue2)
         {
-            return singleIssue(*optIssue2, "optIssue2");
+            // This line cannot be hit since you cannot have Amount2
+            // without Amount
+            return singleIssue(*optIssue2, "optIssue2");  // LCOV_EXCL_LINE
         }
         return std::make_optional(std::make_pair(issue1, issue2));
     }();
@@ -210,19 +218,23 @@ deleteAMMTrustLines(
             // Should only have the trustlines
             if (nodeType != LedgerEntryType::ltRIPPLE_STATE)
             {
+                // LCOV_EXCL_START
                 JLOG(j.error())
                     << "deleteAMMTrustLines: deleting non-trustline "
                     << nodeType;
                 return {tecINTERNAL, SkipEntry::No};
+                // LCOV_EXCL_STOP
             }
 
             // Trustlines must have zero balance
             if (sleItem->getFieldAmount(sfBalance) != beast::zero)
             {
+                // LCOV_EXCL_START
                 JLOG(j.error())
                     << "deleteAMMTrustLines: deleting trustline with "
                        "non-zero balance.";
                 return {tecINTERNAL, SkipEntry::No};
+                // LCOV_EXCL_STOP
             }
 
             return {
@@ -243,18 +255,22 @@ deleteAMMAccount(
     auto ammSle = sb.peek(keylet::amm(asset, asset2));
     if (!ammSle)
     {
+        // LCOV_EXCL_START
         JLOG(j.error()) << "deleteAMMAccount: AMM object does not exist "
                         << asset << " " << asset2;
         return tecINTERNAL;
+        // LCOV_EXCL_STOP
     }
 
     auto const ammAccountID = (*ammSle)[sfAccount];
     auto sleAMMRoot = sb.peek(keylet::account(ammAccountID));
     if (!sleAMMRoot)
     {
+        // LCOV_EXCL_START
         JLOG(j.error()) << "deleteAMMAccount: AMM account does not exist "
                         << to_string(ammAccountID);
         return tecINTERNAL;
+        // LCOV_EXCL_STOP
     }
 
     if (auto const ter =
@@ -266,14 +282,18 @@ deleteAMMAccount(
     if (!sb.dirRemove(
             ownerDirKeylet, (*ammSle)[sfOwnerNode], ammSle->key(), false))
     {
+        // LCOV_EXCL_START
         JLOG(j.error()) << "deleteAMMAccount: failed to remove dir link";
         return tecINTERNAL;
+        // LCOV_EXCL_STOP
     }
     if (sb.exists(ownerDirKeylet) && !sb.emptyDirDelete(ownerDirKeylet))
     {
+        // LCOV_EXCL_START
         JLOG(j.error()) << "deleteAMMAccount: cannot delete root dir node of "
                         << toBase58(ammAccountID);
         return tecINTERNAL;
+        // LCOV_EXCL_STOP
     }
 
     sb.erase(ammSle);
@@ -322,11 +342,11 @@ initializeFeeAuctionVote(
     if (tfee != 0)
         ammSle->setFieldU16(sfTradingFee, tfee);
     else if (ammSle->isFieldPresent(sfTradingFee))
-        ammSle->makeFieldAbsent(sfTradingFee);
+        ammSle->makeFieldAbsent(sfTradingFee);  // LCOV_EXCL_LINE never hit
     if (auto const dfee = tfee / AUCTION_SLOT_DISCOUNTED_FEE_FRACTION)
         auctionSlot.setFieldU16(sfDiscountedFee, dfee);
     else if (auctionSlot.isFieldPresent(sfDiscountedFee))
-        auctionSlot.makeFieldAbsent(sfDiscountedFee);
+        auctionSlot.makeFieldAbsent(sfDiscountedFee);  // LCOV_EXCL_LINE not hit
 }
 
 }  // namespace ripple

src/ripple/app/paths/AMMOffer.h

@@ -75,9 +75,6 @@ class AMMOffer
     Issue const&
     issueIn() const;
 
-    Issue const&
-    issueOut() const;
-
     AccountID const&
     owner() const;
 

src/ripple/app/paths/impl/AMMOffer.cpp

@@ -44,13 +44,6 @@ AMMOffer<TIn, TOut>::issueIn() const
     return ammLiquidity_.issueIn();
 }
 
-template <typename TIn, typename TOut>
-Issue const&
-AMMOffer<TIn, TOut>::issueOut() const
-{
-    return ammLiquidity_.issueOut();
-}
-
 template <typename TIn, typename TOut>
 AccountID const&
 AMMOffer<TIn, TOut>::owner() const

src/ripple/app/tx/impl/AMMDeposit.cpp

@@ -186,16 +186,18 @@ AMMDeposit::preclaim(PreclaimContext const& ctx)
         FreezeHandling::fhIGNORE_FREEZE,
         ctx.j);
     if (!expected)
-        return expected.error();
+        return expected.error();  // LCOV_EXCL_LINE
     auto const [amountBalance, amount2Balance, lptAMMBalance] = *expected;
     if (ctx.tx.getFlags() & tfTwoAssetIfEmpty)
     {
         if (lptAMMBalance != beast::zero)
             return tecAMM_NOT_EMPTY;
         if (amountBalance != beast::zero || amount2Balance != beast::zero)
         {
+            // LCOV_EXCL_START
             JLOG(ctx.j.debug()) << "AMM Deposit: tokens balance is not zero.";
             return tecINTERNAL;
+            // LCOV_EXCL_STOP
         }
     }
     else
@@ -205,9 +207,11 @@ AMMDeposit::preclaim(PreclaimContext const& ctx)
         if (amountBalance <= beast::zero || amount2Balance <= beast::zero ||
             lptAMMBalance < beast::zero)
         {
+            // LCOV_EXCL_START
             JLOG(ctx.j.debug())
                 << "AMM Deposit: reserves or tokens balance is zero.";
             return tecINTERNAL;
+            // LCOV_EXCL_STOP
         }
     }
 
@@ -254,10 +258,12 @@ AMMDeposit::preclaim(PreclaimContext const& ctx)
             if (auto const ter =
                     requireAuth(ctx.view, amount->issue(), accountID))
             {
+                // LCOV_EXCL_START
                 JLOG(ctx.j.debug())
                     << "AMM Deposit: account is not authorized, "
                     << amount->issue();
                 return ter;
+                // LCOV_EXCL_STOP
             }
             // AMM account or currency frozen
             if (isFrozen(ctx.view, ammAccountID, amount->issue()))
@@ -339,7 +345,7 @@ AMMDeposit::applyGuts(Sandbox& sb)
     auto const lpTokensDeposit = ctx_.tx[~sfLPTokenOut];
     auto ammSle = sb.peek(keylet::amm(ctx_.tx[sfAsset], ctx_.tx[sfAsset2]));
     if (!ammSle)
-        return {tecINTERNAL, false};
+        return {tecINTERNAL, false};  // LCOV_EXCL_LINE
     auto const ammAccountID = (*ammSle)[sfAccount];
 
     auto const expected = ammHolds(
@@ -350,7 +356,7 @@ AMMDeposit::applyGuts(Sandbox& sb)
         FreezeHandling::fhZERO_IF_FROZEN,
         ctx_.journal);
     if (!expected)
-        return {expected.error(), false};
+        return {expected.error(), false};  // LCOV_EXCL_LINE
     auto const [amountBalance, amount2Balance, lptAMMBalance] = *expected;
     auto const tfee = (lptAMMBalance == beast::zero)
         ? ctx_.tx[~sfTradingFee].value_or(0)
@@ -421,8 +427,10 @@ AMMDeposit::applyGuts(Sandbox& sb)
                 lptAMMBalance.issue(),
                 tfee);
         // should not happen.
+        // LCOV_EXCL_START
         JLOG(j_.error()) << "AMM Deposit: invalid options.";
         return std::make_pair(tecINTERNAL, STAmount{});
+        // LCOV_EXCL_STOP
     }();
 
     if (result == tesSUCCESS)
@@ -621,10 +629,12 @@ AMMDeposit::equalDepositTokens(
     }
     catch (std::exception const& e)
     {
+        // LCOV_EXCL_START
         JLOG(j_.error()) << "AMMDeposit::equalDepositTokens exception "
                          << e.what();
+        return {tecINTERNAL, STAmount{}};
+        // LCOV_EXCL_STOP
     }
-    return {tecINTERNAL, STAmount{}};
 }
 
 /** Proportional deposit of pool assets with the constraints on the maximum

src/ripple/app/tx/impl/AMMWithdraw.cpp

@@ -201,7 +201,7 @@ AMMWithdraw::preclaim(PreclaimContext const& ctx)
     {
         JLOG(ctx.j.debug())
             << "AMM Withdraw: reserves or tokens balance is zero.";
-        return tecINTERNAL;
+        return tecINTERNAL;  // LCOV_EXCL_LINE
     }
 
     auto const ammAccountID = ammSle->getAccountID(sfAccount);
@@ -300,11 +300,11 @@ AMMWithdraw::applyGuts(Sandbox& sb)
     auto const ePrice = ctx_.tx[~sfEPrice];
     auto ammSle = sb.peek(keylet::amm(ctx_.tx[sfAsset], ctx_.tx[sfAsset2]));
     if (!ammSle)
-        return {tecINTERNAL, false};
+        return {tecINTERNAL, false};  // LCOV_EXCL_LINE
     auto const ammAccountID = (*ammSle)[sfAccount];
     auto const accountSle = sb.read(keylet::account(ammAccountID));
     if (!accountSle)
-        return {tecINTERNAL, false};
+        return {tecINTERNAL, false};  // LCOV_EXCL_LINE
     auto const lpTokens =
         ammLPHolds(ctx_.view(), *ammSle, ctx_.tx[sfAccount], ctx_.journal);
     auto const lpTokensWithdraw =
@@ -372,8 +372,10 @@ AMMWithdraw::applyGuts(Sandbox& sb)
                 *lpTokensWithdraw,
                 tfee);
         // should not happen.
+        // LCOV_EXCL_START
         JLOG(j_.error()) << "AMM Withdraw: invalid options.";
         return std::make_pair(tecINTERNAL, STAmount{});
+        // LCOV_EXCL_STOP
     }();
 
     if (result != tesSUCCESS)
@@ -431,7 +433,7 @@ AMMWithdraw::withdraw(
     auto const ammSle =
         ctx_.view().read(keylet::amm(ctx_.tx[sfAsset], ctx_.tx[sfAsset2]));
     if (!ammSle)
-        return {tecINTERNAL, STAmount{}};
+        return {tecINTERNAL, STAmount{}};  // LCOV_EXCL_LINE
     auto const lpTokens = ammLPHolds(view, *ammSle, account_, ctx_.journal);
     auto const expected = ammHolds(
         view,
@@ -612,12 +614,14 @@ AMMWithdraw::equalWithdrawTokens(
             lpTokensWithdraw,
             tfee);
     }
+    // LCOV_EXCL_START
     catch (std::exception const& e)
     {
         JLOG(j_.error()) << "AMMWithdraw::equalWithdrawTokens exception "
                          << e.what();
     }
     return {tecINTERNAL, STAmount{}};
+    // LCOV_EXCL_STOP
 }
 
 /** All assets withdrawal with the constraints on the maximum amount

src/ripple/protocol/STIssue.h

@@ -56,9 +56,6 @@ class STIssue final : public STBase, CountedObject<STIssue>
     SerializedTypeID
     getSType() const override;
 
-    std::string
-    getText() const override;
-
     Json::Value getJson(JsonOptions) const override;
 
     void
@@ -71,9 +68,6 @@ class STIssue final : public STBase, CountedObject<STIssue>
     isDefault() const override;
 
 private:
-    static std::unique_ptr<STIssue>
-    construct(SerialIter&, SField const& name);
-
     STBase*
     copy(std::size_t n, void* buf) const override;
     STBase*

src/ripple/protocol/impl/STIssue.cpp

@@ -65,12 +65,6 @@ STIssue::getSType() const
     return STI_ISSUE;
 }
 
-std::string
-STIssue::getText() const
-{
-    return issue_.getText();
-}
-
 Json::Value STIssue::getJson(JsonOptions) const
 {
     return to_json(issue_);
@@ -97,12 +91,6 @@ STIssue::isDefault() const
     return issue_ == xrpIssue();
 }
 
-std::unique_ptr<STIssue>
-STIssue::construct(SerialIter& sit, SField const& name)
-{
-    return std::make_unique<STIssue>(sit, name);
-}
-
 STBase*
 STIssue::copy(std::size_t n, void* buf) const
 {

src/test/app/AMMExtended_test.cpp

@@ -42,6 +42,9 @@
 namespace ripple {
 namespace test {
 
+/**
+ * Tests of AMM that use offers too.
+ */
 struct AMMExtended_test : public jtx::AMMTest
 {
 private: