Remove lsfMPTDomainCheck flag, delete expired MPToken

include/xrpl/protocol/LedgerFormats.h

@@ -188,7 +188,6 @@ enum LedgerSpecificFlags {
 
     // ltMPTOKEN
     lsfMPTAuthorized = 0x00000002,
-    lsfMPTDomainCheck = 0x00000004,
 
     // ltCREDENTIAL
     lsfAccepted = 0x00010000,

src/test/app/Vault_test.cpp

@@ -749,6 +749,16 @@ class Vault_test : public beast::unit_test::suite
                  .amount = asset(100)});
             env(tx);
         }
+
+        {
+            testcase("private vault depositor not authorized");
+            auto tx = vault.deposit(
+                {.depositor = depositor,
+                 .id = keylet.key,
+                 .amount = asset(50)});
+            env(tx, ter{tecNO_AUTH});
+            env.close();
+        }
     }
 
 public:

src/xrpld/app/tx/detail/VaultDeposit.cpp

@@ -78,19 +78,12 @@ VaultDeposit::preclaim(PreclaimContext const& ctx)
         // would allow authorization granted by the issuer explicitly, but Vault
         // does not have an MPT issuer (instead it uses pseudo-account, which is
         // blackholed and cannot create any transactions).
-        //
-        // We also need to do similar check inside doApply(), in order to remove
-        // expired credentials and/or adjust authorization flag on tokens owned
-        // by DomainID (i.e. with lsfMPTDomainCheck flag). This is why we
-        // suppress authorization errors if domainId is set.
-        uint256 domainId = beast::zero;
         auto const err = requireAuth(
-            ctx.view,
+            ctx.view, MPTIssue(vault->at(sfMPTokenIssuanceID)), account);
-            MPTIssue(vault->at(sfMPTokenIssuanceID)),
-            account,
-            &domainId);
 
-        if (domainId == beast::zero)
+        // As per requireAuth spec, we suppress tecEXPIRED error here, so we can
+        // delete any expired credentials inside doApply.
+        if (err != tecEXPIRED)
             return err;
     }
 

src/xrpld/app/tx/detail/VaultSet.cpp

@@ -64,19 +64,33 @@ TER
 VaultSet::preclaim(PreclaimContext const& ctx)
 {
     auto const id = ctx.tx[sfVaultID];
-    auto const sle = ctx.view.read(keylet::vault(id));
+    auto const vault = ctx.view.read(keylet::vault(id));
-    if (!sle)
+    if (!vault)
         return tecOBJECT_NOT_FOUND;
 
     // Assert that submitter is the Owner.
-    if (ctx.tx[sfAccount] != sle->at(sfOwner))
+    if (ctx.tx[sfAccount] != vault->at(sfOwner))
         return tecNO_PERMISSION;
 
     // We can only set domain if private flag was originally set
     if (auto const domain = ctx.tx[~sfDomainID])
     {
-        if ((sle->getFlags() & tfVaultPrivate) == 0)
+        if ((vault->getFlags() & tfVaultPrivate) == 0)
             return tecINVALID_DOMAIN;
+
+        auto const sleDomain =
+            ctx.view.read(keylet::permissionedDomain(*domain));
+        if (!sleDomain)
+            return tecNO_ENTRY;
+
+        // Sanity checks, all this should be enforced by VaultCreate
+        auto const mptIssuanceID = (*vault)[sfMPTokenIssuanceID];
+        auto const sleIssuance =
+            ctx.view.read(keylet::mptIssuance(mptIssuanceID));
+        if (!sleIssuance)
+            return tefINTERNAL;
+        if ((sleIssuance->getFlags() & lsfMPTRequireAuth) == 0)
+            return tefINTERNAL;
     }
 
     return tesSUCCESS;

src/xrpld/ledger/View.h

@@ -647,22 +647,22 @@ requireAuth(ReadView const& view, Issue const& issue, AccountID const& account);
  *
  *   This will also check for expired credentials. If it is called directly
  *   from preclaim, the user should convert result tecEXPIRED to tesSUCCESS and
- *   proceed to also check permissions with verifyValidDomain inside doApply.
+ *   proceed to also check permissions with enforceMPTokenAuthorization inside
- *   This will ensure that any expired credentials are deleted.
+ *   doApply. This will ensure that any expired credentials are deleted.
  */
 [[nodiscard]] TER
 requireAuth(
     ReadView const& view,
     MPTIssue const& mptIssue,
-    AccountID const& account,
+    AccountID const& account);
-    uint256* domainId = nullptr);
 
-/** Check if the account lacks required authorization.
+/** Enforce account has MPToken to match its authorization.
  *
  *   Called from doApply - it will check for expired (and delete if found any)
  *   credentials maching DomainID set in MPTIssuance. Must be called if
- *   requireAuth(...MPTIssue...) check passed in preclaim. Will create MPToken
+ *   requireAuth(...MPTIssue...) returned tesSUCCESS or tecEXPIRED in preclaim.
- *   if needed, on the basis of non-expired credentals found.
+ *   Will create MPToken (if needed) on the basis of any non-expired credentals
+ *   and delete any expried credentials (indirectly via verifyValidDomain)
  */
 [[nodiscard]] TER
 enforceMPTokenAuthorization(

src/xrpld/ledger/detail/View.cpp

@@ -2183,8 +2183,7 @@ TER
 requireAuth(
     ReadView const& view,
     MPTIssue const& mptIssue,
-    AccountID const& account,
+    AccountID const& account)
-    uint256* domainId)
 {
     auto const mptID = keylet::mptIssuance(mptIssue.getMptID());
     auto const sleIssuance = view.read(mptID);
@@ -2222,10 +2221,12 @@ requireAuth(
             credentials::validDomain(view, *maybeDomainID, account);
         !isTesSuccess(err))
     {
-        if (err != tecINVALID_DOMAIN && domainId != nullptr)
+        // Force tecEXPIRED error if there's an sleToken but no authorization -
-            (*domainId) = *maybeDomainID;
+        // i.e. an expired MPToken which should be deleted (if zero balance)
-
+        if (sleToken)
-        return err;
+            return tecEXPIRED;
+        else
+            return err;
     }
 
     // We are authorized by permissioned domain.
@@ -2247,86 +2248,86 @@ enforceMPTokenAuthorization(
 
     XRPL_ASSERT(
         sleIssuance->getFlags() & lsfMPTRequireAuth,
-        "ripple::verifyAuth : MPTokenIssuance requires authorization");
+        "ripple::enforceMPTokenAuthorization : authorization required");
 
     if (account == sleIssuance->at(sfIssuer))
         return tesSUCCESS;  // Won't create MPToken for the token issuer
 
     auto const keylet = keylet::mptoken(mptIssuanceID, account);
-    auto sleToken = view.read(keylet);
+    auto const sleToken = view.read(keylet);  //  NOTE: might be null
-    bool const domainOwned =
+    auto const maybeDomainID = sleIssuance->at(~sfDomainID);
-        (sleToken && (sleToken->getFlags() & lsfMPTDomainCheck));
+    bool const authorizedByDomain = maybeDomainID.has_value() &&
-
+        verifyValidDomain(view, account, *maybeDomainID, j) == tesSUCCESS;
-    bool authorizedByDomain = false;
-    if (domainOwned || sleToken == nullptr)
-    {
-        // We check DomainID if:
-        //
-        // 1. Token not found or
-        // 2. Token found and has lsfMPTDomainCheck flag
-        //
-        // In case 1. we check authorization in order to create the token
-        // (below), but only if the account is authorized by the domain. In
-        // case 2. we re-check authorization in case the credentials are
-        // expired, in which case the token needs to be unauthorized (below)
-
-        auto const maybeDomainID = sleIssuance->at(~sfDomainID);
-        authorizedByDomain = maybeDomainID.has_value() &&
-            verifyValidDomain(view, account, *maybeDomainID, j) == tesSUCCESS;
-    }
 
     if (!authorizedByDomain && sleToken == nullptr)
     {
-        // Intentionally empty. This could be either of:
+        // Could not find MPToken and won't create one, could be either of:
         //
         // 1. Field sfDomainID not set in MPTokenIssuance or
         // 2. Account has no matching and accepted credentials or
-        // 3. Account has all expired credentials (removed in verifyValidDomain)
+        // 3. Account has all expired credentials (deleted in verifyValidDomain)
         //
-        // Either way, will return tecNO_AUTH at the end of this function
+        // Either way, return tecNO_AUTH and there is nothing else to do
+        return tecNO_AUTH;
     }
-    else if (!authorizedByDomain && domainOwned)
+    else if (!authorizedByDomain && maybeDomainID.has_value())
     {
-        // We found an MPToken with lsfMPTDomainCheck flag, but the account is
+        // Found an MPToken but the account is not authorized and we expect
-        // no longer authorized.
+        // it to have been authorized by the domain. This could be because the
-        if (sleToken->getFlags() & lsfMPTAuthorized)
+        // credentials used to create the MPToken have expired or been deleted.
-        {
+        //
-            // Must reset lsfMPTAuthorized, no current credentials
+        // Delete the expired MPToken but only if it has zero balance, then
-            auto sleMpt = view.peek(keylet);
+        // return tecNO_AUTH
-            XRPL_ASSERT(sleMpt, "ripple::verifyAuth : non-null bad MPToken");
-            sleMpt->clearFlag(lsfMPTAuthorized);
-            view.update(sleMpt);
 
-            sleToken = sleMpt;  // without lsfMPTAuthorized
+        auto sleMpt = view.peek(keylet);
+        auto sleAcct = view.peek(keylet::account(account));
+
+        XRPL_ASSERT(
+            sleMpt != nullptr && sleAcct != nullptr,
+            "ripple::enforceMPTokenAuthorization : found expired MPToken");
+        if ((*sleMpt)[sfMPTAmount] == 0)
+        {
+            if (!view.dirRemove(
+                    keylet::ownerDir(account),
+                    (*sleMpt)[sfOwnerNode],
+                    sleMpt->key(),
+                    false))
+                return tecINTERNAL;
+
+            adjustOwnerCount(view, sleAcct, -1, j);
+            view.erase(sleMpt);
         }
+
+        return tecNO_AUTH;
     }
     else if (!authorizedByDomain)
     {
+        // We found an MPToken, but sfDomainID is not set, so this is a classic
+        // MPToken which requires authorization by the token issuer.
         XRPL_ASSERT(
-            sleToken != nullptr && !domainOwned,
+            sleToken != nullptr && !maybeDomainID.has_value(),
-            "ripple::verifyAuth : MPToken not owned by domain");
+            "ripple::enforceMPTokenAuthorization : found MPToken");
-        // MPToken was created by other means, we will check its authorization
+        if (sleToken->getFlags() & lsfMPTAuthorized)
-        // at the end of this function. No need to do anything here.
+            return tesSUCCESS;
+
+        return tecNO_AUTH;
     }
     else if (authorizedByDomain && sleToken != nullptr)
     {
+        // Found an MPToken, authorized by the domain. Ignore authorization flag
+        // lsfMPTAuthorized because it is meaningless. Return tesSUCCESS
         XRPL_ASSERT(
-            domainOwned, "ripple::verifyAuth : MPToken owned by domain");
+            maybeDomainID.has_value(),
-
+            "ripple::enforceMPTokenAuthorization : found MPToken for domain");
-        if ((sleToken->getFlags() & lsfMPTAuthorized) == 0)
+        return tesSUCCESS;
-        {
-            // Must set lsfMPTAuthorized, we found new credentials
-            auto sleMpt = view.peek(keylet);
-            XRPL_ASSERT(sleMpt, "ripple::verifyAuth : non-null good MPToken");
-            sleMpt->setFlag(lsfMPTAuthorized);
-            view.update(sleMpt);
-
-            sleToken = sleMpt;  // with lsfMPTAuthorized
-        }
     }
