M4-02: Define canonical OrderIntent model - #191
Conversation
Adds order.Intent, the broker-ignorant value a strategy or operator uses to express what it wants to accomplish before execution planning translates it into a concrete order.Proposal (ADR-005/ADR-006). Per the design discussion on issue #177: the canonical type lives in order, not a new strategy package. execution and risk already depend on order for the neighboring Proposal->Request->Order->Fill vocabulary (ADR-017); putting Intent in strategy instead would have forced execution to import strategy, directly reversing the "execution does not depend on strategy" boundary #176/PR #190 just established. strategy itself remains uncreated, deferred to M6. v0 ships exactly the four intent kinds ADR-005 itself names -- Enter, Exit, AdjustStop, TargetExposure -- with per-kind field requirements enforced by NewIntent: Enter carries only Side (sizing is risk's job, ADR-006); Exit carries neither Side nor Quantity (it means "remove whatever exposure exists," not a specific delta); AdjustStop carries an absolute StopPrice; TargetExposure carries the explicit desired Side+Quantity, the one kind where quantity genuinely originates from the strategy rather than from risk sizing. No CancelEntry or NoAction kind is added yet, per the same discussion. Intent identifies instrument.ID (the canonical economic identity, ADR-003), not a venue-specific instrument.Listing -- keeping one intent meaningful across every broker adapter; execution planning selects the concrete Listing as planning context. Adds id.IntentID, reinstating one of the three identifier kinds id/doc.go's own "Deferred identifiers" section explicitly earmarked for exactly this moment: Intent is now a concrete, persisted domain object to justify a generated identity. Intent also carries id.Metadata so the intent -> proposal -> risk decision -> request -> fill correlation chain ADR-005's own Consequences section names is anchored from the start. Fixes package-boundaries.org's dependency map, which an earlier version (from #176) had drawn with the canonical intent type living in strategy -- exactly the reversed-dependency mistake this issue's own design discussion caught before implementation. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_014JmhCNQ3Nh3veVXzZifMa9
There was a problem hiding this comment.
🟢 Approval recommended
The new intent model and IntentID integration are consistent with existing constructor/validation patterns, include thorough tests, and the documentation update aligns with the stated package-boundary requirements.
Pull request overview
This PR introduces a canonical, broker-agnostic order.Intent model (ADR-005/ADR-006) that strategies/operators can emit before execution planning translates intents into concrete order.Proposal values, and extends the id package with id.IntentID to support end-to-end correlation from intent through fills. It also updates the architecture dependency map to reflect the corrected package placement (intent in order, not strategy), preserving the “execution does not depend on strategy” boundary.
Changes:
- Added
order.Intent+IntentKindvocabulary with constructor validation enforcing per-kind field rules. - Added
id.IntentID(kind marker, prefix, generate/parse helpers) plus tests to ensure correct wiring and cross-kind rejection. - Updated
docs/arch/package-boundaries.orgto reflectorder.Intentas the canonical intent type and keep dependency direction consistent.
File summaries
| File | Description |
|---|---|
| order/intent.go | Defines IntentKind, Intent, and validation (NewIntent / checkIntent). |
| order/intent_test.go | Adds comprehensive tests covering valid construction and per-kind validation failures. |
| order/helpers_test.go | Adds test helpers for generating IntentID and constructing an instrument.ID fixture. |
| order/errors.go | Introduces ErrInvalidIntent consistent with other order constructor errors. |
| id/parse.go | Adds ParseIntentID / MustParseIntentID wrappers and updates kind-count comment. |
| id/parse_test.go | Extends concrete parse wrapper tests to include IntentID. |
| id/kind.go | Adds intentKind marker, prefix, and exported IntentID type alias documentation. |
| id/id_test.go | Extends prefix coverage to include intentKind → "int". |
| id/generate_kinds.go | Adds GenerateIntentID wrapper and updates kind-count comment. |
| id/generate_kinds_test.go | Extends concrete generate wrapper tests to include IntentID. |
| id/doc.go | Updates package docs to reflect seven kinds and explains reinstatement of IntentID. |
| docs/arch/package-boundaries.org | Corrects dependency map narrative and explicitly documents order.Intent placement rationale. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
rustyeddy
left a comment
There was a problem hiding this comment.
Overall this matches the #177 design discussion well: order.Intent is the right package boundary, the four v0 kinds stay narrow, instrument.ID avoids premature listing binding, and id.IntentID is justified now that the domain object exists.
I found one correlation-contract gap before merge: Intent is documented as the anchor for the intent -> proposal -> risk decision -> request/order -> fill chain, but checkIntent only requires Metadata.EventID to be non-zero. A caller can therefore construct a valid Intent with a zero CorrelationID, leaving later stages with no workflow correlation identifier to preserve. Since this is the first/anchor stage, I think NewIntent should require a non-zero Metadata.CorrelationID as well (and add a focused regression test). If the intended rule is instead that the application service assigns correlation after Intent construction, then the docs should not describe Intent metadata as the anchor from creation time. I prefer requiring it here because that matches the current API/docs and prevents an uncorrelatable intent from entering the pipeline.
Other than that, I don't see a blocker in this pass.
|
Good catch — fixed in #192: |
…ow-up) (#192) Rusty's post-merge review on #191 found a real gap: Intent is documented (both in its own field comment and NewIntent's doc comment) as the anchor for the intent -> proposal -> risk decision -> request/order -> fill correlation chain, but NewIntent only required Metadata.EventID to be non-zero, not Metadata.CorrelationID. A caller could construct a valid Intent with no correlation identifier, leaving every later stage with nothing to correlate back to -- exactly the gap the docs claimed didn't exist. Since Intent is the first/anchor stage, nothing later can retroactively assign a CorrelationID an intent never had. NewIntent now requires Metadata.CorrelationID non-zero, matching the documented contract, with a regression test (TestNewIntentRejectsZeroMetadataCorrelationID). Claude-Session: https://claude.ai/code/session_014JmhCNQ3Nh3veVXzZifMa9 Co-authored-by: Claude Sonnet 5 <[email protected]>
What changed
Adds
order.Intent, the broker-ignorant value a strategy or operatoruses to express what it wants to accomplish before execution planning
translates it into a concrete
order.Proposal(ADR-005/ADR-006).Per the design discussion on issue #177 (Rusty's review of the initial
design notes): the canonical type lives in
order, not a newstrategypackage.execution/riskalready depend onorderforthe neighboring
Proposal -> Request -> Order -> Fillvocabulary(ADR-017); putting
Intentinstrategyinstead would have forcedexecutionto importstrategy, directly reversing the "executiondoes not depend on strategy" boundary #176/PR #190 just established.
strategyitself remains uncreated, deferred to M6.v0 ships exactly the four intent kinds ADR-005 itself names —
Enter,Exit,AdjustStop,TargetExposure— with per-kind fieldrequirements enforced by
NewIntent:Enter:Siderequired; sizing is risk's job (ADR-006), so noQuantity.Exit: noSide/Quantity— means "remove whatever exposureexists," not a specific delta.
AdjustStop: an absoluteStopPricerequired.TargetExposure: explicitSide+Quantityrequired — the one kindwhere quantity genuinely originates from the strategy, not from risk
sizing.
No
CancelEntryorNoActionkind yet, per the same discussion.Intentidentifiesinstrument.ID(the canonical economic identity,ADR-003), not a venue-specific
instrument.Listing— keeping oneintent meaningful across every broker adapter; execution planning
selects the concrete
Listingas planning context.Adds
id.IntentID, reinstating one of the three identifier kindsid/doc.go's own "Deferred identifiers" section explicitly earmarkedfor exactly this moment.
Intentalso carriesid.Metadataso theintent -> proposal -> risk decision -> request -> fillcorrelationchain ADR-005 names is anchored from the start.
Also fixes
package-boundaries.org's dependency map, which theversion merged in #176/PR #190 had drawn with the canonical intent
type living in
strategy— exactly the reversed-dependency mistakethis issue's own design discussion caught before implementation.
Why
Issue #177 (M4-02) asks for the canonical intent value ADR-005
requires, independent of
brokerand concrete broker adapters,without prematurely encoding broker request details or implementing a
strategy engine.
Design process
Posted design notes on #177 covering four open questions (package
placement,
order.Sidereuse,Intent's shape,id.IntentID).Rusty's review agreed with the
id.IntentIDproposal but correctedpackage placement (
order.Intent, notstrategy.Intent), narrowedthe v0 kind set to exactly what ADR-005 names, and specified
instrument-identity (not listing) plus the per-kind field semantics
above. Implemented per that direction.
Test plan
go build ./...,go vet ./...,gofmt -l .all cleango test ./... -racepasses across the whole repositoryorder/intent_test.gocover every validconstruction and every per-kind field-requirement violation
idpackage tests extended to cover the newIntentIDkind(prefix, generate, parse/must-parse, cross-kind rejection)
broker/boundary_test.go'sTestAccountAndOrderDoNotImportBrokerre-run directly: still passes,
orderremains broker-freeid98.0%,order100.0% (both re-measured directly)Closes #177
🤖 Generated with Claude Code
https://claude.ai/code/session_014JmhCNQ3Nh3veVXzZifMa9