Document removeFromLedger return value

Add the missing @return tag describing the TER codes removeFromLedger
can return, per reviewer feedback on the doc-comment standard
established in PR #8369/#8368.

include/xrpl/ledger/entries/DelegateEntry.h

@@ -7,9 +7,16 @@
 #include <xrpl/protocol/AccountID.h>
 #include <xrpl/protocol/Indexes.h>
 #include <xrpl/protocol/LedgerFormats.h>
+#include <xrpl/protocol/Permissions.h>
+#include <xrpl/protocol/TER.h>
+#include <xrpl/protocol/TxFormats.h>
+
+#include <unordered_set>
 
 namespace xrpl {
 
+class STTx;
+
 template <typename ViewT>
 class DelegateEntry : public SLEBase<ViewT, ltDELEGATE>
 {
@@ -28,6 +35,41 @@ public:
         : Base(keylet::delegate(account, authorizedAccount), view, j)
     {
     }
+
+    /**
+     * Check if this delegate account has permission to execute the
+     * transaction.
+     * @param tx The transaction that the delegate account intends to
+     * execute.
+     * @return tesSUCCESS if the transaction is allowed,
+     * terNO_DELEGATE_PERMISSION if not.
+     */
+    [[nodiscard]] NotTEC
+    checkTxPermission(STTx const& tx) const;
+
+    /**
+     * Load the granular permissions granted to this delegate account for the
+     * specified transaction type.
+     * @param type Used to determine which granted granular permissions to
+     * load, based on the transaction type.
+     * @return the granted granular permissions tied to the transaction type.
+     */
+    [[nodiscard]] std::unordered_set<GranularPermissionType>
+    granularPermission(TxType const& type) const;
+
+    /**
+     * Removes this Delegate object from the ledger: unlinks it from both
+     * the delegating and (if present) the authorized account's owner
+     * directories, restores the delegating account's owner count, and
+     * erases the entry.
+     * @param owner The account whose owner directory entry led here.
+     * @return tesSUCCESS on success; tecINTERNAL if the entry or the
+     * delegating account's root does not exist; tefBAD_LEDGER if removal
+     * from an owner directory fails.
+     */
+    TER
+    removeFromLedger(AccountID const& owner)
+        requires Base::kIsWritable;
 };
 
 using DelegateEntryR = DelegateEntry<ReadView>;

include/xrpl/ledger/helpers/DelegateHelpers.h

@@ -1,34 +0,0 @@
-#pragma once
-
-#include <xrpl/protocol/Permissions.h>
-#include <xrpl/protocol/STLedgerEntry.h>
-#include <xrpl/protocol/STTx.h>
-#include <xrpl/protocol/TER.h>
-#include <xrpl/protocol/TxFormats.h>
-
-#include <unordered_set>
-
-namespace xrpl {
-
-/**
- * Check if the delegate account has permission to execute the transaction.
- * @param delegate The delegate account.
- * @param tx The transaction that the delegate account intends to execute.
- * @return tesSUCCESS if the transaction is allowed, terNO_DELEGATE_PERMISSION
- * if not.
- */
-NotTEC
-checkTxPermission(SLE::ConstRef delegate, STTx const& tx);
-
-/**
- * Load the granular permissions granted to the delegate account for the
- * specified transaction type
- * @param delegate The delegate account.
- * @param type Used to determine which granted granular permissions to load,
- * based on the transaction type.
- * @return the granted granular permissions tied to the transaction type.
- */
-std::unordered_set<GranularPermissionType>
-getGranularPermission(SLE::ConstRef delegate, TxType const& type);
-
-}  // namespace xrpl

include/xrpl/tx/transactors/delegate/DelegateSet.h

@@ -2,7 +2,6 @@
 
 #include <xrpl/beast/utility/Journal.h>
 #include <xrpl/core/ServiceRegistry.h>
-#include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/ReadView.h>
 #include <xrpl/protocol/STTx.h>
 #include <xrpl/protocol/TER.h>
@@ -40,10 +39,6 @@ public:
         XRPAmount fee,
         ReadView const& view,
         beast::Journal const& j) override;
