Skip to content

feat: Add MPT support to DEX#5285

Open
gregtatcam wants to merge 162 commits intoXRPLF:developfrom
gregtatcam:feature/mpt-v2e
Open

feat: Add MPT support to DEX#5285
gregtatcam wants to merge 162 commits intoXRPLF:developfrom
gregtatcam:feature/mpt-v2e

Conversation

@gregtatcam
Copy link
Copy Markdown
Collaborator

@gregtatcam gregtatcam commented Feb 12, 2025

High Level Overview of Change

Add MPT support to DEX

Context of Change

Implements XLS-82d

Adds MPT support for the following transactions:

  • Payment
  • OfferCreate
  • CheckCash
  • All AMM transactions
  • Pathfinding

Retires FlowCross feature.

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)

Test Plan

Extended MPToken unit-test to test for basic MPT interactions with DEX.

@gregtatcam gregtatcam force-pushed the feature/mpt-v2e branch 3 times, most recently from 0f1e958 to 5134103 Compare February 12, 2025 14:47
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 12, 2025

Codecov Report

❌ Patch coverage is 88.97804% with 261 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.6%. Comparing base (2502bef) to head (97189fb).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
src/libxrpl/tx/paths/MPTEndpointStep.cpp 73.3% 87 Missing ⚠️
src/libxrpl/tx/invariants/MPTInvariant.cpp 76.9% 18 Missing ⚠️
src/libxrpl/tx/paths/PaySteps.cpp 87.9% 16 Missing ⚠️
src/libxrpl/tx/transactors/check/CheckCash.cpp 88.1% 15 Missing ⚠️
src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp 87.6% 11 Missing ⚠️
src/libxrpl/ledger/helpers/TokenHelpers.cpp 90.4% 9 Missing ⚠️
include/xrpl/protocol/AmountConversions.h 78.9% 8 Missing ⚠️
src/libxrpl/protocol/STParsedJSON.cpp 81.1% 7 Missing ⚠️
src/libxrpl/tx/transactors/dex/AMMClawback.cpp 85.7% 7 Missing ⚠️
src/libxrpl/tx/transactors/dex/AMMCreate.cpp 92.6% 7 Missing ⚠️
... and 28 more
Additional details and impacted files

Impacted file tree graph

@@            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     
Files with missing lines Coverage Δ
include/xrpl/ledger/OrderBookDB.h 100.0% <ø> (ø)
include/xrpl/ledger/PaymentSandbox.h 100.0% <100.0%> (ø)
include/xrpl/ledger/View.h 100.0% <ø> (ø)
include/xrpl/ledger/helpers/RippleStateHelpers.h 100.0% <ø> (ø)
include/xrpl/protocol/AMMCore.h 100.0% <ø> (ø)
include/xrpl/protocol/Asset.h 97.7% <100.0%> (+1.4%) ⬆️
include/xrpl/protocol/Concepts.h 100.0% <100.0%> (ø)
include/xrpl/protocol/LedgerFormats.h 100.0% <ø> (ø)
include/xrpl/protocol/MPTAmount.h 90.9% <100.0%> (-9.1%) ⬇️
include/xrpl/protocol/MPTIssue.h 100.0% <100.0%> (ø)
... and 127 more

... and 9 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@shawnxie999
Copy link
Copy Markdown
Collaborator

haven't reviewed the changes but one invariant would be good to have is to ensure that the amount changed for OutstandingBalance in MPTokenIssuance should equal the net change of all the MPTokens modified in the transaction

@gregtatcam gregtatcam force-pushed the feature/mpt-v2e branch 5 times, most recently from 8e91fa9 to 9962c46 Compare March 5, 2025 17:00
@gregtatcam gregtatcam force-pushed the feature/mpt-v2e branch 8 times, most recently from 8e0faf6 to 2c76683 Compare March 15, 2025 22:25
@gregtatcam gregtatcam force-pushed the feature/mpt-v2e branch 2 times, most recently from 2b18e49 to 77ef360 Compare March 20, 2025 19:00
* Payment
* OfferCreate
* CheckCash
* All AMM transactions
* Pathfinding

Retire FlowCross feature
{
if (!view.dirRemove(
keylet::ownerDir(ammAccountID), (*sleMpt)[sfOwnerNode], sleMpt->key(), false))
return tefBAD_LEDGER;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - can exclude line coverage


auto const& issuer = asset.getIssuer();
if (!view.exists(keylet::account(issuer)))
return tecNO_ISSUER;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - exclude line cov

txType == ttCHECK_CASH || txType == ttPAYMENT;
XRPL_ASSERT(validTx, "xrpl::checkMPTAllowed : all MPT tx or DEX");
if (!validTx)
return tefINTERNAL;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - exclude line cov

auto const issuanceKey = keylet::mptIssuance(issuanceID);
auto const issuanceSle = view.read(issuanceKey);
if (!issuanceSle)
return tecOBJECT_NOT_FOUND;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - exclude cov

auto const flags = issuanceSle->getFlags();

if ((flags & lsfMPTLocked) != 0u)
return tecLOCKED;
Copy link
Copy Markdown
Collaborator

@shawnxie999 shawnxie999 Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Copy Markdown
Collaborator

@shawnxie999 shawnxie999 Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Offer and AMMOffer must set the overflow to Yes. So we have to check if the amendment is enabled.

@github-actions
Copy link
Copy Markdown

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

@github-actions
Copy link
Copy Markdown

This PR has conflicts, please resolve them in order for the PR to be reviewed.

@github-actions
Copy link
Copy Markdown

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

@gregtatcam
Copy link
Copy Markdown
Collaborator Author

/ai-review

Copy link
Copy Markdown
Contributor

@xrplf-ai-reviewer xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 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)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy-paste bug: log says removeDeletedTrustLines but this is removeDeletedMPTs.

Suggested change
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)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Null dereference if view.peek() returns nullptr: (*sleState)[sfIssuer] crashes.

Suggested change
deleteAMMMPToken(view, sleState, (*sleState)[sfIssuer], viewJ) != tesSUCCESS)
sleState && deleteAMMMPToken(view, sleState, (*sleState)[sfIssuer], viewJ) != tesSUCCESS)

{
static_assert(sizeof(MPTID) == (sizeof(std::uint32_t) + sizeof(AccountID)));
AccountID const* accountId =
reinterpret_cast<AccountID const*>(mptid.data() + sizeof(std::uint32_t));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

outstanding lacks a default initializer unlike mptAmount{0}. Default-construction yields indeterminate values.

Suggested change
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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())
{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() &&
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic balances it out. MPTInvariant has the ultimate overflow check.

* 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.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in doc comment: 10 should be to.

Suggested change
* changes OutstandingAmount 10 1,000USD.
* changes OutstandingAmount to 1,000USD.

@github-actions
Copy link
Copy Markdown

This PR has conflicts, please resolve them in order for the PR to be reviewed.

@github-actions
Copy link
Copy Markdown

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;
Copy link
Copy Markdown
Collaborator

@shawnxie999 shawnxie999 Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - exclude line cov

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should it be excluded?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm its seems codecov is complaining that this line is not covered

Comment on lines 798 to 813
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;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - exclude line cov for all returns

UNREACHABLE(
"xrpl::MPTEndpointStep::check : prev seen book without a "
"prev step");
return temBAD_PATH_LOOP;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - exclude line cov

Comment on lines 632 to 640
// 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_,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Amendment Blocked: Needs Final XLS The corresponding final XLS must have been merged before this PR is merged. QE test required RippleX QE Team must look at this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants