test: Add tests to raise coverage of AMM (XRPLF#4971)

--------- Co-authored-by: Howard Hinnant <[email protected]> Co-authored-by: Mark Travis <[email protected]> Co-authored-by: Bronek Kozicki <[email protected]> Co-authored-by: Mayukha Vadari <[email protected]> Co-authored-by: Chenna Keshava <[email protected]>
sophiax851 · Jun 12, 2024 · 56c16be · 56c16be
1 parent d304f9c
commit 56c16be
Show file tree

Hide file tree

Showing 15 changed files with 581 additions and 287 deletions.
diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/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,21 @@ ammHolds(
  return std::make_optional(std::make_pair(issue1, issue2));
  else if (checkIssue == issue2)
  return std::make_optional(std::make_pair(issue2, issue1));
+ // Unreachable unless AMM 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");
+ // Cannot have Amount2 without Amount.
+ return singleIssue(*optIssue2, "optIssue2"); // LCOV_EXCL_LINE
  }
  return std::make_optional(std::make_pair(issue1, issue2));
  }();
@@ -210,19 +217,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 +254,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 +281,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 +341,11 @@ initializeFeeAuctionVote(
  if (tfee != 0)
  ammSle->setFieldU16(sfTradingFee, tfee);
  else if (ammSle->isFieldPresent(sfTradingFee))
- ammSle->makeFieldAbsent(sfTradingFee);
+ ammSle->makeFieldAbsent(sfTradingFee); // LCOV_EXCL_LINE
  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
 }
 
 } // namespace ripple
diff --git a/src/ripple/app/paths/AMMOffer.h b/src/ripple/app/paths/AMMOffer.h
@@ -74,9 +74,6 @@ class AMMOffer
  Issue const&
  issueIn() const;
 
- Issue const&
- issueOut() const;
-
  AccountID const&
  owner() const;
 

diff --git a/src/ripple/app/paths/impl/AMMOffer.cpp b/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

diff --git a/src/ripple/app/tx/impl/AMMDeposit.cpp b/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

diff --git a/src/ripple/app/tx/impl/AMMWithdraw.cpp b/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

diff --git a/src/ripple/protocol/STIssue.h b/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*

diff --git a/src/ripple/protocol/impl/STIssue.cpp b/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
 {

diff --git a/src/test/app/AMMExtended_test.cpp b/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:
@@ -3614,6 +3617,8 @@ struct AMMExtended_test : public jtx::AMMTest
  int const signerListOwners{features[featureMultiSignReserve] ? 2 : 5};
  env.require(owners(alice, signerListOwners + 0));
 
+ const msig ms{becky, bogie};
+
  // Multisign all AMM transactions
  AMM ammAlice(
  env,
@@ -3625,7 +3630,7 @@ struct AMMExtended_test : public jtx::AMMTest
  ammCrtFee(env).drops(),
  std::nullopt,
  std::nullopt,
- msig(becky, bogie),
+ ms,
  ter(tesSUCCESS));
  BEAST_EXPECT(ammAlice.expectBalances(
  XRP(10'000), USD(10'000), ammAlice.tokens()));
@@ -3641,7 +3646,7 @@ struct AMMExtended_test : public jtx::AMMTest
  ammAlice.vote({}, 1'000);
  BEAST_EXPECT(ammAlice.expectTradingFee(1'000));
 
- ammAlice.bid(alice, 100);
+ env(ammAlice.bid({.account = alice, .bidMin = 100}), ms).close();
  BEAST_EXPECT(ammAlice.expectAuctionSlot(100, 0, IOUAmount{4'000}));
  // 4000 tokens burnt
  BEAST_EXPECT(ammAlice.expectBalances(