-
-    // Interface used by AccountDelete
-    static TER
-    deleteDelegate(ApplyView& view, SLE::Ref sle, beast::Journal j);
 };
 
 }  // namespace xrpl

src/libxrpl/ledger/entries/DelegateEntry.cpp

@@ -0,0 +1,113 @@
+#include <xrpl/ledger/entries/DelegateEntry.h>
+
+#include <xrpl/basics/Log.h>
+#include <xrpl/ledger/ApplyView.h>
+#include <xrpl/ledger/ReadView.h>
+#include <xrpl/ledger/helpers/AccountRootHelpers.h>
+#include <xrpl/protocol/AccountID.h>
+#include <xrpl/protocol/Indexes.h>
+#include <xrpl/protocol/Permissions.h>
+#include <xrpl/protocol/SField.h>
+#include <xrpl/protocol/STArray.h>  // IWYU pragma: keep
+#include <xrpl/protocol/STTx.h>
+#include <xrpl/protocol/TER.h>
+#include <xrpl/protocol/TxFormats.h>
+
+#include <unordered_set>
+
+namespace xrpl {
+
+template <typename ViewT>
+NotTEC
+DelegateEntry<ViewT>::checkTxPermission(STTx const& tx) const
+{
+    if (!this->exists())
+        return terNO_DELEGATE_PERMISSION;
+
+    auto const& sle = this->operator*();
+    auto const permissionArray = sle.getFieldArray(sfPermissions);
+    auto const txPermission = tx.getTxnType() + 1;
+
+    for (auto const& permission : permissionArray)
+    {
+        auto const permissionValue = permission[sfPermissionValue];
+        if (permissionValue == txPermission)
+            return tesSUCCESS;
+    }
+
+    return terNO_DELEGATE_PERMISSION;
+}
+
+template <typename ViewT>
+std::unordered_set<GranularPermissionType>
+DelegateEntry<ViewT>::granularPermission(TxType const& type) const
+{
+    std::unordered_set<GranularPermissionType> granularPermissions;
+    if (!this->exists())
+        return granularPermissions;
+
+    auto const& sle = this->operator*();
+    auto const permissionArray = sle.getFieldArray(sfPermissions);
+    for (auto const& permission : permissionArray)
+    {
+        auto const permissionValue = permission[sfPermissionValue];
+        auto const granularValue = static_cast<GranularPermissionType>(permissionValue);
+        auto const& permType = Permission::getInstance().getGranularTxType(granularValue);
+        if (permType && *permType == type)
+            granularPermissions.insert(granularValue);
+    }
+
+    return granularPermissions;
+}
+
+template <typename ViewT>
+TER
+DelegateEntry<ViewT>::removeFromLedger(AccountID const&)
+    requires Base::kIsWritable
+{
+    if (!this->exists())
+        return tecINTERNAL;  // LCOV_EXCL_LINE
+
+    auto& sle = this->operator*();
+    auto const delegator = sle[sfAccount];
+    auto const delegatee = sle[sfAuthorize];
+
+    // Remove from delegating account's owner directory
+    if (!this->applyView().dirRemove(
+            keylet::ownerDir(delegator), sle[sfOwnerNode], this->key(), false))
+    {
+        // LCOV_EXCL_START
+        JLOG(this->journal().fatal()) << "Unable to delete Delegate from owner.";
+        return tefBAD_LEDGER;
+        // LCOV_EXCL_STOP
+    }
+
+    // Remove from authorized account's owner directory, if present
+    if (auto const optPage = sle[~sfDestinationNode])
+    {
+        if (!this->applyView().dirRemove(keylet::ownerDir(delegatee), *optPage, this->key(), false))
+        {
+            // LCOV_EXCL_START
+            JLOG(this->journal().fatal()) << "Unable to delete Delegate from authorized account.";
+            return tefBAD_LEDGER;
+            // LCOV_EXCL_STOP
+        }
+    }
+
+    // Only the delegating account's owner count was incremented on creation
+    auto const sleOwner = this->applyView().peek(keylet::account(delegator));
+    if (!sleOwner)
+        return tecINTERNAL;  // LCOV_EXCL_LINE
+
+    decreaseOwnerCountForObject(
+        this->applyView(), sleOwner, this->mutableRawSle(), 1, this->journal());
+
+    this->erase();
+
+    return tesSUCCESS;
+}
+
+template class DelegateEntry<ReadView>;
+template class DelegateEntry<ApplyView>;
+
+}  // namespace xrpl

src/libxrpl/tx/Transactor.cpp

@@ -11,9 +11,9 @@
 #include <xrpl/json/to_string.h>  // IWYU pragma: keep
 #include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/ReadView.h>
+#include <xrpl/ledger/entries/DelegateEntry.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
 #include <xrpl/ledger/helpers/CredentialHelpers.h>
-#include <xrpl/ledger/helpers/DelegateHelpers.h>
 #include <xrpl/ledger/helpers/NFTokenHelpers.h>
 #include <xrpl/ledger/helpers/OfferHelpers.h>
 #include <xrpl/ledger/helpers/RippleStateHelpers.h>
@@ -388,17 +388,17 @@ Transactor::checkPermission(
     if (!delegate)
         return tesSUCCESS;
 
-    auto const sle = view.read(keylet::delegate(tx[sfAccount], *delegate));
-    if (!sle)
+    DelegateEntryR const delegateEntry(tx[sfAccount], *delegate, view);
+    if (!delegateEntry)
         return terNO_DELEGATE_PERMISSION;
 
-    if (isTesSuccess(checkTxPermission(sle, tx)))
+    if (isTesSuccess(delegateEntry.checkTxPermission(tx)))
         return tesSUCCESS;
 
     if (!Permission::getInstance().hasGranularPermissions(tx.getTxnType()))
         return terNO_DELEGATE_PERMISSION;
 
-    heldGranularPermissions = getGranularPermission(sle, tx.getTxnType());
+    heldGranularPermissions = delegateEntry.granularPermission(tx.getTxnType());
     if (heldGranularPermissions.empty())
         return terNO_DELEGATE_PERMISSION;
 

src/libxrpl/tx/transactors/account/AccountDelete.cpp

@@ -9,6 +9,7 @@
 #include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/ReadView.h>
 #include <xrpl/ledger/View.h>
+#include <xrpl/ledger/entries/DelegateEntry.h>
 #include <xrpl/ledger/helpers/CredentialHelpers.h>
 #include <xrpl/ledger/helpers/DirectoryHelpers.h>
 #include <xrpl/ledger/helpers/NFTokenHelpers.h>
@@ -26,7 +27,6 @@
 #include <xrpl/protocol/XRPAmount.h>
 #include <xrpl/tx/Transactor.h>
 #include <xrpl/tx/transactors/account/SignerListSet.h>
-#include <xrpl/tx/transactors/delegate/DelegateSet.h>
 #include <xrpl/tx/transactors/did/DIDDelete.h>
 #include <xrpl/tx/transactors/oracle/OracleDelete.h>
 #include <xrpl/tx/transactors/payment/DepositPreauth.h>
@@ -177,12 +177,13 @@ TER
 removeDelegateFromLedger(
     ServiceRegistry&,
     ApplyView& view,
-    AccountID const&,
-    UInt256 const&,
-    SLE::Ref sleDel,
+    AccountID const& account,
+    UInt256 const& delIndex,
+    SLE::Ref,
     beast::Journal j)
 {
-    return DelegateSet::deleteDelegate(view, sleDel, j);
+    DelegateEntryW entry(Keylet(ltDELEGATE, delIndex), view, j);
+    return entry.removeFromLedger(account);
 }
 
 // Return nullptr if the LedgerEntryType represents an obligation that can't

src/libxrpl/tx/transactors/delegate/DelegateSet.cpp

@@ -1,8 +1,8 @@
 #include <xrpl/tx/transactors/delegate/DelegateSet.h>
 
-#include <xrpl/basics/Log.h>
 #include <xrpl/beast/utility/Journal.h>
 #include <xrpl/core/ServiceRegistry.h>
+#include <xrpl/ledger/entries/DelegateEntry.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
 #include <xrpl/ledger/helpers/DirectoryHelpers.h>
 #include <xrpl/ledger/helpers/SponsorHelpers.h>
@@ -16,7 +16,6 @@
 #include <xrpl/tx/Transactor.h>
 
 #include <cstdint>
-#include <memory>
 #include <unordered_set>
 
 namespace xrpl {
@@ -77,20 +76,19 @@ DelegateSet::doApply()
         return tefINTERNAL;  // LCOV_EXCL_LINE
 
     auto const& authAccount = ctx_.tx[sfAuthorize];
-    auto const delegateKey = keylet::delegate(accountID_, authAccount);
+    DelegateEntryW entry(accountID_, authAccount, ctx_.view(), j_);
 
-    auto sle = ctx_.view().peek(delegateKey);
-    if (sle)
+    if (entry.exists())
     {
         auto const& permissions = ctx_.tx.getFieldArray(sfPermissions);
         if (permissions.empty())
         {
             // if permissions array is empty, delete the ledger object.
-            return deleteDelegate(view(), sle, j_);
+            return entry.removeFromLedger(accountID_);
         }
 
-        sle->setFieldArray(sfPermissions, permissions);
-        ctx_.view().update(sle);
+        entry->setFieldArray(sfPermissions, permissions);
+        entry.update();
         return tesSUCCESS;
     }
 
@@ -107,76 +105,34 @@ DelegateSet::doApply()
         !isTesSuccess(ret))
         return ret;
 
-    sle = std::make_shared<SLE>(delegateKey);
-    sle->setAccountID(sfAccount, accountID_);
-    sle->setAccountID(sfAuthorize, authAccount);
+    entry.newSLE();
+    entry->setAccountID(sfAccount, accountID_);
+    entry->setAccountID(sfAuthorize, authAccount);
 
-    sle->setFieldArray(sfPermissions, permissions);
+    entry->setFieldArray(sfPermissions, permissions);
 
     // Add to delegating account's owner directory
     auto const page = ctx_.view().dirInsert(
-        keylet::ownerDir(accountID_), delegateKey, describeOwnerDir(accountID_));
+        keylet::ownerDir(accountID_), entry.keylet(), describeOwnerDir(accountID_));
 
     if (!page)
         return tecDIR_FULL;  // LCOV_EXCL_LINE
 
