refactor: Add initial wrapper classes for all SLEs by mvadari · Pull Request #7886 · XRPLF/rippled

mvadari · 2026-07-28T15:52:24Z

High Level Overview of Change

Title pretty much says it all - this PR adds some wrapper classes for all ledger entry types, intended to replace the use of raw SLEs.

Context of Change

A production-grade Step 1 version of #7791

API Impact

N/A

codecov · 2026-07-28T16:39:05Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copilot

Pull request overview

This PR introduces a new SLEBase<ViewT> template and a set of thin, per-ledger-entry wrapper classes (e.g., AccountRootEntry<ViewT>, OfferEntry<ViewT>) intended as the first step toward replacing direct use of raw STLedgerEntry/SLE across the codebase.

Changes:

Added include/xrpl/ledger/helpers/SLEBase.h, providing a view-parameterized wrapper with compile-time gated writable operations.
Added initial wrapper headers for a broad set of ledger entry (SLE) types, each providing a convenience constructor that resolves the entry from a Keylet/keylet helper against a view.
Established ReadOnlySLE and WritableSLE aliases as generic wrappers for unknown concrete ledger entry types.

Reviewed changes

Copilot reviewed 32 out of 32 changed files in this pull request and generated 32 comments.

Show a summary per file

File	Description
include/xrpl/ledger/helpers/SLEBase.h	Introduces the core view-parameterized base wrapper for SLE access/mutation.
include/xrpl/ledger/helpers/AccountRootEntry.h	Adds an `AccountRoot` SLE wrapper with a `keylet::account` constructor.
include/xrpl/ledger/helpers/AMMEntry.h	Adds an `AMM` SLE wrapper with a `keylet::amm` constructor.
include/xrpl/ledger/helpers/AmendmentsEntry.h	Adds an `Amendments` SLE wrapper with a `keylet::amendments` constructor.
include/xrpl/ledger/helpers/BridgeEntry.h	Adds a `Bridge` SLE wrapper with a `keylet::bridge` constructor.
include/xrpl/ledger/helpers/CheckEntry.h	Adds a `Check` SLE wrapper with a `keylet::check` constructor.
include/xrpl/ledger/helpers/CredentialEntry.h	Adds a `Credential` SLE wrapper with a `keylet::credential` constructor.
include/xrpl/ledger/helpers/DelegateEntry.h	Adds a `Delegate` SLE wrapper with a `keylet::delegate` constructor.
include/xrpl/ledger/helpers/DepositPreauthEntry.h	Adds a `DepositPreauth` SLE wrapper with a `keylet::depositPreauth` constructor.
include/xrpl/ledger/helpers/DIDEntry.h	Adds a `DID` SLE wrapper with a `keylet::did` constructor.
include/xrpl/ledger/helpers/DirectoryNodeEntry.h	Adds a `DirectoryNode` SLE wrapper with a `keylet::ownerDir` constructor.
include/xrpl/ledger/helpers/EscrowEntry.h	Adds an `Escrow` SLE wrapper with a `keylet::escrow` constructor.
include/xrpl/ledger/helpers/FeeSettingsEntry.h	Adds a `FeeSettings` SLE wrapper with a `keylet::feeSettings` constructor.
include/xrpl/ledger/helpers/LedgerHashesEntry.h	Adds a `LedgerHashes/SkipList` SLE wrapper with a `keylet::skip` constructor.
include/xrpl/ledger/helpers/LoanBrokerEntry.h	Adds a `LoanBroker` SLE wrapper with a `keylet::loanBroker` constructor.
include/xrpl/ledger/helpers/LoanEntry.h	Adds a `Loan` SLE wrapper with a `keylet::loan` constructor.
include/xrpl/ledger/helpers/MPTokenEntry.h	Adds an `MPToken` SLE wrapper with a `keylet::mptoken` constructor.
include/xrpl/ledger/helpers/MPTokenIssuanceEntry.h	Adds an `MPTokenIssuance` SLE wrapper with a `keylet::mptokenIssuance` constructor.
include/xrpl/ledger/helpers/NegativeUNLEntry.h	Adds a `NegativeUNL` SLE wrapper with a `keylet::negativeUNL` constructor.
include/xrpl/ledger/helpers/NFTokenOfferEntry.h	Adds an `NFTokenOffer` SLE wrapper with a `keylet::nftokenOffer` constructor.
include/xrpl/ledger/helpers/NFTokenPageEntry.h	Adds an `NFTokenPage` SLE wrapper with a `keylet::nftokenPage` constructor.
include/xrpl/ledger/helpers/OfferEntry.h	Adds an `Offer` SLE wrapper with a `keylet::offer` constructor.
include/xrpl/ledger/helpers/OracleEntry.h	Adds an `Oracle` SLE wrapper with a `keylet::oracle` constructor.
include/xrpl/ledger/helpers/PayChannelEntry.h	Adds a `PayChannel` SLE wrapper with a `keylet::payChannel` constructor.
include/xrpl/ledger/helpers/PermissionedDomainEntry.h	Adds a `PermissionedDomain` SLE wrapper with a `keylet::permissionedDomain` constructor.
include/xrpl/ledger/helpers/RippleStateEntry.h	Adds a `RippleState` SLE wrapper with a `keylet::trustLine` constructor.
include/xrpl/ledger/helpers/SignerListEntry.h	Adds a `SignerList` SLE wrapper with a `keylet::signerList` constructor.
include/xrpl/ledger/helpers/SponsorshipEntry.h	Adds a `Sponsorship` SLE wrapper with a `keylet::sponsorship` constructor.
include/xrpl/ledger/helpers/TicketEntry.h	Adds a `Ticket` SLE wrapper with a `keylet::ticket` constructor.
include/xrpl/ledger/helpers/VaultEntry.h	Adds a `Vault` SLE wrapper with a `keylet::vault` constructor.
include/xrpl/ledger/helpers/XChainOwnedClaimIDEntry.h	Adds an `XChainOwnedClaimID` SLE wrapper with a `keylet::xChainClaimID` constructor.
include/xrpl/ledger/helpers/XChainOwnedCreateAccountClaimIDEntry.h	Adds an `XChainOwnedCreateAccountClaimID` SLE wrapper with a `keylet::xChainCreateAccountClaimID` constructor.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

xrplf-ai-reviewer

Looks good to me.

…ri/rearch/init

github-actions · 2026-08-11T18:30:24Z

This PR has conflicts, please resolve them in order for the PR to be reviewed.

…ri/rearch/init

github-actions · 2026-08-11T18:34:49Z

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

xrplf-ai-reviewer

This PR adds a large, mechanically-generated family of typed SLE wrapper classes plus a shared SLEBase template. The pattern is applied consistently and looks careful (entry-type binding, const-correctness, converting-constructor safety asserts), but there is one build-breaking issue in the shared base class: a constructor references a type that is never declared or included anywhere in this diff or its transitive includes, and the accompanying test file explicitly instantiates the full class template, which will force this constructor to be compiled and fail.

xrplf-ai-reviewer

This PR adds a well-designed, type-safe wrapper layer (SLEBase + per-entry-type subclasses) for ledger entries, with thoughtful compile-time guarantees (requires-clauses gating write access, converting-constructor entry-type checks) and solid test coverage (explicit template instantiation for every entry type, static_asserts for illegal conversions, lifecycle tests for insert/erase/update). I went through every new header and cross-checked constructors against the corresponding keylet:: functions for argument order/type mismatches (a likely copy-paste failure mode given ~30 near-identical files) and found none. The read-only resolveEntry() const_cast/dynamic_cast trick to reach ApplyView::peek() is inherently delicate, but the author has already documented the exact safety argument and its limits in the code comment, so there's nothing new to add there. No confident correctness, security, or consistency issues found in the added lines.

Drop unused <cstdint> from the ledger entry wrapper headers and add the missing MPTAmount.h include to EscrowHelpers.h. SLEWrapper_test.cpp instantiates every wrapper through the LEDGER_ENTRY macro, which include-cleaner cannot see, so mark those includes with IWYU pragma: keep instead of removing them.

xrplf-ai-reviewer

Writable copy creates double ownership risk - see inline.

xrplf-ai-reviewer

