Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Cal-L
added
no-changelog
|
Cursor note: The only thing worth keeping an eye on as the migration continues is ensuring callers that use localizeLargeNumber with { includeK: true } or custom decimals remain pointed at index.js rather than bigint.ts until those options are added to the new module. |
| let fiatFixed = Math.round(Number(value) * base) / base; | ||
| fiatFixed = isNaN(fiatFixed) ? 0.0 : fiatFixed; | ||
| if (currencySymbols[currencyCode]) { | ||
| return `${currencySymbols[currencyCode]}${fiatFixed}`; |
There was a problem hiding this comment.
renderFiat loses negative value currency formatting
Medium Severity
The renderFiat function in bigint.ts lost the negative value handling present in index.js. The old version extracts the sign, computes the absolute value, and formats as -$5. The new version directly interpolates fiatFixed (which may be negative) after the currency symbol, producing $-5 instead of -$5. This will cause incorrect currency formatting for any negative fiat value once callers migrate to this module.
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection:
These utilities are cross-cutting and likely consumed by:
Because numeric correctness directly affects user-visible balances, transaction values, gas fees, fiat displays, and swap inputs, this is a high-risk change despite being “just utilities.” Any regression could break confirmations, swaps, perps deposits, prediction positions, or balance rendering. Selected tags:
Excluded:
Given the breadth of numeric impact across all financial flows, broad E2E coverage is justified. Performance Test Selection: |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| type SignedHex = `0x${string}` | `-0x${string}`; | ||
|
|
||
| const MAX_DECIMALS_FOR_TOKENS = 36; | ||
| BigNumber.config({ DECIMAL_PLACES: MAX_DECIMALS_FOR_TOKENS }); |
There was a problem hiding this comment.
Global BigNumber config mutates shared library state
Medium Severity
BigNumber.config({ DECIMAL_PLACES: MAX_DECIMALS_FOR_TOKENS }) is a module-level side effect that globally changes the default decimal places from 20 to 36 for every BigNumber instance across the entire application. Any module that imports from bigint.ts (even indirectly) will silently alter the precision behavior of all existing bignumber.js consumers, including index.js and any other code relying on the default DECIMAL_PLACES of 20. This can cause subtle numerical differences in unrelated calculations throughout the app.
| typeof valueInput === 'string' ? valueInput : valueInput.toString(); | ||
| const [from, to] = [value.indexOf(divider), 0]; | ||
| return value.substring(from, to) || value; | ||
| } |
There was a problem hiding this comment.
fastSplit returns empty string for divider-at-start inputs
Medium Severity
fastSplit uses value.substring(from, to) where from is the divider index and to is always 0. When the divider is at position 0 (e.g. ".123" with default "." divider), substring(0, 0) returns "", and the || value fallback returns the entire string ".123" instead of the expected empty prefix. More critically, substring with from > to swaps the arguments, so when the divider is found at any position, it actually returns substring(0, index) — meaning the variable names from and to are misleading and the logic is coincidentally correct only due to JavaScript's substring argument-swapping behavior. This fragile, confusing implementation risks future maintenance bugs.
|
✅ E2E Fixture Validation — Schema is up to date |
|
3 participants
Description
This PR only introduces the Big Int module from #20949 to reduce risk of breaking features. We will ask teams to incrementally update and test their areas once the module is used.
Changelog
CHANGELOG entry:
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Adds a new BigInt-based numeric utility module and adjusts existing
addHexPrefix/renderNumberbehavior; mistakes here could impact value/fee formatting and conversions across the app. Changes are well-covered by extensive new unit tests but touch core number/hex handling.Overview
Introduces a new
app/util/number/bigint.tsmodule that mirrors the existing number helpers but uses nativebigint(and@metamask/utils) for wei/token-unit conversions, fiat rendering, hex handling, and large-number localization, plus adds helpers likesafeNumberToBigInt,safeBigIntToHex,toGwei, andbigIntAbs.Updates the legacy
app/util/number/index.jsto refineaddHexPrefix(handle non-strings, normalize0X/-0X) and fixesrenderNumberto return the full string when no decimal point is present. Adds comprehensive test coverage for the new BigInt module and extends existing tests to cover the adjusted behaviors.Written by Cursor Bugbot for commit eef4f85. This will update automatically on new commits. Configure here.