Skip to content

Conversation

@mccaffers
Copy link
Owner

No description provided.

Copilot AI review requested due to automatic review settings November 14, 2025 18:17
@mccaffers mccaffers linked an issue Nov 14, 2025 that may be closed by this pull request
@mccaffers mccaffers self-assigned this Nov 14, 2025
@mccaffers mccaffers added the enhancement New feature or request label Nov 14, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request migrates the backtesting codebase from streaming files from S3 to using QuestDB as a data source. The changes are extensive and involve significant architectural modifications.

Key Changes

  • Upgraded from .NET 8.0 to .NET 9.0 across all projects
  • Replaced S3-based file streaming with QuestDB direct database queries
  • Refactored environment variable management from mock-based to injection-based pattern
  • Removed web server and UI components
  • Updated test infrastructure to use JSON configuration files instead of mocks

Reviewed Changes

Copilot reviewed 150 out of 155 changed files in this pull request and generated 30 comments.

Show a summary per file
File Description
tests/test.csproj Upgraded target framework to net9.0
tests/resources/environmentVariables.json Added JSON configuration file for test environment variables
tests/backtesting/TestEnvironmentSetup.cs Refactored to load environment from JSON and use injection pattern
tests/backtesting/UtilitesTests/ExtensionTests.cs Updated tests to use new environment setup
tests/backtesting/StrategyTests/RandomStrategyTests.cs Modernized test implementation with proper AAA pattern
tests/backtesting/Shared.cs Added EnvironmentJson class for JSON deserialization
tests/backtesting/ReportingTests/ReportingTests.cs Updated tests to use environment injection
src/utilities/utilities.csproj Upgraded to net9.0, added MathNet.Numerics package
src/utilities/StandardDeviation.cs Added new statistical calculation utilities
src/utilities/ServiceExtension.cs Removed environment variable dependency, added PropertyCopier utility
src/utilities/ReportEnvironmentVariables.cs Added new utility for reporting environment variables
src/utilities/EnvironmentVariables_v2.cs Implemented new enum-based environment variable system
src/utilities/ConsoleLogger.cs Updated to use new environment variable system
src/utilities/MedianExtensions.cs Added median calculation extensions
src/utilities/LinearRegression.cs Added linear regression utilities
src/strategies/strategies.csproj Upgraded to net9.0 and Microsoft.Extensions.Hosting 8.0.1
src/strategies/Random.cs Updated to use new strategy definition pattern
src/strategies/BaseStrategy.cs Removed environment variables and web notification dependencies
src/backtesting/backtesting-engine.csproj Upgraded to net9.0, added Npgsql for QuestDB connectivity
src/backtesting/Program.cs Major refactoring to use QuestDB instead of S3 streaming
src/backtesting/Ingest/IngestFromQuestDB.cs New ingestion layer for QuestDB
src/backtesting/Operations/Positions.cs Updated to use new environment variable system
src/backtesting/TradeManagement/CloseOrder.cs Simplified trade closing logic
src/backtesting/Analysis/Reporting.cs Enhanced reporting with statistical metrics
src/Models/models.csproj Upgraded to net9.0, added MemoryPack serialization
src/Models/StrategyDefinition.cs Added new strategy configuration model

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

"LAMBDA_LOG": "FALSE",
"SYSTEM_LOG": "TRUE",
"CONSOLE_LOG": "TRUE",
"SCALING_FACRTOR": "TestEnvironmentSetup,1000;",
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Spelling error: "SCALING_FACRTOR" should be "SCALING_FACTOR"

Copilot uses AI. Check for mistakes.

// TradingVariables
public required string STRATEGY { get; set; }
public required string SCALING_FACRTOR { get; set; }
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Spelling error: "SCALING_FACRTOR" should be "SCALING_FACTOR". This typo appears consistently throughout the codebase.

