refactor: Address PR comments after the modularisation PRs by a1q123456 · Pull Request #6389 · XRPLF/rippled

a1q123456 · 2026-02-19T15:15:13Z

High Level Overview of Change

Context of Change

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Performance (increase or change in throughput and/or latency)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

API Impact

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

codecov · 2026-02-19T16:24:28Z

Codecov Report

❌ Patch coverage is 71.30435% with 66 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.5%. Comparing base (3d9c545) to head (02fc2f3).
⚠️ Report is 4 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/xrpld/app/misc/NetworkOPs.cpp	67.8%	48 Missing ⚠️
...rc/xrpld/app/rdb/backend/detail/SQLiteDatabase.cpp	50.0%	9 Missing ⚠️
src/xrpld/app/rdb/backend/detail/Node.cpp	60.0%	6 Missing ⚠️
src/libxrpl/rdb/DatabaseCon.cpp	0.0%	1 Missing ⚠️
src/libxrpl/tx/transactors/system/Change.cpp	75.0%	1 Missing ⚠️
src/xrpld/app/ledger/OrderBookDBImpl.cpp	80.0%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #6389     +/-   ##
=========================================
- Coverage     81.5%   81.5%   -0.0%     
=========================================
  Files          999     999             
  Lines        74456   74463      +7     
  Branches      7559    7558      -1     
=========================================
- Hits         60653   60652      -1     
- Misses       13803   13811      +8

Files with missing lines	Coverage Δ
include/xrpl/core/StartUpType.h	`100.0% <ø> (ø)`
include/xrpl/ledger/OrderBookDB.h	`100.0% <ø> (ø)`
include/xrpl/rdb/DatabaseCon.h	`100.0% <100.0%> (ø)`
include/xrpl/rdb/RelationalDatabase.h	`100.0% <ø> (ø)`
include/xrpl/tx/ApplyContext.h	`100.0% <ø> (ø)`
include/xrpl/tx/Transactor.h	`100.0% <ø> (ø)`
src/libxrpl/tx/Transactor.cpp	`93.3% <100.0%> (+<0.1%)`	⬆️
...c/libxrpl/tx/transactors/account/SignerListSet.cpp	`91.9% <100.0%> (ø)`
src/libxrpl/tx/transactors/check/CheckCancel.cpp	`93.5% <100.0%> (ø)`
src/libxrpl/tx/transactors/check/CheckCash.cpp	`92.6% <100.0%> (ø)`
... and 21 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.

godexsoft

Nice! I am not a fan of how we chain accessors on registry to get to the service/dependency a component needs but i think we can improve this later by injecting deps by their interfaces directly or developing a smarter compile-time registry wrapper. For now this is an improvement 👍

src/xrpld/app/misc/make_NetworkOPs.h

src/xrpld/app/misc/NetworkOPs.cpp

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

…pr-comments-after-tx-modularisation Signed-off-by: JCW <a1q123456@users.noreply.github.com> # Conflicts: # include/xrpl/rdb/DatabaseCon.h # src/libxrpl/tx/transactors/Change.cpp # src/libxrpl/tx/transactors/PayChan.cpp # src/test/app/LedgerLoad_test.cpp # src/xrpld/app/ledger/OrderBookDBImpl.cpp # src/xrpld/app/main/Application.cpp # src/xrpld/app/misc/NetworkOPs.cpp # src/xrpld/app/rdb/backend/detail/Node.cpp # src/xrpld/app/rdb/backend/detail/SQLiteDatabase.cpp

…dularisation

…pr-comments-after-tx-modularisation

godexsoft

Nice!

kuznetsss

👍

…pr-comments-after-tx-modularisation Signed-off-by: JCW <a1q123456@users.noreply.github.com> # Conflicts: # src/libxrpl/tx/transactors/escrow/Escrow.cpp # src/libxrpl/tx/transactors/payment_channel/PayChan.cpp # src/xrpld/app/misc/NetworkOPs.cpp

bthomee · 2026-03-20T15:22:19Z

/ai-review

xrplf-ai-reviewer

Code structure and type safety improvements across database and consensus handling.

Review by Claude Opus 4.6 · Prompt: V12

include/xrpl/rdb/RelationalDatabase.h

src/xrpld/app/misc/NetworkOPs.cpp

src/xrpld/app/rdb/backend/detail/Node.cpp

…pr-comments-after-tx-modularisation Signed-off-by: JCW <a1q123456@users.noreply.github.com> # Conflicts: # src/xrpld/app/ledger/OrderBookDBImpl.cpp # src/xrpld/app/main/Main.cpp # src/xrpld/app/misc/NetworkOPs.cpp # src/xrpld/app/rdb/backend/detail/Node.cpp

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

xrplf-ai-reviewer

Three design-level issues flagged inline: a sentinel-value ambiguity in ledger range SQL generation, a lost type-safety assertion from removing safe_downcast, and a pervasive std::reference_wrapper substitution that adds ergonomic cost without benefit.

Review by Claude Opus 4.6 · Prompt: V12

