refactor(AMMClawback): move tfClawTwoAssets check (#5201)

Move tfClawTwoAssets check to preflight and return error temINVALID_FLAG --------- Co-authored-by: yinyiqian1 <yqian@ripple.com>
XRPLF · Nov 25, 2024 · b54d85d · b54d85d
1 parent f419c18
commit b54d85d
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 14 deletions.
diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro
@@ -32,9 +32,9 @@
 XRPL_FEATURE(Credentials,                Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FEATURE(AMMClawback,                Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FIX    (AMMv1_2,                    Supported::yes, VoteBehavior::DefaultNo)
+XRPL_FEATURE(MPTokensV1,                 Supported::yes, VoteBehavior::DefaultNo)
 // InvariantsV1_1 will be changes to Supported::yes when all the
 // invariants expected to be included under it are complete.
-XRPL_FEATURE(MPTokensV1,                 Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FEATURE(InvariantsV1_1,             Supported::no,  VoteBehavior::DefaultNo)
 XRPL_FIX    (NFTokenPageLinks,           Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FIX    (InnerObjTemplate2,          Supported::yes, VoteBehavior::DefaultNo)

diff --git a/src/test/app/AMMClawback_test.cpp b/src/test/app/AMMClawback_test.cpp
@@ -303,13 +303,13 @@ class AMMClawback_test : public jtx::AMMTest
             // gw creates AMM pool of XRP/USD.
             AMM amm(env, gw, XRP(100), USD(100), ter(tesSUCCESS));
 
-            // Return tecNO_PERMISSION because the issuer set tfClawTwoAssets,
+            // Return temINVALID_FLAG because the issuer set tfClawTwoAssets,
             // but the issuer only issues USD in the pool. The issuer is not
             // allowed to set tfClawTwoAssets flag if he did not issue both
-            // assts in the pool.
+            // assets in the pool.
             env(amm::ammClawback(gw, alice, USD, XRP, std::nullopt),
                 txflags(tfClawTwoAssets),
-                ter(tecNO_PERMISSION));
+                ter(temINVALID_FLAG));
         }
 
         // Test clawing back XRP is being prohibited.

diff --git a/src/xrpld/app/tx/detail/AMMClawback.cpp b/src/xrpld/app/tx/detail/AMMClawback.cpp
@@ -42,7 +42,8 @@ AMMClawback::preflight(PreflightContext const& ctx)
     if (auto const ret = preflight1(ctx); !isTesSuccess(ret))
         return ret;  // LCOV_EXCL_LINE
 
-    if (ctx.tx.getFlags() & tfAMMClawbackMask)
+    auto const flags = ctx.tx.getFlags();
+    if (flags & tfAMMClawbackMask)
         return temINVALID_FLAG;
 
     AccountID const issuer = ctx.tx[sfAccount];
@@ -57,10 +58,19 @@ AMMClawback::preflight(PreflightContext const& ctx)
 
     std::optional<STAmount> const clawAmount = ctx.tx[~sfAmount];
     auto const asset = ctx.tx[sfAsset];
+    auto const asset2 = ctx.tx[sfAsset2];
 
     if (isXRP(asset))
         return temMALFORMED;
 
+    if (flags & tfClawTwoAssets && asset.account != asset2.account)
+    {
+        JLOG(ctx.j.trace())
+            << "AMMClawback: tfClawTwoAssets can only be enabled when two "
+               "assets in the AMM pool are both issued by the issuer";
+        return temINVALID_FLAG;
+    }
+
     if (asset.account != issuer)
     {
         JLOG(ctx.j.trace()) << "AMMClawback: Asset's account does not "
@@ -108,15 +118,6 @@ AMMClawback::preclaim(PreclaimContext const& ctx)
         (issuerFlagsIn & lsfNoFreeze))
         return tecNO_PERMISSION;
 
-    auto const flags = ctx.tx.getFlags();
-    if (flags & tfClawTwoAssets && asset.account != asset2.account)
-    {
-        JLOG(ctx.j.trace())
-            << "AMMClawback: tfClawTwoAssets can only be enabled when two "
-               "assets in the AMM pool are both issued by the issuer";
-        return tecNO_PERMISSION;
-    }
-
     return tesSUCCESS;
 }