Repository navigation
Conversation
|
@dangell7 I can't review this PR until it's updated to match the latest |
sorry, done |
|
It looks like the |
|
Does locking a token into a payment channel prevent deletion of the source's trustline (IOU) or MPToken (MPT) while the channel is open? The spec doesn't state this either way, and the "Channel Closure → Source Lacks Trustline or MPT Holding" failure conditions suggest the source's holding may be absent at closure — implying deletion is not blocked. If that's the intent, then in MPT: sfLockedAmount is tracked on the source's MPToken. If that object can be deleted while a channel holds locked value, the per-holder locked accounting is orphaned (the issuance-level sfLockedAmount still counts it, but the holder record is gone). |
|
Please address CI failures and follow the standard templates. |
|
@pwang200 Correct, inherited from XLS-85. IOU line can be deleted (refund then fails tecNO_LINE until its recreated), MPT can't (tecHAS_OBLIGATIONS). Spec documents both, per line lock accounting would be a separate fix amendment covering escrow too. |
|
Restructured to the template, validator passes now. |
There was a problem hiding this comment.
🟡 Changes recommended
Critical protocol encoding and accounting issues, along with multiple token-handling ambiguities, must be resolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Introduces XLS-93, extending XRPL payment channels to IOU and MPT assets.
Changes:
- Adds token channel creation, funding, claims, and closure.
- Defines token accounting, transfer fees, and issuer controls.
- Proposes
PaymentChannelClawback.
File summaries
| File | Description |
|---|---|
XLS-0093-token-paychan/README.md |
Defines Token PayChan. Unresolved issues include issuer-redemption exceptions, supply accounting, clawback semantics and encoding, token-signature serialization, precision loss, and XLS-85 inconsistencies. |
Review details
Suppressed comments (3)
XLS-0093-token-paychan/README.md:271
- This summary contradicts this draft's redemption and clawback transitions, which decrease MPT
OutstandingAmount, and IOUs do not have anOutstandingAmountfield. It should also account for the transfer fee removed from holder balances during an MPT claim.
| **Outstanding Amount** | Remains unchanged during channel operations | Remains unchanged during channel operations |
XLS-0093-token-paychan/README.md:76
- The claim that both primitives use identical creation checks conflicts with required XLS-85, which checks authorization and freeze/lock only for the source (XLS-85 lines 63-71), while this draft checks both parties. Clarify the intended difference or update XLS-85 so implementers have one rule.
- Note: this is deliberately stricter than base freeze semantics, under which an individual freeze only prevents the frozen holder from sending. Opening a lock is blocked by any freeze on either party, while paying out to the destination at claim time only requires that the destination is not deep frozen (see Normal Claim). This matches the lock creation rules of [XLS-85](../XLS-0085-token-escrow/README.md).
XLS-0093-token-paychan/README.md:266
- The summary also needs the issuer exception: requiring destination holder authorization unconditionally contradicts the supported issuer-redemption path, since an issuer does not hold or authorize itself for its own token.
| **Destination Authorization** | Required at creation and at claim; cannot be granted during claim if authorization required | Required at creation and at claim; cannot be granted during claim if authorization required |
- Files reviewed: 1/1 changed files
- Comments generated: 9
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@mvadari the document now follows the amendment template: Abstract, Motivation, Specification built from the Ledger Entry / Transaction / RPC sections, Rationale, Backwards Compatibility, Test Plan, Reference Implementation, Security Considerations. Protocol content is unchanged, only moved. 059db88 Could you re-review? |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved payout-validation, refund-precision, and authorization contradictions could cause unsafe or divergent implementations.
Review effort: Balanced
Findings: 2
Open (5)
Apply destination safety checks to claim-and-close payouts · New Define exactness handling for inexact IOU refunds · New Require destination authorization only for non-source payouts · New Define channel Balance as cumulative gross amount claimed · New Check requested Balance capacity, not signed Amount · New
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The specification leaves source-debit conservation unresolved and assigns a transaction discriminator already used by another standard.
Review effort: Balanced
Findings: 2
Open (2)
Resolved since last review (5)
| - If authorization is required and the destination is not authorized, the transaction fails with `tecNO_AUTH`. For an IOU, a destination with no trustline to the issuer fails with `tecNO_LINE` when authorization is required. | ||
|
|
||
| 3. **Destination Lacks Trustline or MPT Holding:** | ||
| - The destination's trustline or `MPToken` is created during the claim only when the destination itself submits the transaction (and authorization is not required). No other submitter can create a holding for the destination, since holding a token requires the holder's consent. |



High Level Overview of Change
Context of Change
Type of Change