Discussion: TokenEscrowV1 by dangell7 · Pull Request #5523 · XRPLF/rippled

dangell7 · 2025-07-01T17:11:21Z

High Level Overview of Change

add sfLockedCount to IOU trustline. (increase and decrease)
user/issuer cannot delete IOU trustline if user has sfLockedCount
remove requireAuth check in EscrowCancel

Context of Change

Issues:

CancelAfter was not required

If an account creates an escrow, does not specify CancelAfter and then deletes their trustline the escrow object could become "orphaned".

Require Auth is checked on EscrowCancel

If an IOU requires trustline authorization and the account sends their tokens back to the issuer, deletes the trustline and the adds the trustline back (in unauthorized state), the EscrowCancel transactor will fail with tecNO_AUTH which leaves the escrow object "orphaned"

Same for MPT, if the issuer unauthorizes the MPT, the EscrowCancel transactor will fail with tecNO_AUTH which leaves the escrow object "orphaned"

Solutions:

By adding the sfLockedCount onto the IOU trustline and incrementing when an escrow object is created we update the deleteTrustline function to check for a positive sfLockedCount and if there is a positive value, we reject the deletion of the trustline.
1 above solves the issue of an account deleting the IOU. So they will always have an authorized trustline. For MPT we remove the requireAuth check in the EscrowCancel transactor (IOU & MPT).

removing requireAuth check in EscrowCancel effects;

Note: requireAuth is checked for both account and destination in EscrowCreate

IOU: Technically this would never be possible because you cannot delete a trustline that has an escrow and you cannot unauthorize a trustline. So there is no way to get to EscrowCancel with an unauthorized trustline that was previously authorized.
MPT: On EscrowCancel the funds would return to the account, however the funds remain unauthorized.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Performance (increase or change in throughput and/or latency)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

API Impact

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

- add `sfLockedCount` to IOU trustline. (increase and decrease) - user/issuer cannot delete IOU trustline if user has `sfLockedCount` - remove `requireAuth` check in `EscrowCancel`

codecov · 2025-07-02T10:21:33Z

Codecov Report

Attention: Patch coverage is 97.14286% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.1%. Comparing base (e18f27f) to head (949ed92).
Report is 12 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/xrpld/app/tx/detail/Escrow.cpp	96.7%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #5523   +/-   ##
=======================================
  Coverage     79.1%   79.1%           
=======================================
  Files          816     816           
  Lines        71605   71630   +25     
  Branches      8237    8236    -1     
=======================================
+ Hits         56625   56650   +25     
  Misses       14980   14980

Files with missing lines	Coverage Δ
include/xrpl/protocol/detail/ledger_entries.macro	`100.0% <ø> (ø)`
src/xrpld/ledger/detail/View.cpp	`92.0% <100.0%> (+<0.1%)`	⬆️
src/xrpld/app/tx/detail/Escrow.cpp	`93.7% <96.7%> (+<0.1%)`	⬆️

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

shawnxie999 · 2025-07-08T15:38:35Z

What's the current behavior when the escrow creator deleted their trustline and cancels their escrow?

shawnxie999 · 2025-07-08T15:46:01Z

If an account creates an escrow, does not specify CancelAfter and then deletes their trustline the escrow object could become "orphaned".

Woudn't the escrow object still belong to the owner's owner directory? So technically the escrow object couldn't be orphaned since the owner is obligated to delete it before they can delete their account.

dangell7 · 2025-07-08T15:58:33Z

What's the current behavior when the escrow creator deleted their trustline and cancels their escrow?

It adds the trustline back.

mvadari · 2025-07-08T15:59:52Z

src/xrpld/app/tx/detail/Escrow.cpp

        if (!ctx.rules.enabled(featureTokenEscrow))
            return temBAD_AMOUNT;

+        if (ctx.rules.enabled(fixTokenEscrowV1) && !ctx.tx[~sfCancelAfter])


This seems wrong - CancelAfter does not need to be included...?

In the original spec CancelAfter was required or discussed to be required when IOU but never implemented. This implements that requirement. If we decide we don't need that then we can remove that. I just wanted to fix my mismatch between the original spec and the implementation.

I notice this requirement is only added for non-XRP escrows. I'm ok with that, because unlike XRP, there can be many reasons why someone wouldn't want to or may not be able to hold a given IOU, even for the time it takes to send it back to the issuer and destroy it.

dangell7 · 2025-07-08T16:01:12Z

If an account creates an escrow, does not specify CancelAfter and then deletes their trustline the escrow object could become "orphaned".

Woudn't the escrow object still belong to the owner's owner directory? So technically the escrow object couldn't be orphaned since the owner is obligated to delete it before they can delete their account.

I may be using "orphaned" incorrectly here. You are right, they cannot delete their account until they remove it.

ximinez · 2025-07-08T16:25:46Z

src/xrpld/app/tx/detail/Escrow.cpp

+        if (!sleRippleState->isFieldPresent(sfLockedCount))
+            sleRippleState->setFieldU32(sfLockedCount, 1);
+        else
+            sleRippleState->setFieldU32(
+                sfLockedCount,
+                (*sleRippleState)[~sfLockedCount].value_or(0) + 1);


Change sfLockedCount to soeDEFAULT, then this whole block can be written as

Suggested change

if (!sleRippleState->isFieldPresent(sfLockedCount))

sleRippleState->setFieldU32(sfLockedCount, 1);

else

