fix(AMM): prevent orphaned objects, inconsistent ledger state: (updates XLS-30)

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

@@ -282,7 +282,6 @@ applyCreate(
  TOTAL_TIME_SLOT_SECS;
  auctionSlot.setFieldU32(sfExpiration, expiration);
  auctionSlot.setFieldAmount(sfPrice, STAmount{lpTokens.issue(), 0});
- sb.insert(ammSle);
 
  // Add owner directory to link the root account and AMM object.
  if (auto const page = sb.dirInsert(
@@ -294,6 +293,9 @@ applyCreate(
  JLOG(j_.debug()) << "AMM Instance: failed to insert owner dir";
  return {tecDIR_FULL, false};
  }
+ else
+ ammSle->setFieldU64(sfOwnerNode, *page);
+ sb.insert(ammSle);
 
  // Send LPT to LP.
  auto res = accountSend(sb, *ammAccount, account_, lpTokens, ctx_.journal);

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

@@ -25,7 +25,6 @@
 #include <ripple/ledger/Sandbox.h>
 #include <ripple/ledger/View.h>
 #include <ripple/protocol/AMMCore.h>
-#include <ripple/protocol/Feature.h>
 #include <ripple/protocol/STAccount.h>
 #include <ripple/protocol/TxFlags.h>
 
@@ -410,9 +409,78 @@ AMMWithdraw::deleteAccount(Sandbox& sb, AccountID const& ammAccountID)
  if (!sleAMMRoot || !ammSle)
  return tecINTERNAL;
 
- // Note, the AMM trust lines are deleted since the balance
- // goes to 0. It also means there are no linked
- // ledger objects.
+ // Delete directory entry linking the root account and AMM object
+ Keylet const ownerDirKeylet{keylet::ownerDir(ammAccountID)};
+ if (!sb.dirRemove(
+ ownerDirKeylet, (*ammSle)[sfOwnerNode], ammSle->key(), false))
+ {
+ JLOG(j_.fatal()) << "AMM Withdraw: failed to remove dir link";
+ return tecINTERNAL;
+ }
+
+ // Delete the trustlines
+ std::shared_ptr<SLE> sleDirNode{};
+ unsigned int uDirEntry{0};
+ uint256 dirEntry{beast::zero};
+ if (sb.exists(ownerDirKeylet) &&
+ dirFirst(sb, ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry))
+ {
+ do
+ {
+ auto sleItem = sb.peek(keylet::child(dirEntry));
+ if (!sleItem)
+ {
+ // Directory node has an invalid index
+ JLOG(j_.fatal())
+ << "AMM Withdraw: Directory node in ledger " << sb.seq()
+ << " has index to object that is missing: "
+ << to_string(dirEntry);
+ return tefBAD_LEDGER;
+ }
+
+ // Should only have the trustlines
+ if (auto const nodeType = safe_cast<LedgerEntryType>(
+ sleItem->getFieldU16(sfLedgerEntryType));
+ nodeType != LedgerEntryType::ltRIPPLE_STATE)
+ {
+ JLOG(j_.error())
+ << "AMM Withdraw: deleting non-trustline " << nodeType;
+ return tecINTERNAL;
+ }
+
+ // Trustlines must have zero balance
+ if (sleItem->getFieldAmount(sfBalance) != beast::zero)
+ {
+ JLOG(j_.error()) << "AMM Withdraw: deleting trustline with "
+ "non-zero balance.";
+ return tecINTERNAL;
+ }
+
+ auto const& [lowAccountID, highAccountID] = std::minmax(
+ sleItem->getFieldAmount(sfHighLimit).getIssuer(), ammAccountID);
+ if (auto const ter = trustDelete(
+ sb, sleItem, lowAccountID, highAccountID, ctx_.journal);
+ ter != tesSUCCESS)
+ {
+ JLOG(j_.error())
+ << "AMM Withdraw: failed to delete the trustline.";
+ return ter;
+ }
+
+ // Update the iterator
+ assert(uDirEntry == 1);
+ if (uDirEntry != 1)
+ {
+ JLOG(j_.error())
+ << "AMM Withdraw: iterator re-validation failed.";
+ return tefBAD_LEDGER;
+ }
+ uDirEntry = 0;
+
+ } while (
+ dirNext(sb, ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry));
+ }
+
  sb.erase(ammSle);
  sb.erase(sleAMMRoot);
 

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

@@ -97,6 +97,10 @@ CreateCheck::preclaim(PreclaimContext const& ctx)
  (flags & lsfDisallowIncomingCheck))
  return tecNO_PERMISSION;
 
+ // AMM can not cash the check
+ if (flags & lsfAMM)
+ return tecNO_PERMISSION;
+
  if ((flags & lsfRequireDestTag) && !ctx.tx.isFieldPresent(sfDestinationTag))
  {
  // The tag is basically account-specific information we don't

src/ripple/protocol/impl/InnerObjectFormats.cpp

@@ -77,6 +77,12 @@ InnerObjectFormats::InnerObjectFormats()
  {sfPrice, soeREQUIRED},
  {sfAuthAccounts, soeOPTIONAL},
  });
+
+ add(sfAuthAccount.jsonName.c_str(),
+ sfAuthAccount.getCode(),
+ {
+ {sfAccount, soeREQUIRED},
+ });
 }
 
 InnerObjectFormats const&

