fix: Amendment Guard `TokenEscrow` preclaim by dangell7 · Pull Request #5473 · XRPLF/rippled

dangell7 · 2025-06-04T17:40:51Z

High Level Overview of Change

Context of Change

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)

mvadari · 2025-06-04T17:50:47Z

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

-    if (!slep)
-        return tecNO_TARGET;


This error needs to be thrown in doApply in both transactions

It is already thrown in doApply for Cancel & Finish. It wasn't "moved" but rather copied.

In that case, should the doApply one return tecINTERNAL if the amendment is enabled?

sure: dacc539

codecov · 2025-06-04T18:55:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.1%. Comparing base (11edaa4) to head (efb8800).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5473     +/-   ##
=========================================
+ Coverage     79.0%   79.1%   +0.1%     
=========================================
  Files          817     817             
  Lines        71739   71697     -42     
  Branches      8311    8240     -71     
=========================================
+ Hits         56695   56708     +13     
+ Misses       15043   14989     -54     
+ Partials         1       0      -1

Files with missing lines	Coverage Δ
src/libxrpl/protocol/STAmount.cpp	`90.5% <ø> (+0.2%)`	⬆️
src/xrpld/app/tx/detail/Escrow.cpp	`93.6% <100.0%> (+1.0%)`	⬆️
src/xrpld/app/tx/detail/InvariantCheck.cpp	`91.5% <100.0%> (+0.9%)`	⬆️
src/xrpld/app/tx/detail/MPTokenIssuanceDestroy.cpp	`90.0% <ø> (+2.9%)`	⬆️
src/xrpld/ledger/detail/View.cpp	`92.0% <ø> (+2.9%)`	⬆️

... 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.

mvadari · 2025-06-04T19:14:56Z

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

-        if (!ctx.view.rules().enabled(featureTokenEscrow))
-            return temDISABLED;  // LCOV_EXCL_LINE


I think it's fine to keep these blocks in just in case

fixed: 55d03e2

bthomee · 2025-06-04T19:46:47Z

@dangell7 can you please ensure that all the new code you added as part of the Token Escrow change are fully tested? For instance, the newly added logic in GatewayBalances.cpp was only tested to 20%.

@Bronek can you please list the additional issues you identified?

shawnxie999

👍

dangell7 · 2025-06-04T20:53:12Z

@dangell7 can you please ensure that all the new code you added as part of the Token Escrow change are fully tested? For instance, the newly added logic in GatewayBalances.cpp was only tested to 20%.

@Bronek can you please list the additional issues you identified?

This should bring it to 100% except for the JLOG in View.cpp? I can wrap those paths with a full LCOV? I didn't realize the log would trigger a non testable coverage.

1180a24

Bronek · 2025-06-05T09:27:52Z

@dangell7 can you please ensure that all the new code you added as part of the Token Escrow change are fully tested? For instance, the newly added logic in GatewayBalances.cpp was only tested to 20%.
@Bronek can you please list the additional issues you identified?

This should bring it to 100% except for the JLOG in View.cpp? I can wrap those paths with a full LCOV? I didn't realize the log would trigger a non testable coverage.

1180a24

@dangell7 we normally use // LCOV_EXCL_START . . . // LCOV_EXCL_STOP for multi-line defensive blocks of code, for example see isVaultPseudoAccountFrozen . Also, depending on the nature of the check, we would sometimes prefer UNREACHABLE e.g. when condition is related to SLE object integrity. In cases like canAdd, canSubtract I think logging is better.

If you are uncertain if a line should be UNREACHABLE then do not put this macro there.

Bronek · 2025-06-05T09:31:35Z

@dangell7 can you please ensure that all the new code you added as part of the Token Escrow change are fully tested? For instance, the newly added logic in GatewayBalances.cpp was only tested to 20%.
@Bronek can you please list the additional issues you identified?

This should bring it to 100% except for the JLOG in View.cpp? I can wrap those paths with a full LCOV? I didn't realize the log would trigger a non testable coverage.

1180a24

Re. already merged #5185 , I see the following deficiencies in unit tests coverage:

missing 6 lines from Escrow.cpp where 2 lines are badly marked defensive code, 1 line (return temINVALID_FLAG; in preflight) should be very easy to test but wasn’t , remaining 3 lines (return tecINSUFFICIENT_FUNDS; in escrowCreatePreclaimHelper<MPTIssue> , return ret; after check escrowLockApplyHelper in doApply() and finally return tecOBJECT_NOT_FOUND; in escrowFinishPreclaimHelper<MPTIssue> might or might not be difficult to add. Overall it seems like test coverage was slightly less for MPTs than it was for IOUs and I can certainly sympathise with this.
missing 5 lines in InvariantCheck.cpp and I have no idea how difficult (or not) would it be to trigger these invariant violations but then we do all kind of shady things in Invariants_test.cpp to put invalid SLEs in place, so it is certainly possible to test most invariants.
missing 2 lines in STAmount.cpp which are return false; in both canAdd and canSubtract (newly added functions). Both functions do have unit tests (nice!) but both are missing this last line; perhaps it is unreachable ? In which case they should be preceded by UNREACHABLE and inside a // LCOV_EXCL... block (see isVaultPseudoAccountFrozen to which I refereed above for example). If you are uncertain if a line should be UNREACHABLE then do not use this macro, but we have means (fuzzing) of finding such unwarranted UNREACHABLE and removing them, so it's not such a big deal.

We have merge conflicts with other major features in flight (e.g. Lending #5270 ) and I would prefer if we had better assurance than "it compiles, all unit tests pass, but not all cases are tested" as an proof of good conflict resolution.

dangell7 · 2025-06-05T10:48:11Z

escrowLockApplyHelper

Thank you for this review.

tecINSUFFICIENT_FUNDS fixed

The return from escrowLockApplyHelper cannot be tested because rippleCredit only returns tefBAD_LEDGER errors. Unless someone has found a test for that and not included it in the repo its probably a very difficult test to write. For the MPT its the same scenario, defensive overflow/underflow checks that can only be tested by creating isolated function tests...

Do you want me to create functional tests for this?

tecOBJECT_NOT_FOUND. You cannot delete the issuance if there is an EscrowAmount.

But... I believe it is possible inject a Escrow object into the open ledger and potentially write a test for this... I will work on that now but I did mark it excluded in case my idea doesn't work.

InvariantCheck.cpp fixed

STAmount.cpp fixed

Bronek

nice, thank you for unit tests coverage cleanup !

src/libxrpl/protocol/STAmount.cpp

src/test/app/Escrow_test.cpp

src/test/ledger/Invariants_test.cpp

Bronek · 2025-06-05T11:49:45Z

The return from escrowLockApplyHelper cannot be tested because rippleCredit only returns tefBAD_LEDGER errors.
Do you want me to create functional tests for this?

No, it's cool.

PS. apologies for editing your comment, I somehow confused the "reply" and "edit comment" function in Github, unintentionally abusing my maintainer privileges. 🙈 😳 Really sorry for this !

bthomee · 2025-06-05T12:05:05Z

@dangell7 is the PR ready to merge?

dangell7 · 2025-06-05T12:18:06Z

@dangell7 is the PR ready to merge?

Ready

@Bronek I was able to test tecOBJECT_NOT_FOUND by injecting an Escrow object into the ledger: efb8800

Might be useful for later amendments

dangell7 force-pushed the patch-token-escrow branch from 12063d9 to 8412f88 Compare June 4, 2025 17:41

dangell7 mentioned this pull request Jun 4, 2025

XLS-85d: Token-Enabled Escrows #5185

Merged

13 tasks

dangell7 force-pushed the patch-token-escrow branch from 8412f88 to c8c686f Compare June 4, 2025 17:45

mvadari reviewed Jun 4, 2025

View reviewed changes

amendment guard escrow preclaim

a3ac092

dangell7 force-pushed the patch-token-escrow branch from c8c686f to a3ac092 Compare June 4, 2025 18:06

dangell7 added 2 commits June 4, 2025 20:06

Merge branch 'develop' into patch-token-escrow

531d116

add tests

dacc539

dangell7 force-pushed the patch-token-escrow branch from 62a4b63 to dacc539 Compare June 4, 2025 18:28

mvadari reviewed Jun 4, 2025

View reviewed changes

add defensive temDISABLED

55d03e2

mvadari approved these changes Jun 4, 2025

View reviewed changes

mvadari requested review from oleks-rip and shawnxie999 June 4, 2025 19:42

code coverage tests

1180a24

shawnxie999 approved these changes Jun 4, 2025

View reviewed changes

kennyzlei requested a review from Bronek June 4, 2025 21:22

more code coverage

6b1d276

dangell7 requested a review from a team June 4, 2025 21:43

more code coverage

984c7ae

Bronek approved these changes Jun 5, 2025

View reviewed changes

src/libxrpl/protocol/STAmount.cpp Show resolved Hide resolved

src/libxrpl/protocol/STAmount.cpp Show resolved Hide resolved

src/test/app/Escrow_test.cpp Show resolved Hide resolved

src/test/ledger/Invariants_test.cpp Show resolved Hide resolved

add more code coverage

efb8800

Bronek added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Jun 5, 2025

bthomee enabled auto-merge (squash) June 5, 2025 12:32

bthomee merged commit 58c2c82 into XRPLF:develop Jun 5, 2025
4 checks passed

This was referenced Jun 12, 2025

Proposed 2.5.0-rc2 #5491

Merged

Release 2.5.0 #5501

Closed

Set version to 2.5.0 #5502

Closed

legleux mentioned this pull request Jun 23, 2025

Release 2.5.0 #5504

Merged

Bronek mentioned this pull request Aug 28, 2025

Remove extraneous // LCOV_EXCL_START #5744

Merged

1 task

		if (!ctx.view.rules().enabled(featureTokenEscrow))
		return temDISABLED; // LCOV_EXCL_LINE

Conversation

dangell7 commented Jun 4, 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!

mvadari Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

dangell7 Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

mvadari Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

mvadari Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

dangell7 Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

bthomee commented Jun 4, 2025

Uh oh!

shawnxie999 left a comment

Choose a reason for hiding this comment

Uh oh!

dangell7 commented Jun 4, 2025

Uh oh!

Bronek commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bronek commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dangell7 commented Jun 5, 2025 • edited by Bronek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bronek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Bronek commented Jun 5, 2025

Uh oh!

bthomee commented Jun 5, 2025

Uh oh!

dangell7 commented Jun 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dangell7 commented Jun 4, 2025 •

edited

Loading

mvadari Jun 4, 2025 •

edited

Loading

dangell7 Jun 4, 2025 •

edited

Loading

codecov bot commented Jun 4, 2025 •

edited

Loading

Bronek commented Jun 5, 2025 •

edited

Loading

Bronek commented Jun 5, 2025 •

edited

Loading

dangell7 commented Jun 5, 2025 •

edited by Bronek

Loading