Skip to content

chore: fence off legacy BN.js helpers with lint + CODEOWNERS#29386

Merged
weitingsun merged 54 commits into
mainfrom
wsun/introduce-lint-rules-for-bigint-migration
May 1, 2026
Merged

chore: fence off legacy BN.js helpers with lint + CODEOWNERS#29386
weitingsun merged 54 commits into
mainfrom
wsun/introduce-lint-rules-for-bigint-migration

Conversation

@weitingsun
Copy link
Copy Markdown
Contributor

@weitingsun weitingsun commented Apr 27, 2026

Description

Follow-up to the BigInt introduction PR (which adds app/util/number/bigint.ts and marks the helpers in app/util/number/index.js as @deprecated). That PR provides the replacement API; this PR puts the guardrails in place so the migration becomes a one-way ratchet — the count of legacy app/util/number importers can only go down from here.

What this PR adds

  1. @typescript-eslint/no-deprecated: 'warn' in .eslintrc.js for all *.{ts,tsx} files. Surfaces a warning at every use-site of any helper already annotated @deprecated (the app/util/number exports today, plus anything else marked deprecated in the future).

  2. no-restricted-imports fence on app/util/number / app/util/number/index for app/**/*.{ts,tsx,js,jsx}. New imports are a hard error pointing the developer at app/util/number/bigint. Existing importers (107 files at the
    time of writing) live in a top-level utilNumberImportBurndownFiles constant in .eslintrc.js. Entries should be removed as files are migrated, never added. Two override blocks are needed because ESLint's excludedFiles applies to the whole override, not per-rule. A single combined block would silently exempt the burn-down files from the existing expo-haptics and **/controllers/perps fences too. The first override holds all three fences and excludes the burn-down list; the second override re-applies only expo-haptics and perps to the burn-down files.

  3. app/util/number/** is also excluded so the legacy module and its parity tests can keep importing each other for migration parity comparisons.

  4. CODEOWNERS entry: app/util/number/index.js is now owned by @MetaMask/mobile-platform. Any change to the deprecated module (e.g. adding a new export, modifying an existing helper) requires platform review. Scope is intentionally limited to the deprecated file — bigint.ts and the tests are not gated.

  5. Deletion of calcTokenValueToSend from app/util/number/index.js (and its test). The function was dead code (only referenced by its own test).

Why all three layers?

Layer Catches
@typescript-eslint/no-deprecated (warn) Every existing call-site of a deprecated helper, even within the allowlist.
no-restricted-imports (error) Any new file that tries to import from the deprecated module.
CODEOWNERS Any change to the deprecated module itself (new exports, edits).

The combination gives a one-way ratchet: warnings flag what to migrate,
the import fence prevents new debt, and CODEOWNERS prevents the legacy
module from quietly growing.

Changelog

CHANGELOG entry:null

Related issues

Fixes:

Manual testing steps

Feature: my feature name

  Scenario: user [verb for user action]
    Given [describe expected initial app state]

    When user [verb for user action]
    Then [describe expected outcome]

Screenshots/Recordings

Before

After

Pre-merge author checklist

Performance checks (if applicable)

  • I've tested on Android
    • Ideally on a mid-range device; emulator is acceptable
  • I've tested with a power user scenario
    • Use these power-user SRPs to import wallets with many accounts and tokens
  • I've instrumented key operations with Sentry traces for production performance metrics

For performance guidelines and tooling, see the Performance Guide.

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Note

Medium Risk
Primarily tooling/guardrail changes, but new ESLint fences and deprecated-use warnings can break builds for newly added/modified imports and may require follow-up fixes across many files as migration progresses.

Overview
Adds migration guardrails for the BN.js → BigInt transition by enabling @typescript-eslint/no-deprecated (warn) and introducing an ESLint import fence that blocks new imports resolving to deprecated app/util/number/index.js, with a temporary allowlisted burndown set.

Updates CODEOWNERS to require platform review for changes to the deprecated app/util/number/index.js, expands documentation with a burndown-by-team section, and links the migration guide from the main README.

Removes the unused calcTokenValueToSend helper (and its test) from the legacy number module, and increases Node memory limits for lint/tsc scripts to reduce OOMs.

Reviewed by Cursor Bugbot for commit f2197ad. Bugbot is set up for automated code reviews on this repo. Configure here.

Cal-L and others added 30 commits March 3, 2026 16:16
…etaMask/metamask-mobile into feat/MCWP-370-introduce-native-big-int
…etaMask/metamask-mobile into feat/MCWP-370-introduce-native-big-int
Made-with: Cursor
Comment thread .eslintrc.js
…ub.com:MetaMask/metamask-mobile into wsun/introduce-lint-rules-for-bigint-migration
Cal-L
Cal-L previously approved these changes Apr 30, 2026
Copy link
Copy Markdown
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

LGTM

Cal-L
Cal-L previously approved these changes Apr 30, 2026
Copy link
Copy Markdown
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread .eslintrc.js Outdated
@weitingsun weitingsun requested a review from Cal-L May 1, 2026 00:16
…ub.com:MetaMask/metamask-mobile into wsun/introduce-lint-rules-for-bigint-migration
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 683dd65. Configure here.

Comment thread .eslintrc.js
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 1, 2026

🔍 Smart E2E Test Selection

  • Selected E2E tags: None (no tests recommended)
  • Selected Performance tags: None (no tests recommended)
  • Risk Level: low
  • AI Confidence: 97%
click to see 🤖 AI reasoning details

E2E Test Selection:
This PR is a developer tooling and documentation change with no runtime app behavior modifications:

  1. app/util/number/index.js: Removes calcTokenValueToSend function, which was already marked @deprecated no longer used. Confirmed via grep that zero files in the codebase import or call this function - it's a dead code removal.

  2. app/util/number/index.test.ts: Removes the corresponding test for the deleted function. No impact on app behavior.

  3. .eslintrc.js: Adds ESLint configuration for the BN.js → BigInt migration: a burndown allowlist, @typescript-eslint/no-deprecated warning rule, and import-x/no-restricted-paths fence. This is purely a linting/static analysis change with no runtime effect.

  4. package.json: Only changes Node.js memory limits for lint and tsc scripts (8192MB → 12288MB). No dependency additions, removals, or version changes. No impact on the app bundle or runtime.

  5. .github/CODEOWNERS: Adds ownership for app/util/number/index.js. Governance change only.

  6. README.md and docs/bigint-migration-guide.md: Pure documentation additions.

No E2E tests are needed because:

  • No user-facing functionality changed
  • No controllers, Engine, or state management modified
  • No UI components affected
  • The removed function had zero usages in the codebase
  • ESLint/tooling changes don't affect app runtime behavior

Performance Test Selection:
No performance-relevant changes. The package.json change only increases memory limits for lint/tsc CLI scripts, not the app runtime. No UI components, data loading, state management, or critical user flows were modified.

View GitHub Actions results

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

LGTM

@weitingsun weitingsun added this pull request to the merge queue May 1, 2026
Merged via the queue into main with commit 6efa0ce May 1, 2026
59 of 61 checks passed
@weitingsun weitingsun deleted the wsun/introduce-lint-rules-for-bigint-migration branch May 1, 2026 08:58
@github-actions github-actions Bot locked and limited conversation to collaborators May 1, 2026
@metamaskbotv2 metamaskbotv2 Bot added the release-7.77.0 Issue or pull request that will be included in release 7.77.0 label May 1, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

release-7.77.0 Issue or pull request that will be included in release 7.77.0 size-M team-mobile-platform Mobile Platform team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants