Apply de-risking exemption to MaxPositionQuantityRule and MaxInstrumentExposureRule - #208
Conversation
…ntExposureRule (#207) Both rules had the identical latent defect PR #199 already fixed for MaxPositionLeverageRule: each compared only the resulting position's magnitude against its own configured cap, with no comparison to the current position. An account already over either cap could have a legitimate risk-reducing proposal rejected merely because the smaller resulting position was still over the limit -- permanently trapped, since every move that would bring it back into compliance was itself rejected by the very rule it would have satisfied. Fixed by applying the identical exemption MaxPositionLeverageRule already established: a proposal is exempt from the cap when the resulting position's magnitude does not exceed the current position's own magnitude, computed via the same shared resultingPosition/ findPosition helpers -- never mixing the current position's own AvgPrice with a live ReferencePrice. Added regression tests mirroring MaxPositionLeverageRule's own three (ReduceOnly de-risking, non-ReduceOnly partial de-risking, and a reversal past the current magnitude still evaluated normally) for each rule. Widened ADR-034 back to describing the exemption as applied universally across all three rules, per the plan recorded when PR #206's review narrowed it to MaxPositionLeverageRule only pending this fix. Tested: go build ./..., go vet ./..., gofmt -l ., go test ./... -race all clean. risk package coverage: 89.2%. Vertical-slice tests (service/execution, pipeline) unaffected -- their own rejection scenario is a genuine increase, not de-risking.
rustyeddy
left a comment
rustyeddy
left a comment
There was a problem hiding this comment.
Reviewed latest head dfe258c. This closes the gap identified during PR #206 cleanly. Both MaxPositionQuantityRule and MaxInstrumentExposureRule now use the existing findPosition / resultingPosition semantics and compare resulting quantity magnitude to current quantity magnitude before applying their caps, matching the already-established MaxPositionLeverageRule policy rather than inventing a second notion of de-risking. The tests cover the important boundaries: already-over-cap ReduceOnly reduction, non-ReduceOnly partial reduction, and reversal past the current magnitude still being evaluated normally. I also checked the ordering in MaxInstrumentExposureRule: the exemption occurs before requiring ReferencePrice, which is appropriate because the de-risking classification is quantity-based by ADR-034 and should not require a valuation input merely to permit exposure reduction. ADR-034 is now accurate again for all three rules. I don't see a blocker; looks ready to merge.
There was a problem hiding this comment.
🟢 Approval recommended
The core logic and regression tests align with the established de-risking exemption behavior, with only minor optional follow-ups noted.
Pull request overview
This PR extends the “de-risking exemption” behavior (previously implemented for MaxPositionLeverageRule) to MaxPositionQuantityRule and MaxInstrumentExposureRule, ensuring risk-reducing proposals are not rejected solely because the resulting position remains over a configured cap.
Changes:
- Added a de-risking exemption early-return to
MaxPositionQuantityRule.Evaluate. - Added the same de-risking exemption early-return to
MaxInstrumentExposureRule.Evaluate. - Added regression tests for both rules mirroring the existing leverage rule de-risking coverage, and updated ADR-034 to describe the now-unified behavior.
File summaries
| File | Description |
|---|---|
| risk/maxpositionquantity.go | Adds de-risking exemption to the quantity cap evaluation. |
| risk/maxpositionquantity_test.go | Adds regression tests for de-risking and reversal boundary behavior for quantity cap. |
| risk/maxinstrumentexposure.go | Adds de-risking exemption to the exposure cap evaluation. |
| risk/maxinstrumentexposure_test.go | Adds regression tests for de-risking and reversal boundary behavior for exposure cap. |
| docs/arch/adr-034-derisking-exemption-position-limit-rules.org | Updates ADR-034 to reflect the exemption being implemented across all three rules. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| pos, hasPosition := findPosition(in.Account, in.Proposal.Listing) | ||
| currentQty := num.Quantity{} | ||
| if hasPosition { | ||
| currentQty = pos.Quantity | ||
| } | ||
|
|
||
| _, qty, err := resultingPosition(in.Account, in.Proposal) | ||
| if err != nil { | ||
| return RuleResult{}, fmt.Errorf("max position quantity: %w", err) | ||
| } | ||
|
|
||
| // De-risking is exempt from the cap: see this rule's own doc | ||
| // comment and ADR-034. Comparing magnitude directly (never the | ||
| // current position's own AvgPrice) matches MaxPositionLeverageRule's | ||
| // established comparison exactly. | ||
| if qty.Cmp(currentQty) <= 0 { | ||
| return RuleResult{}, nil | ||
| } | ||
|
|
||
| if qty.Cmp(r.max) <= 0 { | ||
| return RuleResult{}, nil | ||
| } |
| this principle during implementation, but it was never captured as a | ||
| standalone ADR — only as inline doc comments on =MaxPositionLeverageRule=, | ||
| the one rule that currently applies it. | ||
| standalone ADR — only as inline doc comments on =MaxPositionLeverageRule=. |
What changed
MaxPositionQuantityRuleandMaxInstrumentExposureRulenow apply the same de-risking exemptionMaxPositionLeverageRulealready had (PR #199): a proposal is exempt from the configured cap when the resulting position's magnitude does not exceed the current position's own magnitude.Why it changed
Both rules had the identical latent defect PR #199 already fixed for
MaxPositionLeverageRule: each compared only the resulting position's magnitude against its own configured cap, with no comparison to the current position. An account already over either cap could have a legitimate risk-reducing proposal rejected merely because the smaller resulting position was still over the limit — permanently trapped, since every move that would bring it back into compliance was itself rejected by the very rule it would have satisfied.This was found during the M4 completion review (#189) while backfilling ADR-034 to document the de-risking exemption principle — the review's own draft incorrectly claimed both rules already had it. Rusty caught the inaccuracy on PR #206; ADR-034 was narrowed to describe only
MaxPositionLeverageRule's actual behavior, and this issue (#207) was filed to close the gap.How it was tested
MaxPositionLeverageRule's own three (...ReduceOnlyDeRiskingAlwaysPasses,...NonReduceOnlyPartialDeRiskingAlwaysPasses,...ReversalPastCurrentMagnitudeIsEvaluatedNormally) for each rule — all passed on the first run with hand-computed numbers.risktest suite passes unmodified — the exemption is purely additive (an early-return before the existing cap check), so every prior passing scenario is unaffected.service/execution's andpipeline's own vertical-slice tests (which useMaxPositionQuantityRulein their real risk-rejection scenario) still pass — that scenario is a genuine increase, not de-risking, so it's unaffected by this change.go build ./...,go vet ./...,gofmt -l .,go test ./... -raceall clean.riskpackage coverage: 89.2%.Which documentation changed
Widened ADR-034 (
docs/arch/adr-034-derisking-exemption-position-limit-rules.org) back to describing the exemption as applied universally across all three rules, per the plan recorded when PR #206's review narrowed it pending this fix.Closes #207.