-    (*sle)[sfOwnerNode] = *page;
+    (*entry)[sfOwnerNode] = *page;
 
     // Add to authorized account's owner directory so AccountDelete can find
     // and clean up inbound delegations when the authorized account is deleted.
     auto const destPage = ctx_.view().dirInsert(
-        keylet::ownerDir(authAccount), delegateKey, describeOwnerDir(authAccount));
+        keylet::ownerDir(authAccount), entry.keylet(), describeOwnerDir(authAccount));
 
     if (!destPage)
         return tecDIR_FULL;  // LCOV_EXCL_LINE
 
-    (*sle)[sfDestinationNode] = *destPage;
+    (*entry)[sfDestinationNode] = *destPage;
 
-    ctx_.view().insert(sle);
+    entry.insert();
     increaseOwnerCount(ctx_.getApplyViewContext(), sleOwner, 1, ctx_.journal);
-    addSponsorToLedgerEntry(ctx_.getApplyViewContext(), sle);
-
-    return tesSUCCESS;
-}
-
-TER
-DelegateSet::deleteDelegate(ApplyView& view, SLE::Ref sle, beast::Journal j)
-{
-    if (!sle)
-        return tecINTERNAL;  // LCOV_EXCL_LINE
-
-    auto const delegator = (*sle)[sfAccount];
-    auto const delegatee = (*sle)[sfAuthorize];
-
-    // Remove from delegating account's owner directory
-    if (!view.dirRemove(keylet::ownerDir(delegator), (*sle)[sfOwnerNode], sle->key(), false))
-    {
-        // LCOV_EXCL_START
-        JLOG(j.fatal()) << "Unable to delete Delegate from owner.";
-        return tefBAD_LEDGER;
-        // LCOV_EXCL_STOP
-    }
-
-    // Remove from authorized account's owner directory, if present
-    if (auto const optPage = (*sle)[~sfDestinationNode])
-    {
-        if (!view.dirRemove(keylet::ownerDir(delegatee), *optPage, sle->key(), false))
-        {
-            // LCOV_EXCL_START
-            JLOG(j.fatal()) << "Unable to delete Delegate from authorized account.";
-            return tefBAD_LEDGER;
-            // LCOV_EXCL_STOP
-        }
-    }
-
-    // Only the delegating account's owner count was incremented on creation
-    auto const sleOwner = view.peek(keylet::account(delegator));
-    if (!sleOwner)
-        return tecINTERNAL;  // LCOV_EXCL_LINE
-
-    decreaseOwnerCountForObject(view, sleOwner, sle, 1, j);
-
-    view.erase(sle);
+    addSponsorToLedgerEntry(ctx_.getApplyViewContext(), entry.mutableRawSle());
 
     return tesSUCCESS;
 }

