XLS 85d: Token Escrow #3027
XLS 85d: Token Escrow #3027
Conversation
…udes models, unit-tests and integ-tests for the associated transactions
|
""" WalkthroughThe changes add support for IOU and MPT token escrows in the XRPL codebase. This includes updating transaction models, ledger objects, and validation logic to accept new amount types, introducing related flags, and expanding integration tests. Configuration and changelog files are updated to reflect the new TokenEscrow amendment. Changes
Possibly related issues
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn EBADENGINE Unsupported engine { ✨ 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/xrpl/src/models/ledger/Escrow.ts (1)
4-5: Update the interface description to reflect token support.The interface description still refers only to "held payment of XRP" but the implementation now supports IOUs and MPTs as well. Consider updating the description to be more inclusive.
/** - * The Escrow object type represents a held payment of XRP waiting to be + * The Escrow object type represents a held payment of XRP, IOU, or MPT waiting to be * executed or canceled. * * @category Ledger Entries */packages/xrpl/HISTORY.md (1)
10-12: Add direct link to the XLS-85d specification for traceabilityIncluding the canonical URL helps readers quickly jump to the amendment details and keeps the changelog style consistent with earlier entries that hyperlink new features.
-* Implement support for XLS-85d Token Escrow amendment. +* Implement support for [XLS-85d Token Escrow](https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0085d) amendment.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.ci-config/rippled.cfg(1 hunks)packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/ledger/AccountRoot.ts(2 hunks)packages/xrpl/src/models/ledger/Escrow.ts(1 hunks)packages/xrpl/src/models/ledger/MPToken.ts(1 hunks)packages/xrpl/src/models/ledger/MPTokenIssuance.ts(1 hunks)packages/xrpl/src/models/transactions/accountSet.ts(1 hunks)packages/xrpl/src/models/transactions/escrowCreate.ts(3 hunks)packages/xrpl/test/integration/transactions/escrowCreate.test.ts(2 hunks)packages/xrpl/test/models/escrowCreate.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/xrpl/test/models/escrowCreate.test.ts (3)
weback.test.config.js (1)
assert(2-2)packages/xrpl/src/models/transactions/escrowCreate.ts (1)
validateEscrowCreate(60-105)packages/xrpl/src/errors.ts (1)
ValidationError(156-156)
packages/xrpl/src/models/transactions/escrowCreate.ts (3)
packages/xrpl/src/models/common/index.ts (1)
MPTAmount(27-30)packages/xrpl/src/models/transactions/common.ts (1)
Account(173-173)packages/xrpl/src/errors.ts (1)
ValidationError(156-156)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: browser (22.x)
- GitHub Check: unit (22.x)
- GitHub Check: integration (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: unit (20.x)
🔇 Additional comments (9)
.ci-config/rippled.cfg (1)
190-194: Confirm amendment availability on all CI nodes.
TokenEscrowis toggled on, but the container image has to include a rippled version ≥ 2.5.0 with that amendment compiled in. Otherwise the node will crash during start-up.
Double-check the Docker image tag used in CI and, if necessary, bump it or pin a commit that contains the amendment.packages/xrpl/src/models/transactions/accountSet.ts (1)
64-66: LGTM – newasfAllowTrustLineLockingflag added cleanly.Value
17does not clash with existing enum entries, and validation logic further down already relies on the enum, so no extra changes are required.packages/xrpl/src/models/ledger/Escrow.ts (1)
65-75: LGTM! Well-documented escrow properties for token support.The new
TransferRateandIssuerNodeproperties are appropriately designed as optional fields with clear documentation explaining their role in token escrow functionality.packages/xrpl/src/models/ledger/AccountRoot.ts (1)
150-154: LGTM! Proper flag implementation for token escrow control.The new
lsfAllowTrustLineLockingflag is correctly implemented:
- Uses the next available bit value (0x40000000)
- Consistent documentation between interface and enum
- Follows established naming patterns
- Provides clear explanation of its purpose for IOU escrows
Also applies to: 219-223
packages/xrpl/test/models/escrowCreate.test.ts (1)
181-204: LGTM! Comprehensive test coverage for IOU escrow validation.The new test case effectively validates the mandatory
CancelAfterrequirement for IOU escrows:
- Properly constructs an IOU Amount object
- Tests both specific and generic validation functions
- Verifies the correct error message is thrown
- Follows existing test patterns in the file
packages/xrpl/src/models/transactions/escrowCreate.ts (3)
2-2: LGTM! Proper import for MPT support.The
MPTAmountimport enables escrow support for Multi-Party Tokens as required by the XLS-85d specification.
22-27: LGTM! Enhanced documentation and type support.The updated documentation clearly explains that
Amountnow supports XRP, IOU tokens, and MPT, and the type union correctly reflects this expanded functionality.
67-77: LGTM! Robust validation logic for token escrow requirements.The validation correctly:
- Accepts both string (XRP) and object (IOU/MPT) Amount types
- Enforces the mandatory
CancelAfterfield for IOU/MPT escrows- Provides clear, descriptive error messages
- Maintains backward compatibility for XRP escrows
packages/xrpl/test/integration/transactions/escrowCreate.test.ts (1)
75-208: LGTM! Comprehensive integration test for IOU escrow lifecycle.This integration test provides excellent coverage of the token escrow feature:
Strengths:
- Tests the complete workflow from setup to completion
- Properly configures issuer permissions with
asfAllowTrustLineLocking- Sets up necessary trust lines and IOU transfers
- Validates escrow object creation and properties
- Tests ledger time progression and escrow finishing
- Verifies proper cleanup (escrow object removal)
Test Structure:
- Well-organized with clear step-by-step comments
- Uses appropriate test utilities and helper functions
- Includes proper assertions at each stage
This test effectively validates the XLS-85d Token Escrow implementation end-to-end.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/xrpl/test/integration/transactions/escrowCreate.test.ts (1)
208-341: Consider refactoring to reduce code duplication.While this test correctly validates IOU escrow cancellation, it duplicates most of the setup logic from the previous test (lines 73-206). The only differences are in the final steps (EscrowCancel vs EscrowFinish and different timing).
Consider extracting the common setup logic into a helper function to improve maintainability:
async function setupIOUEscrowTest(testContext: XrplIntegrationTestContext) { const wallet1 = await generateFundedWallet(testContext.client) const wallet2 = await generateFundedWallet(testContext.client) // Common setup steps 1-4... return { wallet1, wallet2, txn_response, CLOSE_TIME } }This would make both tests more focused on their specific behavior (finish vs cancel) while maintaining the same test coverage.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xrpl/test/integration/transactions/escrowCreate.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2873
File: packages/xrpl/test/integration/transactions/trustSet.test.ts:0-0
Timestamp: 2025-01-31T17:46:25.375Z
Learning: For the XRPL implementation, extensive test cases for deep freeze behavior (high/low side interactions, clearing flags, etc.) are maintained in the C++ implementation and don't need to be duplicated in the JavaScript implementation.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts:29-118
Timestamp: 2024-12-06T19:25:15.376Z
Learning: In the XRPLF/xrpl.js TypeScript client library, when writing tests (e.g., in `packages/xrpl/test/integration/transactions/`), we generally do not need to test rippled server behaviors, because those behaviors are covered by rippled's own integration and unit tests.
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71
Timestamp: 2024-12-06T18:44:55.095Z
Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3027
File: packages/xrpl/src/models/ledger/MPTokenIssuance.ts:13-16
Timestamp: 2025-06-26T17:25:36.405Z
Learning: In the XRPL ecosystem, type choices for amount fields (like `number` vs `string`) in ledger objects such as `LockedAmount` vs `OutstandingAmount` in `MPTokenIssuance` are deliberate design decisions made across multiple products and cannot be changed unilaterally by individual contributors.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2873
File: packages/xrpl/src/models/transactions/trustSet.ts:33-36
Timestamp: 2025-01-08T13:08:52.688Z
Learning: For trust-set transactions in XRPL, validation rules for flags should be implemented comprehensively rather than cherry-picking specific rules, as there are many interdependent validation rules associated with these flags.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2812
File: packages/xrpl/test/integration/transactions/payment.test.ts:41-43
Timestamp: 2024-10-30T01:05:33.583Z
Learning: In tests involving fee calculations for XRPL, avoid using default fee amounts. If unable to retrieve fee values from the latest validated ledger, throw an error instead of defaulting to a specific amount.
packages/xrpl/test/integration/transactions/escrowCreate.test.ts (10)
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `@ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts:29-118
Timestamp: 2024-12-06T19:25:15.376Z
Learning: In the XRPLF/xrpl.js TypeScript client library, when writing tests (e.g., in `packages/xrpl/test/integration/transactions/`), we generally do not need to test rippled server behaviors, because those behaviors are covered by rippled's own integration and unit tests.
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71
Timestamp: 2024-12-06T18:44:55.095Z
Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2873
File: packages/xrpl/test/integration/transactions/trustSet.test.ts:0-0
Timestamp: 2025-01-31T17:46:25.375Z
Learning: For the XRPL implementation, extensive test cases for deep freeze behavior (high/low side interactions, clearing flags, etc.) are maintained in the C++ implementation and don't need to be duplicated in the JavaScript implementation.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/clawback.test.ts:0-0
Timestamp: 2025-02-12T23:28:55.377Z
Learning: The `validate` function in xrpl.js is synchronous and should be tested using `assert.doesNotThrow` rather than async assertions.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2801
File: packages/xrpl/test/models/Batch.test.ts:0-0
Timestamp: 2025-04-16T15:22:45.633Z
Learning: Using `as any` type assertions is acceptable in test files for the XRPL.js project, as strict typing is not required for test code.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/src/models/transactions/common.ts:18-18
Timestamp: 2025-01-16T04:11:37.316Z
Learning: Test libraries like chai should not be used in source code. For runtime checks in browser-compatible code, use vanilla JS error throwing instead of assertion libraries.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/src/models/transactions/common.ts:0-0
Timestamp: 2025-01-16T04:26:36.757Z
Learning: Test libraries like chai should not be used in source code. Use existing validation functions where available, or implement custom validation using ValidationError for runtime checks.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2801
File: packages/xrpl/test/wallet/batchSigner.test.ts:0-0
Timestamp: 2025-04-16T15:28:21.204Z
Learning: In the XRPL.js library, hardcoded seeds in test files are acceptable as they don't represent protected data or real funds - they're only used for consistent test behavior.
🧬 Code Graph Analysis (1)
packages/xrpl/test/integration/transactions/escrowCreate.test.ts (4)
packages/xrpl/test/integration/utils.ts (4)
generateFundedWallet(182-186)testTransaction(240-290)waitForAndForceProgressLedgerTime(342-370)sendLedgerAccept(34-36)packages/xrpl/src/models/transactions/accountSet.ts (1)
AccountSet(133-166)packages/xrpl/src/models/transactions/index.ts (1)
AccountSetAsfFlags(11-11)packages/xrpl/src/models/transactions/escrowCreate.ts (1)
EscrowCreate(19-52)
🔇 Additional comments (3)
packages/xrpl/test/integration/transactions/escrowCreate.test.ts (3)
3-11: Updated imports look correct for Token Escrow functionality.The additional imports for
AccountSet,AccountSetAsfFlags,TrustSet,Payment,EscrowFinish, andEscrowCancelare appropriate for testing IOU escrow operations.
18-23: Appropriate utility imports for escrow timing control.The
sendLedgerAcceptandwaitForAndForceProgressLedgerTimeutilities are essential for controlling ledger time progression required for escrow finish/cancel operations.
73-206: IOU escrow finish test is comprehensive and well-structured.The test properly demonstrates the complete Token Escrow workflow:
- Enables
asfAllowTrustLineLockingflag for the issuer- Sets up trust lines for IOU handling
- Transfers IOUs to the escrow creator
- Creates escrow with IOU amount
- Waits for appropriate timing and finishes the escrow
- Verifies escrow object lifecycle
The step-by-step approach with clear comments makes the test easy to follow and maintain.
High Level Overview of Change
This PR implements support for Token-Escrow in xrpl.js library.
The XLS specification can be found here.
This is the C++ implementation of the Token-Escrow feature.
Context of Change
This PR enables developers to use IOUs and MPTs with the Escrow functionality. This PR will also fix #3026
Type of Change
Did you update HISTORY.md?
Test Plan
Relevant unit and integration tests have been included.