refactor: Start making ledger helpers type-safe, beginning with `AccountRoot` by mvadari · Pull Request #6620 · XRPLF/rippled

mvadari · 2026-03-23T16:27:07Z

High Level Overview of Change

This PR marks the beginning of a process to make SLEs and SLE helpers more type-safe. The original proposal is here. The general idea is each ledger entry will have a read-only and a write-access version of the class. Getter functions will be on the read-only version, and modifier functions will be on the writable version.

This PR begins with implementing base read and write classes, and also implements and migrates AccountRoot to the type-safe architecture. To keep PRs small and easy to review, ~each ledger entry will have its own implementation/migration PR.

There is no functionality change in this PR, only pure refactoring.

Note for reviewers: I recommend enabling "Hide whitespace" when looking at the diff for this PR in the Github UI.

Context of Change

There is a README added as a part of this PR

API Impact

N/A

Test Plan

Tests were not modified at all, and still pass.

codecov · 2026-03-23T17:19:58Z

Codecov Report

❌ Patch coverage is 97.44637% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.0%. Comparing base (cee1574) to head (6c7a8ed).
⚠️ Report is 32 commits behind head on develop.

Files with missing lines	Patch %	Lines
include/xrpl/ledger/helpers/AccountRootHelpers.h	65.0%	7 Missing ⚠️
include/xrpl/ledger/helpers/SLEBase.h	91.8%	5 Missing ⚠️
src/xrpld/rpc/handlers/account/AccountInfo.cpp	68.8%	5 Missing ⚠️
src/libxrpl/ledger/helpers/RippleStateHelpers.cpp	95.5%	2 Missing ⚠️
include/xrpl/ledger/helpers/EscrowHelpers.h	91.7%	1 Missing ⚠️
src/libxrpl/ledger/helpers/LendingHelpers.cpp	90.9%	1 Missing ⚠️
src/libxrpl/ledger/helpers/TokenHelpers.cpp	97.3%	1 Missing ⚠️
src/libxrpl/tx/transactors/dex/AMMCreate.cpp	96.3%	1 Missing ⚠️
src/xrpld/rpc/detail/PathRequest.cpp	80.0%	1 Missing ⚠️
src/xrpld/rpc/detail/TransactionSign.cpp	95.5%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #6620     +/-   ##
=========================================
- Coverage     82.0%   82.0%   -0.0%     
=========================================
  Files         1009    1010      +1     
  Lines        76779   76868     +89     
  Branches      8937    8944      +7     
=========================================
+ Hits         62980   63044     +64     
- Misses       13790   13815     +25     
  Partials         9       9

Files with missing lines	Coverage Δ
include/xrpl/ledger/View.h	`100.0% <ø> (ø)`
include/xrpl/ledger/helpers/LendingHelpers.h	`96.6% <ø> (ø)`
include/xrpl/ledger/helpers/RippleStateHelpers.h	`100.0% <ø> (ø)`
include/xrpl/tx/Transactor.h	`100.0% <ø> (ø)`
include/xrpl/tx/paths/detail/StepChecks.h	`100.0% <100.0%> (ø)`
src/libxrpl/ledger/View.cpp	`96.7% <100.0%> (ø)`
src/libxrpl/ledger/helpers/AMMHelpers.cpp	`95.3% <100.0%> (ø)`
src/libxrpl/ledger/helpers/AccountRootHelpers.cpp	`96.0% <100.0%> (+0.9%)`	⬆️
src/libxrpl/ledger/helpers/CredentialHelpers.cpp	`97.1% <100.0%> (-1.2%)`	⬇️
src/libxrpl/ledger/helpers/MPTokenHelpers.cpp	`96.4% <100.0%> (ø)`
... and 83 more

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mvadari · 2026-03-23T19:14:46Z

/ai-review

xrplf-ai-reviewer

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

Copilot

