fix: Add zero domainID check for permissionedDomain#7362
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens Permissioned DEX DomainID handling to prevent Ledger::read() from being called with a zero key (which triggers UNREACHABLE in assert-enabled builds). It adds preflight validation to reject a zero sfDomainID on OfferCreate and Payment, and adds a defensive guard in permissioned_dex::accountInDomain(), all gated by fixCleanup3_2_0.
Changes:
- Add
temMALFORMEDpreflight rejection whensfDomainID == 0forOfferCreateandPayment(whenfixCleanup3_2_0is enabled). - Add a defensive early-return in
permissioned_dex::accountInDomain()to avoid constructing a zero-key PermissionedDomain keylet. - Extend
PermissionedDEX_testcoverage to asserttemMALFORMEDfor zero DomainID, and run relevant tests both with and withoutfixCleanup3_2_0.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/test/app/PermissionedDEX_test.cpp | Adds regression tests for zero DomainID preflight behavior and runs OfferCreate/Payment tests with/without fixCleanup3_2_0. |
| src/libxrpl/tx/transactors/payment/Payment.cpp | Rejects sfDomainID == 0 as temMALFORMED in preflight (gated by fixCleanup3_2_0). |
| src/libxrpl/tx/transactors/dex/OfferCreate.cpp | Rejects sfDomainID == 0 as temMALFORMED in preflight (gated by fixCleanup3_2_0). |
| src/libxrpl/ledger/helpers/PermissionedDEXHelpers.cpp | Adds a defensive check to avoid view.read(keylet::permissionedDomain(0)) (gated by fixCleanup3_2_0). |
💡 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 #7362 +/- ##
=========================================
- Coverage 82.4% 82.4% -0.0%
=========================================
Files 1011 1011
Lines 76477 76484 +7
Branches 7322 7316 -6
=========================================
- Hits 63005 63004 -1
- Misses 13472 13480 +8
🚀 New features to boost your workflow:
|
PeterChen13579
left a comment
There was a problem hiding this comment.
Looks good to me
|
@yinyiqian1 is this ready to merge? |
Yes. Thanks for reminding. Just added the label |
Because
Ledger::read()asserts on zero keys,view.read(keylet::permissionedDomain(domainID))must only be called with a non-zerodomainID. This PR adds guarded validation for zeroDomainIDinOfferCreateandPayment, plus a defensive check inpermissioned_dex::accountInDomain().Although the asserts are only triggered in debug build, it is still worth adding the defensive check.
OfferCreateandPaymentso a zeroDomainIDreturnstemMALFORMED.permissioned_dex::accountInDomain()to avoid constructing a zero-keyPermissionedDomainkeylet.fixCleanup3_2_0High Level Overview of Change
Context of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)