This is a large, well-structured, and unusually well-documented refactor adding typed wrapper classes over raw SLEs, backed by a matching per-type test suite plus a macro-driven exhaustiveness check. I went through the SLEBase core (const-correctness gating via requires-clauses, the writable→read-only converting constructor and its entry-type guard, existence/type/keylet semantics) and every per-type header's constructor against its corresponding keylet:: call and test, specifically hunting for the classic copy-paste bug of transposed same-typed arguments (e.g. AccountID/AccountID pairs in Delegate/Sponsorship/DepositPreauth/PayChannel/Credential/RippleState) — none found; all match their tests. The one thing worth a second look is the const_cast + dynamic_cast trick in SLEBase.h's resolveEntry(), which the author already flagged with an extensive doc comment, but it's a global, load-bearing invariant for the whole read-only-entry abstraction so it seemed worth surfacing explicitly rather than letting a self-diagnosed risk pass silently.

detail::resolveEntry has two branches and only one was reachable from the tests. OpenView derives from ReadView but not ApplyView, so the dynamic_cast always failed for entries built over *env.current() and the peek() branch -- which carries the const_cast, its safety invariant, and the shared-SLE rationale -- had no coverage at all. Add a case that builds a read-only entry over an ApplyViewImpl, where the dynamic_cast succeeds, and assert what the peek path exists to guarantee: the entry shares the view's SLE rather than the base ledger's copy, so a write through any other entry over that view is visible through it. Verified by removing the branch, which fails exactly these three assertions. Also: - EntryTestEnv now holds the ledger apply() was built over instead of calling env.current() per access. Those could diverge if a suite ever closed a ledger, and holding it keeps the raw pointer inside av_ valid. - Drop expectKeylet's r.type() check. For a typed entry type() returns kEntryType, so it was the kEntryType assertion spelled twice.

xrplf-ai-reviewer

This is a large, well-structured addition of typed SLE wrapper classes plus a shared SLEBase template, with strong inline documentation and a solid, exhaustive test suite (macro-driven instantiation over all ledger entry types, keylet-argument-order regression checks, lifecycle tests). I didn't find copy-paste argument-order bugs across the many near-identical entry classes — the argument order passed to each keylet::* call consistently matches the corresponding accessor names. The one thing worth a second look is the const_cast/dynamic_cast trick in SLEBase.h used to reach ApplyView::peek() from a ReadView const&, which the author already flags as relying on an invariant that could become UB if violated elsewhere in the codebase.

OpenLedger::current() returns a copy of the shared_ptr it holds internally, so ApplyViewImpl av(&*env.current(), TapNone) survives only because the app keeps that reference. A close after the fact reassigns it, frees the OpenView, and leaves av dangling. No test does that today, but the five sites had nothing stopping one from being added. Bind the ledger to a local first. This does not make a later close correct -- av would then describe the pre-close ledger -- but it takes use-after-free off the table and matches what EntryTestEnv already does.

xrplf-ai-reviewer

This is a large, purely additive refactor that introduces a well-designed, view-parameterized SLEBase template plus one thin wrapper class per ledger entry type, along with a matching unit test for every new header. The core machinery (const/mutable dispatch via requires clauses, the writable→read-only converting constructor with entry-type gating, the resolveEntry peek-vs-read logic) is unusually thoroughly documented and exercised by SLEBase_test.cpp, including compile-time negative tests for illegal cross-entry-type conversions. I did not find correctness bugs in the constructors themselves — the various multi-overload entry classes (Check, Ticket, Offer, Vault, etc.) forward to the right keylet:: functions with no argument transposition, and this is independently checked by each *_test.cpp via expectKeylet. The one thing worth double-checking before merge is a possible missing direct include for MPTID/makeMptID in two of the new headers — the corresponding test files had to add <xrpl/protocol/MPTIssue.h> explicitly, which the production headers do not.

