🎯 Practical: fix: Check trustline limits for share-denominated vault withdrawals by Tapanito · Pull Request #6645 · XRPLF/rippled · GitHub

Tapanito · 2026-03-25T12:18:24Z

VaultWithdraw preclaim skipped the canWithdraw trustline limit check when the withdrawal amount was specified in shares (MPT) rather than assets. Convert shares to the equivalent asset amount before calling canWithdraw so that the destination's trustline limit is enforced regardless of denomination.

Gated behind fixAssortedFixes for ledger replay compatibility.

High Level Overview of Change

Context of Change

API Impact

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

VaultWithdraw preclaim skipped the canWithdraw trustline limit check when the withdrawal amount was specified in shares (MPT) rather than assets. Convert shares to the equivalent asset amount before calling canWithdraw so that the destination's trustline limit is enforced regardless of denomination. Gated behind fixAssortedFixes for ledger replay compatibility.

Move sleIssuance ledger read inside the fixAssortedFixes branch where it is actually needed, avoiding an unnecessary read on the pre-amendment and asset-denominated paths. Reuse existing account and dstAcct variables instead of recomputing from and to. Add pre-amendment test to verify the old behavior (share-denominated withdrawal bypasses trustline limit check) alongside the post-fix behavior. Fix stale comments in the test.

github-actions · 2026-03-26T00:44:35Z

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

github-actions · 2026-03-26T09:42:11Z

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

Tapanito · 2026-03-26T09:48:30Z

/ai-review

xrplf-ai-reviewer

Rounding safety concern and two code-quality issues flagged inline — see comments at lines 70, 76, and 90.

Review by ReviewBot 🤖

Review by Claude Opus 4.6 · Prompt: V12

codecov · 2026-03-26T10:16:16Z

Codecov Report

❌ Patch coverage is 60.00000% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.4%. Comparing base (bee1056) to head (3545eee).
⚠️ Report is 26 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp	60.0%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #6645     +/-   ##
=========================================
- Coverage     81.5%   81.4%   -0.0%     
=========================================
  Files          999     999             
  Lines        74467   74483     +16     
  Branches      7554    7560      +6     
=========================================
+ Hits         60660   60663      +3     
- Misses       13807   13820     +13

Files with missing lines	Coverage Δ
src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp	`92.4% <60.0%> (-6.6%)`	⬇️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

xrplf-ai-reviewer

Gave this a review

Two issues flagged inline: a rounding-direction concern in the share→asset conversion that could allow boundary bypass, and a tec/tef inconsistency on the unreachable error path.

Review by ReviewBot 🤖

Review by Claude Opus 4.6 · Prompt: V12

Copilot

Pull request overview

Fixes a VaultWithdraw preclaim gap where withdrawals specified in vault shares (MPT) could bypass the destination IOU trustline limit check by converting share-denominated amounts into the underlying asset amount before applying canWithdraw (gated by an amendment for replay compatibility).

Changes:

Update VaultWithdraw::preclaim to convert share-denominated withdrawals into asset-denominated amounts (when the fix amendment is enabled) before calling canWithdraw.
Add a regression test reproducing the trustline-limit bypass when withdrawing in shares vs assets.

Reviewed changes

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

File	Description
`src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp`	Converts share-denominated withdrawal amounts to asset amounts (behind `fixSecurity3_1_3`) so trustline limits are enforced consistently.
`src/test/app/Vault_test.cpp`	Adds a unit test demonstrating the pre-fix bypass and asserting rejection post-fix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…-withdraw-shares

xrplf-ai-reviewer

One inconsistency flagged inline — tecINTERNAL vs tefINTERNAL for analogous unreachable error paths.

Review by Claude Opus 4.6 · Prompt: V12

xrplf-ai-reviewer

Had a read through

Trust line limit bypass for share-denominated vault withdrawals flagged inline — the fix is correctly gated behind the amendment, preserving pre-amendment ledger replay behaviour.

Review by ReviewBot 🤖

Review by Claude Opus 4.6 · Prompt: V12

pratikmankawde · 2026-03-31T15:21:41Z

+                    ctx.tx.isFieldPresent(sfDestinationTag)))
+                return ret;
+        }
+        catch (std::overflow_error const&)


Theoretically the try block can throw more error types, like std::runtime_error and STObject::FieldErr

Yes, that's right, but here we specifically want to handle only overflow exceptions. Others exceptions are handled up the call-stack.

pratikmankawde

LGTM

…6645)

…RPLF#6645)

Tapanito added 2 commits March 25, 2026 12:31

