fix: PermissionedDEX (CreateOffer/Payment) never deletes expired credentials#6827
fix: PermissionedDEX (CreateOffer/Payment) never deletes expired credentials#6827Kassaking7 wants to merge 6 commits intoXRPLF:developfrom
Conversation
mvadari
left a comment
mvadari
left a comment
There was a problem hiding this comment.
This needs to be amendment gated
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6827 +/- ##
=========================================
- Coverage 82.1% 82.1% -0.0%
=========================================
Files 1010 1010
Lines 76033 76083 +50
Branches 7369 7373 +4
=========================================
+ Hits 62418 62447 +29
- Misses 13615 13636 +21
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
|
/ai-review |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
|
/ai-review |
| return verifyValidDomain(ctx_.view(), acct, domainID, j_); | ||
| }; | ||
|
|
||
| if (auto const err = cleanupFor(account_); !isTesSuccess(err)) |
There was a problem hiding this comment.
I think for here, if sender's credential is expired, it deletes only the sender’s credential and returns before checking destination. It would be good to cleanup for both sender and destination in one go
| { | ||
| if (!permissioned_dex::accountInDomain(ctx.view, id, ctx.tx[sfDomainID])) | ||
| return tecNO_PERMISSION; | ||
| if (ctx.view.rules().enabled(fixSecurity3_1_3)) |
There was a problem hiding this comment.
How to we usually name these? Can it be better names 😄
There was a problem hiding this comment.
This is the right name - it's what they came up for the series of bug fixes. This is to group the numerous bug fixes into one amendment
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
| { | ||
| if (!permissioned_dex::accountInDomain(ctx.view, id, ctx.tx[sfDomainID])) | ||
| return tecNO_PERMISSION; | ||
| if (ctx.view.rules().enabled(fixSecurity3_1_3)) |
There was a problem hiding this comment.
Are you sure that this PR is going into fixSecurity3_1_3? not 3.2? Hasn't 3.1.3 already made a cut off?
| auto const domainID = ctx_.tx[sfDomainID]; | ||
| auto const sleDomain = ctx_.view().read(keylet::permissionedDomain(domainID)); | ||
| if (!sleDomain) | ||
| return tecNO_PERMISSION; |
There was a problem hiding this comment.
should return tecINTERNAL here and exclude line coverage // LCOV_EXCL_LINE, the existence of domain was already checked in preclaim but we still need to check here for nullptr, but it should never be reachable
|
We recently merged a refactor to One-time setupIf you don't already have clang-tidy working in your env, on macOS: brew install llvm@21
# Follow brew's hint to put $(brew --prefix llvm@21)/bin on PATH so run-clang-tidy is found.Workflow on your branch (before merging develop)1. Grab the new git remote -v # should show 'upstream' among others; if not:
# git remote set-url upstream git@github.com:XRPLF/rippled.git
git fetch upstream
git checkout upstream/develop -- .clang-tidy2. Reconfigure conan/cmake so 3. Apply renames for the files modified in your PR: git diff --name-only $(git merge-base HEAD upstream/develop) HEAD \
| grep -E '\.(cpp|h|hpp|ipp)$' \
| xargs run-clang-tidy -p build -fix -allow-no-checks
# or -p .build, or whatever your build dir is called4. Build + test, then commit as a single dedicated commit: cmake --build build -j8
git commit -am "refactor: Align identifier naming with develop"5. Now merge develop: git merge upstream/developExtraRun clang-tidy once more after the merge to catch any stragglers introduced from develop's side: run-clang-tidy -p build -fix -allow-no-checks src tests
# or -p .build, or whatever your build dir is called |
| // devin cannot create offer with expired cred | ||
| env(offer(devin, XRP(10), USD(10)), domain(domainID), ter(tecNO_PERMISSION)); | ||
| // devin cannot create offer with expired cred; the expired credential | ||
| // SLE is deleted by doApply and tecEXPIRED is returned | ||
| env(offer(devin, XRP(10), USD(10)), domain(domainID), ter(tecEXPIRED)); |
There was a problem hiding this comment.
you should not change the result of an existing test, behavior should not change without a amendment gate. Make sure you have a behavior that switches when amendment is on/off
| // doApply now runs, deletes the expired SLE, returns tecEXPIRED | ||
| env(offer(devin, XRP(10), USD(10)), domain(domainID), ter(tecEXPIRED)); | ||
| env.close(); | ||
|
|
||
| BEAST_EXPECT(!env.le(credKey)); // credential was deleted |
There was a problem hiding this comment.
please have a single test setup for the same scenario and then switch the behavior with the amendment on/off. And call the test with
testExpiredCredentialCleanup(all);
testExpiredCredentialCleanup(all - amednemtn);
This would be much easier to maintain and read.
| // Scenario 1: OfferCreate with an expired credential deletes the | ||
| // credential SLE from the ledger and returns tecEXPIRED |
There was a problem hiding this comment.
we don't have a pre-fix test for this scenario. Even if it's tested elsewhere, it'd be good to have it for maintainability and readability - it should be easy to do by gating the test behavior with the amendment. see the comment above
| domain(domainID), | ||
| ter(tecNO_PERMISSION)); | ||
| env.close(); | ||
| BEAST_EXPECT(env.le(credKey)); // credential was NOT deleted |
There was a problem hiding this comment.
minor - would be good to that the bob's offer is still alive and bob's cred is alive
| Account const devin("devin"); | ||
| env.fund(XRP(1000), devin); | ||
| env.close(); | ||
| env.trust(USD(1000), devin); | ||
| env.close(); | ||
| env(pay(gw, devin, USD(100))); | ||
| env.close(); | ||
|
|
||
| auto jv = credentials::create(devin, domainOwner, credType); | ||
| uint32_t const t = env.current()->header().parentCloseTime.time_since_epoch().count(); | ||
| jv[sfExpiration.jsonName] = t + 20; | ||
| env(jv); | ||
| env(credentials::accept(devin, domainOwner, credType)); | ||
| env.close(); | ||
|
|
||
| auto const credKey = | ||
| keylet::credential(devin.id(), domainOwner.id(), makeSlice(credType)); | ||
| BEAST_EXPECT(env.le(credKey)); | ||
|
|
||
| env.close(std::chrono::seconds(20)); |
There was a problem hiding this comment.
this seemed to be shared code between the new tests, can be refactored into a lambda
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
5 participants
High Level Overview of Change
The PermissionedDEX code paths (CreateOffer and Payment) check credential validity via
accountInDomain()in PermissionedDEXHelpers.cpp, but this function uses a ReadView — making it architecturally impossible to delete expired credential SLEs from the ledger. Expired credentials persist indefinitely, locking owner reserves and causing permanent ledger bloat.Root Cause
The XRPL uses a two-phase cleanup pattern for expired credentials:
preclaim()(ReadView): identifies expired credentials, suppresses tecEXPIRED to allow doApply() to rundoApply()(ApplyView): calls verifyValidDomain() → removeExpired() → deleteSLE() to actually delete the expired credential from the ledgerThis pattern is correctly implemented in VaultDeposit (via enforceMPTokenAuthorization → verifyValidDomain → removeExpired). However, the PermissionedDEX paths skip this entirely:
accountInDomain()(PermissionedDEXHelpers.cpp:28-55) takes a ReadView const& parameter. It callscredentials::checkExpired()to skip expired credentials, but cannot call deleteSLE() because ReadView is immutable.Neither
CreateOffer::doApply()norPayment::doApply()have any call toverifyValidDomain()orremoveExpired()for the PermissionedDEX credential check path.Context of Change
Add a
verifyValidDomain()call in thedoApply()of both CreateOffer and Payment when a DomainID is present. This mirrors how VaultDeposit handles it. The pattern is:preclaim:accountInDomain(ReadView) checks validity, suppressestecEXPIREDdoApply: callverifyValidDomain(ApplyView) which triggers removeExpired → deleteSLE for any expired credentials encounteredAPI Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)