fix: Allow zero-value MPT vault withdraw when the asset holding is missing

[Tapanito]

Summary

• A fully impaired vault can pay nothing for a share burn. doWithdraw used to create an empty self-destination MPToken for that zero payout, so ValidVault saw a one-sided zero delta and ValidMPTIssuance could see create+delete in the same transaction.
• Post-fixCleanup3_4_0, skip addEmptyHolding when the payout is zero. ValidVault accepts a missing recipient delta when zeroDeltaIsLegitimate; a present destination delta of zero is still rejected.

Test plan

• xrpld -u VaultBugs (covers leftover and last-share arms, with and without the asset MPToken, plus pre-fixCleanup3_4_0 tecINVARIANT_FAILED).

[xrplf-ai-reviewer]

Fix looks correct and consistent with the description: doWithdraw now skips creating an empty self-destination MPToken/trust line when the payout is zero (post-fixCleanup3_4_0), and ValidVault's destination-delta check gains a zeroDeltaIsLegitimate backstop so a legitimate one-sided zero delta isn't flagged as an invariant violation. The added test exercises leftover-share and last-share redemption paths with and without a pre-existing asset MPToken, plus a pre-amendment control expecting tecINVARIANT_FAILED. No correctness, security, or resource-management issues found in the added lines.

[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 an edge case in vault withdrawals where a fully impaired vault can legitimately pay zero assets when burning shares, ensuring the ledger doesn’t create empty self-destination holdings (MPToken/trustline) and that the vault invariant tolerates a one-sided rounded-zero destination delta when that zero transfer is legitimate.

Changes:

• Update doWithdraw to skip addEmptyHolding for self-destination when the withdrawal payout is zero (post-fixCleanup3_4_0).
• Relax ValidVault withdrawal destination-delta validation to allow a rounded-zero delta when zeroDeltaIsLegitimate.
• Add a regression test covering zero-value MPT vault withdrawals with/without the destination asset MPToken present.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/test/app/vault/VaultBugs_test.cpp	Adds a regression test scenario for zero-value MPT vault withdrawals when the asset holding is missing.
src/libxrpl/tx/invariants/VaultInvariant.cpp	Permits one-sided rounded-zero destination delta for legitimate zero-value withdrawals.
src/libxrpl/ledger/View.cpp	Skips creating empty self-destination holdings when the credited amount is zero (post-amendment).

---

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

[xrplf-ai-reviewer]

Small, well-scoped fix: doWithdraw now only calls addEmptyHolding for a self-destination when the payout is non-zero (or pre-fixCleanup3_4_0, to preserve old behavior), and ValidVault::finalize now tolerates a one-sided zero destination delta when zeroDeltaIsLegitimate. The added test (testBugMptZeroWithdrawMissingHolding) exercises leftover/last-share arms with and without a pre-existing asset MPToken, and confirms the pre-amendment tecINVARIANT_FAILED regression. No clear bugs found in the added lines; one point worth confirming with the author.

[a1q123456]

Small cosmetic issue. All good otherwise.

[xrplf-ai-reviewer]

The core fix is narrow and consistent: doWithdraw now skips addEmptyHolding for a self-destination when the payout is zero and fixCleanup3_4_0 is enabled, and ValidVault tolerates the resulting one-sided zero destination delta via zeroDeltaIsLegitimate. Logic in the two added conditionals looks correct and matches the described bug (zero-payout MPT withdraw creating/destroying holdings in the same tx). The one open question is how broadly zeroDeltaIsLegitimate applies within the invariant check, since that flag isn't defined in the shown diff.

[xrplf-ai-reviewer]

Focused fix that skips inserting an empty self-destination MPToken holding on a zero-value vault withdraw (post-fixCleanup3_4_0) and loosens the ValidVault zero-delta check accordingly. The View.cpp gating logic is straightforward and matches the stated bug (avoids one-sided zero deltas and create+delete-in-same-tx MPToken issues). The one point worth double-checking is the scope of the new zeroDeltaIsLegitimate flag used in the invariant relaxation, since its computation isn't visible in this diff and over-scoping it could mask unrelated accounting bugs that the strict zero-delta check was designed to catch. Test coverage (leftover/last-share, with/without asset MPToken, pre/post-amendment) looks reasonable for the described bug.

[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 change is narrowly scoped and addresses a real bug: skipping addEmptyHolding for the self-destination case when the payout is zero (post-fixCleanup3_4_0), and relaxing the destination-delta invariant to accept a legitimate one-sided zero delta. The View.cpp guard correctly preserves pre-fix behavior when fixCleanup3_4_0 is not enabled (unconditional addEmptyHolding call) and only skips holding creation when amount is exactly zero post-fix, which matches the described fully-impaired-vault scenario. The invariant relaxation in VaultInvariant.cpp only weakens the check for integral assets from '<=0 invalid' to '<0 invalid' when either tolerateZeroDelta or the new zeroDeltaIsLegitimate flag holds, which is the intended fix for the one-sided zero-delta false positive; I don't have visibility into where zeroDeltaIsLegitimate is computed/set (likely elsewhere in this same function, not shown in this diff), so I can't independently verify it is scoped tightly enough to avoid masking unrelated zero-delta bugs, but nothing in the shown code is clearly wrong. The added test exercises the leftover-share and last-share arms, with and without the asset MPToken, and the pre-fix tecINVARIANT_FAILED regression case, which is good coverage for this fix. No clear correctness, security, or resource issues found in the changed lines.

[xrplf-ai-reviewer]

Straightforward, well-scoped fix: doWithdraw now skips addEmptyHolding for a self-destination when the payout is zero (guarded by fixCleanup3_4_0), and ValidVault treats a one-sided zero destination delta as valid when zeroDeltaIsLegitimate. The change matches the stated intent, preserves pre-amendment behavior (still calls addEmptyHolding when the amendment is disabled), and is covered by new targeted tests exercising leftover/last-share and with/without-asset-MPToken combinations, including a pre-fixCleanup3_4_0 tecINVARIANT_FAILED check. No correctness, security, or resource issues found in the added lines; the zeroDeltaIsLegitimate flag referenced in VaultInvariant.cpp isn't shown being defined in this diff hunk, so its scope/semantics couldn't be independently verified here, but the logic as wired is consistent with the description.

[xrplf-ai-reviewer]

This is a small, well-targeted fix. In View.cpp, addEmptyHolding is now skipped for a self-destination when the payout is exactly zero and fixCleanup3_4_0 is enabled, preserving old behavior otherwise via the !enabled(fixCleanup3_4_0) fallback. In VaultInvariant.cpp, the zero-destination-delta check now also tolerates a legitimate zero delta via the new zeroDeltaIsLegitimate disjunct, matching the stated intent of not flagging a one-sided zero-value withdrawal as an invariant violation. Both production changes are minimal, logically consistent with the PR description, and are exercised by the new VaultBugs_test.cpp scenarios (leftover/last-share, with/without asset MPToken, pre/post-fixCleanup3_4_0). I did not find correctness, security, or resource-management issues in the added lines; the test file additions are out of scope per test-code review rules (style/coverage only) and show no leaked resources or lifecycle issues.

[Copilot]

🟡 Changes recommended

The new MPT regression test matrix is missing one pre-fixCleanup3_4_0 scenario arm, leaving the stated coverage incomplete.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

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

[xrplf-ai-reviewer]

The core fix in View.cpp correctly gates the addEmptyHolding call: post-fixCleanup3_4_0, it's only invoked when there's an actual positive payout (amount > beast::kZero), while pre-amendment behavior is preserved unconditionally via the !enabled(fixCleanup3_4_0) fallback. This matches the described intent of avoiding a one-sided zero delta / create+delete MPToken pair for zero-value withdrawals from fully impaired vaults. The VaultInvariant.cpp change is comment-only (no logic change). The added test coverage in VaultBugs_test.cpp is extensive, covering leftover/last-share arms with and without the asset MPToken, IOU trust-line and XRP sponsored-fee analogues, and both pre- and post-amendment TER expectations. I did not find correctness, security, or consistency issues in the added lines.

[Copilot]

🟢 Approval recommended

The behavior change is narrowly scoped, correctly gated on fixCleanup3_4_0, and is backed by targeted regression tests for the affected zero-payout scenarios.

Review details

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

[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]

Focused, well-tested fix: doWithdraw now skips addEmptyHolding for a self-destination when the payout is zero and fixCleanup3_4_0 is enabled, preserving old (buggy) behavior pre-amendment. The gating logic (amount > beast::kZero || !enabled(fixCleanup3_4_0)) correctly preserves backward compatibility while fixing the zero-payout create+delete/one-sided-delta issue described in the MR. The VaultInvariant.cpp change is comment-only (no logic change). The added VaultBugs_test.cpp coverage is extensive and exercises the leftover/last-share and present/missing-holding permutations across both amendment states, matching the described test plan. I did not find correctness, security, or resource-management issues in the changed lines.

[xrplf-ai-reviewer]

The core fix in View.cpp (skip addEmptyHolding for a self-destination zero-value withdraw once fixCleanup3_4_0 is enabled) matches the described bug and looks correct — it's scoped to the self-destination branch only and preserves pre-amendment behavior via the !rules().enabled(fixCleanup3_4_0) fallback. The VaultInvariant.cpp change is comment-only. The large VaultBugs_test.cpp addition covers the MPT/IOU/XRP zero-payout arms with and without pre-existing holdings, and pre/post amendment behavior, consistent with the description. No correctness, security, or resource-leak issues found in the added lines.

[Copilot]

🔵 Needs a closer look

It changes consensus-critical withdrawal ledger behavior and invariant-adjacent handling, which warrants final human review despite the added regression coverage.

Review details

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

[gregtatcam]

LGTM