Pull request overview

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

    return asset.visit(
-        [&](Issue const& issue) -> Rate {
-            if (isXRP(issue.account) || issue.account == dstAccount)
-                return kParityRate;
-            return transferRate(view, issue.account);
-        },
-        [&](MPTIssue const& mptIssue) -> Rate {
+        [&](Issue const& issue) { return AccountRoot(issue.account, view).transferRate(); },
+        [&](MPTIssue const& mptIssue) {


+**`SLEBase<ReadView>`** holds a `std::shared_ptr<SLE const>` and a `ReadView const&`. Write-only members are excluded at compile time.
+holds a mutable `std::shared_ptr<SLE>`, an `ApplyView&`, and a `Keylet`. It exposes `insert()`, `update()`, `erase()`, and `newSLE()` to keep the SLE and its view in sync automatically.


    beast::WrappedSink sink_;
    beast::Journal const j_;

    AccountID const accountID_;
+    WAccountRoot account_;
    XRPAmount preFeeBalance_{};  // Balance before fees.


xrplf-ai-reviewer

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

xrplf-ai-reviewer

ℹ️ Note: This is a large diff (293,703 chars). Complex issues deep in the diff may receive less attention.

One high-severity correctness issue (dropped dstAccount parity guard in BookStep.cpp), plus missing null-check, redundant view lookup, dropped journal, and a doc formatting gap — see inline comments.

Review by Claude Sonnet 4.6 · Prompt: V15

xrplf-ai-reviewer · 2026-06-08T22:11:05Z

+`SLEBase.h` introduces a single template class `SLEBase<ViewT>` that pairs an SLE with its view context and enforces read/write semantics at compile time via `requires` clauses.
+
+**`SLEBase<ReadView>`** holds a `std::shared_ptr<SLE const>` and a `ReadView const&`. Write-only members are excluded at compile time.
+holds a mutable `std::shared_ptr<SLE>`, an `ApplyView&`, and a `Keylet`. It exposes `insert()`, `update()`, `erase()`, and `newSLE()` to keep the SLE and its view in sync automatically.


Missing SLEBase<ApplyView> header — line reads as continuation of the SLEBase<ReadView> description above:

Suggested change

holds a mutable `std::shared_ptr<SLE>`, an `ApplyView&`, and a `Keylet`. It exposes `insert()`, `update()`, `erase()`, and `newSLE()` to keep the SLE and its view in sync automatically.

**`SLEBase<ApplyView>`** holds a mutable `std::shared_ptr<SLE>`, an `ApplyView&`, and a `Keylet`. It exposes `insert()`, `update()`, `erase()`, and `newSLE()` to keep the SLE and its view in sync automatically.

xrplf-ai-reviewer · 2026-06-08T22:11:05Z

+        beast::Journal j = beast::Journal{beast::Journal::getNullSink()})
+        requires kIsWritable
+        : view_(view)
+        , key_(sle ? Keylet(sle->getType(), sle->key()) : Keylet(ltANY, uint256{}))


Null sle produces a bogus Keylet(ltANY, uint256{}), which newSLE() would silently use. Assert non-null instead:

Suggested change

, key_(sle ? Keylet(sle->getType(), sle->key()) : Keylet(ltANY, uint256{}))

, key_(sle ? Keylet(sle->getType(), sle->key()) : (XRPL_ASSERT(false, "xrpl::SLEBase : null sle passed to writable constructor"), Keylet(ltANY, uint256{})))

xrplf-ai-reviewer · 2026-06-08T22:11:05Z

-            return transferRate(view, issue.account);
-        },
-        [&](MPTIssue const& mptIssue) -> Rate {
+        [&](Issue const& issue) { return AccountRoot(issue.account, view).transferRate(); },


issue.account == dstAccount parity guard was dropped. When the IOU issuer is the destination, the old code returned kParityRate (fee-free); AccountRoot::transferRate() never checks dstAccount, so a destination with sfTransferRate set will now incorrectly incur a transfer fee.

xrplf-ai-reviewer · 2026-06-08T22:11:05Z

-    adjustOwnerCount(
-        view, view.peek(accountKeylet), removeFromOwnerCount, registry.getJournal("View"));
+    WAccountRoot wrappedAcct(account, view, j);
+    wrappedAcct.adjustOwnerCount(removeFromOwnerCount);


Missing null-check on wrappedAcct before use. Every other WAccountRoot call site in this PR guards with if (!wrappedAcct) return tefINTERNAL;. XRPL_ASSERT is a no-op in release builds, so a missing account silently dereferences null.

WAccountRoot wrappedAcct(account, view, j); if (!wrappedAcct) return tefINTERNAL; // LCOV_EXCL_LINE wrappedAcct.adjustOwnerCount(removeFromOwnerCount);

xrplf-ai-reviewer · 2026-06-08T22:11:05Z

    // Have to check again in deposit() because
    // amounts might be derived based on tokens or
    // limits.
+    AccountRoot wrappedAcct(accountID, ctx.view);


ctx.j not passed — xrpLiquid trace-logging silently uses null sink. AMMCreate.cpp correctly passes ctx.j in the parallel change.

Suggested change

AccountRoot wrappedAcct(accountID, ctx.view);

AccountRoot wrappedAcct(accountID, ctx.view, ctx.j);

xrplf-ai-reviewer · 2026-06-08T22:11:06Z

-        if (auto err =
-                verifyDepositPreauth(ctx_.tx, ctx_.view(), txAccount, dst, sled, ctx_.journal);
+        if (auto err = verifyDepositPreauth(
+                ctx_.tx, ctx_.view(), txAccount, AccountRoot(dst, ctx_.view(), j_), ctx_.journal);


Redundant view.read() — dstAcct is already in scope and converts implicitly to RAccountRoot. Pass it directly:

Suggested change

ctx_.tx, ctx_.view(), txAccount, AccountRoot(dst, ctx_.view(), j_), ctx_.journal);

ctx_.tx, ctx_.view(), txAccount, dstAcct, ctx_.journal);

Tapanito · 2026-06-09T09:24:20Z

-        return {tefINTERNAL, beast::kZero};  // LCOV_EXCL_LINE
+    auto const feePayer = ctx_.tx.getFeePayer();
+    bool const hasDelegateAcct = (feePayer != accountID_);
+    std::optional<WAccountRoot> delegateAcct;


delegateAcct is already optional, this bool is redundant. All three use-sites where hasDelegateAcct is used, can use delegateAcct directly.

Tapanito · 2026-06-09T09:32:18Z

+    /** Returns true if the ledger entry exists */
+    [[nodiscard]] bool
+    exists() const
+    {
+        return sle_ != nullptr;
+    }
+
+    /** Explicit conversion to bool for convenient existence checking */
+    explicit
+    operator bool() const
+    {
+        return exists();
+    }


There are two ways to check for existence, which is already creating confusion in this PR.

In some places, like the Transactor.cpp both !*acct and !acct->exists() are used interchangeably. There's nothing in the methods indicating that they have the same semantics. Why not picking one and sticking with it?

Tapanito · 2026-06-09T09:43:28Z


 //------------------------------------------------------------------------------

+[[nodiscard]] static bool


Out of scope for this PR, but how are you thinking about invariants? Do you envision they'll operate on raw SLEs or do you have a mechanism in mind to wrap the SLEs in the new classes?

Tapanito · 2026-06-09T09:45:57Z


-    auto const issuer = view.read(keylet::account(issue.getIssuer()));
-    if (!issuer)
+    if (!issuer.exists())


Why are you using .exists() instead of !issuer?

Tapanito · 2026-06-09T09:49:23Z

+
+void
+adjustOwnerCount(
+    std::shared_ptr<SLE> const& sle,


Please change this to SLE::ref, that way we won't have to bring in memory dependency.

Tapanito · 2026-06-09T09:52:28Z

+[[nodiscard]] static bool
+isPseudoAccount(std::shared_ptr<SLE const> sleAcct)
+{
+    auto const& fields = getPseudoAccountFields();


This code, 1:1 replicates the code in the AccountRootHelpers, if the logic has to change for any reason, it'll potentially create discrepancies between the two implementations. Please move the logic to a helper function that both the AccountRootHelpers and the method here can use.

Tapanito · 2026-06-09T09:54:15Z

@@ -116,6 +117,7 @@ class Transactor
    beast::Journal const j_;

    AccountID const accountID_;


Why do we need this member variable? Surely it can be replaced with account_.id()?

Tapanito · 2026-06-09T09:55:42Z

+// accidentally calling this with the wrong type.
+void
+adjustOwnerCount(
+    std::shared_ptr<SLE> const& sle,


Ditto here, please useSLE::ref

github-actions · 2026-06-09T19:45:06Z

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

…ri/rearch/account

github-actions · 2026-06-11T01:34:33Z

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

xrplf-ai-reviewer

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

xrplf-ai-reviewer

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

github-actions · 2026-06-17T13:54:34Z

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

ximinez

I'm just getting started on this one, so just a couple of preliminary comments.

The reason I'm posting it now is to ask how feasible it would be to split this into 2 or 3 stacked PRs?

The base framework - SLEBase, etc.
Maybe the AccountRoot<ViewT>, etc., if you don't include that in the first one.
Changing all the call sites and usage from SLE to AccountRoot.

ximinez · 2026-07-09T01:02:04Z

+    static constexpr bool kIsWritable = WritableView<ViewT>;
+
+    // SLE pointer type: mutable for writable views, const for read-only
+    using sle_ptr_type = std::conditional_t<kIsWritable, std::shared_ptr<SLE>, SLE::const_pointer>;


std::shared_ptr<SLE> is equivalent to SLE::pointer. No point in mixing metaphors.

Suggested change

using sle_ptr_type = std::conditional_t<kIsWritable, std::shared_ptr<SLE>, SLE::const_pointer>;

using sle_ptr_type = std::conditional_t<kIsWritable, SLE::pointer, SLE::const_pointer>;

ximinez · 2026-07-09T01:04:17Z

+ * Derived classes should provide domain-specific accessors that hide
+ * implementation details of the underlying ledger entry format.
+ */
+template <typename ViewT>


Instead of typename, how about another concept?

template <typename V> concept UsableView = WritableView<V> || std::derived_from<V, ReadView>;

Maybe pick a better name than "Usable".

mvadari requested review from Tapanito, a1q123456, godexsoft and ximinez and removed request for Tapanito and a1q123456 March 23, 2026 16:28

a1q123456 reviewed Mar 24, 2026

View reviewed changes

Comment thread include/xrpl/ledger/helpers/AccountRootHelpers.h Outdated

a1q123456 reviewed Mar 24, 2026

View reviewed changes

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

a1q123456 reviewed Mar 24, 2026

View reviewed changes

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

mvadari added 17 commits March 24, 2026 12:13

add WrappedSLEBase

659f455

First cut of WrappedAccountRoot

4157e36

fix all the build issues

a8987cf

fix more build issues

e0f487b

fix remaining build issues

43caa7d

minor improvements

e6369c0

split up read only and write

b5562cc

Add insert/update/erase to WritableSLE

1cc7424

clean up comments

1ccd84e

change . to ->

c24432f

fix more AccountRoot stuff

add3d7e

fix long tail issues

0ffb3e2

more fixes

7900fa9

fix account_

56c173a

fix issues

a423813

WrappedSLEBase -> SLEBase

f41c02d

add readme

9b42e17

mvadari and others added 2 commits June 8, 2026 14:54

Merge branch 'develop' into mvadari/rearch/account

b1f4e61

fix clang-tidy issues

77120e7

xrplf-ai-reviewer Bot approved these changes Jun 8, 2026

View reviewed changes

fix build

f4cb0b8

mvadari requested a review from Copilot June 8, 2026 20:47

Copilot started reviewing on behalf of mvadari June 8, 2026 20:47 View session

Copilot AI reviewed Jun 8, 2026

View reviewed changes

xrplf-ai-reviewer Bot approved these changes Jun 8, 2026

View reviewed changes

fix clang-tidy issues

805d12d

xrplf-ai-reviewer Bot reviewed Jun 8, 2026

View reviewed changes

Tapanito requested changes Jun 9, 2026

View reviewed changes

github-actions Bot added the PR: has conflicts label Jun 9, 2026

mvadari added 2 commits June 10, 2026 21:30

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

3a5eb97

…ri/rearch/account

fix clang-tidy issues

7098e49

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

mvadari added 3 commits June 10, 2026 22:53

roll back unrelated clang-tidy changes

052bb02

clang-tidy broke my build -.-

f91fbc9

fix clang-tidy issues

095a588

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

View reviewed changes

mvadari and others added 2 commits June 11, 2026 14:25

fix merge issues

a0d2d17

Merge branch 'develop' into mvadari/rearch/account

6c7a8ed

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

View reviewed changes

github-actions Bot added the PR: has conflicts label Jun 17, 2026

mvadari dismissed a1q123456’s stale review via 6c7a8ed June 24, 2026 16:06

mvadari modified the milestones: 3.3.0, 3.4.0 Jul 9, 2026

ximinez requested changes Jul 9, 2026

View reviewed changes

		`SLEBase<ReadView>` holds a `std::shared_ptr<SLE const>` and a `ReadView const&`. Write-only members are excluded at compile time.
		holds a mutable `std::shared_ptr<SLE>`, an `ApplyView&`, and a `Keylet`. It exposes `insert()`, `update()`, `erase()`, and `newSLE()` to keep the SLE and its view in sync automatically.

	holds a mutable `std::shared_ptr<SLE>`, an `ApplyView&`, and a `Keylet`. It exposes `insert()`, `update()`, `erase()`, and `newSLE()` to keep the SLE and its view in sync automatically.
	`SLEBase<ApplyView>` holds a mutable `std::shared_ptr<SLE>`, an `ApplyView&`, and a `Keylet`. It exposes `insert()`, `update()`, `erase()`, and `newSLE()` to keep the SLE and its view in sync automatically.

	, key_(sle ? Keylet(sle->getType(), sle->key()) : Keylet(ltANY, uint256{}))
	, key_(sle ? Keylet(sle->getType(), sle->key()) : (XRPL_ASSERT(false, "xrpl::SLEBase : null sle passed to writable constructor"), Keylet(ltANY, uint256{})))

	AccountRoot wrappedAcct(accountID, ctx.view);
	AccountRoot wrappedAcct(accountID, ctx.view, ctx.j);

	ctx_.tx, ctx_.view(), txAccount, AccountRoot(dst, ctx_.view(), j_), ctx_.journal);
	ctx_.tx, ctx_.view(), txAccount, dstAcct, ctx_.journal);


		//------------------------------------------------------------------------------

		[[nodiscard]] static bool

		@@ -116,6 +117,7 @@ class Transactor
		beast::Journal const j_;

		AccountID const accountID_;

	using sle_ptr_type = std::conditional_t<kIsWritable, std::shared_ptr<SLE>, SLE::const_pointer>;
	using sle_ptr_type = std::conditional_t<kIsWritable, SLE::pointer, SLE::const_pointer>;

Uh oh!

Conversation

mvadari commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High Level Overview of Change

Context of Change

API Impact

Test Plan

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

mvadari commented Mar 23, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

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 Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 9, 2026

Uh oh!

github-actions Bot commented Jun 11, 2026

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!

github-actions Bot commented Jun 17, 2026

Uh oh!

ximinez left a comment

Choose a reason for hiding this comment

Uh oh!

mvadari commented Mar 23, 2026 •

edited

Loading

codecov Bot commented Mar 23, 2026 •

edited

Loading