src/xrpld/app/rdb/backend/detail/Node.cpp

src/xrpld/app/misc/NetworkOPs.cpp

include/xrpl/tx/ApplyContext.h

…dularisation

xrplf-ai-reviewer

Took a pass through this

Two correctness issues flagged: a stale guard condition in Node.cpp that checks options.maxLedger while the body uses options.ledgerRange.max (high severity), and a std::convertible_to constraint on rangeCheckedCast that defeats its safe-narrowing purpose. Two lower-severity lifetime concerns noted for async lambdas capturing this/std::ref without shutdown-ordering guarantees — see inline.

Review by ReviewBot 🤖

Review by Claude Opus 4.6 · Prompt: V12

src/xrpld/app/rdb/backend/detail/Node.cpp

src/xrpld/app/rdb/backend/detail/SQLiteDatabase.cpp

include/xrpl/rdb/RelationalDatabase.h

src/xrpld/app/misc/NetworkOPs.cpp

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

xrplf-ai-reviewer

Three design concerns flagged inline: a safe_downcast regression in NetworkOPs.cpp, an over-constrained concept in RelationalDatabase.h, and a pervasive reference_wrapper ergonomics issue in ApplyContext.h (and related structs).

Review by Claude Opus 4.6 · Prompt: V12

src/xrpld/app/misc/NetworkOPs.cpp

include/xrpl/rdb/RelationalDatabase.h

include/xrpl/tx/ApplyContext.h

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

…dularisation

xrplf-ai-reviewer

Two low-to-medium issues flagged inline: a lost runtime type assertion in NetworkOPs.cpp and a misleading concept constraint in RelationalDatabase.h.

Review by Claude Opus 4.6 · Prompt: V12

src/xrpld/app/misc/NetworkOPs.cpp

include/xrpl/rdb/RelationalDatabase.h

github-actions · 2026-03-26T14:31:33Z

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

…pr-comments-after-tx-modularisation

github-actions · 2026-03-30T13:53:06Z

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

bthomee · 2026-03-30T13:56:32Z

@a1q123456 I'll wait with merging this PR until the MPT on DEX PR has been merged, as that one is a bit on the large size and this PR might introduce conflicts that will be harder to resolve than the other way around.

bthomee · 2026-03-30T13:58:57Z

@a1q123456 I'll wait with merging this PR until the MPT on DEX PR has been merged, as that one is a bit on the large size and this PR might introduce conflicts that will be harder to resolve than the other way around.

@gregtatcam said it was ok to merge this one - will have it done the moment the CI passes.

xrplf-ai-reviewer

Two issues flagged inline: a removed type-safety guard in the Sqlite dispatch branch, and an unguarded this capture in the job queue lambda.

Review by Claude Opus 4.6 · Prompt: V12

src/xrpld/app/misc/NetworkOPs.cpp

xrplf-ai-reviewer

Gave this a look

One redundant concept constraint flagged inline — see line 461.

Review by ReviewBot 🤖

Review by Claude Opus 4.6 · Prompt: V12

include/xrpl/rdb/RelationalDatabase.h

Signed-off-by: JCW <a1q123456@users.noreply.github.com> Co-authored-by: Bart <bthomee@users.noreply.github.com>

Address PR comments in #6223

376be0d

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

This was referenced Feb 19, 2026

Modularise WalletDB and Manifest #6223

Merged

Modularise the NetworkOPs interface #6225

Merged

Modularise HashRouter, Conditions, and OrderBookDB #6226

Merged

Decouple app/tx from Application and Config #6227

Merged

a1q123456 added 3 commits February 19, 2026 15:59

Address PR comments in #6224

1f84174

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

Address PR comments in #6225

276fe7d

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

Address PR comments in #6226

ce23f20

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

a1q123456 force-pushed the a1q123456/cleanup-pr-comments-after-tx-modularisation branch from 9fca482 to ce23f20 Compare February 19, 2026 15:59

a1q123456 requested review from godexsoft, kuznetsss and pratikmankawde February 19, 2026 15:59

a1q123456 marked this pull request as ready for review February 19, 2026 16:00

Fix warnings

99bda74

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

godexsoft requested changes Feb 20, 2026

View reviewed changes

src/xrpld/app/misc/make_NetworkOPs.h Outdated Show resolved Hide resolved

src/xrpld/app/misc/NetworkOPs.cpp Outdated Show resolved Hide resolved

a1q123456 added 2 commits February 24, 2026 14:33

Replace ioSvc with ioCtx

860bb28

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

a1q123456 requested a review from godexsoft February 24, 2026 15:26

a1q123456 added 2 commits March 5, 2026 12:24

Merge branch 'develop' into a1q123456/cleanup-pr-comments-after-tx-mo…

b5e1360

…dularisation

Merge remote-tracking branch 'origin/develop' into a1q123456/cleanup-…

9fee71b

…pr-comments-after-tx-modularisation

godexsoft approved these changes Mar 18, 2026

View reviewed changes

kuznetsss approved these changes Mar 18, 2026

View reviewed changes

xrplf-ai-reviewer bot reviewed Mar 20, 2026

View reviewed changes

include/xrpl/rdb/RelationalDatabase.h Outdated Show resolved Hide resolved

include/xrpl/rdb/RelationalDatabase.h Outdated Show resolved Hide resolved

src/xrpld/app/misc/NetworkOPs.cpp Show resolved Hide resolved

src/xrpld/app/rdb/backend/detail/Node.cpp Show resolved Hide resolved

a1q123456 added 3 commits March 25, 2026 16:34

Address comments

404de2d

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

Address comments

8c26e6f

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

a1q123456 changed the title ~~Address PR comments after the modularisation PRs~~ refactor: Address PR comments after the modularisation PRs Mar 25, 2026

xrplf-ai-reviewer bot reviewed Mar 25, 2026

View reviewed changes

src/xrpld/app/rdb/backend/detail/Node.cpp Outdated Show resolved Hide resolved

src/xrpld/app/misc/NetworkOPs.cpp Show resolved Hide resolved

include/xrpl/tx/ApplyContext.h Show resolved Hide resolved

Merge branch 'develop' into a1q123456/cleanup-pr-comments-after-tx-mo…

129fd8f

…dularisation

xrplf-ai-reviewer bot reviewed Mar 26, 2026

View reviewed changes

src/xrpld/app/rdb/backend/detail/Node.cpp Outdated Show resolved Hide resolved

src/xrpld/app/rdb/backend/detail/SQLiteDatabase.cpp Outdated Show resolved Hide resolved

include/xrpl/rdb/RelationalDatabase.h Show resolved Hide resolved

src/xrpld/app/misc/NetworkOPs.cpp Show resolved Hide resolved

Fix issues

96fa698

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

xrplf-ai-reviewer bot reviewed Mar 26, 2026

View reviewed changes

src/xrpld/app/misc/NetworkOPs.cpp Show resolved Hide resolved

include/xrpl/rdb/RelationalDatabase.h Show resolved Hide resolved

include/xrpl/tx/ApplyContext.h Show resolved Hide resolved

a1q123456 added 2 commits March 26, 2026 13:51

Fix issues

afc50f7

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

Merge branch 'develop' into a1q123456/cleanup-pr-comments-after-tx-mo…

fc17969

…dularisation

xrplf-ai-reviewer bot reviewed Mar 26, 2026

View reviewed changes

src/xrpld/app/misc/NetworkOPs.cpp Show resolved Hide resolved

include/xrpl/rdb/RelationalDatabase.h Show resolved Hide resolved

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

Merge remote-tracking branch 'origin/develop' into a1q123456/cleanup-…

7476e8b

…pr-comments-after-tx-modularisation

github-actions bot removed the PR: has conflicts label Mar 30, 2026

a1q123456 removed the request for review from pratikmankawde March 30, 2026 13:53

a1q123456 added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Mar 30, 2026

bthomee enabled auto-merge March 30, 2026 13:59

xrplf-ai-reviewer bot reviewed Mar 30, 2026

View reviewed changes

src/xrpld/app/misc/NetworkOPs.cpp Show resolved Hide resolved

src/xrpld/app/misc/NetworkOPs.cpp Show resolved Hide resolved

Fix clang-tidy error

02fc2f3

xrplf-ai-reviewer bot reviewed Mar 30, 2026

View reviewed changes

include/xrpl/rdb/RelationalDatabase.h Show resolved Hide resolved

bthomee added this pull request to the merge queue Mar 30, 2026

Merged via the queue into develop with commit 3a477e4 Mar 30, 2026
33 of 34 checks passed

bthomee deleted the a1q123456/cleanup-pr-comments-after-tx-modularisation branch March 30, 2026 15:42

bthomee added a commit that referenced this pull request Mar 30, 2026

refactor: Address PR comments after the modularisation PRs (#6389)

48dac13

Signed-off-by: JCW <a1q123456@users.noreply.github.com> Co-authored-by: Bart <bthomee@users.noreply.github.com>

bthomee added a commit that referenced this pull request Mar 30, 2026

refactor: Address PR comments after the modularisation PRs (#6389)

b36aedb

Signed-off-by: JCW <a1q123456@users.noreply.github.com> Co-authored-by: Bart <bthomee@users.noreply.github.com>

Conversation

a1q123456 commented Feb 19, 2026

High Level Overview of Change

Context of Change

Type of Change

API Impact

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

godexsoft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

godexsoft left a comment

Choose a reason for hiding this comment

Uh oh!

kuznetsss left a comment

Choose a reason for hiding this comment

Uh oh!

bthomee commented Mar 20, 2026

Uh oh!

xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Mar 26, 2026

Uh oh!

github-actions bot commented Mar 30, 2026

Uh oh!

bthomee commented Mar 30, 2026

Uh oh!

bthomee commented Mar 30, 2026

Uh oh!

xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

codecov bot commented Feb 19, 2026 •

edited

Loading