refactor: Move ledger entry helper functions from View.h/View.cpp to dedicated helper files - #6453
Conversation
…dedicated helper files
There was a problem hiding this comment.
Pull request overview
This PR refactors ledger-entry helper logic out of View.h/View.cpp into dedicated helper headers/sources grouped by ledger entry type (AccountRoot, RippleState, MPToken, Offer, Directory, Vault, and token-agnostic dispatch), as groundwork for the “Typed Ledger Entry Views” proposal while keeping existing callers working via View.h includes.
Changes:
- Split helper implementations into new
*Helpers.h/.cppfiles and updatedView.hto include them. - Introduced token-agnostic dispatch layer (
TokenHelpers.*) for XRP/IOU/MPT behavior viastd::visit. - Deprecated
Credit.*implementation files and redirected the API toRippleStateHelpers.*while keepingCredit.*as compatibility shims.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/test/app/Vault_test.cpp | Adds an include in vault unit tests (appears to be unused). |
| src/libxrpl/ledger/VaultHelpers.cpp | New vault share/asset conversion helper implementations. |
| src/libxrpl/ledger/TokenHelpers.cpp | New token-agnostic helper implementations (balance, freeze, transfers, dispatch). |
| src/libxrpl/ledger/RippleStateHelpers.cpp | New trustline (IOU) helpers including credit/freeze/auth and trustline ops. |
| src/libxrpl/ledger/OfferHelpers.cpp | New offer deletion helper implementation. |
| src/libxrpl/ledger/MPTokenHelpers.cpp | New MPToken helpers (freeze/auth/escrow/transfer, etc.). |
| src/libxrpl/ledger/DirectoryHelpers.cpp | New directory iteration and directory utility helper implementations. |
| src/libxrpl/ledger/Credit.cpp | Deprecated shim translation unit retained for build compatibility. |
| src/libxrpl/ledger/AccountRootHelpers.cpp | New account-root helpers (owner count, liquid XRP, pseudo-account utilities, etc.). |
| include/xrpl/tx/paths/detail/StrandFlow.h | Adds View.h include to satisfy dependencies after header refactor. |
| include/xrpl/ledger/View.h | Removes many helpers and replaces them with includes of the new helper headers. |
| include/xrpl/ledger/VaultHelpers.h | Declares vault conversion helper APIs and supporting enum. |
| include/xrpl/ledger/TokenHelpers.h | Declares token-agnostic dispatch helpers + shared enums used by callers. |
| include/xrpl/ledger/RippleStateHelpers.h | Declares IOU/trustline helpers (credit, freeze, auth, trustline ops, etc.). |
| include/xrpl/ledger/OfferHelpers.h | Declares offer deletion helper API. |
| include/xrpl/ledger/MPTokenHelpers.h | Declares MPToken helper APIs (freeze/auth/escrow/transfer, etc.). |
| include/xrpl/ledger/DirectoryHelpers.h | Declares directory iteration helpers and related utilities. |
| include/xrpl/ledger/Credit.h | Deprecated shim header redirecting to RippleStateHelpers.h. |
| include/xrpl/ledger/AccountRootHelpers.h | Declares account-root helper APIs (liquid XRP, owner count, pseudo-accounts, etc.). |
Comments suppressed due to low confidence (4)
src/libxrpl/ledger/VaultHelpers.cpp:66
- The assert message string uses
assetsToSharesDepositinsideassetsToSharesWithdraw, which is misleading when diagnosing assertion failures. Update the message to referenceassetsToSharesWithdraw.
XRPL_ASSERT(!assets.negative(), "xrpl::assetsToSharesDeposit : non-negative assets");
XRPL_ASSERT(
assets.asset() == vault->at(sfAsset),
src/libxrpl/ledger/VaultHelpers.cpp:92
- The assert message string uses
sharesToAssetsDepositinsidesharesToAssetsWithdraw, which is misleading when diagnosing assertion failures. Update the message to referencesharesToAssetsWithdraw.
XRPL_ASSERT(!shares.negative(), "xrpl::sharesToAssetsDeposit : non-negative shares");
XRPL_ASSERT(
shares.asset() == vault->at(sfShareMPTID),
src/libxrpl/ledger/TokenHelpers.cpp:1017
- Typo in the
XRPL_ASSERT_PARTSlabel string:"rippler::rippleSendMultiMPT"should be"xrpl::rippleSendMultiMPT"(or consistent with the surrounding function naming).
XRPL_ASSERT_PARTS(
takeFromSender == beast::zero,
"rippler::rippleSendMultiMPT",
"sender == issuer, takeFromSender == zero");
include/xrpl/tx/paths/detail/StrandFlow.h:6
- Including
<xrpl/ledger/View.h>here significantly increases compile-time dependencies. This header appears to only need specific symbols (e.g.keylet/ReadView), so prefer including the narrowest headers that provide the required declarations instead of the full View interface.
#include <xrpl/basics/Log.h>
#include <xrpl/ledger/Credit.h>
#include <xrpl/ledger/View.h>
#include <xrpl/protocol/Feature.h>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
src/libxrpl/ledger/VaultHelpers.cpp:67
- In assetsToSharesWithdraw(), the XRPL_ASSERT message string still says "assetsToSharesDeposit". This makes debug assertions misleading; please update the message to reference assetsToSharesWithdraw().
XRPL_ASSERT(!assets.negative(), "xrpl::assetsToSharesDeposit : non-negative assets");
XRPL_ASSERT(
assets.asset() == vault->at(sfAsset),
"xrpl::assetsToSharesWithdraw : assets and vault match");
src/libxrpl/ledger/VaultHelpers.cpp:93
- In sharesToAssetsWithdraw(), the XRPL_ASSERT message string still says "sharesToAssetsDeposit". Please update the assertion text so it matches the withdraw function for clearer diagnostics.
XRPL_ASSERT(!shares.negative(), "xrpl::sharesToAssetsDeposit : non-negative shares");
XRPL_ASSERT(
shares.asset() == vault->at(sfShareMPTID),
"xrpl::sharesToAssetsWithdraw : shares and vault match");
src/libxrpl/ledger/TokenHelpers.cpp:1017
- XRPL_ASSERT_PARTS in rippleSendMultiMPT uses the string "rippler::rippleSendMultiMPT" (typo). This should likely be "xrpl::rippleSendMultiMPT" to match the namespace used elsewhere and avoid confusing assertion output.
XRPL_ASSERT_PARTS(
takeFromSender == beast::zero,
"rippler::rippleSendMultiMPT",
"sender == issuer, takeFromSender == zero");
src/libxrpl/ledger/AccountRootHelpers.cpp:59
- In confineOwnerCount(), the underflow branch logs a fatal message when
idis set, but then asserts!idwith the message "id is not set". This assertion/message pairing is confusing and makes it hard to interpret debug failures; consider making the assertion intent explicit (e.g., assert false with an "unexpected underflow" message whenidis provided, or adjust the message/condition accordingly).
JLOG(j.fatal()) << "Account " << *id << " owner count set below 0!";
}
adjusted = 0;
XRPL_ASSERT(!id, "xrpl::confineOwnerCount : id is not set");
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
src/libxrpl/ledger/TokenHelpers.cpp:1018
- The XRPL_ASSERT_PARTS tag string has a typo (
rippler::rippleSendMultiMPT). This should match the surrounding namespace/function naming (xrpl::rippleSendMultiMPT) to keep assertion diagnostics consistent and searchable.
XRPL_ASSERT_PARTS(
takeFromSender == beast::zero,
"rippler::rippleSendMultiMPT",
"sender == issuer, takeFromSender == zero");
auto const sendAmount = amount.mpt().value();
include/xrpl/tx/paths/detail/StrandFlow.h:6
<xrpl/ledger/View.h>appears to be unused in this header (no symbols fromView.hare referenced directly here), and it is a particularly heavyweight include. Consider removing it, or replacing it with the narrowest header(s) that provide the needed declarations if something is indirectly relying on it.
#include <xrpl/ledger/Credit.h>
#include <xrpl/ledger/View.h>
#include <xrpl/protocol/Feature.h>
src/libxrpl/ledger/VaultHelpers.cpp:66
- The XRPL_ASSERT message string references
assetsToSharesDeposit, but this function isassetsToSharesWithdraw. This makes assertion failures harder to diagnose and can mislead debugging; update the assertion text to match the function name.
XRPL_ASSERT(!assets.negative(), "xrpl::assetsToSharesDeposit : non-negative assets");
XRPL_ASSERT(
assets.asset() == vault->at(sfAsset),
src/libxrpl/ledger/VaultHelpers.cpp:93
- The XRPL_ASSERT message string references
sharesToAssetsDeposit, but this function issharesToAssetsWithdraw. Updating the assertion text will make failures easier to interpret.
XRPL_ASSERT(!shares.negative(), "xrpl::sharesToAssetsDeposit : non-negative shares");
XRPL_ASSERT(
shares.asset() == vault->at(sfShareMPTID),
"xrpl::sharesToAssetsWithdraw : shares and vault match");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #6453 +/- ##
=========================================
- Coverage 81.5% 81.4% -0.0%
=========================================
Files 988 997 +9
Lines 74411 74416 +5
Branches 7579 7557 -22
=========================================
+ Hits 60608 60609 +1
- Misses 13803 13807 +4
🚀 New features to boost your workflow:
|
Tapanito
left a comment
There was a problem hiding this comment.
This is a great change. LGTM.
|
I think it'd be nice if we could create some jira tickets for the follow up work (i.e. use the auto generated class) to keep things on track |
Happy to, but I'd like to get this PR merged first so that I can open up the PRs that I have so far. |
| #include <xrpl/ledger/ApplyView.h> | ||
| #include <xrpl/ledger/CredentialHelpers.h> | ||
| #include <xrpl/ledger/View.h> | ||
| #include <xrpl/ledger/entries/CredentialHelpers.h> |
There was a problem hiding this comment.
Is ledger/entries the best name? We've got xrpl/protocol_autogen/ledger_entries, which people will confuse it with xrpl/ledger/entries.
I think ledger/operations, ledger/helpers, ledger/modifiers, or ledger/views are better
There was a problem hiding this comment.
helpers is fine for now, but these will eventually morph into classes for each ledger entry, so we may want to revisit naming then.
There was a problem hiding this comment.
Yeah maybe in the future, we can use ledger/modifiers or something, but that’s for future
a1q123456
left a comment
There was a problem hiding this comment.
let's move the header files as well
…dedicated helper files (#6453) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…dedicated helper files (XRPLF#6453) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
High Level Overview of Change
Reorganizes helper functions from
View.h/View.cppinto dedicated files based on the ledger entry type they operate on. This is a preparatory step for the Typed Ledger Entry Views proposal.There are no changes to functionality, only functions moved to new files.
New helper files created:
DirectoryHelpers.h/cpp- Directory iteration (dirFirst,dirNext,cdirFirst,cdirNext,forEachItem, etc.)AccountRootHelpers.h/cpp- Account operations (xrpLiquid,adjustOwnerCount,transferRate,isPseudoAccount, etc.)RippleStateHelpers.h/cpp- Trust line operations (isFrozen,trustCreate,trustDelete,issueIOU,redeemIOU, etc.)MPTokenHelpers.h/cpp- MPToken operations (isFrozen,transferRate,authorizeMPToken,accountHolds, etc.)OfferHelpers.h/cpp- Offer operations (offerDelete)VaultHelpers.h/cpp- Vault calculations (assetsToSharesDeposit,sharesToAssetsWithdraw, etc.)TokenHelpers.h/cpp- Asset-agnostic dispatchers that work with XRP/IOU/MPT viastd::visitContext of Change
View.hhad grown to contain helper functions for many different ledger entry types, making it difficult to find and maintain ledger-entry-specific logic. This refactoring:View.hwhere they cross unrelated entry type boundariesType of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)Before / After
Before: All ledger entry helper functions lived in
View.h/View.cpp(~1500+ lines)After: Functions organized by ledger entry type:
View.h/cpp- Core view classes + cross-cutting functions (~530 lines)DirectoryHelpers.h/cpp- ltDIR_NODEAccountRootHelpers.h/cpp- ltACCOUNT_ROOTRippleStateHelpers.h/cpp- ltRIPPLE_STATEMPTokenHelpers.h/cpp- ltMPTOKEN / ltMPTOKEN_ISSUANCEOfferHelpers.h/cpp- ltOFFERVaultHelpers.h/cpp- ltVAULTTokenHelpers.h/cpp- Asset-agnostic (XRP/IOU/MPT) dispatchersNo functional changes - existing code continues to work through includes in
View.h.Test Plan