Support for Lending Protocol (XLS-66d) #866
Conversation
…ng; LoanDraw transaction has nt been implemented
WalkthroughAdds XLS-66d Lending Protocol: new loan and loan-broker transaction types/models/enums, binary codec updates, validation rules, LOAN_SET fee/autofill signer handling, AccountObjects enum extensions, new unit/integration tests (XRP/IOU/MPT), and minor config/changelog edits. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-01-15T00:41:02.631ZApplied to files:
📚 Learning: 2024-11-01T18:53:01.394ZApplied to files:
📚 Learning: 2024-11-01T16:20:50.192ZApplied to files:
🧬 Code graph analysis (2)tests/integration/reqs/test_sign.py (5)
tests/unit/models/requests/test_sign.py (2)
🪛 Ruff (0.14.8)tests/integration/reqs/test_sign.py8-8: Possible hardcoded password assigned to: "_SECRET" (S105) ⏰ 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). (7)
🔇 Additional comments (4)
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.
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. |
| 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.
TransactionType.LOAN_SET should not have any special handling of fees calculation. So, I was suggesting to remove all changes in this file.
As explained in earlier message, we would not hit this code-path. If you want to try it, maybe try writing an integration test that hits this codepath with multi-signing enabled. As far as I understand counterparty_signature cannot be included in a transaction that needs to be signed. So counterparty_signature would always be None when this method is executed.
There was a problem hiding this comment.
To auto-fill the approach should be to use loan_set.counterparty instead of loan_set.counterparty_signature:
-
If loan_set.counterparty is present?
a. Check loan_set.counterparty has multi-signing enabled. If so, get the count of minimum number of signers required to sign this transaction using another API call and perform the fee calculation that you are doing.
b. If loan_set.counterparty does not have multi-sign enabled, then it would be net_fee. -
If loan_set.counterparty is not present?
a. Get the loan broker from LoanBrokerID and perform#1.a and #1.b.
We should write integration tests, if we implement this logic. I am planning to write such integration test in xrpl.js once, the multi-signing starts working in cpp implementation for LoanSet transaction.
There was a problem hiding this comment.
1.a: Even if we get the minimum quorum required for the counterparty, the actual number of signers can always exceed the minimum number of signers. In this case, the transaction will fail due to insufficient fee.
Will users want this feature at all? It is easier to communicate the exact number of signers offline between the loan-broker and the borrower. This autofill procedure involves significant network communication and associated delays. If a user wants to use the library for time-sensitive tasks (high frequency trading, flash loans, etc), won't the delays be unacceptable?
There was a problem hiding this comment.
1.a: Even if we get the minimum quorum required for the counterparty, the actual number of signers can always exceed the minimum number of signers. In this case, the transaction will fail due to insufficient fee.
You made a valid point that if multi-signing is setup as three signers (1+1+1) with quorum as 2, it could happen that even though only two needs to sign, but all three can sign and the transaction is still valid. So, we need to find a balance between making the autofill feature usable and not over-estimate the fees. Using total number of signers will always ensure that the transaction passes (at a cost of increased transaction fees) and using quorum will give optimal cost but transaction might fail if more than the quorum signs that transaction. I would lean towards using quorum to begin with and adjust to max signers if we receive many issues from library users. This calculations can always be changed without incurring a breaking change. Does this makes sense?
With the current setup, the autofill feature is totally not usable when multi-signing gets involved and I feel that Lending Protocol will be used by intuitions where multi-signing will be the norm.
This autofill procedure involves significant network communication and associated delays. If a user wants to use the library for time-sensitive tasks (high frequency trading, flash loans, etc), won't the delays be unacceptable?
Autofilling is optional and they can completely skip it for such use cases. And on another note, we do call server_info to auto-fill Payment/Offer related transactions which would be used for high frequency trading.
There was a problem hiding this comment.
@Patel-Raj11 As per our offline discussion, I have updated the design of the autofill feature for the LoanSet transaction. 2ba7a5a
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?
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.
… adds utility methods that aid in this effort
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (18)
tests/unit/core/binarycodec/types/test_account_id.py (1)
32-46: Avoid asserting the codec against itselfRight now both sides of these assertions call
AccountID.from_value(...).to_hex(), so even if a regression caused both conversions to return the wrong bytes, the test would still pass. Compare the decoded value to the literal hex instead to lock in the expected byte pattern for these special accounts.self.assertEqual( AccountID.from_value("rrrrrrrrrrrrrrrrrrrrBZbvji").to_hex(), - AccountID.from_value("0000000000000000000000000000000000000001").to_hex(), + "0000000000000000000000000000000000000001", ) … self.assertEqual( AccountID.from_value("rrrrrrrrrrrrrrrrrrrrrhoLvTp").to_hex(), - AccountID.from_value("0000000000000000000000000000000000000000").to_hex(), + "0000000000000000000000000000000000000000", )xrpl/core/binarycodec/types/issue.py (5)
24-26: Define and compare using raw bytes, not base58 JSON.Storing the sentinel as AccountID is fine, but downstream comparisons use to_json() (base58). Prefer byte-wise comparisons for consistency and performance. Optionally expose a bytes constant alongside the AccountID.
Apply this targeted change in from_parser (see separate comment), and consider adding:
+ # Optional: bytes view for fast equality checks + BLACK_HOLED_ACCOUNT_ID_BYTES = bytes(BLACK_HOLED_ACCOUNT_ID)
62-71: Docstring typos and outdated hint.
- Fix: “however it it interpreted” → “however it is interpreted”.
- The length_hint note (“pass 24 for Hash192”) is no longer applicable to from_parser, which consumes 20+20(+4) directly. Update/remove to avoid confusion.
80-95: Simplify endianness handling using bytes (avoid hex slicing).Use byte-reversal on the 4-byte sequence instead of hex string manipulation. It’s clearer and faster. Also rename issuer_account_in_hex → issuer_account_bytes.
Apply this diff:
- # rippled accepts sequence number in big-endian format only. - sequence_in_hex = mpt_issuance_id_bytes[:4].hex().upper() - sequenceBE = ( - sequence_in_hex[6:8] - + sequence_in_hex[4:6] - + sequence_in_hex[2:4] - + sequence_in_hex[0:2] - ) - issuer_account_in_hex = mpt_issuance_id_bytes[4:] - return cls( - bytes( - bytes(issuer_account_in_hex) - + bytes(cls.BLACK_HOLED_ACCOUNT_ID) - + bytearray.fromhex(sequenceBE) - ) - ) + # Sequence is little-endian in the MPT issuance ID; store big-endian in Issue. + sequence_be = mpt_issuance_id_bytes[:4][::-1] + issuer_account_bytes = mpt_issuance_id_bytes[4:] + return cls(issuer_account_bytes + bytes(cls.BLACK_HOLED_ACCOUNT_ID) + sequence_be)
119-133: Compare AccountID by bytes, not base58 JSON.Avoid base58 conversions in hot paths and reduce risk of formatting discrepancies.
Apply this diff:
- issuer_account_id = AccountID.from_parser(parser) - if issuer_account_id.to_json() == cls.BLACK_HOLED_ACCOUNT_ID.to_json(): + issuer_account_id = AccountID.from_parser(parser) + if bytes(issuer_account_id) == bytes(cls.BLACK_HOLED_ACCOUNT_ID): sequence = UInt32.from_parser(parser) return cls( bytes(currency_or_account) + bytes(cls.BLACK_HOLED_ACCOUNT_ID) + bytes(sequence) )
142-163: Build mpt_issuance_id via bytes, remove redundant .upper(), and shorten message (TRY003).
- Use bytes slicing + reversal instead of hex string slicing.
- to_hex() already uppercases; extra .upper() is redundant.
- Ruff TRY003: shorten the exception message string.
Apply this diff:
- if len(self.buffer) == 20 + 20 + 4: - serialized_mpt_in_hex = self.to_hex().upper() - if serialized_mpt_in_hex[40:80] != self.BLACK_HOLED_ACCOUNT_ID.to_hex(): - raise XRPLBinaryCodecException( - "Invalid MPT Issue encoding: black-hole AccountID mismatch." - ) - return { - # Although the sequence bytes are stored in big-endian format, the JSON - # representation is in little-endian format. This is required for - # compatibility with c++ rippled implementation. - "mpt_issuance_id": ( - serialized_mpt_in_hex[86:88] - + serialized_mpt_in_hex[84:86] - + serialized_mpt_in_hex[82:84] - + serialized_mpt_in_hex[80:82] - ) - + serialized_mpt_in_hex[:40] - } + if len(self.buffer) == 20 + 20 + 4: + buf = self.buffer + if buf[20:40] != bytes(self.BLACK_HOLED_ACCOUNT_ID): + raise XRPLBinaryCodecException("Invalid MPT Issue encoding: black-hole AccountID mismatch.") + # JSON uses little-endian sequence; storage uses big-endian. + seq_le = buf[40:44][::-1] + issuer = buf[:20] + return {"mpt_issuance_id": (seq_le + issuer).hex().upper()}tests/unit/core/binarycodec/types/test_issue.py (1)
43-49: Good negative test for length check.Covers invalid 47-char mpt_issuance_id. Consider adding a “non-hex” 48-char case to assert Hash192 validation.
tests/integration/it_utils.py (5)
42-44: Normalize devnet endpoints (remove trailing slashes).Minor consistency nit; trailing slashes aren’t used for testnet/local URLs and can cause subtle double-slash joins in some HTTP stacks.
-JSON_DEVNET_URL = "https://s.devnet.rippletest.net:51234/" -WEBSOCKET_DEVNET_URL = "wss://s.devnet.rippletest.net:51233/" +JSON_DEVNET_URL = "https://s.devnet.rippletest.net:51234" +WEBSOCKET_DEVNET_URL = "wss://s.devnet.rippletest.net:51233"
101-107: Use asyncio.create_task instead of ensure_future.create_task is the modern API and clearer intent for scheduling a coroutine.
- self._task = asyncio.ensure_future(self._job()) + self._task = asyncio.create_task(self._job())
202-205: Fix misleading comment about devnet behavior.We skip ledger_accept on devnet; we don't “wait” explicitly here.
- # On devnet, wait for the transaction to be validated + # On devnet, do not send ledger_accept; rely on network validation if not use_devnet: client.request(LEDGER_ACCEPT_REQUEST)
242-245: Fix misleading comment about devnet behavior (async).Same as sync path: we skip ledger_accept on devnet.
- # On devnet, wait for the transaction to be validated + # On devnet, do not send ledger_accept; rely on network validation if not use_devnet: await client.request(LEDGER_ACCEPT_REQUEST)
288-294: Unify naming for clarity: is_devnet vs use_devnet.Across the file we mix “is_devnet” (here) and “use_devnet” (elsewhere). Pick one (e.g., is_devnet) for consistency.
tests/integration/transactions/test_lending_protocol.py (4)
106-109: Fix typo in comment (transferred).- # transaction and the requested principal (excluding fees) is transered to + # transaction and the requested principal (excluding fees) is transferred to
145-146: Guard LEDGER_ACCEPT for devnet runs.Direct LEDGER_ACCEPT won’t work on devnet/public nets. Consider gating by a devnet flag (like use_devnet) or using a helper that no-ops on devnet.
- await client.request(LEDGER_ACCEPT_REQUEST) + # Only applicable in standalone/local networks + try: + await client.request(LEDGER_ACCEPT_REQUEST) + except Exception: + # Ignore on networks that don't support ledger_accept (e.g., devnet/mainnet) + pass
317-339: DRY: Extract “counterparty sign and submit” into a helper.The borrower-sign + CounterpartySignature + submit sequence is duplicated across tests. A small helper reduces repetition and mistakes.
Also applies to: 520-542
304-306: Fix typo in comment (transferred).- # transaction and the requested principal (excluding fees) is transered to + # transaction and the requested principal (excluding fees) is transferred toAlso applies to: 506-508
tests/integration/transactions/test_single_asset_vault.py (2)
46-48: Combine flags with bitwise OR, not addition.Use | for bitmasks; addition works only incidentally and is less clear.
- flags=MPTokenIssuanceCreateFlag.TF_MPT_CAN_TRANSFER - + MPTokenIssuanceCreateFlag.TF_MPT_CAN_CLAWBACK, + flags=( + MPTokenIssuanceCreateFlag.TF_MPT_CAN_TRANSFER + | MPTokenIssuanceCreateFlag.TF_MPT_CAN_CLAWBACK + ),
104-105: Fix typo in VaultSet data string (auxiliary).- data=str_to_hex("auxilliary data pertaining to the vault"), + data=str_to_hex("auxiliary data pertaining to the vault"),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/integration/it_utils.py(9 hunks)tests/integration/transactions/test_lending_protocol.py(1 hunks)tests/integration/transactions/test_single_asset_vault.py(1 hunks)tests/unit/core/binarycodec/types/test_account_id.py(1 hunks)tests/unit/core/binarycodec/types/test_issue.py(2 hunks)xrpl/core/binarycodec/types/issue.py(5 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 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.751Z
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-06-04T22:17:47.164Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#814
File: xrpl/models/amounts/mpt_amount.py:54-64
Timestamp: 2025-06-04T22:17:47.164Z
Learning: For MPTIssue class in xrpl-py, the user ckeshava confirmed that a simple class implementation without BaseModel inheritance works correctly for their current requirements, even though other currency types in the same Union (IssuedCurrency, MPTCurrency, XRP) extend BaseModel.
Applied to files:
xrpl/core/binarycodec/types/issue.pytests/unit/core/binarycodec/types/test_issue.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/core/binarycodec/types/issue.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, the UINT192 type from the MPT amendment is translated to UInt192, following the same pattern as other non-hash UINT types.
Applied to files:
xrpl/core/binarycodec/types/issue.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-15T15:48:47.751Z
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.751Z
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 (6)
xrpl/core/binarycodec/types/issue.py (5)
xrpl/core/binarycodec/types/hash192.py (1)
Hash192(17-26)xrpl/core/binarycodec/types/serialized_type.py (5)
SerializedType(15-100)from_value(39-42)from_parser(29-35)to_json(64-73)to_hex(84-91)xrpl/core/binarycodec/types/uint32.py (3)
UInt32(18-71)from_value(44-71)from_parser(29-41)xrpl/core/binarycodec/types/account_id.py (3)
AccountID(27-89)from_value(45-80)to_json(82-89)xrpl/core/binarycodec/types/currency.py (3)
from_value(97-120)Currency(64-131)to_json(122-131)
tests/integration/transactions/test_single_asset_vault.py (8)
xrpl/models/amounts/mpt_amount.py (1)
MPTAmount(18-51)xrpl/models/currencies/mpt_currency.py (1)
MPTCurrency(28-63)xrpl/models/requests/account_objects.py (2)
AccountObjects(50-73)AccountObjectType(19-45)xrpl/models/requests/tx.py (1)
Tx(20-87)xrpl/models/response.py (2)
ResponseStatus(23-27)is_successful(74-82)xrpl/models/transactions/mptoken_authorize.py (1)
MPTokenAuthorize(45-63)xrpl/models/transactions/mptoken_issuance_create.py (2)
MPTokenIssuanceCreate(57-143)MPTokenIssuanceCreateFlag(25-37)tests/integration/it_utils.py (2)
fund_wallet_async(134-143)sign_and_reliable_submission_async(209-245)
tests/integration/transactions/test_lending_protocol.py (22)
tests/integration/integration_test_case.py (1)
IntegrationTestCase(9-18)tests/integration/it_utils.py (3)
fund_wallet_async(134-143)sign_and_reliable_submission_async(209-245)test_async_and_sync(296-400)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_broker_set.py (1)
LoanBrokerSet(19-112)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/amounts/issued_currency_amount.py (1)
IssuedCurrencyAmount(21-51)xrpl/models/amounts/mpt_amount.py (1)
MPTAmount(18-51)xrpl/models/currencies/issued_currency.py (1)
IssuedCurrency(31-76)xrpl/models/currencies/mpt_currency.py (1)
MPTCurrency(28-63)xrpl/models/currencies/xrp.py (1)
XRP(26-91)xrpl/models/requests/tx.py (1)
Tx(20-87)xrpl/models/response.py (2)
ResponseStatus(23-27)is_successful(74-82)xrpl/models/transactions/mptoken_authorize.py (1)
MPTokenAuthorize(45-63)xrpl/models/transactions/mptoken_issuance_create.py (2)
MPTokenIssuanceCreate(57-143)MPTokenIssuanceCreateFlag(25-37)xrpl/wallet/main.py (4)
Wallet(16-289)create(121-135)address(24-29)classic_address(34-41)
tests/unit/core/binarycodec/types/test_account_id.py (2)
xrpl/core/binarycodec/types/account_id.py (1)
AccountID(27-89)xrpl/core/binarycodec/types/serialized_type.py (1)
to_hex(84-91)
tests/unit/core/binarycodec/types/test_issue.py (3)
xrpl/core/binarycodec/types/issue.py (4)
Issue(21-171)from_value(39-100)to_json(135-171)from_parser(103-133)xrpl/core/binarycodec/types/serialized_type.py (1)
to_hex(84-91)xrpl/core/binarycodec/binary_wrappers/binary_parser.py (1)
BinaryParser(31-261)
tests/integration/it_utils.py (3)
xrpl/asyncio/clients/async_client.py (2)
request(19-29)AsyncClient(12-29)xrpl/clients/sync_client.py (2)
request(21-31)SyncClient(14-31)xrpl/asyncio/clients/client.py (1)
Client(20-56)
🪛 Ruff (0.13.1)
xrpl/core/binarycodec/types/issue.py
74-77: Avoid specifying long messages outside the exception class
(TRY003)
149-151: Avoid specifying long messages outside the exception class
(TRY003)
tests/integration/it_utils.py
292-292: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (4)
xrpl/core/binarycodec/types/issue.py (1)
72-79: Length validation: good; keep relying on Hash192 for hex validation.The explicit 48-char check is correct and clear. Hash192.from_value will take care of hex validity.
If desired, add a unit test for a 48-char non-hex string (e.g., containing “Z”) to assert Hash192 validation triggers.
tests/unit/core/binarycodec/types/test_issue.py (3)
35-41: MPT happy-path test looks good.Correct 48-hex input and round-trip expectation. Matches endianness rules.
50-69: Excellent binary decomposition assertion.Verifies issuer + black-hole + sequence layout precisely. This will catch regressions in ordering/endianness.
106-115: Parser round-trip test for MPT is solid.Confirms from_parser detection via black-hole AccountID and sequence consumption.
…ported::no. Remove their names from the amendment-list inside the ci-config file
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.ci-config/rippled.cfg(0 hunks)xrpl/core/binarycodec/definitions/definitions.json(9 hunks)
💤 Files with no reviewable changes (1)
- .ci-config/rippled.cfg
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/unit/models/transactions/test_account_delete.py:52-60
Timestamp: 2024-12-12T00:48:38.042Z
Learning: The credential ID format validation for xrpl-py is being tracked in issue #766 and should not be duplicated in other PRs.
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_cover_deposit.py:5-12
Timestamp: 2025-09-15T15:48:47.751Z
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
Repo: XRPLF/xrpl-py PR: 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
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_cover_withdraw.py:8-13
Timestamp: 2025-09-15T15:50:48.800Z
Learning: LoanBrokerCoverWithdraw allows withdrawal of First-Loss Capital in XRP, IOU, or MPT assets, similar to LoanBrokerCoverDeposit. Unlike LoanBrokerCoverClawback which rejects XRP amounts, both Deposit and Withdraw operations should allow XRP assets.
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_set.py:105-110
Timestamp: 2025-09-18T18:20:02.896Z
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.
📚 Learning: 2025-05-13T18:43:34.412Z
Learnt from: Patel-Raj11
Repo: XRPLF/xrpl-py PR: 840
File: xrpl/core/binarycodec/definitions/definitions.json:2594-2602
Timestamp: 2025-05-13T18:43:34.412Z
Learning: The `definitions.json` file in XRPL is a generated file and should not be directly edited. Constraints like maximum array lengths are enforced at the model level (e.g., in transaction models) rather than in the binary codec layer.
Applied to files:
xrpl/core/binarycodec/definitions/definitions.json
📚 Learning: 2025-09-15T15:21:38.832Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 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/core/binarycodec/definitions/definitions.json
🪛 Biome (2.1.2)
xrpl/core/binarycodec/definitions/definitions.json
[error] 839-839: The key nth was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
⏰ 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). (9)
- GitHub Check: Integration test (3.14)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.8)
- GitHub Check: Integration test (3.12)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
xrpl/core/binarycodec/definitions/definitions.json (1)
183-192: Verify that directly editingdefinitions.jsonis the correct approach for XLS-66d support.According to codebase learnings, this file is marked as a generated artifact and should not be directly edited. Constraints are typically enforced at the model level rather than in the binary codec layer. Ensure this change aligns with the codebase generation workflow.
If this file is generated by a tool or process, please clarify:
- What tool/process generates this file?
- Should the lending protocol definitions be added via a generation script instead of manual editing?
- Are there downstream tools or build steps that need to be aware of these changes?
Also applies to: 699-712, 1114-1133, 1484-1503, 2284-2303, 2364-2441, 2783-2792, 3399-3400, 3640-3648
…transaction. The client library takes a conservative approach to estimate the transaction fees. We assume that all the signers of the counterparty might provide their signatures to the transaction. However, it is likely that only the minimum quorum of signers sign the transaction and hence, the fees might be over-estimated by the library. If users prefer optimized version of the transaction fees, they need to input the fees themselves without relying on autofill.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
xrpl/asyncio/transaction/main.py (1)
186-187: Avoid bare print() calls in library code (use logging or surface warnings via API)
submitand the LOAN_SET fee branch both useprint(...):
- Lines 186–187: printing the full transaction and response on submit failure.
- Lines 570–581: printing a warning about fee estimation for multiple counterparty signers.
Concerns:
- Unstructured stdout noise from a library function is hard for callers to control.
- Printing full transactions may leak potentially sensitive information into logs.
- Other parts of this module don’t use
Consider either:
- Removing these prints and relying on the raised
XRPLRequestFailureExceptionplus caller-side logging, or- Replacing them with the project’s logging facility (e.g.,
logging.getLogger(__name__).debug(...)/.warning(...)), so callers can configure log levels/handlers.Example change sketch:
- if response.is_successful(): - return response - - print("input txn: ", transaction) - print(response.result) - raise XRPLRequestFailureException(response.result) + if response.is_successful(): + return response + + # Consider logging at debug level instead of printing, or let callers log. + raise XRPLRequestFailureException(response.result)and similarly remove or log the LOAN_SET warning instead of printing it.
Also applies to: 570-581
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.ci-config/rippled.cfg(1 hunks)tests/integration/transactions/test_lending_protocol.py(1 hunks)xrpl/asyncio/transaction/main.py(4 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/unit/models/transactions/test_account_delete.py:52-60
Timestamp: 2024-12-12T00:48:38.042Z
Learning: The credential ID format validation for xrpl-py is being tracked in issue #766 and should not be duplicated in other PRs.
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_cover_deposit.py:5-12
Timestamp: 2025-09-15T15:48:47.751Z
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
Repo: XRPLF/xrpl-py PR: 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
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_cover_withdraw.py:8-13
Timestamp: 2025-09-15T15:50:48.800Z
Learning: LoanBrokerCoverWithdraw allows withdrawal of First-Loss Capital in XRP, IOU, or MPT assets, similar to LoanBrokerCoverDeposit. Unlike LoanBrokerCoverClawback which rejects XRP amounts, both Deposit and Withdraw operations should allow XRP assets.
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_set.py:105-110
Timestamp: 2025-09-18T18:20:02.896Z
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.
📚 Learning: 2025-09-15T15:48:47.751Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_cover_deposit.py:5-12
Timestamp: 2025-09-15T15:48:47.751Z
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:
.ci-config/rippled.cfgtests/integration/transactions/test_lending_protocol.pyxrpl/asyncio/transaction/main.py
📚 Learning: 2024-11-01T16:20:50.192Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/integration/transactions/test_credential.py:61-63
Timestamp: 2024-11-01T16:20:50.192Z
Learning: In integration tests for xrpl-py, tests should only be testing the library, not rippled functionalities.
Applied to files:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2024-10-30T20:34:35.451Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 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:
tests/integration/transactions/test_lending_protocol.pyxrpl/asyncio/transaction/main.py
📚 Learning: 2025-09-15T15:21:38.832Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 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.pyxrpl/asyncio/transaction/main.py
📚 Learning: 2024-11-01T18:53:01.394Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/unit/models/requests/test_deposit_authorized.py:7-15
Timestamp: 2024-11-01T18:53:01.394Z
Learning: In `tests/unit/models/requests/test_deposit_authorized.py`, additional tests for invalid credentials are unnecessary because `rippled` handles those checks, and the `xrpl-py` library does not include such checks.
Applied to files:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2025-01-15T00:41:02.631Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 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-30T21:01:15.823Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/integration/transactions/test_credential.py:42-44
Timestamp: 2024-10-30T21:01:15.823Z
Learning: In this codebase, extensive unit tests are located in `rippled`, and additional tests are not required in this library.
Applied to files:
tests/integration/transactions/test_lending_protocol.pyxrpl/asyncio/transaction/main.py
📚 Learning: 2025-02-05T21:42:42.425Z
Learnt from: achowdhry-ripple
Repo: XRPLF/xrpl-py PR: 803
File: tests/integration/transactions/test_trust_set.py:0-0
Timestamp: 2025-02-05T21:42:42.425Z
Learning: Failure cases for deep freeze functionality (e.g., setting deep freeze without regular freeze, clearing deep freeze while regular freeze is set) are handled by rippled server logic and don't need to be tested in the xrpl-py library.
Applied to files:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2025-09-15T15:54:25.836Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_set.py:158-181
Timestamp: 2025-09-15T15:54:25.836Z
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:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2025-09-15T15:50:48.800Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_cover_withdraw.py:8-13
Timestamp: 2025-09-15T15:50:48.800Z
Learning: LoanBrokerCoverWithdraw allows withdrawal of First-Loss Capital in XRP, IOU, or MPT assets, similar to LoanBrokerCoverDeposit. Unlike LoanBrokerCoverClawback which rejects XRP amounts, both Deposit and Withdraw operations should allow XRP assets.
Applied to files:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2025-09-18T18:20:02.896Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_set.py:105-110
Timestamp: 2025-09-18T18:20:02.896Z
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/asyncio/transaction/main.py
📚 Learning: 2024-11-04T19:43:00.465Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/integration/transactions/test_deposit_preauth.py:40-67
Timestamp: 2024-11-04T19:43:00.465Z
Learning: Negative test cases for credential validation should be included in unit tests rather than integration tests.
Applied to files:
xrpl/asyncio/transaction/main.py
📚 Learning: 2024-11-01T20:24:17.202Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 765
File: xrpl/models/transactions/xchain_add_claim_attestation.py:82-88
Timestamp: 2024-11-01T20:24:17.202Z
Learning: Changing the type of `was_locking_chain_send` in `xrpl/models/transactions/xchain_add_claim_attestation.py` to `bool` is out of scope for the current PR.
Applied to files:
xrpl/asyncio/transaction/main.py
📚 Learning: 2025-06-05T20:52:31.099Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 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 (2)
tests/integration/transactions/test_lending_protocol.py (13)
xrpl/asyncio/transaction/main.py (3)
autofill_and_sign(128-154)submit(157-188)sign(82-125)xrpl/models/requests/account_objects.py (2)
AccountObjects(50-73)AccountObjectType(19-45)xrpl/models/transactions/loan_broker_set.py (1)
LoanBrokerSet(19-112)xrpl/models/transactions/loan_delete.py (1)
LoanDelete(15-27)xrpl/models/transactions/loan_set.py (1)
LoanSet(61-237)xrpl/models/transactions/transaction.py (2)
Transaction(184-549)to_xrpl(340-348)xrpl/models/transactions/vault_create.py (1)
VaultCreate(64-140)xrpl/models/amounts/mpt_amount.py (1)
MPTAmount(18-51)xrpl/models/currencies/mpt_currency.py (1)
MPTCurrency(28-63)xrpl/models/currencies/xrp.py (1)
XRP(26-91)xrpl/models/requests/tx.py (1)
Tx(20-87)xrpl/models/transactions/mptoken_issuance_create.py (1)
MPTokenIssuanceCreate(56-143)xrpl/models/transactions/signer_list_set.py (2)
SignerEntry(34-56)SignerListSet(61-156)
xrpl/asyncio/transaction/main.py (3)
xrpl/models/transactions/loan_set.py (1)
LoanSet(61-237)xrpl/models/requests/account_info.py (1)
AccountInfo(19-39)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)
🪛 GitHub Actions: Unit test
xrpl/asyncio/transaction/main.py
[error] 567-567: Command failed: poetry run mypy --strict --implicit-reexport xrpl. mypy error: Argument 2 to "_fetch_counterparty_signers_count" has incompatible type "str | None"; expected "str".
⏰ 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). (8)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.14)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.8)
- GitHub Check: Integration test (3.13)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
.ci-config/rippled.cfg (1)
200-208: Enablement of LendingProtocol amendment looks correctAdding
LendingProtocolunder the 2.5.0 amendments in[features]aligns the test rippled config with the new Lending Protocol transactions introduced in this PR. No issues from a config standpoint.
…r is present in both sides of LoanSet transaction. In this case, the presence of Counterparty field is optional in the LoanSet transaction
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/integration/transactions/test_lending_protocol.py (1)
445-451: Passclientargument for consistency.This call only passes 2 arguments but should pass 3 like all other calls in this file. Missing the
clientparameter will cause aTypeError.Apply this diff:
response = await sign_and_reliable_submission_async( AccountSet( account=loan_issuer.classic_address, set_flag=AccountSetAsfFlag.ASF_DEFAULT_RIPPLE, ), loan_issuer, + client, )
🧹 Nitpick comments (1)
tests/integration/transactions/test_lending_protocol.py (1)
108-109: Fix spelling in comments."transered" should be "transferred" (lines 108, 551, 754).
Also applies to: 551-551, 754-754
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/integration/transactions/test_lending_protocol.py(1 hunks)xrpl/asyncio/transaction/main.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- xrpl/asyncio/transaction/main.py
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/unit/models/transactions/test_account_delete.py:52-60
Timestamp: 2024-12-12T00:48:38.042Z
Learning: The credential ID format validation for xrpl-py is being tracked in issue #766 and should not be duplicated in other PRs.
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_cover_deposit.py:5-12
Timestamp: 2025-09-15T15:48:47.751Z
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
Repo: XRPLF/xrpl-py PR: 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
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_cover_withdraw.py:8-13
Timestamp: 2025-09-15T15:50:48.800Z
Learning: LoanBrokerCoverWithdraw allows withdrawal of First-Loss Capital in XRP, IOU, or MPT assets, similar to LoanBrokerCoverDeposit. Unlike LoanBrokerCoverClawback which rejects XRP amounts, both Deposit and Withdraw operations should allow XRP assets.
📚 Learning: 2025-09-15T15:48:47.751Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_cover_deposit.py:5-12
Timestamp: 2025-09-15T15:48:47.751Z
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
📚 Learning: 2024-11-01T16:20:50.192Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/integration/transactions/test_credential.py:61-63
Timestamp: 2024-11-01T16:20:50.192Z
Learning: In integration tests for xrpl-py, tests should only be testing the library, not rippled functionalities.
Applied to files:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2025-09-15T15:21:38.832Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 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: 2024-11-01T18:53:01.394Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/unit/models/requests/test_deposit_authorized.py:7-15
Timestamp: 2024-11-01T18:53:01.394Z
Learning: In `tests/unit/models/requests/test_deposit_authorized.py`, additional tests for invalid credentials are unnecessary because `rippled` handles those checks, and the `xrpl-py` library does not include such checks.
Applied to files:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2025-01-15T00:41:02.631Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 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-30T21:01:15.823Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/integration/transactions/test_credential.py:42-44
Timestamp: 2024-10-30T21:01:15.823Z
Learning: In this codebase, extensive unit tests are located in `rippled`, and additional tests are not required in this library.
Applied to files:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2025-02-05T21:42:42.425Z
Learnt from: achowdhry-ripple
Repo: XRPLF/xrpl-py PR: 803
File: tests/integration/transactions/test_trust_set.py:0-0
Timestamp: 2025-02-05T21:42:42.425Z
Learning: Failure cases for deep freeze functionality (e.g., setting deep freeze without regular freeze, clearing deep freeze while regular freeze is set) are handled by rippled server logic and don't need to be tested in the xrpl-py library.
Applied to files:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2025-09-15T15:54:25.836Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_set.py:158-181
Timestamp: 2025-09-15T15:54:25.836Z
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:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2024-10-30T20:34:35.451Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 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:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2024-10-30T20:45:02.301Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 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:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2025-09-15T15:50:48.800Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_cover_withdraw.py:8-13
Timestamp: 2025-09-15T15:50:48.800Z
Learning: LoanBrokerCoverWithdraw allows withdrawal of First-Loss Capital in XRP, IOU, or MPT assets, similar to LoanBrokerCoverDeposit. Unlike LoanBrokerCoverClawback which rejects XRP amounts, both Deposit and Withdraw operations should allow XRP assets.
Applied to files:
tests/integration/transactions/test_lending_protocol.py
⏰ 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). (7)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.14)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.8)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/integration/transactions/test_lending_protocol.py (1)
445-451: Missingclientargument in sign_and_reliable_submission_async call.This call is missing the
clientargument, which is inconsistent with all other calls in this file and will likely cause aTypeError. The past review flagged this but it appears to still be missing.response = await sign_and_reliable_submission_async( AccountSet( account=loan_issuer.classic_address, set_flag=AccountSetAsfFlag.ASF_DEFAULT_RIPPLE, ), loan_issuer, + client, )
🧹 Nitpick comments (4)
xrpl/core/binarycodec/definitions/definitions.json (1)
2422-2422: Minor formatting inconsistency.Line 2422 has
], [with extra spacing that differs from the rest of the file. This doesn't affect functionality but creates inconsistent formatting.- ], [ + ], + [xrpl/models/transactions/loan_set.py (1)
180-184: Clarify error message for hex data length.The error message states "less than 256 bytes" but
MAX_DATA_LENGTH = 512represents hex characters (256 bytes = 512 hex chars). Consider clarifying the message to match the sibling model patterns (e.g.,LoanBrokerSetand vault transactions).- 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)." + )xrpl/asyncio/transaction/main.py (2)
572-575: Add error handling for LedgerEntry response.If the
LedgerEntryrequest fails or returns unexpected data, accessingresult["node"]["Owner"]will raise aKeyError. Consider adding error handling for robustness.else: # The sfCounterparty field is optional if the counterparty is the # LoanBroker owner. Deduce the counterparty account from the LoanBroker # ID. loan_broker_object_info = await client._request_impl( LedgerEntry(index=loan_set.loan_broker_id) ) + if not loan_broker_object_info.is_successful(): + raise XRPLRequestFailureException( + loan_broker_object_info.result + ) counterparty_account = loan_broker_object_info.result["node"]["Owner"]
580-591: Use logging module instead of print() for warnings.Using
print()for warnings isn't consistent with library best practices. Consider using Python'sloggingmodule orwarnings.warn()to allow users to configure warning behavior.+import logging + +_logger = logging.getLogger(__name__) + if counterparty_signers_count > 1: - print( - ( - f"Warning: You are using autofill feature for the LoanSet " - f"transaction: {transaction.to_dict()}. The transaction fee " - "estimation is based on the number of signers in the " - "CounterpartySignature field. It might be possible to optimize " - "the fee further by considering the minimum quorum of signers." - "\nIf you prefer optimized transaction fee, please fill the fee" - " field manually." - ) - ) + _logger.warning( + "LoanSet fee autofill: fee estimation is based on total signers " + "(%d) rather than minimum quorum. Consider setting fee manually " + "for optimization.", + counterparty_signers_count, + )
📜 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)xrpl/asyncio/transaction/main.py(3 hunks)xrpl/core/binarycodec/definitions/definitions.json(9 hunks)xrpl/models/transactions/loan_pay.py(1 hunks)xrpl/models/transactions/loan_set.py(1 hunks)
🧰 Additional context used
🧠 Learnings (29)
📓 Common learnings
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_cover_deposit.py:5-12
Timestamp: 2025-09-15T15:48:47.751Z
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
Repo: XRPLF/xrpl-py PR: 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
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_cover_withdraw.py:8-13
Timestamp: 2025-09-15T15:50:48.800Z
Learning: LoanBrokerCoverWithdraw allows withdrawal of First-Loss Capital in XRP, IOU, or MPT assets, similar to LoanBrokerCoverDeposit. Unlike LoanBrokerCoverClawback which rejects XRP amounts, both Deposit and Withdraw operations should allow XRP assets.
📚 Learning: 2025-09-18T18:20:02.896Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_set.py:105-110
Timestamp: 2025-09-18T18:20:02.896Z
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/asyncio/transaction/main.pyxrpl/models/transactions/loan_pay.pyxrpl/models/transactions/loan_set.py
📚 Learning: 2024-11-04T19:43:00.465Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/integration/transactions/test_deposit_preauth.py:40-67
Timestamp: 2024-11-04T19:43:00.465Z
Learning: Negative test cases for credential validation should be included in unit tests rather than integration tests.
Applied to files:
xrpl/asyncio/transaction/main.py
📚 Learning: 2024-10-30T21:01:15.823Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/integration/transactions/test_credential.py:42-44
Timestamp: 2024-10-30T21:01:15.823Z
Learning: In this codebase, extensive unit tests are located in `rippled`, and additional tests are not required in this library.
Applied to files:
xrpl/asyncio/transaction/main.pytests/integration/transactions/test_lending_protocol.pyxrpl/models/transactions/loan_set.py
📚 Learning: 2025-09-15T15:48:47.751Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_cover_deposit.py:5-12
Timestamp: 2025-09-15T15:48:47.751Z
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:
xrpl/asyncio/transaction/main.pytests/integration/transactions/test_lending_protocol.pyxrpl/models/transactions/loan_set.py
📚 Learning: 2025-09-15T15:21:38.832Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 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/asyncio/transaction/main.pytests/integration/transactions/test_lending_protocol.pyxrpl/models/transactions/loan_set.pyxrpl/core/binarycodec/definitions/definitions.json
📚 Learning: 2024-11-01T20:24:17.202Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 765
File: xrpl/models/transactions/xchain_add_claim_attestation.py:82-88
Timestamp: 2024-11-01T20:24:17.202Z
Learning: Changing the type of `was_locking_chain_send` in `xrpl/models/transactions/xchain_add_claim_attestation.py` to `bool` is out of scope for the current PR.
Applied to files:
xrpl/asyncio/transaction/main.py
📚 Learning: 2024-10-30T20:36:02.813Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 759
File: xrpl/models/transactions/credential_delete.py:36-38
Timestamp: 2024-10-30T20:36:02.813Z
Learning: In `xrpl/models/transactions/credential_delete.py`, changing required field declarations from `credential_type: str = REQUIRED # type: ignore` to `credential_type: str = field(default=REQUIRED)` can cause mypy errors due to type incompatibility. It's acceptable to use `# type: ignore` in such cases in the `xrpl-py` codebase.
Applied to files:
xrpl/asyncio/transaction/main.pytests/integration/transactions/test_lending_protocol.pyxrpl/models/transactions/loan_set.py
📚 Learning: 2024-12-10T20:35:09.762Z
Learnt from: khancode
Repo: XRPLF/xrpl-py PR: 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/asyncio/transaction/main.pytests/integration/transactions/test_lending_protocol.pyxrpl/models/transactions/loan_pay.pyxrpl/models/transactions/loan_set.py
📚 Learning: 2024-10-30T20:32:03.246Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 759
File: xrpl/models/transactions/credential_accept.py:27-39
Timestamp: 2024-10-30T20:32:03.246Z
Learning: In the `xrpl` codebase, when defining required fields in dataclasses (e.g., `account: str = REQUIRED`), it's necessary to include `# type: ignore` to prevent `mypy` errors.
Applied to files:
xrpl/asyncio/transaction/main.pyxrpl/models/transactions/loan_set.py
📚 Learning: 2025-06-05T20:52:31.099Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 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: 2024-10-30T20:34:35.451Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 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:
tests/integration/transactions/test_lending_protocol.pyxrpl/models/transactions/loan_pay.pyxrpl/models/transactions/loan_set.py
📚 Learning: 2024-11-01T16:20:50.192Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/integration/transactions/test_credential.py:61-63
Timestamp: 2024-11-01T16:20:50.192Z
Learning: In integration tests for xrpl-py, tests should only be testing the library, not rippled functionalities.
Applied to files:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2024-11-01T18:53:01.394Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/unit/models/requests/test_deposit_authorized.py:7-15
Timestamp: 2024-11-01T18:53:01.394Z
Learning: In `tests/unit/models/requests/test_deposit_authorized.py`, additional tests for invalid credentials are unnecessary because `rippled` handles those checks, and the `xrpl-py` library does not include such checks.
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
Repo: XRPLF/xrpl-py PR: 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-02-05T21:42:42.425Z
Learnt from: achowdhry-ripple
Repo: XRPLF/xrpl-py PR: 803
File: tests/integration/transactions/test_trust_set.py:0-0
Timestamp: 2025-02-05T21:42:42.425Z
Learning: Failure cases for deep freeze functionality (e.g., setting deep freeze without regular freeze, clearing deep freeze while regular freeze is set) are handled by rippled server logic and don't need to be tested in the xrpl-py library.
Applied to files:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2025-09-15T15:54:25.836Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_set.py:158-181
Timestamp: 2025-09-15T15:54:25.836Z
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:
tests/integration/transactions/test_lending_protocol.pyxrpl/models/transactions/loan_set.py
📚 Learning: 2024-10-30T20:45:02.301Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 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:
tests/integration/transactions/test_lending_protocol.pyxrpl/models/transactions/loan_set.py
📚 Learning: 2025-09-15T15:50:48.800Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_cover_withdraw.py:8-13
Timestamp: 2025-09-15T15:50:48.800Z
Learning: LoanBrokerCoverWithdraw allows withdrawal of First-Loss Capital in XRP, IOU, or MPT assets, similar to LoanBrokerCoverDeposit. Unlike LoanBrokerCoverClawback which rejects XRP amounts, both Deposit and Withdraw operations should allow XRP assets.
Applied to files:
tests/integration/transactions/test_lending_protocol.py
📚 Learning: 2024-12-11T01:47:28.074Z
Learnt from: khancode
Repo: XRPLF/xrpl-py PR: 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-10-30T20:44:27.753Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 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: 2025-09-18T18:20:02.896Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_set.py:105-110
Timestamp: 2025-09-18T18:20:02.896Z
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: 2025-06-04T22:17:47.822Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 814
File: xrpl/models/requests/vault_info.py:16-31
Timestamp: 2025-06-04T22:17:47.822Z
Learning: In the xrpl-py library, Request classes do not perform client-side validation on their contents. The library relies on the rippled node to return appropriate error codes for malformed requests, maintaining a consistent pattern across all Request models.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2025-05-13T18:43:34.412Z
Learnt from: Patel-Raj11
Repo: XRPLF/xrpl-py PR: 840
File: xrpl/core/binarycodec/definitions/definitions.json:2594-2602
Timestamp: 2025-05-13T18:43:34.412Z
Learning: The `definitions.json` file in XRPL is a generated file and should not be directly edited. Constraints like maximum array lengths are enforced at the model level (e.g., in transaction models) rather than in the binary codec layer.
Applied to files:
xrpl/models/transactions/loan_set.pyxrpl/core/binarycodec/definitions/definitions.json
📚 Learning: 2025-09-18T18:20:02.896Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_set.py:105-110
Timestamp: 2025-09-18T18:20:02.896Z
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: 2024-12-12T00:48:38.042Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/unit/models/transactions/test_account_delete.py:52-60
Timestamp: 2024-12-12T00:48:38.042Z
Learning: The credential ID format validation for xrpl-py is being tracked in issue #766 and should not be duplicated in other PRs.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2025-02-11T21:11:19.326Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 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
📚 Learning: 2025-03-06T22:45:59.640Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 814
File: xrpl/core/binarycodec/types/number.py:77-97
Timestamp: 2025-03-06T22:45:59.640Z
Learning: The `to_bytes()` method in Python requires the `byteorder` parameter even for single-byte operations, despite endianness not functionally affecting single-byte representation. The XRPL Python library implementation of serialization methods like `add32` and `add64` need to specify this parameter.
Applied to files:
xrpl/models/transactions/loan_set.py
📚 Learning: 2025-04-07T23:32:27.184Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 824
File: tools/fetch_rippled_amendments.py:0-0
Timestamp: 2025-04-07T23:32:27.184Z
Learning: For CI/CD scripts in the xrpl-py repository, ckeshava prefers concise code over comprehensive error handling since developers can check the logs for error details.
Applied to files:
xrpl/models/transactions/loan_set.py
🧬 Code graph analysis (3)
xrpl/asyncio/transaction/main.py (6)
xrpl/models/transactions/loan_set.py (1)
LoanSet(66-242)xrpl/models/requests/simulate.py (2)
Simulate(18-100)to_dict(89-100)xrpl/models/transactions/transaction.py (2)
Transaction(184-549)to_dict(296-312)xrpl/models/requests/account_info.py (1)
AccountInfo(19-39)xrpl/models/requests/ledger_entry.py (1)
LedgerEntry(340-420)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-79)
xrpl/models/transactions/loan_pay.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(140-193)
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(140-193)xrpl/models/transactions/loan_broker_set.py (1)
_get_errors(73-112)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). (7)
- GitHub Check: Integration test (3.14)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.8)
- GitHub Check: Integration test (3.9)
🔇 Additional comments (11)
xrpl/models/transactions/loan_pay.py (1)
1-32: LGTM!The
LoanPaytransaction model follows established patterns in the codebase. The use ofAmounttype for theamountfield correctly supports XRP, IOU, and MPT assets as required by the Lending Protocol specification. The structure aligns with other transaction models (frozen dataclass, keyword-only init, required fields).xrpl/core/binarycodec/definitions/definitions.json (1)
183-192: LGTM - Lending Protocol binary codec definitions.The new fields and types for the Lending Protocol are properly added with correct
nthvalues, types, and serialization flags. Notable additions:
CounterpartySignaturewithisSigningField: falsecorrectly excludes signature data from signing- New transaction types (LoanSet, LoanPay, etc.) and ledger entry types (Loan, LoanBroker)
- Numeric fields for loan parameters (interest rates, fees, payment intervals) with proper bounds
Also applies to: 703-842, 1113-1132, 1483-1502, 2283-2302, 2363-2441, 2782-2791, 3398-3399, 3639-3647
xrpl/models/transactions/loan_set.py (2)
23-38: LGTM - CounterpartySignature model.The
CounterpartySignaturedataclass properly models the counterparty signature payload with optional fields for both single-sign (signing_pub_key,txn_signature) and multi-sign (signers) scenarios. UsingList[Signer]aligns with the standard Signer type.
64-242: LGTM - LoanSet transaction model.The
LoanSetmodel is well-structured with:
- Proper required fields (
loan_broker_id,principal_requested)- Comprehensive validation for rate bounds, payment intervals, and hex data format
- Consistent patterns with other transaction models in the codebase
Per retrieved learnings, comprehensive field validation is handled by rippled, so the structural validations here are appropriate.
xrpl/asyncio/transaction/main.py (2)
467-494: LGTM - Counterparty signer count helper.The
_fetch_counterparty_signers_countfunction correctly queriesAccountInfowithsigner_lists=Trueand safely defaults to 1 when no signer list is configured. This aligns with the fee calculation requirements for multi-signed counterparty scenarios.
552-593: LGTM - LOAN_SET fee calculation logic.The fee calculation correctly handles both scenarios:
- When
counterparty_signatureis present: uses signer count from the signature- When absent: fetches signer count from counterparty account or LoanBroker owner
The formula
base_fee += net_fee * signer_countcorrectly accounts for additional signature costs per XRPL fee structure.tests/integration/transactions/test_lending_protocol.py (5)
46-184: LGTM - XRP lifecycle test.The test thoroughly covers the lending protocol lifecycle with XRP assets: vault creation, loan broker setup, deposit, LoanSet with counterparty signature, loan retrieval, deletion attempt validation, LoanManage, and LoanPay. The use of
LEDGER_ACCEPT_REQUESTafter direct submit ensures ledger advancement.
185-317: LGTM - Multisign autofill test.This test validates the fee autofill logic for multisigned counterparty accounts. The fee assertion
str(200 + NUM_SIGNERS * 200)correctly verifies the per-signer fee calculation.
318-429: LGTM - Counterparty-as-owner scenario test.This test covers the edge case where the borrower is also the LoanBroker owner (same account). The fee assertion
str(200 + 200)correctly accounts for the additional counterparty signature.
431-628: IOU lifecycle test has missing client argument (see earlier comment).Otherwise, the test structure correctly covers IOU asset handling with trustlines, payments, and the full lending protocol lifecycle.
629-828: LGTM - MPT lifecycle test.The test comprehensively covers the lending protocol with MPT assets, including MPToken issuance, authorization, payments, vault creation, and the full loan lifecycle.
3 participants
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.