Skip to content

feat: execution result feedback — callback, records, base executor#48

Merged
lwtsn merged 1 commit into
mainfrom
fix/execution-result-feedback
Apr 10, 2026
Merged

feat: execution result feedback — callback, records, base executor#48
lwtsn merged 1 commit into
mainfrom
fix/execution-result-feedback

Conversation

@lwtsn

@lwtsn lwtsn commented Apr 10, 2026

Copy link
Copy Markdown
Contributor
  • Add ExecutionCallback wrapping <-chan ExecutionResult with Await() and AwaitWithTimeout(); Emit() always returns one, callers can discard it
  • Add ExecutionRecord + ExecutionRecords store for durable per-signal bookkeeping written after every execution regardless of Await() usage
  • Add Activity.Executions() so strategies can query execution history
  • Add base executor (pkg/markets/base/executor) shared by all domain executors; eliminates duplicate connector-lookup boilerplate
  • Wire options SignalExecutor into the main executor switch — options signals were previously hitting "unsupported signal type"
  • Fix perp and options executors not calling store.AddOrder after a successful order placement
  • Distinguish HookError (order placed, post-hook failed) from Error (order itself failed) in ExecutionResult and ExecutionRecord
  • Regenerate mocks to cover ExecutionRecords and updated interfaces

- Add ExecutionCallback wrapping <-chan ExecutionResult with Await() and
  AwaitWithTimeout(); Emit() always returns one, callers can discard it
- Add ExecutionRecord + ExecutionRecords store for durable per-signal
  bookkeeping written after every execution regardless of Await() usage
- Add Activity.Executions() so strategies can query execution history
- Add base executor (pkg/markets/base/executor) shared by all domain
  executors; eliminates duplicate connector-lookup boilerplate
- Wire options SignalExecutor into the main executor switch — options
  signals were previously hitting "unsupported signal type"
- Fix perp and options executors not calling store.AddOrder after a
  successful order placement
- Distinguish HookError (order placed, post-hook failed) from Error
  (order itself failed) in ExecutionResult and ExecutionRecord
- Regenerate mocks to cover ExecutionRecords and updated interfaces
Copilot AI review requested due to automatic review settings April 10, 2026 02:36
@lwtsn
lwtsn merged commit d6abca9 into main Apr 10, 2026
4 of 5 checks passed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds first-class execution outcome feedback to signal dispatch (callbacks + durable execution records), refactors domain executors around a shared base executor, and fixes/extends executor routing (including options signals and HookError vs Error semantics).

Changes:

  • Add ExecutionCallback + SignalRouter.RouteWithResult to allow strategies to await execution outcomes without blocking Emit.
  • Add ExecutionRecord + ExecutionRecords store and expose execution history via Activity.Executions().
  • Introduce a shared baseExecutor.Base, wire options signals into the main executor, and fix missing store.AddOrder calls in perp/options executors.

Reviewed changes

Copilot reviewed 32 out of 32 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
wisp/wisp.go Make Emit return an ExecutionCallback backed by RouteWithResult.
pkg/types/wisp/wisp.go Update Wisp interface Emit signature to return ExecutionCallback.
pkg/types/wisp/activity/activity.go Extend Activity interface with Executions() accessor.
pkg/types/execution/types.go Add HookError to ExecutionResult to distinguish hook failures.
pkg/types/execution/router.go Extend SignalRouter with RouteWithResult.
pkg/types/execution/record.go Introduce ExecutionRecord and ExecutionRecords interface.
pkg/types/execution/hook.go Extend Executor interface with ExecuteSignalWithResult.
pkg/types/execution/callback.go Add ExecutionCallback with Await/AwaitWithTimeout.
pkg/markets/spot/executor/executor.go Embed base executor and refactor connector lookup via GetOrderExecutor.
pkg/markets/prediction/executor/executor.go Embed base executor and minor execution result handling tweaks.
pkg/markets/perp/types/store.go Add PositionsStoreExtension to perp MarketStore interface.
pkg/markets/perp/store/store.go Wire PositionsStoreExtension into perp store implementation.
pkg/markets/perp/executor/executor.go Embed base executor, mark orders filled on trades, and persist placed orders.
pkg/markets/options/types/store.go Add PositionsStoreExtension to options store interface.
pkg/markets/options/store/store.go Wire PositionsStoreExtension into options store implementation.
pkg/markets/options/executor/executor.go Replace order API with signal executor implementation and persist placed orders.
pkg/markets/options/executor/executor_test.go Update tests to cover options SignalExecutor behavior and order persistence.
pkg/markets/base/executor/base.go Add shared executor dependency container + GetOrderExecutor helper.
pkg/executor/suite_test.go Add Ginkgo suite for executor package tests.
pkg/executor/router.go Add RouteWithResult; extend profiling router with RouteWithResult.
pkg/executor/module.go Provide ExecutionRecords store via records.NewStore.
pkg/executor/executor_test.go Add coverage for ExecuteSignalWithResult and RouteWithResult.
pkg/executor/default.go Add execution recording, options signal routing, and HookError handling.
pkg/execution/records/store.go Implement in-memory ExecutionRecords store.
pkg/activity/activity.go Inject and expose ExecutionRecords via Activity.Executions().
mocks/github.com/wisp-trading/sdk/pkg/types/wisp/Wisp.go Regenerate mock for Emit returning ExecutionCallback.
mocks/github.com/wisp-trading/sdk/pkg/types/wisp/activity/Activity.go Regenerate mock for new Executions() method.
mocks/github.com/wisp-trading/sdk/pkg/types/execution/SignalRouter.go Regenerate mock for RouteWithResult.
mocks/github.com/wisp-trading/sdk/pkg/types/execution/Executor.go Regenerate mock for ExecuteSignalWithResult.
mocks/github.com/wisp-trading/sdk/pkg/types/execution/ExecutionRecords.go Add mock for new ExecutionRecords interface.
mocks/github.com/wisp-trading/sdk/pkg/markets/perp/types/MarketStore.go Regenerate mock to include new store extension methods.
mocks/github.com/wisp-trading/sdk/pkg/markets/options/types/OptionsStore.go Regenerate mock to include new store extension methods.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/executor/router.go
Comment on lines +84 to +100