Tapanito added Amendment AI Triage Bugs and fixes that have been triaged via AI initiatives labels Mar 25, 2026

github-actions Bot added the PR: has conflicts label Mar 26, 2026

Merge origin/develop; replace fixAssortedFixes with fixSecurity3_1_3

cdecd5d

github-actions Bot removed the PR: has conflicts label Mar 26, 2026

Tapanito requested review from pratikmankawde and shawnxie999 March 26, 2026 09:48

Tapanito marked this pull request as ready for review March 26, 2026 09:48

xrplf-ai-reviewer Bot reviewed Mar 26, 2026

View reviewed changes

Comment thread src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp

Comment thread src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp Outdated

Comment thread src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp Outdated

kuznetsss requested a review from Copilot March 27, 2026 14:01

Copilot started reviewing on behalf of kuznetsss March 27, 2026 14:02 View session

xrplf-ai-reviewer Bot reviewed Mar 27, 2026

View reviewed changes

Comment thread src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp

Comment thread src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp Outdated

Copilot AI reviewed Mar 27, 2026

View reviewed changes

Comment thread src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp Outdated

Comment thread src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp

Merge remote-tracking branch 'origin/develop' into tapanito/fix-vault…

6594b5d

…-withdraw-shares

xrplf-ai-reviewer Bot reviewed Mar 31, 2026

View reviewed changes

Comment thread src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp Outdated

Tapanito added 2 commits March 31, 2026 11:52

address review comments

73b0e6c

fix: Catch overflow in preclaim for share-denominated withdrawals

a2cc0ae

xrplf-ai-reviewer Bot reviewed Mar 31, 2026

View reviewed changes

Comment thread src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp

shawnxie999 approved these changes Mar 31, 2026

View reviewed changes

pratikmankawde reviewed Mar 31, 2026

View reviewed changes

pratikmankawde approved these changes Mar 31, 2026

View reviewed changes

Tapanito added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Mar 31, 2026

Tapanito added 2 commits April 1, 2026 10:10

Merge branch 'develop' into tapanito/fix-vault-withdraw-shares

3f204fb

fix: clang-tidy

cf197be

Merge branch 'develop' into tapanito/fix-vault-withdraw-shares

3545eee

ximinez added this pull request to the merge queue Apr 1, 2026

Merged via the queue into develop with commit 02fa55d Apr 1, 2026
2 of 3 checks passed

ximinez deleted the tapanito/fix-vault-withdraw-shares branch April 1, 2026 19:54

bthomee pushed a commit that referenced this pull request Apr 8, 2026

fix: Check trustline limits for share-denominated vault withdrawals (#…

a8f692d

…6645)

bthomee mentioned this pull request Apr 8, 2026

release: Add various fixes - part 2 #6815

Merged

ximinez pushed a commit that referenced this pull request Apr 14, 2026

fix: Check trustline limits for share-denominated vault withdrawals (#…

72f3903

…6645)

ximinez added this to the 3.1.3 milestone Apr 15, 2026

ximinez added Needs 2 PRs (staging / develop) and removed Needs 2 PRs (staging / develop) labels Apr 15, 2026

ximinez modified the milestones: 3.1.3, 3.1.3 (develop) Apr 15, 2026

ximinez pushed a commit that referenced this pull request Apr 22, 2026

fix: Check trustline limits for share-denominated vault withdrawals (#…

a59a613

…6645)

marek-foss-neti pushed a commit to marek-foss-neti/rippled that referenced this pull request May 5, 2026

fix: Check trustline limits for share-denominated vault withdrawals (X…

28c5336

…RPLF#6645)

mvadari modified the milestones: 3.1.3 (develop), 3.2.0 May 20, 2026

beartec-jpg pushed a commit to beartec-jpg/FalconLedger that referenced this pull request Jun 1, 2026

fix: Check trustline limits for share-denominated vault withdrawals (X…

fef3754

…RPLF#6645)

LeJamon mentioned this pull request Jul 7, 2026

Rippled 3.1.3 parity — tracking issue (fixCleanup3_1_3 + fix wave) LeJamon/go-xrpl#1248

Closed

32 tasks

Uh oh!

Conversation

Tapanito commented Mar 25, 2026

High Level Overview of Change

Context of Change

API Impact

Uh oh!

github-actions Bot commented Mar 26, 2026

Uh oh!

github-actions Bot commented Mar 26, 2026

Uh oh!

Tapanito commented Mar 26, 2026

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pratikmankawde Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Tapanito Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

pratikmankawde left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov Bot commented Mar 26, 2026 •

edited

Loading