Support for MPT-DEX in Explorer#1302
Conversation
…saction-Simple view page
|
|
||
| const normalize = (value: number | string, currency: string): string => | ||
| currency === 'XRP' ? (Number(value) / XRP_BASE).toString() : String(value) | ||
| const getCurrency = (takerAmount: any): string => { |
There was a problem hiding this comment.
Can we use a specific type for takerAmount instead of any?
There was a problem hiding this comment.
Hello! thanks for the review. I can think of two alternatives for the question of type safety:
-
Wait for this PR to be merged -- this provides for
MPTAmountas a variant of theAmounttype in xrpl package. -
Introduce an interface in all these files that simulates the true
Amounttype i.e.
interface AmountType {
value: string
issuerAccount?: string
currency?: string
mpt_issuance_id?: string
}
Option-1 is more type-safe because 'xrpl' package includes additional validation for the different kinds of Amount -- IOU, MPT or XRP.
What do you suggest?
There was a problem hiding this comment.
Maybe we can add the shared interface in option 2 and add a TODO comment to use xrpl package when that PR is merged and released so we don't have to wait
| return takerAmount?.currency || 'XRP' | ||
| } | ||
|
|
||
| const getIsMPT = (takerAmount: any): boolean => !!takerAmount?.mpt_issuance_id |
There was a problem hiding this comment.
Use a specific type instead of any
|
|
||
| const getIsMPT = (takerAmount: any): boolean => !!takerAmount?.mpt_issuance_id | ||
|
|
||
| const getIssuer = (takerAmount: any): string | undefined => { |
There was a problem hiding this comment.
Use a specific type instead of any
| const isCurrency = (value: any) => | ||
| isMPTAsset(value) || | ||
| (typeof value === 'object' && | ||
| Object.keys(value).length <= 2 && |
There was a problem hiding this comment.
Is 2 something we should put as a constant at the top of the file?
There was a problem hiding this comment.
000003C31D321B7DDA58324DC38CDF18934FAFFFCDF69D5F is used several times in this file. Can we define this value at the top of the file and then use a constant? Something like
export const TEST_MPT_ID = '000003C31D321B7DDA58324DC38CDF18934FAFFFCDF69D5F'
| }) | ||
|
|
||
| it('renders', () => { | ||
| const { container, unmount } = renderComponent(mockAMMDelete) // TOOD: - Make this look up asset 1 / asset 2 currency codes |
There was a problem hiding this comment.
Has this // TODO been addressed?
There was a problem hiding this comment.
Hello, no the TODO comment has not been addressed. I was not convinced that the TODO is relevant.
Most of the Explorer pages render the incoming data in Simple, Description, Detail and Raw views. They do not explicitly fetch the ledger-object in question. The exception to this behavior is the fetching of mpt_issuance_id in Vault Explorer pages because we need the Vault-metadata.
However, in the case of AMMs, I do not understand why we need to fetch the AMM ledger-object. Indeed, I have not found examples of such behavior in other AMM related transactions (like AMMCreate).
what do you think?
There was a problem hiding this comment.
We should add a new test file to cover this, specifically the error handling
There was a problem hiding this comment.
-
getMPTDisplayNamemethod is already covered by the tests in theCurrency with MPT tickerdescribe block ofcurrency.test.tsxfile. -
useMPTIssuanceis a thin wrapper arounduseQuery. Did you have any other parameters in mind for the tests? ad688b6
There was a problem hiding this comment.
With all the changes to this file, we should add more tests to/update the DefaultSimple.test file
| return s | ||
| } | ||
|
|
||
| export function normalizeAmount( |
There was a problem hiding this comment.
We should add a test for this function
There was a problem hiding this comment.
On second thought -- the changes in this file are not required. This file is only used in PaymentChannelClaim and TrustSet transactions. Both of those transactions have nothing to do with MPTs
There was a problem hiding this comment.
Since 000003C31D321B7DDA58324DC38CDF18934FAFFFCDF69D5F is used multiple times in this file we can assign it as a constant at the top of the file and just reuse that constant throughout the file
| export const formatAsset = (asset) => { | ||
| if (typeof asset === 'string') return { currency: 'XRP' } | ||
| if (asset.mpt_issuance_id) { | ||
| return { | ||
| currency: asset.mpt_issuance_id, | ||
| isMPT: true, | ||
| } | ||
| } | ||
| return { | ||
| currency: asset.currency, | ||
| issuer: asset.issuer, | ||
| } | ||
| } |
There was a problem hiding this comment.
We should add a test for this change
| import { | ||
| formatAmount, | ||
| isMPTAmount, | ||
| formatAsset, |
There was a problem hiding this comment.
There are 2 separate formatAsset (from formatAmount and utils.js). Can we dedup and just use one?
| const amount = | ||
| node.FinalFields?.MPTAmount != null | ||
| ? Math.abs( | ||
| Number(node.FinalFields.MPTAmount) - |
There was a problem hiding this comment.
I think we should use BigInt for MPTAmount since maximum for Number is only around 53 bits so this might be inaccurate for large amount
| if (typeof amount === 'object' && 'mpt_issuance_id' in amount) { | ||
| const currency = amount.mpt_issuance_id | ||
| const numberOption = { ...CURRENCY_OPTIONS, currency } | ||
| return localizeNumber(amount.value, language, numberOption) |
There was a problem hiding this comment.
Do we need to apply asset scale for this value? See convertScaledPrice in utils.js?
There was a problem hiding this comment.
the changes in this file are not required. I have reverted them.
- Update test imports from react-router-dom to react-router (deprecated post-v7) - Replace @testing-library/react-hooks with @testing-library/react (merged renderHook) - Add `as any` cast to <Trans> interpolation objects in Offer.tsx, matching the pattern already applied to sibling `change:` blocks during the React 18 upgrade Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| const changePays = normalize(prevPays - finalPays, paysCurrency, paysIsMPT) | ||
| const changeGets = normalize(prevGets - finalGets, getsCurrency, getsIsMPT) |
There was a problem hiding this comment.
Do we need to use BigInt here like we do in src/containers/shared/metaParser.tsx?
Mirror the precision-preserving pattern already used in metaParser.tsx so large MPTAmount values (up to 2^63 - 1) are not truncated when computing the change between previous and final TakerPays / TakerGets. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
MPTAmount values can be up to 2^63 - 1, beyond Number.MAX_SAFE_INTEGER. Three layers were silently truncating large MPT values: - Amount.tsx routed `amount` through parseInt before scaling, losing precision before convertScaledPrice could apply BigInt arithmetic. - formatAmountWithAsset parseFloat'd the value before reaching the MPT branch, so the returned `amount` was already lossy. - localizeNumber parseFloat'd its input as the first step, undoing any precision preserved upstream. Fix each layer to keep MPT amounts as exact integer/decimal strings and add a BigInt-backed grouping path in localizeNumber that activates only for isMPT string inputs. Non-MPT call sites are unaffected. Tests cover MPT amounts at the spec maximum (2^63 - 1) at both assetScale 0 and 6, and IOU amounts near the ripple-binary-codec limits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| { | ||
| previous: localizeNumber( | ||
| normalize(prevPays, paysCurrency), | ||
| normalize(prevPays, paysCurrency, paysIsMPT), |
There was a problem hiding this comment.
localizeNumber has a argument isMPT, should we set it in all of the localizeNumber calls here (e.g payIsMPT) so that BigInt will be execute?
There was a problem hiding this comment.
follow-up in b02b7c6: added regression tests in Meta.test.tsx covering all 6 localizeNumber calls, plus mixed MPT/XRP offers and the MPToken/MPTokenIssuance renderers. they surfaced a small bug where Intl.NumberFormat rejected the mpt_issuance_id as a currency code (it validates currency even with style: decimal) — fix is in the same commit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Intl.NumberFormat validates `currency` even when `style: 'decimal'`, so the BigInt branch threw on mpt_issuance_id values (48-hex, not ISO). Adds regression tests on the Offer meta renderer covering all 6 localizeNumber calls plus mixed MPT/XRP offers and MPToken / MPTokenIssuance renderers at full precision. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
High Level Overview of Change
This feature implements the MPT-DEX feature in Explorer.
English specification: https://github.com/XRPLF/XRPL-Standards/pull/451/changes
C++ implementation of MPT-DEX feature: XRPLF/rippled#5285
xrpl.js library implementation of MPT-DEX feature: XRPLF/xrpl.js#3214
Please watch the attached video for the visual experience of a few transactions that highlight the changes in this PR:
explorer-mpt-dex.mov
This PR updates the visual look of the following transactions:
AMMCreate
AMMDeposit
AMMWithdraw
AMMClawback
AMMDelete
Payment
OfferCreate
Any other transaction that displays MPT / IOU values in the Simple/ Description/Meta/Detail views of the Explorer.
The PR also updates the IOUs and MPTs to appear in green color in the Simple view for all transactions (commit: 7498ce0). This indicates that it is a clickable link.
This PR has been deployed in the staging environment of the Explorer. However, since MPT-DEX feature is unavailable on a public devnet, readers will need to run a custom rippled image to be able to play around the feature (as demonstrated in the video)
Context of Change
Type of Change
Codebase Modernization
Before / After
Please watch the attached video for before/after changes
Test Plan
Appropriate tests have been added