Sponsored Content
Skip to content

Add ServiceRegistry to help migration - #6222

Merged
bthomee merged 9 commits into
developfrom
a1q123456/add_service_registry
Feb 3, 2026
Merged

Add ServiceRegistry to help migration#6222
bthomee merged 9 commits into
developfrom
a1q123456/add_service_registry

Conversation

@a1q123456

@a1q123456 a1q123456 commented Jan 15, 2026

Copy link
Copy Markdown
Contributor

High Level Overview of Change

This PR adds a service registry class.

Context of Change

We're passing Application around and the Application class acts more like a service registry that gives other classes access to other services. In order to allow modularisation, we should replace Application with a service registry class so that modules depending on Application for other services can be moved easily.

This PR is part of the modularisation of the transactors. See all PRs here:
#6222
#6223
#6224
#6225
#6226
#6227
#6228

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

@codecov

codecov Bot commented Jan 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.9%. Comparing base (7813683) to head (e14cffa).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #6222   +/-   ##
=======================================
  Coverage     79.9%   79.9%           
=======================================
  Files          839     840    +1     
  Lines        65546   65548    +2     
  Branches      7269    7266    -3     
=======================================
+ Hits         52346   52348    +2     
  Misses       13200   13200           
Files with missing lines Coverage Δ
include/xrpl/core/ServiceRegistry.h 100.0% <100.0%> (ø)
src/xrpld/app/main/Application.h 100.0% <ø> (ø)

... and 2 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bthomee

bthomee commented Jan 30, 2026

Copy link
Copy Markdown
Collaborator

@vvysokikh1 how does this look to you?

@a1q123456 a1q123456 added the Needs additional review PR requires at least one more code review approval before it can be merged label Jan 30, 2026

@vvysokikh1 vvysokikh1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any concrete numbers on how this will affect performance, but this change adds another layer of virtual dispatch into the picture. Considering functions from application being called multiple times anywhere in a hot path, I'd suggest we consider something like CRTP to design the registry (that should accept app as a template param).

Comment thread include/xrpl/core/ServiceRegistry.h Outdated
Comment thread src/test/core/ServiceRegistry_test.cpp Outdated
Comment thread src/test/core/ServiceRegistry_test.cpp Outdated
Signed-off-by: JCW <a1q123456@users.noreply.github.com>
Signed-off-by: JCW <a1q123456@users.noreply.github.com>
Signed-off-by: JCW <a1q123456@users.noreply.github.com>
Signed-off-by: JCW <a1q123456@users.noreply.github.com>
Signed-off-by: JCW <a1q123456@users.noreply.github.com>
@a1q123456
a1q123456 force-pushed the a1q123456/add_service_registry branch 2 times, most recently from 524d1fc to 7c5faca Compare February 3, 2026 16:21
Signed-off-by: JCW <a1q123456@users.noreply.github.com>
@a1q123456
a1q123456 force-pushed the a1q123456/add_service_registry branch 2 times, most recently from 7c5faca to 09b52e4 Compare February 3, 2026 16:23
@a1q123456
a1q123456 requested a review from vvysokikh1 February 3, 2026 16:25
Signed-off-by: JCW <a1q123456@users.noreply.github.com>
@a1q123456 a1q123456 added Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. and removed Needs additional review PR requires at least one more code review approval before it can be merged labels Feb 3, 2026
Comment thread include/xrpl/core/ServiceRegistry.h Outdated
@bthomee

bthomee commented Feb 3, 2026

Copy link
Copy Markdown
Collaborator

There's an unrelated file, src/xrpld/app/tx/detail/Taker.h with only whitespace changes (according to the "New GitHub Experience") or no changes (according to the legacy experience) included in this PR. The file curiously is actually empty.

I would suggest to revert the modification to the empty file in this PR, and then to delete it later as part of a modularization PR.

Signed-off-by: JCW <a1q123456@users.noreply.github.com>
@bthomee
bthomee enabled auto-merge (squash) February 3, 2026 18:31
@bthomee
bthomee merged commit 6c1a92f into develop Feb 3, 2026
1 check passed
@bthomee
bthomee deleted the a1q123456/add_service_registry branch February 3, 2026 19:08
mvadari pushed a commit to mvadari/rippled that referenced this pull request Feb 4, 2026
Currently we're passing the `Application` object around, whereby the `Application` class acts more like a service registry that gives other classes access to other services. In order to allow modularization, we should replace `Application` with a service registry class so that modules depending on `Application` for other services can be moved easily. This change adds the `ServiceRegistry` class.
mvadari pushed a commit that referenced this pull request Feb 4, 2026
Currently we're passing the `Application` object around, whereby the `Application` class acts more like a service registry that gives other classes access to other services. In order to allow modularization, we should replace `Application` with a service registry class so that modules depending on `Application` for other services can be moved easily. This change adds the `ServiceRegistry` class.
@mvadari mvadari added this to the 3.2.0 milestone May 20, 2026
beartec-jpg pushed a commit to beartec-jpg/FalconLedger that referenced this pull request Jun 1, 2026
Currently we're passing the `Application` object around, whereby the `Application` class acts more like a service registry that gives other classes access to other services. In order to allow modularization, we should replace `Application` with a service registry class so that modules depending on `Application` for other services can be moved easily. This change adds the `ServiceRegistry` class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants