Sponsored Content
Skip to content

breaks down InvariantCheck to multiple classes - #6440

Merged
bthomee merged 5 commits into
developfrom
tapanito/refactor-invariant-check
Feb 27, 2026
Merged

breaks down InvariantCheck to multiple classes#6440
bthomee merged 5 commits into
developfrom
tapanito/refactor-invariant-check

Conversation

@Tapanito

Copy link
Copy Markdown
Contributor

Summary

  • Split InvariantCheck.h (733 lines) and InvariantCheck.cpp (3,483 lines) into 10 focused header/source pairs organized by domain under a new invariants/ subdirectory
  • Extract the shared Privilege enum and hasPrivilege() function into a dedicated InvariantCheckPrivilege.h header so domain-specific files can reference them independently

Motivation

The invariant check system had grown into a single monolithic file pair containing 24 invariant checker classes. At 3,483 lines, InvariantCheck.cpp was a frequent source of merge conflicts and difficult to navigate. This refactoring improves maintainability and readability with zero behavioral changes.

File layout

include/xrpl/tx/invariants/
  InvariantCheck.h              # Core invariants + InvariantChecks tuple + sub-header aggregation
  InvariantCheckPrivilege.h     # Privilege enum, operator|, hasPrivilege() declaration
  AMMInvariant.h                # ValidAMM
  FreezeInvariant.h             # TransfersNotFrozen
  LoanInvariant.h               # ValidLoanBroker, ValidLoan
  MPTInvariant.h                # ValidMPTIssuance
  NFTInvariant.h                # ValidNFTokenPage, NFTokenCountTracking
  PermissionedDEXInvariant.h    # ValidPermissionedDEX
  PermissionedDomainInvariant.h # ValidPermissionedDomain
  VaultInvariant.h              # ValidVault

src/libxrpl/tx/invariants/
  InvariantCheck.cpp              # Core invariant impls + hasPrivilege() definition
  AMMInvariant.cpp                # ValidAMM + validBalances()
  FreezeInvariant.cpp             # TransfersNotFrozen (all methods)
  LoanInvariant.cpp               # ValidLoanBroker, ValidLoan
  MPTInvariant.cpp                # ValidMPTIssuance
  NFTInvariant.cpp                # ValidNFTokenPage, NFTokenCountTracking
  PermissionedDEXInvariant.cpp    # ValidPermissionedDEX
  PermissionedDomainInvariant.cpp # ValidPermissionedDomain
  VaultInvariant.cpp              # ValidVault

Design decisions

  • InvariantCheck.h remains the single entry point: It #includes all sub-headers and still defines the InvariantChecks tuple and getInvariantChecks(). Consumers like ApplyContext.cpp only update the include path — no logic changes.
  • InvariantCheckPrivilege.h: The Privilege enum and hasPrivilege() are used by 6 different invariant classes across separate translation units. The enum and constexpr operator| live in the header; hasPrivilege() is declared there but defined in InvariantCheck.cpp because it uses the transactions.macro X-macro which requires single-TU compilation.
  • No CMake changes needed: add_module(xrpl tx) uses GLOB_RECURSE which automatically discovers files in subdirectories.

Next Steps

  • Breakdown Invariants_test into separate classes, see Refactors vault invariant tests to test all asset types. #6269 for an example.
  • Breakdown each top level invariant into per-transaction invariants. For example, VaultInvariant would become:
    • VaultInvariant < ---- Holds base invariants that must hold true for the object
    • VaultCreateInvariant < ----- Holds invariants associated with the VaultCreate transaction
    • VaultDeleteInvariant < ----- Holds invariants associated with the VaultCreate transaction
      Vault...Invariant

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)

@Tapanito
Tapanito requested a review from ximinez February 26, 2026 17:30
@codecov