src/libxrpl/tx/transactors/delegate/DelegateUtils.cpp

@@ -1,52 +0,0 @@
-#include <xrpl/ledger/helpers/DelegateHelpers.h>
-#include <xrpl/protocol/Permissions.h>
-#include <xrpl/protocol/SField.h>
-#include <xrpl/protocol/STArray.h>
-#include <xrpl/protocol/STLedgerEntry.h>
-#include <xrpl/protocol/STTx.h>
-#include <xrpl/protocol/TER.h>
-#include <xrpl/protocol/TxFormats.h>
-
-#include <unordered_set>
-
-namespace xrpl {
-NotTEC
-checkTxPermission(SLE::ConstRef delegate, STTx const& tx)
-{
-    if (!delegate)
-        return terNO_DELEGATE_PERMISSION;
-
-    auto const permissionArray = delegate->getFieldArray(sfPermissions);
-    auto const txPermission = tx.getTxnType() + 1;
-
-    for (auto const& permission : permissionArray)
-    {
-        auto const permissionValue = permission[sfPermissionValue];
-        if (permissionValue == txPermission)
-            return tesSUCCESS;
-    }
-
-    return terNO_DELEGATE_PERMISSION;
-}
-
-std::unordered_set<GranularPermissionType>
-getGranularPermission(SLE::ConstRef delegate, TxType const& txType)
-{
-    std::unordered_set<GranularPermissionType> granularPermissions;
-    if (!delegate)
-        return granularPermissions;
-
-    auto const permissionArray = delegate->getFieldArray(sfPermissions);
-    for (auto const& permission : permissionArray)
-    {
-        auto const permissionValue = permission[sfPermissionValue];
-        auto const granularValue = static_cast<GranularPermissionType>(permissionValue);
-        auto const& type = Permission::getInstance().getGranularTxType(granularValue);
-        if (type && *type == txType)
-            granularPermissions.insert(granularValue);
-    }
-
-    return granularPermissions;
-}
-
-}  // namespace xrpl

src/test/app/Delegate_test.cpp

