Add support for Token Escrow - XLS-85d #853
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe changes add support for Multi-Party Token (MPT) escrows by enabling the TokenEscrow amendment, introducing a new account flag for trustline locking, enhancing escrow creation validation to require positive amounts, and expanding integration and unit tests to cover MPT escrow scenarios. The changelog and configuration were updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WalletUtils
participant Ledger
participant Escrow
participant MPToken
Client->>WalletUtils: Create issuer, source, destination wallets
Client->>MPToken: Issue MPToken with escrow/transfer flags
Client->>Ledger: Authorize source wallet for MPToken
Client->>Ledger: Transfer MPToken to source
Client->>Escrow: Create escrow with MPToken amount
Escrow->>Ledger: Record escrow, MPToken, and issuance objects
Client->>Escrow: Wait until finish time, finish escrow
Escrow->>Ledger: Transfer MPToken to destination
Client->>Ledger: Query for MPToken ledger object (destination)
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Patel-Raj11
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/models/transactions/test_escrow_create.py (1)
37-37: Fix typo in method name.The method name has a typo: "postive" should be "positive".
- def test_amount_not_postive(self): + def test_amount_not_positive(self):
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.ci-config/rippled.cfg(1 hunks)CHANGELOG.md(1 hunks)tests/integration/transactions/test_escrow_create.py(2 hunks)tests/unit/models/transactions/test_escrow_create.py(2 hunks)xrpl/models/transactions/account_set.py(1 hunks)xrpl/models/transactions/escrow_create.py(4 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: mvadari
PR: XRPLF/xrpl-py#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.
Learnt from: mvadari
PR: XRPLF/xrpl-py#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
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.
xrpl/models/transactions/account_set.py (3)
Learnt from: mvadari
PR: XRPLF/xrpl-py#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.
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:78-84
Timestamp: 2024-12-11T01:47:28.074Z
Learning: In the `xrpl-py` codebase, within the models (e.g., in `xrpl/models/transactions/`), flag validation checks are not performed; flag checks are handled elsewhere outside of the models.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/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.
.ci-config/rippled.cfg (3)
Learnt from: mvadari
PR: XRPLF/xrpl-py#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.
Learnt from: Patel-Raj11
PR: XRPLF/xrpl-py#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.
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.
tests/unit/models/transactions/test_escrow_create.py (11)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: tests/unit/models/transactions/test_credential_create.py:107-114
Timestamp: 2024-10-30T20:26:32.925Z
Learning: In `tests/unit/models/transactions/test_credential_create.py`, when testing invalid inputs, avoid adding assertions on specific error messages if it increases the complexity and maintenance overhead of the tests.
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]`.
Learnt from: mvadari
PR: XRPLF/xrpl-py#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.
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.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Learnt from: mvadari
PR: XRPLF/xrpl-py#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.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/unit/models/transactions/test_credential_create.py:13-22
Timestamp: 2024-11-01T20:41:08.207Z
Learning: In Python test files in the xrpl-py codebase (e.g., `tests/unit/models/transactions/test_credential_create.py`), docstrings are not required for test classes and methods if their names are self-explanatory enough.
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:78-84
Timestamp: 2024-12-11T01:47:28.074Z
Learning: In the `xrpl-py` codebase, within the models (e.g., in `xrpl/models/transactions/`), flag validation checks are not performed; flag checks are handled elsewhere outside of the models.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#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.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#814
File: tests/integration/transactions/test_sav.py:71-77
Timestamp: 2025-03-06T22:47:16.581Z
Learning: For test files in the xrpl-py repository, exception handling is considered not critical, and the preference is to let tests fail with natural errors rather than adding explicit error handling.
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.
tests/integration/transactions/test_escrow_create.py (13)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: tests/unit/models/transactions/test_credential_create.py:107-114
Timestamp: 2024-10-30T20:26:32.925Z
Learning: In `tests/unit/models/transactions/test_credential_create.py`, when testing invalid inputs, avoid adding assertions on specific error messages if it increases the complexity and maintenance overhead of the tests.
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]`.
Learnt from: mvadari
PR: XRPLF/xrpl-py#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.
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.
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.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/unit/models/transactions/test_credential_create.py:13-22
Timestamp: 2024-11-01T20:41:08.207Z
Learning: In Python test files in the xrpl-py codebase (e.g., `tests/unit/models/transactions/test_credential_create.py`), docstrings are not required for test classes and methods if their names are self-explanatory enough.
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.
Learnt from: mvadari
PR: XRPLF/xrpl-py#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.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Learnt from: mvadari
PR: XRPLF/xrpl-py#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.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#758
File: xrpl/transaction/__init__.py:8-8
Timestamp: 2024-10-17T17:45:46.828Z
Learning: In the xrpl-py project, importing private functions (indicated by a leading underscore) for integration testing purposes is acceptable when the methods are used internally.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/integration/transactions/test_credential.py:31-37
Timestamp: 2024-11-04T19:41:04.808Z
Learning: The `is_cred_object_present` function in `tests/integration/transactions/test_credential.py` assumes that the fields `'Issuer'`, `'Subject'`, and `'CredentialType'` are always present in the account object dictionaries within `account_objects`.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: tests/integration/transactions/test_credential.py:37-108
Timestamp: 2024-10-30T20:28:45.649Z
Learning: In integration tests, setup and teardown are performed using the `CredentialCreate` and `CredentialDelete` transactions, and we prefer not to abstract these transactions into separate utility functions.
xrpl/models/transactions/escrow_create.py (9)
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]`.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#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.
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:78-84
Timestamp: 2024-12-11T01:47:28.074Z
Learning: In the `xrpl-py` codebase, within the models (e.g., in `xrpl/models/transactions/`), flag validation checks are not performed; flag checks are handled elsewhere outside of the models.
Learnt from: mvadari
PR: XRPLF/xrpl-py#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.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: tests/unit/models/transactions/test_credential_create.py:107-114
Timestamp: 2024-10-30T20:26:32.925Z
Learning: In `tests/unit/models/transactions/test_credential_create.py`, when testing invalid inputs, avoid adding assertions on specific error messages if it increases the complexity and maintenance overhead of the tests.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#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.
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.
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.
🧬 Code Graph Analysis (2)
tests/unit/models/transactions/test_escrow_create.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/escrow_create.py (1)
EscrowCreate(19-97)xrpl/models/base_model.py (1)
is_valid(295-302)
xrpl/models/transactions/escrow_create.py (1)
xrpl/models/amounts/amount.py (3)
get_amount_value(59-75)is_issued_currency(31-42)is_mpt(45-56)
🔇 Additional comments (15)
CHANGELOG.md (1)
10-11: LGTM! Clean changelog entry.The changelog entry is properly formatted and correctly placed in the "Unreleased" section to document the Token Escrow feature addition.
.ci-config/rippled.cfg (1)
205-205: LGTM! Amendment enablement is correctly placed.The TokenEscrow amendment is properly added under the "2.5.0 Amendments" section, following the established pattern in the configuration file.
xrpl/models/transactions/account_set.py (1)
109-110: LGTM! New account flag is properly implemented.The
ASF_ALLOW_TRUSTLINE_LOCKINGflag is correctly added with the next sequential value (17) and includes clear documentation explaining its purpose for Token Escrow functionality.xrpl/models/transactions/escrow_create.py (4)
10-10: LGTM! Import statement properly adds required utility functions.The import correctly adds the amount utility functions needed for the new validation logic supporting XRP, IOU, and MPT amounts.
27-29: LGTM! Documentation accurately reflects expanded functionality.The docstring clearly documents that the amount field now supports XRP, IOU tokens, and MPT, and correctly states the positive value requirement.
53-53: LGTM! Documentation correctly reflects conditional requirement.The updated docstring accurately documents that
cancel_afteris required when creating escrows with IOU or MPT amounts.
87-95: LGTM! Validation logic is well-implemented.The new validation rules are correctly implemented:
get_amount_value()properly handles all amount types (XRP, IOU, MPT) for positive validation- The conditional requirement for
cancel_afterwith IOU/MPT amounts aligns with the XLS-85d specification- Error messages are clear and descriptive
tests/unit/models/transactions/test_escrow_create.py (5)
3-3: LGTM! Import correctly adds required amount types for testing.The import statement properly adds
IssuedCurrencyAmountandMPTAmountneeded for the new validation tests.
7-8: LGTM! Constants improve test maintainability.Adding constants for source and destination addresses improves code reusability and maintainability across test methods.
37-52: LGTM! Test correctly validates positive amount requirement.The test properly verifies that zero-value amounts trigger the positive amount validation error. The test structure and error message assertion follow the codebase conventions.
54-68: LGTM! Test correctly validates cancel_after requirement for MPT.The test properly verifies that MPT amounts require the
cancel_afterfield. The error message assertion confirms the expected validation behavior.
70-80: LGTM! Test validates successful escrow creation with MPT.The test correctly verifies that a valid
EscrowCreatetransaction with MPT amount and propercancel_afterpasses validation. This provides positive test coverage for the new functionality.tests/integration/transactions/test_escrow_create.py (3)
3-21: LGTM! Well-organized imports for MPToken escrow testing.The new imports are properly structured and necessary for testing the Token Escrow feature with MPToken support.
53-201: Comprehensive test coverage for MPToken escrow lifecycle.The test thoroughly validates the entire MPToken escrow flow including:
- Proper setup of issuer, source, and destination wallets
- MPToken creation with appropriate flags
- Authorization and transfer workflows
- Escrow creation and finish operations
- Ledger object state verification at each step
Well done on the thorough test implementation!
203-277: Well-designed negative test case for permission validation.The test effectively validates that MPToken escrow creation fails with
tecNO_PERMISSIONwhen theTF_MPT_CAN_ESCROWflag is not set. The clear comment explaining the expected failure reason enhances test maintainability.
ckeshava
requested changes
Jul 7, 2025
Co-authored-by: Chenna Keshava B S <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
xrpl/models/transactions/escrow_create.py (1)
10-10: Remove unused imports.The static analysis correctly identifies that
is_issued_currencyandis_mptare imported but not used in the code.-from xrpl.models.amounts import Amount, get_amount_value, is_issued_currency, is_mpt +from xrpl.models.amounts import Amount, get_amount_value
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unit/models/transactions/test_escrow_create.py(2 hunks)xrpl/models/transactions/escrow_create.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/models/transactions/test_escrow_create.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mvadari
PR: XRPLF/xrpl-py#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.
Learnt from: mvadari
PR: XRPLF/xrpl-py#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
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.
xrpl/models/transactions/escrow_create.py (12)
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]`.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#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.
Learnt from: Patel-Raj11
PR: XRPLF/xrpl-py#853
File: tests/integration/transactions/test_escrow_create.py:169-174
Timestamp: 2025-07-03T14:11:37.423Z
Learning: In XRPL integration tests for escrow transactions, when waiting for an escrow to become finishable, the loop condition `close_time <= finish_after` is correct because EscrowFinish transactions require the ledger's close_time to be beyond (greater than) the finish_after field. The loop must continue until close_time is actually greater than finish_after to ensure the escrow can be successfully finished.
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.
Learnt from: mvadari
PR: XRPLF/xrpl-py#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.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#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.
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:78-84
Timestamp: 2024-12-11T01:47:28.074Z
Learning: In the `xrpl-py` codebase, within the models (e.g., in `xrpl/models/transactions/`), flag validation checks are not performed; flag checks are handled elsewhere outside of the models.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: tests/unit/models/transactions/test_credential_create.py:107-114
Timestamp: 2024-10-30T20:26:32.925Z
Learning: In `tests/unit/models/transactions/test_credential_create.py`, when testing invalid inputs, avoid adding assertions on specific error messages if it increases the complexity and maintenance overhead of the tests.
Learnt from: mvadari
PR: XRPLF/xrpl-py#773
File: xrpl/models/transactions/permissioned_domain_set.py:22-22
Timestamp: 2024-11-20T21:16:43.491Z
Learning: In the `xrpl.models.transactions.PermissionedDomainSet` class, the `domain_id` field is optional in the `PermissionedDomainSet` transaction.
Learnt from: mvadari
PR: XRPLF/xrpl-py#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
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.
🪛 Ruff (0.11.9)
xrpl/models/transactions/escrow_create.py
10-10: xrpl.models.amounts.is_issued_currency imported but unused
Remove unused import
(F401)
10-10: xrpl.models.amounts.is_mpt imported but unused
Remove unused import
(F401)
⏰ 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: Integration test (3.8)
🔇 Additional comments (3)
xrpl/models/transactions/escrow_create.py (3)
27-29: LGTM: Clear documentation update for multi-token support.The documentation update accurately reflects the Token Escrow feature's support for XRP, IOU tokens, and MPT amounts.
52-52: LGTM: Minor grammatical improvement.The wording fix improves clarity of the documentation.
87-88: LGTM: Proper validation for positive amounts.The validation correctly ensures that escrow amounts must be positive, which is essential for all token types (XRP, IOU, MPT).
ckeshava
approved these changes
Jul 8, 2025
There was a problem hiding this comment.
Hello Raj, some changes are being proposed for this amendment here. Depending on the consensus of the community, we might need to make appropriate changes in this client library as well.
For the existing rippled code, this PR looks good to me.
khancode
reviewed
Jul 10, 2025
mvadari
reviewed
Jul 10, 2025
Comment on lines
62
to
63
| destination = Wallet.create() | ||
| await fund_wallet_async(destination) |
There was a problem hiding this comment.
Can use DESTINATION here too
mvadari
reviewed
Jul 10, 2025
| @test_async_and_sync(globals()) | ||
| async def test_mpt_based_escrow(self, client): | ||
| # Create issuer, source, and destination wallets. | ||
| issuer = Wallet.create() |
There was a problem hiding this comment.
Can all this setup be done independently in setupTests or something (I might be misremembering the function name)? That way it doesn't have to execute 4 times
There was a problem hiding this comment.
@mvadari I want to issue two different types of MPT's (with difference flags) for test_mpt_based_escrow and test_mpt_based_escrow_failure. Using setUpClass that executes only once will not help here. Do you see any other benefit of moving it?
There was a problem hiding this comment.
The way the tests run, it runs each test once per client (Sync/Async, Json/Websocket) so each test actually runs 4 times under the hood. So you could still set up 2 MPTs in setupClass and it would still save.
There was a problem hiding this comment.
I have moved account and MPT setup to setUpClass in af8a450.
Haven't reused WALLET AND DESTINATION since, MPT ledger objects were being tested in other MPT specific IT's and they were being interfered by MPT ledger objects created through EscrowFinish transaction.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/integration/transactions/test_escrow_create.py (1)
129-129: Minor typo in assertion message.There's a small typo in the assertion message.
- "No MPTOKEN object with expected MPTokenIssuanceID amd LockedAmount found", + "No MPTOKEN object with expected MPTokenIssuanceID and LockedAmount found",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/integration/it_utils.py(3 hunks)tests/integration/transactions/test_escrow_create.py(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mvadari
PR: XRPLF/xrpl-py#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.
Learnt from: mvadari
PR: XRPLF/xrpl-py#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
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.
tests/integration/it_utils.py (11)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: tests/unit/models/transactions/test_credential_create.py:107-114
Timestamp: 2024-10-30T20:26:32.925Z
Learning: In `tests/unit/models/transactions/test_credential_create.py`, when testing invalid inputs, avoid adding assertions on specific error messages if it increases the complexity and maintenance overhead of the tests.
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]`.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/integration/transactions/test_credential.py:31-37
Timestamp: 2024-11-04T19:41:04.808Z
Learning: The `is_cred_object_present` function in `tests/integration/transactions/test_credential.py` assumes that the fields `'Issuer'`, `'Subject'`, and `'CredentialType'` are always present in the account object dictionaries within `account_objects`.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: tests/integration/transactions/test_credential.py:37-108
Timestamp: 2024-10-30T20:28:45.649Z
Learning: In integration tests, setup and teardown are performed using the `CredentialCreate` and `CredentialDelete` transactions, and we prefer not to abstract these transactions into separate utility functions.
Learnt from: mvadari
PR: XRPLF/xrpl-py#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.
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.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#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.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#758
File: xrpl/transaction/__init__.py:8-8
Timestamp: 2024-10-17T17:45:46.828Z
Learning: In the xrpl-py project, importing private functions (indicated by a leading underscore) for integration testing purposes is acceptable when the methods are used internally.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#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.
Learnt from: mvadari
PR: XRPLF/xrpl-py#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.
tests/integration/transactions/test_escrow_create.py (21)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: tests/unit/models/transactions/test_credential_create.py:107-114
Timestamp: 2024-10-30T20:26:32.925Z
Learning: In `tests/unit/models/transactions/test_credential_create.py`, when testing invalid inputs, avoid adding assertions on specific error messages if it increases the complexity and maintenance overhead of the tests.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: tests/integration/transactions/test_credential.py:37-108
Timestamp: 2024-10-30T20:28:45.649Z
Learning: In integration tests, setup and teardown are performed using the `CredentialCreate` and `CredentialDelete` transactions, and we prefer not to abstract these transactions into separate utility functions.
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.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/unit/models/transactions/test_credential_create.py:13-22
Timestamp: 2024-11-01T20:41:08.207Z
Learning: In Python test files in the xrpl-py codebase (e.g., `tests/unit/models/transactions/test_credential_create.py`), docstrings are not required for test classes and methods if their names are self-explanatory enough.
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.
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]`.
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.
Learnt from: mvadari
PR: XRPLF/xrpl-py#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.
Learnt from: mvadari
PR: XRPLF/xrpl-py#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.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Learnt from: Patel-Raj11
PR: XRPLF/xrpl-py#853
File: tests/integration/transactions/test_escrow_create.py:169-174
Timestamp: 2025-07-03T14:11:37.423Z
Learning: In XRPL integration tests for escrow transactions, when waiting for an escrow to become finishable, the loop condition `close_time <= finish_after` is correct because EscrowFinish transactions require the ledger's close_time to be beyond (greater than) the finish_after field. The loop must continue until close_time is actually greater than finish_after to ensure the escrow can be successfully finished.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#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.
Learnt from: mvadari
PR: XRPLF/xrpl-py#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
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.
Learnt from: mvadari
PR: XRPLF/xrpl-py#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.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#814
File: tests/integration/transactions/test_sav.py:71-77
Timestamp: 2025-03-06T22:47:16.581Z
Learning: For test files in the xrpl-py repository, exception handling is considered not critical, and the preference is to let tests fail with natural errors rather than adding explicit error handling.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/unit/models/requests/test_ledger_entry.py:29-36
Timestamp: 2024-12-12T00:36:13.410Z
Learning: For basic dataclass testing in xrpl-py, simple initialization with sample values is sufficient when the main goal is to verify the validation logic works. Extensive property verification and negative test cases are unnecessary.
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl-py#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.
Learnt from: mvadari
PR: XRPLF/xrpl-py#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.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#758
File: xrpl/transaction/__init__.py:8-8
Timestamp: 2024-10-17T17:45:46.828Z
Learning: In the xrpl-py project, importing private functions (indicated by a leading underscore) for integration testing purposes is acceptable when the methods are used internally.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/integration/transactions/test_credential.py:31-37
Timestamp: 2024-11-04T19:41:04.808Z
Learning: The `is_cred_object_present` function in `tests/integration/transactions/test_credential.py` assumes that the fields `'Issuer'`, `'Subject'`, and `'CredentialType'` are always present in the account object dictionaries within `account_objects`.
🧬 Code Graph Analysis (1)
tests/integration/transactions/test_escrow_create.py (8)
tests/integration/it_utils.py (4)
create_mpt_token_and_authorize_source(553-595)fund_wallet(109-117)sign_and_reliable_submission_async(191-223)test_async_and_sync(268-363)xrpl/models/transactions/escrow_create.py (1)
EscrowCreate(19-90)xrpl/models/transactions/escrow_finish.py (1)
EscrowFinish(22-78)xrpl/models/transactions/mptoken_issuance_create.py (1)
MPTokenIssuanceCreateFlag(19-31)xrpl/models/amounts/mpt_amount.py (1)
MPTAmount(18-51)xrpl/models/requests/account_objects.py (2)
AccountObjects(47-70)AccountObjectType(19-42)xrpl/models/response.py (1)
ResponseStatus(23-27)xrpl/wallet/main.py (4)
Wallet(16-289)create(121-135)classic_address(34-41)address(24-29)
🪛 Ruff (0.11.9)
tests/integration/it_utils.py
557-557: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
⏰ 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: semgrep-cloud-platform/scan
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.8)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.10)
🔇 Additional comments (4)
tests/integration/it_utils.py (1)
553-595: Well-structured utility function for MPT setup.The function effectively encapsulates the complex workflow of creating MPT issuances, authorizing wallets, and distributing tokens. This provides a clean interface for integration tests that need MPT functionality.
tests/integration/transactions/test_escrow_create.py (3)
29-58: Well-structured test setup for MPT escrow scenarios.The setUpClass method efficiently creates the necessary wallets and MPT issuances with different permission flags, enabling comprehensive testing of both successful and failed escrow scenarios.
79-186: Comprehensive MPT escrow lifecycle test.The test thoroughly validates the complete MPT escrow workflow:
- Creates escrow with MPT amount
- Verifies ledger object creation (Escrow, MPToken, MPTokenIssuance)
- Waits for finish time using the correct loop condition
- Executes escrow finish
- Confirms destination wallet receives the MPT
The test coverage is excellent and follows established patterns in the codebase.
187-214: Effective negative test case for permission validation.The test properly validates that MPT escrow creation fails with
tecNO_PERMISSIONwhen the MPT lacks the required escrow permission flag. This ensures the permission system works correctly.
khancode
approved these changes
Jul 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
This PR implements Token Escrow feature for xrpl-py.
XLS spec: https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0085d-token-escrow
c++ implementation: XRPLF/rippled#5185
Context of Change
Type of Change
Did you update CHANGELOG.md?
Test Plan
Added unit and integration test cases.