Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
Production change extends LoanBrokerDelete::preclaim to run the same transfer/authorization checks as LoanBrokerCoverWithdraw under fixCleanup3_4_0 before releasing cover; the added canTransfer/requireAuth/checkDeepFrozen/checkFrozen ordering looks consistent with the test expectations (terNO_RIPPLE for canTransfer failures, tecNO_AUTH for requireAuth failures) and the waive-CanTransfer logic correctly mirrors the fixCleanup3_2_0 self-withdrawal exemption. The new holdingExists/canAddHolding branch (taken when the broker owner has no existing holding for the asset) is not exercised by any of the added tests, which otherwise cover authorized/unauthorized MPT owners, IOU NoRipple, both amendment states, and zero-cover deletion.
There was a problem hiding this comment.
🔵 Needs a closer look
It changes consensus-critical transaction preclaim behavior for asset transfer/authorization enforcement and should receive a final human review despite good targeted test coverage.
Pull request overview
This PR updates the LoanBrokerDelete transactor to enforce the same asset transfer restriction and authorization checks that already apply to LoanBrokerCoverWithdraw, but only when fixCleanup3_4_0 is enabled. This closes a gap where deletion could previously pay out nonzero broker cover via accountSend without the newer transferability/authorization gating.
Changes:
- Added post-
fixCleanup3_4_0preclaim checks inLoanBrokerDeleteforcanTransfer,requireAuth, and (when needed)canAddHoldingviability before allowing cover payout. - Expanded lending unit tests to cover IOU NoRipple blocking behavior and MPT owner authorization revoked/allowed scenarios for
LoanBrokerDelete.
File summaries
| File | Description |
|---|---|
src/test/app/lending/LoanBroker_test.cpp |
Adds regression coverage for deletion payout restrictions: IOU NoRipple enforcement post-fix and MPT owner-authorization failure/success paths. |
src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp |
Aligns delete-with-cover behavior (post-fixCleanup3_4_0) with cover-withdraw transfer/authorization/holding checks while preserving pre-amendment behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The cover payout has to recreate a deleted holding, so canAddHolding decides whether deletion can proceed. Post-fixCleanup3_4_0 an issuer that cleared DefaultRipple blocks it with terNO_RIPPLE, while deletion still succeeds when the line can be recreated.
There was a problem hiding this comment.
The core fix in LoanBrokerDelete.cpp consolidates the freeze/frozen checks under the existing coverAvailable > 0 guard and adds a new fixCleanup3_4_0-gated block (canTransfer, requireAuth, holding-existence/canAddHolding) before the pre-existing deep-freeze and pseudo-account-freeze checks. The restructuring correctly preserves pre-amendment behavior (only freeze checks run) while adding the self-withdrawal-style authorization/transfer checks post-amendment, and it removes the now-redundant duplicate sfAccount lookup. This matches the accompanying test additions (NoRipple IOU line, deleted trustline recreation via canAddHolding, and MPT auth-revoked owner scenarios with clawback-then-delete), which cover both amendment states and the described behaviors. I did not find any lines added in this diff that introduce a clear correctness or security bug within the visible context; the logic is consistent with the stated intent and the referenced helper functions (canTransfer, requireAuth, holdingExists, canAddHolding) are presumably already vetted elsewhere (e.g. LoanBrokerCoverWithdraw) for this same self-withdrawal policy.
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core transaction preclaim rules for asset movement on broker deletion (consensus-sensitive behavior) and should receive final human review despite strong test coverage.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
The core fix in LoanBrokerDelete.cpp cleanly folds the new cover-payout authorization/transfer checks (canTransfer, requireAuth, holdingExists/canAddHolding) into the existing coverAvailable-gated block, gated by featureLendingProtocolV1_2, and mirrors the self-withdrawal policy used by LoanBrokerCoverWithdraw. Test coverage for authorized/unauthorized MPT owners, IOU NoRipple, and both amendment states looks thorough and internally consistent. One notable discrepancy: the PR description attributes the new self-withdrawal enforcement to fixCleanup3_4_0, but the actual code (and the test comments) gate this behavior on featureLendingProtocolV1_2 — worth reconciling so reviewers assess risk against the correct amendment.
There was a problem hiding this comment.
🟡 Changes recommended
Fix missing MPT holding creation and preserve pre-Cleanup3.4 behavior through amendment gating.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp:117
canAddHoldingis an issuer-level create-path check, so this must be gated byfixCleanup3_4_0just likeLoanBrokerCoverWithdrawandVaultWithdraw. As written, a post-featureLendingProtocolV1_2deletion before Cleanup3.4 is rejected whenever the owner has no holding (for example, an MPT whoselsfMPTCanTransferis cleared), even though the pre-fixaccountSendpath is allowed to create the holding; this changes the advertised pre-fix behavior.
if (!holdingExists(ctx.view, brokerOwner, asset))
{
if (auto const ret = canAddHolding(ctx.view, asset); !isTesSuccess(ret))
return ret;
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| if (!holdingExists(ctx.view, brokerOwner, asset)) | ||
| { | ||
| if (auto const ret = canAddHolding(ctx.view, asset); !isTesSuccess(ret)) | ||
| return ret; | ||
| } |
| // Pre-featureLendingProtocolV1_2: only freeze checks apply to the cover payout. | ||
| // Post-featureLendingProtocolV1_2: apply the cover-withdraw transfer and authorization | ||
| // checks too. | ||
| if (ctx.view.rules().enabled(featureLendingProtocolV1_2)) |
There was a problem hiding this comment.
Since this is a fix, let's place it under fixCleanup3_5_0
| // checks too. | ||
| if (ctx.view.rules().enabled(featureLendingProtocolV1_2)) | ||
| { | ||
| auto const waive = ctx.view.rules().enabled(fixCleanup3_2_0) ? WaiveMPTCanTransfer::Yes |
There was a problem hiding this comment.
This is a big no no. When an amendment is released, new changes cannot be gated under it.
That said, I'm not sure this wave is needed in the first place. Think of it this way: after 3_2_0 we will wave the transfer check. Since this code is adding new logic, the 3_2_0 gating is unnecessary, as this code didn't exist when 3_2_0 was released.
| } | ||
|
|
||
| void | ||
| testLoanBrokerDeleteDeletedLineIOU( |
There was a problem hiding this comment.
Don't pass clearDefaultRipple in from run(). The policy belongs in the test.
Call sites like testLoanBrokerDeleteDeletedLineIOU(all_, true, terNO_RIPPLE) are unreadable. Either loop DefaultRipple on/off inside this function, or split into two named tests.
|
|
||
| void | ||
| testLoanBrokerDeleteRequireAuthMPT(FeatureBitset features) | ||
| testLoanBrokerDeleteNoRippleIOU(FeatureBitset features) |
There was a problem hiding this comment.
These two new IOU tests duplicate the fund / vault / broker / cover setup and only differ in the restriction applied before del. Pull the shared setup into a small helper, then keep two named tests (NoRipple vs deleted dest line). Don't merge them into one bool-parameterized function.
| } | ||
|
|
||
| void | ||
| testLoanBrokerDeleteRequireAuthMPT(FeatureBitset features, bool ownerAuthorized) |
There was a problem hiding this comment.
This used to be one claim: authorized owner, Cleanup 3.4 vs not. The ownerAuthorized flag folds in two more: unauthorized dest (tecNO_AUTH / invariant) and clawback-to-zero then delete. Those are separate tests.
Restore this function to authorized-owner only (and drop tfMPTCanClawback unless this test clawbacks). Add testLoanBrokerDeleteUnauthorizedOwnerMPT that always unauthorizes and computes TER from the amendments. Put the zero-cover success path in its own test so it is not an early return here.
| testLoanBrokerDeleteNoRippleIOU(all_); | ||
| testLoanBrokerDeleteNoRippleIOU(all_ - featureLendingProtocolV1_2); | ||
|
|
||
| testLoanBrokerDeleteDeletedLineIOU(all_, true, terNO_RIPPLE); |
There was a problem hiding this comment.
Same pattern as the helpers above: positional true / false / TER does not say what is being varied. run() should look like FrozenIOU — feature sets only.
Also, unauthorized owner with neither V1.2 nor Cleanup 3.4 is never run. If pre-amendment delete is supposed to succeed, that cell should be explicit.
High Level Overview of Change
LoanBrokerDelete now checks asset transfer restrictions and owner authorization before returning nonzero broker cover.
Context of Change
LoanBrokerDelete previously checked freeze state but returned cover through accountSend without the checks used by LoanBrokerCoverWithdraw. Under fixCleanup3_4_0, deletion now follows the same self-withdrawal policy. Pre-amendment and zero-cover behavior remain unchanged.
API Impact
libxrplchangeTest Plan
LoanBroker tests cover authorized and revoked MPT owners, IOU NoRipple restrictions, both amendment states, and deletion after cover reaches zero.