-    else if (authorizedByDomain && sleToken == nullptr)
+    else if (authorizedByDomain)
     {
-        // Create MPToken with the lsfMPTDomainCheck flag set
+        // Could not find MPToken but there should be one because we are
+        // authorized by domain. Proceed to create it, then return tesSUCCESS
+        XRPL_ASSERT(
+            maybeDomainID.has_value() && sleToken == nullptr,
+            "ripple::enforceMPTokenAuthorization : new MPToken for domain");
         if (auto const err = MPTokenAuthorize::authorize(
                 view,
                 j,
@@ -2339,18 +2340,12 @@ enforceMPTokenAuthorization(
             !isTesSuccess(err))
             return err;
 
-        auto sleMpt = view.peek(keylet);
+        return tesSUCCESS;
-        XRPL_ASSERT(sleMpt, "ripple::verifyAuth : non-null new MPToken");
-        sleMpt->setFlag(lsfMPTDomainCheck | lsfMPTAuthorized);
-        view.update(sleMpt);
-
-        sleToken = sleMpt;  // with lsfMPTAuthorized
     }
 
-    if (sleToken == nullptr || (sleToken->getFlags() & lsfMPTAuthorized) == 0)
+    UNREACHABLE(
-        return tecNO_AUTH;
+        "ripple::enforceMPTokenAuthorization : condition list is incomplete");
-
+    return tefINTERNAL;
-    return tesSUCCESS;
 }
 
 TER