Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There is a spec inconsistency in XLS-65 where cash-basis accounting is described as LEVersion = 2 in one section but LEVersion = 1 everywhere else, which would make amendment gating ambiguous.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the XLS-65 and XLS-66 specifications to define LendingProtocolV1_1 accounting behavior where Vault.AssetsTotal and LoanBroker.DebtTotal become principal-only (excluding interest), while preserving pre-amendment behavior for legacy (LEVersion absent) vaults.
Changes:
- Adds
LendingProtocolV1_1-scoped Failure Conditions and State Changes forLoanSet,LoanPay, andLoanManageto implement principal-onlyAssetsTotal/DebtTotalaccounting and removevalueChangereconciliation under the amendment. - Introduces
Vault.LEVersion(protocol-set) in XLS-65 to gate legacy vs cash-basis accounting behavior, plus correspondingVaultCreatestate-change semantics. - Adds an extended worked example comparing pre- vs post-amendment trajectories for
LoanSet→LoanPay→LoanManage(tfLoanDefault).
File summaries
| File | Description |
|---|---|
| XLS-0066-lending-protocol/README.md | Adds amendment-specific Failure Conditions/State Changes and clarifies valueChange becomes dead output under principal-only accounting. |
| XLS-0065-single-asset-vault/README.md | Defines Vault.LEVersion and documents cash-basis vs accrual-basis accounting and amendment-gated creation behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Add XLS-65.1/66.1 patch folders so this PR merges under LendingProtocolV1_1.
There was a problem hiding this comment.
Doc-only spec amendment (XLS-65/XLS-66) defining principal-only Vault/LoanBroker accounting under LendingProtocolV1_1. The worked numeric example in §3.10.5.2 checks out arithmetically for both pre- and post-amendment trajectories, and the new patch-changelog README pattern is consistent with the #570 convention. Found one internal inconsistency: the amended AssetsMaximum failure-condition check in LoanSet keeps a parenthetical description referencing 'interest' even though the whole point of the amendment is that AssetsTotal no longer includes interest at LoanSet time.
There was a problem hiding this comment.
This is a documentation-only change adding two amendment patches (XLS-65.1 and XLS-66.1) plus corresponding cross-linked sections in the XLS-65/XLS-66 top-level READMEs. The worked numeric example in the LoanManage section was checked line-by-line and the arithmetic (LoanSet → LoanPay → default, pre- vs post-amendment) is internally consistent. Cross-reference anchors (GitHub heading-slug format) all resolve correctly to the new subsections they point to. Three issues are worth a second look: the MR description's stated non-goal ('no new Vault-level field') is contradicted by the diff, which does add Vault.LEVersion; the new LEVersion field's stated default value disagrees between the XLS-65.1 patch doc ('absent') and the consolidated XLS-65 README ('0'); and the LoanBrokerSet closed-ended check is spliced directly into the existing numbered failure list instead of following the dedicated 'amendment subsection' pattern (X.X.X.3 (LendingProtocolV1_1)) used consistently everywhere else in this same diff.
Skip the cash-basis AssetsMaximum origination check to match develop, move the closed-ended LoanBrokerSet failure into an amendment subsection, and treat absent LEVersion as 0.
There was a problem hiding this comment.
This is a doc-only spec amendment (XLS-65.1 / XLS-66.1) defining principal-only accounting for LendingProtocolV1_1. The worked numeric example in XLS-66's LoanManage section was checked end-to-end and is internally consistent (all arithmetic ties out across pre- and post-amendment trajectories). Cross-references and anchors between the two top-level READMEs and their new subpages line up correctly. One concrete inconsistency was found: the new LEVersion field is documented as immutable in prose and in an explicit invariant, but the field table in the main XLS-65 README marks it as non-constant (mutable), while the parallel table in the new XLS-65.1 file marks the same field as constant.
There was a problem hiding this comment.
This is a documentation-only PR (three new/updated XLS spec files) with no source code changed. The math in the worked LoanManage default example checks out exactly, and nearly all new cross-reference anchors/relative links resolve correctly, and steps replacing the base spec's numbered items (3.2.6.1, 3.8.6.1, 3.10.5.1, 3.11.5.1) correctly preserve the original item numbers. The one clear defect found is a self-inconsistent list numbering in the new 66.1 patch doc's Failure Conditions section, which says checks '19 and 20' are being replaced but then labels the replacement items '1.' and '2.' instead — inconsistent both internally and against the main README's §3.8.5.3, which correctly keeps '19.'/'20.'. Separately, the MR description states LoanBrokerSet now requires attaching only to a closed-ended Vault, but no such change appears anywhere in the provided diff — worth double-checking that this part of the change wasn't dropped from the submitted diff.
There was a problem hiding this comment.
🟡 Changes recommended
Two remaining documentation conflicts make the cap and default-example semantics inconsistent.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
XLS-0065-single-asset-vault/README.md:118
- The new canonical definition makes
AssetsMaximuma deposit cap that interest may exceed, but the unchangedVaultSetfield table at line 481 still calls it “the maximum asset amount that can be held in a vault.” These definitions conflict and can lead clients to treat an interest overrun as invalid; update theVaultSetdescription to use the same deposit-cap semantics while retaining its rule against setting a nonzero cap below currentAssetsTotal.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
This is a documentation-only change adding the LendingProtocolV1_1 cash-basis accounting amendment (XLS-65.1.3, XLS-66.1) plus corresponding edits to the XLS-65/XLS-66 base specs. I traced the amendment gating logic (LEVersion branches in LoanSet/LoanPay/LoanManage failure conditions and state changes) and re-derived the numeric worked example in §3.10.5.1 step by step (100,000 → 100,090/100,000 → 99,550.45/99,550) — all arithmetic and cross-reference anchors check out and are internally consistent between the main README and the new 66.1/65.1.3 sub-specs. I did not find a confident correctness bug in the changed (+) lines.
There was a problem hiding this comment.
🟡 Changes recommended
Remaining cap and valueChange descriptions conflict with the newly specified cash-basis behavior.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
XLS-0065-single-asset-vault/README.md:118
- should-fix: Although the canonical and
VaultCreaterows now defineAssetsMaximumas a deposit cap that interest may exceed, theVaultSetfield row at line 481 still calls it “the maximum asset amount that can be held.” That remaining hard-cap definition contradicts the reachable interest-over-cap states documented by invariant 11 and cash-basisLoanPay. Update theVaultSetrow to use the same deposit-cap semantics while retaining its rule against setting a nonzero cap below the current total.
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
Documentation-only PR (XLS-65.1.3 / XLS-66.1) that defines cash-basis (principal-only) accounting for Vault/LoanBroker under LendingProtocolV1_1. The core amendment logic (LEVersion gating, DebtTotal/AssetsTotal changes for LoanSet/LoanPay/LoanManage, and the added CoverAvailable cap on DefaultCovered) is internally consistent and the worked numeric example checks out arithmetically. One notable inconsistency: the base Vault field table's 'Constant' column was bulk-edited to mark several fields as unconditionally immutable, which contradicts the amendment-gated immutability described elsewhere in the same document for those same fields.
## High Level Overview of Change <!-- Please include a summary/list of the changes. If too broad, please consider splitting into multiple PRs. If a relevant Asana task, please link it here. --> Support LendingProtocolV1_1 rippled: https://github.com/XRPLF/rippled/pulls?page=1&q=is%3Apr+label%3A%22feature%3A+LP1.1%22 Specs: XRPLF/XRPL-Standards#587 XRPLF/XRPL-Standards#582 ### Context of Change <!-- Please include the context of a change. If a bug fix, when was the bug introduced? What was the behavior? If a new feature, why was this architecture chosen? What were the alternatives? If a refactor, how is this better than the previous implementation? If there is a design document for this feature, please link it here. --> ### Type of Change <!-- Please check relevant options, delete irrelevant ones. --> - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] Refactor (non-breaking change that only restructures code) - [ ] Tests (You added tests for code that already exists, or your new feature included in this PR) - [ ] Documentation Updates - [ ] Release ### Did you update HISTORY.md? - [x] Yes - [ ] No, this change does not impact library users
The pre-amendment Vault books all expected InterestDue at origination, which is not accrual-basis accounting.
There was a problem hiding this comment.
This is a documentation-only change to the XLS-65/XLS-66 spec adding LEVersion-gated cash-basis accounting for Vault/LoanBroker under LendingProtocolV1_1. I traced the new worked numeric example in 66.1 §3.10.5.1 end-to-end (LoanSet → LoanPay → LoanManage default) for both the legacy and cash-basis trajectories and all arithmetic checks out (100,090/1,090 pre-amendment vs 100,000/1,000 post-amendment at origination; 99,550.45 vs 99,550 at default). I also cross-checked that every failure-condition and state-change branch introduced for LendingProtocolV1_1 in the main XLS-65/XLS-66 READMEs has a matching, consistent counterpart in the new 65.1.3 and 66.1 sub-spec files (checks 6/14/19/20 on LoanSet, check 8 on LoanManage, DefaultAmount/LossUnrealized/LossReversed on LoanManage, and the LoanPay valueChange vs principalPaid/interestPaid split), and found no missed branches or contradictions. The unrelated-looking 'Constant' column churn in the Vault field table and the added CoverAvailable cap in the DefaultCovered formula both appear to be legitimate, self-consistent corrections (matching the already-documented 65.1.1 immutability list and the worked example, respectively) rather than bugs introduced by this diff. I did not find any correctness, security, or consistency issues worth flagging in the changed lines.
## High Level Overview of Change <!-- Please include a summary/list of the changes. If too broad, please consider splitting into multiple PRs. If a relevant Asana task, please link it here. --> Support LendingProtocolV1_1 rippled: https://github.com/XRPLF/rippled/pulls?page=1&q=is%3Apr+label%3A%22feature%3A+LP1.1%22 Specs: XRPLF/XRPL-Standards#587 XRPLF/XRPL-Standards#582 ### Type of Change <!-- Please check relevant options, delete irrelevant ones. --> - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] Refactor (non-breaking change that only restructures code) - [ ] Tests (You added tests for code that already exists, or your new feature included in this PR) - [ ] Documentation Updates - [ ] Release ### Did you update CHANGELOG.md? - [x] Yes - [ ] No, this change does not impact library users
Merge master. Keep the base XLS-65 and XLS-66 READMEs on master text, apart from list pointers and the CoverAvailable cap and late-payment DebtTotal fixes. Fill the reserved 65.1.3 and 66.1.1 slots.
|
@frytegg This is obsolete now. The §2.7 deferral line to #587 was removed when this PR was split into 65.1.3 / 66.1.1. On master, the closed-ended |
| 9. - `LendingProtocol (parent)`: `LoanBroker.CoverAvailable < (LoanBroker.DebtTotal + PrincipalRequested + InterestDue) × LoanBroker.CoverRateMinimum` (insufficient first-loss capital). (`tecINSUFFICIENT_FUNDS`) | ||
| - `LendingProtocolV1_1`: If `Vault.LEVersion == 1`, `LoanBroker.CoverAvailable < (LoanBroker.DebtTotal + PrincipalRequested) × LoanBroker.CoverRateMinimum` (insufficient first-loss capital). (`tecINSUFFICIENT_FUNDS`) | ||
|
|
||
| Checks 6 and 14 do not apply to a cash-basis Vault because origination does not credit `InterestDue` into `Vault.AssetsTotal`. A replacement such as `Vault.AssetsTotal > Vault.AssetsMaximum` is not used: it would reject origination from a Vault that is over the cap only because of collected interest. The matching change to the Vault cap invariant is in [XLS-65.1.3](../../XLS-0065-single-asset-vault/65.1/65.1.3-vault-cash-basis.md) §3.1.4. |
There was a problem hiding this comment.
Normative cross-reference error: 'check 14' doesn't exist in enumeration (checks 6–7 above). Change '14' to '7':
| Checks 6 and 14 do not apply to a cash-basis Vault because origination does not credit `InterestDue` into `Vault.AssetsTotal`. A replacement such as `Vault.AssetsTotal > Vault.AssetsMaximum` is not used: it would reject origination from a Vault that is over the cap only because of collected interest. The matching change to the Vault cap invariant is in [XLS-65.1.3](../../XLS-0065-single-asset-vault/65.1/65.1.3-vault-cash-basis.md) §3.1.4. | |
| Checks 6 and 7 do not apply to a cash-basis Vault because origination does not credit `InterestDue` into `Vault.AssetsTotal`. |
| Checks 6 and 14 do not apply to a cash-basis Vault because origination does not credit `InterestDue` into `Vault.AssetsTotal`. A replacement such as `Vault.AssetsTotal > Vault.AssetsMaximum` is not used: it would reject origination from a Vault that is over the cap only because of collected interest. The matching change to the Vault cap invariant is in [XLS-65.1.3](../../XLS-0065-single-asset-vault/65.1/65.1.3-vault-cash-basis.md) §3.1.4. | |
| Checks 6 and 7 do not apply to a cash-basis Vault because origination does not credit `InterestDue` into `Vault.AssetsTotal`. A replacement such as `Vault.AssetsTotal > Vault.AssetsMaximum` is not used: it would reject origination from a Vault that is over the cap only because of collected interest. The matching change to the Vault cap invariant is in [XLS-65.1.3](../../XLS-0065-single-asset-vault/65.1/65.1.3-vault-cash-basis.md) §3.1.4. |
| 7. - `LendingProtocol (parent)`: `Vault.AssetsMaximum != 0` and `Vault.AssetsTotal + InterestDue > Vault.AssetsMaximum` (expected interest would exceed vault assets cap). (`tecLIMIT_EXCEEDED`) | ||
| - `LendingProtocolV1_1`: The check does not apply if `Vault.LEVersion == 1`. | ||
| 8. - `LendingProtocol (parent)`: `LoanBroker.DebtMaximum != 0` and `LoanBroker.DebtMaximum < LoanBroker.DebtTotal + PrincipalRequested + InterestDue` (exceeds maximum debt). (`tecLIMIT_EXCEEDED`) | ||
| - `LendingProtocolV1_1`: If `Vault.LEVersion == 1`, `LoanBroker.DebtMaximum != 0` and `LoanBroker.DebtMaximum < LoanBroker.DebtTotal + PrincipalRequested` (exceeds maximum debt). (`tecLIMIT_EXCEEDED`) | ||
| 9. - `LendingProtocol (parent)`: `LoanBroker.CoverAvailable < (LoanBroker.DebtTotal + PrincipalRequested + InterestDue) × LoanBroker.CoverRateMinimum` (insufficient first-loss capital). (`tecINSUFFICIENT_FUNDS`) | ||
| - `LendingProtocolV1_1`: If `Vault.LEVersion == 1`, `LoanBroker.CoverAvailable < (LoanBroker.DebtTotal + PrincipalRequested) × LoanBroker.CoverRateMinimum` (insufficient first-loss capital). (`tecINSUFFICIENT_FUNDS`) |
There was a problem hiding this comment.
This is a reasonable suggestion
| ## 5. Security Considerations | ||
|
|
||
| Principal-only accounting makes `DebtTotal` a smaller number for the same set of Loans, so a `CoverRateMinimum` that was calibrated under instant interest recognition yields less first-loss capital. A Broker migrating to a cash-basis Vault should re-derive the rate rather than reuse it. | ||
|
|
||
| Both accounting models coexist on the ledger for as long as pre-amendment Vaults exist. An implementation must branch on `Vault.LEVersion` in every place it adjusts `AssetsTotal` or `DebtTotal`; branching in some places and not others corrupts the share exchange rate of the Vault. |
| proposal-from: https://github.com/XRPLF/XRPL-Standards/discussions/190 | ||
| status: Draft | ||
| category: Amendment | ||
| requires: [XLS-65.1.3](../../XLS-0065-single-asset-vault/65.1/65.1.3-vault-cash-basis.md) |
There was a problem hiding this comment.
This is a documentation-only MR adding two new XLS spec patches (65.1.3 and 66.1.1) plus index/README wiring and two base-spec fixes to XLS-66 (capping DefaultCovered by CoverAvailable, and clarifying that late-payment valueChange is reflected in LoanBroker.DebtTotal). I checked the worked example in 66.1.1 §3.5 arithmetic end-to-end (LoanSet, LoanPay, LoanManage default under both the legacy and cash-basis models) and all intermediate figures (AssetsTotal, AssetsAvailable, DebtTotal, DefaultAmount, MinimumCover, DefaultCovered, VaultLoss, CoverAvailable) are internally consistent and reconcile correctly. The cross-references between the two new files (65.1.3's LoanSet cap invariant rewrite vs. 66.1.1's checks 6/14/19/20) are mutually consistent, and the two base-spec fixes in XLS-66 README correctly match their stated rationale (capping DefaultCovered by CoverAvailable prevents it from driving CoverAvailable negative; the A-3.2.2 clarification matches the actual §3.11.5 state-change formula). No logic errors, broken cross-links, or arithmetic mistakes were found in the changed content, so no actionable issues are being raised.


