fix(perps-controller): repair CommonJS build and add close leverage + order latency#9471
fix(perps-controller): repair CommonJS build and add close leverage + order latency#9471abretonc7s wants to merge 10 commits into
Conversation
… order latency
The published 9.2.1 CJS build emits require("file:///home/runner/.../mod.ts")
because @nktkas/hyperliquid ships .d.ts with an amd-module directive carrying an
absolute CI path; ts-bridge uses it as the CJS require target, so any CJS consumer
throws Cannot find module. Fixed via a yarn patch on the SDK applied through the
root monorepo resolutions (build-time only); perps-controller's dependency range
stays ^0.33.1. Upstream: nktkas/hyperliquid#165
Also:
- TAT-3147: close events emit computed effective leverage (abs(positionValue)/marginUsed,
1 dp), incl. TP/SL closes.
- TAT-3084: add optional TrackingData.orderExecutionLatencyMs, emitted as
order_execution_latency_ms on the trade terminal event only.
Bumps perps-controller to 9.2.2.
abretonc7s
temporarily deployed
to
default-branch
GitHub Actions
Inactive
…rnal ticket refs Move the pending analytics and CommonJS-fix entries from a premature [9.2.2] section to [Unreleased] and revert the package version to 9.2.1 so the release check passes (package versions are bumped at release-cut, not in feature PRs). Remove internal ticket references from the changelog, code comments, and test names, and apply oxfmt formatting.
The check-changelog action requires user-facing Unreleased entries to link to the current pull request. Add the PR reference to the added, changed, and fixed entries.
Mirror the close path on the open-trade path in #trackOrderResult: when a successful order fills for less than the requested size, emit an additional PERPS_TRADE_TRANSACTION event with status=partially_filled plus amount_filled and remaining_amount. Full fills are unchanged. Previously partial fills only emitted status=executed, hiding the fill's partiality.
![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 2 potential issues.
❌ 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 82545e5. Configure here.
…ing failures
updateMargin only emitted the failed Perp Risk Management analytics event on
the thrown-error path, so a non-throwing { success: false } provider result
silently lost the terminal event. Emit the failed event on that branch too,
with the error message from the result, keeping exactly-once semantics per
operation (success emits executed, non-throwing failure emits failed, thrown
errors emit failed in the catch).
Add flip_long_to_short and flip_short_to_long to the TradeAction union alongside create_position and increase_exposure. The runtime already forwards these values verbatim, so consumers no longer need casts when deriving flip actions.
…mmary The closePositions batch summary event carried only status, completion_duration and bulk_action_id, silently dropping the closed count after clients removed their own close-all summary emission. Add a NUMBER_POSITIONS_CLOSED event property and emit the successful-close count on the batch summary event.
geositta
left a comment
geositta
left a comment
There was a problem hiding this comment.
I am requesting one change before approval: partial-fill classification should use the exact size submitted to the provider rather than the earlier request size. Provider normalization or retry adjustment can otherwise report a complete fill as partial and emit an incorrect remainder.
I also left two nonblocking contract questions concerning how order_execution_latency_ms differs from the existing completion_duration, and whether the supplemental partial-fill event should mirror the close-event schema.
| // Emit an additional partially filled trade event when the fill is partial, | ||
| // mirroring the close path so the fill's partiality is visible in analytics | ||
| // rather than hidden behind a status=executed event. | ||
| const requestedSize = parseFloat(params.size); |
There was a problem hiding this comment.
Could we classify partial fills using the exact size submitted by the provider? This comparison uses params.size, while HyperLiquidProvider.placeOrder can normalize that value to asset precision, recalculate it from usdAmount, or adjust it during the minimum order retry. A complete fill of the normalized size can therefore emit partially_filled with a small incorrect remaining_amount. Returning the submitted size in OrderResult, or having the provider return the partial-fill classification, would keep the analytics status aligned with the executed order.
There was a problem hiding this comment.
Fixed in 7a0e3f3. HyperLiquidProvider.placeOrder now returns the final normalized size it actually submits to the exchange as OrderResult.submittedSize (post precision rounding, USD recalculation, and the $10-minimum retry), and TradingService classifies partial fills against that value (filledSize < submittedSize) rather than the pre-normalization params.size. When the provider does not report a submitted size we no longer classify at all, so a complete fill of the normalized size is never misreported as partially filled.
|
|
||
| // Order execution latency on the terminal trade event | ||
| if (params.trackingData?.orderExecutionLatencyMs !== undefined) { | ||
| properties[PERPS_EVENT_PROPERTY.ORDER_EXECUTION_LATENCY_MS] = |
There was a problem hiding this comment.
What distinction should order_execution_latency_ms have from the existing completion_duration on this terminal event? The new field accepts a completed elapsed duration in the request data, although callers construct that request before the operation completes. If a client specific start boundary is required, accepting a start timestamp and calculating the elapsed value here would make that contract easier to use. If the controller boundary is sufficient, the existing duration already provides it.
There was a problem hiding this comment.
Agreed — removed entirely. An input tracking param cannot carry a post-completion measurement, and completion_duration already measures submission through terminal result at the controller boundary. Dropped the field, its emission, tests, and the CHANGELOG entry in 7a0e3f3.
| filledSize < requestedSize | ||
| ) { | ||
| this.#deps.metrics.trackPerpsEvent(PerpsAnalyticsEvent.TradeTransaction, { | ||
| ...properties, |
There was a problem hiding this comment.
Nit: Could we confirm the intended schema for the supplemental partial-fill event? Spreading properties gives it order_size = filledSize and order_execution_latency_ms, while the close partial-fill path keeps order_size as the requested size and reports the fill separately through amount_filled. If dashboard parity is intended, we should override order_size with the requested size and decide explicitly whether latency belongs on both outcomes.
There was a problem hiding this comment.
Aligned with the close-event schema in 7a0e3f3: order_size is now the submitted size, the fill is reported separately via amount_filled, and remaining_amount = submittedSize - filledSize. The open-trade partial event now mirrors the close path exactly.
Base the open-trade partially_filled analytics event on the provider's final submitted size rather than the caller's pre-normalization size. HyperLiquidProvider transforms the requested size before submission (precision rounding, USD recalculation, $10-minimum retry) and never echoed it back, so a complete fill of the normalized size was reported as partially_filled with a wrong remaining_amount. - Add optional OrderResult.submittedSize, returned by HyperLiquidProvider as the final normalized main-order size actually sent to the exchange. - Classify partial fills only when submittedSize is present (filledSize < submittedSize); when absent, do not classify. - Align the partial event schema with the close path: order_size is the submitted size, the fill is reported via amount_filled, and remaining_amount = submittedSize - filledSize. - Remove order_execution_latency_ms: an input tracking param cannot carry a post-completion measurement, and completion_duration already measures submission to terminal result at the controller boundary.
Classify open-trade partial fills with arbitrary-precision BigNumber comparison and subtraction instead of parseFloat. Routing the decimal size strings through parseFloat can collapse distinct values (float artifacts) into an equal comparison — misclassifying a partial fill — and leaves binary-float dust in remaining_amount (e.g. 10 - 9.7 = 0.30000000000000071). Values are converted to Number only for the emitted analytics, after the exact decimal subtraction. Also tighten the $10-minimum retry test to assert submittedSize equals the second (post-retry) submission's size, not the first rejected attempt, and add precision regression tests for exact remaining_amount and parseFloat-collapse classification.
Add explicit coverage for the guards already enforced by the open-trade partial-fill classifier: a non-finite filledSize or submittedSize emits no partially_filled event (and no NaN size on any trade event), and a failed result that still carries filled/submitted sizes never emits a partial event since classification is gated on success.
2 participants
Explanation
perps-controller@9.2.1 (#9311) has a broken CommonJS build: dist/services/HyperLiquidClientService.cjs and dist/utils/standaloneInfoClient.cjs emit require("file:///home/runner/work/hyperliquid/hyperliquid/src/mod.ts") — an absolute build-machine path — so any CJS/bundler/Jest consumer throws Cannot find module. Root cause is upstream: @nktkas/hyperliquid (0.33.0+) ships .d.ts with a /// directive (a Deno->npm dnt build artifact); ts-bridge uses that amd-module name as the CJS require() target, leaking the absolute path. The ESM build is unaffected.
This PR:
All changes are additive and recorded under
## [Unreleased]in the CHANGELOG (version stays 9.2.1 in this PR per release-cut convention; the next perps-controller release picks them up).References
Checklist
Note
Medium Risk
Touches live trading analytics classification and provider order results alongside a monorepo-wide dependency patch; changes are additive and well-tested but affect how partial fills and margin failures are reported.
Overview
Repairs the broken CommonJS build for
@metamask/perps-controllerconsumers by adding a Yarn patch on@nktkas/hyperliquid@0.33.1(wired through rootresolutions) that strips/// <amd-module name="file:///...">lines from shipped.d.tsfiles, sots-bridgeno longer emitsrequire("file:///home/runner/...")in.cjsartifacts.Perps analytics and trading telemetry are extended in
TradingServiceandHyperLiquidProvider: close events report effective leverage (abs(positionValue) / marginUsed, 1 dp) instead of configured leverage; successful opens can emit an extrapartially_filledPERPS_TRADE_TRANSACTIONwhenfilledSizeis below the exchange submitted size (newOrderResult.submittedSizefrom the provider, including post-retry normalization); batch closes addnumber_positions_closed;updateMarginnow emits a failed Risk Management event when the provider returns{ success: false }without throwing.TradeActiontypes now include flip variants.Reviewed by Cursor Bugbot for commit d71e795. Bugbot is set up for automated code reviews on this repo. Configure here.