You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The reason will be displayed to describe this comment to others. Learn more.
walkthrough.md references BUILD_TAG = "950" and suggests verification via IPC commands, but this PR updates BUILD_TAG to 954 and also disables IPC connectivity in the panel. Please update the walkthrough to match the current build number and the current (non-IPC) verification procedure so operators aren’t following a stale runbook.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The reason will be displayed to describe this comment to others. Learn more.
Dead code left in place after early return
The return; at line 3003 leaves the entire try/catch block unreachable. While the comment is clear about the intent, the dead code includes non-trivial logic: thread creation (receiveThread), TcpClient/TcpStream allocation, and a catch that resets isConnected / increments _ipcRetryCount. If isConnected or _ipcRetryCount are read elsewhere (e.g. a status display), they will permanently show their initial/stale values rather than reflecting "intentionally offline".
Consider either removing the body entirely (leaving only the comment + return) or adding a one-time log line that records the intentional no-op state so the status LED / connection state surface the right information.
The reason will be displayed to describe this comment to others. Learn more.
Remove unconditional return in ConnectToStrategy
ConnectToStrategy() now exits immediately, so SendCommand() can never establish tcpStream and every panel action silently becomes a no-op. This is a functional regression because the strategy still starts and uses the IPC server (StartIpcServer() in src/V12_002.cs and command handling in src/V12_002.UI.IPC.cs), so disabling connection at the indicator side breaks runtime control/sync flows rather than removing dead code.
The reason will be displayed to describe this comment to others. Learn more.
ConnectToStrategy now has an unconditional return before a try block, leaving the subsequent statements unreachable. This will fail compilation (unreachable code). If IPC is deprecated, remove/comment the old implementation or gate it behind a conditional compilation symbol instead of returning mid-method.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The reason will be displayed to describe this comment to others. Learn more.
TOCTOU on RemainingContracts in follower path
The guard at line 601 reads _rPos.RemainingContracts > 0 without holding stateLock, then immediately re-reads the same field under the lock at line 604. Between these two reads another thread could decrement RemainingContracts to 0 (e.g. a fill callback), so _rQty could be 0 or negative despite the earlier check passing, and CreateNewStopOrder would be called with _rQty = 0.
The simpler — and consistent — fix is to read once under the lock:
Note: the master path in HandleOrderCancelled (line 372-374) has the same pattern without any lock at all — consistency across both paths would be worth aligning.
if (_psr.Value.BracketRestorationNeeded && _psr.Value.CapturedTargets != null)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The reason will be displayed to describe this comment to others. Learn more.
In the follower branch, the result of ExecutingAccount.CreateOrder is submitted immediately, before checking for null. If CreateOrder returns null, Submit(new[] { newStop }) can throw and the later null-handling/flatten path is bypassed, leaving the position potentially unprotected. Null-guard the order before Submit (and consider wrapping Submit in try/catch consistent with the repo’s CreateOrder-null convention).
Suggested change
pos.ExecutingAccount.Submit(new[]{newStop});
// Guard against null CreateOrder results and submit failures to preserve later null-handling logic.
if(newStop!=null)
{
try
{
pos.ExecutingAccount.Submit(new[]{newStop});
}
catch(ExceptionsubmitEx)
{
// On submit failure, fall back to the same path as a null order (newStop == null).
Print("Error submitting follower replacement stop for "+entryName+": "+submitEx.Message);
The reason will be displayed to describe this comment to others. Learn more.
Null check can be simplified
The ?. operator allows you to access a property or a member only if it is non-null. Consider simplifying the highlighted expression, i.e. the object and its member's null checks via the usage of this ?. operator.
// Only restore targets the broker OCO cascade-cancelled.
// Filled targets have OrderState.Filled -- skip them.
if (snap.CapturedOrder.OrderState != OrderState.Cancelled
The reason will be displayed to describe this comment to others. Learn more.
In the unmanaged/local restore path, the target signal name ("T{n}_" + entryName) is not trimmed and can exceed the established max length for target signals. Use the same trimming approach as the follower path (e.g., SymmetryTrim to 40 chars / ensure <= 50) to avoid NinjaTrader signal truncation/collisions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The reason will be displayed to describe this comment to others. Learn more.
Nullable check expression can be simplified
While it is common to explicitly check an object's nullability before accessing its value, you can further simplify this expression by
either using the conditional access operator ? or pattern matching instead. These proposed solutions are succinct and reduce your
keystrokes.
The reason will be displayed to describe this comment to others. Learn more.
Nullable check expression can be simplified
While it is common to explicitly check an object's nullability before accessing its value, you can further simplify this expression by
either using the conditional access operator ? or pattern matching instead. These proposed solutions are succinct and reduce your
keystrokes.
The reason will be displayed to describe this comment to others. Learn more.
When pendingStopReplacements.TryAdd fails, the snapshot block populates CapturedTargets on the local newPending object, which is not the instance stored in pendingStopReplacements. This can drop the captured target snapshot entirely on the TryAdd-failed path. Populate CapturedTargets/BracketRestorationNeeded on the actual pending instance in the dictionary (either the one returned by TryGetValue or via AddOrUpdate pattern).
The reason will be displayed to describe this comment to others. Learn more.
Nullable check expression can be simplified
While it is common to explicitly check an object's nullability before accessing its value, you can further simplify this expression by
either using the conditional access operator ? or pattern matching instead. These proposed solutions are succinct and reduce your
keystrokes.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
walkthrough.md references BUILD_TAG = "950" and suggests verification via IPC commands, but this PR updates BUILD_TAG to 954 and also disables IPC connectivity in the panel. Please update the walkthrough to match the current build number and the current (non-IPC) verification procedure so operators aren’t following a stale runbook.