Copilot uses AI. Check for mistakes.
public enum TradingVariables
{
STRATEGY,
SCALING_FACRTOR,
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Spelling error: "SCALING_FACRTOR" should be "SCALING_FACTOR"

Copilot uses AI. Check for mistakes.
/// <param name="exclusiveEnd">The exclusive exclusiveEnd index.</param>
/// <param name="rsquared">The r^2 value of the line.</param>
/// <param name="yintercept">The y-intercept value of the line (i.e. y = ax + b, yintercept is b).</param>
/// <param name="slope">The slop of the line (i.e. y = ax + b, slope is a).</param>
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Spelling error in comment: "slop" should be "slope"

Copilot uses AI. Check for mistakes.
EnvironmentVariables.Inject(LoggingVariables.LAMBDA_LOG, BACKTESTING_VARIABLES.LAMBDA_LOG.ToString());
EnvironmentVariables.Inject(LoggingVariables.SYSTEM_LOG, BACKTESTING_VARIABLES.SYSTEM_LOG.ToString());

EnvironmentVariables.Inject(TradingVariables.SCALING_FACRTOR, "AUDUSD,10000;EURUSD,10000;GBRIDXGBP,1;GBPUSD,10000;NZDUSD,10000;USDJPY,100;GBPJPY,100;EURJPY,100;USDCAD,10000;FRAIDXEUR,1;EURGBP,10000;USA500IDXUSD,1;AUSIDXAUD,1;USDCHF,10000;XAUUSD,1;XAGUSD,1;USATECHIDXUSD,1;EURCHF,10000;DEUIDXEUR,1;USA30IDXUSD,1;LIGHTCMDUSD,1;JPNIDXJPY,1;BRENTCMDUSD,1;");
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Spelling error: "SCALING_FACRTOR" should be "SCALING_FACTOR"

Copilot uses AI. Check for mistakes.
}

private static IEnumerable<DictionaryEntry>? entries = Environment.GetEnvironmentVariables().Cast<DictionaryEntry>();
private static Dictionary<String, String> envCache = [];
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Field 'envCache' can be 'readonly'.

Copilot uses AI. Check for mistakes.
Comment on lines +73 to +80
catch (Exception ex)
{
result = ex.Message;
Console.WriteLine(ex);
ConsoleLogger.Log($"Unexpected error: {result}");
await SendStackException(ex);

}
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Generic catch clause.

Copilot uses AI. Check for mistakes.
Comment on lines +99 to +102
catch (Exception ex)
{
ConsoleLogger.Log($"Error during cleanup: {ex.Message}");
}
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Generic catch clause.

Copilot uses AI. Check for mistakes.
Comment on lines +231 to +234
catch (Exception ex)
{
throw new TradingException($"Error retrieving OHLC data: {ex.Message}", ex);
}
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Generic catch clause.

Copilot uses AI. Check for mistakes.
Comment on lines +213 to 220
if (tradingObjects.openTrades.Count > 0)
{
return tradingObjects.openTrades.Select(x => x.Value);
}
else
{
return [];
}
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Both branches of this 'if' statement return - consider using '?' to express intent better.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 150 out of 155 changed files in this pull request and generated 2 comments.


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

Host=localhost;
Port=8812;
Username=admin;
Password=quest;
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

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

Hardcoded database credentials are a security risk. The password 'quest' should be loaded from environment variables or a secure configuration source instead of being hardcoded in the connection string.

Copilot uses AI. Check for mistakes.
count++; // to track results
if (count % 5 != 0)
{
continue; // skip every 10th tick
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

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

The comment says "skip every 10th tick" but the code actually skips 4 out of every 5 ticks (only processes when count % 5 == 0). This comment is misleading and should be corrected to accurately reflect the filtering logic.

Copilot uses AI. Check for mistakes.
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
2 Security Hotspots
33.6% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@mccaffers mccaffers merged commit 626421e into main Nov 30, 2025
3 of 4 checks passed
@mccaffers mccaffers deleted the 1-refactor-to-make-use-of-questdb-instead-of-tick-csv-files branch November 30, 2025 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

2 participants