feat: change share valuation logic - #6564
Conversation
* adds sfMemoData field to VaultDelete transaction
Co-authored-by: Ed Hennis <ed@ripple.com>
…#6527) Use XRPL_FEATURE macro instead of XRPL_FIX since LendingProtocolV1_1 is a feature amendment, not a fix. Update all references in VaultDelete and related tests.
Move vault pricing helpers from xrpl namespace into xrpl::vault with amendment-gated dispatch (fixLendingProtocolV1_1). Extract v2 pure math into public xrpl::vault::math::v2 for unit testability. v1 logic is intentionally left as-is to avoid risk since Single Asset Vault is already released. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ca6a477 to
b45068b
Compare
…mespace The separate pure-math functions added unnecessary indirection since they won't be unit-tested directly. Inline the arithmetic back into the v2 SLE wrappers to match v1's pattern.
…into tapanito/vault-share-pricing
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
1 similar comment
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
Co-authored-by: Bart <bthomee@users.noreply.github.com>
…into tapanito/vault-share-pricing
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| } | ||
| return vault::computeWithdrawByAssets(rules, vault, sleIssuance, amount, j_); | ||
| if (amount.asset() == share) | ||
| return vault::computeWithdrawByShares(rules, vault, sleIssuance, amount, j_); |
There was a problem hiding this comment.
🟠 Severity: HIGH
computeWithdrawByShares also hardcodes WaiveUnrealizedLoss::No via the internal dispatch. The sole shareholder withdrawing by share count is equally affected — they receive fewer assets than entitled to, contradicting the fixCleanup3_2_0 waiver logic still enforced in preclaim.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: The computeWithdrawByShares function (and the anonymous-namespace sharesToAssetsWithdraw it calls) hardcodes WaiveUnrealizedLoss::No, discarding the waiver logic that preclaim properly computes via shouldWaiveWithdrawal. To fix this:
-
In
VaultHelpers.handVaultHelpers.cpp: Add aWaiveUnrealizedLossparameter tocomputeWithdrawByShares(and the anonymous-namespacesharesToAssetsWithdrawit calls at line ~314-322), so the waiver can be propagated instead of hardcoded toNo. -
In
VaultWithdraw.cppdoApply: ComputewaiveUnrealizedLossusingshouldWaiveWithdrawal(ctx_.view(), accountID_, sleIssuance)(same as done inpreclaimat line 114), and pass it to the updatedcomputeWithdrawBySharescall at line 198. -
Similarly for
computeWithdrawByAssets: The same hardcodedWaiveUnrealizedLoss::Nois present in theassetsToSharesWithdrawanonymous-namespace helper (line ~301-311). Consider propagating the waiver there too for consistency.
This ensures that doApply uses the same waived valuation that preclaim approved, preventing the sole shareholder from receiving fewer assets than entitled (or hitting the UNREACHABLE assertion on full withdrawal with unrealized losses).
| } | ||
|
|
||
| return std::make_pair(assetsRecovered, sharesDestroyed); | ||
| auto const result = vault::computeClawback( |
There was a problem hiding this comment.
🟠 Severity: HIGH
When fixCleanup3_1_3 is enabled and clawbackAmount is zero (documented to mean "clawback all"), computeClawback(0) converts 0 assets → 0 shares → returns {0, 0}, causing tecPRECISION_LOSS. The old code explicitly fetched all holder shares for this case. The "clawback all" semantic is broken.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: When fixCleanup3_1_3 is enabled and clawbackAmount is zero ("clawback all" semantic), the code must resolve zero to the holder's actual share balance before computing the exchange, just as the pre-fix path does, but with additional clamping to assetsAvailable.
Add a new branch between the existing pre-fix handler (lines 246–256) and the computeClawback call (line 258) for the post-fix zero-amount case:
- Check
clawbackAmount == beast::kZero(at this point,fixCleanup3_1_3must be enabled since the pre-fix path didn't match). - Fetch the holder's shares via
accountHolds(view(), holder, share, ...). - Compute assets from shares via
vault::computeWithdrawByShares(rules, vault, sleShareIssuance, sharesHeld, j_). - If the computed
assets <= assetsAvailable, return the result directly. - Otherwise, clamp by calling
vault::computeClawback(rules, vault, sleShareIssuance, STAmount{asset, assetsAvailable}, Number{assetsAvailable}, j_)to properly handle the clamping with share truncation and rounding.
This preserves the pre-fix path for ledger replay compatibility while correctly implementing the "clawback all" semantic under the amendment.
| { | ||
| if (rules.enabled(featureLendingProtocolV1_1)) | ||
| return detail::assetsToSharesWithdraw(vault, issuance, assets, truncate); | ||
| return v1::assetsToSharesWithdraw(vault, issuance, assets, truncate, WaiveUnrealizedLoss::No); |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
The anonymous-namespace v1 dispatch hardcodes WaiveUnrealizedLoss::No, making it impossible for the high-level compute* API to ever apply the sole-shareholder waiver. This is the root cause of the VaultWithdraw regression — the waiver path is architecturally blocked.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Thread the WaiveUnrealizedLoss parameter through the entire withdrawal computation path so that the sole-shareholder waiver can actually take effect in doApply():
-
Add a
WaiveUnrealizedLoss waiveparameter to the anonymous-namespace wrapper functionsassetsToSharesWithdraw(line 301) andsharesToAssetsWithdraw(line 313), and forward it instead of hardcodingWaiveUnrealizedLoss::No. -
Add the same
WaiveUnrealizedLoss waiveparameter to the publiccomputeWithdrawByAssetsandcomputeWithdrawBySharesAPIs (and their declarations in the header), forwarding it to the anonymous-namespace helpers. -
In
VaultWithdraw::doApply()(VaultWithdraw.cpp ~line 194-198), computeshouldWaiveWithdrawal()and pass the result tovault::computeWithdrawByAssets/vault::computeWithdrawBySharesso the waiver is consistently applied in bothpreclaim()anddoApply().
This ensures that when the user is the sole shareholder, the unrealized-loss subtraction is waived during the actual withdrawal computation, matching the existing behaviour in preclaim().
| // Update vault state | ||
| if (view.rules().enabled(featureLendingProtocolV1_1)) | ||
| vault->at(sfInterestUnrealized) += yield; | ||
| vault->at(sfAssetsAvailable) -= amount; |
There was a problem hiding this comment.
⚪ Severity: LOW
borrowFromVault mutates the vault SLE in-place (shared_ptr) before calling validateVaultState. If validation fails at line 575, the error is returned but the SLE is never restored. The caller's reference sees corrupted state (reduced AssetsAvailable, inflated AssetsTotal/InterestUnrealized), which can cause incorrect financial calculations if the caller doesn't immediately discard the SLE.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Save the original vault field values before in-place mutation and restore them if validateVaultState (or the issuance read) fails. This prevents callers from observing corrupted in-memory state on error paths.
Replace the mutation + validation block (lines 562–577) with a pattern that saves originals and uses a rollback lambda on error:
- Save
sfAssetsAvailableandsfAssetsTotalbefore mutation. - After mutation, on every error path inside the
featureLendingProtocolV1_1block, roll backsfInterestUnrealized,sfAssetsAvailable, andsfAssetsTotalbefore returning the error.
A concise approach is a rollback lambda that reverses the three mutations, called on every early-return path.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| // Update vault state | |
| if (view.rules().enabled(featureLendingProtocolV1_1)) | |
| vault->at(sfInterestUnrealized) += yield; | |
| vault->at(sfAssetsAvailable) -= amount; | |
| // Update vault state | |
| // Save original values for rollback on validation failure | |
| auto const origAssetsAvailable = vault->at(sfAssetsAvailable); | |
| auto const origAssetsTotal = vault->at(sfAssetsTotal); | |
| if (view.rules().enabled(featureLendingProtocolV1_1)) | |
| vault->at(sfInterestUnrealized) += yield; | |
| vault->at(sfAssetsAvailable) -= amount; | |
| vault->at(sfAssetsTotal) += yield; | |
| if (view.rules().enabled(featureLendingProtocolV1_1)) | |
| { | |
| auto const rollback = [&]() { | |
| vault->at(sfInterestUnrealized) -= yield; | |
| vault->at(sfAssetsAvailable) = origAssetsAvailable; | |
| vault->at(sfAssetsTotal) = origAssetsTotal; | |
| }; | |
| std::shared_ptr<SLE const> issuance = | |
| view.read(keylet::mptIssuance(vault->at(sfShareMPTID))); | |
| if (!issuance) | |
| { | |
| rollback(); // LCOV_EXCL_LINE | |
| return tecINTERNAL; // LCOV_EXCL_LINE | |
| } | |
| if (auto const ter = validateVaultState(vault, issuance, j)) | |
| { | |
| rollback(); | |
| return ter; | |
| } | |
| } |
eaf80ce to
60700c5
Compare
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
There was a problem hiding this comment.
Pull request overview
This PR updates vault share valuation for Lending Protocol v1.1 by introducing sfInterestUnrealized into NAV calculations (deposit vs. withdrawal), refactoring share-pricing into higher-level compute* APIs, and adding new invariants/tests to enforce the new accounting rules. It also extends VaultDelete to optionally carry a top-level sfMemoData field (amendment-gated) for a deletion reason payload.
Changes:
- Add
sfInterestUnrealized(defaulted NUMBER) to the Vault ledger entry and incorporate it into v1.1 deposit/withdraw NAV math. - Introduce new high-level vault exchange APIs returning
std::expected<ExchangeResult, TER>and aborrowFromVaultstate update helper with post-modification validation. - Add/extend tests and invariants for
interestUnrealized, and add amendment-gatedsfMemoDatasupport onVaultDelete.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/libxrpl/protocol_autogen/transactions/VaultDeleteTests.cpp | Adds round-trip coverage for new optional sfMemoData on VaultDelete autogen wrapper/builder. |
| src/tests/libxrpl/protocol_autogen/ledger_entries/VaultTests.cpp | Adds round-trip + nullopt coverage for new default field sfInterestUnrealized on Vault autogen wrapper/builder. |
| src/test/app/VaultHelpers_test.cpp | New unit tests for v2 share-pricing math + high-level compute* APIs + borrowFromVault, plus v1 routing checks. |
| src/test/app/Vault_test.cpp | Adds app-level tests for VaultDelete memo-data validation and feature gating. |
| src/test/app/Invariants_test.cpp | Adds invariant tests for sfInterestUnrealized constraints and immutability rules. |
| src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp | Refactors withdraw pricing to use vault::computeWithdraw* APIs (but currently misses loss-waiver semantics; see comments). |
| src/libxrpl/tx/transactors/vault/VaultDeposit.cpp | Refactors deposit pricing to use vault::computeDeposit. |
| src/libxrpl/tx/transactors/vault/VaultDelete.cpp | Adds amendment-gated top-level sfMemoData validation (length + non-empty). |
| src/libxrpl/tx/transactors/vault/VaultClawback.cpp | Refactors clawback math to vault::computeWithdrawByShares / vault::computeClawback. |
| src/libxrpl/tx/invariants/VaultInvariant.cpp | Adds v1.1 invariant checks for interestUnrealized and restricts when it may change. |
| include/xrpl/tx/invariants/VaultInvariant.h | Extends invariant snapshot struct with interestUnrealized. |
| src/libxrpl/ledger/helpers/VaultHelpers.cpp | Implements v2 NAV logic, validation, compute* APIs, and borrowFromVault; retains v1 math internally. |
| include/xrpl/ledger/helpers/VaultHelpers.h | Declares new vault::detail math + high-level compute* APIs and borrowFromVault. |
| include/xrpl/protocol/detail/sfields.macro | Registers sfInterestUnrealized (NUMBER). |
| include/xrpl/protocol/detail/ledger_entries.macro | Adds sfInterestUnrealized as SoeDefault on ltVAULT. |
| include/xrpl/protocol/detail/transactions.macro | Adds optional top-level sfMemoData to ttVAULT_DELETE. |
| include/xrpl/protocol/detail/features.macro | Introduces featureLendingProtocolV1_1. |
| include/xrpl/protocol_autogen/transactions/VaultDelete.h | Autogen wrapper/builder support for sfMemoData. |
| include/xrpl/protocol_autogen/ledger_entries/Vault.h | Autogen wrapper/builder support for sfInterestUnrealized. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| auto const waiveUnrealizedLoss = shouldWaiveWithdrawal(ctx.view, account, sleIssuance); | ||
| try | ||
| { | ||
| auto const maybeAssets = | ||
| sharesToAssetsWithdraw(vault, sleIssuance, amount, waiveUnrealizedLoss); | ||
| if (!maybeAssets) | ||
| return tefINTERNAL; // LCOV_EXCL_LINE | ||
| auto const assetsEquiv = | ||
| vault::sharesToAssetsWithdraw(vault, sleIssuance, amount, waiveUnrealizedLoss); |
| auto const result = [&]() -> std::expected<vault::ExchangeResult, TER> { | ||
| if (amount.asset() == vaultAsset) | ||
| { | ||
| // Fixed assets, variable shares. | ||
| { | ||
| auto const maybeShares = assetsToSharesWithdraw( | ||
| vault, sleIssuance, amount, TruncateShares::No, waiveUnrealizedLoss); | ||
| if (!maybeShares) | ||
| return tecINTERNAL; // LCOV_EXCL_LINE | ||
| sharesRedeemed = *maybeShares; | ||
| } | ||
|
|
||
| if (sharesRedeemed == beast::kZero) | ||
| return tecPRECISION_LOSS; | ||
| auto const maybeAssets = | ||
| sharesToAssetsWithdraw(vault, sleIssuance, sharesRedeemed, waiveUnrealizedLoss); | ||
| if (!maybeAssets) | ||
| return tecINTERNAL; // LCOV_EXCL_LINE | ||
| assetsWithdrawn = *maybeAssets; | ||
| } | ||
| else if (amount.asset() == share) | ||
| { | ||
| // Fixed shares, variable assets. | ||
| sharesRedeemed = amount; | ||
| auto const maybeAssets = | ||
| sharesToAssetsWithdraw(vault, sleIssuance, sharesRedeemed, waiveUnrealizedLoss); | ||
| if (!maybeAssets) | ||
| return tecINTERNAL; // LCOV_EXCL_LINE | ||
| assetsWithdrawn = *maybeAssets; | ||
| } | ||
| else | ||
| { | ||
| return tefINTERNAL; // LCOV_EXCL_LINE | ||
| } | ||
| } | ||
| catch (std::overflow_error const&) | ||
| { | ||
| // It's easy to hit this exception from Number with large enough Scale | ||
| // so we avoid spamming the log and only use debug here. | ||
| JLOG(j_.debug()) // | ||
| << "VaultWithdraw: overflow error with" | ||
| << " scale=" << (int)vault->at(sfScale).value() // | ||
| << ", assetsTotal=" << vault->at(sfAssetsTotal).value() | ||
| << ", sharesTotal=" << sleIssuance->at(sfOutstandingAmount) | ||
| << ", amount=" << amount.value(); | ||
| return tecPATH_DRY; | ||
| } | ||
| return vault::computeWithdrawByAssets(rules, vault, sleIssuance, amount, j_); | ||
| if (amount.asset() == share) | ||
| return vault::computeWithdrawByShares(rules, vault, sleIssuance, amount, j_); |
| [[nodiscard]] STAmount | ||
| sharesToAssetsWithdraw( | ||
| SLE::const_ref vault, | ||
| SLE::const_ref issuance, | ||
| STAmount const& shares, | ||
| WaiveUnrealizedLoss waive = WaiveUnrealizedLoss::No); |
| computeWithdrawByAssets( | ||
| Rules const& rules, | ||
| SLE::const_ref vault, | ||
| SLE::const_ref issuance, | ||
| STAmount const& assets, |
| #include <expected> | ||
|
|
||
| namespace xrpl { | ||
| namespace xrpl::vault { |
|
Closing in favor of: #7817 |
Summary
Specification: XRPLF/XRPL-Standards#485
interestUnrealizedinto vault share pricing math (v2, gated behindfeatureLendingProtocolV1_1). Deposit NAV usesassetsTotal - interestUnrealized, withdrawal NAV usesassetsTotal - interestUnrealized - lossUnrealized. v1 behaviour is unchanged.compute*API returningExpected<ExchangeResult, TER>, replacing rawstd::optional<STAmount>in transactors.borrowFromVaulthelper that updates vault state when a loan is issued, tracking yield ininterestUnrealized(v1_1 only) with post-modification state validation.interestUnrealized: non-negative, within lent assets, deposit/withdrawal NAV bounds, shares consistency, and immutability outside loan transactions.borrowFromVaultand the new share pricing API in a follow-up PR.High Level Overview of Change
Context of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)