refactor: Move `LendingHelpers` into `libxrpl/ledger/helpers` by mvadari · Pull Request #6638 · XRPLF/rippled

mvadari · 2026-03-24T19:51:21Z

High Level Overview of Change

This PR moves all remaining helper files into the libxrpl/ledger/helpers folder, after #6453.

This is a pure refactor. There is no functionality change.

Context of Change

Prepping for #6620 and downstream PRs

API Impact

N/A

xrplf-ai-reviewer

No issues.

Review by Claude Opus 4.6 · Prompt: V12

bthomee · 2026-03-24T20:28:34Z

+#include <xrpl/ledger/helpers/AMMHelpers.h>
+#include <xrpl/ledger/helpers/AMMUtils.h>


Can these two files be combined perhaps? Looking at their contents, seems like it should be fine to move the functionality from AMMUtils into AMMHelpers, unless that's problematic for (circular) header inclusion reasons.

I was going to leave that for a second PR, to keep this one dead simple.

xrplf-ai-reviewer

Took a pass through this

Three concerns worth confirming: (1) missing MPTokenHelpers.h include in EscrowHelpers.h, (2) potential MPToken authorization bypass via EscrowFinish/Cancel, (3) the checkLendingProtocolDependencies refactor may not be behaviorally equivalent to the VaultCreate::checkExtraFeatures it replaced. Also flagged a question about whether AMM transactors have PermissionedDEX credential checks comparable to OfferCreate. See inline comments.

Review by ReviewBot 🤖

Review by Claude Opus 4.6 · Prompt: V12

xrplf-ai-reviewer · 2026-03-24T22:41:02Z


-        if (auto const ter = MPTokenAuthorize::createMPToken(view, mptID, receiver, 0);
-            !isTesSuccess(ter))
+        if (auto const ter = createMPToken(view, mptID, receiver, 0); !isTesSuccess(ter))


Header isn't self-contained — createMPToken() is used but MPTokenHelpers.h is never included. Add the missing include near the top of the file:

#include <xrpl/ledger/helpers/MPTokenHelpers.h>

(This was lost when MPTokenAuthorize.h was removed.)

xrplf-ai-reviewer · 2026-03-24T22:41:02Z

+
 bool
-checkLendingProtocolDependencies(PreflightContext const& ctx)
+checkLendingProtocolDependencies(Rules const& rules, STTx const& tx)


Replacing VaultCreate::checkExtraFeatures(ctx) with inlined logic changes the function signature and behavior. Please confirm the new checks (featureSingleAssetVault, featureMPTokensV1, conditional featurePermissionedDomains) are exactly equivalent to what VaultCreate::checkExtraFeatures enforced — any divergence silently changes preflight acceptance in a consensus system.

xrplf-ai-reviewer · 2026-03-24T22:41:02Z

@@ -1,17 +1,16 @@
 #include <xrpl/ledger/OrderBookDB.h>


AMM transactors may be missing PermissionedDEX credential checks that OfferCreate enforces via PermissionedDEXHelpers. AMM deposits/withdrawals are economically equivalent to offer placement, so accounts could bypass DEX access controls by routing through an AMM pool. Please verify all AMM transactors apply the same access-control gates as OfferCreate.

Permissioned DEX doesn't support AMM right now.

codecov · 2026-03-24T22:46:44Z

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 81.4%. Comparing base (f239256) to head (068c583).
⚠️ Report is 19 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/libxrpl/ledger/helpers/LendingHelpers.cpp	87.5%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #6638     +/-   ##
=========================================
- Coverage     81.4%   81.4%   -0.0%     
=========================================
  Files         1006    1006             
  Lines        74462   74468      +6     
  Branches      7561    7554      -7     
=========================================
+ Hits         60646   60647      +1     
- Misses       13816   13821      +5

