fix: Use account ledger entry when canceling token escrows - #6171
Conversation
There was a problem hiding this comment.
Pull request overview
This PR consolidates the TokenEscrow feature and fixTokenEscrowV1 fix into a new TokenEscrowV2 amendment, while addressing a critical bug in EscrowCancel that caused an edge case when canceling token escrows after trust line deletion.
- Fixes a bug in
EscrowCancel::doApply()where the escrow ledger entry was incorrectly passed instead of the account ledger entry to the token return helper function - Consolidates
TokenEscrowandfixTokenEscrowV1into a singleTokenEscrowV2amendment for cleaner implementation - Adds comprehensive test coverage for IOU and MPT escrow cancellation scenarios after trust line deletion
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
include/xrpl/protocol/detail/features.macro |
Introduces TokenEscrowV2 feature and retires TokenEscrow and fixTokenEscrowV1 |
src/xrpld/app/tx/detail/Escrow.cpp |
Fixes critical bug (line 1283: passes sle instead of slep), updates all feature checks to featureTokenEscrowV2, and simplifies MPT unlock logic |
src/xrpld/app/tx/detail/SetAccount.cpp |
Updates feature check for trust line locking flag to use featureTokenEscrowV2 |
src/xrpld/rpc/handlers/AccountInfo.cpp |
Updates feature check for trust line locking flag to use featureTokenEscrowV2 |
src/libxrpl/ledger/View.cpp |
Removes fixTokenEscrowV1 conditional logic from rippleUnlockEscrowMPT |
src/test/app/EscrowToken_test.cpp |
Adds testIOUCancelDoApply and testMPTCancelDoApply tests for the bug fix scenario, updates feature references, removes fixTokenEscrowV1 test variant |
src/test/app/Escrow_test.cpp |
Updates feature checks to featureTokenEscrowV2 |
src/test/app/AccountDelete_test.cpp |
Updates feature check to featureTokenEscrowV2 |
src/test/rpc/Feature_test.cpp |
Updates feature name test to expect "TokenEscrowV2" |
src/test/rpc/AccountInfo_test.cpp |
Updates feature checks to featureTokenEscrowV2 |
src/test/jtx/Env_test.cpp |
Updates all test feature bitset operations to use featureTokenEscrowV2 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6171 +/- ##
=========================================
- Coverage 82.1% 82.1% -0.0%
=========================================
Files 1010 1010
Lines 76163 76164 +1
Branches 7414 7413 -1
=========================================
- Hits 62503 62502 -1
- Misses 13660 13662 +2
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
f36b160 to
44d21b8
Compare
dangell7
changed the title
TokenEscrowV2
63d3115 to
c543d42
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dangell7
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
This comment has been minimized.
This comment has been minimized.
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
|
Please fix clang-tidy |
|
@dangell7 looks like everything broke lol |
mvadari
added
the
Ready to merge
bthomee
changed the title
High Level Overview of Change
This PR fixes a critical bug in
EscrowCancel.Context of Change
1. EscrowCancel Bug Fix (Original Issue)
A bug in
EscrowCancel::doApply()caused a edge case when canceling token escrows in a specific scenario:Scenario:
Problem: Line 1288 incorrectly passed the escrow ledger entry (
slep) instead of the account ledger entry (sle) toescrowUnlockApplyHelper, causing it to access the wrong object type when checkingOwnerCountfor trust line recreation.Solution: Pass the correct account SLE, allowing proper trust line recreation and token return.
Type of Change
API Impact
Before / After
Before
tefEXCEPTIONAfter (with
fixCleanup3_2_0)tesSUCCESS(recreates trust line)