src/ripple/protocol/impl/LedgerFormats.cpp

@@ -277,7 +277,8 @@ LedgerFormats::LedgerFormats()
  {sfAuctionSlot, soeOPTIONAL},
  {sfLPTokenBalance, soeREQUIRED},
  {sfAsset, soeREQUIRED},
- {sfAsset2, soeREQUIRED}
+ {sfAsset2, soeREQUIRED},
+ {sfOwnerNode, soeREQUIRED}
  },
  commonFields);
 

src/test/app/AMMExtended_test.cpp

@@ -32,7 +32,6 @@
 #include <test/jtx/AMM.h>
 #include <test/jtx/AMMTest.h>
 #include <test/jtx/PathSet.h>
-#include <test/jtx/TestHelpers.h>
 #include <test/jtx/amount.h>
 #include <test/jtx/sendmax.h>
 

src/test/app/AMM_test.cpp

@@ -27,7 +27,6 @@
 #include <test/jtx.h>
 #include <test/jtx/AMM.h>
 #include <test/jtx/AMMTest.h>
-#include <test/jtx/TestHelpers.h>
 #include <test/jtx/amount.h>
 #include <test/jtx/sendmax.h>
 
@@ -1770,9 +1769,16 @@ struct AMM_test : public jtx::AMMTest
 
  // Withdraw all tokens.
  testAMM([&](AMM& ammAlice, Env& env) {
+ env(trust(carol, STAmount{ammAlice.lptIssue(), 10000}));
+ env(trust(
+ carol,
+ STAmount{Issue{EUR.currency, ammAlice.ammAccount()}, 10000}));
+ env.close();
  ammAlice.withdrawAll(alice);
  BEAST_EXPECT(!ammAlice.ammExists());
 
+ BEAST_EXPECT(!env.le(keylet::ownerDir(ammAlice.ammAccount())));
+
  // Can create AMM for the XRP/USD pair
  AMM ammCarol(env, carol, XRP(10'000), USD(10'000));
  BEAST_EXPECT(ammCarol.expectBalances(
@@ -2750,6 +2756,12 @@ struct AMM_test : public jtx::AMMTest
  ter(tecNO_PERMISSION));
  });
 
+ // Can't pay into AMM with checks.
+ testAMM([&](AMM& ammAlice, Env& env) {
+ env(check::create(env.master.id(), ammAlice.ammAccount(), XRP(100)),
+ ter(tecNO_PERMISSION));
+ });
+
  // Pay amounts close to one side of the pool
  testAMM(
  [&](AMM& ammAlice, Env& env) {

src/test/app/CrossingLimits_test.cpp

@@ -18,7 +18,6 @@
 #include <ripple/beast/unit_test.h>
 #include <ripple/protocol/Feature.h>
 #include <test/jtx.h>
-#include <test/jtx/TestHelpers.h>
 
 namespace ripple {
 namespace test {

src/test/app/Escrow_test.cpp

@@ -24,7 +24,6 @@
 #include <ripple/protocol/TxFlags.h>
 #include <ripple/protocol/jss.h>
 #include <test/jtx.h>
-#include <test/jtx/TestHelpers.h>
 
 #include <algorithm>
 #include <iterator>

src/test/app/Flow_test.cpp

@@ -28,7 +28,6 @@
 #include <ripple/protocol/jss.h>
 #include <test/jtx.h>
 #include <test/jtx/PathSet.h>
-#include <test/jtx/TestHelpers.h>
 
 namespace ripple {
 namespace test {

src/test/app/Freeze_test.cpp

@@ -22,7 +22,6 @@
 #include <ripple/protocol/TxFlags.h>
 #include <ripple/protocol/jss.h>
 #include <test/jtx.h>
-#include <test/jtx/TestHelpers.h>
 
 namespace ripple {
 

src/test/app/Offer_test.cpp

@@ -22,7 +22,6 @@
 #include <ripple/protocol/jss.h>
 #include <test/jtx.h>
 #include <test/jtx/PathSet.h>
-#include <test/jtx/TestHelpers.h>
 #include <test/jtx/WSClient.h>
 
 namespace ripple {

src/test/app/Path_test.cpp

@@ -35,7 +35,6 @@
 #include <condition_variable>
 #include <mutex>
 #include <test/jtx.h>
-#include <test/jtx/TestHelpers.h>
 #include <test/jtx/envconfig.h>
 #include <thread>
 

src/test/app/PayChan_test.cpp

@@ -25,7 +25,6 @@
 #include <ripple/protocol/TxFlags.h>
 #include <ripple/protocol/jss.h>
 #include <test/jtx.h>
-#include <test/jtx/TestHelpers.h>
 
 #include <chrono>
 

src/test/app/PayStrand_test.cpp

@@ -29,7 +29,6 @@
 #include "ripple/app/paths/AMMContext.h"
 #include <test/jtx.h>
 #include <test/jtx/PathSet.h>
-#include <test/jtx/TestHelpers.h>
 
 namespace ripple {
 namespace test {

src/test/app/TrustAndBalance_test.cpp

@@ -22,7 +22,6 @@
 #include <ripple/protocol/SField.h>
 #include <ripple/protocol/jss.h>
 #include <test/jtx.h>
-#include <test/jtx/TestHelpers.h>
 #include <test/jtx/WSClient.h>
 
 namespace ripple {

src/test/jtx.h

@@ -27,6 +27,7 @@
 #include <test/jtx/Env.h>
 #include <test/jtx/Env_ss.h>
 #include <test/jtx/JTx.h>
+#include <test/jtx/TestHelpers.h>
 #include <test/jtx/account_txn_id.h>
 #include <test/jtx/acctdelete.h>
 #include <test/jtx/amount.h>

src/test/jtx/TestHelpers.h

@@ -23,6 +23,7 @@
 #include <ripple/json/json_value.h>
 #include <ripple/protocol/AccountID.h>
 #include <ripple/protocol/Quality.h>
+#include <ripple/protocol/TxFlags.h>
 #include <ripple/protocol/jss.h>
 #include <test/jtx/Env.h>
 
@@ -405,6 +406,38 @@ STPathElement
 allpe(AccountID const& a, Issue const& iss);
 /***************************************************************/
 
+/* Check */
+/***************************************************************/
+namespace check {
+
+/** Create a check. */
+// clang-format off
+template <typename A>
+ requires std::is_same_v<A, AccountID>
+Json::Value
+create(A const& account, A const& dest, STAmount const& sendMax)
+{
+ Json::Value jv;
+ jv[sfAccount.jsonName] = to_string(account);
+ jv[sfSendMax.jsonName] = sendMax.getJson(JsonOptions::none);
+ jv[sfDestination.jsonName] = to_string(dest);
+ jv[sfTransactionType.jsonName] = jss::CheckCreate;
+ jv[sfFlags.jsonName] = tfUniversal;
+ return jv;
+}
+// clang-format on
+
+inline Json::Value
+create(
+ jtx::Account const& account,
+ jtx::Account const& dest,
+ STAmount const& sendMax)
+{
+ return create(account.id(), dest.id(), sendMax);
+}
+
+} // namespace check
+
 } // namespace jtx
 } // namespace test
 } // namespace ripple

src/test/jtx/check.h

@@ -31,13 +31,6 @@ namespace jtx {
 /** Check operations. */
 namespace check {
 
-/** Create a check. */
-Json::Value
-create(
- jtx::Account const& account,
- jtx::Account const& dest,
- STAmount const& sendMax);
-
 /** Cash a check requiring that a specific amount be delivered. */
 Json::Value
 cash(jtx::Account const& dest, uint256 const& checkId, STAmount const& amount);

src/test/jtx/impl/check.cpp

@@ -27,22 +27,6 @@ namespace jtx {
 
 namespace check {
 
-// Create a check.
-Json::Value
-create(
- jtx::Account const& account,
- jtx::Account const& dest,
- STAmount const& sendMax)
-{
- Json::Value jv;
- jv[sfAccount.jsonName] = account.human();
- jv[sfSendMax.jsonName] = sendMax.getJson(JsonOptions::none);
- jv[sfDestination.jsonName] = dest.human();
- jv[sfTransactionType.jsonName] = jss::CheckCreate;
- jv[sfFlags.jsonName] = tfUniversal;
- return jv;
-}
-
 // Cash a check requiring that a specific amount be delivered.
 Json::Value
 cash(jtx::Account const& dest, uint256 const& checkId, STAmount const& amount)

src/test/rpc/AccountOffers_test.cpp

@@ -19,7 +19,6 @@
 
 #include <ripple/protocol/jss.h>
 #include <test/jtx.h>
-#include <test/jtx/TestHelpers.h>
 
 namespace ripple {
 namespace test {

src/test/rpc/LedgerData_test.cpp

@@ -20,7 +20,6 @@
 #include <ripple/basics/StringUtilities.h>
 #include <ripple/protocol/jss.h>
 #include <test/jtx.h>
-#include <test/jtx/TestHelpers.h>
 
 namespace ripple {
 

src/test/rpc/NoRipple_test.cpp

@@ -20,7 +20,6 @@
 #include <ripple/protocol/Feature.h>
 #include <ripple/protocol/jss.h>
 #include <test/jtx.h>
-#include <test/jtx/TestHelpers.h>
 
 namespace ripple {