resolve review comments

include/xrpl/protocol/SField.h

@@ -588,7 +588,6 @@ extern SF_ACCOUNT const sfRegularKey;
 extern SF_ACCOUNT const sfNFTokenMinter;
 extern SF_ACCOUNT const sfEmitCallback;
 extern SF_ACCOUNT const sfHolder;
-extern SF_ACCOUNT const sfAMMAccount;
 
 // account (uncommon)
 extern SF_ACCOUNT const sfHookAccount;

include/xrpl/protocol/STObject.h

@@ -243,8 +243,6 @@ class STObject : public STBase, public CountedObject<STObject>
     getFieldArray(SField const& field) const;
     const STCurrency&
     getFieldCurrency(SField const& field) const;
-    const STIssue&
-    getFieldIssue(SField const& field) const;
 
     /** Get the value of a field.
         @param A TypedField built from an SField value representing the desired

include/xrpl/protocol/jss.h

@@ -48,7 +48,6 @@ JSS(AccountDelete);        // transaction type.
 JSS(AccountRoot);          // ledger type.
 JSS(AccountSet);           // transaction type.
 JSS(AMM);                  // ledger type
-JSS(AMMAccount);           // field
 JSS(AMMBid);               // transaction type
 JSS(AMMClawback);          // transaction type.
 JSS(AMMID);                // field

src/test/app/AMMClawback_test.cpp

@@ -93,6 +93,11 @@ class AMMClawback_test : public jtx::AMMTest
             // Issuer can not clawback from himself.
             env(amm::ammClawback(gw, gw, USD, XRP, std::nullopt, std::nullopt),
                 ter(temMALFORMED));
+
+            // Holder can not clawback from himself.
+            env(amm::ammClawback(
+                    alice, alice, USD, XRP, std::nullopt, std::nullopt),
+                ter(temMALFORMED));
         }
 
         // Test if the Asset field matches the Account field.
@@ -640,11 +645,11 @@ class AMMClawback_test : public jtx::AMMTest
             BEAST_EXPECT(!amm.ammExists());
         }
 
-        // Test AMMClawback for USD/XRP pool. The assets are issued by different
-        // issuer. Claw back USD for multiple times, and XRP goes back to the
-        // holder. The last AMMClawback transaction exceeds the holder's USD
-        // balance in AMM pool. In this case, gw creates the AMM pool USD/XRP,
-        // both alice and bob deposit into it. gw2 creates the AMM pool EUR/XRP.
+        // Test AMMClawback for USD/XRP pool. Claw back USD for multiple times,
+        // and XRP goes back to the holder. The last AMMClawback transaction
+        // exceeds the holder's USD balance in AMM pool. In this case, gw
+        // creates the AMM pool USD/XRP, both alice and bob deposit into it. gw2
+        // creates the AMM pool EUR/XRP.
         {
             Env env(*this, features);
             Account gw{"gateway"};
@@ -1000,8 +1005,8 @@ class AMMClawback_test : public jtx::AMMTest
             BEAST_EXPECT(!amm.ammExists());
         }
 
-        // Test AMMClawback for USD/XRP pool. The assets are issued by different
-        // issuer. Claw back all the USD for different users.
+        // Test AMMClawback for USD/XRP pool. Claw back all the USD for
+        // different users.
         {
             Env env(*this, features);
             Account gw{"gateway"};

src/test/app/AMM_test.cpp

@@ -6951,44 +6951,45 @@ struct AMM_test : public jtx::AMMTest
         using namespace jtx;
 
         // This lambda function is used to create trustlines
-        // between gw and alice, and create and return an AMM account.
-        auto setupAMM = [&](Env& env) -> AMM* {
+        // between gw and alice, and create an AMM account.
+        // And also test the callback function.
+        auto testAMMDeposit = [&](Env& env, std::function<void(AMM & amm)> cb) {
             env.fund(XRP(1'000), gw);
             fund(env, gw, {alice}, XRP(1'000), {USD(1'000)}, Fund::Acct);
             env.close();
             AMM* amm = new AMM(env, alice, XRP(100), USD(100), ter(tesSUCCESS));
             env(trust(gw, alice["USD"](0), tfSetFreeze));
-            return amm;
+            cb(*amm);
         };
 
+        // Deposit two assets, one of which is frozen,
+        // then we should get tecFROZEN error.
         {
-            // Deposit two assets, one of which is frozen,
-            // then we should get tecFROZEN error.
             Env env(*this, features);
-            auto amm = setupAMM(env);
-            amm->deposit(
-                alice,
-                USD(100),
-                XRP(100),
-                std::nullopt,
-                tfTwoAsset,
-                ter(tecFROZEN));
-            delete amm;
+            testAMMDeposit(env, [&](AMM& amm) {
+                amm.deposit(
+                    alice,
+                    USD(100),
+                    XRP(100),
+                    std::nullopt,
+                    tfTwoAsset,
+                    ter(tecFROZEN));
+            });
         }
 
+        // Deposit one asset, which is the frozen token,
+        // then we should get tecFROZEN error.
         {
-            // Deposit one asset, which is the frozen token,
-            // then we should get tecFROZEN error.
             Env env(*this, features);
-            auto amm = setupAMM(env);
-            amm->deposit(
-                alice,
-                USD(100),
-                std::nullopt,
-                std::nullopt,
-                tfSingleAsset,
-                ter(tecFROZEN));
-            delete amm;
+            testAMMDeposit(env, [&](AMM& amm) {
+                amm.deposit(
+                    alice,
+                    USD(100),
+                    std::nullopt,
+                    std::nullopt,
+                    tfSingleAsset,
+                    ter(tecFROZEN));
+            });
         }
 
         if (features[featureAMMClawback])
@@ -6997,29 +6998,31 @@ struct AMM_test : public jtx::AMMTest
             // but the other asset is frozen. We should get tecFROZEN error
             // when feature AMMClawback is enabled.
             Env env(*this, features);
-            auto amm = setupAMM(env);
-            amm->deposit(
-                alice,
-                XRP(100),
-                std::nullopt,
-                std::nullopt,
-                tfSingleAsset,
-                ter(tecFROZEN));
+            testAMMDeposit(env, [&](AMM& amm) {
+                amm.deposit(
+                    alice,
+                    XRP(100),
+                    std::nullopt,
+                    std::nullopt,
+                    tfSingleAsset,
+                    ter(tecFROZEN));
+            });
         }
         else
         {
             // Deposit one asset which is not the frozen token,
             // but the other asset is frozen. We will get tecSUCCESS
             // when feature AMMClawback is not enabled.
             Env env(*this, features);
-            auto amm = setupAMM(env);
-            amm->deposit(
-                alice,
-                XRP(100),
-                std::nullopt,
-                std::nullopt,
-                tfSingleAsset,
-                ter(tesSUCCESS));
+            testAMMDeposit(env, [&](AMM& amm) {
+                amm.deposit(
+                    alice,
+                    XRP(100),
+                    std::nullopt,
+                    std::nullopt,
+                    tfSingleAsset,
+                    ter(tesSUCCESS));
+            });
         }
     }
 

src/xrpld/app/misc/AMMUtils.h

@@ -123,31 +123,6 @@ isOnlyLiquidityProvider(
     Issue const& ammIssue,
     AccountID const& lpAccount);
 
-std::tuple<TER, STAmount, STAmount, std::optional<STAmount>>
-withdraw(
-    Sandbox& view,
-    AccountID const& ammAccount,
-    AccountID const& account,
-    SLE const& ammSle,
-    STAmount const& amountBalance,
-    STAmount const& amountWithdraw,
-    std::optional<STAmount> const& amount2Withdraw,
-    STAmount const& lpTokensAMMBalance,
-    STAmount const& lpTokensWithdraw,
-    std::uint16_t tfee,
-    beast::Journal const& journal,
-    STTx const& tx,
-    bool withdrawAll);
-
-std::pair<TER, bool>
-deleteAMMAccountIfEmpty(
-    Sandbox& sb,
-    std::shared_ptr<SLE> const ammSle,
-    STAmount const& lpTokenBalance,
-    Issue const& issue1,
-    Issue const& issue2,
-    beast::Journal const& journal);
-
 }  // namespace ripple
 
 #endif  // RIPPLE_APP_MISC_AMMUTILS_H_INLCUDED