Conversation
0f1e958 to
5134103
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #5285 +/- ##
=========================================
+ Coverage 81.5% 81.6% +0.1%
=========================================
Files 999 1004 +5
Lines 74458 75962 +1504
Branches 7553 7631 +78
=========================================
+ Hits 60653 61967 +1314
- Misses 13805 13995 +190
🚀 New features to boost your workflow:
|
|
haven't reviewed the changes but one invariant would be good to have is to ensure that the amount changed for |
8e91fa9 to
9962c46
Compare
8e0faf6 to
2c76683
Compare
2b18e49 to
77ef360
Compare
70aac26 to
c7be58f
Compare
* Payment * OfferCreate * CheckCash * All AMM transactions * Pathfinding Retire FlowCross feature
| { | ||
| if (!view.dirRemove( | ||
| keylet::ownerDir(ammAccountID), (*sleMpt)[sfOwnerNode], sleMpt->key(), false)) | ||
| return tefBAD_LEDGER; |
There was a problem hiding this comment.
nit - can exclude line coverage
|
|
||
| auto const& issuer = asset.getIssuer(); | ||
| if (!view.exists(keylet::account(issuer))) | ||
| return tecNO_ISSUER; |
There was a problem hiding this comment.
nit - exclude line cov
| txType == ttCHECK_CASH || txType == ttPAYMENT; | ||
| XRPL_ASSERT(validTx, "xrpl::checkMPTAllowed : all MPT tx or DEX"); | ||
| if (!validTx) | ||
| return tefINTERNAL; |
There was a problem hiding this comment.
nit - exclude line cov
| auto const issuanceKey = keylet::mptIssuance(issuanceID); | ||
| auto const issuanceSle = view.read(issuanceKey); | ||
| if (!issuanceSle) | ||
| return tecOBJECT_NOT_FOUND; |
| auto const flags = issuanceSle->getFlags(); | ||
|
|
||
| if ((flags & lsfMPTLocked) != 0u) | ||
| return tecLOCKED; |
There was a problem hiding this comment.
this isn't covered. I believe this function's callers already checked for locking before they reache here so it's unreachable. should be fine to exclude cov but worth double checking
| auto const amt = sle->getFieldU64(sfMPTAmount); | ||
| auto const pay = saAmount.mpt().value(); | ||
| if (amt < pay) | ||
| auto const holderBalance = sle->getFieldU64(sfMPTAmount); |
There was a problem hiding this comment.
nit - senderBalance is a better name since both sender and receiver can be holders
| auto const maxAmount = maxMPTAmount(*sle); | ||
| auto const outstanding = sle->getFieldU64(sfOutstandingAmount); | ||
| auto const mptokensV2 = view.rules().enabled(featureMPTokensV2); | ||
| allowOverflow = (allowOverflow == AllowMPTOverflow::Yes && mptokensV2) |
There was a problem hiding this comment.
allowOverflow = (allowOverflow == AllowMPTOverflow::Yes && mptokensV2)
^ is checking for mptokensV2 necessary? AllowMPTOverflow is introduced in this amendment and has a default of No for existing call sites
There was a problem hiding this comment.
Offer and AMMOffer must set the overflow to Yes. So we have to check if the amendment is enabled.
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
|
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 |
There was a problem hiding this comment.
ℹ️ Note: This is a large diff (1,119,614 chars). Complex issues deep in the diff may receive less attention.
Several high-severity issues flagged inline: null dereference in removeDeletedMPTs, use-after-free race in AssetCache::getMPTs, potential server crash in NFTokenUtils via .get<Issue>() on MPT amounts, and a 0.1% tolerance applied to integer-denominated MPT amounts in checkNear. Additional correctness concerns around deleteAMMMPToken missing outstanding-amount accounting, strict aliasing UB in getMPTIssuer, uninitialized outstanding array, auto-authorization bypass in AMMCreate, and missing MPT issuer validation in path JSON parsing.
Review by Claude Opus 4.6 · Prompt: V12
|
|
||
| bool | ||
| checkNear(XRPAmount const& expected, XRPAmount const& actual) | ||
| checkNear(MPTAmount const& expected, MPTAmount const& actual) |
There was a problem hiding this comment.
MPT gets IOU-style 0.1% tolerance instead of exact equality like XRP. Both are integer-denominated assets — under-delivery up to 0.1% would silently pass checkNear.
src/libxrpl/tx/Transactor.cpp
Outdated
| // There could be at most two MPTs - one for each side of AMM pool | ||
| if (mpts.size() > 2) | ||
| { | ||
| JLOG(viewJ.error()) << "removeDeletedTrustLines: deleted mpts exceed 2 " << mpts.size(); |
There was a problem hiding this comment.
Copy-paste bug: log says removeDeletedTrustLines but this is removeDeletedMPTs.
| JLOG(viewJ.error()) << "removeDeletedTrustLines: deleted mpts exceed 2 " << mpts.size(); | |
| JLOG(viewJ.error()) << "removeDeletedMPTs: deleted mpts exceed 2 " << mpts.size(); |
| for (auto const& index : mpts) | ||
| { | ||
| if (auto const sleState = view.peek({ltMPTOKEN, index}); | ||
| deleteAMMMPToken(view, sleState, (*sleState)[sfIssuer], viewJ) != tesSUCCESS) |
There was a problem hiding this comment.
Null dereference if view.peek() returns nullptr: (*sleState)[sfIssuer] crashes.
| deleteAMMMPToken(view, sleState, (*sleState)[sfIssuer], viewJ) != tesSUCCESS) | |
| sleState && deleteAMMMPToken(view, sleState, (*sleState)[sfIssuer], viewJ) != tesSUCCESS) |
include/xrpl/protocol/MPTIssue.h
Outdated
| { | ||
| static_assert(sizeof(MPTID) == (sizeof(std::uint32_t) + sizeof(AccountID))); | ||
| AccountID const* accountId = | ||
| reinterpret_cast<AccountID const*>(mptid.data() + sizeof(std::uint32_t)); |
There was a problem hiding this comment.
Strict aliasing violation: reinterpret_cast from uint8_t* to AccountID const* is UB. In a consensus system, compiler-version-dependent behavior is a real risk. Use std::memcpy or std::bit_cast.
| enum Order { Before = 0, After = 1 }; | ||
| struct MPTData | ||
| { | ||
| std::array<std::int64_t, After + 1> outstanding; |
There was a problem hiding this comment.
outstanding lacks a default initializer unlike mptAmount{0}. Default-construction yields indeterminate values.
| std::array<std::int64_t, After + 1> outstanding; | |
| std::array<std::int64_t, After + 1> outstanding{}; |
See: std::array
| } | ||
|
|
||
| p.emplace_back(uAccount, uCurrency, uIssuer, hasCurrency); | ||
| p.emplace_back(uAccount, uAsset, uIssuer, hasAsset); |
There was a problem hiding this comment.
When mpt_issuance_id is present but no issuer field, uIssuer stays as zero-AccountID. The issuer check only fires when issuer IS provided. Fix: auto-populate uIssuer from the MPTID when isMPT and no issuer was supplied.
There was a problem hiding this comment.
Issuer can be a path element. If it's not included in the path then it should not be set.
| // can be different from the issuer. If cur is MPT then curAsset | ||
| // is just set to MPTID. | ||
| if (curAsset.holds<MPTIssue>() && cur->hasCurrency()) | ||
| { |
There was a problem hiding this comment.
curAsset = Issue{} sets a zero-account issuer when transitioning from MPT to Currency. If curAsset.getIssuer() is used before a subsequent account/issuer node sets it, the zero issuer propagates into step construction. Is this intentional?
There was a problem hiding this comment.
The issuer is set in the code block right after this transition.
| // implied to be the second-to-last step of the path. If normPath.back | ||
| // is an offer, which sells MPT then the added path element account is | ||
| // the MPT's issuer. | ||
| if (!((normPath.back().isAccount() && |
There was a problem hiding this comment.
lastAsset.getPathAsset() != deliver compares PathAsset vs Asset — previously this only compared currencies. If operator!= also compares issuers, path normalization behavior changes and could affect payment routing correctness. Verify the semantics match the original intent.
There was a problem hiding this comment.
For currencies it compares the currency only. For MPT, it compares MPTID, which must include the issuer.
|
|
||
| template <class TIn, class TOut> | ||
| template <StepAmount TIn, StepAmount TOut> | ||
| TAmounts<TIn, TOut> |
There was a problem hiding this comment.
TOffer::send() unconditionally passes AllowMPTOverflow::Yes. DEX offer settlement can temporarily cause MPT sfOutstandingAmount to exceed sfMaximumAmount. Is there reconciliation logic ensuring final outstanding stays within bounds?
There was a problem hiding this comment.
The logic balances it out. MPTInvariant has the ultimate overflow check.
include/xrpl/ledger/ApplyView.h
Outdated
| * BookStep limits it's output to 150USD. This in turn limits A3's send | ||
| * amount to 150XRP: A1 buys 100XRP and sells 100USD to A3. This doesn't | ||
| * change OutstandingAmount. GW buys 50XRP and sells 50USD to A3. This | ||
| * changes OutstandingAmount 10 1,000USD. |
There was a problem hiding this comment.
Typo in doc comment: 10 should be to.
| * changes OutstandingAmount 10 1,000USD. | |
| * changes OutstandingAmount to 1,000USD. |
|
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. |
| MPTEndpointStep<TDerived>::lineQualityIn(ReadView const& v) const | ||
| { | ||
| // dst quality in | ||
| return QUALITY_ONE; |
There was a problem hiding this comment.
nit - exclude line cov
There was a problem hiding this comment.
Why should it be excluded?
There was a problem hiding this comment.
hmm its seems codecov is complaining that this line is not covered
| JLOG(j_.debug()) << "MPTEndpointStep: specified bad account."; | ||
| return temBAD_PATH; | ||
| } | ||
|
|
||
| if (src_ == dst_) | ||
| { | ||
| JLOG(j_.debug()) << "MPTEndpointStep: same src and dst."; | ||
| return temBAD_PATH; | ||
| } | ||
|
|
||
| auto const sleSrc = ctx.view.read(keylet::account(src_)); | ||
| if (!sleSrc) | ||
| { | ||
| JLOG(j_.warn()) << "MPTEndpointStep: can't receive MPT from non-existent issuer: " << src_; | ||
| return terNO_ACCOUNT; | ||
| } |
There was a problem hiding this comment.
nit - exclude line cov for all returns
| UNREACHABLE( | ||
| "xrpl::MPTEndpointStep::check : prev seen book without a " | ||
| "prev step"); | ||
| return temBAD_PATH_LOOP; |
There was a problem hiding this comment.
nit - exclude line cov
| // limiting node | ||
| MPTAmount const actualIn = mulRatio(maxSrcToDst, srcQOut, QUALITY_ONE, /*roundUp*/ true); | ||
| // Don't have to factor in dstQIn since it's always QUALITY_ONE | ||
| MPTAmount const out = maxSrcToDst; | ||
| setCacheLimiting(actualIn, maxSrcToDst, out, srcDebtDir); | ||
| auto const ter = rippleCredit( | ||
| sb, | ||
| src_, | ||
| dst_, |
There was a problem hiding this comment.
nit - might be worth adding a comment that this code will not be reachable, as executing forward direction should not be limiting since the reverse direction already did the job
High Level Overview of Change
Add MPT support to DEX
Context of Change
Implements XLS-82d
Adds MPT support for the following transactions:
Retires
FlowCrossfeature.Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)Test Plan
Extended MPToken unit-test to test for basic MPT interactions with DEX.