Repository navigation
Conversation
8 of 10 tasks
geositta
changed the title
geositta
force-pushed
the
feat/perps-ioc-price-movement-failures
branch
from
September 23, 2026 14:51
5b9c32b to
4f493fb
Compare
geositta
marked this pull request as ready for review
September 23, 2026 14:51
Contributor
Author
|
@metamaskbot publish-previews |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
geositta
force-pushed
the
feat/perps-ioc-price-movement-failures
branch
from
September 23, 2026 21:22
060640c to
68d7430
Compare
Contributor
Author
|
@metamaskbot publish-previews |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
geositta
force-pushed
the
feat/perps-ioc-price-movement-failures
branch
from
September 28, 2026 14:54
68d7430 to
bc32068
Compare
geositta
force-pushed
the
feat/perps-ioc-price-movement-failures
branch
from
October 8, 2026 02:27
bc32068 to
098d2e6
Compare
abretonc7s
requested changes
Oct 9, 2026
abretonc7s
left a comment
abretonc7s
left a comment
Contributor
There was a problem hiding this comment.
VERDICT: REQUEST_CHANGES
COMMIT: 47ff195
Static review
Recommendation: REQUEST_CHANGES. One minor analytics defect reports a thrown partial close as a full close. The intended USD-only guard and ordinary venue slippage protection are supported by static inspection. Required consumer/API evidence is missing; no runtime success is claimed.
Request and frozen evidence
Review #10385, "fix(perps): clarify IOC and price movement failures", for the Core package and its published consumers. No formal acceptance criteria were supplied. The PR description claims structured PRICE_MOVED and IOC_CANCEL failures, preserved legacy errors, a USD-only snapshot guard, venue slippage protection for exact sizes, and normalized trade/close analytics. These are claims to verify.
Base: 541d74e.
Head and inspected checkout: 47ff195.
Run: 73de2a15-7d59-497c-a0bc-e5a1c2addc53.
Workspace: 0b76af50-d2c7-4c0e-ab0e-a942b1e580a4.
Attempt: 382a03f0-6032-4964-ba2f-2f05e7f38e3c.
Frozen support digest: 4565f019308972b456cd0509b7b81e8d3df3dd8b47e0fc25d38f44a823e1a939. Review skill revision: 672608ec56d399376a4fbdf38b187aa3087e9515. Recipe library and generated policy revision: f9f8cb914c4c0fd1f7a592b9b4507b410a5d8651. Harness skill revision: 3f1567f019c18c938185cadfcc38dd20db64eee6. Harness version: 0.85.0, unused because TASK.md requires static inspection.
Read TASK.md, CHECKLIST.md, instruction-manifest.json, review-subject.json, review-support.json, handoff.json, worker-terminal-contract.json, the frozen skills, and repository AGENTS.md. The instruction manifest contains no additional instruction files. No prior review artifacts were supplied. No Mobile or Extension checkout or recorded consumer SHA is present in the frozen inputs. The Mobile PR link is descriptive data, not comparison evidence.
Source is read-only. No fetch, install, build, test execution, runtime harness, launch, source edit, cleanup, or publication. Initial source status contains an untracked .cursor/skills/ directory, which is preserved.
Scope and caller inventory
All 14 changed files are under packages/perps-controller. Production changes cover CHANGELOG.md, constants/eventNames.ts, errors.ts, index.ts, perpsErrorCodes.ts, providers/HyperLiquidProvider.ts, services/TradingService.ts, types/index.ts, utils/hyperLiquidValidation.ts, and utils/orderCalculations.ts. Tests change HyperLiquidProvider.agent-rejection.test.ts, HyperLiquidProvider.trading.test.ts, TradingService.test.ts, and orderCalculations.test.ts.
Affected callers include PerpsController.placeOrder/closePosition/closePositions, AggregatedPerpsProvider write routing, HyperLiquidProvider order construction and full/partial close paths, createErrorResult call sites, and TradingService trade/close result tracking. Optional result fields are exposed through the package root; TrackingData has optional slippage fields. Package version and dependencies are unchanged.
Applicable shared families are portability, missing numeric data, constants, protocol abstraction, telemetry, data flow, trade execution, and test coverage. Connection checks apply to the affected async reads and post-trade invalidation; connection ownership and subscriptions do not change. Sentry checks apply to trace cleanup in the affected operations; background trace volume does not change. Pro UI gates, locale files and embedded signer boundaries are outside the diff. Consumer localization and publisher compatibility remain comparison gaps.
Setup criteria ledger
| Family / rule | Outcome | Evidence |
|---|---|---|
| Setup / request and exact revisions | PASS | Frozen review-subject.json; git rev-parse HEAD matches the supplied head. |
| Setup / inventory and affected callers | PASS | Exact base...head diff has 14 Perps package files; caller inventory above. |
| Setup / individual criteria and evidence | PASS | This report retains the criteria ledger; pending checks are not described as passed. |
Shared behavior and test evidence
Paths below are relative to packages/perps-controller/src unless tests are named. PASS means source inspection supports the criterion; it does not mean runtime execution passed.
| Claim / path | Outcome | Evidence |
|---|---|---|
| USD-derived snapshot rejection | PASS | utils/orderCalculations.ts:623 checks USD sizing and snapshot price; :630 retains strict greater-than tolerance rejection. errors.ts:75 stores raw numbers. orderCalculations.test.ts:514 asserts code and 1000-bps delta. |
| Exact-size bypass with ordinary venue protection | PASS | HyperLiquidProvider.ts:6531 normalizes caller tolerance; :6542 and :6556 forward it to sizing and venue price. Full closes remove USD sizing at :11978 but keep tolerance at :11980. trading.test.ts:2427 checks submitted sell limit 38800 for a 40000 live price and 300-bps cap. Native TWAP had no price cap before this PR; it is not evidence for ordinary IOC protection. |
| Metadata crosses error boundaries | PASS | HyperLiquidProvider.ts:4852,4861 produces typed mapped errors; utils/hyperLiquidValidation.ts:56 copies code/details. AggregatedPerpsProvider.ts:649,855 spreads results. PerpsController.ts:3235,3503 returns service results. TradingService.ts:816,2232 preserves the result. |
| Legacy error compatibility | PASS | IOC/account mapping still uses the existing code as message. errors.ts:77 retains the price-error prefix and changes prices to provider formatting. For non-Error inputs, createErrorResult now uses ensureError instead of UNKNOWN_ERROR; no consumer acceptance evidence is available. |
| Analytics numbers and raw failures | PASS | TradingService.ts:186,194 converts bps to percent and omits absent fields; :206 stores stable code, :211 permits finite price delta. Raw errors remain at :397,1102,1159. Returned trade test :934 asserts 300 bps -> 3%, 125 bps -> 1.25%, raw message and 336-bps delta. |
| Failed close intent | FINDING | F1. New field at TradingService.ts:1014 exposes unconditional FULL in exception fallback at :1181. |
| State and cleanup | PASS | Success invalidation remains at TradingService.ts:797,2205; fee context cleanup at :590; timers/traces end at :853,2271. No failure clears pending trade configuration; PerpsController.ts:3231 preserves success-only behavior. |
| Tests inspected | PASS | Four changed test files, plus adjacent sizing, invalid-size, clamping, close-failure, routing and partial-fill tests. Changed trading test uses real createErrorResult via requireActual at :71. Existing account rejection tests assert unchanged message plus new code at agent-rejection.test.ts:1040,1190,1390. |
| Test/build execution | NOT_CHECKED | Prohibited by TASK.md. No CI logs, compilation report or executed evidence supplied. |
| Coverage boundaries | NOT_CHECKED | No new close analytics assertions, thrown partial-close case, zero/absent tracking contract, isPerpsErrorCode assertions, exact-cap assertion or end-to-end USD PRICE_MOVED assertion in the changed tests. QA handoff below names these checks. |
Finding
F1, minor, packages/perps-controller/src/services/TradingService.ts:1014.
Derive close_type for thrown partial-close failures. If provider.closePosition throws after a 1-unit position was loaded for params.size='0.25', the catch passes result:null to #trackPositionCloseResult. Its fallback hardcodes closeType to FULL at line 1181, so this new property emits close_type:'full' alongside percentage_closed:25. Returned failures correctly classify the same request as 'partial'. Compute the fallback classification from the requested size and add a thrown-partial-close regression test.
This affects analytics classification, not the size or venue order submitted. The base emitted close_type only for executed closes; the new base-properties field makes the existing exception fallback observable. Reachable throwing boundary: AggregatedPerpsProvider.closePosition at :850 looks up the route without a catch; :264 throws PROVIDER_NOT_FOUND for an unavailable selected provider. With a position already loaded and a route removed during an intervening await, TradingService.ts:2242 passes null and :1181 classifies a partial request as full. HyperLiquid itself converts routine close errors to results; routine rewards failures are handled internally. No runtime reproduction was executed.
Base review criteria
| Family / rule | Outcome | Evidence |
|---|---|---|
| Base / normal, error, empty, boundary and cleanup paths | FINDING | Behavior table above; F1 distinguishes returned failures from exceptions. Zero/non-numeric sizes are rejected in trading.test.ts:2394; empty size is full at :2412; partial USD clamping at :2468. |
| Base / meaningful tests and regressions | FINDING | Sizing and returned metadata assertions cover the changed branch; F1 lacks an exception regression. Static inspection only. |
| Base / permissions, secrets, dependencies and wiring | PASS | New fields contain prices, deltas and error codes, not credentials. No dependency, permissions or flag changes. Venue account mapping still strips the address at HyperLiquidProvider.ts:4852. Localization is left to consumers. |
| Base / signal over noise | PASS | Changed code has no ticket/tool markers, new TODO, console log, commented-out block or unused helper. The price-error factory constructs a typed contract; the private analytics helper has several call sites. Existing close comment at provider :11923 incorrectly describes the removed guard and should be corrected with follow-up documentation. |
Shared individual rule ledger
Frozen references/shared.md supplies these labels. Every rule is listed, including scope exclusions.
| Family / rule | Outcome | Evidence or exclusion |
|---|---|---|
| Controller Portability (Core) / Platform import in controller | PASS | Changed imports use portable TypeScript and local modules; errors.ts:1; types/index.ts:10; TradingService.ts:179. |
| Controller Portability (Core) / Deep import from app code | NOT_CHECKED | No frozen Mobile or Extension source or revision supplied. |
Controller Portability (Core) / __DEV__ or platform globals in controller code |
PASS | Changed src diff adds no platform globals; errors.ts:29 and TradingService.ts:179 use standard Error/Number. |
| Controller Portability (Core) / New dependency not in DI interface | PASS | Changed imports use portable TypeScript and local modules; errors.ts:1; types/index.ts:10; TradingService.ts:179. |
| Controller Portability (Core) / Breaking the publisher contract | NOT_CHECKED | Optional public fields/types are additive at types/index.ts:208,481 and index.ts:736; both consumers are unavailable. |
| Missing numeric data stays undefined / Missing data converted to zero | PASS | TradingService.ts:186,194 omits absent bps; :199 leaves absent codes undefined; :211 filters non-finite price deltas. Existing sizing fallbacks are unchanged. |
| Missing numeric data stays undefined / Unknown treated as confirmed zero | PASS | Explicit undefined checks preserve real zero for new bps metrics at TradingService.ts:186,194. |
| Missing numeric data stays undefined / Reviewer proposes a fabricated value | NOT_APPLICABLE | The proposed fix derives close type from known request/position data; no numeric fallback proposed. |
| Missing numeric data stays undefined / Placeholder written into numeric state | NOT_APPLICABLE | No placeholder or numeric-state write is added; optional telemetry fields remain optional. |
| Magic Strings, Magic Numbers & Placeholder Values / Inline timeout/delay values | NOT_APPLICABLE | No new timeout, leverage, endpoint, provider, validation threshold or cache configuration. |
| Magic Strings, Magic Numbers & Placeholder Values / Hardcoded slippage | PASS | orderCalculations.ts:627 uses ORDER_SLIPPAGE_CONFIG.DefaultMarketSlippageBps; provider :6531 preserves caller tolerance. |
| Magic Strings, Magic Numbers & Placeholder Values / Hardcoded leverage fallback | NOT_APPLICABLE | No new timeout, leverage, endpoint, provider, validation threshold or cache configuration. |
| Magic Strings, Magic Numbers & Placeholder Values / Hardcoded precision | PASS | orderCalculations.ts:634 delegates price formatting to formatHyperLiquidPrice; errors.ts:77 rounds bps for the legacy string only. |
| Magic Strings, Magic Numbers & Placeholder Values / Hardcoded API URLs | NOT_APPLICABLE | No new timeout, leverage, endpoint, provider, validation threshold or cache configuration. |
| Magic Strings, Magic Numbers & Placeholder Values / Hardcoded provider name | NOT_APPLICABLE | No new timeout, leverage, endpoint, provider, validation threshold or cache configuration. |
| Magic Strings, Magic Numbers & Placeholder Values / Hardcoded validation thresholds | NOT_APPLICABLE | No new timeout, leverage, endpoint, provider, validation threshold or cache configuration. |
| Magic Strings, Magic Numbers & Placeholder Values / Hardcoded cache durations | NOT_APPLICABLE | No new timeout, leverage, endpoint, provider, validation threshold or cache configuration. |
| Protocol Abstraction / Execution identity inferred from display fields | NOT_APPLICABLE | No identity, fill aggregation, market symbol or UI display transformation changes. |
| Protocol Abstraction / Provider identity lost during transformation | NOT_APPLICABLE | No identity, fill aggregation, market symbol or UI display transformation changes. |
| Protocol Abstraction / Hardcoded provider | PASS | Changed TradingService analytics is provider-neutral; HyperLiquid mapping remains at its boundary, provider :4859. |
| Protocol Abstraction / Provider-specific branching in UI | NOT_APPLICABLE | No identity, fill aggregation, market symbol or UI display transformation changes. |
| Protocol Abstraction / Provider-specific error handling | PASS | HyperLiquidProvider.ts:4852,4861 normalizes venue codes; createErrorResult :56 forwards metadata; AggregatedPerpsProvider.ts:648,854 preserves optional fields. Lighter remains in stated POC scope with optional fields. |
| Protocol Abstraction / Hardcoded market symbols | NOT_APPLICABLE | No identity, fill aggregation, market symbol or UI display transformation changes. |
| Protocol Abstraction / Hardcoded decimals/precision | PASS | Formatting is contained in the HyperLiquid sizing utility at orderCalculations.ts:634; client details remain numeric at errors.ts:10. |
Protocol Abstraction / detailedOrderType rendered directly in UI |
NOT_APPLICABLE | No identity, fill aggregation, market symbol or UI display transformation changes. |
| Pro Mode UI Gating / Single-gate assumption | NOT_APPLICABLE | No UI, gates, tabs or runtime fixture change in the 14-file diff. |
| Pro Mode UI Gating / Fixture starts in Lite mode | NOT_APPLICABLE | No UI, gates, tabs or runtime fixture change in the 14-file diff. |
| Pro Mode UI Gating / Pro-only code tested only via live UI | NOT_APPLICABLE | No UI, gates, tabs or runtime fixture change in the 14-file diff. |
| Pro Mode UI Gating / Hardcoded tab index across feature gates | NOT_APPLICABLE | No UI, gates, tabs or runtime fixture change in the 14-file diff. |
| MetaMetrics Events / Magic string event properties | PASS | TradingService.ts:186,206,1014 uses PERPS_EVENT_PROPERTY; eventNames.ts:186 names price_delta_bps. |
| MetaMetrics Events / Magic string event values | PASS | TradingService.ts:1014 takes calculated values from PERPS_EVENT_VALUE; no new literal event value added. |
| MetaMetrics Events / New event instead of property | PASS | Existing TradeTransaction/PositionCloseTransaction events reused at TradingService.ts:719,1152,1232. |
MetaMetrics Events / Missing source on screen view |
NOT_APPLICABLE | No screen or reusable component added; trade/close events only. |
| MetaMetrics Events / Hardcoded source in reusable component | NOT_APPLICABLE | No screen or reusable component added; trade/close events only. |
| MetaMetrics Events / New screen/view without tracking | NOT_APPLICABLE | No screen or reusable component added; trade/close events only. |
MetaMetrics Events / Missing completion_duration on transaction events |
PASS | Terminal trade/close duration retained at TradingService.ts:301,1158,1235; submitted events retain existing semantics. |
| Sentry Tracing / Unbounded background trace volume | NOT_APPLICABLE | No new background spans, polling or reconnect instrumentation. |
| Connection & WebSocket Architecture / Cleanup has no owner for in-flight setup | NOT_APPLICABLE | No lifecycle owner, subscription, throttle or chart changes. |
| Connection & WebSocket Architecture / A second lifecycle owner | NOT_APPLICABLE | No lifecycle owner, subscription, throttle or chart changes. |
| Connection & WebSocket Architecture / Unthrottled WS → setState | NOT_APPLICABLE | No lifecycle owner, subscription, throttle or chart changes. |
| Connection & WebSocket Architecture / Per-component WS subscription | NOT_APPLICABLE | No lifecycle owner, subscription, throttle or chart changes. |
| Connection & WebSocket Architecture / WS subscription leak | NOT_APPLICABLE | No lifecycle owner, subscription, throttle or chart changes. |
| Connection & WebSocket Architecture / Stale data after async gap | PASS | Full close live-size selection and read refresh are unchanged; changed guard consumes the submission price at HyperLiquidProvider.ts:6537. |
| Connection & WebSocket Architecture / Static WebView work coupled to live ticks | NOT_APPLICABLE | No lifecycle owner, subscription, throttle or chart changes. |
| Connection & WebSocket Architecture / Missing cache invalidation | PASS | TradingService.ts:797,2205 invalidates positions/accountState after success. |
| Data Flow & State / Delayed balance tracker loses its initiating context | NOT_APPLICABLE | No client state, balance watcher, preload, mutation lock or UI interaction change. |
| Data Flow & State / A changed classification leaves old priority rules | NOT_CHECKED | Local snapshot rejection is removed for exact sizes; Core behavior traced, but consumer CTA/message ranking unavailable. |
| Data Flow & State / State persists outside the rendered control | NOT_APPLICABLE | No client state, balance watcher, preload, mutation lock or UI interaction change. |
| Data Flow & State / React persistence mistaken for WebView synchronization | NOT_APPLICABLE | No client state, balance watcher, preload, mutation lock or UI interaction change. |
| Data Flow & State / Old context remains actionable | NOT_APPLICABLE | No client state, balance watcher, preload, mutation lock or UI interaction change. |
| Data Flow & State / Unknown balance treated as usable balance | NOT_APPLICABLE | No client state, balance watcher, preload, mutation lock or UI interaction change. |
| Data Flow & State / Late defaults overwrite a user choice | NOT_APPLICABLE | No client state, balance watcher, preload, mutation lock or UI interaction change. |
| Data Flow & State / Direct controller call from component | NOT_APPLICABLE | No client state, balance watcher, preload, mutation lock or UI interaction change. |
Data Flow & State / Missing accountState check |
NOT_APPLICABLE | No client state, balance watcher, preload, mutation lock or UI interaction change. |
| Data Flow & State / Derived flag promoted to structural state without its own lifecycle | NOT_APPLICABLE | No client state, balance watcher, preload, mutation lock or UI interaction change. |
| Data Flow & State / Unknown async value treated as an absent blocker | NOT_APPLICABLE | No client state, balance watcher, preload, mutation lock or UI interaction change. |
| Data Flow & State / Async flow loses ownership of cleanup | NOT_APPLICABLE | No client state, balance watcher, preload, mutation lock or UI interaction change. |
| Data Flow & State / One in-flight mutation lock replaces earlier accepted outcomes | NOT_APPLICABLE | No client state, balance watcher, preload, mutation lock or UI interaction change. |
| Data Flow & State / Stale position after close | PASS | Success cache invalidation remains at TradingService.ts:2205; no new state persistence added. |
| Data Flow & State / Preload data not seeded | NOT_APPLICABLE | No client state, balance watcher, preload, mutation lock or UI interaction change. |
| Data Flow & State / Order state race | NOT_APPLICABLE | No client state, balance watcher, preload, mutation lock or UI interaction change. |
| Data Flow & State / Leverage/validation bypass | PASS | HyperLiquidProvider.ts:6537 retains sizing validation; :6548 retains venue price bounds; the removed local check applies only without USD sizing. |
| Trade Flow & Order Execution / Signed bounds collapsed into magnitudes | NOT_APPLICABLE | No signed RoE or TP/SL conversion change. |
| Trade Flow & Order Execution / Pre-trade checks missing | PASS | Existing validation/order construction is retained; provider :6531 passes user/default tolerance through sizing and limit calculation. |
| Trade Flow & Order Execution / Post-trade state not refreshed | PASS | TradingService.ts:797,2205 retains success invalidation; PerpsController.ts:3231 clears pending config only on success. |
| Trade Flow & Order Execution / Missing slippage in order params | PASS | HyperLiquidProvider.ts:6542,6556,11980 forwards normalized/caller maxSlippageBps. |
| Locale Coverage & Orphaned Keys / Duplicate JSON keys shadow new copy | NOT_APPLICABLE | No locale files or strings() calls changed; client localization requires unavailable consumer source. |
Locale Coverage & Orphaned Keys / Hardcoded string replacing a strings(...) call |
NOT_APPLICABLE | No locale files or strings() calls changed; client localization requires unavailable consumer source. |
| Locale Coverage & Orphaned Keys / Orphaned locale keys | NOT_APPLICABLE | No locale files or strings() calls changed; client localization requires unavailable consumer source. |
| Locale Coverage & Orphaned Keys / Test revert-sensitivity for locale keys | NOT_APPLICABLE | No locale files or strings() calls changed; client localization requires unavailable consumer source. |
| Test Layer Coverage / Degenerate fixtures hide formula errors | PASS | orderCalculations.test.ts:514 uses 1000-bps drift against 300; :559 uses sub-cent prices; trading.test.ts:2427 asserts the actual 38800 venue limit. |
| Test Layer Coverage / Clock control changes the test mechanism | NOT_APPLICABLE | No clock mechanism, numeric gesture or rendered UI change. |
| Test Layer Coverage / Numeric control units change partially | NOT_APPLICABLE | No clock mechanism, numeric gesture or rendered UI change. |
| Test Layer Coverage / Assertions miss the behavior under review | FINDING | F1. TradingService.test.ts:934 covers returned trade metadata; :2328 asserts failed close status/error but misses thrown partial-close type at source :1181. |
| Embedded Signer Boundaries / Navigation policy mistaken for network isolation | NOT_APPLICABLE | No key material, bridge, WebView transport or isolation changes. |
| Embedded Signer Boundaries / Bridge messages trusted by assertion | NOT_APPLICABLE | No key material, bridge, WebView transport or isolation changes. |
Shared prose constraints and comparison checks
| Family / constraint | Outcome | Evidence |
|---|---|---|
| Portability / package publisher and DI | PASS | Package remains portable TypeScript; package.json exposes root types and optional result fields. No platform service added. |
| Missing numbers / absent, invalid and genuine zero | PASS | Optional telemetry is omitted when unavailable and preserves zero; finite delta guard at TradingService.ts:211. No new numeric state placeholder. Validity tests for new tracking fields remain a coverage gap. |
| Constants / controller-owned configuration | PASS | Stable error/property constants are in the package. Default tolerance and venue formatting use existing configuration. |
| Protocol / aggregated routing and normalized failures | PASS | PerpsController.ts:3185,3497 resolves routed provider; AggregatedPerpsProvider.ts:850 preserves results. New fields are optional so POC Lighter can omit them. |
| MetaMetrics / consolidated event correctness | FINDING | Existing events and typed properties retained; F1 corrupts the new failed close classification. |
| Sentry / named async traces and end calls | PASS | Existing PlaceOrder/ClosePosition traces at TradingService.ts:675,2129 end in finally at :858,2273. No added spans. |
| Connection / single owner and subscription policy | NOT_APPLICABLE | Diff changes no connection ownership or subscriptions; success invalidation is covered above. |
| Data flow / controller to client ownership | NOT_CHECKED | Core returns optional metadata without persisting a new flag; no frozen Redux, hook or component source. |
| Trade flow / validation, caller tolerance and refresh | PASS | Provider :6537,6548; service :797,2205. Guard removed only for exact sizes; USD sizing retains snapshot validation. |
| Test layer / real behavior and preserved coverage | FINDING | Real helper metadata and actual SDK order asserted; thrown close classification is untested and wrong, F1. |
| Client counterpart comparison | NOT_CHECKED | Frozen parity.md read. Order-entry, close modal and error translator counterparts are unavailable for both consumers. |
| Shared package imports, versions and migrations | NOT_CHECKED | Frozen shared-packages.md and owned-paths.json read. Root exports and unchanged version 20.0.0 inspected; no recorded Mobile or Extension revision, package usage or migration evidence. No client compile claim. |
Run QA handoff
- Run focused Perps sizing/provider/service tests, type checks, lint and changelog validation at the frozen head after authorization for Run QA.
- Cover a thrown partial close with a loaded position and a full close; assert close_type, percentage_closed, error_message and completion_duration. Include an unavailable aggregated route after the position read.
- Verify zero and omitted slippage tracking, typed thrown metadata, returned/no-position close failures and partial-fill analytics.
- Verify USD PRICE_MOVED survives the full controller/provider result path without exchange submission; exercise exact-cap equality, ordinary buy/sell venue limits and sub-cent messages.
- At recorded Mobile and Extension revisions, inspect package imports/version adoption, passive localized PRICE_MOVED/IOC_CANCEL explanations, fallback handling for missing structured fields, slippage tracking and close UI behavior.
Unchecked areas
Runtime tests, lint, compilation, changelog validation, live venue behavior, both client integrations and localized UI behavior are NOT_CHECKED. The frozen inputs contain no runtime evidence. No unchecked evidence is described as passed.
Core contract impact matrix
| Contract | Impact | Evidence |
|---|---|---|
| Controller state | None | No PerpsController state or metadata diff. |
| Method signatures | Controller signatures unchanged; utility return extended | createErrorResult now returns TValue & PerpsErrorResultFields; utils/hyperLiquidValidation.ts:52. |
| Results and input types | Optional metadata and slippage tracking | types/index.ts:209,482; legacy results remain structurally valid by inspection. |
| Events | Same names; added properties and changed close_type availability | constants/eventNames.ts:186; TradingService.ts:1014; F1 affects exceptions. |
| Exports | Added predicate and detail/slippage types | index.ts:463,736,738; constants/index.ts:5 re-exports eventNames. |
| Constants | New PRICE_MOVED and PRICE_DELTA_BPS | perpsErrorCodes.ts:133; constants/eventNames.ts:186. |
| Default behavior | Exact-size snapshot rejection removed | orderCalculations.ts:623; ordinary venue tolerance remains. |
| Version and release | Version remains 20.0.0; Unreleased records additions/fix | package.json:3; CHANGELOG.md:12,16,24. No release or consumer bump supplied. |
Core individual criteria ledger
| Family / rule | Outcome | Evidence or exclusion |
|---|---|---|
| Public contracts / state shape changes without client migration | NOT_APPLICABLE | No controller state shape change. |
| Public contracts / method signature changes without compatibility plan | PASS | Controller methods unchanged; utility adds optional fields while retaining legacy error strings for Error inputs. types/index.ts:481. |
| Public contracts / event name/payload drift | FINDING | Existing names retained. Payload additions are documented, but F1 misclassifies thrown partial closes. |
| Public contracts / package export changes without package-level tests | NOT_CHECKED | Existing tests/public-api.test.ts:18 and capabilities-public-api.test.ts:3 use the root, but neither covers the new predicate/types. New tests import internals. No consumer-style assertions or consumer compilation evidence supplied for these exports. |
| Release metadata / public API/state change without changelog | PASS | CHANGELOG.md:12,16,24 describes errors, exports, telemetry and exact-size guard correction. |
| Release metadata / breaking change released as minor/patch | NOT_APPLICABLE | This is an unreleased source PR; no new version is released. Optional fields are additive; behavior change is documented. Client reliance on the old rejection guard is unverified. |
| Release metadata / controller package and client integration out of sync | NOT_CHECKED | Mobile paired PR link is supplied, but no consumer revision/package bump or Extension compatibility note is frozen. |
| Multi-sig / catch classifier without proactive probe | NOT_APPLICABLE | No write, probe or catch flow added. Typed #mapError retains EXCHANGE_MULTI_SIG_REQUIRED message at provider :1695,4861. Existing setup probe at :3087 remains unchanged. |
| Multi-sig / proactive probe without catch classifier | PASS | Affected mapping preserves the existing message; isHyperLiquidMultiSigRequiredError uses Error.message. Setup catch at provider :3195 is unchanged. This does not certify every venue write. |
| Multi-sig / probe placed too early | NOT_APPLICABLE | No probe moved; existing setup probe follows compatible-mode, deferred and unknown-mode returns at provider :3030,3061,3073. |
| Unified setup / permanent account condition cached as retryable | NOT_APPLICABLE | No readiness-cache or retry flag change; new error subclass preserves existing message matching. |
| Unified setup / retryable flag plus permanent condition | NOT_APPLICABLE | No setup semantics change. Permanent multi-sig branch at provider :3208 and transient branch at :3236 remain as before. |
| Unified setup / deferred or skipped path treated as a failure | NOT_APPLICABLE | Deferred and unknown-mode returns at provider :3061,3073 are outside the diff. |
| Metered benefits / strict economic improvement, equality and quantization | NOT_APPLICABLE | No fee resolver, allowance consumption or economic predicate changed. |
| Read-only accounts / inventory all writes and guard shared boundary | NOT_APPLICABLE | No read-only account capability or write boundary changed or claimed by this PR. |
| Expected evidence / contract impact matrix | PASS | Matrix above reconstructs every changed public family from frozen source. PR body describes the changed surfaces without a formal matrix. |
| Expected evidence / Mobile and Extension compatibility | NOT_CHECKED | Mobile PR link exists, source/revision absent; Extension note/revision absent. |
| Expected evidence / provider abstraction and fallback tests | NOT_CHECKED | Core spreads optional results and inspects real HyperLiquid helper; no new aggregated-provider legacy-result contract test. Lighter is expressly excluded as POC. |
| Expected evidence / grep for client imports and environment globals | PASS | Exact added src diff searched for react-native, Engine, Sentry, DevLogger, DEV, window/document/chrome/browser access; no matches. New imports are local portable modules. |
Core prose constraints and terminal reconciliation
Public-contract versioning and migration readiness are NOT_CHECKED because package-root assertions and both consumer revisions are unavailable. Release notes are present; no version bump or release is claimed. Multi-sig setup semantics are unchanged and no complete account write-guard certification is claimed.
All 14 changed files are accounted for in the inventory and behavior/contract tables. There are no formal ACs; all six description claims were inspected. The normalized failure and sizing claims pass static checks; failed-close type has F1. Both shared child checks and the Core overlay record every criterion, including scope exclusions and missing evidence.
Required NOT_CHECKED evidence prevents approval. REQUEST_CHANGES follows the actionable F1; the missing comparisons are evidence gaps, not invented code defects. Correct the exception close-type calculation and add its regression test, then supply consumer/API evidence and run the separately authorized QA checks. No previous finding or re-review disposition was supplied. Verdict publication, release and source changes are outside this task.
Terminal artifacts are review.md, line-comments.json, review-result.json and learnings.md. The machine result and line comment use the same finding text, severity, path and line. The result binds the run, workspace, frozen SHAs, SIGNAL.json attempt and exact report digest.
geositta
enabled auto-merge
October 9, 2026 21:15
Add structured PRICE_MOVED errors and full-close handling in the perps controller, then surface slippage-aware recovery copy, controls, and analytics in Mobile.
…size A thrown close has no order result, so the analytics fallback reported close_type full even when percentage_closed reflected a partial size. Co-authored-by: Cursor <cursoragent@cursor.com>
geositta
force-pushed
the
feat/perps-ioc-price-movement-failures
branch
from
October 9, 2026 21:16
6faceac to
a8644e3
Compare
![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 a8644e3. Configure here.
Co-authored-by: Cursor <cursoragent@cursor.com>
abretonc7s
previously approved these changes
Oct 10, 2026
abretonc7s
left a comment
abretonc7s
left a comment
Contributor
There was a problem hiding this comment.
Re-reviewed a8644e3..d5a350f, plus the full range since my last review. My earlier finding is addressed: returned and thrown close failures are classified separately (TradingService.ts:1173, :2212/:2231), and partial closes are no longer reported as full. The Bugbot min-order retry finding is fixed as well: HyperLiquidProvider.ts:6696-6709 clears priceAtCalculation when the original usdAmount is absent, and trading.test.ts:1158 covers it. No new issues. Approving.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Oct 10, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Oct 10, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Oct 10, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Oct 10, 2026
geositta
enabled auto-merge
October 10, 2026 03:22
This branch was successfully deployed
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
2 participants
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.
Explanation
Hyperliquid market orders can fail in two different ways that were previously flattened into weak or misleading client feedback:
IOC_CANCEL).PRICE_MOVED).This PR adds a stable
PRICE_MOVEDcode and structured details while preservingOrderResult.error. Exact size orders bypass only the USD sizing snapshot guard; the venue side slippage capped limit remains protection. USD derived orders retain the guard. Hyperliquid now preserves structured failure metadata, and TradingService adds normalized failure/slippage analytics without removing rawerror_messagevalues.Lighter is intentionally unchanged because its integration remains a POC and is outside TAT-3085's production scope.
The paired Mobile consumer change is MetaMask/metamask-mobile#36680. Mobile uses this structured contract only to present passive localized explanations; it adds no recovery actions, controls, navigation, or close-tolerance behavior changes.
References
Checklist
Note
Medium Risk
Changes when orders are rejected locally versus submitted (exact-size/full closes with stale snapshots), and expands the public failure contract clients rely on for messaging.
Overview
Introduces
PRICE_MOVEDand optionalerrorCode/errorDetailsonOrderResult(viaPerpsControllerErrorandcreatePriceMovedError), so clients can localize failures without parsingOrderResult.error. HyperLiquid mapped rejections—includingIOC_CANCELandEXCHANGE_ACCOUNT_NOT_FOUND—now populateerrorCodewhile keeping the same message string;createErrorResultpropagates structured fields from controller errors.Behavior change: the calculation-time price guard runs only for USD-derived sizing. Exact-size orders (including full closes and minimum-value retries that synthesize USD) no longer fail locally on a stale
priceAtCalculation; venue slippage-capped limits still apply. Minimum-order retries clear the unrelated snapshot on exact-size paths.Analytics: trade and close events add
failure_reason, slippage fields from callertrackingData,price_delta_bpsforPRICE_MOVED, andclose_typeon failed closes (including thrown errors). Rawerror_messageis unchanged.Reviewed by Cursor Bugbot for commit 9f1b078. Bugbot is set up for automated code reviews on this repo. Configure here.