From f56e2a9dc0e3b916f39bbb0fa64ea25b7fc3ca5b Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Wed, 30 Sep 2026 16:50:59 +0530 Subject: [PATCH] refactor: Use DelegateEntry everywhere Migrate every read/peek/make_shared/insert/update/erase of a Delegate ledger entry in DelegateSet.cpp, AccountDelete.cpp, and Transactor.cpp to go through DelegateEntryR/DelegateEntryW instead of raw SLE pointers. Also move checkTxPermission()/getGranularPermission() (formerly free functions in DelegateHelpers.h/DelegateUtils.cpp) and the delegate deletion logic (formerly DelegateSet::deleteDelegate, called from AccountDelete's deleter table) onto DelegateEntry as checkTxPermission(), granularPermission(), and removeFromLedger(). DelegateHelpers.h and DelegateUtils.cpp are now empty and deleted. --- include/xrpl/ledger/entries/DelegateEntry.h | 39 ++++++ include/xrpl/ledger/helpers/DelegateHelpers.h | 34 ------ .../tx/transactors/delegate/DelegateSet.h | 4 - src/libxrpl/ledger/entries/DelegateEntry.cpp | 113 ++++++++++++++++++ src/libxrpl/tx/Transactor.cpp | 10 +- .../tx/transactors/account/AccountDelete.cpp | 11 +- .../tx/transactors/delegate/DelegateSet.cpp | 76 +++--------- .../tx/transactors/delegate/DelegateUtils.cpp | 52 -------- src/test/app/Delegate_test.cpp | 13 +- 9 files changed, 188 insertions(+), 164 deletions(-) delete mode 100644 include/xrpl/ledger/helpers/DelegateHelpers.h create mode 100644 src/libxrpl/ledger/entries/DelegateEntry.cpp delete mode 100644 src/libxrpl/tx/transactors/delegate/DelegateUtils.cpp diff --git a/include/xrpl/ledger/entries/DelegateEntry.h b/include/xrpl/ledger/entries/DelegateEntry.h index de908d4dcd..e126a2cb60 100644 --- a/include/xrpl/ledger/entries/DelegateEntry.h +++ b/include/xrpl/ledger/entries/DelegateEntry.h @@ -7,9 +7,16 @@ #include #include #include +#include +#include +#include + +#include namespace xrpl { +class STTx; + template class DelegateEntry : public SLEBase { @@ -28,6 +35,38 @@ 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 + 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. + */ + TER + removeFromLedger(AccountID const& owner) + requires Base::kIsWritable; }; using DelegateEntryR = DelegateEntry; diff --git a/include/xrpl/ledger/helpers/DelegateHelpers.h b/include/xrpl/ledger/helpers/DelegateHelpers.h deleted file mode 100644 index f602bec22e..0000000000 --- a/include/xrpl/ledger/helpers/DelegateHelpers.h +++ /dev/null @@ -1,34 +0,0 @@ -#pragma once - -#include -#include -#include -#include -#include - -#include - -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 -getGranularPermission(SLE::ConstRef delegate, TxType const& type); - -} // namespace xrpl diff --git a/include/xrpl/tx/transactors/delegate/DelegateSet.h b/include/xrpl/tx/transactors/delegate/DelegateSet.h index 374bd2f6bf..181d019baa 100644 --- a/include/xrpl/tx/transactors/delegate/DelegateSet.h +++ b/include/xrpl/tx/transactors/delegate/DelegateSet.h @@ -40,10 +40,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 diff --git a/src/libxrpl/ledger/entries/DelegateEntry.cpp b/src/libxrpl/ledger/entries/DelegateEntry.cpp new file mode 100644 index 0000000000..8e69abf8b2 --- /dev/null +++ b/src/libxrpl/ledger/entries/DelegateEntry.cpp @@ -0,0 +1,113 @@ +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include // IWYU pragma: keep +#include +#include +#include + +#include + +namespace xrpl { + +template +NotTEC +DelegateEntry::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 +std::unordered_set +DelegateEntry::granularPermission(TxType const& type) const +{ + std::unordered_set 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(permissionValue); + auto const& permType = Permission::getInstance().getGranularTxType(granularValue); + if (permType && *permType == type) + granularPermissions.insert(granularValue); + } + + return granularPermissions; +} + +template +TER +DelegateEntry::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; +template class DelegateEntry; + +} // namespace xrpl diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index 09725370e9..4c2630d9c3 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -11,9 +11,9 @@ #include // IWYU pragma: keep #include #include +#include #include #include -#include #include #include #include @@ -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; diff --git a/src/libxrpl/tx/transactors/account/AccountDelete.cpp b/src/libxrpl/tx/transactors/account/AccountDelete.cpp index 991af69bff..745ff7186d 100644 --- a/src/libxrpl/tx/transactors/account/AccountDelete.cpp +++ b/src/libxrpl/tx/transactors/account/AccountDelete.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -26,7 +27,6 @@ #include #include #include -#include #include #include #include @@ -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 diff --git a/src/libxrpl/tx/transactors/delegate/DelegateSet.cpp b/src/libxrpl/tx/transactors/delegate/DelegateSet.cpp index afded198ba..8741ce6a3a 100644 --- a/src/libxrpl/tx/transactors/delegate/DelegateSet.cpp +++ b/src/libxrpl/tx/transactors/delegate/DelegateSet.cpp @@ -1,8 +1,8 @@ #include -#include #include #include +#include #include #include #include @@ -16,7 +16,6 @@ #include #include -#include #include 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(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; } diff --git a/src/libxrpl/tx/transactors/delegate/DelegateUtils.cpp b/src/libxrpl/tx/transactors/delegate/DelegateUtils.cpp deleted file mode 100644 index 70ae012e10..0000000000 --- a/src/libxrpl/tx/transactors/delegate/DelegateUtils.cpp +++ /dev/null @@ -1,52 +0,0 @@ -#include -#include -#include -#include -#include -#include -#include -#include - -#include - -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 -getGranularPermission(SLE::ConstRef delegate, TxType const& txType) -{ - std::unordered_set 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(permissionValue); - auto const& type = Permission::getInstance().getGranularTxType(granularValue); - if (type && *type == txType) - granularPermissions.insert(granularValue); - } - - return granularPermissions; -} - -} // namespace xrpl diff --git a/src/test/app/Delegate_test.cpp b/src/test/app/Delegate_test.cpp index bda89cdf98..a55f8a08f7 100644 --- a/src/test/app/Delegate_test.cpp +++ b/src/test/app/Delegate_test.cpp @@ -35,13 +35,14 @@ #include #include #include -#include +#include #include #include #include #include #include #include +#include #include #include #include @@ -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()); }