spec(XLS-65): Update VaultWithdraw failure conditions and state changes - #553
Open
Tapanito wants to merge 36 commits into
Open
spec(XLS-65): Update VaultWithdraw failure conditions and state changes#553Tapanito wants to merge 36 commits into
Tapanito wants to merge 36 commits into
Conversation
…tion - Add Example JSON sections for Vault ledger entry and all transactions (VaultCreate, VaultSet, VaultDelete, VaultDeposit, VaultWithdraw, VaultClawback, Payment) with real transaction data - Add invariants for the Vault ledger entry (universal checks) and all transaction types derived from the ValidVault invariant checker - Restructure section 10 from "API" to "RPC: vault_info" matching the amendment template format with Request Fields, Response Fields, Failure Conditions, Example Request, and Example Response subsections - Update response fields table with missing fields (Data, Asset.mpt_issuance_id, shares.DomainID, shares.MPTokenMetadata) and correct Always Present values - Update response examples to use proper JSON format with response envelope - Add section 9.1 Fields for Payment transaction - Remove Index section and all Return to Index links Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Reorganize top-level sections: Abstract (1), Introduction (2), Specification (3), Rationale (4), Security Considerations (5), Appendix - Move all ledger entry, transaction, and RPC sections under "3. Specification" as subsections (3.1-3.9) - Remove "1.1 Overview" heading, merge content into Introduction body - Renumber Introduction subsections: Terminology (2.1), Actors (2.2), Connecting to the Vault (2.3) - Demote all specification headings by one level with new numbering - Add Rationale section explaining decoupled vault design - Rename FAQ section to "Appendix A: FAQ" with A.x numbering - Fix heading levels for Key Variables and Vault State Update Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove functional additions (invariants, example JSONs, error codes) added in this branch and retain only structural changes that bring the spec into conformance with AMENDMENT_TEMPLATE.md and XLS_TEMPLATE.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Convert failure conditions and state changes from numbered lists back to master's original nested bullet-point format. Keep the Data Verification / Protocol-Level Failures subsection headers as template compliance, but use master's original content and structure inside them. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Convert bullet points in Failure Conditions and State Changes sections to numbered lists with nested sub-numbering, per template requirements. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Mayukha Vadari <mvadari@gmail.com>
An error occurred while trying to automatically change base from
tapanito/vault-enhanced
to
master
May 28, 2026 15:12
…-withdraw # Conflicts: # XLS-0065-single-asset-vault/README.md
tyalymov
reviewed
Jul 22, 2026
tyalymov
left a comment
tyalymov
left a comment
There was a problem hiding this comment.
The definition changed from Vault.AssetsTotal to the total assets in the vault, including any future yield.
Two things I'd check:
- What field or formula is "future yield"? The withdrawal conversion in the code uses
Vault.AssetsTotal - Vault.LossUnrealizedwith no yield term anywhere. - The rest of 3.6 uses concrete fields: State Changes and invariant 3.6.4 reconcile
Vault.AssetsTotal, and the liquidity check usesVault.AssetsAvailable. IfGamma_assetis a larger, yield-inclusive value, the conversion rate and the accounting no longer share a base, which could over-value shares on withdrawal.
Could we bind Gamma_asset to a concrete field , or spell out what "future yield" refers to?
Redefine Gamma_asset as Vault.AssetsTotal - Vault.LossUnrealized, matching the conversion base actually used in assetsToSharesWithdraw/sharesToAssetsWithdraw, instead of the undefined "including any future yield" wording. Note the sole-shareholder waiver exception (fixCleanup3_2_0). Addresses review feedback on PR #553.
Collaborator
Author
|
You're right — checked |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Syncs the VaultWithdraw spec section with the current implementation in
src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp.Data Verification (3.6.2.1) — was
_None._, now has three items:temMALFORMED)temBAD_AMOUNT)temMALFORMED)Protocol-Level Failures (3.6.2.2) — reworked:
fixCleanup3_2_0, VaultWithdraw waiveslsfMPTCanTransfer(recovery path allowing withdrawal even when issuer revokes transferability)tecFROZEN/tecLOCKED)tecNO_AUTH)tecNO_DST,tecDST_TAG_NEEDED,tecNO_PERMISSION)tecPRECISION_LOSS)tecPATH_DRY)State Changes (3.6.3) — corrected:
Destinationif provided, otherwiseAccount