sleRippleState->setFieldU32(

sfLockedCount,

(*sleRippleState)[~sfLockedCount].value_or(0) + 1);

(*sleRippleState)[sfLockedCount] += 1;

(ValueProxy handles all the steps for a default field behind the scenes. += is needed only because ValueProxy does not define a ++ operator, for reasons.)

Wow this is amazing thank you!

ximinez · 2025-07-08T16:26:39Z

src/xrpld/app/tx/detail/Escrow.cpp

+            auto const finalCount =
+                (*sleRippleState)[~sfLockedCount].value_or(0) - 1;
+            if (finalCount == 0)
+                sleRippleState->makeFieldAbsent(sfLockedCount);
+            else
+                sleRippleState->setFieldU32(sfLockedCount, finalCount);


Also here, if you change sfLockedCount to soeDEFAULT, then this whole block can be written as

Suggested change

auto const finalCount =

(*sleRippleState)[~sfLockedCount].value_or(0) - 1;

if (finalCount == 0)

sleRippleState->makeFieldAbsent(sfLockedCount);

else

sleRippleState->setFieldU32(sfLockedCount, finalCount);

(*sleRippleState)[sfLockedCount] = 1;

ximinez · 2025-07-08T16:30:06Z

src/xrpld/ledger/detail/View.cpp

+    if (view.rules().enabled(fixTokenEscrowV1) &&
+        (*sleRippleState)[~sfLockedCount].value_or(0) != 0)


Also here, if you change sfLockedCount to soeDEFAULT, then this whole block can be written as

Suggested change

if (view.rules().enabled(fixTokenEscrowV1) &&

(*sleRippleState)[~sfLockedCount].value_or(0) != 0)

if (view.rules().enabled(fixTokenEscrowV1) &&

(*sleRippleState)[sfLockedCount] != 0)

mvadari · 2025-07-08T16:38:36Z

So I think I disagree with both "fixes" added in this amendment (as in I would be against adding them - though I could be convinced otherwise).

CancelAfter should not be required - that changes the whole mechanics of an escrow, and changes the contract that underlies the escrow.

And an issue with authorized trustlines is equivalent to both sides being deep-frozen (the aforementioned "orphaned" escrow) which IMO should be up to the issuer and the account to figure out a resolution to. I'd rather something akin to the InstrumentClawback strategy (where an issuer can claw back directly from the escrow) over anything else.

dangell7 · 2025-07-08T17:04:36Z

So I think I disagree with both "fixes" added in this amendment (as in I would be against adding them - though I could be convinced otherwise).

CancelAfter should not be required - that changes the whole mechanics of an escrow, and changes the contract that underlies the escrow.

And an issue with authorized trustlines is equivalent to both sides being deep-frozen (the aforementioned "orphaned" escrow) which IMO should be up to the issuer and the account to figure out a resolution to. I'd rather something akin to the InstrumentClawback strategy (where an issuer can claw back directly from the escrow) over anything else.

Thank you for the review. I'm happy to hear you disagree with both of the fixes because I was under the impression I had made a mistake.

I agree, an EscrowClawback or InstrumentClawback is a better approach.

As its not an immediate issue, I converted it to a draft and we can discuss it further.

dangell7 requested a review from a team July 1, 2025 17:11

fix token escrow v1

8f53d92

- add `sfLockedCount` to IOU trustline. (increase and decrease) - user/issuer cannot delete IOU trustline if user has `sfLockedCount` - remove `requireAuth` check in `EscrowCancel`

dangell7 force-pushed the fix-escrow branch from 821896b to 8f53d92 Compare July 1, 2025 17:29

dangell7 mentioned this pull request Jul 1, 2025

XLS-85d Token Escrow XRPLF/XRPL-Standards#272

Merged

dangell7 marked this pull request as draft July 1, 2025 17:41

fix escrow test

949ed92

dangell7 marked this pull request as ready for review July 2, 2025 09:58

bthomee requested review from oleks-rip and shawnxie999 July 8, 2025 14:34

mvadari reviewed Jul 8, 2025

View reviewed changes

ximinez requested changes Jul 8, 2025

View reviewed changes

dangell7 marked this pull request as draft July 8, 2025 16:59

dangell7 changed the title ~~Fix: TokenEscrowV1~~ Discussion: TokenEscrowV1 Jul 8, 2025

ckeshava mentioned this pull request Jul 8, 2025

Add support for Token Escrow - XLS-85d XRPLF/xrpl-py#853

Merged

9 tasks

dangell7 closed this Jul 16, 2025

		if (view.rules().enabled(fixTokenEscrowV1) &&
		(*sleRippleState)[~sfLockedCount].value_or(0) != 0)

Conversation

dangell7 commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High Level Overview of Change

Context of Change

Type of Change

API Impact

Uh oh!

codecov bot commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

shawnxie999 commented Jul 8, 2025

Uh oh!

shawnxie999 commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dangell7 commented Jul 8, 2025

Uh oh!

mvadari Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

dangell7 Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

ximinez Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

dangell7 commented Jul 8, 2025

Uh oh!

ximinez Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

dangell7 Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

ximinez Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

ximinez Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

mvadari commented Jul 8, 2025

Uh oh!

dangell7 commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dangell7 commented Jul 1, 2025 •

edited

Loading

codecov bot commented Jul 2, 2025 •

edited

Loading

shawnxie999 commented Jul 8, 2025 •

edited

Loading

dangell7 commented Jul 8, 2025 •

edited

Loading