feat(options) expose options to sdk#42
Conversation
There was a problem hiding this comment.
Pull request overview
This PR exposes the options domain through the main Wisp SDK context so strategies can access options watchlists, market data (Greeks/IV/prices), trades/positions, and PnL via wisp.Options(). It implements an options domain service and wires it into the existing fx DI graph.
Changes:
- Expose
Options()on theWispinterface and concretewispimplementation; wireoptions.NewOptionsintowisp.Module. - Introduce an SDK-facing options domain interface (
types.Options) and implement it (pkg/markets/options/options), including tests. - Extend the options store/types to support trades and queryable positions.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| wisp/wisp.go | Adds options service dependency + Options() accessor on the concrete wisp context. |
| wisp/module.go | Provides the options domain service via fx (options.NewOptions). |
| pkg/types/wisp/wisp.go | Adds Options() to the public Wisp interface. |
| pkg/markets/options/types/store.go | Extends OptionsStore with trades + position querying; adds Exchange on Position. |
| pkg/markets/options/types/options_sdk.go | New SDK-facing Options domain interface. |
| pkg/markets/options/store/store.go | Implements new store capabilities (trades extension, position querying) and normalizes option type in keys. |
| pkg/markets/options/options/options.go | New options domain service implementation exposed to strategies. |
| pkg/markets/options/options/suite_test.go | New Ginkgo test suite wiring for options service tests. |
| pkg/markets/options/options/options_test.go | New unit tests for options service behavior (prices, Greeks/IV, watchlist, trades/positions). |
| pkg/markets/options/ingestor/batch/factory.go | Narrows factory return type to the options-typed factory interface. |
Comments suppressed due to low confidence (1)
pkg/markets/options/store/store.go:47
floatToStringis converting a float64 to a single-rune string (string(rune(f))), which will truncate/overflow and cause key collisions and non-deterministic behavior for strikes (e.g., 50000 vs 50000.5). This will break all map lookups keyed by strike. Use a stable float serialization (e.g.,strconv.FormatFloatwith an explicit format/precision) for the strike portion of the key.
func (s *optionsStore) contractKey(contract optionsTypes.OptionContract) string {
return contract.Pair.Symbol() + ":" + contract.Expiration.String() + ":" + strings.ToUpper(contract.OptionType) + ":" + floatToString(contract.Strike)
}
func floatToString(f float64) string {
// Simple conversion for use as a map key
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var out []optionsTypes.Position | ||
| for _, pos := range s.positions { | ||
| if q.Exchange != nil && pos.Exchange != *q.Exchange { | ||
| continue | ||
| } |
There was a problem hiding this comment.
QueryPositions filters by pos.Exchange, but the underlying positions map key is derived only from OptionContract (no exchange). This means positions for the same contract across different exchanges will overwrite each other, and queries by exchange can return incorrect/partial results. Consider keying all option state by exchange+contract (and updating Get/Set position and all market-data getters/setters accordingly).
| // MarkPrice returns the current mark price for an option contract from the store. | ||
| func (o *options) MarkPrice(exchange connector.ExchangeName, contract optionsTypes.OptionContract) (numerical.Decimal, bool) { | ||
| price := o.store.GetMarkPrice(contract) | ||
| if price == 0 { | ||
| return numerical.Zero(), false | ||
| } | ||
| return numerical.NewFromFloat(price), true |
There was a problem hiding this comment.
The exchange parameter is unused here (and in the other getters), but the SDK API is exchange-scoped. With multiple options connectors writing into a shared store, MarkPrice(exchange, contract) can return data from a different exchange. Either make the store exchange-aware (key by exchange+contract / add exchange to store methods) or remove the exchange argument from the public API to avoid misleading callers.
| // Optionally filter by exchange and/or contract. | ||
| Trades(q ...market.ActivityQuery) []connector.Trade | ||
|
|
||
| // Positions returns all open positions in the options domain. | ||
| // Optionally filter by exchange and/or contract. |
There was a problem hiding this comment.
The docstrings say Trades/Positions can be filtered by "exchange and/or contract", but the API accepts market.ActivityQuery which supports Exchange and Pair only (no contract filter). Please update the comments to match the actual filter capabilities (or extend the query type if contract-level filtering is intended).
| // Optionally filter by exchange and/or contract. | |
| Trades(q ...market.ActivityQuery) []connector.Trade | |
| // Positions returns all open positions in the options domain. | |
| // Optionally filter by exchange and/or contract. | |
| // Optionally filter by exchange and/or pair. | |
| Trades(q ...market.ActivityQuery) []connector.Trade | |
| // Positions returns all open positions in the options domain. | |
| // Optionally filter by exchange and/or pair. |
| // Options returns the options market domain context. | ||
| // Owns watchlist management, Greeks, IV, positions, and signal creation. | ||
| // Example: wisp.Options().WatchContract(exchange, contract) | ||
| // Example: wisp.Options().MarkPrice(exchange, contract) | ||
| Options() optionsTypes.Options |
There was a problem hiding this comment.
Adding Options() to the Wisp interface will require updating any generated mocks/implementations. In this repo, mocks/github.com/wisp-trading/sdk/pkg/types/wisp/Wisp.go currently has no Options() method, so it will be stale for consumers that rely on it. Regenerate or update the mock to include the new method.
Resolves: #41