Repository navigation
Conversation
Introduces a dedicated `LoanAccept` transaction (type 84) for the two-step loan creation flow instead of overloading `LoanSet` with `tfLoanSetAccept`. Clarifies that expired pending loans require an explicit `LoanDelete` to reclaim `Vault.AssetsReserved`.
Keep expired acceptances from mutating the pending Loan, assign LoanAccept type 83, and apply V1.1 cash-basis deltas on proposal and delete.
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall the two-step loan flow (LoanAccept) and AssetsReserved additions are documented thoroughly, and the extensive section renumbering (3.9→3.12) plus internal anchor links were re-verified and are all correctly updated. TransactionType values (LoanAccept=83, LoanPay bumped to 84) avoid collisions, and the reserve/Vault bookkeeping for propose→accept→delete is symmetric. The one notable gap is that LoanAccept's state changes never recompute Loan.StartDate/NextPaymentDueDate after acceptance, even though the two-step flow's StartDate is described as an acceptance deadline rather than the loan's actual start.
There was a problem hiding this comment.
Documentation-only change adding XLS-65 AssetsReserved and the XLS-66 two-step LoanSet/LoanAccept flow. The spec text is largely internally consistent (flag bitmasks don't collide, new anchors resolve correctly, reserve/vault bookkeeping nets out correctly across proposal/accept/delete). Two things worth double-checking before merge: the TransactionType reassignment for LoanPay, and whether interest should be recognized in Vault.AssetsTotal before a loan proposal is actually accepted.
|
@ai-review --aggressive |
There was a problem hiding this comment.
🟡 Changes recommended
The updated specs contain concrete field-table inaccuracies (e.g., incorrect internal types) and a mismatch between the PR description and the documented LoanSet additions that should be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the XLS-65 and XLS-66 specifications to document the LendingProtocolV1_1 amendment, adding asset reservation accounting in Vaults and introducing a two-step loan creation flow (proposal + acceptance) using a new LoanAccept transaction.
Changes:
- XLS-65: Adds
AssetsReservedto theVaultledger entry andvault_infoRPC response to track assets reserved for pending loans. - XLS-66: Introduces pending-loan semantics (
lsfLoanPending), addsLoanAccept, and updatesLoanSet,LoanDelete,LoanManage, andLoanPayrules to support the two-step flow. - Expands/adjusts failure conditions and state changes to cover proposal expiration, acceptance, reserve transfer, and pending-loan deletion.
File summaries
| File | Description |
|---|---|
XLS-0066-lending-protocol/README.md |
Documents the two-step loan creation flow and pending-loan lifecycle, adds LoanAccept, and updates related ledger/transaction behaviors. |
XLS-0065-single-asset-vault/README.md |
Adds AssetsReserved to the Vault ledger entry and vault_info RPC response to support pending-loan asset reservation tracking. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
This is a well-specified two-step loan creation flow for XLS-66 plus the AssetsReserved field for XLS-65. Cross-references, failure-condition numbering, and state-change bookkeeping (reserve/OwnerCount/Vault balances) are internally consistent between LoanSet proposal, LoanAccept, and pending-Loan deletion. One spec-table inconsistency stands out: AssetsReserved is marked as an unconditionally Required field on the Vault ledger entry even though it's gated behind the LendingProtocolV1_1 amendment (and is correctly marked optional in the vault_info RPC table), which mirrors the exact conditional treatment this same diff correctly applied to Loan.OwnerNode.
There was a problem hiding this comment.
🟡 Changes recommended
The spec introduces a transaction-type ID conflict (renumbering LoanPay) and has a missing data-verification rule for StartDate without Borrower, which must be resolved for protocol correctness and implementability.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
XLS-0066-lending-protocol/README.md:1547
LoanPayis currently specified as transaction type84, but transaction type IDs are protocol constants andLoanPaywas already assigned83in this spec. KeepLoanPayat83and assign the new ID (84) toLoanAcceptinstead.
| `TransactionType` | Yes | `string` | `UINT16` | `84` | The transaction type. |
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
| 19. `StartDate` is specified and either the `Counterparty` or the `CounterpartySignature` field is specified. (`temINVALID`) | ||
| 20. `Borrower` is specified and `StartDate` is not specified. (`temINVALID`) |
There was a problem hiding this comment.
Fixed in 66.3.1 §3.2.3.1. Added the check 'StartDate is specified and Borrower is not specified (temINVALID)'. The field-combination checks now run before parent check 2, so they fail with temINVALID and not temBAD_SIGNER.
Merge origin/master. Restore the XLS-65 and XLS-66 base READMEs to master and move the change into 65.3.3 (Vault.AssetsReserved) and 66.3.1 (two-step loan creation, LoanAccept). Add the 65.3 and 66.3 indexes.
Tapanito
changed the title
There was a problem hiding this comment.
This is a well-structured, internally self-consistent spec addition (XLS-65.3.3 AssetsReserved + XLS-66.3.1 two-step LoanSet/LoanAccept). Field tables, invariants, failure conditions, and state-change sections cross-reference each other correctly (e.g., AssetsReserved increase/decrease amounts match between the Vault spec and the Loan spec, reserve charges move correctly between LoanBroker.Owner and Borrower). The main issues found are consistency/reference problems: the new section-index READMEs point at sibling specs that aren't part of this diff (so their existence can't be verified here), and the amendment name used throughout every new file (LendingProtocolV1_2) does not match the MR title/description (LendingProtocolV1_1), which is worth confirming before merge.
There was a problem hiding this comment.
This is a well-structured, internally-consistent pair of XLS spec patches for two-step loan creation. The main issue found is a JSON-type inconsistency in the new Vault.AssetsReserved field: the field table declares it as JSON type number, but the Example JSON directly below renders it as a quoted string ("100"), and the sibling vault_info RPC field for the same value is documented as string. Everything else (invariants, reserve/ownership transfer logic between LoanSet/LoanAccept/LoanDelete, StartDate/expiry boundary conditions, Vault.AssetsReserved increase/decrease bookkeeping across the three transactions) checks out as self-consistent.
There was a problem hiding this comment.
Documentation-only MR adding XLS-65.3.3 (Vault.AssetsReserved) and XLS-66.3.1 (two-step LoanSet/LoanAccept) plus their index READMEs. The spec content is internally consistent (invariants, state-change deltas for AssetsAvailable/AssetsReserved/AssetsTotal, and the reserve-transfer logic between LoanBroker.Owner and Borrower all check out). Found a couple of minor documentation issues: the two new index READMEs link to sibling spec files (65.3.1/65.3.2/65.3.4, 66.3.2/66.3.3) that aren't part of this diff, and the new 66.3.1 file uses markdown link syntax inside a <pre> block where it won't render.
There was a problem hiding this comment.
This is a well-constructed, internally consistent pair of spec patches (XLS-65.3.3 and XLS-66.3.1) for two-step loan creation. I traced the state-change and invariant logic across proposal creation, acceptance, and deletion (AssetsReserved/AssetsAvailable/AssetsTotal transitions, OwnerCount/reserve transfers, LoanManage/LoanPay rejection of pending loans) and found no logical contradictions — the accounting is symmetric between creation and reversal paths, and failure-condition numbering changes are scoped correctly to the proposal vs. single-transaction flows. The only issue worth flagging is a documentation-rendering defect in the new 66.3.1 frontmatter.
Summary
Two-step loan creation under the
LendingProtocolV1_2amendment, written as patches to XLS-65 and XLS-66. The base READMEs only gain a one-line pointer to the new indexes.New files:
XLS-0065-single-asset-vault/65.3/README.md: index of LendingProtocolV1_2 Vault changes.XLS-0065-single-asset-vault/65.3/65.3.3-vault-assets-reserved.md: addsVault.AssetsReservedfor assets held for pending Loans, with invariants and thevault_inforesponse field.XLS-0066-lending-protocol/66.3/README.md: index of LendingProtocolV1_2 Lending changes.XLS-0066-lending-protocol/66.3/66.3.1-two-step-loan-creation.md(requires 65.3.3):LoanSetgainsBorrowerandStartDatefor a Loan proposal from theLoanBroker.Owner.LoanAccepttransaction, withLoanIDas its only field, for the Borrower to accept the Loan.lsfLoanPendingflag onLoan.OwnerNodeis absent until the Loan is accepted.LoanManageandLoanPayreject a pending Loan.Motivation
The current
LoanSetneeds a custom dual signature (CounterpartySignature). Only wallets and custodians that implement it can be used to borrow. The two-step flow uses two standard single-signer transactions instead.