High Level Overview of Change
Specifies cash-basis (principal-only) Vault and LoanBroker accounting under the
LendingProtocolV1_1amendment, as two nested patches:XLS-0065-single-asset-vault/65.1/65.1.3-vault-cash-basis.md: addsVault.LEVersion.VaultCreatesets it to1while the amendment is enabled. Absent means instant interest recognition (pre-amendment Vaults);1means cash basis. Also amends the XLS-65LoanSetcap invariant (item 11) and addsLEVersiontovault_info.XLS-0066-lending-protocol/66.1/66.1.1-loan-cash-basis.md: for a Vault withLEVersion == 1,LoanBroker.DebtTotaltracks principal only andVault.AssetsTotaltakes interest only when it is paid.LoanSet: noInterestDueinAssetsTotalorDebtTotal. Checks 6 and 14 do not apply. Checks 19 and 20 dropInterestDue.LoanPay:DebtTotaldecreases byprincipalPaid,AssetsTotalincreases byinterestPaid.valueChangeis not applied to either.LoanManage:DefaultAmount,LossUnrealizedandLossReversedareLoan.PrincipalOutstanding, and impair check 8 matches.valueChangestatements are scoped. Includes a worked example of a default under both models.These fill the 65.1.3 and 66.1.1 numbers that master reserved. The 65.1 and 66.1 index tables, and the amendment lists and changelogs in the base READMEs, now link them.
The base XLS-65 and XLS-66 READMEs match master, apart from those links and two base-spec fixes in XLS-66 that apply to every Vault:
DefaultCoveredin §3.1.11 and Appendix formula (35) is also capped byCoverAvailable, as §3.10.5 already is.valueChangeis reflected inLoanBroker.DebtTotalthrough thetotalToVault - valueChangereduction in §3.11.5.Out of scope: moving existing Vaults to cash basis (pre-amendment Vaults keep instant interest recognition), and changes to
Loanfields.Type of Change