Support for Lending Protocol (XLS-66d) #866
Conversation
…ng; LoanDraw transaction has nt been implemented
WalkthroughAdds LendingProtocol amendment and full lending support: new loan and loan-broker transaction models/enums, binary codec fields, AccountObjects enum entries, LOAN_SET fee handling for counterparty signers, tests, and a CHANGELOG entry for XLS-66d. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Issuer
actor Borrower
actor Depositor
participant Client as xrpl-py Client
participant Ledger as XRPL Ledger
rect rgb(230,245,255)
note right of Client: Setup vault & broker
Issuer->>Client: VaultCreate(...)
Client->>Ledger: VAULT_CREATE
Ledger-->>Client: tesSUCCESS
Issuer->>Client: LoanBrokerSet(vault_id,...)
Client->>Ledger: LOAN_BROKER_SET
Ledger-->>Client: tesSUCCESS
end
rect rgb(240,255,240)
note right of Client: Deposit & Loan issuance (counterparty signing)
Depositor->>Client: VaultDeposit(amount)
Client->>Ledger: VAULT_DEPOSIT
Ledger-->>Client: tesSUCCESS
Issuer->>Client: LoanSet(..., CounterpartySignature?)
alt CounterpartySignature present
Borrower->>Client: provide CounterpartySignature.signers (N)
Client->>Client: fee calc adds net_fee * N
Client->>Ledger: LOAN_SET (includes signers)
else No counterparty signature
Client->>Ledger: LOAN_SET
end
Ledger-->>Client: tesSUCCESS / tec*/tef*
Client->>Ledger: AccountObjects(query LOAN)
Ledger-->>Client: Loan object
end
rect rgb(255,250,230)
note right of Client: Manage, Pay, Delete flows
Borrower->>Client: LoanManage(flags)
Client->>Ledger: LOAN_MANAGE
Ledger-->>Client: tesSUCCESS
Borrower->>Client: LoanPay(loan_id, amount)
Client->>Ledger: LOAN_PAY
Ledger-->>Client: tesSUCCESS / tecTOO_SOON
Issuer->>Client: LoanDelete(loan_id)
Client->>Ledger: LOAN_DELETE
Ledger-->>Client: tecHAS_OBLIGATIONS / tesSUCCESS
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
xrpl/core/binarycodec/definitions/definitions.json (1)
3384-3413: Likely omissions: LEDGER_ENTRY_TYPES missing Loan and LoanBroker.Given added fields (LoanID, LoanBrokerID, LoanBrokerNode) and AccountObjects support, the codec should define
LEDGER_ENTRY_TYPESfor these. Without them, decoding such ledger objects can fail or produce “Invalid”.Provide the canonical numeric codes and I’ll draft the exact patch.
xrpl/models/transactions/loan_broker_set.py (1)
59-63: Add server-side-aligned validation for documented ranges.Enforce the documented bounds and basic hex-size checks for data.
transaction_type: TransactionType = field( default=TransactionType.LOAN_BROKER_SET, init=False, ) + + def _get_errors(self: Self) -> Dict[str, str]: + parent = {k: v for k, v in {**super()._get_errors()}.items() if v is not None} + + if self.management_fee_rate is not None and not (0 <= self.management_fee_rate <= 10_000): + parent["LoanBrokerSet:ManagementFeeRate"] = "Must be between 0 and 10000." + + if self.debt_maximum is not None and self.debt_maximum < 0: + parent["LoanBrokerSet:DebtMaximum"] = "Must be >= 0." + + if self.cover_rate_minimum is not None and not (0 <= self.cover_rate_minimum <= 100_000): + parent["LoanBrokerSet:CoverRateMinimum"] = "Must be between 0 and 100000." + + if self.cover_rate_liquidation is not None and not (0 <= self.cover_rate_liquidation <= 100_000): + parent["LoanBrokerSet:CoverRateLiquidation"] = "Must be between 0 and 100000." + + if self.data is not None: + try: + b = bytes.fromhex(self.data) + except ValueError: + parent["LoanBrokerSet:Data"] = "Must be hex-encoded." + else: + if len(b) > 256: + parent["LoanBrokerSet:Data"] = "Must be <= 256 bytes." + + return parent
🧹 Nitpick comments (14)
CHANGELOG.md (1)
10-12: Match existing changelog style for new entry.Use backticks around the feature name for consistency with prior entries.
-### Added -- Support for the Lending Protocol (XLS-66d) +### Added +- Support for `Lending Protocol` (XLS-66d)xrpl/models/transactions/loan_manage.py (2)
3-3: Outdated comment on Python version.The project targets Python ≥3.8. The “Requires Python 3.7+” note is misleading.
-from __future__ import annotations # Requires Python 3.7+ +from __future__ import annotations # Postponed evaluation of annotations
14-47: Hook up flag interface to flag mapping.Defining LoanManageFlag/Interface is good; ensure
interface_to_flag_listandcheck_false_flag_definitionknow these flags for dict-basedflags(e.g.,{"TF_LOAN_IMPAIR": True}) to work.If not yet mapped, add the LoanManage flags to the central per-transaction flag map (same place other tx flags live), and consider a unit test that sets flags as a dict and asserts correct int encoding.
tests/unit/models/transactions/test_loan_broker_cover_clawback.py (1)
11-53: Add a positive IOU case.Consider a test with a valid IssuedCurrencyAmount (>0) to complement the negative and XRP cases.
@@ def test_valid_loan_broker_cover_clawback(self): tx = LoanBrokerCoverClawback( account=_SOURCE, amount=MPTAmount( mpt_issuance_id=_ISSUER, value="10.20", ), loan_broker_id=_ISSUER, ) self.assertTrue(tx.is_valid()) + + def test_valid_iou_amount(self): + tx = LoanBrokerCoverClawback( + account=_SOURCE, + amount=IssuedCurrencyAmount(issuer=_ISSUER, currency="USD", value="10"), + ) + self.assertTrue(tx.is_valid())xrpl/models/transactions/loan_delete.py (2)
3-3: Outdated comment on Python version.Align the comment with project targets.
-from __future__ import annotations # Requires Python 3.7+ +from __future__ import annotations # Postponed evaluation of annotations
15-27: Optional: basic client-side loan_id validation.A light hex-length check can surface errors earlier without breaking existing flows.
@dataclass(frozen=True, **KW_ONLY_DATACLASS) class LoanDelete(Transaction): @@ transaction_type: TransactionType = field( default=TransactionType.LOAN_DELETE, init=False, ) + + def _get_errors(self) -> dict[str, str]: + errors = super()._get_errors() + # Hash256 expects 64 hex chars + if not isinstance(self.loan_id, str) or len(self.loan_id) != 64: + errors["loan_id"] = "loan_id must be a 64-character hex string" + return errorsxrpl/asyncio/transaction/main.py (1)
520-539: Clarify fee math and variable naming for LOAN_SET counterparty signatures.Rename local var to avoid confusion with multisign signers_count; also confirm the “+ BaseFee” assumption when counterparty_signature is absent matches rippled (PR 5270). Otherwise we may over/under-estimate fees.
Apply this diff (rename only):
- if loan_set.counterparty_signature is not None: - signer_count = ( - len(loan_set.counterparty_signature.signers) - if loan_set.counterparty_signature.signers is not None - else 1 - ) - base_fee += net_fee * signer_count + if loan_set.counterparty_signature is not None: + counterparty_signer_count = ( + len(loan_set.counterparty_signature.signers) + if loan_set.counterparty_signature.signers is not None + else 1 + ) + base_fee += net_fee * counterparty_signer_countWould you like me to wire an override knob (e.g., counterparty_signers_count) into autofill/check_fee to avoid the default “assume 1” path?
xrpl/models/transactions/loan_broker_cover_clawback.py (1)
16-63: Solid validation; minor doc terminology nit.Logic mirrors spec intent (no XRP, non-negative OK). Consider standardizing to “First‑Loss Capital” phrasing like other files.
Apply this diff:
- """This transaction withdraws First Loss Capital from a Loan Broker""" + """This transaction withdraws First‑Loss Capital from a Loan Broker""" @@ - The Loan Broker ID from which to withdraw First-Loss Capital. Must be provided if + The Loan Broker ID from which to withdraw First‑Loss Capital. Must be provided if @@ - The First-Loss Capital amount to clawback. If the amount is 0 or not provided, + The First‑Loss Capital amount to clawback. If the amount is 0 or not provided,tests/integration/transactions/test_lending_protocol.py (3)
111-121: Fee may be underfilled when adding CounterpartySignature after signing.autofill_and_sign computes Fee before counterparty_signature inflates the tx size. Consider autofill(tx) → compute borrower signature → issuer sign(tx) → submit, or recompute Fee and re-sign both parties after adding counterparty_signature.
I can propose a small helper to build two-party-signed LoanSet with correct fee ordering—want a patch?
Also applies to: 129-141
108-109: Typo in comment.“transered” → “transferred”.
- # transaction and the requested principal (excluding fees) is transered to + # transaction and the requested principal (excluding fees) is transferred to
62-66: Address property consistency.You mix classic_address and address for the same wallet. Prefer classic_address throughout tests to avoid ambiguity.
xrpl/models/transactions/loan_broker_set.py (1)
5-7: Import typing.Self and Dict for validator.Needed for a typed _get_errors implementation below.
-from typing import Optional +from typing import Optional, Dict +from typing_extensions import Selfxrpl/models/transactions/loan_set.py (2)
22-24: Doc: counterparty vs lender terminology.The signature here is from the counterparty (borrower in tests), not necessarily the “Lender”.
- An inner object that contains the signature of the Lender over the transaction. + An inner object that contains the signature of the counterparty over the transaction.
104-108: Fix minor doc typos (“in in”) and tighten wording.- Annualized interest rate of the Loan in in 1/10th basis points. Valid values are + Annualized interest rate of the Loan in 1/10th basis points. Valid values are @@ - A premium added to the interest rate for late payments in in 1/10th basis points. + A premium added to the interest rate for late payments in 1/10th basis points. @@ - A Fee Rate charged for repaying the Loan early in 1/10th basis points. Valid values + A fee rate charged for repaying the Loan early in 1/10th basis points. Valid values @@ - An interest rate charged on overpayments in 1/10th basis points. Valid values are + An interest rate charged on overpayments in 1/10th basis points. Valid values areAlso applies to: 110-114, 116-120, 122-126
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.ci-config/rippled.cfg(1 hunks)CHANGELOG.md(1 hunks)tests/integration/transactions/test_lending_protocol.py(1 hunks)tests/unit/models/transactions/test_loan_broker_cover_clawback.py(1 hunks)tests/unit/models/transactions/test_loan_set.py(1 hunks)xrpl/asyncio/transaction/main.py(2 hunks)xrpl/core/binarycodec/definitions/definitions.json(8 hunks)xrpl/models/requests/account_objects.py(1 hunks)xrpl/models/transactions/__init__.py(2 hunks)xrpl/models/transactions/loan_broker_cover_clawback.py(1 hunks)xrpl/models/transactions/loan_broker_cover_deposit.py(1 hunks)xrpl/models/transactions/loan_broker_cover_withdraw.py(1 hunks)xrpl/models/transactions/loan_broker_delete.py(1 hunks)xrpl/models/transactions/loan_broker_set.py(1 hunks)xrpl/models/transactions/loan_delete.py(1 hunks)xrpl/models/transactions/loan_manage.py(1 hunks)xrpl/models/transactions/loan_pay.py(1 hunks)xrpl/models/transactions/loan_set.py(1 hunks)xrpl/models/transactions/types/transaction_type.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-10-30T20:34:35.451Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Applied to files:
xrpl/models/transactions/__init__.pyxrpl/models/transactions/loan_pay.py
📚 Learning: 2024-12-11T01:47:28.074Z
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:78-84
Timestamp: 2024-12-11T01:47:28.074Z
Learning: In the `xrpl-py` codebase, within the models (e.g., in `xrpl/models/transactions/`), flag validation checks are not performed; flag checks are handled elsewhere outside of the models.
Applied to files:
xrpl/models/transactions/loan_manage.py
📚 Learning: 2025-06-05T20:52:31.099Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#757
File: xrpl/asyncio/transaction/main.py:505-509
Timestamp: 2025-06-05T20:52:31.099Z
Learning: The correct fee calculation for XRPL Batch transactions follows the C++ rippled implementation structure: (2 × base_fee) + sum of individual inner transaction fees + (signer_count × base_fee). Each inner transaction's fee should be calculated individually using the same fee calculation logic as standalone transactions, not assumed to be equal to base_fee.
Applied to files:
xrpl/asyncio/transaction/main.py
🧬 Code graph analysis (14)
tests/unit/models/transactions/test_loan_broker_cover_clawback.py (5)
xrpl/models/amounts/issued_currency_amount.py (1)
IssuedCurrencyAmount(21-51)xrpl/models/amounts/mpt_amount.py (1)
MPTAmount(18-51)xrpl/models/exceptions.py (1)
XRPLModelException(6-9)xrpl/models/transactions/loan_broker_cover_clawback.py (1)
LoanBrokerCoverClawback(18-63)xrpl/models/base_model.py (1)
is_valid(295-302)
xrpl/models/transactions/loan_delete.py (3)
xrpl/models/transactions/transaction.py (1)
Transaction(184-551)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)xrpl/models/utils.py (1)
require_kwargs_on_init(317-370)
xrpl/models/transactions/loan_broker_cover_withdraw.py (4)
xrpl/core/binarycodec/types/amount.py (1)
Amount(296-456)xrpl/models/transactions/transaction.py (1)
Transaction(184-551)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)xrpl/models/utils.py (1)
require_kwargs_on_init(317-370)
xrpl/models/transactions/loan_broker_cover_clawback.py (3)
xrpl/models/amounts/amount.py (2)
get_amount_value(59-75)is_xrp(17-28)xrpl/models/transactions/transaction.py (1)
Transaction(184-551)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)
xrpl/models/transactions/loan_set.py (5)
xrpl/models/base_model.py (1)
BaseModel(93-418)xrpl/models/transactions/transaction.py (2)
Transaction(184-551)TransactionFlagInterface(173-179)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)xrpl/models/utils.py (1)
require_kwargs_on_init(317-370)xrpl/models/transactions/loan_broker_cover_clawback.py (1)
_get_errors(39-63)
tests/unit/models/transactions/test_loan_set.py (3)
xrpl/models/exceptions.py (1)
XRPLModelException(6-9)xrpl/models/transactions/loan_set.py (1)
LoanSet(56-177)xrpl/models/base_model.py (1)
is_valid(295-302)
xrpl/models/transactions/loan_broker_cover_deposit.py (4)
xrpl/core/binarycodec/types/amount.py (1)
Amount(296-456)xrpl/models/transactions/transaction.py (1)
Transaction(184-551)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)xrpl/models/utils.py (1)
require_kwargs_on_init(317-370)
xrpl/models/transactions/loan_broker_delete.py (3)
xrpl/models/transactions/transaction.py (1)
Transaction(184-551)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)xrpl/models/utils.py (1)
require_kwargs_on_init(317-370)
tests/integration/transactions/test_lending_protocol.py (5)
tests/integration/integration_test_case.py (1)
IntegrationTestCase(9-18)tests/integration/it_utils.py (3)
fund_wallet_async(120-129)sign_and_reliable_submission_async(191-223)test_async_and_sync(268-363)xrpl/asyncio/transaction/main.py (3)
autofill_and_sign(127-153)submit(156-185)sign(81-124)xrpl/core/binarycodec/main.py (1)
encode_for_signing(37-52)xrpl/wallet/main.py (4)
Wallet(16-289)create(121-135)classic_address(34-41)address(24-29)
xrpl/models/transactions/__init__.py (9)
xrpl/models/transactions/loan_broker_cover_clawback.py (1)
LoanBrokerCoverClawback(18-63)xrpl/models/transactions/loan_broker_cover_deposit.py (1)
LoanBrokerCoverDeposit(16-32)xrpl/models/transactions/loan_broker_cover_withdraw.py (1)
LoanBrokerCoverWithdraw(17-38)xrpl/models/transactions/loan_broker_delete.py (1)
LoanBrokerDelete(15-27)xrpl/models/transactions/loan_broker_set.py (1)
LoanBrokerSet(16-62)xrpl/models/transactions/loan_delete.py (1)
LoanDelete(15-27)xrpl/models/transactions/loan_manage.py (1)
LoanManage(51-63)xrpl/models/transactions/loan_pay.py (1)
LoanPay(15-33)xrpl/models/transactions/loan_set.py (1)
LoanSet(56-177)
xrpl/models/transactions/loan_pay.py (3)
xrpl/models/transactions/transaction.py (1)
Transaction(184-551)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)xrpl/models/utils.py (1)
require_kwargs_on_init(317-370)
xrpl/models/transactions/loan_manage.py (3)
xrpl/models/transactions/transaction.py (2)
Transaction(184-551)TransactionFlagInterface(173-179)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)xrpl/models/utils.py (1)
require_kwargs_on_init(317-370)
xrpl/models/transactions/loan_broker_set.py (3)
xrpl/models/transactions/transaction.py (1)
Transaction(184-551)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)xrpl/models/utils.py (1)
require_kwargs_on_init(317-370)
xrpl/asyncio/transaction/main.py (2)
xrpl/models/transactions/loan_set.py (1)
LoanSet(56-177)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)
🪛 GitHub Actions: Integration test
xrpl/asyncio/transaction/main.py
[error] 490-490: Error calculating fee per transaction type: failed to fetch fee from ledger (ConnectError).
🔇 Additional comments (10)
.ci-config/rippled.cfg (1)
209-210: Ensure CI rippled image includes LendingProtocol amendment.If the container’s rippled doesn’t have LendingProtocol, node startup will fail. Please pin the CI Docker image/tag (or commit) that includes this amendment, or add a pre-check that skips tests if the amendment isn’t present.
Would you like a short check added in the integration test to assert LendingProtocol is enabled via
feature/server_infobefore proceeding?xrpl/models/transactions/types/transaction_type.py (1)
33-41: Verified — no action required. definitions.json TRANSACTION_TYPES, concrete model classes, and xrpl/models/transactions/init.py exports exist for all new Loan* transaction types.xrpl/models/requests/account_objects.py (1)
30-31: Missing LEDGER_ENTRY_TYPES for Loan and LoanBroker in definitions.jsonxrpl/models/requests/account_objects.py adds LOAN / LOAN_BROKER (around lines 30–31); definitions.json must include matching LEDGER_ENTRY_TYPES with the rippled numeric codes or decoding ledger entries may fail. Add these two entries to LEDGER_ENTRY_TYPES with the correct numeric values used by rippled, or revert the types until supported. I can provide the exact numeric codes once you confirm where definitions.json is maintained.
xrpl/models/transactions/loan_manage.py (1)
22-33: Confirm flag bit values match rippled.Please verify TF_LOAN_DEFAULT/IMPAIR/UNIMPAIR bitmasks match rippled’s constants to avoid interop issues.
I can cross-check against the C++ impl and propose an update if needed.
xrpl/core/binarycodec/definitions/definitions.json (1)
183-192: Sanity check complete — no duplicate field indices and all new tx types present.
JSON parses; no duplicate (type:nth) in FIELDS; all requested Loan TRANSACTION_TYPES found.xrpl/models/transactions/__init__.py (1)
43-51: Exports for new Loan/ transactions look correct.*Imports and all entries for LoanBroker* and Loan* are consistent with TransactionType and match project patterns.
Also applies to: 177-185
xrpl/models/transactions/loan_broker_delete.py (1)
13-27: LGTM — minimal, consistent with other transaction models.Fields, defaults, and decorators follow established patterns.
tests/unit/models/transactions/test_loan_set.py (1)
11-51: Good coverage of LoanSet validation paths.Tests assert both invalid branches and a valid case; messages match model keys.
xrpl/asyncio/transaction/main.py (1)
19-26: Importing LoanSet for fee calc: OK.Scoped import is minimal and aligns with usage below.
xrpl/models/transactions/loan_broker_set.py (1)
41-45: Clarify default semantics for debt_maximum.Doc says “default value of 0 means no limit” but the field defaults to None (omitted). Confirm server treats omitted as unlimited or set explicit 0 in code/docs.
| def _get_errors(self: Self) -> Dict[str, str]: | ||
| parent_class_errors = { | ||
| key: value | ||
| for key, value in { | ||
| **super()._get_errors(), | ||
| }.items() | ||
| if value is not None | ||
| } | ||
|
|
||
| if self.payment_interval is not None and self.payment_interval < 60: | ||
| parent_class_errors["LoanSet:PaymentInterval"] = ( | ||
| "Payment interval must be at least 60 seconds." | ||
| ) | ||
|
|
||
| if ( | ||
| self.grace_period is not None | ||
| and self.payment_interval is not None | ||
| and self.grace_period > self.payment_interval | ||
| ): | ||
| parent_class_errors["LoanSet:GracePeriod"] = ( | ||
| "Grace period must be less than the payment interval." | ||
| ) | ||
|
|
||
| return parent_class_errors |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden validation: amounts, rates, hex metadata, and signature consistency.
Add checks for documented bounds, principal string, and counterparty_signature coherence.
def _get_errors(self: Self) -> Dict[str, str]:
parent_class_errors = {
key: value
for key, value in {
**super()._get_errors(),
}.items()
if value is not None
}
if self.payment_interval is not None and self.payment_interval < 60:
parent_class_errors["LoanSet:PaymentInterval"] = (
"Payment interval must be at least 60 seconds."
)
if (
self.grace_period is not None
and self.payment_interval is not None
and self.grace_period > self.payment_interval
):
parent_class_errors["LoanSet:GracePeriod"] = (
"Grace period must be less than the payment interval."
)
+ # principal_requested must be a non-negative integer string
+ try:
+ if int(self.principal_requested) < 0:
+ parent_class_errors["LoanSet:PrincipalRequested"] = "Must be >= 0."
+ except (TypeError, ValueError):
+ parent_class_errors["LoanSet:PrincipalRequested"] = "Must be an integer string."
+
+ # Validate optional fees as non-negative
+ for field_name in ("loan_origination_fee", "loan_service_fee", "late_payment_fee", "close_payment_fee"):
+ val = getattr(self, field_name)
+ if val is not None and val < 0:
+ parent_class_errors[f"LoanSet:{field_name}"] = "Must be >= 0."
+
+ # Validate rate fields in 0..100000
+ for field_name in (
+ "overpayment_fee",
+ "interest_rate",
+ "late_interest_rate",
+ "close_interest_rate",
+ "overpayment_interest_rate",
+ ):
+ val = getattr(self, field_name)
+ if val is not None and not (0 <= val <= 100_000):
+ parent_class_errors[f"LoanSet:{field_name}"] = "Must be between 0 and 100000."
+
+ # data: hex and <= 256 bytes
+ if self.data is not None:
+ try:
+ b = bytes.fromhex(self.data)
+ except ValueError:
+ parent_class_errors["LoanSet:Data"] = "Must be hex-encoded."
+ else:
+ if len(b) > 256:
+ parent_class_errors["LoanSet:Data"] = "Must be <= 256 bytes."
+
+ # Counterparty signature coherence
+ if self.counterparty_signature is not None:
+ if self.counterparty is None:
+ parent_class_errors["LoanSet:Counterparty"] = "Required when counterparty_signature is provided."
+ if not self.counterparty_signature.signing_pub_key or not self.counterparty_signature.txn_signature:
+ parent_class_errors["LoanSet:CounterpartySignature"] = "signing_pub_key and txn_signature are required."
+
return parent_class_errors📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _get_errors(self: Self) -> Dict[str, str]: | |
| parent_class_errors = { | |
| key: value | |
| for key, value in { | |
| **super()._get_errors(), | |
| }.items() | |
| if value is not None | |
| } | |
| if self.payment_interval is not None and self.payment_interval < 60: | |
| parent_class_errors["LoanSet:PaymentInterval"] = ( | |
| "Payment interval must be at least 60 seconds." | |
| ) | |
| if ( | |
| self.grace_period is not None | |
| and self.payment_interval is not None | |
| and self.grace_period > self.payment_interval | |
| ): | |
| parent_class_errors["LoanSet:GracePeriod"] = ( | |
| "Grace period must be less than the payment interval." | |
| ) | |
| return parent_class_errors | |
| def _get_errors(self: Self) -> Dict[str, str]: | |
| parent_class_errors = { | |
| key: value | |
| for key, value in { | |
| **super()._get_errors(), | |
| }.items() | |
| if value is not None | |
| } | |
| if self.payment_interval is not None and self.payment_interval < 60: | |
| parent_class_errors["LoanSet:PaymentInterval"] = ( | |
| "Payment interval must be at least 60 seconds." | |
| ) | |
| if ( | |
| self.grace_period is not None | |
| and self.payment_interval is not None | |
| and self.grace_period > self.payment_interval | |
| ): | |
| parent_class_errors["LoanSet:GracePeriod"] = ( | |
| "Grace period must be less than the payment interval." | |
| ) | |
| # principal_requested must be a non-negative integer string | |
| try: | |
| if int(self.principal_requested) < 0: | |
| parent_class_errors["LoanSet:PrincipalRequested"] = "Must be >= 0." | |
| except (TypeError, ValueError): | |
| parent_class_errors["LoanSet:PrincipalRequested"] = "Must be an integer string." | |
| # Validate optional fees as non-negative | |
| for field_name in ("loan_origination_fee", "loan_service_fee", "late_payment_fee", "close_payment_fee"): | |
| val = getattr(self, field_name) | |
| if val is not None and val < 0: | |
| parent_class_errors[f"LoanSet:{field_name}"] = "Must be >= 0." | |
| # Validate rate fields in 0..100000 | |
| for field_name in ( | |
| "overpayment_fee", | |
| "interest_rate", | |
| "late_interest_rate", | |
| "close_interest_rate", | |
| "overpayment_interest_rate", | |
| ): | |
| val = getattr(self, field_name) | |
| if val is not None and not (0 <= val <= 100_000): | |
| parent_class_errors[f"LoanSet:{field_name}"] = "Must be between 0 and 100000." | |
| # data: hex and <= 256 bytes | |
| if self.data is not None: | |
| try: | |
| b = bytes.fromhex(self.data) | |
| except ValueError: | |
| parent_class_errors["LoanSet:Data"] = "Must be hex-encoded." | |
| else: | |
| if len(b) > 256: | |
| parent_class_errors["LoanSet:Data"] = "Must be <= 256 bytes." | |
| # Counterparty signature coherence | |
| if self.counterparty_signature is not None: | |
| if self.counterparty is None: | |
| parent_class_errors["LoanSet:Counterparty"] = "Required when counterparty_signature is provided." | |
| if not self.counterparty_signature.signing_pub_key or not self.counterparty_signature.txn_signature: | |
| parent_class_errors["LoanSet:CounterpartySignature"] = "signing_pub_key and txn_signature are required." | |
| return parent_class_errors |
🤖 Prompt for AI Agents
In xrpl/models/transactions/loan_set.py around lines 154-177, augment
_get_errors to harden validation: verify numeric amount fields (e.g., principal)
are strings that parse to non-negative integers and fall within documented
bounds; validate rate fields (e.g., interest_rate) are numeric and within
documented min/max bounds; ensure metadata fields that are supposed to be hex
(e.g., hex_metadata) match a hex regexp /^[0-9A-Fa-f]*$/; enforce that
counterparty and counterparty_signature are consistent (either both set or both
unset) and that counterparty_signature is a non-empty hex string; and keep
existing payment_interval/grace_period checks unchanged—return error messages
keyed similarly when any of these checks fail.
There was a problem hiding this comment.
All these validations are already implemented in rippled (The cpp implementation of the XRPL client).
Furthermore, we are still deliberating the exact JSON-type of these fields. Once we have clarity on the type of these fields, we can consider adding more validations for these fields.
There was a problem hiding this comment.
@ckeshava, understood! That makes perfect sense from an architectural perspective. If rippled already handles these validations at the protocol level, duplicating them in the Python client could indeed create unnecessary redundancy and potential inconsistencies.
I appreciate the context about the ongoing deliberations on JSON field types as well. It's wise to wait for that clarity before implementing additional validation logic that might need to change later.
Thank you for explaining how the validation responsibilities are distributed between rippled and the Python client implementations.
🐰✨
Learned something
new today! Thanks
for the context
about rippled vs
Python validation
responsibilities
🤓📚
✏️ Learnings added
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: xrpl/models/transactions/loan_set.py:158-181
Timestamp: 2025-09-15T15:54:25.813Z
Learning: In the xrpl-py codebase, validation logic for transaction fields is primarily handled by rippled (the C++ XRPL implementation) rather than being duplicated in the Python models. Python models should focus on basic structural validation rather than comprehensive field validation that rippled already performs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:78-84
Timestamp: 2024-12-11T01:47:28.074Z
Learning: In the `xrpl-py` codebase, within the models (e.g., in `xrpl/models/transactions/`), flag validation checks are not performed; flag checks are handled elsewhere outside of the models.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: tests/integration/transactions/test_lending_protocol.py:116-118
Timestamp: 2025-09-15T15:21:38.832Z
Learning: The start_date field in LoanSet transactions is scheduled for deprecation/removal in the next revision of the XLS-66d Lending Protocol specification, so timestamp format corrections are not needed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
xrpl/models/transactions/loan_pay.py (1)
25-34: Good: Amount typed correctly; default transaction_type set.Using Amount here aligns with other tx models and supports XRP/IOU/MPT; init=False default for LOAN_PAY is correct.
tests/integration/transactions/test_lending_protocol.py (2)
117-118: start_date: leave as-is per spec roadmap.Acknowledging your note: start_date is slated for deprecation/removal in the next XLS-66d revision; no epoch conversion requested.
138-149: Good: ledger_accept after direct submit avoids races.This resolves the flakiness risk after submit(); thanks for adding it.
xrpl/models/transactions/loan_set.py (1)
30-33: Signers type fixed — LGTM.CounterpartySignature.signers now uses Signer; aligns with Transaction.signers.
🧹 Nitpick comments (7)
xrpl/models/transactions/loan_pay.py (1)
3-3: Remove commented-out future import (noise).Either enable it consistently across models or drop the commented line. Prefer removal here.
Apply:
-# from __future__ import annotations # Requires Python 3.7+tests/integration/transactions/test_lending_protocol.py (2)
109-111: Fix typos in comment (and step numbering).“transered” → “transferred”; also Step-5 follows Step-3 — renumber for clarity.
Apply:
- # Step-5: The Loan Broker and Borrower create a Loan object with a LoanSet - # transaction and the requested principal (excluding fees) is transered to + # Step-4: The Loan Broker and Borrower create a Loan object with a LoanSet + # transaction and the requested principal (excluding fees) is transferred to
53-54: Use wallet.address consistently (minor consistency nit).Elsewhere in the test you use .address; prefer the same here unless there’s a reason to mix.
Apply:
- account=loan_issuer.classic_address, + account=loan_issuer.address,xrpl/models/transactions/loan_set.py (4)
25-33: Doc tweak: “Lender” → “counterparty (e.g., Borrower)”.Matches how the test uses borrower as the counterparty signer.
- An inner object that contains the signature of the Lender over the transaction. + An inner object that contains the signature of the counterparty (e.g., Borrower) + over the transaction.
108-118: Fix minor doc typos (“in in”) and tighten phrasing.Small polish to rate field docs.
- Annualized interest rate of the Loan in in 1/10th basis points. Valid values are + Annualized interest rate of the Loan in 1/10th basis points. Valid values are @@ - A premium added to the interest rate for late payments in in 1/10th basis points. + A premium added to the interest rate for late payments in 1/10th basis points. @@ - An interest rate charged on overpayments in 1/10th basis points. Valid values are + Interest charged on overpayments in 1/10th basis points. Valid values areAlso applies to: 120-124, 126-130
68-71: Add basic validation: hex data length, principal format, rates bounds, signature coherence.Strengthens model-level checks; mirrors patterns in other tx models.
class LoanSet(Transaction): @@ def _get_errors(self: Self) -> Dict[str, str]: parent_class_errors = { @@ if ( self.grace_period is not None and self.payment_interval is not None and self.grace_period > self.payment_interval ): parent_class_errors["LoanSet:GracePeriod"] = ( "Grace period must be less than the payment interval." ) + # principal_requested must be a non-negative integer string + try: + if int(self.principal_requested) < 0: + parent_class_errors["LoanSet:PrincipalRequested"] = "Must be >= 0." + except (TypeError, ValueError): + parent_class_errors["LoanSet:PrincipalRequested"] = "Must be an integer string." + + # Validate optional nominal fees as non-negative + for _f in ("loan_origination_fee", "loan_service_fee", "late_payment_fee", "close_payment_fee"): + _v = getattr(self, _f) + if _v is not None and _v < 0: + parent_class_errors[f"LoanSet:{_f}"] = "Must be >= 0." + + # Validate rate fields in 0..100000 (0–100% in 1/10 bp) + for _f in ("overpayment_fee","interest_rate","late_interest_rate","close_interest_rate","overpayment_interest_rate"): + _v = getattr(self, _f) + if _v is not None and not (0 <= _v <= 100_000): + parent_class_errors[f"LoanSet:{_f}"] = "Must be between 0 and 100000." + + # data must be hex and <= 256 bytes + if self.data is not None: + try: + _b = bytes.fromhex(self.data) + except ValueError: + parent_class_errors["LoanSet:Data"] = "Must be hex-encoded." + else: + if len(_b) > 256: + parent_class_errors["LoanSet:Data"] = "Must be <= 256 bytes." + + # Counterparty signature coherence + if self.counterparty_signature is not None: + if self.counterparty is None: + parent_class_errors["LoanSet:Counterparty"] = "Required when counterparty_signature is provided." + if not self.counterparty_signature.signing_pub_key or not self.counterparty_signature.txn_signature: + parent_class_errors["LoanSet:CounterpartySignature"] = "signing_pub_key and txn_signature are required." + return parent_class_errorsAlso applies to: 158-181
148-151: Clarify grace_period doc sentence.Current phrasing is awkward.
- The number of seconds after the Loan's Payment Due Date can be Defaulted. + The number of seconds after the payment due date before the Loan can be defaulted.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/integration/transactions/test_lending_protocol.py(1 hunks)xrpl/core/binarycodec/definitions/definitions.json(9 hunks)xrpl/models/transactions/loan_broker_cover_deposit.py(1 hunks)xrpl/models/transactions/loan_broker_cover_withdraw.py(1 hunks)xrpl/models/transactions/loan_pay.py(1 hunks)xrpl/models/transactions/loan_set.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- xrpl/models/transactions/loan_broker_cover_withdraw.py
- xrpl/core/binarycodec/definitions/definitions.json
- xrpl/models/transactions/loan_broker_cover_deposit.py
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: tests/integration/transactions/test_lending_protocol.py:116-118
Timestamp: 2025-09-15T15:21:38.804Z
Learning: The start_date field in LoanSet transactions is scheduled for deprecation/removal in the next revision of the XLS-66d Lending Protocol specification, so timestamp format corrections are not needed.
📚 Learning: 2024-10-30T20:34:35.451Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Applied to files:
xrpl/models/transactions/loan_pay.pyxrpl/models/transactions/loan_set.py
📚 Learning: 2024-12-10T20:35:09.762Z
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:70-76
Timestamp: 2024-12-10T20:35:09.762Z
Learning: In the `xrpl/models/transactions/mptoken_issuance_create.py` file, the `maximum_amount` field in the `MPTokenIssuanceCreate` class is a non-negative integer string and should be typed as `Optional[str]`.
Applied to files:
xrpl/models/transactions/loan_pay.py
📚 Learning: 2025-09-15T15:21:38.804Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: tests/integration/transactions/test_lending_protocol.py:116-118
Timestamp: 2025-09-15T15:21:38.804Z
Learning: The start_date field in LoanSet transactions is scheduled for deprecation/removal in the next revision of the XLS-66d Lending Protocol specification, so timestamp format corrections are not needed.
Applied to files:
tests/integration/transactions/test_lending_protocol.pyxrpl/models/transactions/loan_set.py
📚 Learning: 2025-01-15T00:41:02.631Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#773
File: tests/integration/reusable_values.py:112-119
Timestamp: 2025-01-15T00:41:02.631Z
Learning: In xrpl-py integration tests, errors during test setup should be allowed to terminate the test suite naturally to aid debugging, rather than being caught and wrapped in custom error handlers. This applies to functions like `sign_and_reliable_submission_async` which already have robust error handling.
Applied to files:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2024-10-30T20:44:27.753Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:89-93
Timestamp: 2024-10-30T20:44:27.753Z
Learning: In the Python file `xrpl/models/transactions/deposit_preauth.py`, within the `_validate_credentials_length` function, when validating `authorize_credentials` and `unauthorize_credentials`, it's acceptable to assign error messages to the same key in the `errors` dictionary if the conditions are mutually exclusive, as the error messages won't overwrite each other.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-10-30T20:45:02.301Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:54-64
Timestamp: 2024-10-30T20:45:02.301Z
Learning: In the `DepositPreauth` class in `xrpl/models/transactions/deposit_preauth.py`, when validating that exactly one of the parameters `authorize`, `unauthorize`, `authorize_credentials`, or `unauthorize_credentials` is set, ensure that any refactoring maintains logical equivalence with the original condition to prevent altering the intended functionality.
Applied to files:
xrpl/models/transactions/loan_set.py
🧬 Code graph analysis (3)
xrpl/models/transactions/loan_pay.py (4)
xrpl/core/binarycodec/types/amount.py (1)
Amount(296-456)xrpl/models/transactions/transaction.py (1)
Transaction(184-551)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)xrpl/models/utils.py (1)
require_kwargs_on_init(317-370)
tests/integration/transactions/test_lending_protocol.py (17)
tests/integration/integration_test_case.py (1)
IntegrationTestCase(9-18)tests/integration/it_utils.py (3)
fund_wallet_async(120-129)sign_and_reliable_submission_async(191-223)test_async_and_sync(268-363)xrpl/asyncio/transaction/main.py (3)
autofill_and_sign(127-153)submit(156-185)sign(81-124)xrpl/core/binarycodec/main.py (1)
encode_for_signing(37-52)xrpl/models/requests/account_objects.py (2)
AccountObjects(50-73)AccountObjectType(19-45)xrpl/models/transactions/account_set.py (2)
AccountSet(175-307)AccountSetAsfFlag(26-113)xrpl/models/transactions/loan_broker_set.py (1)
LoanBrokerSet(16-62)xrpl/models/transactions/loan_delete.py (1)
LoanDelete(15-27)xrpl/models/transactions/loan_manage.py (2)
LoanManage(51-63)LoanManageFlag(14-35)xrpl/models/transactions/loan_pay.py (1)
LoanPay(16-34)xrpl/models/transactions/loan_set.py (2)
LoanSet(60-181)CounterpartySignature(24-32)xrpl/models/transactions/transaction.py (2)
Transaction(184-551)to_xrpl(342-350)xrpl/models/transactions/vault_create.py (2)
VaultCreate(65-139)WithdrawalPolicy(56-60)xrpl/models/transactions/vault_deposit.py (1)
VaultDeposit(14-26)xrpl/models/currencies/xrp.py (1)
XRP(26-91)xrpl/models/response.py (2)
ResponseStatus(23-27)is_successful(74-82)xrpl/wallet/main.py (4)
Wallet(16-289)create(121-135)classic_address(34-41)address(24-29)
xrpl/models/transactions/loan_set.py (4)
xrpl/models/base_model.py (1)
BaseModel(93-418)xrpl/models/transactions/transaction.py (3)
Signer(131-161)Transaction(184-551)TransactionFlagInterface(173-179)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)xrpl/models/utils.py (1)
require_kwargs_on_init(317-370)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
tests/integration/transactions/test_lending_protocol.py (1)
178-182: Confirmed: Amount accepts string literals — no change required.xrpl-py’s Amount type alias includes str, and the docs/examples show transaction amounts provided as strings (so amount="100" in the test is valid). (xrpl-py.readthedocs.io)
|
The unit test violations are triggered by code which is un-modified by this PR. I will address them at a later date. |
|
|
||
| @require_kwargs_on_init | ||
| @dataclass(frozen=True, **KW_ONLY_DATACLASS) | ||
| class LoanBrokerSet(Transaction): |
There was a problem hiding this comment.
Should we check the following?
- Bounds for fields like CoverRateMinimum, CoverRateLiquidation, DebtMaximum, ManagementFeeRate, CoverRateLiquidation.
- Bounds for Data and make sure its content is HEX?
- Add unit tests for these checks?
There was a problem hiding this comment.
if the data is not in hex format, it will fail the transaction-encode step. It will not be encoded into binary format at all.
There was a problem hiding this comment.
updated in 029d65c
There was a problem hiding this comment.
if the data is not in hex format, it will fail the transaction-encode step. It will not be encoded into binary format at all.
I think it will throw the error dump to the user. Not sure if thats what we want the user to see. Just check with some non hex value or out of bound value in unit test case and if thats readable, its ok it skip this check at model level.
There was a problem hiding this comment.
I have added explicit hex validation in this commit: 4e5cf35
The error message should be readable now. I have also added unit tests in the binary-codec module.
| """ | ||
|
|
||
|
|
||
| class LoanSetFlagInterface(TransactionFlagInterface): |
There was a problem hiding this comment.
Once rippled codebase and spec solidifies, I think its good to add bound checks for other fields and their unit tests.
Snippet from xrpl.js implementation:
const MAX_DATA_LENGTH = 512
const MAX_OVER_PAYMENT_FEE_RATE = 100000
const MAX_INTEREST_RATE = 100000
const MAX_LATE_INTEREST_RATE = 100000
const MAX_CLOSE_INTEREST_RATE = 100000
const MAX_OVER_PAYMENT_INTEREST_RATE = 100000
const MIN_PAYMENT_INTERVAL = 60
| for raw_txn in batch.raw_transactions | ||
| ] | ||
| ) | ||
| elif transaction.transaction_type == TransactionType.LOAN_SET: |
There was a problem hiding this comment.
We should add unit/integration test to verify this logic.
There was a problem hiding this comment.
The existing integ test already uses this piece of code to automatically calculate the transaction fees
There was a problem hiding this comment.
The existing integ test already uses this piece of code to automatically calculate the transaction fees
I highly doubt that they will check TransactionType.LOAN_SET code path since its recently introduced. This is the most important part of this PR and getting the autofilled fee wrong will cause users to loose XRP. If it's a low hanging fruit we should test it. I recall there is a similar test for batch transaction. If it's not exported function maybe just assert the fee test in one of the integration test.
There was a problem hiding this comment.
Which transaction-type are you concerned about? The integration test invokes LoanBrokerSet, LoanSet and many other Lending Protocol related transactions. All these transactions are autofilled.
The Fees portion of the transaction is autofilled using the _calculate_fee_per_transaction_type method (which I linked in my previous msg)
All the integration tests already test this code-path because autofill method invokes _calculate_fee_per_transaction_type
There was a problem hiding this comment.
I am referring to a similar test like test_basic_calculate_fee_per_transaction_type, where you provide multiple signatures for LOAN_SET transaction and assert for the fee.
There was a problem hiding this comment.
Such a test is not feasible for the Lending Protocol spec. Please look at my note toward the end of this file.
The LoanSet transaction requires off-chain communication between the LoanIssuer and the Borrower. Whoever prepares the transaction needs to be aware of the Number-of-Signers on the counterparty account. The xrpl-py client library cannot autofill this value without reading the latest state of the counterparty account. However, this could increase the latency in submitting the LoanSet transaction.
I felt that it is easiest to let this field be populated by the LoanInitiator themselves.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
xrpl/models/transactions/loan_broker_set.py (5)
25-28: Field type is correct based on protocol requirements.The
loan_broker_idfield is properly typed asOptional[str]since it's used when modifying an existing loan broker but not required when creating a new one.
30-33: Verify hex format validation for the data field.The documentation states the field should be in hex format and limited to 256 bytes, but there's no validation logic implemented for these constraints.
Based on the retrieved learnings and the past review comment, this validation concern was previously raised. Please verify if validation should be added at the model level or handled elsewhere in the transaction processing pipeline:
#!/bin/bash # Search for hex validation patterns in the codebase rg -n "hex" --type=py -A3 -B3 # Search for data field validation in other transaction models rg -n "data.*Optional\[str\]" --type=py -A5 -B2
35-39: Verify bounds validation for management_fee_rate.The documentation specifies valid values are between 0 and 10000 inclusive, but no validation is implemented.
This mirrors the concern raised in past reviews about bounds checking. Please verify if this validation should be added:
#!/bin/bash # Search for similar fee rate validation patterns rg -n "fee_rate.*Optional\[int\]" --type=py -A5 -B2 # Look for existing rate validation in transaction models rg -n "10000" --type=py -A3 -B3
47-51: Verify bounds validation for cover_rate_minimum.The documentation specifies valid values are between 0 and 100000 inclusive, but no validation is implemented.
This is part of the broader validation concern raised in past reviews. Please verify the validation approach:
#!/bin/bash # Search for cover rate validation patterns rg -n "cover_rate.*Optional\[int\]" --type=py -A5 -B2 # Look for 100000 bound validation rg -n "100000" --type=py -A3 -B3
53-57: Verify bounds validation for cover_rate_liquidation.The documentation specifies valid values are between 0 and 100000 inclusive, but no validation is implemented.
This continues the pattern of missing bounds validation noted in past reviews.
🧹 Nitpick comments (4)
xrpl/models/transactions/loan_set.py (4)
24-33: Complete the CounterpartySignature docstring (and align wording).Docstring trails off and says “Lender” while the field is “counterparty”. Tighten it up for clarity.
class CounterpartySignature(BaseModel): - """ - An inner object that contains the signature of the Lender over the transaction. - The fields contained in this object are: - """ + """ + Signature payload supplied by the counterparty. + Fields: + - signing_pub_key: hex-encoded public key of the counterparty (required if txn_signature is set). + - txn_signature: hex-encoded signature over the canonical LoanSet transaction (required if signing_pub_key is set). + - signers: optional multisign array reusing the standard Signer objects. + """
108-112: Minor doc typos and consistency (“in in”, capitalization).Tidy repeated “in” and use consistent phrasing.
- Annualized interest rate of the Loan in in 1/10th basis points. Valid values are + Annualized interest rate of the loan in 1/10th basis points. Valid values are - A premium added to the interest rate for late payments in in 1/10th basis points. + A premium added to the interest rate for late payments in 1/10th basis points. - A Fee Rate charged for repaying the Loan early in 1/10th basis points. Valid values + A fee rate charged for repaying the loan early in 1/10th basis points. Valid valuesAlso applies to: 116-118, 122-124
137-138: start_date slated for deprecation — add note.Per current XLS‑66d discussions, add a brief deprecation note to help users.
start_date: int = REQUIRED + """ + [Deprecated (pending spec update)] Loan start timestamp (Ripple epoch seconds). + """
148-151: Grammar nit in grace_period doc.- The number of seconds after the Loan's Payment Due Date can be Defaulted. + Number of seconds after the payment due date after which the loan can be defaulted.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/unit/models/transactions/test_loan_broker_cover_clawback.py(1 hunks)xrpl/models/transactions/loan_broker_cover_clawback.py(1 hunks)xrpl/models/transactions/loan_broker_set.py(1 hunks)xrpl/models/transactions/loan_set.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/models/transactions/test_loan_broker_cover_clawback.py
- xrpl/models/transactions/loan_broker_cover_clawback.py
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: xrpl/models/transactions/loan_broker_cover_deposit.py:5-12
Timestamp: 2025-09-15T15:48:47.704Z
Learning: In the Lending Protocol (XLS-66d), LoanBrokerCoverDeposit allows depositing First-Loss Capital in XRP, IOU, or MPT assets, unlike LoanBrokerCoverClawback which rejects XRP amounts. These transactions have different validation requirements and should not be treated as having identical constraints.
📚 Learning: 2025-09-15T15:21:38.832Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: tests/integration/transactions/test_lending_protocol.py:116-118
Timestamp: 2025-09-15T15:21:38.832Z
Learning: The start_date field in LoanSet transactions is scheduled for deprecation/removal in the next revision of the XLS-66d Lending Protocol specification, so timestamp format corrections are not needed.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-10-30T20:34:35.451Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-10-30T20:44:27.753Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:89-93
Timestamp: 2024-10-30T20:44:27.753Z
Learning: In the Python file `xrpl/models/transactions/deposit_preauth.py`, within the `_validate_credentials_length` function, when validating `authorize_credentials` and `unauthorize_credentials`, it's acceptable to assign error messages to the same key in the `errors` dictionary if the conditions are mutually exclusive, as the error messages won't overwrite each other.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-10-30T20:45:02.301Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:54-64
Timestamp: 2024-10-30T20:45:02.301Z
Learning: In the `DepositPreauth` class in `xrpl/models/transactions/deposit_preauth.py`, when validating that exactly one of the parameters `authorize`, `unauthorize`, `authorize_credentials`, or `unauthorize_credentials` is set, ensure that any refactoring maintains logical equivalence with the original condition to prevent altering the intended functionality.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2025-09-15T15:54:25.813Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: xrpl/models/transactions/loan_set.py:158-181
Timestamp: 2025-09-15T15:54:25.813Z
Learning: In the xrpl-py codebase, validation logic for transaction fields is primarily handled by rippled (the C++ XRPL implementation) rather than being duplicated in the Python models. Python models should focus on basic structural validation rather than comprehensive field validation that rippled already performs.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-12-11T01:47:28.074Z
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:78-84
Timestamp: 2024-12-11T01:47:28.074Z
Learning: In the `xrpl-py` codebase, within the models (e.g., in `xrpl/models/transactions/`), flag validation checks are not performed; flag checks are handled elsewhere outside of the models.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-12-10T20:35:09.762Z
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:70-76
Timestamp: 2024-12-10T20:35:09.762Z
Learning: In the `xrpl/models/transactions/mptoken_issuance_create.py` file, the `maximum_amount` field in the `MPTokenIssuanceCreate` class is a non-negative integer string and should be typed as `Optional[str]`.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2025-02-11T21:11:19.326Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#772
File: tools/generate_definitions.py:0-0
Timestamp: 2025-02-11T21:11:19.326Z
Learning: In XRPL's binary codec format, large unsigned integers (UINT128, UINT160, UINT192, UINT256) should be consistently treated as Hash types, while smaller UINTs are translated to UInt types. This is reflected in generate_tx_models.py where they are mapped to "str" type.
Applied to files:
xrpl/models/transactions/loan_set.py
🧬 Code graph analysis (2)
xrpl/models/transactions/loan_set.py (4)
xrpl/models/base_model.py (1)
BaseModel(93-418)xrpl/models/transactions/transaction.py (3)
Signer(131-161)Transaction(184-549)TransactionFlagInterface(173-179)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)xrpl/models/utils.py (1)
require_kwargs_on_init(317-370)
xrpl/models/transactions/loan_broker_set.py (3)
xrpl/models/transactions/transaction.py (1)
Transaction(184-549)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)xrpl/models/utils.py (1)
require_kwargs_on_init(317-370)
🔇 Additional comments (9)
xrpl/models/transactions/loan_set.py (4)
30-33: Good fix: Signer typing for nested signers.Using List[Signer] here matches Transaction.signers and avoids stringly-typed signers.
35-56: Verify flag plumbing (enum/interface → Flags int).Please confirm LoanSetFlag/LoanSetFlagInterface are actually picked up by Transaction._flags_to_int for to_dict()/to_xrpl, e.g., that {"TF_LOAN_OVER_PAYMENT": True} yields 0x00010000 and round-trips back.
68-72: Data length: 256 vs 512 bytes — confirm spec and align doc.Comment says 256 bytes; xrpl.js reference in a prior discussion used 512. Confirm XLS‑66d and update the docstring (and tests) accordingly.
- Arbitrary metadata in hex format. The field is limited to 256 bytes. + Arbitrary metadata in hex format. The field is limited to 512 bytes.
158-181: Validation scope looks right-sized.The minimal timing checks align with xrpl-py’s pattern of deferring deeper validation to rippled.
xrpl/models/transactions/loan_broker_set.py (5)
1-12: LGTM! Clean imports and proper structure.The file structure follows the established patterns with appropriate imports and future annotations.
14-17: LGTM! Proper dataclass configuration for transaction model.The use of
@require_kwargs_on_init,frozen=True, andKW_ONLY_DATACLASSfollows the established patterns for transaction models in this codebase.
19-23: LGTM! Required field properly defined.The
vault_idfield is correctly marked as required with appropriate documentation explaining its purpose in the Lending Protocol.
41-45: LGTM! Field type change aligns with protocol requirements.The change from
inttoOptional[str]fordebt_maximumis appropriate as it allows for larger values that might exceed integer limits and aligns with the protocol's handling of monetary amounts as strings.
59-62: LGTM! Transaction type field properly configured.The
transaction_typefield is correctly set withinit=Falseand the appropriate default value, following the established pattern from the base Transaction class.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tests/unit/models/transactions/test_loan_broker_set.py (3)
12-22: Clarify limit wording: “at most 256 bytes” (and align test with model).The message “less than 256 bytes” suggests 255 is max, while code allows 256. Recommend “at most 256 bytes” and update expectation here to match the model change.
- "{'LoanBrokerSet:data': 'Data must be less than 256 bytes.'}", + "{'LoanBrokerSet:data': 'Data must be at most 256 bytes.'}",
103-115: Add test for non‑numeric debt_maximum to guard against ValueError surfaced from model.Currently, passing a non‑numeric string will raise ValueError inside _get_errors. Add a unit test to assert a clean XRPLModelException with a specific message once the model fix is applied.
@@ def test_invalid_debt_maximum_too_low(self): with self.assertRaises(XRPLModelException) as error: LoanBrokerSet( account=_SOURCE, vault_id=_VAULT_ID, debt_maximum="-1", ) self.assertEqual( error.exception.args[0], "{'LoanBrokerSet:debt_maximum': 'Debt maximum must not be negative or" + " greater than 9223372036854775807.'}", ) + def test_invalid_debt_maximum_non_numeric(self): + with self.assertRaises(XRPLModelException) as error: + LoanBrokerSet( + account=_SOURCE, + vault_id=_VAULT_ID, + debt_maximum="notanumber", + ) + self.assertEqual( + error.exception.args[0], + "{'LoanBrokerSet:debt_maximum': 'Debt maximum must be a base-10 integer string.'}", + ) +
24-50: Consider parameterizing the range tests to reduce duplication and improve coverage of edge values.Use subTest or parameterization to DRY these near-identical blocks and add exact-boundary success cases (0, max). Keeps tests compact and comprehensive.
+ def test_boundary_values_valid(self): + # Data payload exactly 256 bytes (512 hex chars) + ok_data = "A" * (256 * 2) + tx = LoanBrokerSet( + account=_SOURCE, + vault_id=_VAULT_ID, + data=ok_data, + management_fee_rate=0, + cover_rate_minimum=0, + cover_rate_liquidation=100_000, + debt_maximum="9223372036854775807", + ) + self.assertTrue(tx.is_valid())Also applies to: 51-76, 77-102, 116-129
xrpl/models/transactions/loan_broker_set.py (3)
32-35: Fix limit wording to match implementation: allow 256 bytes exactly.Change docstring and error message to “at most 256 bytes” for clarity and consistency with the inclusive check.
@@ - Arbitrary metadata in hex format. The field is limited to 256 bytes. + Arbitrary metadata in hex format. The field is limited to at most 256 bytes. @@ - errors["LoanBrokerSet:data"] = "Data must be less than 256 bytes." + errors["LoanBrokerSet:data"] = "Data must be at most 256 bytes."Also applies to: 75-77
37-41: Minor doc/drift: use consistent numeral style in docs to match messages/constants.Prefer the same underscores as in constants/messages for readability: 10_000 and 100_000.
- Valid values are between 0 and 10000 inclusive. + Valid values are between 0 and 10_000 inclusive. @@ - Valid values are between 0 and 100000 inclusive. + Valid values are between 0 and 100_000 inclusive. @@ - Loan default. Valid values are between 0 and 100000 inclusive. + Loan default. Valid values are between 0 and 100_000 inclusive.Also applies to: 49-53, 55-59, 66-71
32-35: Optional: early hex/length sanity for data to improve error locality.Although the binary encoder will reject non‑hex or odd‑length strings, adding a quick regex/even‑length check here would surface clearer, earlier errors to users.
Do you want a short follow-up patch to validate: even length, only [0-9A-Fa-f], and that len(data) % 2 == 0 when provided?
Also applies to: 66-71, 72-108
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unit/models/transactions/test_loan_broker_set.py(1 hunks)xrpl/models/transactions/loan_broker_set.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: xrpl/models/transactions/loan_broker_cover_deposit.py:5-12
Timestamp: 2025-09-15T15:48:47.704Z
Learning: In the Lending Protocol (XLS-66d), LoanBrokerCoverDeposit allows depositing First-Loss Capital in XRP, IOU, or MPT assets, unlike LoanBrokerCoverClawback which rejects XRP amounts. These transactions have different validation requirements and should not be treated as having identical constraints.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: tests/integration/transactions/test_lending_protocol.py:116-118
Timestamp: 2025-09-15T15:21:38.832Z
Learning: The start_date field in LoanSet transactions is scheduled for deprecation/removal in the next revision of the XLS-66d Lending Protocol specification, so timestamp format corrections are not needed.
🧬 Code graph analysis (2)
tests/unit/models/transactions/test_loan_broker_set.py (3)
xrpl/models/exceptions.py (1)
XRPLModelException(6-9)xrpl/models/transactions/loan_broker_set.py (1)
LoanBrokerSet(18-108)xrpl/models/base_model.py (1)
is_valid(295-302)
xrpl/models/transactions/loan_broker_set.py (3)
xrpl/models/transactions/transaction.py (1)
Transaction(184-549)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)xrpl/models/utils.py (1)
require_kwargs_on_init(317-370)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
tests/unit/models/transactions/test_loan_broker_set.py (1)
130-135: LGTM for minimal valid construction.Happy path looks good.
xrpl/models/transactions/loan_broker_set.py (2)
21-27: Confirm semantic of debt_maximum=0 vs omitted.Doc says “default value of 0 means no limit.” If omitted, is behavior identical to explicitly “0”? If not, consider clarifying.
Also applies to: 43-48
1-19: Overall: solid model with clear bounds and kw‑only construction.Structure and validations look good; nice use of require_kwargs_on_init and frozen dataclass.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tests/unit/models/transactions/test_loan_set.py (1)
207-214: Add boundary-success tests (optional).Consider adding passing tests for exact boundaries:
- overpayment_fee, interest_rate, late_interest_rate, close_interest_rate, overpayment_interest_rate at 0 and 100000
- payment_interval exactly 60
Improves guard against accidental off‑by‑one regressions.
xrpl/models/transactions/loan_set.py (3)
110-112: Fix minor doc typos.Remove duplicate “in” and normalize capitalization.
Apply this diff:
- Annualized interest rate of the Loan in in 1/10th basis points. Valid values are + Annualized interest rate of the Loan in 1/10th basis points. Valid values are @@ - A premium added to the interest rate for late payments in in 1/10th basis points. + A premium added to the interest rate for late payments in 1/10th basis points. @@ - A Fee Rate charged for repaying the Loan early in 1/10th basis points. Valid values + A fee rate charged for repaying the Loan early in 1/10th basis points. Valid valuesAlso applies to: 116-118, 122-124
25-33: Tighten CounterpartySignature docstring.Current text trails off. Clarify the fields succinctly.
Apply this diff:
- """ - An inner object that contains the signature of the Lender over the transaction. - The fields contained in this object are: - """ + """ + Inner object containing the Lender’s signature over the transaction. + Fields: signing_pub_key, txn_signature, and signers (for multisign). + """
3-3: Drop outdated version note.“Requires Python 3.7+” is unnecessary here; the project’s supported versions govern this.
Apply this diff:
-from __future__ import annotations # Requires Python 3.7+ +from __future__ import annotations
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/unit/models/transactions/test_loan_broker_set.py(1 hunks)tests/unit/models/transactions/test_loan_set.py(1 hunks)xrpl/models/transactions/loan_broker_set.py(1 hunks)xrpl/models/transactions/loan_set.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- xrpl/models/transactions/loan_broker_set.py
- tests/unit/models/transactions/test_loan_broker_set.py
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: xrpl/models/transactions/loan_broker_cover_deposit.py:5-12
Timestamp: 2025-09-15T15:48:47.704Z
Learning: In the Lending Protocol (XLS-66d), LoanBrokerCoverDeposit allows depositing First-Loss Capital in XRP, IOU, or MPT assets, unlike LoanBrokerCoverClawback which rejects XRP amounts. These transactions have different validation requirements and should not be treated as having identical constraints.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: tests/integration/transactions/test_lending_protocol.py:116-118
Timestamp: 2025-09-15T15:21:38.832Z
Learning: The start_date field in LoanSet transactions is scheduled for deprecation/removal in the next revision of the XLS-66d Lending Protocol specification, so timestamp format corrections are not needed.
📚 Learning: 2025-09-15T15:21:38.832Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: tests/integration/transactions/test_lending_protocol.py:116-118
Timestamp: 2025-09-15T15:21:38.832Z
Learning: The start_date field in LoanSet transactions is scheduled for deprecation/removal in the next revision of the XLS-66d Lending Protocol specification, so timestamp format corrections are not needed.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-10-30T20:34:35.451Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-10-30T20:44:27.753Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:89-93
Timestamp: 2024-10-30T20:44:27.753Z
Learning: In the Python file `xrpl/models/transactions/deposit_preauth.py`, within the `_validate_credentials_length` function, when validating `authorize_credentials` and `unauthorize_credentials`, it's acceptable to assign error messages to the same key in the `errors` dictionary if the conditions are mutually exclusive, as the error messages won't overwrite each other.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-10-30T20:45:02.301Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:54-64
Timestamp: 2024-10-30T20:45:02.301Z
Learning: In the `DepositPreauth` class in `xrpl/models/transactions/deposit_preauth.py`, when validating that exactly one of the parameters `authorize`, `unauthorize`, `authorize_credentials`, or `unauthorize_credentials` is set, ensure that any refactoring maintains logical equivalence with the original condition to prevent altering the intended functionality.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2025-09-15T15:54:25.813Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: xrpl/models/transactions/loan_set.py:158-181
Timestamp: 2025-09-15T15:54:25.813Z
Learning: In the xrpl-py codebase, validation logic for transaction fields is primarily handled by rippled (the C++ XRPL implementation) rather than being duplicated in the Python models. Python models should focus on basic structural validation rather than comprehensive field validation that rippled already performs.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-12-11T01:47:28.074Z
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:78-84
Timestamp: 2024-12-11T01:47:28.074Z
Learning: In the `xrpl-py` codebase, within the models (e.g., in `xrpl/models/transactions/`), flag validation checks are not performed; flag checks are handled elsewhere outside of the models.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-12-10T20:35:09.762Z
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:70-76
Timestamp: 2024-12-10T20:35:09.762Z
Learning: In the `xrpl/models/transactions/mptoken_issuance_create.py` file, the `maximum_amount` field in the `MPTokenIssuanceCreate` class is a non-negative integer string and should be typed as `Optional[str]`.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2025-02-11T21:11:19.326Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#772
File: tools/generate_definitions.py:0-0
Timestamp: 2025-02-11T21:11:19.326Z
Learning: In XRPL's binary codec format, large unsigned integers (UINT128, UINT160, UINT192, UINT256) should be consistently treated as Hash types, while smaller UINTs are translated to UInt types. This is reflected in generate_tx_models.py where they are mapped to "str" type.
Applied to files:
xrpl/models/transactions/loan_set.py
🧬 Code graph analysis (2)
tests/unit/models/transactions/test_loan_set.py (3)
xrpl/models/exceptions.py (1)
XRPLModelException(6-9)xrpl/models/transactions/loan_set.py (1)
LoanSet(60-234)xrpl/models/base_model.py (1)
is_valid(295-302)
xrpl/models/transactions/loan_set.py (6)
xrpl/models/base_model.py (1)
BaseModel(93-418)xrpl/models/transactions/transaction.py (3)
Signer(131-161)Transaction(184-549)TransactionFlagInterface(173-179)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)xrpl/models/utils.py (1)
require_kwargs_on_init(317-370)xrpl/models/transactions/loan_broker_set.py (1)
_get_errors(72-108)xrpl/models/transactions/loan_broker_cover_clawback.py (1)
_get_errors(44-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (4)
tests/unit/models/transactions/test_loan_set.py (3)
26-40: LGTM on bounds tests for fee and rate fields.Good coverage for lower/upper bounds and messages; matches constants in the model.
Also applies to: 41-70, 71-100, 101-130, 131-160, 161-175
176-191: LGTM on grace vs interval constraint.Covers the “grace_period < payment_interval” rule with a clear failure message.
192-206: LGTM on minimum payment interval.Validates the 60‑second minimum with an explicit message.
xrpl/models/transactions/loan_set.py (1)
137-137: Track start_date deprecation (per XLS‑66d notes).start_date is slated for removal in the next spec revision; consider a TODO to revisit required-ness once the spec merges, and prepare tests accordingly.
Per retrieved learnings for this PR, no format changes needed now; just ensure we update when the spec lands.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
xrpl/models/transactions/loan_set.py (1)
69-72: Align hex/byte limit docs and message.Same limit as above: 256 bytes = 512 hex chars. Make the wording explicit and consistent.
@@ - """ - Arbitrary metadata in hex format. The field is limited to 256 bytes. - """ + """ + Arbitrary metadata as a hex string. Maximum 256 bytes (512 hex characters). + """ @@ - MAX_DATA_LENGTH = 256 * 2 + MAX_DATA_LENGTH = 256 * 2 # 512 hex characters (256 bytes) @@ - if self.data is not None and len(self.data) > self.MAX_DATA_LENGTH: - parent_class_errors["LoanSet:data"] = "Data must be less than 256 bytes." + if self.data is not None and len(self.data) > self.MAX_DATA_LENGTH: + parent_class_errors["LoanSet:data"] = ( + "Data must be at most 512 hex characters (256 bytes)." + )Also applies to: 159-166, 176-181
xrpl/models/transactions/loan_broker_set.py (1)
105-111: Robustly parse debt_maximum; don’t raise ValueError from _get_errors.int(self.debt_maximum) can raise and bypass normal aggregation. Safely parse and emit a deterministic field error.
- if self.debt_maximum is not None and ( - int(self.debt_maximum) < 0 or int(self.debt_maximum) > self.MAX_DEBT_MAXIMUM - ): - errors["LoanBrokerSet:debt_maximum"] = ( - "Debt maximum must not be negative or greater than 9223372036854775807." - ) + if self.debt_maximum is not None: + try: + debt_max = int(self.debt_maximum) + except (TypeError, ValueError): + errors["LoanBrokerSet:debt_maximum"] = ( + "Debt maximum must be a base-10 integer string." + ) + else: + if debt_max < 0 or debt_max > self.MAX_DEBT_MAXIMUM: + errors["LoanBrokerSet:debt_maximum"] = ( + "Debt maximum must not be negative or greater than 9223372036854775807." + )
🧹 Nitpick comments (7)
xrpl/models/transactions/loan_broker_set.py (2)
33-37: Hex length vs bytes: fix wording and keep limit unambiguous.Data is a hex string; 256 bytes = 512 hex chars. Keep the check as-is but update doc/message to “at most 512 hex characters (256 bytes)”.
@@ - """ - Arbitrary metadata in hex format. The field is limited to 256 bytes. - """ + """ + Arbitrary metadata as a hex string. Maximum 256 bytes (512 hex characters). + """ @@ - if self.data is not None and len(self.data) > self.MAX_DATA_PAYLOAD_LENGTH: - errors["LoanBrokerSet:data"] = "Data must be less than 256 bytes." + if self.data is not None and len(self.data) > self.MAX_DATA_PAYLOAD_LENGTH: + errors["LoanBrokerSet:data"] = ( + "Data must be at most 512 hex characters (256 bytes)." + )Also applies to: 76-81
82-89: Unify numeric formatting in messages (avoid underscores).Other messages use 100000; make these consistent for polish.
@@ - errors["LoanBrokerSet:management_fee_rate"] = ( - "Management fee rate must be between 0 and 10_000 inclusive." - ) + errors["LoanBrokerSet:management_fee_rate"] = ( + "Management fee rate must be between 0 and 10000 inclusive." + ) @@ - errors["LoanBrokerSet:cover_rate_minimum"] = ( - "Cover rate minimum must be between 0 and 100_000 inclusive." - ) + errors["LoanBrokerSet:cover_rate_minimum"] = ( + "Cover rate minimum must be between 0 and 100000 inclusive." + ) @@ - errors["LoanBrokerSet:cover_rate_liquidation"] = ( - "Cover rate liquidation must be between 0 and 100_000 inclusive." - ) + errors["LoanBrokerSet:cover_rate_liquidation"] = ( + "Cover rate liquidation must be between 0 and 100000 inclusive." + )Also applies to: 90-97, 98-104
xrpl/models/transactions/loan_set.py (2)
26-33: Tighten CounterpartySignature docstring.Remove trailing sentence fragment; it promises a list that isn’t present.
- """ - An inner object that contains the signature of the Lender over the transaction. - The fields contained in this object are: - """ + """ + Inner object containing the lender’s signature over the transaction. + """
110-113: Grammar/wording nits in docstrings.
- “in in” duplicated.
- “Fee Rate” capitalization.
- Awkward sentence about default timing.
@@ - Annualized interest rate of the Loan in in 1/10th basis points. Valid values are - between 0 and 100000 inclusive. (0 - 100%) + Annualized interest rate of the loan in 1/10th basis points. Valid values are + between 0 and 100000 inclusive (0–100%). @@ - A premium added to the interest rate for late payments in in 1/10th basis points. - Valid values are between 0 and 100000 inclusive. (0 - 100%) + A premium added to the interest rate for late payments, in 1/10th basis points. + Valid values are between 0 and 100000 inclusive (0–100%). @@ - A Fee Rate charged for repaying the Loan early in 1/10th basis points. Valid values - are between 0 and 100000 inclusive. (0 - 100%) + A fee rate charged for repaying the loan early, in 1/10th basis points. Valid values + are between 0 and 100000 inclusive (0–100%). @@ - The number of seconds after the Loan's Payment Due Date can be Defaulted. + Number of seconds after the loan’s payment due date after which it can be defaulted.Also applies to: 116-119, 122-125, 150-152
tests/unit/models/transactions/test_loan_set.py (3)
21-24: Update expected message to match clarified hex/byte wording.If you adopt the model change, adjust the test expectation accordingly.
self.assertEqual( error.exception.args[0], - "{'LoanSet:data': 'Data must be less than 256 bytes.'}", + "{'LoanSet:data': 'Data must be at most 512 hex characters (256 bytes).'}", )
12-20: Optional: add boundary test for exactly-256-byte payload.Prevents regressions around off‑by‑one.
@@ class TestLoanSet(TestCase): def test_invalid_data_too_long(self): with self.assertRaises(XRPLModelException) as error: LoanSet( account=_SOURCE, loan_broker_id=_ISSUER, principal_requested="100000000", start_date=int(datetime.datetime.now().timestamp()), data="A" * 257 * 2, ) self.assertEqual( error.exception.args[0], "{'LoanSet:data': 'Data must be less than 256 bytes.'}", ) + + def test_data_at_boundary_is_allowed(self): + tx = LoanSet( + account=_SOURCE, + loan_broker_id=_ISSUER, + principal_requested="100000000", + start_date=int(datetime.datetime.now().timestamp()), + data="A" * 512, # 512 hex chars = 256 bytes + ) + self.assertTrue(tx.is_valid())
14-19: Tiny DRY: reuse a computed timestamp.Not required, but factoring a local ts var per test reduces duplication noise.
Also applies to: 28-34, 42-48, 57-63, 71-78, 86-93, 101-108, 116-123, 131-138, 146-153, 161-168, 176-183, 192-199, 206-214, 221-227
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/unit/core/binarycodec/types/test_blob.py(1 hunks)tests/unit/models/transactions/test_loan_broker_set.py(1 hunks)tests/unit/models/transactions/test_loan_set.py(1 hunks)xrpl/models/transactions/loan_broker_set.py(1 hunks)xrpl/models/transactions/loan_set.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/models/transactions/test_loan_broker_set.py
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: tests/integration/transactions/test_lending_protocol.py:116-118
Timestamp: 2025-09-15T15:21:38.832Z
Learning: The start_date field in LoanSet transactions is scheduled for deprecation/removal in the next revision of the XLS-66d Lending Protocol specification, so timestamp format corrections are not needed.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: xrpl/models/transactions/loan_broker_cover_deposit.py:5-12
Timestamp: 2025-09-15T15:48:47.704Z
Learning: In the Lending Protocol (XLS-66d), LoanBrokerCoverDeposit allows depositing First-Loss Capital in XRP, IOU, or MPT assets, unlike LoanBrokerCoverClawback which rejects XRP amounts. These transactions have different validation requirements and should not be treated as having identical constraints.
📚 Learning: 2024-10-30T20:34:35.451Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-10-30T20:44:27.753Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:89-93
Timestamp: 2024-10-30T20:44:27.753Z
Learning: In the Python file `xrpl/models/transactions/deposit_preauth.py`, within the `_validate_credentials_length` function, when validating `authorize_credentials` and `unauthorize_credentials`, it's acceptable to assign error messages to the same key in the `errors` dictionary if the conditions are mutually exclusive, as the error messages won't overwrite each other.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-10-30T20:45:02.301Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:54-64
Timestamp: 2024-10-30T20:45:02.301Z
Learning: In the `DepositPreauth` class in `xrpl/models/transactions/deposit_preauth.py`, when validating that exactly one of the parameters `authorize`, `unauthorize`, `authorize_credentials`, or `unauthorize_credentials` is set, ensure that any refactoring maintains logical equivalence with the original condition to prevent altering the intended functionality.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2025-09-15T15:54:25.813Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: xrpl/models/transactions/loan_set.py:158-181
Timestamp: 2025-09-15T15:54:25.813Z
Learning: In the xrpl-py codebase, validation logic for transaction fields is primarily handled by rippled (the C++ XRPL implementation) rather than being duplicated in the Python models. Python models should focus on basic structural validation rather than comprehensive field validation that rippled already performs.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-12-11T01:47:28.074Z
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:78-84
Timestamp: 2024-12-11T01:47:28.074Z
Learning: In the `xrpl-py` codebase, within the models (e.g., in `xrpl/models/transactions/`), flag validation checks are not performed; flag checks are handled elsewhere outside of the models.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-12-10T20:35:09.762Z
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:70-76
Timestamp: 2024-12-10T20:35:09.762Z
Learning: In the `xrpl/models/transactions/mptoken_issuance_create.py` file, the `maximum_amount` field in the `MPTokenIssuanceCreate` class is a non-negative integer string and should be typed as `Optional[str]`.
Applied to files:
xrpl/models/transactions/loan_set.pyxrpl/models/transactions/loan_broker_set.py
📚 Learning: 2025-02-11T21:11:19.326Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#772
File: tools/generate_definitions.py:0-0
Timestamp: 2025-02-11T21:11:19.326Z
Learning: In XRPL's binary codec format, large unsigned integers (UINT128, UINT160, UINT192, UINT256) should be consistently treated as Hash types, while smaller UINTs are translated to UInt types. This is reflected in generate_tx_models.py where they are mapped to "str" type.
Applied to files:
xrpl/models/transactions/loan_set.py
🧬 Code graph analysis (4)
tests/unit/core/binarycodec/types/test_blob.py (1)
xrpl/core/binarycodec/types/blob.py (1)
Blob(17-64)
tests/unit/models/transactions/test_loan_set.py (3)
xrpl/models/exceptions.py (1)
XRPLModelException(6-9)xrpl/models/transactions/loan_set.py (1)
LoanSet(61-238)xrpl/models/base_model.py (1)
is_valid(295-302)
xrpl/models/transactions/loan_set.py (5)
xrpl/models/base_model.py (1)
BaseModel(93-418)xrpl/models/transactions/transaction.py (3)
Signer(131-161)Transaction(184-549)TransactionFlagInterface(173-179)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)xrpl/models/utils.py (1)
require_kwargs_on_init(317-370)xrpl/models/transactions/loan_broker_set.py (1)
_get_errors(73-112)
xrpl/models/transactions/loan_broker_set.py (3)
xrpl/models/transactions/transaction.py (1)
Transaction(184-549)xrpl/models/transactions/loan_set.py (1)
_get_errors(167-238)xrpl/models/transactions/loan_broker_cover_clawback.py (1)
_get_errors(44-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
xrpl/models/transactions/loan_set.py (1)
138-139: start_date slated for deprecation in next XLS-66d rev—track this.Consider a TODO/deprecation note to ease migration when the spec lands.
Would you like a PR follow-up that adds a deprecation note and a unit test guarding current behavior until the spec updates?
| def test_raises_invalid_non_hex_input(self): | ||
| invalid_value = "Z" | ||
| self.assertRaises(ValueError, Blob.from_value, invalid_value) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Align exception contract for Blob.from_value (docstring vs behavior).
This test asserts ValueError, which matches current behavior (bytes.fromhex raises ValueError), but contradicts Blob.from_value’s docstring that promises XRPLBinaryCodecException for unconstructible values. Please choose and align the contract:
Option A — normalize on XRPLBinaryCodecException (recommended for SDK consistency):
- Wrap ValueError in Blob.from_value and update this test to expect XRPLBinaryCodecException.
*** xrpl/core/binarycodec/types/blob.py
@@
- if isinstance(value, str):
- return cls(bytes.fromhex(value))
+ if isinstance(value, str):
+ try:
+ return cls(bytes.fromhex(value))
+ except ValueError as e:
+ raise XRPLBinaryCodecException(
+ f"Invalid hex string to construct a Blob: {value!r}"
+ ) from e*** tests/unit/core/binarycodec/types/test_blob.py
@@
- invalid_value = "Z"
- self.assertRaises(ValueError, Blob.from_value, invalid_value)
+ invalid_value = "Z"
+ self.assertRaises(XRPLBinaryCodecException, Blob.from_value, invalid_value)Option B — keep ValueError and update docstring to reflect it:
*** xrpl/core/binarycodec/types/blob.py
@@ def from_value(cls: Type[Self], value: str) -> Self:
- Raises:
- XRPLBinaryCodecException: If the Blob can't be constructed from value.
+ Raises:
+ XRPLBinaryCodecException: If value is not a str.
+ ValueError: If value is a str but not valid hex for bytes.fromhex.Nit: prefer the context-manager form for readability.
- self.assertRaises(ValueError, Blob.from_value, invalid_value)
+ with self.assertRaises(ValueError):
+ Blob.from_value(invalid_value)Run to check for call sites/tests relying on either exception:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Blob.from_value call sites:"
rg -n --type=python -C2 'Blob\.from_value\(' || true
echo
echo "Catch sites for XRPLBinaryCodecException around Blob/fromhex:"
rg -n --type=python -C2 'except\s+XRPLBinaryCodecException' || true
echo
echo "Catch sites for ValueError around Blob/fromhex:"
rg -n --type=python -C2 'except\s+ValueError' | rg -n -C1 'Blob|fromhex' || trueLength of output: 232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Blob.from_value call sites:"
rg -n -S 'Blob\.from_value\(' || true
echo
echo "2) Find Blob class definition(s):"
rg -n -S 'class\s+Blob\b' || true
echo
echo "3) Find 'def from_value' definitions:"
rg -n -S 'def\s+from_value\(' || true
echo
echo "4) Show file where 'class Blob' is defined (first match) and print its from_value method (if present):"
blob_file=$(rg -n -S 'class\s+Blob\b' | sed -n '1s/:.*//p' || true)
if [ -n "$blob_file" ]; then
echo "Blob file: $blob_file"
echo "---- from_value method excerpt ----"
# Print the from_value method (approximate) with context
rg -n -S 'def\s+from_value\(' "$blob_file" -n -C5 || true
echo "-----------------------------------"
else
echo "Blob class file not found"
fi
echo
echo "5) Search for except XRPLBinaryCodecException and except ValueError occurrences (show context):"
rg -n -S 'except\s+XRPLBinaryCodecException' -C2 || true
echo
rg -n -S 'except\s+ValueError' -C2 || true
echo
echo "6) List tests mentioning Blob or Blob.from_value:"
rg -n -S 'Blob' tests || true
rg -n -S 'Blob\.from_value\(' tests || trueLength of output: 6804
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== xrpl/core/binarycodec/types/blob.py ==="
sed -n '1,240p' xrpl/core/binarycodec/types/blob.py || true
echo
echo "=== tests/unit/core/binarycodec/types/test_blob.py ==="
sed -n '1,240p' tests/unit/core/binarycodec/types/test_blob.py || trueLength of output: 2792
Normalize Blob.from_value to raise XRPLBinaryCodecException for invalid hex input (recommended)
Wrap bytes.fromhex in Blob.from_value to convert ValueError into XRPLBinaryCodecException and update the failing test to expect that; prefer the context-manager form for assertRaises.
*** xrpl/core/binarycodec/types/blob.py
@@
- if isinstance(value, str):
- return cls(bytes.fromhex(value))
+ if isinstance(value, str):
+ try:
+ return cls(bytes.fromhex(value))
+ except ValueError as e:
+ raise XRPLBinaryCodecException(
+ f"Invalid hex string to construct a Blob: {value!r}"
+ ) from e*** tests/unit/core/binarycodec/types/test_blob.py
@@
- invalid_value = "Z"
- self.assertRaises(ValueError, Blob.from_value, invalid_value)
+ invalid_value = "Z"
+ with self.assertRaises(XRPLBinaryCodecException):
+ Blob.from_value(invalid_value)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_raises_invalid_non_hex_input(self): | |
| invalid_value = "Z" | |
| self.assertRaises(ValueError, Blob.from_value, invalid_value) | |
| # tests/unit/core/binarycodec/types/test_blob.py | |
| def test_raises_invalid_non_hex_input(self): | |
| invalid_value = "Z" | |
| with self.assertRaises(XRPLBinaryCodecException): | |
| Blob.from_value(invalid_value) |
| def test_raises_invalid_non_hex_input(self): | |
| invalid_value = "Z" | |
| self.assertRaises(ValueError, Blob.from_value, invalid_value) | |
| # xrpl/core/binarycodec/types/blob.py | |
| if isinstance(value, str): | |
| try: | |
| return cls(bytes.fromhex(value)) | |
| except ValueError as e: | |
| raise XRPLBinaryCodecException( | |
| f"Invalid hex string to construct a Blob: {value!r}" | |
| ) from e |
🤖 Prompt for AI Agents
In tests/unit/core/binarycodec/types/test_blob.py around lines 21 to 23, the
test expects ValueError for non-hex input but the project standard is to raise
XRPLBinaryCodecException; update the test to use the context-manager form
self.assertRaises(XRPLBinaryCodecException): Blob.from_value(invalid_value) and
then modify Blob.from_value implementation to wrap the bytes.fromhex call in a
try/except that catches ValueError and raises XRPLBinaryCodecException (ensure
XRPLBinaryCodecException is imported where used).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
xrpl/asyncio/transaction/main.py (1)
185-186: Remove stdout prints in library path; use logging at debug level.Printing tx contents on failure is noisy and can leak details to stdout.
Apply:
- print("input txn: ", transaction) - print(response.result) + logger.debug("submit() failed; txn=%s result=%s", transaction, response.result)Add at top-level (outside this hunk):
# at file top import logging logger = logging.getLogger(__name__)tests/integration/transactions/test_lending_protocol.py (1)
96-106: Optional: assert autofilled fee matches expectation (1 extra counterparty sig).Helps guard future regressions in LOAN_SET fee logic.
Example (pseudo):
# after autofill_and_sign(...) base_fee = int((await get_fee(client))) self.assertEqual(int(loan_issuer_signed_txn.fee), base_fee * 2)xrpl/models/transactions/loan_set.py (2)
26-33: Clarify role: “counterparty” rather than “Lender”.Docstring should reflect that this signature is from the counterparty (borrower or issuer depending on initiator).
Apply:
- """ - An inner object that contains the signature of the Lender over the transaction. - The fields contained in this object are: - """ + """ + Inner object containing the counterparty's signature over the transaction. + """
69-72: Align data length docs/message with hex semantics.Limit is 256 bytes = 512 hex chars; make this explicit and consistent.
Apply:
- """ - Arbitrary metadata in hex format. The field is limited to 256 bytes. - """ + """ + Arbitrary metadata as a hex string. Maximum 256 bytes (512 hex characters). + """ @@ - if self.data is not None and len(self.data) > self.MAX_DATA_LENGTH: - parent_class_errors["LoanSet:data"] = "Data must be less than 256 bytes." + if self.data is not None and len(self.data) > self.MAX_DATA_LENGTH: + parent_class_errors["LoanSet:data"] = ( + "Data must be at most 256 bytes (512 hex characters)." + )Also applies to: 175-177
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/integration/transactions/test_lending_protocol.py(1 hunks)tests/unit/models/transactions/test_loan_set.py(1 hunks)xrpl/asyncio/transaction/main.py(3 hunks)xrpl/core/binarycodec/definitions/definitions.json(9 hunks)xrpl/models/transactions/loan_set.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/models/transactions/test_loan_set.py
- xrpl/core/binarycodec/definitions/definitions.json
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: tests/integration/transactions/test_lending_protocol.py:116-118
Timestamp: 2025-09-15T15:21:38.832Z
Learning: The start_date field in LoanSet transactions is scheduled for deprecation/removal in the next revision of the XLS-66d Lending Protocol specification, so timestamp format corrections are not needed.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: xrpl/models/transactions/loan_broker_cover_deposit.py:5-12
Timestamp: 2025-09-15T15:48:47.704Z
Learning: In the Lending Protocol (XLS-66d), LoanBrokerCoverDeposit allows depositing First-Loss Capital in XRP, IOU, or MPT assets, unlike LoanBrokerCoverClawback which rejects XRP amounts. These transactions have different validation requirements and should not be treated as having identical constraints.
📚 Learning: 2025-06-05T20:52:31.099Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#757
File: xrpl/asyncio/transaction/main.py:505-509
Timestamp: 2025-06-05T20:52:31.099Z
Learning: The correct fee calculation for XRPL Batch transactions follows the C++ rippled implementation structure: (2 × base_fee) + sum of individual inner transaction fees + (signer_count × base_fee). Each inner transaction's fee should be calculated individually using the same fee calculation logic as standalone transactions, not assumed to be equal to base_fee.
Applied to files:
xrpl/asyncio/transaction/main.py
📚 Learning: 2025-09-15T15:21:38.832Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: tests/integration/transactions/test_lending_protocol.py:116-118
Timestamp: 2025-09-15T15:21:38.832Z
Learning: The start_date field in LoanSet transactions is scheduled for deprecation/removal in the next revision of the XLS-66d Lending Protocol specification, so timestamp format corrections are not needed.
Applied to files:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2025-01-15T00:41:02.631Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#773
File: tests/integration/reusable_values.py:112-119
Timestamp: 2025-01-15T00:41:02.631Z
Learning: In xrpl-py integration tests, errors during test setup should be allowed to terminate the test suite naturally to aid debugging, rather than being caught and wrapped in custom error handlers. This applies to functions like `sign_and_reliable_submission_async` which already have robust error handling.
Applied to files:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2025-09-18T18:20:02.865Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: xrpl/models/transactions/loan_broker_set.py:105-110
Timestamp: 2025-09-18T18:20:02.865Z
Learning: The xrpl-py codebase contains many string-typed numeric fields in transaction models (like loan_origination_fee, maximum_amount, signature_reward, debt_maximum, etc.) and consistently uses direct int() calls for validation without try-catch blocks, allowing ValueError exceptions to bubble up naturally for clear error communication.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-10-30T20:34:35.451Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-10-30T20:44:27.753Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:89-93
Timestamp: 2024-10-30T20:44:27.753Z
Learning: In the Python file `xrpl/models/transactions/deposit_preauth.py`, within the `_validate_credentials_length` function, when validating `authorize_credentials` and `unauthorize_credentials`, it's acceptable to assign error messages to the same key in the `errors` dictionary if the conditions are mutually exclusive, as the error messages won't overwrite each other.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-10-30T20:45:02.301Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:54-64
Timestamp: 2024-10-30T20:45:02.301Z
Learning: In the `DepositPreauth` class in `xrpl/models/transactions/deposit_preauth.py`, when validating that exactly one of the parameters `authorize`, `unauthorize`, `authorize_credentials`, or `unauthorize_credentials` is set, ensure that any refactoring maintains logical equivalence with the original condition to prevent altering the intended functionality.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2025-09-15T15:54:25.813Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: xrpl/models/transactions/loan_set.py:158-181
Timestamp: 2025-09-15T15:54:25.813Z
Learning: In the xrpl-py codebase, validation logic for transaction fields is primarily handled by rippled (the C++ XRPL implementation) rather than being duplicated in the Python models. Python models should focus on basic structural validation rather than comprehensive field validation that rippled already performs.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-12-11T01:47:28.074Z
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:78-84
Timestamp: 2024-12-11T01:47:28.074Z
Learning: In the `xrpl-py` codebase, within the models (e.g., in `xrpl/models/transactions/`), flag validation checks are not performed; flag checks are handled elsewhere outside of the models.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2025-09-18T18:20:02.865Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: xrpl/models/transactions/loan_broker_set.py:105-110
Timestamp: 2025-09-18T18:20:02.865Z
Learning: In xrpl-py, numeric string fields in transaction models typically use direct int() calls for validation without try-catch blocks. The team prefers to let ValueError exceptions bubble up naturally rather than wrapping them in try-catch for field-level error aggregation, prioritizing code simplicity and performance.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2025-09-18T18:20:02.865Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: xrpl/models/transactions/loan_broker_set.py:105-110
Timestamp: 2025-09-18T18:20:02.865Z
Learning: In xrpl-py transaction models, numeric string fields are validated using direct int() calls without try-catch blocks in _get_errors methods. This is the established pattern across the codebase for consistency and performance reasons, and ValueError exceptions provide clear error communication to users.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2024-12-10T20:35:09.762Z
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:70-76
Timestamp: 2024-12-10T20:35:09.762Z
Learning: In the `xrpl/models/transactions/mptoken_issuance_create.py` file, the `maximum_amount` field in the `MPTokenIssuanceCreate` class is a non-negative integer string and should be typed as `Optional[str]`.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2025-02-11T21:11:19.326Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#772
File: tools/generate_definitions.py:0-0
Timestamp: 2025-02-11T21:11:19.326Z
Learning: In XRPL's binary codec format, large unsigned integers (UINT128, UINT160, UINT192, UINT256) should be consistently treated as Hash types, while smaller UINTs are translated to UInt types. This is reflected in generate_tx_models.py where they are mapped to "str" type.
Applied to files:
xrpl/models/transactions/loan_set.py
🧬 Code graph analysis (3)
xrpl/asyncio/transaction/main.py (2)
xrpl/models/transactions/loan_set.py (1)
LoanSet(61-237)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)
tests/integration/transactions/test_lending_protocol.py (14)
tests/integration/integration_test_case.py (1)
IntegrationTestCase(9-18)tests/integration/it_utils.py (3)
fund_wallet_async(120-129)sign_and_reliable_submission_async(191-223)test_async_and_sync(268-363)xrpl/asyncio/transaction/main.py (3)
autofill_and_sign(127-153)submit(156-187)sign(81-124)xrpl/core/binarycodec/main.py (1)
encode_for_signing(37-52)xrpl/models/requests/account_objects.py (2)
AccountObjects(50-73)AccountObjectType(19-45)xrpl/models/transactions/loan_delete.py (1)
LoanDelete(15-27)xrpl/models/transactions/loan_manage.py (2)
LoanManage(51-63)LoanManageFlag(14-35)xrpl/models/transactions/loan_pay.py (1)
LoanPay(16-34)xrpl/models/transactions/loan_set.py (2)
LoanSet(61-237)CounterpartySignature(25-33)xrpl/models/transactions/vault_create.py (2)
VaultCreate(65-139)WithdrawalPolicy(56-60)xrpl/models/transactions/vault_deposit.py (1)
VaultDeposit(14-26)xrpl/models/currencies/xrp.py (1)
XRP(26-91)xrpl/models/response.py (1)
ResponseStatus(23-27)xrpl/wallet/main.py (3)
Wallet(16-289)create(121-135)address(24-29)
xrpl/models/transactions/loan_set.py (5)
xrpl/models/base_model.py (1)
BaseModel(93-418)xrpl/models/transactions/transaction.py (3)
Signer(131-161)Transaction(184-549)TransactionFlagInterface(173-179)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)xrpl/models/utils.py (1)
require_kwargs_on_init(317-370)xrpl/models/transactions/loan_broker_set.py (1)
_get_errors(73-112)
🔇 Additional comments (3)
tests/integration/transactions/test_lending_protocol.py (2)
121-131: Nice: ledger_accept after direct submit.This avoids races in standalone mode. Looks good.
108-111: Bug: signing wrong payload type (hex string instead of bytes).
sign()expects bytes. Current code passes a hex string.Apply:
- borrower_txn_signature = sign( - encode_for_signing(loan_issuer_signed_txn.to_xrpl()), - borrower_wallet.private_key, - ) + encoded = encode_for_signing(loan_issuer_signed_txn.to_xrpl()) + borrower_txn_signature = sign( + bytes.fromhex(encoded), + borrower_wallet.private_key, + )Likely an incorrect or invalid review comment.
xrpl/asyncio/transaction/main.py (1)
522-541: Fee calc: guard empty signers and gate fallback on counterparty presence.Treat signers=[] as one signer and only add the fallback per-signature fee when a counterparty is present to avoid under/over-charging.
Apply:
- elif transaction.transaction_type == TransactionType.LOAN_SET: + elif transaction.transaction_type == TransactionType.LOAN_SET: # Compute the additional cost of each signature in the # CounterpartySignature, whether a single signature or a multisignature loan_set = cast(LoanSet, transaction) if loan_set.counterparty_signature is not None: - signer_count = ( - len(loan_set.counterparty_signature.signers) - if loan_set.counterparty_signature.signers is not None - else 1 - ) + signers = loan_set.counterparty_signature.signers + signer_count = max(1, len(signers)) if signers is not None else 1 base_fee += net_fee * signer_count - else: + elif loan_set.counterparty is not None: # Note: Due to lack of information, the client-library assumes that # there is only one signer. However, the LoanIssuer and Borrower need to # communicate the number of CounterpartySignature.signers # (or the appropriate transaction-fee) # with each other off-chain. This helps with efficient fee-calculation for # the LoanSet transaction. base_fee += net_feeRepo scan: integration test submits LoanSet with counterparty; unit tests instantiate LoanSet without counterparty (no submission).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/integration/transactions/test_lending_protocol.py (2)
298-321: Duplicate: counterparty co-sign pattern.Same co-sign flow as above; looks good. Ensure no mutation between signing and submit.
323-334: Good: ledger_accept after direct submit (IOU path).Prevents races before AccountObjects query.
🧹 Nitpick comments (4)
tests/integration/transactions/test_lending_protocol.py (4)
98-101: Fix typo in comment."transered" → "transferred".
Apply:
- # transaction and the requested principal (excluding fees) is transered to + # transaction and the requested principal (excluding fees) is transferred to
188-199: Prefer wallet.address for consistency.Use address consistently; classic_address is redundant here.
Apply:
- AccountSet( - account=loan_issuer.classic_address, + AccountSet( + account=loan_issuer.address,
295-298: Fix repeated typo in comment."transered" → "transferred".
Apply:
- # transaction and the requested principal (excluding fees) is transered to + # transaction and the requested principal (excluding fees) is transferred to
102-126: Reduce duplication: extract a helper to build a counterparty-signed LoanSet.Both tests duplicate the co-sign assembly. Consider a small helper to keep tests DRY.
Example (place near other test helpers):
def build_counterparty_signed_loanset(issuer_signed: Transaction, borrower: Wallet) -> dict: borrower_sig = sign(encode_for_signing(issuer_signed.to_xrpl()), borrower.private_key) tx = issuer_signed.to_dict() tx["counterparty_signature"] = CounterpartySignature( signing_pub_key=borrower.public_key, txn_signature=borrower_sig, ) return txThen:
- loan_issuer_and_borrower_signature = loan_issuer_signed_txn.to_dict() - loan_issuer_and_borrower_signature["counterparty_signature"] = ( - CounterpartySignature( - signing_pub_key=borrower_wallet.public_key, - txn_signature=borrower_txn_signature, - ) - ) + loan_issuer_and_borrower_signature = build_counterparty_signed_loanset( + loan_issuer_signed_txn, borrower_wallet + )Also applies to: 298-321
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/transactions/test_lending_protocol.py(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: tests/integration/transactions/test_lending_protocol.py:116-118
Timestamp: 2025-09-15T15:21:38.832Z
Learning: The start_date field in LoanSet transactions is scheduled for deprecation/removal in the next revision of the XLS-66d Lending Protocol specification, so timestamp format corrections are not needed.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: xrpl/models/transactions/loan_broker_cover_deposit.py:5-12
Timestamp: 2025-09-15T15:48:47.704Z
Learning: In the Lending Protocol (XLS-66d), LoanBrokerCoverDeposit allows depositing First-Loss Capital in XRP, IOU, or MPT assets, unlike LoanBrokerCoverClawback which rejects XRP amounts. These transactions have different validation requirements and should not be treated as having identical constraints.
📚 Learning: 2025-09-15T15:21:38.832Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: tests/integration/transactions/test_lending_protocol.py:116-118
Timestamp: 2025-09-15T15:21:38.832Z
Learning: The start_date field in LoanSet transactions is scheduled for deprecation/removal in the next revision of the XLS-66d Lending Protocol specification, so timestamp format corrections are not needed.
Applied to files:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2025-01-15T00:41:02.631Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#773
File: tests/integration/reusable_values.py:112-119
Timestamp: 2025-01-15T00:41:02.631Z
Learning: In xrpl-py integration tests, errors during test setup should be allowed to terminate the test suite naturally to aid debugging, rather than being caught and wrapped in custom error handlers. This applies to functions like `sign_and_reliable_submission_async` which already have robust error handling.
Applied to files:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2025-09-15T15:48:47.704Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#866
File: xrpl/models/transactions/loan_broker_cover_deposit.py:5-12
Timestamp: 2025-09-15T15:48:47.704Z
Learning: In the Lending Protocol (XLS-66d), LoanBrokerCoverDeposit allows depositing First-Loss Capital in XRP, IOU, or MPT assets, unlike LoanBrokerCoverClawback which rejects XRP amounts. These transactions have different validation requirements and should not be treated as having identical constraints.
Applied to files:
tests/integration/transactions/test_lending_protocol.py
🧬 Code graph analysis (1)
tests/integration/transactions/test_lending_protocol.py (6)
tests/integration/integration_test_case.py (1)
IntegrationTestCase(9-18)tests/integration/it_utils.py (3)
fund_wallet_async(120-129)sign_and_reliable_submission_async(191-223)test_async_and_sync(268-363)xrpl/asyncio/transaction/main.py (3)
autofill_and_sign(127-153)submit(156-187)sign(81-124)xrpl/core/binarycodec/main.py (1)
encode_for_signing(37-52)xrpl/models/response.py (2)
ResponseStatus(23-27)is_successful(74-82)xrpl/wallet/main.py (3)
Wallet(16-289)create(121-135)address(24-29)
🔇 Additional comments (4)
tests/integration/transactions/test_lending_protocol.py (4)
102-126: Counterparty co-sign flow looks correct; keep preimage stable.Good pattern: issuer autofills/signs, borrower signs encode_for_signing of the issuer-signed JSON, then attach CounterpartySignature. Please confirm no fields affecting the signing preimage mutate between borrower signing and submit.
127-138: Good: ledger_accept after direct submit prevents race.Adding LEDGER_ACCEPT after submit() avoids AccountObjects fetch flakiness on standalone.
167-175: Confirm amount units for LoanPay and VaultDeposit.Please confirm these string amounts are in the correct units per XLS-66d (XRP vs drops; IOU value precision). A mismatch could cause silent logic changes if the spec evolves.
Also applies to: 362-372
39-40: Sync/async function mapping — sync shims verified.
autofill_and_sign and submit are defined in xrpl/transaction/main.py, so the sync path will not raise a NameError.
High Level Overview of Change
This PR adds support for the Lending Protocol. The specification can be found here: XRPLF/XRPL-Standards#240
The cpp implementation is available here: XRPLF/rippled#5270
Type of Change
Did you update CHANGELOG.md?
Test Plan
Appropriate unit and integ tests have been added.