feat: change share valuation logic#6564
Conversation
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>
Tapanito
force-pushed
the
tapanito/vault-share-pricing
branch
from
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
Tapanito
force-pushed
the
tapanito/vault-share-pricing
branch
from
98c963a to
2e2c88a
Compare
…into tapanito/vault-share-pricing
Tapanito
force-pushed
the
tapanito/vault-share-pricing
branch
from
2e2c88a to
d9487a0
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## tapanito/lending-fix-amendment #6564 +/- ##
================================================================
- Coverage 81.5% 81.4% -0.0%
================================================================
Files 999 999
Lines 74550 74759 +209
Branches 7560 7612 +52
================================================================
+ Hits 60722 60866 +144
- Misses 13828 13893 +65
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Hey @ximinez , This discussion: #6381 (comment) made me think.. We're changing share valuation logic by introducing sfInterestUnrealized field, and doing a bunch of "stuff" with it. The question is, how do we handle existing Vaults that were created before introducing this field? I'd say, these objects should still use the old share valuation logic, even if the amendment was enabled. However, perhaps they shouldn't be allowed to create new loans? |
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
…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}
| sle->at(sfLossUnrealized) = lossUnrealized; | ||
| sle->at(sfInterestUnrealized) = interestUnrealized; | ||
| sle->at(sfScale) = scale; | ||
| return sle; |
There was a problem hiding this comment.
makeVault omits sfAssetsAvailable, so validateVaultState sees it as 0, masking invariant checks. Add it before returning:
sle->at(sfAssetsAvailable) = assetsTotal;
return sle;
| { | ||
| JLOG(j.error()) << "borrowFromVault: invalid input" | ||
| << " (amount=" << amount << ", yield=" << yield << ")"; | ||
| return tecINTERNAL; |
There was a problem hiding this comment.
SLE fields are mutated before validateVaultState — on error the in-memory SLE is dirty and inconsistent. Validate the expected post-state values before applying mutations, or snapshot and restore fields on failure.
Whatever solution you use, you'll need to change Some ideas:
|
|
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. |
…into tapanito/vault-share-pricing
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
![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; | |
| } | |
| } |
2 participants
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)