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

✅ All modified and coverable lines are covered by tests.

📢 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.

[xrplf-ai-reviewer]

This MR reorders addEmptyHolding() for IOUs so an existing trust line short-circuits to tecDUPLICATE before issuer freeze/DefaultRipple checks (gated on fixCleanup3_4_0), fixing tecINTERNAL failures on self-destination vault/loan-broker withdrawals after an issuer clears asfDefaultRipple. Pre-fix behavior is preserved exactly (same check order, same error codes) when the amendment is disabled, and new terNO_RIPPLE handling is added for the create-a-new-line path post-amendment. The new holdingExists() helpers and the preclaim early-outs in VaultWithdraw/LoanBrokerCoverWithdraw are logically consistent with the stated design (freeze on an existing line is deferred to checkWithdrawFreeze), and the added VaultBugs_test.cpp coverage exercises the existing-line, deleted-line, cover-withdraw, and private-vault scenarios for both pre- and post-amendment behavior. I traced the control flow carefully and did not find a correctness or security defect introduced by this diff.

[gregtatcam]

LGTM

[Copilot]

🔵 Needs a closer look

It changes ledger/transaction semantics in consensus-critical helpers and transactor preclaim paths, so it warrants final human verification despite strong test coverage.

Review details

Suppressed comments (1)

src/libxrpl/ledger/helpers/RippleStateHelpers.cpp:676

• Same as the earlier block: the duplicate-check only needs key existence; using exists() avoids loading the trust line SLE.

    // If the line already exists, don't create it again.
    if (!fix340Enabled && ctx.view.read(index))
        return tecDUPLICATE;

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Lite

[xrplf-ai-reviewer]

This is a well-scoped, well-tested bug fix. The core change in addEmptyHolding (RippleStateHelpers.cpp) correctly reorders the existing-line check ahead of the issuer freeze/DefaultRipple checks when fixCleanup3_4_0 is enabled, while preserving the old (buggy but amendment-gated) ordering when the amendment is disabled — I traced both branches against the pre/post diff and the logic holds together (freeze check still runs first pre-amendment, the final duplicate-check if (!fix340Enabled && ...) correctly becomes a no-op post-amendment since the early return already handled it). The new holdingExists overloads (Issue/MPTIssue/Asset) are straightforward existence checks and are used consistently in VaultWithdraw.cpp and LoanBrokerCoverWithdraw.cpp to gate the new preclaim-time canAddHolding self-destination check, matching the PR description precisely. The terNO_RIPPLE vs tecINTERNAL swap on the create-path DefaultRipple miss is intentional and covered by the runDeletedLine test. No SQL/command injection, resource leak, or race-condition concerns apply to this C++ ledger-processing code. I did not find any high-confidence correctness or security issues in the added (+) lines; the change appears internally consistent with its own test suite (VaultBugs_test.cpp) and the documented amendment-gating convention used elsewhere in the file (fixCleanup3_2_0/3_3_0).

[Copilot]

🔵 Needs a closer look

The change affects consensus-critical transaction/ledger helper semantics (TER mapping and preclaim/doApply interactions) and should receive final human validation despite the added test coverage.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

[xrplf-ai-reviewer]

The core fix in RippleStateHelpers.cpp reorders the existing-line check ahead of the issuer freeze/DefaultRipple checks in a way that's consistent with the fixCleanup3_4_0 gating (legacy behavior preserved when the amendment is disabled), and the supporting holdingExists()/preclaim gating in VaultWithdraw, LoanBrokerCoverWithdraw, and LoanSet look correctly scoped to only run canAddHolding when a new holding would actually be created. Test coverage is thorough (pre/post amendment, deleted-line, private vault, cover-withdraw paths). One likely build-breaking issue found in LoanSet.cpp: it compares a Number against beast::kZero, but the codebase's zero-comparison sentinel is beast::zero (lowercase, no k prefix) — this would fail to compile.

[Copilot]

🟢 Approval recommended

The behavior change is narrowly scoped behind fixCleanup3_4_0, updates dependent preclaim logic accordingly, and adds targeted regression tests for the reported failure modes.

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

[xrplf-ai-reviewer]

Undefined namespace constant - see inline.

[xrplf-ai-reviewer]

The fix reorders addEmptyHolding() so an existing IOU trust line short-circuits to tecDUPLICATE before the issuer freeze/DefaultRipple checks run, gated behind fixCleanup3_4_0, and updates VaultWithdraw/LoanBrokerCoverWithdraw/LoanSet to only invoke canAddHolding in preclaim when a holding doesn't already exist. I traced the reordered logic in RippleStateHelpers.cpp: post-amendment the existence check runs first (returns tecDUPLICATE), freeze/DefaultRipple checks only apply on the create path (returning tecFROZEN / terNO_RIPPLE respectively), and pre-amendment behavior is preserved bit-for-bit via the fix340Enabled gate. The new holdingExists() helpers and their usage in VaultWithdraw/LoanBrokerCoverWithdraw/LoanSet look consistent with the documented intent, and even where the LoanSet gating condition is skipped, doApply's own addEmptyHolding still degrades to a non-fee-burning terNO_RIPPLE rather than tecINTERNAL post-amendment, so no fee-loss regression. No clear correctness, security, or resource-leak issues were found in the changed lines.

[Copilot]

🔵 Needs a closer look

It changes consensus-critical transaction result semantics (gated by an amendment) and should receive final human review despite strong regression coverage.

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Lite