@@ -35,13 +35,14 @@
 #include <xrpl/json/json_value.h>
 #include <xrpl/json/to_string.h>
 #include <xrpl/ledger/Dir.h>
-#include <xrpl/ledger/helpers/DelegateHelpers.h>
+#include <xrpl/ledger/entries/DelegateEntry.h>
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Indexes.h>
 #include <xrpl/protocol/Issue.h>
 #include <xrpl/protocol/KeyType.h>
 #include <xrpl/protocol/Permissions.h>
 #include <xrpl/protocol/SField.h>
+#include <xrpl/protocol/STLedgerEntry.h>
 #include <xrpl/protocol/STObject.h>
 #include <xrpl/protocol/STTx.h>
 #include <xrpl/protocol/SecretKey.h>
@@ -2942,12 +2943,16 @@ class Delegate_test : public beast::unit_test::Suite
     {
         testcase("DelegateUtils nullptr check");
 
+        using namespace jtx;
+        Env const env(*this);
+        DelegateEntryR const delegate(SLE::const_pointer(), *env.current());
+
         // checkTxPermission nullptr check
         STTx const tx{ttPAYMENT, [](STObject&) {}};
-        BEAST_EXPECT(checkTxPermission(nullptr, tx) == terNO_DELEGATE_PERMISSION);
+        BEAST_EXPECT(delegate.checkTxPermission(tx) == terNO_DELEGATE_PERMISSION);
 
-        // getGranularPermission nullptr check
-        auto const granularPermissions = getGranularPermission(nullptr, ttPAYMENT);
+        // granularPermission nullptr check
+        auto const granularPermissions = delegate.granularPermission(ttPAYMENT);
         BEAST_EXPECT(granularPermissions.empty());
     }
 

src/tests/libxrpl/ledger/SLEBase.cpp

@@ -38,6 +38,7 @@
 #include <xrpl/ledger/entries/XChainOwnedClaimIDEntry.h>               // IWYU pragma: keep
 #include <xrpl/ledger/entries/XChainOwnedCreateAccountClaimIDEntry.h>  // IWYU pragma: keep
 #include <xrpl/protocol/Indexes.h>
+#include <xrpl/protocol/Keylet.h>
 #include <xrpl/protocol/LedgerFormats.h>
 #include <xrpl/protocol/SField.h>
 #include <xrpl/protocol/STLedgerEntry.h>
@@ -76,12 +77,22 @@ template class SLEBase<ApplyView>;
 #pragma push_macro("LEDGER_ENTRY")
 #undef LEDGER_ENTRY
 
+// No explicit instantiation here, neither a definition nor a declaration.
+// A definition would instantiate an entry a second time once its member
+// bodies live in its own .cpp (MSVC rejects that with C4661 under /WX), and a
+// declaration (`extern template`) stops clang from emitting the inline
+// constructors of the header-only entries at -O0, which leaves them undefined
+// at link time in the debug builds. Naming the class in a static_assert is
+// enough to instantiate its definition, which is all this exhaustiveness
+// check needs; the per-type suites exercise the members.
 #define LEDGER_ENTRY(tag, value, name, ...)                                                    \
-    template class name##Entry<ReadView>;                                                      \
-    template class name##Entry<ApplyView>;                                                     \
     static_assert(                                                                             \
         name##Entry<ReadView>::kEntryType == tag && name##Entry<ApplyView>::kEntryType == tag, \
-        #name "Entry must be bound to " #tag);
+        #name "Entry must be bound to " #tag);                                                 \
+    static_assert(                                                                             \
+        std::is_constructible_v<name##Entry<ReadView>, Keylet const&, ReadView const&> &&      \
+            std::is_constructible_v<name##Entry<ApplyView>, Keylet const&, ApplyView&>,        \
+        #name "Entry must be constructible from a Keylet and a view");
 
 #include <xrpl/protocol/detail/ledger_entries.macro>