r.inner.RouteWithResult(signal, ch)

executionTime := time.Since(start)

r.store.RecordExecution(profileTypes.StrategyMetrics{
StrategyName: string(signal.GetStrategy()),
ExecutionTime: executionTime,
SignalGenTime: signalAge,
Timestamp: time.Now(),
Success: true,
})

if r.detector != nil {
r.detector.CheckExecution(string(signal.GetStrategy()), executionTime)
r.detector.UpdateBaseline(string(signal.GetStrategy()), executionTime)
}

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

profilingRouter.RouteWithResult measures executionTime immediately after calling inner.RouteWithResult, but that call is asynchronous. As a result, the recorded ExecutionTime will be near-zero and won’t reflect the actual signal execution duration. Consider wrapping the provided channel (e.g., route into an internal buffered channel), then in a goroutine wait for the result, compute duration, record metrics, and finally forward the result to the caller’s channel.

Suggested change
r.inner.RouteWithResult(signal, ch)
executionTime := time.Since(start)
r.store.RecordExecution(profileTypes.StrategyMetrics{
StrategyName: string(signal.GetStrategy()),
ExecutionTime: executionTime,
SignalGenTime: signalAge,
Timestamp: time.Now(),
Success: true,
})
if r.detector != nil {
r.detector.CheckExecution(string(signal.GetStrategy()), executionTime)
r.detector.UpdateBaseline(string(signal.GetStrategy()), executionTime)
}
resultCh := make(chan execution.ExecutionResult, 1)
r.inner.RouteWithResult(signal, resultCh)
go func() {
result := <-resultCh
executionTime := time.Since(start)
r.store.RecordExecution(profileTypes.StrategyMetrics{
StrategyName: string(signal.GetStrategy()),
ExecutionTime: executionTime,
SignalGenTime: signalAge,
Timestamp: time.Now(),
Success: true,
})
if r.detector != nil {
r.detector.CheckExecution(string(signal.GetStrategy()), executionTime)
r.detector.UpdateBaseline(string(signal.GetStrategy()), executionTime)
}
ch <- result
}()

Copilot uses AI. Check for mistakes.
Comment thread pkg/executor/router.go
Comment on lines +85 to +100
r.inner.RouteWithResult(signal, ch)

executionTime := time.Since(start)

r.store.RecordExecution(profileTypes.StrategyMetrics{
StrategyName: string(signal.GetStrategy()),
ExecutionTime: executionTime,
SignalGenTime: signalAge,
Timestamp: time.Now(),
Success: true,
})

if r.detector != nil {
r.detector.CheckExecution(string(signal.GetStrategy()), executionTime)
r.detector.UpdateBaseline(string(signal.GetStrategy()), executionTime)
}

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

profilingRouter.RouteWithResult always records Success: true regardless of whether the execution failed (result.Success=false, Error/HookError set). This will skew profiling stats and anomaly detection. When recording metrics, set Success based on the actual ExecutionResult and populate the Error field when applicable.

Suggested change
r.inner.RouteWithResult(signal, ch)
executionTime := time.Since(start)
r.store.RecordExecution(profileTypes.StrategyMetrics{
StrategyName: string(signal.GetStrategy()),
ExecutionTime: executionTime,
SignalGenTime: signalAge,
Timestamp: time.Now(),
Success: true,
})
if r.detector != nil {
r.detector.CheckExecution(string(signal.GetStrategy()), executionTime)
r.detector.UpdateBaseline(string(signal.GetStrategy()), executionTime)
}
resultCh := make(chan execution.ExecutionResult, 1)
r.inner.RouteWithResult(signal, resultCh)
go func() {
result := <-resultCh
executionTime := time.Since(start)
metrics := profileTypes.StrategyMetrics{
StrategyName: string(signal.GetStrategy()),
ExecutionTime: executionTime,
SignalGenTime: signalAge,
Timestamp: time.Now(),
Success: result.Success,
}
if result.Error != nil {
metrics.Error = result.Error.Error()
} else if result.HookError != nil {
metrics.Error = result.HookError.Error()
}
r.store.RecordExecution(metrics)
if r.detector != nil {
r.detector.CheckExecution(string(signal.GetStrategy()), executionTime)
r.detector.UpdateBaseline(string(signal.GetStrategy()), executionTime)
}
ch <- result
}()

Copilot uses AI. Check for mistakes.
Comment on lines +117 to +121
Quantity: action.Quantity,
Price: action.Price,
Status: connector.OrderStatusPending,
Type: connector.OrderTypeLimit,
CreatedAt: e.TimeProvider.Now(),

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When action.Price.IsZero() the code places a market order, but the stored order is still recorded with Type: connector.OrderTypeLimit (and Price will be zero). This will misclassify market orders in activity/history. Set the stored order Type to Market when placing market orders (and keep Limit for limit orders).

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants