Repository navigation
fix(math): fmtCompact promotes the suffix when rounding reaches 1000 - #109
Open
hey-research-lab wants to merge 1 commit into
Open
hey-research-lab wants to merge 1 commit into
hey-research-lab wants to merge 1 commit into
Conversation
fmtCompact picked its unit from the raw value and then rounded inside it, so 999,999 printed as "1000.00K" and a $999,960 compact USD figure (fmtUsd with compact, the OG image and the dashboard stats) as "$1000.0K". fmtQuoteUnits already avoids this by using Intl's compact notation; fmtCompact, which the trade panel, holders panel, token page and fmtTokens use, still did not. A value whose rounded mantissa reaches 1000 now takes the next unit.
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
This fixes a real formatting edge case and the change stays nicely scoped. I checked the signed boundaries, the existing precision settings and the fmtUsd/fmtTokens callers. The 18 math tests and additional boundary checks pass; I didn't find a blocker in the two-file diff.
Full CI hasn't run yet: run 39199114992 is waiting for fork-run approval. Let's get that running and check the app/build results before merging. The green guard and CodeRabbit contexts aren't the full CI result.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
fmtCompact(app/src/lib/launchpad/math.ts) chooses K/M/B/T from the raw value and only then rounds inside that unit. A value just under a unit boundary therefore rounds up to "1000" of the smaller unit:fmtCompact(999_999)1000.00K1.00MfmtCompact(999_996_000)1000.00M1.00BfmtUsd(999_960, { compact: true })$1000.0K$1.0MfmtQuoteUnitswas already fixed for this ("no 1000.00K" inmath.test.ts) by switching to Intl's compact notation, butfmtCompactitself is still used byfmtUsd({ compact: true })(OG image volume, dashboard "Fees earned" / "Holdings value"),fmtTokens, the trade panel (amount out, minimum received, balances), the holders panel, the token page supply label and the trades table.How it was verified
New test in
math.test.ts; against the oldmath.tsit fails:With the fix:
The full app suite (
npm test),tsc --noEmit -p .andeslintwere clean onmainbefore the change (1369 pass, 11 skipped live tests); after the changemath.tstypechecks under--strictand the math suite passes.What changed
fmtCompactwalks a[size, suffix]table; when the rounded mantissa in the chosen unit reaches 1000 it formats in the next unit up. Values below 1,000 and the—/0.00cases are unchanged.Checks run (CONTRIBUTING)
Lint and tsc clean;
npm test: 1,370 passed, 0 failed, 11 skipped; the build completes. No contract, CI or dependency file is touched.Summary by CodeRabbit