Skip to content

HashRouter flags refactor#5371

Merged
bthomee merged 14 commits intodevelopfrom
vlntb/5336-hashrouter-flags-refactor
Jul 23, 2025
Merged

HashRouter flags refactor#5371
bthomee merged 14 commits intodevelopfrom
vlntb/5336-hashrouter-flags-refactor

Conversation

@vlntb
Copy link
Copy Markdown
Contributor

@vlntb vlntb commented Mar 27, 2025 •
edited
Loading

High Level Overview of Change

This change is addressing the issue #5336 : Refactor HashRouter flags to be more type-safe

  • Switched numeric flags to enum type
  • Updated unit tests

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)

@vlntb vlntb mentioned this pull request Apr 3, 2025
Closed
@vlntb vlntb marked this pull request as ready for review April 3, 2025 13:37
@vlntb vlntb requested review from mvadari and ximinez April 3, 2025 13:37
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 3, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 68.08511% with 15 lines in your changes missing coverage. Please review.

Project coverage is 78.4%. Comparing base (7ff4f79) to head (b0e86cf).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
src/xrpld/overlay/detail/PeerImp.cpp 0.0% 11 Missing ⚠️
src/xrpld/app/misc/NetworkOPs.cpp 55.6% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5371     +/-   ##
=========================================
- Coverage     78.4%   78.4%   -0.0%     
=========================================
  Files          816     816             
  Lines        71715   71734     +19     
  Branches      8577    8579      +2     
=========================================
+ Hits         56250   56262     +12     
- Misses       15465   15472      +7
Files with missing lines Coverage Δ
src/xrpld/app/ledger/Ledger.cpp 82.9% <100.0%> (+<0.1%) ⬆️
src/xrpld/app/misc/HashRouter.cpp 100.0% <100.0%> (ø)
src/xrpld/app/misc/HashRouter.h 100.0% <100.0%> (ø)
src/xrpld/app/tx/detail/Escrow.cpp 93.6% <100.0%> (ø)
src/xrpld/app/tx/detail/apply.cpp 93.3% <100.0%> (ø)
src/xrpld/overlay/detail/PeerImp.h 13.2% <ø> (ø)
src/xrpld/app/misc/NetworkOPs.cpp 70.4% <55.6%> (-<0.1%) ⬇️
src/xrpld/overlay/detail/PeerImp.cpp 3.7% <0.0%> (-<0.1%) ⬇️

... and 6 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@vlntb vlntb changed the title wip: hashrouter flags refactor HashRouter flags refactor Apr 7, 2025
mvadari
mvadari reviewed Apr 7, 2025
View reviewed changes
mvadari
mvadari reviewed Apr 7, 2025
View reviewed changes
@vlntb vlntb requested a review from a team May 28, 2025 11:31
@vlntb vlntb requested a review from mvadari May 28, 2025 13:16
@bthomee bthomee requested a review from a1q123456 June 20, 2025 15:06
a1q123456
a1q123456 approved these changes Jul 23, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@a1q123456 a1q123456 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me.
Having an enum to replace integer flag values is a good idea, but I think it'd be better to have a generic Flag<enum class> type that provides all the methods we've done in HashRouter.h

I'll create a ticket for this

@vlntb vlntb 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 Jul 23, 2025
@bthomee bthomee enabled auto-merge (squash) July 23, 2025 11:04
@bthomee bthomee merged commit c233df7 into develop Jul 23, 2025
2 of 3 checks passed
@bthomee bthomee deleted the vlntb/5336-hashrouter-flags-refactor branch July 23, 2025 12:03
ximinez added a commit that referenced this pull request Jul 24, 2025
@ximinez
Merge remote-tracking branch 'XRPLF/ximinez/lending-refactoring-2' in…
760a9aa 
…to ximinez/lending-refactoring-3

* XRPLF/ximinez/lending-refactoring-2:
  refactor: Update rocksdb (#5568)
  Switch instrumentation workflow to use dependencies (#5607)
  chore: Rename conan profile to `default` (#5599)
  Include `network_id` in validations and subscription stream responses (#5579)
  Add support for `DomainID` in `MPTokenIssuance` transactions (#5509)
  chore: Remove unused code after flow cross retirement (#5575)
  Remove obsolete owner pays fee feature and XRPL_ABANDON stanza (#5550)
  refactor: Makes HashRouter flags more type-safe (#5371)
  Fix clang-format CI job (#5598)
This was referenced Aug 27, 2025
Merged
Merged
@mvadari mvadari linked an issue Aug 29, 2025 that may be closed by this pull request
Refactor HashRouter flags to be more type-safe #5336
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@a1q123456 a1q123456 a1q123456 approved these changes

@mvadari mvadari mvadari approved these changes

@ximinez ximinez Awaiting requested review from ximinez

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Refactor HashRouter flags to be more type-safe

4 participants

@vlntb @a1q123456 @mvadari @bthomee