Files with missing lines	Coverage Δ
include/xrpl/ledger/helpers/LendingHelpers.h	`95.2% <ø> (ø)`
include/xrpl/tx/transactors/lending/LoanSet.h	`100.0% <ø> (ø)`
...tx/transactors/lending/LoanBrokerCoverClawback.cpp	`97.8% <100.0%> (ø)`
.../tx/transactors/lending/LoanBrokerCoverDeposit.cpp	`94.7% <100.0%> (ø)`
...tx/transactors/lending/LoanBrokerCoverWithdraw.cpp	`96.3% <100.0%> (ø)`
...ibxrpl/tx/transactors/lending/LoanBrokerDelete.cpp	`88.6% <100.0%> (ø)`
...c/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp	`96.0% <100.0%> (ø)`
src/libxrpl/tx/transactors/lending/LoanDelete.cpp	`96.4% <100.0%> (ø)`
src/libxrpl/tx/transactors/lending/LoanManage.cpp	`89.7% <100.0%> (ø)`
src/libxrpl/tx/transactors/lending/LoanPay.cpp	`95.3% <100.0%> (ø)`
... and 2 more

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

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

xrplf-ai-reviewer

Took a pass through this

The MR description says "pure refactor, no source code change" but LendingHelpers.cpp inlines feature checks that previously delegated to VaultCreate::checkExtraFeatures — in a consensus system any behavioral difference could cause forks. Also flagged: a stale forward declaration, a missing MPTokenHelpers.h include in EscrowHelpers.h, and widened visibility of the security-sensitive createMPToken primitive. See inline comments.

Review by ReviewBot 🤖

Review by Claude Opus 4.6 · Prompt: V12

xrplf-ai-reviewer · 2026-03-24T23:38:22Z

+
 bool
-checkLendingProtocolDependencies(PreflightContext const& ctx)
+checkLendingProtocolDependencies(Rules const& rules, STTx const& tx)


Correctness risk in a consensus system — the old code delegated to VaultCreate::checkExtraFeatures(ctx) but this inlines three explicit checks. Please confirm these are exactly equivalent to what VaultCreate::checkExtraFeatures returned. Any behavioral difference could cause ledger forks.

xrplf-ai-reviewer · 2026-03-24T23:38:22Z

+
 bool
-checkLendingProtocolDependencies(PreflightContext const& ctx)
+checkLendingProtocolDependencies(Rules const& rules, STTx const& tx)


Possible incomplete feature-gate replication — if VaultCreate::checkExtraFeatures checked flags beyond featureSingleAssetVault, featureMPTokensV1, and featurePermissionedDomains, lending transactions could become reachable in states where they should still be gated. Audit VaultCreate::checkExtraFeatures and add a unit test covering every flag-disabled combination the old code would have rejected.

xrplf-ai-reviewer · 2026-03-24T23:38:22Z


-        if (auto const ter = MPTokenAuthorize::createMPToken(view, mptID, receiver, 0);
-            !isTesSuccess(ter))
+        if (auto const ter = createMPToken(view, mptID, receiver, 0); !isTesSuccess(ter))


Missing include: createMPToken (declared in MPTokenHelpers.h) is called here but the header for it was removed with no replacement. Add #include <xrpl/ledger/helpers/MPTokenHelpers.h> — the header currently compiles only by accident via consumer TUs.

xrplf-ai-reviewer · 2026-03-24T23:38:22Z

 }

