Fix sprint-critical runtime issues and add benchmarks workspace - #1
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a17244baa5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let rpc_url = "https://api.mainnet-beta.solana.com".to_string(); | ||
| Self { | ||
| selector, | ||
| signer: signer.map(Arc::new), | ||
| rpc_client: Arc::new(RpcClient::new(rpc_url)), |
There was a problem hiding this comment.
The cached RpcClient is now always created with https://api.mainnet-beta.solana.com, so the executor no longer uses NodeSelector output (or the SOL_RPC value wired in daemon/main.rs) for balance checks and transaction submission. Any deployment configured for a different cluster or private RPC will send trades to the wrong endpoint, which can cause balance checks to fail and submitted signatures to be invalid for the intended environment.
Useful? React with 👍 / 👎.
| }; | ||
| fields.push((35, msg_type.to_string())); | ||
| } | ||
| fields.sort_by_key(|(tag, _)| *tag); |
There was a problem hiding this comment.
Sorting all tags numerically produces invalid FIX framing for session messages (for example, tag 34 will precede 35, and no explicit trailer construction is performed), and FixSession writes this output directly to the socket for logon/heartbeat/logout. In a real FIX counterparty this format is rejected, so the new serializer can break all outbound session traffic instead of producing interoperable FIX 4.4 messages.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR addresses several runtime/compilation issues across the trading workspace and introduces a dedicated benchmarks/ crate so benchmark tooling can coexist with a virtual workspace manifest.
Changes:
- Added a
benchmarks/crate to host Criterion benches and removed top-level virtual-manifest bench configuration. - Hardened/adjusted runtime behavior across risk (kill switch), execution routing, daemon shutdown, and daemon persistence/execution paths.
- Improved backtest multi-symbol mark-to-market and feature-engine short-window metrics handling.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Adds benchmarks as a workspace member and removes illegal top-level bench config. |
| crates/risk/src/kill_switch.rs | Swaps kill-switch active flag to AtomicBool while keeping metadata behind RwLock. |
| crates/execution/src/router.rs | Caches EXECUTION_MODE via OnceLock to avoid repeated env lookups. |
| crates/daemon/src/shutdown.rs | Removes panic-prone Clone path for ShutdownReceiver; promotes subscribe(). |
| crates/fix/src/lib.rs | Implements a basic FIX serializer encode() (tag=value SOH-delimited bytes). |
| crates/feature/src/lib.rs | Tracks last_price and adds 5-minute buy-count auto-reset timestamping. |
| crates/backtest/src/engine.rs | Tracks last_prices per symbol for correct multi-symbol mark-to-market. |
| crates/executor/src/lib.rs | Caches an RpcClient inside ExecutorService to reuse it across calls. |
| crates/daemon/src/main.rs | Adds Sell handling on executor success path and changes persisted fill-price handling. |
| benchmarks/Cargo.toml | New benchmarks crate manifest with Criterion benches. |
| benchmarks/src/lib.rs | New crate root for the benchmarks harness. |
| benchmarks/ci_regression.json | Adds benchmark baseline/threshold configuration for CI regression tracking. |
| benchmarks/benches/01_tick_pipeline.rs | Adds tick-pipeline performance benchmarks. |
| benchmarks/benches/03_risk_checks.rs | Adds risk-check microbenchmarks (kill switch, branchless check, GARCH). |
| benchmarks/benches/05_fix_serializer.rs | Adds FIX serializer encoding benchmark. |
| benchmarks/benches/09_memory_layout.rs | Adds memory layout / false sharing / SPSC microbenchmarks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| let mut out = String::new(); | ||
| for (tag, val) in fields { | ||
| out.push_str(&format!("{}={}", tag, val)); | ||
| } | ||
| out.into_bytes() |
| pub async fn check(&self) -> Result<(), String> { | ||
| let ks = self.kill_switch.read().await; | ||
| if ks.active { | ||
| if ks.active.load(Ordering::Relaxed) { |
|
@copilot open a new pull request to apply changes based on the comments in this thread and also fix the ci |
|
@Ashutosh0x I've opened a new pull request, #2, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Motivation
Description
benchmarks/workspace crate and removed illegal[[bench]]sections from the top-levelCargo.tomlso Cargo can parse the virtual manifest; copied existing bench sources intobenchmarks/benches/and addedbenchmarks/Cargo.toml.OnceLockincrates/execution/src/router.rsto avoid repeated env lookups at runtime (EXECUTION_MODE).Cloneimplementation path forShutdownReceiverincrates/daemon/src/shutdown.rsand require callers to useShutdownController::subscribe()for new receivers.encode()incrates/fix/src/lib.rsthat emits SOH-delimited tag=value bytes and synthesizes tag35when missing.crates/feature/src/lib.rsto tracklast_priceand added a 5-minutebuy_count_5mauto-reset timestamp to keep short-window counters accurate.crates/backtest/src/engine.rsby trackinglast_pricesper symbol and using them for non-current-symbol valuation.activeboolean with anAtomicBoolincrates/risk/src/kill_switch.rsto allow cheap atomic inspection in hot paths while leaving lock-protected metadata underRwLock.Sellhandling to the executor success path and removed persisting afill_price = 0.0sentinel incrates/daemon/src/main.rs(temporary non-zero estimate used until execution returns fills).RpcClientinsideExecutorServiceincrates/executor/src/lib.rsand reuse it for pre-trade balance checks and send/confirm paths to reduce allocation/lookup overhead.Testing
cargo check --workspace; this was attempted but failed due to environment network restrictions when contacting crates.io so the workspace could not be fully validated in this environment.cargo check --workspace --offline; this failed because required dependencies are not available in the local offline cache.cargo bench -p benchmarks --no-run; the bench invocation failed for the same crates.io access limitation, but thebenchmarks/crate and sources were created and copied successfully.cargo fmt --all; this failed becauserustfmtis not installed in the current toolchain environment.Full compile, test and benchmark runs should be executed in a network-enabled CI environment to validate end-to-end build and runtime behavior.
Codex Task