refactor: Use ApplyViewContext everywhere instead of ApplyView+STTx#7618
refactor: Use ApplyViewContext everywhere instead of ApplyView+STTx#7618mvadari wants to merge 10 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## xrplf/sponsor #7618 +/- ##
===============================================
- Coverage 82.2% 82.2% -0.1%
===============================================
Files 1016 1016
Lines 78385 78214 -171
Branches 8994 9000 +6
===============================================
- Hits 64462 64255 -207
- Misses 13914 13950 +36
Partials 9 9
🚀 New features to boost your workflow:
|
… mvadari/sponsor/apply-view-context
There was a problem hiding this comment.
Pull request overview
Refactors transaction application helpers and transactors to pass a unified ApplyViewContext (view + tx) instead of separate ApplyView and STTx parameters, aiming to simplify call sites and reduce parameter threading across the tx/ledger helper layer.
Changes:
- Introduces
ApplyViewContextand updates many helper APIs (doWithdraw, token/escrow/sponsor helpers, etc.) to take it. - Adds
ApplyContext::getApplyViewContext()and updates many transactors to use it. - Updates internal implementations to read
ctx.view/ctx.txinstead of separate parameters.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp | Switches helper calls to pass ctx_.getApplyViewContext() instead of (view(), ctx_.tx). |
| src/libxrpl/tx/transactors/vault/VaultDeposit.cpp | Updates MPToken authorization/enforcement calls to use ApplyViewContext. |
| src/libxrpl/tx/transactors/vault/VaultDelete.cpp | Updates holding removal calls to use ApplyViewContext. |
| src/libxrpl/tx/transactors/vault/VaultCreate.cpp | Updates empty-holding creation and MPToken issuance/authorization to use ApplyViewContext. |
| src/libxrpl/tx/transactors/vault/VaultClawback.cpp | Updates holding removal to use ApplyViewContext. |
| src/libxrpl/tx/transactors/token/TrustSet.cpp | Uses getTxReserveSponsor(ctx_.getApplyViewContext()). |
| src/libxrpl/tx/transactors/token/MPTokenIssuanceCreate.cpp | Changes create() signature to accept ApplyViewContext& and updates sponsor lookup. |
| src/libxrpl/tx/transactors/token/MPTokenAuthorize.cpp | Updates authorizeMPToken call to use ApplyViewContext. |
| src/libxrpl/tx/transactors/Sponsor/SponsorshipSet.cpp | Updates sponsor lookup to use ApplyViewContext. |
| src/libxrpl/tx/transactors/payment/DepositPreauth.cpp | Updates sponsor lookup to use ApplyViewContext in reserve checks. |
| src/libxrpl/tx/transactors/payment_channel/PaymentChannelFund.cpp | Updates sponsor lookup to use ApplyViewContext. |
| src/libxrpl/tx/transactors/payment_channel/PaymentChannelCreate.cpp | Updates sponsor lookup to use ApplyViewContext. |
| src/libxrpl/tx/transactors/lending/LoanSet.cpp | Updates addEmptyHolding calls to use ApplyViewContext. |
| src/libxrpl/tx/transactors/lending/LoanPay.cpp | Updates addEmptyHolding calls to use ApplyViewContext. |
| src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp | Updates addEmptyHolding calls to use ApplyViewContext. |
| src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp | Updates removeEmptyHolding call to use ApplyViewContext. |
| src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp | Updates doWithdraw call to use ApplyViewContext. |
| src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp | Updates escrow unlock helper call to use ApplyViewContext. |
| src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp | Updates sponsor lookup to use ApplyViewContext. |
| src/libxrpl/tx/transactors/escrow/EscrowCancel.cpp | Updates escrow unlock helper call to use ApplyViewContext. |
| src/libxrpl/tx/transactors/delegate/DelegateSet.cpp | Updates sponsor lookup to use ApplyViewContext. |
| src/libxrpl/tx/transactors/check/CheckCreate.cpp | Updates sponsor lookup to use ApplyViewContext. |
| src/libxrpl/tx/transactors/check/CheckCash.cpp | Introduces a local ApplyViewContext for sponsor lookup. |
| src/libxrpl/tx/transactors/account/SignerListSet.cpp | Updates sponsor lookup to use ApplyViewContext. |
| src/libxrpl/tx/ApplyContext.cpp | Constructs/stores viewCtx_ and initializes view_ in the initializer list. |
| src/libxrpl/ledger/View.cpp | Changes doWithdraw to accept ApplyViewContext& and routes through ctx.view/ctx.tx. |
| src/libxrpl/ledger/helpers/TokenHelpers.cpp | Updates asset-dispatching helpers to accept ApplyViewContext&. |
| src/libxrpl/ledger/helpers/RippleStateHelpers.cpp | Updates IOU trustline holding helpers to accept ApplyViewContext&. |
| src/libxrpl/ledger/helpers/MPTokenHelpers.cpp | Updates MPToken holding/auth helpers to accept ApplyViewContext& and use ctx.tx. |
| include/xrpl/tx/transactors/token/MPTokenIssuanceCreate.h | Public header signature change: create(ApplyViewContext&, ...). |
| include/xrpl/tx/ApplyContext.h | Adds getApplyViewContext() and stores an ApplyViewContext member. |
| include/xrpl/ledger/View.h | Public header signature change: doWithdraw(ApplyViewContext&, ...). |
| include/xrpl/ledger/helpers/TokenHelpers.h | Public header signature changes for add/remove empty holdings. |
| include/xrpl/ledger/helpers/SponsorHelpers.h | Public header signature change: getTxReserveSponsor(ApplyViewContext&). |
| include/xrpl/ledger/helpers/RippleStateHelpers.h | Public header signature changes to accept ApplyViewContext&. |
| include/xrpl/ledger/helpers/MPTokenHelpers.h | Public header signature changes to accept ApplyViewContext&. |
| include/xrpl/ledger/helpers/EscrowHelpers.h | Public header template signature changes to accept ApplyViewContext&. |
| include/xrpl/ledger/ApplyView.h | Introduces the ApplyViewContext struct type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| auto const sponsorSle = getTxReserveSponsor(psb, ctx_.tx); | ||
| ApplyViewContext ctx(psb, ctx_.tx); | ||
| auto const sponsorSle = getTxReserveSponsor(ctx); |
There was a problem hiding this comment.
getTxReserveSponsor( ApplyViewContext (psb, ctx_.tx));
| // A temporary helper object that passes around ApplyContext info | ||
| // Only necessary (for now) because the ApplyContext can't be passed into helpers due to | ||
| // levelization | ||
| ApplyViewContext viewCtx_; |
There was a problem hiding this comment.
It is not needed
inline ApplyViewContext
getApplyViewContext()
{
XRPL_ASSERT(&viewCtx_.view == &view_, "Previous view discarded");
return ApplyViewContext(view_, tx_);
}
There was a problem hiding this comment.
@Tapanito wanted it to be passed by reference
There was a problem hiding this comment.
Additional object == synchronization burden. No one need it. You still can pass it as right reference (which is perfect for the object consists only from references)
There was a problem hiding this comment.
By synchronization do you mean from a development perspective? I wouldn't worry about that too much, in the next couple months at worst we're going to migrate ApplyViewContext to be ApplyContext (see the PR description).
| , flags_(flags) | ||
| , view_{std::in_place, &base_, flags_} | ||
| , parentBatchId_(parentBatchId) | ||
| , viewCtx_{*view_, tx} |
There was a problem hiding this comment.
Not needed, see comment above
|
Converting to Draft to avoid CI being run, this is still ready for review |
High Level Overview of Change
This PR adds a new tiny struct,
ApplyViewContext, that's just ApplyView+STTx, that can be passed around more easily than ApplyView+STTx separately as parameters. It should be used in place of all ApplyView parameters in helpers.Context of Change
Adding an extra
txparameter is a bit messyFuture Tasks
Long term, once #7404 is merged, we can switch this to just use
ApplyContextdirectly. But to avoid that being a blocker for Sponsor, we're doing this instead temporarily.