feat(perps): sync controller code with extension#28509
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. |
Two changes enable @metamask/perps-controller to be consumed from the extension without pulling @myx-trade/sdk into the static webpack bundle or violating the LavaMoat policy: 1. Add /* webpackIgnore: true */ magic comment to the MYXProvider dynamic import so webpack skips static resolution of the intentionally-unshipped module. Runtime behavior on mobile's Metro bundler is unchanged — the magic comment is extension/webpack-only. 2. Remove the 8 MYX adapter functions (adaptMarketFromMYX, adaptPriceFromMYX, adaptMarketDataFromMYX, filterMYXExclusiveMarkets, isOverlappingMarket, buildPoolSymbolMap, buildSymbolPoolsMap, extractSymbolFromPoolId) from the public barrel and drop the ./myxAdapter re-export from utils/index.ts. These functions are still used internally by MYXProvider, which imports them via relative path (../utils/myxAdapter) — no runtime change. Drift cleanup (required to unblock the core sync): 3. Move app/util/perpsConnectionAttemptContext.ts into app/controllers/perps/utils/ so HyperLiquidClientService can import it via a relative path that stays inside the synced perps package. The mobile-external consumer (PerpsConnectionManager) now imports withPerpsConnectionAttemptContext from @metamask/perps-controller instead of a deep relative path, satisfying the no-restricted-imports rule for code outside app/controllers/perps/. Note: an earlier draft of this commit also declared MetaMetricsController:trackEvent in PerpsControllerAllowedActions to let the extension drop a type cast. That was reverted because mobile has no MetaMetricsController (uses MetaMetrics singleton), so adding it forced @metamask/messenger's parent type to narrow to never and broke perps-controller-messenger/index.ts. Extension keeps its existing cast workaround until both host apps share a real MetaMetricsController.
abretonc7s
force-pushed
the
feat/tat-2863-sync-controller-code-extension
branch
from
2c400fa to
8b18a2b
Compare
|
Two fixes, both uncovered while getting core PR #8398 green: 1. Preflight freshness check (the main fix) The previous preflight only compared the core working tree against .sync-state.json.lastSyncedCoreCommit, so it could not detect that someone else had merged a competing perps-controller change into origin/main while the current branch was in review. That gap let #8398 land in a conflicting state with #8333. New check at the top of step_conflict_check: - Fetches origin/main quietly. - Runs `git rev-list --count HEAD..origin/main -- packages/perps-controller/`. - Hard-fails (not warns) with the offending commit list + a merge suggestion if any such commits exist. - Gracefully degrades to a WARN if offline or if origin/main is missing. This would have caught the #8333 collision before any file was copied, forcing a merge-then-sync workflow instead of a merge-after-sync cleanup. 2. Prettier pass on eslint-suppressions.json Core's lint:misc:check runs prettier on eslint-suppressions.json, but `yarn eslint --suppress-all` writes the file in a format that prettier disagrees with (trailing newline, key quoting). This caused PR #8398's lint:misc job to fail on a file that our own sync script had just written. Added a `yarn prettier --write eslint-suppressions.json` step at the end of the ESLint auto-fix stage so the sync output stays lint-clean.
Adds `BinaryExpression[operator='in']` (along with `WithStatement` and `SequenceExpression`) to the mobile `.eslintrc.js` perps-controller override so it mirrors core's `@metamask/eslint-config` base rules. Without this, `'x' in y` type-guards in the perps package passed mobile lint but landed in core as `no-restricted-syntax` suppressions, and the extension team then saw them as hard errors in their editor. Replaces 24 simple existence-check uses of `'x' in obj` with `hasProperty(obj, 'x')` from `@metamask/utils` across: - providers/HyperLiquidProvider.ts (9 replacements) - services/HyperLiquidSubscriptionService.ts (4) - services/TradingService.ts (1) - types/index.ts (2) - types/transactionTypes.ts (4) - utils/errorUtils.ts (1) - utils/hyperLiquidAdapter.ts (2) - utils/marketDataTransform.ts (1) Four uses where `in` provides discriminated-union narrowing that `hasProperty` does not (the HyperLiquid SDK status-union branches in `HyperLiquidProvider.ts` and the `HYPERLIQUID_ASSET_CONFIGS` keyof narrowing in `hyperLiquidValidation.ts`) are kept with inline `/* eslint-disable no-restricted-syntax */` comments and explanatory notes. Net effect in core: perps-controller suppression count drops from 30 to 6 on the next sync. All four remaining suppressions for `no-restricted-syntax` are the intentional inline-disabled discriminated-union narrowings above.
Extend `scripts/perps/validate-core-sync.sh` step 6 to snapshot per-file/per-rule suppression counts from `eslint-suppressions.json` BEFORE running `--fix` / `--suppress-all` / `--prune-suppressions`, then diff against the post-run counts and hard-fail if any (file, rule) pair's count increased. Reducing counts (a mobile fix that removes a previously-suppressed violation) is always allowed. Increases mean the current sync is introducing NEW violations that would land in core as fresh suppressions — e.g. a new `'x' in y` use, a new `@typescript-eslint/*` violation. Those must be fixed at source in mobile before the sync can proceed. Replaces the old "WARN if count > 20" heuristic with a precise, actionable per-entry report that lists every offending file+rule pair and points at `hasProperty()` from `@metamask/utils` as the canonical replacement for `'x' in y`. This is the canonical local detection point for the problem that "it should have been detected locally!" — a violation now fails the sync script at step 6 BEFORE the core PR is opened.
abretonc7s
force-pushed
the
feat/tat-2863-sync-controller-code-extension
branch
from
25763af to
a41b258
Compare
github-actions
bot
added
size-M
risk-medium
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ 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 a41b258. Configure here.
Bugbot caught that `hasProperty(provider, 'closePositions')` from `@metamask/utils` is `Object.hasOwnProperty.call(...)` under the hood, which only checks own properties and never traverses the prototype chain. Since `provider` is a class instance and `closePositions` lives on the class prototype, the diagnostic log was always reporting `hasBatchMethod: false` even when the provider supports batch close. The actual feature gate at line 1527 (`if (provider.closePositions)`) still works because normal property access traverses prototypes, so runtime behavior is unchanged. Simplify by dropping the bogus `hasBatchMethod` field from the debug log entirely instead of working around it with `'in'` + an eslint disable comment. The remaining `providerKeys` field already gives enough signal for debugging.
…ntroller-code-extension
github-actions
bot
added
risk-medium
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection:
Tag Selection Rationale:
Not selected:
Performance Test Selection: |
Sync of mobile commit ec93364a98 (PR MetaMask/metamask-mobile#28509). Bugbot caught that `hasProperty(provider, 'closePositions')` from `@metamask/utils` is `Object.hasOwnProperty.call(...)` under the hood, which only checks own properties and never traverses the prototype chain. Since `provider` is a class instance and `closePositions` lives on the class prototype, the diagnostic log was always reporting `hasBatchMethod: false` even when the provider supports batch close. The actual feature gate at line 1525 (`if (provider.closePositions)`) still works because normal property access traverses prototypes, so runtime behavior is unchanged. Simplified by dropping the bogus `hasBatchMethod` field from the debug log entirely instead of working around it with `'in'` + an eslint disable comment.
abretonc7s
added
skip-sonar-cloud
|
|
✅ E2E Fixture Validation — Schema is up to date |
weitingsun
added
release-7.74.0
4 participants
Description
Makes
@metamask/perps-controllersafe to consume from the MetaMask extension without pulling@myx-trade/sdkinto the static webpack bundle or violating the LavaMoat policy. Mobile owns the source-of-truth for the perps controller package viascripts/perps/validate-core-sync.sh, so the fix is made in mobile and then rsync'd to core on a matching branch.Companion core PR: MetaMask/core#8398 — supersedes MetaMask/core#8374.
What changed
app/controllers/perps/PerpsController.ts— add/* webpackIgnore: true */magic comment to theMYXProviderdynamicimport()so webpack (extension) skips static resolution of the intentionally-unshipped module. Metro (mobile) ignores the magic comment, so runtime behavior on mobile is unchanged.app/controllers/perps/index.ts— remove 8 MYX adapter functions from the public barrel (adaptMarketFromMYX,adaptPriceFromMYX,adaptMarketDataFromMYX,filterMYXExclusiveMarkets,isOverlappingMarket,buildPoolSymbolMap,buildSymbolPoolsMap,extractSymbolFromPoolId). These are still used internally byMYXProvider, which imports them via relative path../utils/myxAdapter. No runtime change in mobile — the functions are still available to MYXProvider.app/controllers/perps/utils/index.ts— drop theexport * from './myxAdapter'barrel re-export so the utils index no longer transitively re-exports MYX symbols.Drift fix —
perpsConnectionAttemptContext— moved fromapp/util/perpsConnectionAttemptContext.tsintoapp/controllers/perps/utils/perpsConnectionAttemptContext.tsand exported from@metamask/perps-controllerso:HyperLiquidClientService(inside the perps package) imports it via a relative path that stays inside the synced directory — required for the core sync to build.PerpsConnectionManager(outside the perps package) imports it from@metamask/perps-controller— satisfies theno-restricted-importsrule for code outsideapp/controllers/perps/..eslintrc.js— align mobile perps override with core's base rules. AddedBinaryExpression[operator='in'],WithStatement, andSequenceExpressionselectors to the existingno-restricted-syntaxrule in theapp/controllers/perps/**override. Mobile's override was missing these, which meant'x' in ytype-guards passed mobile lint silently and then landed in core as newno-restricted-syntaxsuppressions at sync time. The override now mirrors what@metamask/eslint-configenforces in core, catching the violations at source.Replace
'x' in ywithhasProperty(y, 'x')across 8 perps-controller files —HyperLiquidProvider.ts,HyperLiquidSubscriptionService.ts,TradingService.ts,marketDataTransform.ts,hyperLiquidAdapter.ts,types/index.ts,types/transactionTypes.ts,utils/errorUtils.ts. UseshasPropertyfrom@metamask/utils(the idiomatic replacement documented in core's eslint config). 24 of the 28 occurrences were converted cleanly; the remaining 4 are kept as'in'behind/* eslint-disable no-restricted-syntax */blocks becausehasPropertynarrows the KEY but not the discriminated-union branch — losing access to.filled,.resting,.oid,.totalSzon the HyperLiquid SDK types and losingkeyof typeof HYPERLIQUID_ASSET_CONFIGSnarrowing. Each disable comment is documented with the narrowing rationale.scripts/perps/validate-core-sync.sh— hardened preflight + per-file suppression delta check.chore(perps-sync): hard-fail sync script when core main has drift): the previous preflight only compared the core working tree against.sync-state.json.lastSyncedCoreCommit, so it could not detect that someone else had merged a competing perps-controller change intoorigin/mainwhile this branch was in review. That gap let #8398 land in a conflicting state with #8333. The new check at the top ofstep_conflict_checkfetchesorigin/main, runsgit rev-list --count HEAD..origin/main -- packages/perps-controller/, and hard-fails (not warns) with the offending commit list + a merge suggestion if any such commits exist. Gracefully degrades toWARNif offline or iforigin/mainis missing.chore(perps-sync): hard-fail on per-file suppression delta increase): step 6 now snapshots the per-file/per-rule suppression counts fromeslint-suppressions.jsonBEFORE running--fix/--suppress-all/--prune-suppressions, then diffs against the post-run counts and hard-fails if any (file, rule) pair's count INCREASED. Reducing counts (mobile fixes removing previously-suppressed violations) is always allowed. Increases mean the current sync is introducing NEW violations that would land in core as fresh suppressions and must be fixed at source. The script prints every offending file+rule pair and points athasProperty()from@metamask/utilsas the canonical replacement. Replaces the old "WARN if count > 20" heuristic. This is the canonical local detection point for the problem that "it should have been detected locally!" — a violation now fails the sync script at step 6 BEFORE the core PR is opened.Not included (intentional)
An earlier draft of this PR also declared
MetaMetricsController:trackEventinPerpsControllerAllowedActionsto let the extension drop a type cast. That change was reverted because mobile has noMetaMetricsController— it uses aMetaMetricssingleton (app/core/Analytics/MetaMetrics.ts) — so adding the action to the allowed-actions union forced@metamask/messenger's parent type to narrow toneverand brokeapp/core/Engine/messengers/perps-controller-messenger/index.ts. The extension keeps its existingas unknown as PackagePerpsControllerMessengercast workaround until both host apps share a realMetaMetricsController.Changelog
CHANGELOG entry: null
Related issues
Fixes: TAT-2863
Related: MetaMask/core#8374 (superseded by companion core PR #8398)
Manual testing steps
Automated verification (all passing locally):
yarn linton all touched files — cleanNODE_OPTIONS='--max-old-space-size=8192' npx tsc --noEmit— clean (full, no incremental cache)yarn jest app/controllers/perps/PerpsController.test.ts app/controllers/perps/services/HyperLiquidSubscriptionService.test.ts app/controllers/perps/services/TradingService.test.ts— 412 passingbash scripts/perps/validate-core-sync.sh --core-path …/core— all 9 steps PASS. Suppression count drops from 30 → 6 after the lint cleanup.Screenshots/Recordings
N/A — this PR is a bundler / import-graph + lint cleanup with zero UI impact. Mobile runtime behavior is unchanged because:
webpackIgnoremagic comment is extension/webpack-only; Metro ignores it.MYXProvidervia relative import.perpsConnectionAttemptContextkeeps its module-level state, just at a new file path.hasProperty(obj, key)is runtime-equivalent tokey in objfor plain objects (both delegate toObject.prototype.hasOwnPropertysemantics via the prototype chain).Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Medium risk because it changes perps controller module boundaries/exports and touches trading/provider code paths (type-guard refactors and a few scoped lint suppressions), plus alters sync script behavior that can block releases if misconfigured.
Overview
Improves extension-safe consumption of
@metamask/perps-controllerby ensuring the optionalMYXProviderstays excluded from webpack’s static bundle and by removing MYX adapter re-exports from the public perps barrels.Aligns mobile’s perps linting with core by restricting the
inoperator (and a couple of other syntaxes) and refactors multiple perps files to usehasProperty()instead of'key' in obj, keeping a few documentedinusages behind targetedno-restricted-syntaxdisables where TypeScript narrowing is required.Moves/exports
perpsConnectionAttemptContextunder the perps utils surface so in-package code imports remain sync-safe and out-of-package callers use the package export, and hardensscripts/perps/validate-core-sync.shwith anorigin/mainfreshness check plus a per-file/per-rule eslint suppression delta hard-fail (including prettier formatting ofeslint-suppressions.json).Reviewed by Cursor Bugbot for commit 1f90a9b. Bugbot is set up for automated code reviews on this repo. Configure here.