fix: Treat an existing IOU line as a no-op in addEmptyHolding

[Tapanito]

Bug

Issue

addEmptyHolding() in RippleStateHelpers.cpp evaluates issuer state (isGlobalFrozen and !lsfDefaultRipple) before checking whether the holding line already exists (tecDUPLICATE).

Root Cause

Issuer preconditions returning tecINTERNAL or tecFROZEN run prior to the ctx.view.read(index) existing-line short circuit. When a holding line already exists, no line creation is required, so these checks should not run or fail the call.

Impact

An IOU issuer clearing asfDefaultRipple causes all self-destination VaultWithdraw and LoanBrokerCoverWithdraw transactions for existing trust lines to fail with tecINTERNAL, locking depositor funds and burning fees on retries. Similarly, globally frozen assets evaluate tecFROZEN prematurely before checking line existence.

Summary

• addEmptyHolding for IOUs checked issuer freeze and DefaultRipple before noticing the trust line already existed. doWithdraw (VaultWithdraw and LoanBrokerCoverWithdraw) only treats tecDUPLICATE as success, so an issuer clearing asfDefaultRipple made self-destination payouts fail with tecINTERNAL even when the destination already held the asset.
• Gated on fixCleanup3_4_0: existing line returns tecDUPLICATE first; creating a new line with DefaultRipple off returns terNO_RIPPLE instead of tecINTERNAL.
• VaultWithdraw and LoanBrokerCoverWithdraw call canAddHolding in preclaim only when the destination is self and holdingExists is false.

Test plan

• VaultBugs self-withdraw with an existing line after DefaultRipple is cleared (pre tecINTERNAL, post tesSUCCESS)
• Deleted-line create path (pre tecINTERNAL, post terNO_RIPPLE)
• LoanBrokerCoverWithdraw to self with an existing line
• Private vault: self-withdraw vs non-member destination (tecNO_AUTH post-3.4.0)

[xrplf-ai-reviewer]

This is a well-scoped, feature-flag-gated fix. The core change in RippleStateHelpers.cpp correctly moves the existing-line check ahead of the freeze/DefaultRipple checks only when fixCleanup3_4_0 is enabled, while preserving the old check order (and old tecINTERNAL/tecDUPLICATE semantics) when the flag is off — I traced both branches and didn't find a regression in pre-fix behavior. The new holdingExists() helpers and the preclaim guards added to VaultWithdraw and LoanBrokerCoverWithdraw are consistently applied (self-destination + !holdingExists before calling canAddHolding), matching the described intent of letting an existing line short-circuit as a no-op rather than being blocked by canAddHolding. Test coverage in VaultBugs_test.cpp exercises the existing-line, deleted-line, cover-withdraw, and private-vault scenarios for both pre- and post-fix behavior. I didn't find correctness, security, or resource-management issues in the changed lines worth flagging.

[codecov]

Codecov Report

❌ Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...tx/transactors/lending/LoanBrokerCoverWithdraw.cpp	50.0%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[Copilot]

Pull request overview

This PR fixes a vault/lending withdrawal regression where addEmptyHolding() could fail (e.g., tecINTERNAL / tecFROZEN) before noticing that the destination already had the required IOU trust line, causing self-destination payouts to fail unnecessarily. The fix is gated behind fixCleanup3_4_0 and is accompanied by targeted regression tests.

Changes:

• Update IOU addEmptyHolding to short-circuit on an existing trust line before evaluating issuer freeze / lsfDefaultRipple (post-fixCleanup3_4_0), and map missing DefaultRipple on create-path to terNO_RIPPLE.
• Add holdingExists(ReadView, AccountID, Asset) and use it to conditionally run canAddHolding in VaultWithdraw and LoanBrokerCoverWithdraw preclaim only when a self-destination would need to create a holding.
• Add regression coverage for self-withdraw scenarios (including deleted-line create-path and private vault behavior).

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/test/app/vault/VaultBugs_test.cpp	Adds regression tests covering self-withdraw after issuer clears asfDefaultRipple, including deleted-line and private vault cases.
src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp	Adds a fixCleanup3_4_0-gated preclaim check to call canAddHolding only when a self-destination holding would need to be created.
src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp	Mirrors the vault preclaim behavior for loan broker cover withdraw when destination is self.
src/libxrpl/ledger/helpers/TokenHelpers.cpp	Introduces holdingExists helper for Asset/IOU/MPT to detect whether a holding object already exists.
src/libxrpl/ledger/helpers/RippleStateHelpers.cpp	Reorders IOU addEmptyHolding checks (post-fixCleanup3_4_0) to treat an existing trust line as a no-op before issuer preconditions; maps create-path DefaultRipple miss to terNO_RIPPLE.
src/libxrpl/ledger/helpers/MPTokenHelpers.cpp	Adds clarifying comment noting MPT behavior still checks locked issuance before “already exists” short-circuit.
include/xrpl/ledger/helpers/TokenHelpers.h	Exposes holdingExists in the public helper header.
include/xrpl/ledger/helpers/RippleStateHelpers.h	Updates documentation to reflect the post-fixCleanup3_4_0 no-op behavior and correct preclaim guidance.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[xrplf-ai-reviewer]

