feat(perps): add batch leverage updates - #295
Conversation
brunson-bot
left a comment
There was a problem hiding this comment.
Reviewed against the Polymarket/perpetuals API definitions and gateway. The core of this is verified correct:
- The signing bytes match the backend exactly.
UPDATE_LEVERAGES_DATA_HASHintrading.test.tsis byte-identical to the asserted hash inengine/platform/src/evm/op.rs::test_op_update_leverages_hashfor the same two-item batch, so the msgpack field naming/ordering intoPerpsCommandBodyOpis right. - The 1–100 / unique-ids / u32-range client-side checks mirror the documented gateway admission checks one for one (
e2e-defi specs/perps/trade/updateLeveragesBatch.feature: "empty batch, size cap, u32 ranges, duplicate ids" reject the WHOLE request with 400). - Treating a rejection without
instrument_idas a whole-request throw is exactly the wire contract —ws/pubs.rssays "Request-level failures use a one-element array so clients never need to branch on the JSON shape", andLeverageRejectedrequiresinstrument_idwhileGenericRejecteddoes not. Nice that the schema shape is what discriminates. - The two suites deleted in
05c6561are genuinely redundant:session.test.tscovers both the mixed-result normalization and the request-level-rejection throw end to end.
Five things below, none blocking.
|
Addressed — all five findings from the previous review are resolved in
Nothing new introduced. Validation steps
|
| const BatchPerpsLeverageUpdateSchema = z.object({ | ||
| instrumentId: PerpsInstrumentIdSchema.refine( | ||
| (instrumentId) => instrumentId <= MAX_UINT32, | ||
| 'Expected instrumentId to be at most 4294967295', |
There was a problem hiding this comment.
I don't think this check give much value. Instrument IDs are retrieve from the API, not guessed. At most if this is important should be self-contained in the PerpsInstrumentIdSchema.
I would drop this refine.
There was a problem hiding this comment.
Trial period: automated Brunson reviews on this repo are being evaluated. Suggestions are a work in progress and not necessarily accurate or vetted by the team yet — please verify before acting on them.
[issue] Agree it shouldn't stay here, with one correction to the premise and one knock-on.
The bound isn't invented — it mirrors a real gateway admission check. e2e-defi specs/perps/trade/updateLeveragesBatch.feature has a dedicated scenario ("Instrument id above u32::MAX" → "the gateway rejects the request because the instrument id is out of range"), and its header note lists "empty batch, size cap, u32 ranges, duplicate ids" as admission checks that reject the whole request with a 400. So dropping the refine trades a local UserInputError for a whole-batch 400. That's a cheap round trip, not data loss, so I don't think it changes the conclusion — but it's the reason the constant is there.
Your structural point is the stronger one and I'd weight it above the above: it's bolted onto one call site, so today updatePerpsLeverage and updatePerpsMargin accept an instrumentId that updateLeverages rejects for the same field. That inconsistency is worse than either uniform choice.
On moving it into PerpsInstrumentIdSchema — worth being deliberate, because that schema is used ~55 times and most of those are response and websocket-payload parsing (PerpsAccountConfig, PerpsUpdateLeverageResult, the new rejection branch, notifications, subscriptions/perps). A .max() there starts policing inbound wire data too, and the generated backend type is u64 (gateway/common/v1.rs:4213), so it would be the SDK asserting a bound the response contract doesn't state. Probably safe in practice — the gateway won't emit an id it wouldn't accept — but it's a wider change than it looks.
Knock-on if you drop it: leverage: z.number().int().positive().max(MAX_UINT32) on the next line has the same problem and less justification, since leverage is bounded by instrument.maxLeverage (tens, not billions) long before u32. Drop both and the MAX_UINT32 constant with them, rather than leaving a lone constant serving one field.
| if (instrumentIds.has(update.instrumentId)) { | ||
| context.addIssue({ | ||
| code: 'custom', | ||
| message: 'Expected each instrumentId to be unique', |
There was a problem hiding this comment.
Is this an hard rule on the BE?
I understand they are updated sequentially by the perps engine.
I am not 100% sure but if some of these are not in isolation mode, there could be a sequence that allows an operator in a single update to shifts leverage across positions without impacting their margin limits.
There was a problem hiding this comment.
Trial period: automated Brunson reviews on this repo are being evaluated. Suggestions are a work in progress and not necessarily accurate or vetted by the team yet — please verify before acting on them.
Yes, uniqueness is a hard gateway rule — and separately, the margin concern you describe is guarded in the engine. Unlike the u32 refine above, I'd keep this one.
Uniqueness. update-leverages.yaml says "up to 100 unique instruments", and e2e-defi specs/perps/trade/updateLeveragesBatch.feature has a dedicated scenario: "Same instrument id named twice in one batch → the gateway rejects the whole batch because the instrument id is duplicated / And neither slot applies, so the instrument's leverage is left untouched". Its header note groups duplicate ids with the other admission checks that reject the WHOLE request with a 400. So a duplicate never reaches the engine, and no partial application happens.
On the margin-shifting sequence. From specs/perps/trade/updateLeverage.feature, each leverage change is margin-checked on its own merits, and the check is deliberately walled off per mode:
- A decrease "requires sufficient backing at the requested leverage — cross from account-level free collateral, isolated from position equity alone — else 422
insufficient_margin"; an increase is capped by the position's risk tier. There is an explicit negative scenario rejecting an underfunded isolated decrease "despite ample cross collateral" — that is exactly the cross-subsidy you are describing, and it is tested as rejected. - "Leverage changes never move isolated margin."
specs/perps/trade/marginModeChangeGuard.feature: a Cross↔Isolated flip is rejected outright while the instrument has a resting order ("open orders exist") or an open position ("position exists"). So a batch cannot flip an instrument out of isolation to free its backing.
Since the batch is sequential and each slot fails independently on the 200, a slot that fails its own margin check just errs in place — batching does not give a slot any privilege a single updateLeverage would not have. The one known race nearby is PERP-257 (a mode flip racing an in-flight order, fixed in perpetuals#622); that is a concurrency window on the single-op path, not something batching introduces or widens.
Caveat on my confidence: this is from the API definitions and the e2e specs, i.e. specified and tested behavior, not a read of the engine's risk code. If you want certainty on the ordering interaction specifically, worth a sanity check with the perps team — but I don't see anything here that this PR needs to change.
| }, | ||
| ); | ||
|
|
||
| it.runIf(runMeteredTests)( |
There was a problem hiding this comment.
If we end up dropping this this whole test could be massively simplified by adjusting the leverage on a single position to a value and back in the same request. As far as integration test goes it would be enough to proof the SDK works with the API.
There was a problem hiding this comment.
Trial period: automated Brunson reviews on this repo are being evaluated. Suggestions are a work in progress and not necessarily accurate or vetted by the team yet — please verify before acting on them.
That specific shape won't work, but a simpler test is still available.
Naming the same instrument twice in one request is rejected at the gateway, not by this SDK refine — so dropping the refine doesn't unlock it. e2e-defi specs/perps/trade/updateLeveragesBatch.feature: "Same instrument id named twice in one batch → the gateway rejects the whole batch because the instrument id is duplicated / neither slot applies". A there-and-back batch on one instrument would just 400.
The simplification that does work is a single-instrument batch — the same spec blesses it explicitly ("Single-instrument batch is a valid batch"). Set the leverage, assert the one ok slot, restore in a second call. That deletes the .slice(0, 2), the two-candidate requirement, and makes the skip precondition very unlikely to fire.
Two things worth weighing before you pick:
- A 1-item batch cannot prove ordered results, which is the one property this API has that
updateLeveragedoesn't. If the bar is "the SDK talks to the API correctly", 1 item clears it; if it's "slot k maps to update k", you need ≥ 2. - Don't drop the
usedInstrumentIdsfilter in either version — it's load-bearing, not ceremony. The test flips to1whenever the current value isn't 1, and perspecs/perps/trade/updateLeverage.featurea decrease against a live position or resting order has to be fully backed at the requested leverage, with explicit scenarios rejecting an underfunded cross decrease to 1x and an underfunded isolated decrease with 422insufficient_margin. Without the filter that's a real flake path, not a hypothetical one.
|
The partial-success shape looks useful for a live trading client. One caller pattern may be worth adding to the TSDoc example: snapshot first, reconcile selectively, never replay the whole batch. Suggested flow:
That distinction is important in order-entry flows: replaying a full sequential, non-atomic batch after an ambiguous acknowledgement can race a position/open-order change and can also overwrite a later user choice. The PR already documents reconciliation; a compact code sample showing selective retry would prevent consumers from treating this like an atomic request. |
DEV-552: Adds ordered batch Perps leverage updates with mixed-result handling.
Note
Medium Risk
Touches signed trading commands and leverage/margin mode, with partial success semantics that callers must handle; scope is additive behind experimental APIs with strong validation and test coverage.
Overview
Adds batch Perps leverage updates so callers can change leverage and cross/isolated margin for up to 100 instruments in one signed
updateLeveragescommand, with results returned in request order.Bindings gain
PerpsUpdateLeveragesResult(per-instrumentokorerrwithinstrumentIdanderror). The client exposesupdatePerpsLeverages/PerpsSession.updateLeverages, wirestoPerpsCommandBodyOpfor the new op, and validates batches (1–100 items, unique instrument IDs, u32 bounds) before send. Docs note updates are sequential and not atomic; whole-request failures throw, while per-instrument failures (includinginternal_error) are returned as data.Unit and session tests cover signing, validation, mixed results, and rate-limit-style batch rejection; a metered integration test applies a batch and restores prior configs.
Reviewed by Cursor Bugbot for commit 05c6561. Bugbot is set up for automated code reviews on this repo. Configure here.