fix(perps): harden HyperLiquid cancel readiness and map account-mode errors - #9750
Conversation
Automated fix-bug run — TAT-3633
Worker reportExplanationAfter a service-worker restart, HyperLiquid also returns raw References
Checklist
Client follow-up: Mobile and Extension should add translation entries for Harness follow-up: Land |
abretonc7s
changed the title
GitHub Actions
Inactive
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Link changelog entries to #9750 and route placeOrder error logging through Co-authored-by: Cursor <cursoragent@cursor.com> #getTradingErrorContext so getUserAddressWithDefault is awaited correctly.
aganglada
left a comment
aganglada
left a comment
There was a problem hiding this comment.
Review summary
Solid fix for the cold-start ORDER_UNKNOWN_COIN cancel bug and good follow-through on the Bugbot findings (#ensureReady() vs #ensureReadyForTrading(), unified #getTradingErrorContext). Tests and CI look strong.
Requesting changes before approval: the single-cancel success path still drops HyperLiquid's per-status error and returns a generic string, so the new EXCHANGE_MULTI_SIG_REQUIRED / EXCHANGE_INVALID_NONCE mappings never apply when HL returns a non-thrown status rejection — which is the common cancel failure shape.
What looks good
cancelOrdernow hydrates the asset map via#ensureReady()before coin validation, matchingplaceOrder/editOrderand avoiding signature prompts on invalid symbols.- Catch-path cancel failures correctly route through
#mapErrorand#getTradingErrorContext. - New error codes and HL string mappings in
#errorMappingsare well covered by tests (for thrown rejections).
Before merge (blocking)
- Parse and map non-thrown cancel status errors — see inline comment.
Non-blocking follow-ups
- Mark the new codes as BREAKING in the changelog (same union-widening pattern as
EXCHANGE_ACCOUNT_NOT_FOUND). - Consider parity for
cancelOrderscatch path (#mapError+#getTradingErrorContext). - Extension/Mobile will need translation entries before bumping
@metamask/perps-controller(called out in the PR description).
…rors Reorder cancelOrder to rebuild the asset map before coin validation so service-worker restarts no longer surface ORDER_UNKNOWN_COIN for valid markets, and map HyperLiquid multi-sig/nonce rejections to stable codes. Co-authored-by: Cursor <cursoragent@cursor.com>
Link changelog entries to #9750 and route placeOrder error logging through Co-authored-by: Cursor <cursoragent@cursor.com> #getTradingErrorContext so getUserAddressWithDefault is awaited correctly.
Run #ensureReady() before coin validation and defer #ensureReadyForTrading() until after, matching placeOrder/editOrder. Avoids signature prompts on invalid cancels while preserving cold-start self-heal for valid markets. Co-authored-by: Cursor <cursoragent@cursor.com>
Single cancelOrder only checked for a 'success' status and returned a hard-coded 'Order cancellation failed' string, so the EXCHANGE_MULTI_SIG_REQUIRED and EXCHANGE_INVALID_NONCE mappings never applied to the shape HyperLiquid actually returns most of the time: a resolved status object carrying the raw exchange error. Read that error and pipe it through #mapError, as cancelOrders already does for its per-status errors. Give the cancelOrders catch path the same treatment (#mapError plus #getTradingErrorContext), and label the new error codes BREAKING in the changelog since they widen the exported PerpsErrorCode union.
b9c8968 to
b3bce24
Compare
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 b3bce24. Configure here.
cancelOrders returned HyperLiquid's raw per-status error strings while cancelOrder maps that same non-thrown shape, so a batch cancel could surface an unmapped "multi-sig required" that single cancel standardizes. Route each per-status rejection through #mapError, keeping undefined when a status carries no error text.
Automated pr-complete run — #9750
Worker reportPR 9750 — Comments ReportPR: #9750 Integration
Fetched comments
Review-summary non-blocking follow-ups from aganglada:
Fixes applied
Downstream compatibility assessment (Mobile / Extension)Touched published surface: Risk items for consumers:
No mainnet targeting introduced; no Validation
Note on the broad build: the bounded Recipe / headless validation (step 10)Perps read smoke — PASS. The PR touches Evidence: Note: the checklist's Inherited recipe re-run — SKIPPED (blocked, not a failure). This is the harness gap the PR body already tracks as a follow-up ("Land The recipe is also a write-path proof (it places a real signed BTC limit on HyperLiquid testnet); the single-cancel behavior it would exercise is covered by the Jest cases added for this review instead. Confirmed against the runner's own manifest — the available core perps actions are Artifact-contract consequence. The terminal contract's Commit and push
Files changed in this follow-up commit:
Replies and thread resolution
Notes for the reviewer
|
aganglada
left a comment
aganglada
left a comment
There was a problem hiding this comment.
Re-review summary
Verdict: Approve — all blocking items from the prior review are addressed. Good to merge once client translation PRs are lined up for the union-widening codes.
Blocking items — resolved
-
Non-thrown cancel status mapping (
b3bce24,26e4313) —cancelOrdernow early-returns on'success'and otherwise reads the per-statuserror, pipes it through#mapError, and falls back to'Order cancellation failed'only when no error text is present. Thrown and non-thrown paths are both covered by tests. -
cancelOrdersparity — catch path now uses#mapError+#getTradingErrorContext; per-status rejections in the success response also go through#mapError, matching single cancel. Bugbot's batch-mapping finding is fixed. -
Changelog BREAKING label — new codes are documented with migration bullets mirroring
EXCHANGE_ACCOUNT_NOT_FOUND.
What still looks good
- Cold-start fix —
#ensureReady()before coin validation,#ensureReadyForTrading()deferred until after validation. MatchesplaceOrder/editOrderand avoids signature prompts on invalid symbols. Tests cover self-heal + unknown-coin rejection. #getTradingErrorContext— unified async context for place/cancel paths; abstraction mode attached for account-mode exchange errors.#handleOrderErrorfix (awaitedgetUserAddressWithDefault) is in place.- Error mapping —
multi-sig required/invalid nonce→ stable codes with good test coverage (thrown + non-thrown cancel, batch cancel, placeOrder). - Sentry hygiene — non-thrown cancel path intentionally skips
logger.errorfor benign status rejections like "already canceled"; thrown path still logs with abstraction context.
Non-blocking notes (no action required)
- Client coordination — Mobile/Extension need
EXCHANGE_MULTI_SIG_REQUIREDandEXCHANGE_INVALID_NONCEin their translate maps before bumping@metamask/perps-controller(already called out in PR description). - Harness gap —
metamask.perps.cancel_orderaction still missing from harness; Jest coverage is a reasonable substitute until that's landed. - SDK typing — the
{ error?: string }cast on cancel statuses is pragmatic given HL's types; comment explains why and matches the existingcancelOrderspattern.
Test plan
- Cancel-related unit tests pass locally (19 cases across error-handling + trading suites)
- Cold-start cancel on Extension after SW restart (original TAT-3633 repro)
- Confirm mapped codes render in Mobile/Extension translate helpers before release bump
Both the changelog entry and the isHyperLiquidMultiSigRequiredError JSDoc asserted that HyperLiquid returns the unhyphenated "multisig required" spelling. Only the hyphenated "Multi-sig required" has been observed, and the mapping table added in #9750 carries that key alone. Matching both spellings is unchanged and still correct; only the stated justification was wrong. Documentation only, no behaviour change. Refs TAT-3214
2 participants
Explanation
After a service-worker restart,
HyperLiquidProvider.cancelOrdervalidated the coin against an empty prefetch asset map before#ensureReadyForTrading()could rebuild it, causing valid cancels to fail withORDER_UNKNOWN_COIN. This change hydrates trading readiness first and routes cancel failures through#mapError.HyperLiquid also returns raw
"multi-sig required"and"invalid nonce"strings on exchange writes. These are now mapped toEXCHANGE_MULTI_SIG_REQUIREDandEXCHANGE_INVALID_NONCE, with cached abstraction mode attached to account-mode error log context for Sentry triage.References
Checklist
Client follow-up: Mobile and Extension should add translation entries for
EXCHANGE_MULTI_SIG_REQUIREDandEXCHANGE_INVALID_NONCE(widensPerpsErrorCodeunion, same pattern asEXCHANGE_ACCOUNT_NOT_FOUND).Harness follow-up: Land
metamask.perps.cancel_orderin metamask-harness (core adapter). Cold-start E2E proof documented in task artifacts (recipe-cancel-cold.json+ before/after traces).Note
Medium Risk
Breaking change widens
PerpsErrorCodeand touches trading error paths (place/cancel); behavior is well-tested but clients must add translations for the new codes.Overview
Fixes cold-start cancel failures by calling
#ensureReady()before coin validation incancelOrder, so an empty prefetch asset map is rebuilt and valid markets no longer getORDER_UNKNOWN_COINafter a service-worker restart.Adds breaking
EXCHANGE_MULTI_SIG_REQUIREDandEXCHANGE_INVALID_NONCEtoPERPS_ERROR_CODES, mapping HyperLiquid"multi-sig required"and"invalid nonce"onplaceOrder,cancelOrder, andcancelOrders.cancelOrdernow maps per-status rejections that do not throw (not only the generic'Order cancellation failed'). Account-mode failures attach cached abstraction mode to error log context via newgetCachedAbstractionModeonHyperLiquidSubscriptionService.Reviewed by Cursor Bugbot for commit b3bce24. Bugbot is set up for automated code reviews on this repo. Configure here.