codecov Bot commented Feb 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.07642% with 106 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.8%. Comparing base (404f35d) to head (918bf5e).
⚠️ Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
src/libxrpl/tx/invariants/MPTInvariant.cpp 73.3% 24 Missing ⚠️
src/libxrpl/tx/invariants/NFTInvariant.cpp 79.5% 24 Missing ⚠️
src/libxrpl/tx/invariants/LoanInvariant.cpp 82.5% 20 Missing ⚠️
src/libxrpl/tx/invariants/InvariantCheck.cpp 95.8% 17 Missing ⚠️
src/libxrpl/tx/invariants/VaultInvariant.cpp 98.2% 9 Missing ⚠️
src/libxrpl/tx/invariants/FreezeInvariant.cpp 91.2% 7 Missing ⚠️
src/libxrpl/tx/invariants/AMMInvariant.cpp 95.3% 5 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #6440   +/-   ##
=======================================
  Coverage     79.8%   79.8%           
=======================================
  Files          848     858   +10     
  Lines        67757   67757           
  Branches      7558    7550    -8     
=======================================
+ Hits         54074   54077    +3     
+ Misses       13683   13680    -3     
Files with missing lines Coverage Δ
include/xrpl/tx/invariants/AMMInvariant.h 100.0% <100.0%> (ø)
include/xrpl/tx/invariants/InvariantCheck.h 100.0% <100.0%> (ø)
...clude/xrpl/tx/invariants/InvariantCheckPrivilege.h 100.0% <100.0%> (ø)
src/libxrpl/tx/ApplyContext.cpp 100.0% <ø> (ø)
...libxrpl/tx/invariants/PermissionedDEXInvariant.cpp 100.0% <100.0%> (ø)
...xrpl/tx/invariants/PermissionedDomainInvariant.cpp 100.0% <100.0%> (ø)
...ctors/PermissionedDomain/PermissionedDomainSet.cpp 100.0% <ø> (ø)
src/libxrpl/tx/invariants/AMMInvariant.cpp 95.3% <95.3%> (ø)
src/libxrpl/tx/invariants/FreezeInvariant.cpp 91.2% <91.2%> (ø)
src/libxrpl/tx/invariants/VaultInvariant.cpp 98.2% <98.2%> (ø)
... and 4 more

... and 5 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.

@Tapanito
Tapanito requested review from godexsoft and kuznetsss and removed request for ximinez February 27, 2026 12:39

@godexsoft godexsoft 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 like this new structure. We should consider adding a namespace like 'invariants' for all of this (could be done in a separate pr if there is interest).

To fix pre-commit you may want to setup pre-commit hooks so that clang-format automatically fixes code when you commit/push.

@kuznetsss kuznetsss 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.

👍

@Tapanito Tapanito 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 Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. labels Feb 27, 2026
@Tapanito

Copy link
Copy Markdown
Contributor Author

I like this new structure. We should consider adding a namespace like 'invariants' for all of this (could be done in a separate pr if there is interest).

To fix pre-commit you may want to setup pre-commit hooks so that clang-format automatically fixes code when you commit/push.

Thanks @godexsoft ! I'm not sure why my local formatter keeps making these changes... I configured pre-commit

@Tapanito Tapanito 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 Feb 27, 2026
@bthomee
bthomee enabled auto-merge (squash) February 27, 2026 18:40
@bthomee
bthomee merged commit 1a7f824 into develop Feb 27, 2026
3 checks passed
@bthomee
bthomee deleted the tapanito/refactor-invariant-check branch February 27, 2026 21:02
@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
The invariant check system had grown into a single monolithic file pair containing 24 invariant checker classes. The large `InvariantCheck.cpp` file was a frequent source of merge conflicts and difficult to navigate. This refactoring improves maintainability and readability with zero behavioral changes.

In particular, this change:
- Splits `InvariantCheck.h` and `InvariantCheck.cpp` into 10 focused header/source pairs organized by domain under a new `invariants/` subdirectory.
- Extracts the shared `Privilege` enum and `hasPrivilege()` function into a dedicated `InvariantCheckPrivilege.h` header, so domain-specific files can reference them independently.
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.

5 participants