The core fix in RippleStateHelpers.cpp reorders the checks in addEmptyHolding so that an existing trust line short-circuits to tecDUPLICATE before the issuer freeze/DefaultRipple checks run (gated behind fixCleanup3_4_0), which matches the described bug and is preserved correctly for the pre-fix path. The new holdingExists() helpers and the preclaim additions in VaultWithdraw.cpp/LoanBrokerCoverWithdraw.cpp correctly gate the canAddHolding() preclaim call to self-destination cases where the holding doesn't yet exist, consistent with the MR's stated design. The accompanying VaultBugs_test.cpp coverage exercises both pre- and post-amendment behavior for the existing-line, deleted-line, LoanBrokerCoverWithdraw, and private-vault scenarios, including the terNO_RIPPLE vs tecINTERNAL code-path change, which gives good confidence the new control flow behaves as intended. I did not find a confident correctness, security, or resource-management issue in the changed lines of this diff.

[github-actions]

This PR has conflicts, please resolve them in order for the PR to be reviewed.

[github-actions]

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

[xrplf-ai-reviewer]

The core fix (checking for an existing trust line before evaluating issuer freeze/DefaultRipple in addEmptyHolding, gated on fixCleanup3_4_0) is sound and well-tested. However, the new create-path return value terNO_RIPPLE is a 'ter'-class code, and this function executes during doApply (per the header doc and the VaultWithdraw/LoanBrokerCoverWithdraw call sites) — XRPL's Transactor convention only permits tesSUCCESS or tec-class results to flow out of doApply, so this return is likely to be sanitized into tefINTERNAL (or trip an internal assertion) rather than actually surface as terNO_RIPPLE, undermining the intended behavior tested in runDeletedLine.

[xrplf-ai-reviewer]

This is a well-scoped, carefully gated fix. The reordering in addEmptyHolding correctly checks for an existing trust line before evaluating issuer freeze/DefaultRipple state when fixCleanup3_4_0 is enabled, and falls back to the exact prior ordering/return codes when the amendment is disabled — I traced both branches and they match the documented pre/post behavior. The new holdingExists helpers and their use in VaultWithdraw/LoanBrokerCoverWithdraw preclaim (only invoking canAddHolding for self-destination when no holding exists yet) are consistent with the stated intent and the added VaultBugs_test.cpp coverage exercises the existing-line, deleted-line, cover-withdraw, and private-vault scenarios described in the test plan. No correctness, security, or resource-management issues found in the added lines.

[xrplf-ai-reviewer]

This is a well-scoped, correctly-gated bug fix. The core change in RippleStateHelpers.cpp reorders addEmptyHolding's checks so a pre-existing trust line short-circuits to tecDUPLICATE before issuer freeze/DefaultRipple checks run, exactly matching the described bug (tecINTERNAL blocking self-withdraws after asfDefaultRipple is cleared). The behavior is properly gated behind fixCleanup3_4_0, preserving pre-amendment behavior (old tecINTERNAL/tecFROZEN ordering) while introducing terNO_RIPPLE for the genuine create-path DefaultRipple failure post-amendment. The new holdingExists() helpers mirror the existing keylet lookups used by addEmptyHolding (trustLine for IOU, mptoken for MPT), and the new preclaim short-circuits in VaultWithdraw and LoanBrokerCoverWithdraw are correctly restricted to the self-destination + no-existing-holding case, avoiding blocking the no-op path that the fix is meant to unblock. The MPTokenHelpers.cpp change is comment-only, documenting an intentional divergence from the IOU fix. Test coverage in VaultBugs_test.cpp exercises the pre/post-amendment behavior for existing lines, deleted lines, LoanBrokerCoverWithdraw, and private vaults with third-party destinations. I did not find any lines added in this diff that introduce a clear bug, security issue, or correctness regression.