- namespace xrpl { namespace test { -> namespace xrpl::test, matching the rest of src/test (modernize-concat-nested-namespaces). - EntryTestEnv no longer holds its ApplyViewImpl in a std::optional. Funding moved to a private helper called from the member initializer list, so both members are initialized once and there is no unchecked optional access. - [[nodiscard]] on read(), apply() and someID(); const on the writable entry in expectKeylet and on fromView in SLEBase_test. - ApplyViewContext built with designated initializers. - Prune the includes the generated suites carried but never used. Applied via run-clang-tidy -export-fixes with the repo hook's path canonicalisation, then fix_include_style.py; UintTypes.h regrouped by hand.

xrplf-ai-reviewer

This MR adds a large, well-documented set of template wrapper classes (SLEBase + per-entry-type subclasses) around raw SLE access, plus matching unit tests. The individual per-type entry classes are mechanically consistent with their corresponding keylet:: functions and argument order, and the tests explicitly verify argument-order sensitivity where it matters (e.g. swapped AccountIDs producing different keys). The core SLEBase.h is carefully reasoned about ownership, const-correctness, and view semantics. The main issue found is a doc/implementation mismatch around XRPL_ASSERT's Release-build behavior repeated in three places; a secondary, lower-confidence note flags that writable entries are freely copy-constructible despite holding shared mutable state.

"raw" says what these hand back: the shared_ptr itself, rather than field access through operator-> / operator*. Doc comments now point at those for the common case. Renamed the test's detection concept to HasMutableRawSle to match. The sle_ member keeps its name.

xrplf-ai-reviewer

This is a large but mechanical PR that introduces a templated SLEBase wrapper plus ~30 per-entry-type subclasses (AccountRootEntry, OfferEntry, etc.) with matching unit tests for every type, including a macro-driven exhaustive instantiation check in SLEBase_test.cpp. The design is well thought out (compile-time read/write gating via requires, assert-only preconditions consistent with existing codebase conventions, careful member-declaration ordering to avoid dangling pointers in the test harness) and test coverage is thorough — every entry class has a dedicated test verifying its keylet resolution, and cross-argument-order collisions are explicitly checked where two arguments share a type. I didn't find correctness bugs in the ~30 generated entry classes; the one thing worth a second look is the const_cast + dynamic_cast trick in detail::resolveEntry, which the authors already flag as relying on a project-wide convention rather than anything the compiler enforces.

bthomee

Not done yet with the manual review, but here's some early feedback.

Also, given the value of this change, I think it would be better to put these files in include/xrpl/ledger/entries instead of in the helpers directory, so they are not mixed in with other helpers.

bthomee · 2026-08-13T20:34:32Z

+using RAccountRootEntry = AccountRootEntry<ReadView>;
+using WAccountRootEntry = AccountRootEntry<ApplyView>;


How would you feel about renaming this (and similarly, the other ones) to:

using AccountRootEntryRo = AccountRootEntry<ReadView>; using AccountRootEntryRw = AccountRootEntry<ApplyView>;

I think ro for read-onlyandrw for read-write are more standard. In particular, I prefer Ro and Rw over R and W resp. because the ApplyView is not write-only and can also be read from.

I also think that having the read vs. write indicators as suffixes is nicer than having them as prefixes; also note that the ViewT has the T as suffix and not as prefix, so having them as a suffix would be consistent with other uses.

Interesting, those suffixes feel more confusing to me (not opposed to the renaming in general, just ro and rw don't really have meaning to me). What about sticking to R and W but making them suffixes?

That would be fine with me. Let's get another reviewer's opinion on this as well though before making the change.

bthomee · 2026-08-13T20:53:56Z

+ *       side effects: peek() installs an Action::Cache entry in the apply
+ *       state table. That is benign for transaction metadata -- Cache entries
+ *       are skipped in ApplyStateTable::apply(), ::visit() and in metadata
+ *       generation -- but it does cost one deep SLE copy on first touch.


Is there a performance impact expected from this change, or is it no different from what is currently done?

The SLE wrappers are going to make everything much safer, but if there's a likely performance impact we should quantify it first.

bthomee · 2026-08-13T21:06:04Z

+    STLedgerEntry const*
+    operator->() const
+    {
+        XRPL_ASSERT(exists(), "xrpl::SLEBase::operator-> : exists");
+        return sle_.get();
+    }
+
+    STLedgerEntry const&
+    operator*() const
+    {
+        XRPL_ASSERT(exists(), "xrpl::SLEBase::operator* : exists");
+        return *sle_;
+    }


In release mode the asserts will be compiled away. Would there be any benefit from adding protection in release mode if exists() is false, e.g. returning a dummy entry that otherwise does nothing? On the one hand it could be better than crashing hard, but on the other hand if we get to this state things may be messed up so badly that crashing is the least-worst option.

bthomee · 2026-08-13T21:08:38Z

+    canModify() const
+        requires kIsWritable
+    {
+        return sle_ != nullptr;


Suggested change

return sle_ != nullptr;

return exists();

It's fine as it is, but you also did this in the operator bool() const so may as well for consistency.

FWIW I think that you may as well remove the canModify function completely. It's the same as exists() and in places where you already used it, it's used as an alias: XRPL_ASSERT(!canModify(), "xrpl::SLEBase::newSLE : no existing SLE");.

The doc comment also describes a different property than the body checks: "Returns true if this entry supports write operations" but all that happens here is that it's checked if the sle is valid.

Whether the entry supports write operations is already determined by kIsWritable, which is a compile-time property of the type. A read-only entry won't even have this function.

If the intent really were "may I write?" then something more useful would be return kIsWritable && sle_ != nullptr;, in which case the function actually means something different than exists().

refactor: Add wrapper classes for all SLEs

f34585d

mvadari requested review from a1q123456, bthomee and ximinez July 28, 2026 15:52

mvadari added 2 commits July 28, 2026 11:59

fix clang-tidy issues

cb90ba0

fix clang-tidy (hopefully)

1a151c8

mvadari changed the title ~~refactor: Add wrapper classes for all SLEs~~ refactor: Add initial wrapper classes for all SLEs Jul 28, 2026

bthomee requested a review from Copilot July 28, 2026 17:31

Copilot started reviewing on behalf of bthomee July 28, 2026 17:31 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

respond to comments

fb809c2

This comment was marked as resolved.

Sign in to view

mvadari and others added 2 commits July 30, 2026 13:00

add stuff from downstream

9aed87e

Merge branch 'develop' into mvadari/rearch/init

bea1d0d

xrplf-ai-reviewer Bot approved these changes Jul 30, 2026

View reviewed changes

mvadari added 2 commits July 30, 2026 13:35

fix clang-tidy issues

1c39bec

fix clang-tidy weirdness

755b966

This comment was marked as resolved.

Sign in to view

mvadari added 2 commits August 3, 2026 16:06

Merge branch 'develop' of https://github.com/XRPLF/rippled into mvada…

2c5fa02

…ri/rearch/init

fix clang-tidy issues

47b2035

This comment was marked as resolved.

Sign in to view

Merge branch 'develop' of https://github.com/XRPLF/rippled into mvada…

0900ce7

…ri/rearch/init

This comment was marked as resolved.

Sign in to view

improve setup based on comments from bart

19f742a

This comment was marked as resolved.

Sign in to view

mvadari added 2 commits August 11, 2026 14:25

fix clang-tidy issues

3ff036d

fix clang-tidy

dc75de0

github-actions Bot added the PR: has conflicts label Aug 11, 2026

Merge branch 'develop' of https://github.com/XRPLF/rippled into mvada…

2981b97

…ri/rearch/init

github-actions Bot removed the PR: has conflicts label Aug 11, 2026

This comment was marked as resolved.

Sign in to view

mvadari added this to the 3.4.0 milestone Aug 11, 2026

mvadari and others added 2 commits August 11, 2026 15:26

fix clang-tidy better

f071af7

Merge branch 'develop' into mvadari/rearch/init

4cc2d3f

xrplf-ai-reviewer Bot approved these changes Aug 11, 2026

View reviewed changes

fix build issues

d0008a3

xrplf-ai-reviewer Bot approved these changes Aug 11, 2026

View reviewed changes

xrplf-ai-reviewer Bot reviewed Aug 12, 2026

View reviewed changes

Comment thread include/xrpl/ledger/helpers/SLEBase.h

add tests, respond to feedback

2791e6e

xrplf-ai-reviewer Bot approved these changes Aug 12, 2026

View reviewed changes

xrplf-ai-reviewer Bot approved these changes Aug 13, 2026

View reviewed changes

mvadari mentioned this pull request Aug 13, 2026

refactor: Start making ledger helpers type-safe, beginning with AccountRoot #6620

Closed

bthomee reviewed Aug 13, 2026

View reviewed changes

		using RAccountRootEntry = AccountRootEntry<ReadView>;
		using WAccountRootEntry = AccountRootEntry<ApplyView>;

Uh oh!

Conversation

mvadari commented Jul 28, 2026

High Level Overview of Change

Context of Change

API Impact

Uh oh!

codecov Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

github-actions Bot commented Aug 11, 2026

Uh oh!

github-actions Bot commented Aug 11, 2026

Uh oh!

This comment was marked as resolved.

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

bthomee left a comment

Choose a reason for hiding this comment

Uh oh!

bthomee Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

mvadari Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

bthomee Aug 14, 2026

codecov Bot commented Jul 28, 2026 •

edited

Loading