+TER
+createMPToken(


Widened visibility of a security-sensitive primitive — createMPToken was previously scoped to MPTokenAuthorize and is now a free function callable by any includer. It inserts an MPToken into the ledger with no authorization check. Document in the header that callers must verify authorization independently, add [[nodiscard]] to the return TER, and audit all call sites to confirm authorization is enforced upstream.

Co-authored-by: xrplf-ai-reviewer[bot] <266832837+xrplf-ai-reviewer[bot]@users.noreply.github.com>

xrplf-ai-reviewer

Took a pass through this

Two correctness concerns flagged inline: a potential semantic mismatch in the isFlag() → bitwise replacement in MPTokenHelpers.cpp, and a possibly incomplete inlining of VaultCreate::checkExtraFeatures in LendingHelpers.cpp that could silently drop amendment gates.

Review by ReviewBot 🤖

Review by Claude Opus 4.6 · Prompt: V12

xrplf-ai-reviewer · 2026-03-25T04:05:44Z

        return tecOBJECT_NOT_FOUND;

-    if (!sleIssuance->isFlag(lsfMPTCanTransfer))
+    if ((sleIssuance->getFieldU32(sfFlags) & lsfMPTCanTransfer) == 0u)


Verify semantic equivalence with the replaced isFlag() call — confirm lsfMPTCanTransfer is the correct bit mask and no normalization step was dropped. If this tests a different bit position than isFlag() did, flag state written by MPTokenIssuanceSet will be misread here and in MPTokenAuthorize, potentially enabling features (transferability, clawback) that should remain disabled.

Please confirm this is equivalent to the previous:

if (!sleIssuance->isFlag(lsfMPTCanTransfer))

xrplf-ai-reviewer · 2026-03-25T04:05:44Z

+checkLendingProtocolDependencies(Rules const& rules, STTx const& tx)
 {
-    return ctx.rules.enabled(featureSingleAssetVault) && VaultCreate::checkExtraFeatures(ctx);
+    if (!rules.enabled(featureSingleAssetVault))


Missing amendment gates from the removed VaultCreate::checkExtraFeatures call — verify the inline expansion is complete. The old implementation was:

return ctx.rules.enabled(featureSingleAssetVault) && VaultCreate::checkExtraFeatures(ctx);

If checkExtraFeatures enforced additional amendment checks beyond featureMPTokensV1 and featurePermissionedDomains, all eight lending transactors that delegate here will silently skip those gates, potentially accepting transactions that should be rejected — which could cause consensus divergence between nodes on different amendment sets.

xrplf-ai-reviewer

Three issues flagged inline: possible dropped feature-gate constraints in LendingHelpers (high severity), undocumented reserve-check precondition on the new public createMPToken API, and a isFlag() → manual bitwise substitution that needs semantic verification.

Review by Claude Opus 4.6 · Prompt: V12

xrplf-ai-reviewer · 2026-03-25T04:23:08Z

    beast::Journal j);

+TER
+createMPToken(


Newly public createMPToken has no documented reserve-check precondition — callers may skip it.

The function was extracted from MPTokenAuthorize where the reserve check was always performed before calling it. Now that it's a shared public API, document the precondition prominently so future callers know they must validate reserve balance first.

// PRECONDITION: Caller must have already verified the account has // sufficient owner reserve before invoking this function. TER createMPToken( ApplyView& view, MPTID const& mptIssuanceID, AccountID const& account, std::uint32_t const flags);

xrplf-ai-reviewer · 2026-03-25T04:23:08Z


 bool
-checkLendingProtocolDependencies(PreflightContext const& ctx)
+checkLendingProtocolDependencies(Rules const& rules, STTx const& tx)


Refactor may silently drop constraints from VaultCreate::checkExtraFeatures — verify nothing was lost.

The old implementation was ctx.rules.enabled(featureSingleAssetVault) && VaultCreate::checkExtraFeatures(ctx). The new version inlines only three rules.enabled() checks. If VaultCreate::checkExtraFeatures enforced any additional feature flags, field validations, or invariants beyond these three, those checks are now silently removed. In a consensus-critical path, this could allow transactions that should be rejected to proceed.

Please audit the full implementation of VaultCreate::checkExtraFeatures and confirm that every condition it enforced is either replicated here or explicitly justified as removed. Add regression tests covering each previously-checked condition.

xrplf-ai-reviewer · 2026-03-25T04:23:08Z

        return tecOBJECT_NOT_FOUND;

-    if (!sleIssuance->isFlag(lsfMPTCanTransfer))
+    if ((sleIssuance->getFieldU32(sfFlags) & lsfMPTCanTransfer) == 0u)


Manual bitwise check replaces isFlag() — verify the semantics are identical.

If SLE::isFlag() does anything beyond (getFieldU32(sfFlags) & flag) != 0 (e.g., reads a cached/normalized field), this substitution silently changes transfer-authorization logic. Confirm isFlag() is a pure bitwise check, or revert to using it.

Suggested change

if ((sleIssuance->getFieldU32(sfFlags) & lsfMPTCanTransfer) == 0u)

if (!sleIssuance->isFlag(lsfMPTCanTransfer))

Tapanito · 2026-03-26T14:27:48Z


 bool
-checkLendingProtocolDependencies(PreflightContext const& ctx)
+checkLendingProtocolDependencies(Rules const& rules, STTx const& tx)


This PR is no longer just structural change. Could we keep this logic as it was for now?

It fails levelization if I don't change it.

You can run the levelization script, and commit the updated output. Of course, if it introduces a new loop (circular dependency) then that's not great and would need to be fixed through other changes.

Yes, that's what I had to do here

@Tapanito this PR has shrunk to just moving LendingHelpers over, so we can discuss specifically this issue. The business logic is still identical with this change.

github-actions · 2026-04-06T23:02:42Z

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

github-actions · 2026-04-07T13:08:21Z

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

github-actions · 2026-04-17T17:10:26Z

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

mvadari added 11 commits March 24, 2026 12:40

move LendingHelpers

21dfb60

move PaymentChannelHelpers

8e45edd

move NFTokenUtils

fd0ebc8

move AMMHelpers

05d661c

move AMMUtils

f2a36a2

move PermissionedDEXHelpers

f441563

move EscrowHelpers

7517d01

move remaining cpp files

e2d913e

move DelegateUtils

29eb6ed

fix copy paste issues

24fa6b5

fix more stuff

155f2e8

mvadari requested review from Tapanito and bthomee March 24, 2026 19:57

xrplf-ai-reviewer bot approved these changes Mar 24, 2026

View reviewed changes

bthomee approved these changes Mar 24, 2026

View reviewed changes

mvadari added 2 commits March 24, 2026 15:16

fix NFT includes

6b0f230

fix more build issues

396a481

xrplf-ai-reviewer bot reviewed Mar 24, 2026

View reviewed changes

fix clang-tidy issues

e8f5ee9

xrplf-ai-reviewer bot reviewed Mar 24, 2026

View reviewed changes

mvadari and others added 2 commits March 24, 2026 20:51

Update src/libxrpl/ledger/helpers/LendingHelpers.cpp

2b60bf9

Co-authored-by: xrplf-ai-reviewer[bot] <266832837+xrplf-ai-reviewer[bot]@users.noreply.github.com>

Merge branch 'develop' into mvadari/move-more-helpers

4103fd8

xrplf-ai-reviewer bot reviewed Mar 25, 2026

View reviewed changes

fix pre-commit issues

0ccc592

xrplf-ai-reviewer bot reviewed Mar 25, 2026

View reviewed changes

github-actions bot added the PR: has conflicts label Mar 26, 2026

This comment was marked as resolved.

Sign in to view

Tapanito reviewed Mar 26, 2026

View reviewed changes

Merge branch 'develop' into mvadari/move-more-helpers

bca7c29

Copilot AI review requested due to automatic review settings April 3, 2026 14:24

github-actions bot removed the PR: has conflicts label Apr 3, 2026

This comment was marked as resolved.

Sign in to view

Copilot started reviewing on behalf of mvadari April 3, 2026 14:24 View session

This comment was marked as resolved.

Sign in to view

mvadari added 2 commits April 3, 2026 10:58

remove undeclared function

0fc901e

fix import

3bbe525

mvadari mentioned this pull request Apr 6, 2026

refactor: Move more helper files into libxrpl/ledger/helpers #6731

Merged

github-actions bot added the PR: has conflicts label Apr 6, 2026

Merge branch 'develop' into mvadari/move-more-helpers

b452bd0

github-actions bot removed the PR: has conflicts label Apr 7, 2026

mvadari added 2 commits April 7, 2026 09:12

roll back unrelated changes

691bcb2

roll back some more

068c583

mvadari changed the title ~~refactor: Move more helper files into libxrpl/ledger/helpers~~ refactor: Move LendingHelper into libxrpl/ledger/helpers Apr 7, 2026

mvadari changed the title ~~refactor: Move LendingHelper into libxrpl/ledger/helpers~~ refactor: Move LendingHelpers into libxrpl/ledger/helpers Apr 7, 2026

mvadari mentioned this pull request Apr 7, 2026

refactor: Combine AMMHelpers and AMMUtils #6733

Merged

github-actions bot added the PR: has conflicts label Apr 17, 2026

		#include <xrpl/ledger/helpers/AMMHelpers.h>
		#include <xrpl/ledger/helpers/AMMUtils.h>

	if ((sleIssuance->getFieldU32(sfFlags) & lsfMPTCanTransfer) == 0u)
	if (!sleIssuance->isFlag(lsfMPTCanTransfer))

Conversation

mvadari commented Mar 24, 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

Uh oh!

xrplf-ai-reviewer bot left a comment

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!

xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xrplf-ai-reviewer bot Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 24, 2026

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 Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 25, 2026

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 Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

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!

mvadari commented Mar 24, 2026 •

edited

Loading

codecov bot commented Mar 24, 2026 •

edited

Loading