Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Added

- Add `platform/notifications-category-sync`: a repo-agnostic workflow for syncing a client with the backend notifications category manifest (fallback snapshot, preference keys, settings rows), with a MetaMask Mobile overlay.
- Add a MetaMask Extension overlay for `feature-flags` (registry sync, E2E flag seeding) and a Core overlay for `controller-guidelines` (failure shapes, public type changes).
- Add `performance/profiling-regression-proposal`: an evidence-only skill that proposes follow-up actions after MetaMask Mobile CI has already classified a Hermes CPU-profile regression.
- Support explicit-only workflow skills through native invocation controls, preserve repository overlays, and prune managed retired skill names during sync.
- Distribute Perps static review as a shared execution checklist with repository overlays, source-tracked client rules and per-rule evidence outcomes, without a duplicate template catalog.
- Distribute Perps static review as a shared execution checklist with repository overlays, source-tracked client rules and per-rule evidence outcomes, without a duplicate template catalog. Reviews read consumer code from the provided reference checkouts at a cited revision and end in APPROVE or REQUEST_CHANGES; a check the reviewer could not run is listed as not verified instead of downgrading the verdict to COMMENT.
- Add `navigation` skill with a repo-agnostic base and a MetaMask Mobile overlay for `Routes` and `NavigationService`. Marked `base: true` so it installs even when its domain is filtered out.
- Add `feature-flags` skill with a repo-agnostic base and a MetaMask Mobile overlay for version-gated remote flags. Marked `base: true` so it installs even when its domain is filtered out. ([#147](https://github.com/MetaMask/skills/pull/147))
- Add `analytics` skill (`platform/analytics`, moved from `coding`) with a repo-agnostic base and a MetaMask Mobile overlay for the canonical tracking API. Marked `base: true` so it installs even when its domain is filtered out. ([#140](https://github.com/MetaMask/skills/pull/140))

### Fixed

- Keep a source's skill when a later source has the same directory without `skill.md`, and remove unprefixed aliases of managed skills during `--prune-stale`.

## [0.3.1]

### Fixed
Expand Down
9 changes: 9 additions & 0 deletions domains/coding/skills/controller-guidelines/repos/core.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
repo: core
parent: controller-guidelines
---

# Controller guidelines — core

- API response handling: test both failure shapes, a thrown error and a non-`ok` result the caller wraps.
- Classify a public type change by `AGENTS.md` ("changes the signature of any public export"), not by neighbouring code. Widening an exported union breaks exhaustive `Record<Union, …>` consumers.
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ Use `docs/perps/perps-sentry-reference.md` for Mobile trace names and lifecycle.

Mobile UI constants live in `app/components/UI/Perps/constants/perpsConfig.ts`.
Use `docs/perps/perps-metametrics-reference.md` for Mobile event definitions; controller constants remain the shared contract.
A change to a pattern documented in `docs/perps/` updates that document in the same PR.

<a id="mobile-test-layers"></a>

Expand Down
17 changes: 1 addition & 16 deletions domains/perps/skills/perps-review-pr/references/parity.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,22 +50,7 @@ Extension adds: `usePerpsLiveMarketData`, `usePerpsStreamManager`, `usePerpsView

## Formatting Divergence

See `formatting-rules` knowledge file for full rules.

| Platform | Formatter | Behavior |
|---|---|---|
| Mobile | `formatPerpsFiat` | Adaptive sig-dig by price range |
| Extension | `formatCurrencyWithMinThreshold` | Generic, no sig-dig |
| Extension | `formatNumber({min:2,max:2})` | Always 2 decimals |
| Extension | `.toFixed(2)` | Hardcoded 2 decimals |

**Files with hardcoded formatting (extension):**
- `ui/components/app/perps/utils/transactionTransforms.ts` -- `.toFixed(2)` x7
- `ui/components/app/perps/order-entry/components/auto-close-section/` -- `{min:2, max:2}`
- `ui/components/app/perps/order-entry/components/limit-price-input/` -- `{min:2, max:2}`
- `ui/components/app/perps/edit-margin/edit-margin-modal-content.tsx` -- `.toFixed(2)`
- `ui/components/app/perps/reverse-position/reverse-position-modal.tsx` -- `.toFixed(2)`
- `ui/hooks/perps/usePerpsOrderForm.ts` -- `formatCurrencyWithMinThreshold` x6
Use `formatPerpsFiat` on both platforms (Extension: `shared/lib/perps-formatters.ts`). Do not add `.toFixed(2)`, `formatNumber({min:2,max:2})` or `formatCurrencyWithMinThreshold` for displayed perps fiat values on Extension; migrate a legacy call site the PR touches.

## TestID Mapping

Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
{
"repository": "MetaMask/experimental-metamask-recipe-perps",
"revision": "e06bb8d750acbbd90ce1af62063260e85f0a0995",
"revision": "f9f8cb914c4c0fd1f7a592b9b4507b410a5d8651",
"files": {
"review/antipatterns.md": "5d16b3b25757a95c39748a2fe699dbc6bf02a77e42a3bc8d8ed66b7eeed7be36",
"review/antipatterns.mobile.md": "541cedb7c79dfe01991472147d5de8b5a38dcbc1a182057e68c2f0fb470b3ab1",
"review/antipatterns.md": "552598c77f9ac2e4dbb5d9c1157c52106ad289fa22d01bf085981fef2f19c3b7",
"review/antipatterns.mobile.md": "2c6611037e8fc135b17f1f79e7b517e36cb789ffc1b433ae9ec7c01bfcf6eb17",
"review/antipatterns.extension.md": "6fd041ff96e9d864fe5bc62b3d27f5bd022b221d25c159658d00bcd78501bf57",
"review/antipatterns.core.md": "2c7b3990b111b50bf2a6b7a4aa3a12344dc5858ce57fa086a4da1a1798c4f392",
"review/parity.md": "8895a333d526d7819c3bbf526f09f6f216aca41ddd197581e454b0bbd4bc482c",
"review/shared-packages.md": "11268974eef5e69a3ca2c8b68202bc7789133c352855e3e758bd32069ee8255d",
"review/parity.md": "02cdefde81f1d0ea4f26da1b082873bef89e188efc9b258bb01976dced863666",
"review/shared-packages.md": "b83c7d946f85d8b72af193c206c5822cb3c8d180c61c079ae1ebfb706523817a",
"owned-paths.json": "e5a3994708c61c158dd2c4adbc321269bf11435d82ff18f900d9372e831e9eb3"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@ Companion to `parity.md`. Tracks what's shared, what should be, and what can't b

## Already Shared via @metamask/perps-controller

**Utils (20)**: `significantFigures`, `orderValidation`, `orderCalculations`, `marketDataTransform`, `sortMarkets`, `marketUtils`, `accountUtils`, `errorUtils`, `hyperLiquidAdapter`, `hyperLiquidOrderBookProcessor`, `hyperLiquidValidation`, `myxAdapter`, `standaloneInfoClient`, `stringParseUtils`, `idUtils`, `rewardsUtils`, `transferData`, `wait`

**Services (14)**: `AccountService`, `TradingService`, `MarketDataService`, `DepositService`, `EligibilityService`, `HyperLiquidClientService`, `HyperLiquidSubscriptionService`, `HyperLiquidWalletService`, `MYXClientService`, `MYXWalletService`, `RewardsIntegrationService`, `TradingReadinessCache`, `DataLakeService`, `FeatureFlagConfigurationService`
**Utils** (`src/utils/`) and **Services** (`src/services/`) of `@metamask/perps-controller`: check them on core `main` before adding a client-side copy.

## Priority 1 -- Move to Controller (pure TS, no React deps)

Expand Down Expand Up @@ -35,22 +33,6 @@ Once in controller, extension imports directly instead of reimplementing.
| `isHip3Market`/`isCryptoMarket` | `UI/Perps/utils/` | `ui/components/app/perps/utils.ts` | YES |
| `groupTransactionsByDate` | `UI/Perps/utils/transactionTransforms.ts` | `ui/components/app/perps/utils/transactionTransforms.ts` | Near-identical |

## Priority 3 -- Formatting Abstraction

Extract pure formatting logic into controller:

```
Controller exports:
PRICE_RANGES_CONFIG (range thresholds + sig dig rules)
calculateDisplayDecimals(value, config) -> { decimals, sigDigs }
roundToDisplayPrecision(value, config) -> number

Platform layer:
formatPerpsFiat(value, opts) -> calls calculateDisplayDecimals + locale formatter
```

Extension currently uses `.toFixed(2)` and `formatNumber({min:2, max:2})` everywhere -- both wrong for low-value and high-precision assets.

## Can't Share (platform-bound)

| File | Reason |
Expand Down
28 changes: 19 additions & 9 deletions domains/perps/skills/perps-review-pr/references/shared.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,27 @@
- [ ] **New dependency not in DI interface** — controller code reaching outside its boundary (e.g., importing a hook, React context, or an app utility). Everything the controller needs must come through `PerpsPlatformDependencies`.
- [ ] **Breaking the publisher contract** — changing PerpsController's public API (state shape, method signatures, event names) without considering both consumers. Controller is a publisher — mobile and extension both consume it.

<a id="magic-strings-magic-numbers-placeholder-values"></a>
<a id="missing-numeric-data-stays-undefined"></a>

## Magic Strings, Magic Numbers & Placeholder Values
## Missing numeric data stays undefined

Constants live in the controller package (`core/packages/perps-controller/src/constants/perpsConfig.ts`, exported by `@metamask/perps-controller`) and in the reviewed client's UI constants module. PRs must use these — not inline literals.
Missing Perps numeric data must remain `undefined` through provider adapters, controller state, selectors, hooks and calculations. Zero means an observed or computed zero from known inputs. It must never stand for missing, loading, failed or invalid data. This applies to prices, balances, PnL, fees, margin, leverage and percentages.

- [ ] **Defaulting to `0` when data is unavailable** — the most common mistake. When price/percentage/data hasn't loaded yet, use the placeholder constants, NOT `0`, `$0`, or `0%`:
- [ ] **Missing data converted to zero**: Reject `value ?? 0`, `value || 0`, default parameters of `0`, and parsing or arithmetic that turns absent or invalid input into zero. Check availability before conversion or calculation; if a required input is missing, the result stays `undefined`. An explicit accumulator seed for known values is valid, but must not conceal an unavailable collection or missing required member.
- [ ] **Unknown treated as confirmed zero**: Preserve a genuine `0` with explicit availability checks. Do not use truthiness to decide whether a value exists. Keep actions that require the missing value disabled until it is available.
- [ ] **Reviewer proposes a fabricated value**: Review fixes follow the same rule. A crash or type error caused by missing data requires explicit missing-state handling, never a numeric fallback. Require separate cases for absent input, invalid input and genuine zero; verify missing inputs cannot enable submission or feed financial calculations as zero.
- [ ] **Placeholder written into numeric state**: Render missing values with display placeholders only at the formatting boundary:
- `PERPS_CONSTANTS.FallbackPriceDisplay` (`'$---'`) — price not yet loaded
- `PERPS_CONSTANTS.FallbackPercentageDisplay` (`'--%'`) — percentage not yet loaded
- `PERPS_CONSTANTS.FallbackDataDisplay` (`'--'`) — generic data not yet loaded
- `PERPS_CONSTANTS.ZeroAmountDisplay` (`'$0'`) / `ZeroAmountDetailedDisplay` (`'$0.00'`) — ONLY for actual confirmed zero values (e.g., no volume), never for "loading" or "unavailable"
- Defaulting to `0` hides loading states, makes bugs invisible, and can mislead users into thinking their balance/PnL is actually zero.

<a id="magic-strings-magic-numbers-placeholder-values"></a>

## Magic Strings, Magic Numbers & Placeholder Values

Constants live in the controller package (`core/packages/perps-controller/src/constants/perpsConfig.ts`, exported by `@metamask/perps-controller`) and in the reviewed client's UI constants module. PRs must use these — not inline literals.

- [ ] **Inline timeout/delay values** — hardcoded `5000`, `10000`, `300` instead of `PERPS_CONSTANTS.WebsocketTimeout`, `PERPS_CONSTANTS.ConnectionTimeoutMs`, `PERFORMANCE_CONFIG.ValidationDebounceMs`, etc. Every timing constant has a named export.
- [ ] **Hardcoded slippage** — using `0.03` or `300` instead of `ORDER_SLIPPAGE_CONFIG.DefaultMarketSlippageBps`, `DefaultTpslSlippageBps`, `DefaultLimitSlippageBps`.
- [ ] **Hardcoded leverage fallback** — using `3` or `50` instead of `PERPS_CONSTANTS.DefaultMaxLeverage` or `MARGIN_ADJUSTMENT_CONFIG.FallbackMaxLeverage`.
Expand All @@ -41,14 +50,14 @@ Constants live in the controller package (`core/packages/perps-controller/src/co

- [ ] **Provider identity lost during transformation**: Preserve provider identity through fill aggregation and apply provider-specific classification at the normalization boundary. Adding a provider must retain existing providers and cover equivalent inputs with different provider semantics.

All provider access must go through `AggregatedPerpsProvider` → `ProviderRouter`. HyperLiquid is primary, MYX is feature-flagged.
All provider access must go through `AggregatedPerpsProvider` → `ProviderRouter`. HyperLiquid is primary; Lighter is feature-flagged (`perpsLighterProviderEnabled`).

- [ ] **Hardcoded provider** — uses HyperLiquid or MYX APIs directly instead of going through `AggregatedPerpsProvider` / `ProviderRouter`. All operations must route through the abstraction.
- [ ] **Hardcoded provider** — uses a provider's API directly instead of going through `AggregatedPerpsProvider` / `ProviderRouter`. All operations must route through the abstraction.
- [ ] **Provider-specific branching in UI** — `if (provider === 'hyperliquid')` in components or hooks. Provider differences must be normalized in the aggregation layer, not leaked to the view.
- [ ] **Provider-specific error handling** — catches errors from one provider but not others. All providers must have consistent error boundaries via the aggregated layer.
- [ ] **Hardcoded market symbols** — string literals `"BTC"` or `"ETH"` instead of market config constants. Breaks when new markets or providers are added.
- [ ] **Hardcoded decimals/precision** — using provider-native decimal formats without normalization. HyperLiquid and MYX use different precision for prices, sizes, and leverage. Must go through `MarketDataFormatters` (DI).
- [ ] **`detailedOrderType` rendered directly in UI** — `detailedOrderType` is provider-native text, not an enum. HyperLiquid returns `Limit`, `Market`, `Stop Limit`, `Stop Market`, `Take Profit Limit`, `Take Profit Market`; MYX (`myxAdapter.mjs`) returns `Take Profit`, `Stop Loss`, `Liquidation` — which are not in that set. Any UI that renders `detailedOrderType` directly is provider-dependent by construction. **Grep for `detailedOrderType` in any PR touching order display** — it should be mapped through a locale string or normalized constant, not rendered raw.
- [ ] **Hardcoded decimals/precision** — using provider-native decimal formats without normalization. Providers use different precision for prices, sizes, and leverage. Must go through `MarketDataFormatters` (DI).
- [ ] **`detailedOrderType` rendered directly in UI** — `detailedOrderType` is provider-native text, not an enum. HyperLiquid returns, e.g., `Limit`, `Market`, `Stop Limit`, `Stop Market`, `Take Profit Limit`, `Take Profit Market`. Any UI that renders `detailedOrderType` directly is provider-dependent by construction. **Grep for `detailedOrderType` in any PR touching order display** — it should be mapped through a locale string or normalized constant, not rendered raw.

<a id="pro-mode-ui-gating"></a>

Expand Down Expand Up @@ -103,6 +112,7 @@ A single `PerpsAlwaysOnProvider` at the wallet root owns connect/disconnect; `Pe

## Data Flow & State

- [ ] **Delayed balance tracker loses its initiating context**: Bind the initiating account, provider and network. The first channel delivery may replay persisted preload data; it is not proof of fresh credit. Channel clearing also occurs on reconnect, so it is not an account-change signal. A timeout means credit was not observed, not that funds are available. Test provider and network changes while the account address stays fixed.
- [ ] **A changed classification leaves old priority rules**: When a validation becomes advisory, audit message ranking and CTA gating together. A finished-input warning needs commit/blur state that clears on the next edit; interaction alone is insufficient. Test mixed blockers/advice and editing an already committed value.
- [ ] **State persists outside the rendered control**: Disabling new presses does not dismiss an open keypad or active gesture. Test the transition while editing, and preserve the intended input when live limits update.
- [ ] **React persistence mistaken for WebView synchronization**: Inline and fullscreen charts can remain mounted together. Prove the handoff updates each chart's local range/state, including subsequent stream updates.
Expand Down
5 changes: 3 additions & 2 deletions domains/perps/skills/perps-review-pr/repos/core.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Required for changes to the controller package or its public contract, in additi

## Verdict and handoff

- [ ] Write artifacts/review.md with Summary, Criteria outcomes, Findings, Evidence, Limitations and Recommended Action. Include the frozen head and rule revision. Findings need severity, file:line, impact and the smallest correction. Preserve prior findings and their re-review disposition. Required NOT_CHECKED items prevent APPROVE; use COMMENT for missing evidence in standalone reports and REQUEST_CHANGES for actionable findings. If the host only accepts pass/issues, missing required evidence must block the task instead of fabricating an issue or passing it. Follow the host's required verdict/header fields. Distinguish runtime QA requests from static conclusions.
- [ ] Write artifacts/review.md with Summary, Criteria outcomes, Findings, Evidence, Limitations and Recommended Action. Include the frozen head, the rule revision and each consumer revision read. Findings need severity, file:line, impact and the smallest correction. Preserve prior findings and their re-review disposition. Follow the host's required verdict/header fields. Distinguish runtime QA requests from static conclusions.
- [ ] Decide APPROVE or REQUEST_CHANGES. APPROVE needs an empty BLOCKERS list; non-blocking nits may come with it. REQUEST_CHANGES names each blocker with a concrete ask. COMMENT is only for a draft PR or an explicitly informational request. A row the reviewer could not check (no reference checkout, no runtime) goes under Limitations as "not verified by this review" and does not block APPROVE. Evidence the author owes, such as runtime behaviour a PR claims without the proof its repository guidelines require, is a NIT or BLOCKER with a concrete ask, never a silent COMMENT. A pass/issues host maps APPROVE to pass and REQUEST_CHANGES to issues. For pass, keep issues empty; retain non-blocking nits in review.md and line-comments.json under the host contract.
- [ ] Write artifacts/line-comments.json using the host contract, or {"pr_number": <number>, "recommendation": "APPROVE|REQUEST_CHANGES|COMMENT", "summary": "...", "comments": [{"path": "...", "line": 1, "body": "...", "severity": "must_fix|suggestion|nitpick"}]} for a PR task. Only attach changed-line findings; retain other findings in review.md. Write artifacts/learnings.md. For a branch-only review, use an empty comments array without inventing a PR number when the terminal contract requires that file.
- [ ] Reconcile the changed-file/acceptance-criteria inventory with the rule outcomes before choosing a verdict. Every applicable rule needs evidence or an explicit gap; required NOT_CHECKED items prevent approval. In a hosted child checklist, return the report to the caller without completing the parent. For a standalone materialized task, satisfy inputs/worker-terminal-contract.json and its completion command. The caller owns publication, retained sessions and cleanup.
- [ ] Reconcile the changed-file/acceptance-criteria inventory with the rule outcomes before choosing a verdict. Every applicable rule needs evidence or an explicit gap: a gap the author owes is a finding, a reviewer-side gap is listed as not verified. In a hosted child checklist, return the report to the caller without completing the parent. For a standalone materialized task, satisfy inputs/worker-terminal-contract.json and its completion command. The caller owns publication, retained sessions and cleanup.
Loading
Loading