feat: shared gross-notional / initial-margin calculation

[rustyeddy]

What changed

Adds internal/account/margin, a pure calculation package with no I/O and no state (ADR-066). Both the account-level risk rule (#413) and the simulator's fill-time check (#415) will import it, so there's exactly one definition of gross exposure and required margin. internal/account already sits below both risk and sim, and sim still doesn't import risk.

API	Purpose
Notional(Valued, currency)	|qty| × price × multiplier, exact num.Money
Policy interface / Ratio	Per-position required margin. ADR-066's extension point, so futures' fixed amount per contract can plug in later without a pseudo-rate. Ratio (|notional| × initial_margin_ratio) is the only v1 policy.
Account(positions, Marks, policy, currency)	Current gross and required margin; every open position valued at its mark, never AvgPrice
Assess(positions, Marks, Change, policy, currency)	Current vs prospective. The changed instrument is valued at Change.Price in both states (so the comparison is pure quantity) and needs no mark; other positions use marks. Change.Resulting is the caller's resulting-position magnitude.
Requirement.Within(equity)	Admission invariant; exact equality is admitted
Assessment.Increases()	De-risking comparison

Errors: ErrMissingMark, ErrCurrencyMismatch, ErrInvalidPolicy (nil policy, non-positive or zero-value Ratio), ErrInvalidInput (unconstructed listing, or two open positions in the changed instrument).

Deciding whether a proposal is de-risking before touching prices stays with the caller (#413), per ADR-066. This package only does the arithmetic.

Why

#411, step 2 of the Account Margin Admission milestone (#409).

How it was tested

go test -race -cover ./internal/account/margin/ passes, 91.5% coverage; the uncovered lines are arithmetic-overflow error paths. Tests use testify and cover every case the issue lists:

• flat account, and a flat position that needs no mark
• single long; single short
• long + short counted gross (20000), not net (0)
• multi-instrument at ratio 0.5
• reversal: 100 long → 100 short counts 100 units; → 150 short increases
• partial reduction and close
• exact limit admitted; one unit over rejected
• currency mismatch (in positions, on the change, on a closing change)
• missing mark; the changed instrument needs no mark
• multiplier ≠ 1 (ES × 50)
• the feat: enforce account-level margin ratio in backtests #409 SPY case: 100 @ 463.08 rejected at $10k; 21 shares admitted
• an over-limit account whose reduction is still not an increase
• policy errors

No floating point is used.

make fmt-check vet lint is clean. The full go test ./... passes except TestPublicSurface, which fails on main too because of the top-level clock/ package. It's unrelated to this PR.

Documentation changed

• Package doc comment (internal/account/margin/doc.go)
• docs/arch/package-boundaries.org: new row for internal/account/margin

Closes #411

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟡 Changes recommended

Five unresolved moderate correctness issues remain in the core calculation logic.

Review effort: Lite
Findings: 2

Open (2)

• Compare gross notional instead of rounded required margin · New
• Validate zero-value margin policy during accumulator construction · New

What changed in this PR

Adds a shared, pure package for gross-notional and initial-margin calculations.

Changes:

• Implements account, policy, and prospective assessment calculations.
• Adds validation, error handling, and comprehensive tests.
• Documents the package boundary and ADR-066 behavior.

File	Summary
internal/​account/​margin/​margin.go	Core calculations; five unresolved moderate issues involving listing consistency, mark identity, policy validation, input validation order, and gross-vs-rounded comparisons.
internal/​account/​margin/​margin_test.go	Calculation and edge-case tests.
internal/​account/​margin/​errors.go	Calculation error definitions.
internal/​account/​margin/​doc.go	Package documentation and ADR-066 semantics.
docs/​arch/​package-boundaries.org	Documents the new package boundary.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[rustyeddy]

Fixed in 5be0759: Policy now has Validate() error, and newAccumulator calls it up front. Tests cover a zero-value Ratio on a flat Account and on a closing Assess.

[rustyeddy]

Reviewed PR #421 at head 7c4e2ace52b0f6f026f0746aab2605962df3e47b.

The overall direction is good: a small pure package below both risk and sim is exactly the right architectural home for this calculation, and I like the explicit Policy extension point rather than baking percentage margin into the aggregate API.

I see three correctness issues that should be fixed before merge.

1. Assessment.Increases() must compare gross notional, not required margin

I agree with Copilot here.

ADR-066 defines de-risking as:

prospective gross notional <= current gross notional

but Assessment.Increases() currently compares:

a.Prospective.Required.Cmp(a.Current.Required)

That can misclassify a small increase when Money × Rate rounds to the same required-margin amount.

For this v1 rule, the classification should be based on:

a.Prospective.Gross.Cmp(a.Current.Gross)

The required-margin values are what we compare against equity for admission; gross is what determines whether the change is increasing or de-risking.

Please add a regression test where gross increases by the smallest representable amount under a sub-unit ratio but required margin rounds to the same value.

2. Validate the Policy at accumulator construction

Also agree with Copilot.

newAccumulator rejects only a nil interface. A zero-value Ratio{} can silently succeed for:

• a flat account;
• an Assess whose resulting quantity is zero;
• any path that never reaches RequiredMargin.

That contradicts the package contract that a zero-value Ratio is invalid.

I would make policy validity an explicit part of the interface/contract rather than relying on an actual position to exercise it. For example:

type Policy interface {
    Validate() error
    RequiredMargin(Valued, num.Currency) (num.Money, error)
}

or an equivalent internal validation mechanism.

Then newAccumulator validates once up front.

3. Marks and changed positions are keyed at the wrong identity level

This is the larger issue I found beyond Copilot's comments.

The new package defines:

type Marks map[instrument.ID]num.Price

and Assess matches the changed position only by:

p.Listing.InstrumentID().Equal(changedID)

But Trader's runtime position model explicitly says:

one net position per account/listing pair

and the simulator already preserves that distinction with:

type positionKey struct {
    instrumentID instrument.ID
    provider     string
    venue        string
}

Its marks are also stored by that same listing-level key.

That distinction matters here because two Listings for the same economic instrument can have different:

• provider / venue;
• contract multiplier;
• settlement mechanics/currency;
• mark.

With the current Marks map[instrument.ID]...:

• two open listings of the same instrument cannot carry different marks;
• one mark can accidentally value both listings;
• Assess treats two legitimate positions in different listings of the same instrument as an invalid duplicate;
• a Change.Listing for a different provider/venue can be matched against the wrong existing position solely because the economic instrument ID is equal.

I think the margin package should use the same listing identity semantics as the account/simulator.

Something along the lines of a small shared/listing key:

type ListingKey struct {
    InstrumentID instrument.ID
    Provider     string
    Venue        string
}

(or an existing canonical equivalent if one already exists).

Then:

type Marks map[ListingKey]num.Price

and Assess matches the changed position by listing identity, not merely instrument identity.

I would avoid solving this by simply documenting "one listing per instrument" because that would contradict the existing Position and simulator model.

What looks good

The rest of the package direction looks solid:

• exact numeric types throughout;
• current/prospective values use one consistent price for the changed listing;
• other positions use marks, never AvgPrice;
• exact-limit equality is admitted;
• ratio policy is isolated behind the future margin-policy seam;
• flat positions need no mark;
• currency mismatch is explicit rather than silently converted;
• package boundary is clean and does not make sim import risk.

The test coverage is also broad and useful, but I would add tests specifically for the three cases above.

Current state: changes requested before merge.

Once gross-vs-required classification, policy validation, and listing-level mark/change identity are corrected, I expect the package to be in good shape for #413/#415 to build on.

[rustyeddy]

Addressed all three items in 5be0759:

1. Gross vs required: Assessment.Increases() now compares Gross, with a rounding regression test (at ratio 0.1, 1.00000000 → 1.00000001 leaves required margin at 0.10000000 but still counts as an increase).
2. Policy validation: Policy gains Validate(), called once in newAccumulator. A zero-value Ratio is now rejected for a flat account and for a closing change.
3. Listing-level identity: there were already two identical private keys (account.listingKey and sim.positionKey), so rather than add a third I exported one canonical account.ListingKey / account.KeyOf(listing).
   • margin.Marks is now map[account.ListingKey]num.Price, and Assess matches the changed position by listing key.
   • The snapshot's duplicate check uses the exported key (a same-package replacement of the private one; behavior unchanged, internal/account stays at 100% coverage).
   • sim.positionKey is left for feat: sim snapshot per-position marks and margin-aware buying power #412, which touches the simulator anyway.
   • New tests: each listing uses its own mark; a mark for one listing doesn't value another; two listings of one instrument aren't duplicates; a change in another listing doesn't match the existing position.

A consequence for #413 (I'll note it on the issue): risk.findPosition, and so resultingPosition, matches by instrument ID, so the risk rule's resulting quantity and this package's listing-level matching could disagree when an account holds the same instrument in two listings. #413 needs to reconcile that.

[rustyeddy]

Re-review of PR #421 at head 5be07599006d3c551399950b7a548eede872f614.

The three issues from my previous review are fixed:

1. Gross-vs-required classification

   • Assessment.Increases() now compares Prospective.Gross to Current.Gross.
   • The regression test covers the rounding case where gross increases but required margin rounds to the same value.

2. Policy validation

   • Policy now has Validate() error.
   • newAccumulator validates up front, so a zero-value Ratio{} is rejected even for flat/closing paths.
   • Tests cover both cases.

3. Listing-level identity

   • A shared account.ListingKey now represents the same identity already used by account snapshots/simulator positions: instrument + provider + venue.
   • Marks is keyed by ListingKey.
   • Assess matches the changed position by listing identity rather than economic instrument alone.
   • Tests cover two listings of the same instrument with separate marks and verify that changing one does not accidentally match the other.

I also like that ListingKey was added to internal/account rather than duplicated inside margin; that aligns the new package with the existing snapshot uniqueness rule.

The rest of the package still looks consistent with ADR-066:

• exact numeric arithmetic;
• changed listing valued at one consistent price in both current/prospective states;
• other listings use marks, never AvgPrice;
• exact-limit equality is admitted;
• v1 ratio policy remains behind a general per-position margin policy seam;
• currency mismatches remain explicit;
• sim still does not import risk.

I do not see another substantive blocker in the latest diff.

PR #421 is merge-ready from my review.

GitHub currently shows no combined-status entries for this head, so this assessment is based on the current diff, tests described in the PR, and review state rather than an external CI status signal.