Conversation
![ripple-code-reviewer[bot]](https://avatars.githubusercontent.com/u/3165523?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a well-scoped, well-tested fix for the four reported MPT object page display bugs plus a shared circulating-supply util extracted for both token pages. The BigInt-based supply math, RWA skip logic, registrable-domain parsing, and JsonView string-collapse fix all look correct and are backed by targeted unit tests that match the described before/after behavior. I did not find any high-confidence correctness or security issues in the added/changed lines.
…p as a registrable domain, keeping shortenDomain for NFT URLs
| @@ -1,48 +0,0 @@ | |||
| import { useMemo } from 'react' | |||
There was a problem hiding this comment.
Removed as it's dead code that isn't used anywhere.
| @@ -1,273 +0,0 @@ | |||
| import { renderHook } from '@testing-library/react' | |||
There was a problem hiding this comment.
Removed as it's dead code that isn't used anywhere.
| @@ -1,217 +0,0 @@ | |||
| import { | |||
There was a problem hiding this comment.
Removed as it's dead code that isn't used anywhere.
| @@ -1,38 +0,0 @@ | |||
| import { parseIntegerAmount } from '../../../shared/NumberFormattingUtils' | |||
There was a problem hiding this comment.
Removed as it's dead code that isn't used anywhere.
There was a problem hiding this comment.
Solid, well-tested fix for the MPT supply/circ-supply, issuer-name, metadata-truncation, and website-chip issues, with the shared BigInt-based circulating-supply util extracted cleanly from the IOU page. The one thing worth double-checking before merge is that the RWA detection (isRwaAssetClass) is a strict, case-sensitive equality check applied to two different data sources: a curated LOS field for IOU and raw on-chain metadata (written by arbitrary token issuers) for MPT. The tests explicitly lock in strict-only matching, so this looks intentional, but on-chain data isn't guaranteed to be spec-compliant the way a controlled backend field is — worth confirming with the author that this was a deliberate choice for the on-chain case specifically.
There was a problem hiding this comment.
Well-scoped fix for the four reported MPT object-page display bugs (supply/circ-supply, issuer name truncation, metadata clipping, website chip TLD destruction), plus a shared BigInt-based circulating-supply util extracted for both IOU and MPT pages with solid unit test coverage. The diff is clean and most of the changed code (MarketData, GeneralOverview, Metadata, DomainLink, domainUtils, circulatingSupply utils) matches its own tests and the described behavior. One spot worth a second look before merging: a new field in the MPT holders mapper references a bare balance identifier whose scope isn't visible in this diff — worth a quick confirmation it resolves to the intended unscaled BigInt rather than an undefined variable.
|
|
||
| const supply = Number(tokenData.supply) || holdersData?.totalSupply || 0 | ||
|
|
||
| if (isRwaAssetClass(tokenData.asset_class)) { |
There was a problem hiding this comment.
This checks if asset_class is rwa, but the function it replaces checked if asset_subclass was stablecoin. Is this intentional?
There was a problem hiding this comment.
It's intentional. Previously, we only perform the check against stablecoin, which is a subset of RWA. Now, we have decided (confirmed with our PM) to extend the check to all RAW assets
There was a problem hiding this comment.
Solid refactor: replaces the broken maxAmt-based supply figure with real OutstandingAmount/holder-based circulating supply (BigInt-safe), removes address-style truncation from the issuer name and metadata URIs, and consolidates domain-shortening logic into a shared, unit-tested domainUtils module. Logic in the new BigInt-based MPT circulating-supply util, the JsonView collapse sentinel, and the domain helpers all check out against their tests. The one thing worth double-checking before merge is the semantic swap in the shared IOU calculation from asset_subclass !== 'stablecoin' to asset_class === 'rwa'.
1c68713 to
059582a
Compare
There was a problem hiding this comment.
Solid, well-tested change set that fixes the four MPT display bugs and correctly shares circulating-supply logic between the MPT and IOU pages. The MPT circulating-supply math now correctly uses BigInt to avoid float residue, and the domain-truncation/URI-truncation fixes look sound with good test coverage. The one thing worth double-checking before merge is a field-name swap in the extracted IOU circulating-supply logic that changes which tokens get the large-holder exclusion.
…ating supply translation key
There was a problem hiding this comment.
Overall this is a well-scoped, well-tested fix for the four reported MPT display bugs plus a legitimate shared circulating-supply util. Most of the diff (translations, dead-code removal, domain/JsonView refactors) looks solid and matches the description. The one thing worth double-checking before merge is a likely undefined-variable reference in the new rawBalance field in holders.ts, which would break the MPT holders fetch (and therefore the whole new circulating-supply feature) if balance isn't actually a variable in scope at that point.
| // Circulating supply is only meaningful once holders resolve. While the fetch | ||
| // is pending (or hasn't started yet) show a spinner; if it failed, leave it | ||
| // undefined so the header renders "--" rather than the unadjusted supply. | ||
| const circulatingSupplyLoading = |
There was a problem hiding this comment.
Is there a chance that mptokenIssuance.issuer could ever be missing? If it is ever missing, then the holders query never runs so holdersLoading, holdersData, and holdersError are false, undefined, and false, respectively. This would mean that circulatingSupplyLoading is true and the loading spinner in MarketData.tsx never resolves
There was a problem hiding this comment.
Issuer is a required field on MPTokenIssuance, so any issuance we successfully fetch has it.
| const isRwa = isRwaAssetClass(mptokenIssuance?.parsedMPTMetadata?.asset_class) | ||
| const circulatingSupply = useMemo( | ||
| () => | ||
| holdersData |
There was a problem hiding this comment.
Does it make sense to have circulatingSupply dependent on holdersData even in the case where isRwa is true? The RWA branch of calculateMptCirculatingSupply never uses holder data. The spinner will appear until the holder fetch resolves even when the holder data isn't needed
There was a problem hiding this comment.
Good point. I updated the code so that circulatingSupply is computed immediately when isRwa is true without relying on the holder data (see 5065260)
…blocking on the holders fetch
There was a problem hiding this comment.
This is a well-scoped, well-tested fix for the four reported MPT object page display bugs plus a legitimate refactor that shares circulating-supply logic between the MPT and IOU pages. I traced the Supply/Circ Supply math (BigInt-based, floors at 0, exact for values beyond double precision), the issuer-name/metadata truncation removals, and the new domainUtils helpers (getRegistrableDomain, shortenDomainFromLeft, shortenDomain) against their unit tests and callers, and didn't find correctness or security problems in the changed lines. The only thing worth a quick sanity check is the new rawBalance field in holders.ts.
There was a problem hiding this comment.
Solid, well-tested fix for the reported MPT display bugs (supply/circ-supply, issuer name truncation, metadata clipping, domain-chip TLD loss) with logic properly extracted into shared, unit-tested utilities. Found two things worth a second look before merge: a possibly-undefined balance identifier newly referenced in the MPT holders mapper, and a missing floor-at-zero guard in the newly-extracted IOU subtractLargeHolderBalances helper (the sibling MPT helper added this exact guard in the same PR).
3 participants
High Level Overview of Change
Fixes four reported display issues on the MPToken object page, and replaces the placeholder circulating-supply figure with a real calculation shared with the IOU page.
Supplyis now the on-chainOutstandingAmount;Circ Supplysubtracts holders owning ≥ 20% (skipped for RWA tokens). Fixes the reported "circulating supply > supply".franklintempleton.com) instead of a middle-truncated string that destroyed the TLD; all domain helpers are consolidated indomainUtils.Context of Change
Reported against livenet.xrpl.org/mpt/064E27366D15D1F5614B4D4E3183F835BCD62E7F6FD9FFD8 (Franklin
sgBENJI). Each root cause was confirmed against the liveledger_entry mpt_issuanceresponse rather than inferred.1. Circulating supply > supply. This issuance sets no
MaximumAmount, somaxAmtwasundefinedandMarketDatarenderedBigInt(maxAmt || '0')→ Supply = 0, while Circ Supply showed the real ~28.14. Per spec an unset maximum means the cap is2^63-1, which is meaningless to display.After product review the fix is: Supply =
OutstandingAmount, and Circ Supply = Supply minus holders ≥ 20% — the same rule the IOU page already applied. Large holders are issuer/treasury/whale wallets that are not meaningfully in circulation. The exclusion is skipped for RWA tokens (asset_class === 'rwa'), whose large holders are custodians; stablecoins are an RWA subclass and are covered by the same check.2. Truncated issuer name.
shortenAccount()— a 7…5 address truncator — was applied to the human-readable issuer name, rendering "Franklin Templeton Investments" as "Frankli…ments".3. Truncated metadata. The shared
JsonViewhard-codedcollapseStringsAfterLength={65}, clipping the 122-character URI in the metadata panel.4. Website chip destroyed the TLD.
shortenDomain(url, 12, 7)middle-truncates, producingwww.franklin…hnology. The TLD is the part users rely on to judge whether a link is safe, so website chips now show the registrable domain parsed with a real public-suffix list (tldts) — correctly keepingthing.co.ukrather than collapsing it toco.uk.tldtswas already in the tree as a transitive dependency and is promoted to a direct one. The same treatment is applied to the IOU issuer chip. The NFT URL column keepsshortenDomain— its per-NFT resource URIs need the path to stay visible, so domain-only display would collapse distinct rows. All three helpers (getRegistrableDomain,shortenDomainFromLeft,shortenDomain) now live together inshared/domainUtils.ts; non-HTTP schemes (ipfs://,ar://) fall back to the host/CID token.Shared logic. The ≥20% and RWA rules now live in
Token/shared/utils/circulatingSupply.tsand are used by both token pages. IOU's calculation was promoted out of its component into that module so it can be unit-tested directly.isRwaAssetClassacceptsunknownand strict-compares against'rwa', so untrusted, non-string issuer metadata compares unequal rather than throwing.Dead code.
IOU/hooks/useMarketCalculations.tsandIOU/utils/tokenCalculations.ts(plus their tests) were duplicates reachable only from their own tests — the shipping IOU page had its own copy.Type of Change
Codebase Modernization
All touched and new files are function components / TypeScript. New modules (
shared/domainUtils.ts,Token/shared/utils/circulatingSupply.ts,Token/MPT/utils/circulatingSupply.ts) are TypeScript.Before / After
Market Data box
0.00(noMaximumAmount)28.14(on-chainOutstandingAmount)28.14(> Supply)≤ Supply, excluding ≥20% holdersCirc Supply shows a spinner while holders load, and
--if that request fails — it never silently shows the unadjusted supply as if it were a real circulating figure.Header
Frankli…ments(truncated, in title)www.franklin…hnology(TLD destroyed)franklintempleton.com, full URL on hoverRule summary
asset_classsourceasset_class === 'rwa'asset_class === 'rwa'Stablecoins are an RWA subclass, so this single
asset_class === 'rwa'check covers them on both pages.Test Plan
Automated — all green:
npm run build-ts— no type errorsnpm run lint:ci— ESLint, Stylelint, Prettier (--max-warnings 0)npm run test:ci— 1713 tests pass, coverage above the 70/80 thresholdspre-commit run --all-filesnpm run build— production bundle builds with the new dependencyNew/updated tests:
Token/shared/test/utils/circulatingSupply.test.ts— threshold boundary (exactly 20%), large-holder subtraction,isRwaAssetClass(strict lowercase match + non-string/unknownsafety), andcalculateIouCirculatingSupply(reported supply, supply fallback, RWA skip)Token/MPT/test/utils/circulatingSupply.test.ts— asset-scale handling, RWA skip, negative floor, and two exact-integer regressions: float residue →0, and amounts beyond double precisionshared/test/domainUtils.test.ts— registrable domain (incl.thing.co.uk), non-URL fallback, left-truncation preserving the TLD, andshortenDomainmiddle-truncationMarketData/Header/Metadata/GeneralOverview/DomainLink— Supply vs Circ Supply, spinner and--states, untruncated issuer name and metadata,titletooltipManual:
npm start, open/mpt/064E27366D15D1F5614B4D4E3183F835BCD62E7F6FD9FFD8(see screenshot below) — Supply is the full outstanding amount, Circ Supply is ≤ Supply (cross-check excluded accounts against Holders table rows with % ≥ 20), the chip readsfranklintempleton.comwith the full URL on hover, the issuer name renders in full, and the metadata URI is not clipped. IOU token pages show unchanged supply and market-cap values after the refactor.