Conversation
moconnell
temporarily deployed
to
development
GitHub Actions
Inactive
📝 WalkthroughWalkthroughThis PR extracts order lifecycle into a new IOrderManager/OrderManager with a TradeAdvisor for repricing, replaces broker-streaming ManageOrdersAsync with placement + subscription APIs, adds effective-weights Azure Function endpoints and DTOs, updates configs/package versions and dev tooling, and expands/refactors tests and DI wiring accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as RebalanceCommand
participant OrdMgr as OrderManager
participant Broker as IYoloBroker
participant Advisor as TradeAdvisor
participant Events as BrokerOrderEventStream
Client->>OrdMgr: ManageOrdersAsync(trades, settings, advisor, ct)
OrdMgr->>Broker: SubscribeOrderUpdatesAsync(ct)
Broker-->>Events: emit BrokerOrderEvent...
Events-->>OrdMgr: BrokerOrderEvent(update)
OrdMgr->>Broker: PlaceTradesAsync(pending trades)
Broker-->>OrdMgr: TradeResult(s)
OrdMgr->>OrdMgr: maintain OrderTracker(s) / yield OrderUpdate(s)
par Timeout handling
OrdMgr->>OrdMgr: detect timeout for tracker
OrdMgr->>Advisor: GetReplacementTradeAsync(timedOutTrade, ct)
Advisor->>Broker: GetPositionsAsync/GetMarketsAsync(ct)
Broker-->>Advisor: positions/markets
Advisor-->>OrdMgr: replacement Trade?
OrdMgr->>Broker: PlaceTradesAsync([replacement]) (if present)
Broker-->>OrdMgr: TradeResult
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
moconnell
had a problem deploying
to
development
GitHub Actions
Failure
moconnell
temporarily deployed
to
development
GitHub Actions
Inactive
moconnell
temporarily deployed
to
development
GitHub Actions
Inactive
moconnell
temporarily deployed
to
development
GitHub Actions
Inactive
moconnell
had a problem deploying
to
development
GitHub Actions
Failure
moconnell
temporarily deployed
to
development
GitHub Actions
Inactive
🚀 Deployed to Azure FunctionsEnvironment: This environment will be automatically cleaned up when the PR is closed. |
🚀 Deployed to Azure FunctionsEnvironment: This environment will be automatically cleaned up when the PR is closed. |
🚀 Deployed to Azure FunctionsEnvironment: This environment will be automatically cleaned up when the PR is closed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (4)
test/YoloAbstractions.Test/TradeTest.cs (1)
59-66: Prefer aLimitorder in this test to make intent explicit.The test is validating minimum order value via price, but currently uses
OrderType.Marketwith a supplied limit price. Switching this case toOrderType.Limit(or adding both variants) would make the behavior under test clearer.Proposed test tweak
- OrderType.Market, + OrderType.Limit,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/YoloAbstractions.Test/TradeTest.cs` around lines 59 - 66, The test constructs a Trade with a supplied limit price but uses OrderType.Market; change the order type to OrderType.Limit in the Trade constructor call (the new Trade(...) invocation) so the test explicitly exercises price-based minimum order value logic, or alternatively add a second test case that uses OrderType.Limit in addition to the existing Market case to cover both variants.test/YoloAbstractions.Test/BrokerOrderEventTest.cs (1)
7-29: Add a default-arguments test forBrokerOrderEvent.Current coverage validates the explicit failure payload, but it misses the constructor path where
ErrorandErrorCodeare omitted. A second test would lock down the default/null behavior.Proposed test addition
public class BrokerOrderEventTest { [Fact] public void Constructor_ShouldPopulateProperties() @@ evt.ErrorCode.ShouldBe(500); } + + [Fact] + public void Constructor_ShouldUseNullDefaults_ForOptionalErrorFields() + { + var order = new Order( + Id: 42, + Symbol: "SOL", + AssetType: AssetType.Future, + Created: DateTime.UtcNow, + OrderSide: OrderSide.Buy, + OrderStatus: OrderStatus.Open, + Amount: 1.5m, + Filled: 0.5m, + LimitPrice: 100m, + ClientId: "c1"); + + var evt = new BrokerOrderEvent("c1", order, Success: true); + + evt.Success.ShouldBeTrue(); + evt.Error.ShouldBeNull(); + evt.ErrorCode.ShouldBeNull(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/YoloAbstractions.Test/BrokerOrderEventTest.cs` around lines 7 - 29, Add a new unit test that constructs BrokerOrderEvent using the constructor overload without Error and ErrorCode to verify default/null behavior: create an Order as in the existing test, instantiate var evt = new BrokerOrderEvent("c1", order); and assert evt.ClientOrderId equals "c1", evt.Order equals the order, evt.Success is true (or the expected default), and that evt.Error and evt.ErrorCode are null (or the expected defaults); place the test as a new [Fact] method (e.g., Constructor_DefaultArguments_ShouldHaveDefaults) alongside the existing Constructor_ShouldPopulateProperties test to lock down default argument behavior for BrokerOrderEvent.test/YoloApp.Test/Commands/RebalanceCommandTest.cs (1)
399-406: Minor: Inconsistent indentation in mock setup.The
It.IsAny<ITradeAdvisor>()andIt.IsAny<CancellationToken>()parameters have inconsistent indentation compared to the other parameters.♻️ Suggested fix for consistent indentation
var mockOrderManager = new Mock<IOrderManager>(); mockOrderManager.Setup(x => x.ManageOrdersAsync( It.IsAny<Trade[]>(), It.IsAny<OrderManagementSettings>(), - It.IsAny<ITradeAdvisor>(), - It.IsAny<CancellationToken>())) + It.IsAny<ITradeAdvisor>(), + It.IsAny<CancellationToken>())) .Returns(channel.Reader.ReadAllAsync());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/YoloApp.Test/Commands/RebalanceCommandTest.cs` around lines 399 - 406, Update the parameter indentation in the mock setup for mockOrderManager so all It.IsAny(...) arguments align consistently; specifically adjust the ManageOrdersAsync setup call (mockOrderManager.Setup(x => x.ManageOrdersAsync(...))) so It.IsAny<ITradeAdvisor>() and It.IsAny<CancellationToken>() use the same indentation level as the previous parameters to match the existing formatting convention.test/YoloBroker.Hyperliquid.Test/HyperliquidBrokerTest.cs (1)
347-395: Remove or skip commented-out tests instead of leaving dead code.These three
ManageOrdersAsynctests are commented out, leaving dead code in the test file. If the functionality has been removed, delete the tests. If they're temporarily disabled, use[Fact(Skip = "Reason")]to document why they're skipped.♻️ Option: Delete or use Skip attribute
Either delete the commented tests entirely, or if they need to be retained temporarily:
[Fact(Skip = "ManageOrdersAsync moved to OrderManager")] public async Task GivenNullTrades_WhenManageOrdersAsync_ShouldThrowArgumentNullException() { // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/YoloBroker.Hyperliquid.Test/HyperliquidBrokerTest.cs` around lines 347 - 395, Remove the dead commented tests for ManageOrdersAsync in HyperliquidBrokerTest.cs: either delete the three commented blocks (GivenNullTrades_WhenManageOrdersAsync_ShouldThrowArgumentNullException, GivenNullSettings_WhenManageOrdersAsync_ShouldThrowArgumentNullException, GivenEmptyTrades_WhenManageOrdersAsync_ShouldYieldNothing) or re-enable them as skipped tests by replacing the comment with valid [Fact(Skip = "reason")] attributes and a short reason (e.g., "ManageOrdersAsync moved to OrderManager" or "temporarily disabled"); ensure references to ManageOrdersAsync and OrderManagementSettings.Default remain correct and that the test method names are preserved when switching to Skip so history/traceability is clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vscode/launch.json:
- Line 47: The debug launch configuration removed the Azurite pre-launch task by
setting "preLaunchTask": "func: host start", which prevents local Durable
Functions (e.g., RebalanceDurableWorkflow, YoloDailyManualRebalance,
YoloDailyScheduledRebalance, UnravelDailyScheduledRebalance) from getting an
Azure Storage backend; restore or add a preLaunchTask that starts Azurite (or
document/auto-inject a valid AzureWebJobsStorage connection string into
local.settings.json) so the Functions host (func: host start) has storage
available before starting the debugger.
In @.vscode/settings.json:
- Line 2: The workspace setting
dotnetAcquisitionExtension.sharedExistingDotnetPath is hardcoded to a macOS
path; remove this key or make it platform-specific so other developers aren’t
broken: either delete "dotnetAcquisitionExtension.sharedExistingDotnetPath" from
.vscode/settings.json, or replace it with documented per-platform overrides (or
add a comment in README that this setting is macOS-only) and ensure any
references use the VS Code platform API or environment variables instead of a
fixed "/usr/local/share/dotnet/dotnet".
In `@src/YoloAbstractions/Config/YoloConfig.cs`:
- Line 15: YoloConfig exposes MaxRepriceRetries with no bounds so a negative
value can disable repricing; enforce validation during options binding by adding
a constraint that MaxRepriceRetries >= 0. Update the options registration (e.g.,
where AddOptions/Configure/Bind is called) to use
OptionsBuilder<YoloConfig>.Validate(...) or ValidateDataAnnotations() with a
[Range(0, int.MaxValue)] on the MaxRepriceRetries property so binding fails when
the value is negative; ensure the validation error message mentions
"MaxRepriceRetries" for clarity.
In `@src/YoloAbstractions/OrderManagementSettings.cs`:
- Line 3: OrderManagementSettings allows invalid values; add constructor-time
validation to enforce invariants: in the record OrderManagementSettings(TimeSpan
UnfilledOrderTimeout, int MaxRepriceRetries) add a primary-constructor body that
throws ArgumentOutOfRangeException (or ArgumentException) when
UnfilledOrderTimeout is <= TimeSpan.Zero or MaxRepriceRetries is < 0, so invalid
configurations fail fast at object creation.
In `@src/YoloBroker.Hyperliquid/HyperliquidBroker.cs`:
- Around line 788-823: The batch placement helpers PlaceSpotOrdersAsync and
PlaceFuturesOrdersAsync are still using trade.LimitPrice.GetValueOrDefault(),
causing market orders to be sent with price 0; update both to call and await
ResolveOrderPriceAsync(trade, ct) when building the HyperLiquidOrderRequest
price (or when trade.OrderType == OrderType.Market) and use its returned decimal
as the request Price; ensure you pass the same CancellationToken (ct) and
propagate or handle exceptions the same way PlaceTradeAsync does so batch
submissions mirror the single-order path.
- Around line 263-288: SubscribeOrderUpdatesAsync currently discards the
websocket subscription and leaks callbacks; change the CallAsync call to the
generic CallAsync<UpdateSubscription> overload to capture the returned
UpdateSubscription from
_hyperliquidSocketClient.FuturesApi.SubscribeToOrderUpdatesAsync, keep a
reference to that subscription, and ensure it is disposed/closed when the async
enumerable completes or when ct is cancelled (for example by converting
SubscribeOrderUpdatesAsync into an async iterator that yields from
tradeResultUpdateChannel.Reader.ReadAllAsync(ct) inside a try/finally and in
finally call subscription.DisposeAsync()/CloseAsync and complete the channel).
Also ensure HandleOrderStatusUpdates continues to write to
tradeResultUpdateChannel while the subscription is active and the channel is
closed only after disposing the subscription.
In `@src/YoloBroker/OrderManager.cs`:
- Around line 35-60: The subscription pump can continue writing after the caller
closes the channel, so wrap the pump lifecycle in a try/finally: create
subscriptionCts and start subscriptionPump as before, but in the finally block
call subscriptionCts.Cancel() (or CancelAsync if available), await
subscriptionPump to finish, then call
eventChannel.Writer.TryComplete()/Complete(); ensure cancellation is propagated
to orderUpdates enumeration and that you await subscriptionPump before
completing the channel to avoid ChannelClosedException from ManagerEvent.Broker
writes.
- Around line 132-140: The code removes the tracker and returns null when
tracker.IsCompleted() is true, preventing terminal updates from being sent;
change the flow so you create the terminal OrderUpdate (using new
OrderUpdate(trade.Symbol, GetOrderUpdateType(order), order)) before removing the
tracker, then call pending.TryRemove(evt.ClientOrderId, out _) and finally
return that OrderUpdate; keep the existing behavior for non-terminal cases
(return the update directly after tracker.AddOrder(order)).
- Around line 156-198: The replacement is being based on tracker.Trade (original
size) which can re-submit an already-filled amount; before placing the
replacement from advisor.GetReplacementTradeAsync(tracker.Trade, ct) compute the
remaining amount (e.g. remaining = tracker.Trade.Quantity -
tracker.CurrentOrder.FilledQuantity or tracker.FilledAmount depending on your
tracker model), and if remaining <= 0 treat the order as completed (log,
pending.TryRemove(...) and return a TimedOut/Canceled OrderUpdate) instead of
placing a replacement; if remaining > 0 adjust the replacement's size/quantity
to remaining (and any related price/flags) before calling
_broker.PlaceTradeAsync(replacement, ct). Ensure you reference
tracker.CurrentOrder, tracker.Trade, advisor.GetReplacementTradeAsync and
_broker.PlaceTradeAsync when applying the fix.
In `@src/YoloFunk/appsettings.json`:
- Line 29: The appsettings.json currently has "UseTestnet": true for the
strategies; confirm whether this is intentional for development (YoloDaily and
UnravelDaily) and if not, change the UseTestnet flag to false for those strategy
entries (YoloDaily, UnravelDaily) before merging to master so live trading uses
mainnet; if the true value is required for CI/test branches, document the reason
in the PR description or switch to an environment-specific override instead.
In `@src/YoloFunk/Functions/EffectiveWeightsFunctionBase.cs`:
- Around line 104-106: The factors creation in EffectiveWeightsFunctionBase
(where rawWeights is converted using kvp.Key.GetBaseAndQuoteAssets().BaseAsset)
collapses different raw symbols that normalize to the same BaseAsset (e.g.,
"BTC" and "BTC/USDC"); before materializing factors, detect duplicates by
grouping rawWeights by kvp.Key.GetBaseAndQuoteAssets().BaseAsset and either (a)
aggregate their weights (e.g., sum or normalized combination) into a single
Weight per BaseAsset or (b) throw a clear, handled exception that lists
conflicting symbols so the caller can correct input; then build factors from the
grouped result (use the grouped key as BaseAsset and set IsInUniverse
accordingly). Ensure the chosen behavior is implemented where factors is created
and that the error path returns a meaningful message rather than a generic 500.
In `@src/YoloKonsole/Program.cs`:
- Line 86: Program.cs is resolving IOrderManager with
serviceProvider.GetService<IOrderManager>()! but IOrderManager is not registered
in the console app DI (it is only registered as a keyed singleton in
AddStrategyServices.cs), so fix by either registering the required
implementation in the console app's IServiceCollection (add the same
registration used in AddStrategyServices for IOrderManager) or, if the service
must be required, replace GetService<IOrderManager>()! with
serviceProvider.GetRequiredService<IOrderManager>() to fail fast and surface a
clear diagnostic; update the Program.cs resolution accordingly and ensure the
registration matches the expected keyed singleton configuration if you choose to
register it.
In `@test/YoloBroker.Test/OrderManagerTest.cs`:
- Around line 276-289: The test sets subscriptionReady before returning the
async enumerable which only verifies call order; change the mock for
SubscribeOrderUpdatesAsync to return an IAsyncEnumerable that sets
subscriptionReady when the enumerable is actually consumed (inside its async
iterator) instead of before returning, e.g. replace the Thread.Sleep +
pre-return flag set with an async iterator returned by
SubscribeOrderUpdatesAsync so the flag is set on first MoveNext/await (refer to
SubscribeOrderUpdatesAsync, EmptyEventsAsync and ManageOrdersAsync) and keep
PlaceTradesAsync assertion unchanged so it verifies consumption timing.
In `@test/YoloFunk.Test/Extensions/AddStrategyServicesTest.cs`:
- Around line 55-67: The test
AddStrategy_WhenHyperliquidConfigMissing_ShouldThrow currently uses
Should.Throw<Exception> which can hide unrelated failures; update the assertion
on services.AddStrategy (the AddStrategy extension invocation) to assert the
specific failure by either expecting a concrete exception type (e.g.,
Should.Throw<InvalidOperationException> or the actual type thrown by
AddStrategy) or at minimum assert the thrown exception's Message contains the
"Hyperliquid" fragment so the test pins the Hyperliquid-config validation (e.g.,
use Should.Throw<Exception>(... ).Message.ShouldContain("Hyperliquid")).
---
Nitpick comments:
In `@test/YoloAbstractions.Test/BrokerOrderEventTest.cs`:
- Around line 7-29: Add a new unit test that constructs BrokerOrderEvent using
the constructor overload without Error and ErrorCode to verify default/null
behavior: create an Order as in the existing test, instantiate var evt = new
BrokerOrderEvent("c1", order); and assert evt.ClientOrderId equals "c1",
evt.Order equals the order, evt.Success is true (or the expected default), and
that evt.Error and evt.ErrorCode are null (or the expected defaults); place the
test as a new [Fact] method (e.g.,
Constructor_DefaultArguments_ShouldHaveDefaults) alongside the existing
Constructor_ShouldPopulateProperties test to lock down default argument behavior
for BrokerOrderEvent.
In `@test/YoloAbstractions.Test/TradeTest.cs`:
- Around line 59-66: The test constructs a Trade with a supplied limit price but
uses OrderType.Market; change the order type to OrderType.Limit in the Trade
constructor call (the new Trade(...) invocation) so the test explicitly
exercises price-based minimum order value logic, or alternatively add a second
test case that uses OrderType.Limit in addition to the existing Market case to
cover both variants.
In `@test/YoloApp.Test/Commands/RebalanceCommandTest.cs`:
- Around line 399-406: Update the parameter indentation in the mock setup for
mockOrderManager so all It.IsAny(...) arguments align consistently; specifically
adjust the ManageOrdersAsync setup call (mockOrderManager.Setup(x =>
x.ManageOrdersAsync(...))) so It.IsAny<ITradeAdvisor>() and
It.IsAny<CancellationToken>() use the same indentation level as the previous
parameters to match the existing formatting convention.
In `@test/YoloBroker.Hyperliquid.Test/HyperliquidBrokerTest.cs`:
- Around line 347-395: Remove the dead commented tests for ManageOrdersAsync in
HyperliquidBrokerTest.cs: either delete the three commented blocks
(GivenNullTrades_WhenManageOrdersAsync_ShouldThrowArgumentNullException,
GivenNullSettings_WhenManageOrdersAsync_ShouldThrowArgumentNullException,
GivenEmptyTrades_WhenManageOrdersAsync_ShouldYieldNothing) or re-enable them as
skipped tests by replacing the comment with valid [Fact(Skip = "reason")]
attributes and a short reason (e.g., "ManageOrdersAsync moved to OrderManager"
or "temporarily disabled"); ensure references to ManageOrdersAsync and
OrderManagementSettings.Default remain correct and that the test method names
are preserved when switching to Skip so history/traceability is clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81449ef9-9c38-49e6-84a1-4a2b405b05fe
📒 Files selected for processing (39)
.vscode/launch.json.vscode/settings.json.vscode/tasks.jsonDirectory.Packages.propsREADME.mdsrc/YoloAbstractions/BrokerAccountContext.cssrc/YoloAbstractions/BrokerOrderEvent.cssrc/YoloAbstractions/Config/YoloConfig.cssrc/YoloAbstractions/Interfaces/ITradeAdvisor.cssrc/YoloAbstractions/OrderManagementSettings.cssrc/YoloAbstractions/Trade.cssrc/YoloApp/Commands/RebalanceCommand.cssrc/YoloBroker.Hyperliquid/HyperliquidBroker.cssrc/YoloBroker/Exceptions/BrokerException.cssrc/YoloBroker/Interface/IOrderManager.cssrc/YoloBroker/Interface/IYoloBroker.cssrc/YoloBroker/OrderManager.cssrc/YoloBroker/YoloBroker.csprojsrc/YoloFunk/Dto/EffectiveWeightsResponse.cssrc/YoloFunk/Extensions/AddStrategyServices.cssrc/YoloFunk/Functions/EffectiveWeightsFunctionBase.cssrc/YoloFunk/Functions/UnravelDailyEffectiveWeights.cssrc/YoloFunk/Functions/YoloDailyEffectiveWeights.cssrc/YoloFunk/appsettings.jsonsrc/YoloKonsole/Extensions/BrokerServiceCollectionExtensions.cssrc/YoloKonsole/Program.cssrc/YoloKonsole/appsettings.jsonsrc/YoloTrades/TradeAdvisor.cstest/YoloAbstractions.Test/BrokerOrderEventTest.cstest/YoloAbstractions.Test/TradeTest.cstest/YoloApp.Test/Commands/RebalanceCommandTest.cstest/YoloBroker.Hyperliquid.Test/HyperliquidBrokerIntegrationTest.cstest/YoloBroker.Hyperliquid.Test/HyperliquidBrokerTest.cstest/YoloBroker.Test/OrderManagerTest.cstest/YoloFunk.Test/Dto/EffectiveWeightsResponseTest.cstest/YoloFunk.Test/Extensions/AddStrategyServicesTest.cstest/YoloFunk.Test/Functions/EffectiveWeightsEndpointsTest.cstest/YoloFunk.Test/Functions/EffectiveWeightsFunctionBaseTest.cstest/YoloTrades.Test/TradeAdvisorTest.cs
💤 Files with no reviewable changes (1)
- src/YoloKonsole/Extensions/BrokerServiceCollectionExtensions.cs
🚀 Deployed to Azure FunctionsEnvironment: This environment will be automatically cleaned up when the PR is closed. |
🚀 Deployed to Azure FunctionsEnvironment: This environment will be automatically cleaned up when the PR is closed. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
src/YoloBroker/OrderManager.cs (3)
161-184:⚠️ Potential issue | 🔴 CriticalResize the retry to the unfilled remainder.
GetReplacementTradeAsynconly getstracker.Trade, so after a partial fill the advisor has no guaranteed way to knowCurrentOrder.Filled. Ifreplacementkeeps the original size, the retry can overshoot the target position. Compute the remaining signed amount fromtracker.CurrentOrderand override the replacement quantity beforePlaceTradeAsync.Also applies to: 200-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/YoloBroker/OrderManager.cs` around lines 161 - 184, The advisor's replacement trade from GetReplacementTradeAsync(tracker.Trade, ct) may still carry the original size and can overshoot after partial fills; compute the remaining signed amount using tracker.CurrentOrder (taking into account CurrentOrder.Filled vs original quantity and its side) and override replacement.Quantity (or the equivalent size field on the replacement trade) before calling PlaceTradeAsync; also preserve replacement.ClientOrderId = tracker.Trade.ClientOrderId as already done and ensure the adjusted replacement is logged/used when invoking PlaceTradeAsync so the retry uses the unfilled remainder.
134-142:⚠️ Potential issue | 🟠 MajorReturn the terminal broker update before removing the tracker.
After
tracker.AddOrder(order), a terminal broker event currently returnsnull, so callers never see the finalFilled/Cancelled/Errorstate. Build theOrderUpdate, remove the tracker, and still return that update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/YoloBroker/OrderManager.cs` around lines 134 - 142, When tracker.AddOrder(order) results in a terminal state (tracker.IsCompleted()), construct the OrderUpdate (using new OrderUpdate(trade.Symbol, GetOrderUpdateType(order), order)) before removing the tracker so callers receive the final Filled/Cancelled/Error update; then call pending.TryRemove(evt.ClientOrderId, out _) and return that OrderUpdate instead of returning null. Ensure you still remove the tracker and return the built update.
40-62:⚠️ Potential issue | 🟠 MajorShut down the pump and timeout writers from
finally, before completing the channel.Cleanup only runs on the happy path,
TryComplete()happens before the subscription pump is stopped, and the timeout tasks launched byStartTimeoutare never cancelled or awaited. An exception or normal exit can leave background writers targeting a completed channel or running after the reader is gone.Also applies to: 85-85, 97-116, 208-208, 271-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/YoloBroker/OrderManager.cs` around lines 40 - 62, The cleanup must cancel and await background writers before completing the channel: in the finally block, call subscriptionCts.Cancel(), await the subscriptionPump Task (subscriptionPump), cancel/await any timeout tasks started by StartTimeout (ensure you track and cancel those tokens/tasks), and only after all background writer tasks have finished do the eventChannel.Writer.TryComplete()/Complete() call; adjust the code paths where TryComplete() is currently called (including the areas around StartTimeout usages and other exits) so that TryComplete() happens last to avoid writers targeting a completed channel.
🧹 Nitpick comments (3)
test/YoloFunk.Test/Extensions/AddStrategyServicesTest.cs (1)
16-24: Consider extracting a config-builder helper to reduce repetition.The repeated in-memory config setup is clear but duplicated; a small helper would make future test additions easier to maintain.
♻️ Optional refactor sketch
+private static IConfiguration BuildConfig(Dictionary<string, string?> values) => + new ConfigurationBuilder() + .AddInMemoryCollection(values) + .Build(); - var config = new ConfigurationBuilder() - .AddInMemoryCollection(new Dictionary<string, string?> - { - ... - }) - .Build(); + var config = BuildConfig(new Dictionary<string, string?> + { + ... + });Also applies to: 34-43, 58-64, 74-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/YoloFunk.Test/Extensions/AddStrategyServicesTest.cs` around lines 16 - 24, Extract the repeated in-memory configuration setup in AddStrategyServicesTest into a small helper (e.g., a private static method like BuildTestConfiguration or CreateInMemoryConfig) that wraps ConfigurationBuilder().AddInMemoryCollection(...).Build(), and call that helper from the existing test cases instead of duplicating the Dictionary and ConfigurationBuilder logic; locate usages by searching for ConfigurationBuilder, AddInMemoryCollection, and the keys like "Strategies:Test:Hyperliquid:Address"/"Strategies:Test:Hyperliquid:PrivateKey"/"Strategies:Test:Hyperliquid:UseTestnet" and replace the inline setup with the new helper (optionally accept a Dictionary or typed parameters to keep tests readable).test/YoloFunk.Test/Functions/EffectiveWeightsFunctionBaseTest.cs (1)
57-191: Add a regression test for mixed-case symbol keys (solvsSOL).Current coverage validates duplicate-normalization errors, but not the case-mismatch path between
positionsand weight symbols. A focused test here would prevent future regressions in dropped-token handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/YoloFunk.Test/Functions/EffectiveWeightsFunctionBaseTest.cs` around lines 57 - 191, Add a new unit test in EffectiveWeightsFunctionBaseTest.cs that simulates mixed-case symbol keys: mock IYoloBroker.GetPositionsAsync to return positions under the lower-case key "sol" while ICalcWeights.CalculateWeightsAsync returns weights with the upper-case key "SOL"; build services and YoloConfig like the existing GivenValidStrategyServices_WhenRun_ShouldReturnEffectiveWeights test, invoke EffectiveWeightsFunctionHarness.Run, and assert an OK response and that the EffectiveWeightsResponse contains the expected token ("SOL"), strategy, address and nominal values to ensure mixed-case normalization doesn't drop the token.test/YoloBroker.Test/OrderManagerTest.cs (1)
30-32: These timeout-based assertions will be flaky on busy CI agents.The suite depends on 20ms wall-clock delays and 250/500ms cancellation windows to drive the timeout path. A controllable time source/timeout hook, or explicit
TaskCompletionSourcecoordination, would make these tests deterministic.Also applies to: 65-70, 100-104, 131-136, 171-176, 224-229, 297-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/YoloBroker.Test/OrderManagerTest.cs` around lines 30 - 32, The tests use fragile wall-clock waits (UnfilledOrderTimeout set to 20ms and fixed cancellation windows) which cause flakiness; update the tests to use a deterministic time/control mechanism instead: either modify the system under test to accept an injectable time provider/clock (introduce an ITimeProvider or IClock and wire it into OrderManager / OrderManagementSettings so you can advance time deterministically in tests) or replace the sleep-based assertions with TaskCompletionSource coordination (have the test await a TCS that the test controls and trigger the timeout path by completing/advancing the injected clock). Target references: OrderManagementSettings and its UnfilledOrderTimeout usage, and the OrderManagerTest test methods that rely on 20ms waits and the 250/500ms cancellation windows; make the minimal change to allow deterministic advancement of time or explicit signaling in those tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/YoloAbstractions/Extensions/YoloConfigExtensions.cs`:
- Line 23: The Ensure predicate currently uses TimeSpan.Parse which throws on
malformed UnfilledOrderTimeout; change it to use TimeSpan.TryParse and validate
the parsed value is > TimeSpan.Zero (e.g., replace the predicate with
TimeSpan.TryParse(c.UnfilledOrderTimeout, out var t) && t > TimeSpan.Zero) so
invalid config entries cause the Ensure predicate to fail instead of throwing;
update the lambda in YoloConfigExtensions (the Ensure call referencing
UnfilledOrderTimeout) accordingly.
In `@src/YoloBroker/OrderManager.cs`:
- Around line 122-142: The code currently reuses a timed-out/canceled order's
ClientOrderId for its replacement which causes late cancel/fill events to be
applied to the old tracker and removes the live replacement from the pending
map; when creating a replacement for a timed-out order (the logic that currently
overwrites evt.ClientOrderId), generate/assign a new ClientOrderId for the
replacement and register that new id in the pending dictionary (or alternatively
correlate incoming updates by the immutable Order.Id rather than ClientOrderId).
Update tracker.AddOrder and IsCompleted to validate Order.Id (or ensure the
tracker is keyed by the active replacement's ClientOrderId) so that late events
for the old terminal order do not mark the replacement completed; ensure
pending.TryRemove(evt.ClientOrderId, out _) only runs for the exact active
ClientOrderId associated with the tracker.
In `@src/YoloFunk/Functions/EffectiveWeightsFunctionBase.cs`:
- Around line 151-155: The Except/Union operations on positions.Keys and
factors.Keys are currently case-sensitive, causing tokens like "sol" vs "SOL" to
be treated as different; fix by using a case-insensitive comparison for these
set operations (e.g., pass StringComparer.OrdinalIgnoreCase as the
IEqualityComparer to Union and Except or normalize keys before comparing) so
droppedTokens correctly detects tokens regardless of case—apply this change
around the droppedTokens computation that references positions, factors and
yoloConfig.BaseAsset in EffectiveWeightsFunctionBase (the droppedTokens variable
and related set ops).
- Around line 147-150: Protect the weightConstraint calculation from
divide-by-zero and negative MaxLeverage by validating and clamping inputs:
compute a non-negative safeMax = Math.Max(0, yoloConfig.MaxLeverage) and if
safeMax == 0 or unconstrainedTargetLeverage <= 0 set weightConstraint = 1 (no
constraint); otherwise compute weightConstraint = Math.Min(1, safeMax /
unconstrainedTargetLeverage) and then clamp to [0,1] to avoid negative or >1
results. Update the code around the weightConstraint assignment that references
unconstrainedTargetLeverage and yoloConfig.MaxLeverage accordingly.
---
Duplicate comments:
In `@src/YoloBroker/OrderManager.cs`:
- Around line 161-184: The advisor's replacement trade from
GetReplacementTradeAsync(tracker.Trade, ct) may still carry the original size
and can overshoot after partial fills; compute the remaining signed amount using
tracker.CurrentOrder (taking into account CurrentOrder.Filled vs original
quantity and its side) and override replacement.Quantity (or the equivalent size
field on the replacement trade) before calling PlaceTradeAsync; also preserve
replacement.ClientOrderId = tracker.Trade.ClientOrderId as already done and
ensure the adjusted replacement is logged/used when invoking PlaceTradeAsync so
the retry uses the unfilled remainder.
- Around line 134-142: When tracker.AddOrder(order) results in a terminal state
(tracker.IsCompleted()), construct the OrderUpdate (using new
OrderUpdate(trade.Symbol, GetOrderUpdateType(order), order)) before removing the
tracker so callers receive the final Filled/Cancelled/Error update; then call
pending.TryRemove(evt.ClientOrderId, out _) and return that OrderUpdate instead
of returning null. Ensure you still remove the tracker and return the built
update.
- Around line 40-62: The cleanup must cancel and await background writers before
completing the channel: in the finally block, call subscriptionCts.Cancel(),
await the subscriptionPump Task (subscriptionPump), cancel/await any timeout
tasks started by StartTimeout (ensure you track and cancel those tokens/tasks),
and only after all background writer tasks have finished do the
eventChannel.Writer.TryComplete()/Complete() call; adjust the code paths where
TryComplete() is currently called (including the areas around StartTimeout
usages and other exits) so that TryComplete() happens last to avoid writers
targeting a completed channel.
---
Nitpick comments:
In `@test/YoloBroker.Test/OrderManagerTest.cs`:
- Around line 30-32: The tests use fragile wall-clock waits
(UnfilledOrderTimeout set to 20ms and fixed cancellation windows) which cause
flakiness; update the tests to use a deterministic time/control mechanism
instead: either modify the system under test to accept an injectable time
provider/clock (introduce an ITimeProvider or IClock and wire it into
OrderManager / OrderManagementSettings so you can advance time deterministically
in tests) or replace the sleep-based assertions with TaskCompletionSource
coordination (have the test await a TCS that the test controls and trigger the
timeout path by completing/advancing the injected clock). Target references:
OrderManagementSettings and its UnfilledOrderTimeout usage, and the
OrderManagerTest test methods that rely on 20ms waits and the 250/500ms
cancellation windows; make the minimal change to allow deterministic advancement
of time or explicit signaling in those tests.
In `@test/YoloFunk.Test/Extensions/AddStrategyServicesTest.cs`:
- Around line 16-24: Extract the repeated in-memory configuration setup in
AddStrategyServicesTest into a small helper (e.g., a private static method like
BuildTestConfiguration or CreateInMemoryConfig) that wraps
ConfigurationBuilder().AddInMemoryCollection(...).Build(), and call that helper
from the existing test cases instead of duplicating the Dictionary and
ConfigurationBuilder logic; locate usages by searching for ConfigurationBuilder,
AddInMemoryCollection, and the keys like
"Strategies:Test:Hyperliquid:Address"/"Strategies:Test:Hyperliquid:PrivateKey"/"Strategies:Test:Hyperliquid:UseTestnet"
and replace the inline setup with the new helper (optionally accept a Dictionary
or typed parameters to keep tests readable).
In `@test/YoloFunk.Test/Functions/EffectiveWeightsFunctionBaseTest.cs`:
- Around line 57-191: Add a new unit test in EffectiveWeightsFunctionBaseTest.cs
that simulates mixed-case symbol keys: mock IYoloBroker.GetPositionsAsync to
return positions under the lower-case key "sol" while
ICalcWeights.CalculateWeightsAsync returns weights with the upper-case key
"SOL"; build services and YoloConfig like the existing
GivenValidStrategyServices_WhenRun_ShouldReturnEffectiveWeights test, invoke
EffectiveWeightsFunctionHarness.Run, and assert an OK response and that the
EffectiveWeightsResponse contains the expected token ("SOL"), strategy, address
and nominal values to ensure mixed-case normalization doesn't drop the token.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aba1eac6-8e9f-47b0-85ba-d8153b3d15d3
📒 Files selected for processing (9)
src/YoloAbstractions/Extensions/YoloConfigExtensions.cssrc/YoloBroker.Hyperliquid/HyperliquidBroker.cssrc/YoloBroker/OrderManager.cssrc/YoloFunk/Functions/EffectiveWeightsFunctionBase.cssrc/YoloKonsole/Extensions/BrokerServiceCollectionExtensions.cssrc/YoloKonsole/Program.cstest/YoloBroker.Test/OrderManagerTest.cstest/YoloFunk.Test/Extensions/AddStrategyServicesTest.cstest/YoloFunk.Test/Functions/EffectiveWeightsFunctionBaseTest.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/YoloKonsole/Extensions/BrokerServiceCollectionExtensions.cs
- src/YoloKonsole/Program.cs
- src/YoloBroker.Hyperliquid/HyperliquidBroker.cs
| ?.Ensure(c => c.NominalCash > 0) | ||
| ?.Ensure(c => c.SpreadSplit >= 0 && c.SpreadSplit <= 1) | ||
| ?.Ensure(c => c.TradeBuffer >= 0) | ||
| ?.Ensure(c => TimeSpan.Parse(c.UnfilledOrderTimeout) > TimeSpan.Zero); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In .NET, what happens when TimeSpan.Parse receives an invalid string, and how does that differ from TimeSpan.TryParse?
💡 Result:
When TimeSpan.Parse receives an invalid string in .NET, it throws a FormatException if the string has an invalid format, an OverflowException if the value is outside TimeSpan.MinValue/MaxValue or components are out of range, or ArgumentNullException if the input is null. In contrast, TimeSpan.TryParse does not throw exceptions on invalid input; it returns false and sets the out TimeSpan parameter to TimeSpan.Zero, indicating parsing failure without exceptions.
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.timespan.parse?view=net-7.0
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.timespan.parse?view=net-10.0
- 3: https://learn.microsoft.com/en-us/dotnet/api/system.timespan.tryparse?redirectedfrom=MSDN&view=net-7.0
- 4: https://learn.microsoft.com/en-us/dotnet/api/system.timespan.tryparse?view=net-10.0
- 5: https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-timespan-tryparse
🏁 Script executed:
cd src/YoloAbstractions/Extensions && cat -n YoloConfigExtensions.cs | head -40Repository: moconnell/yolo
Length of output: 1206
Use TryParse so invalid timeout config fails validation instead of throwing.
TimeSpan.Parse will throw on malformed UnfilledOrderTimeout, which breaks this Ensure chain instead of rejecting the config like the other predicates do.
Suggested fix
- ?.Ensure(c => TimeSpan.Parse(c.UnfilledOrderTimeout) > TimeSpan.Zero);
+ ?.Ensure(c => TimeSpan.TryParse(c.UnfilledOrderTimeout, out var timeout) && timeout > TimeSpan.Zero);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ?.Ensure(c => TimeSpan.Parse(c.UnfilledOrderTimeout) > TimeSpan.Zero); | |
| ?.Ensure(c => TimeSpan.TryParse(c.UnfilledOrderTimeout, out var timeout) && timeout > TimeSpan.Zero); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/YoloAbstractions/Extensions/YoloConfigExtensions.cs` at line 23, The
Ensure predicate currently uses TimeSpan.Parse which throws on malformed
UnfilledOrderTimeout; change it to use TimeSpan.TryParse and validate the parsed
value is > TimeSpan.Zero (e.g., replace the predicate with
TimeSpan.TryParse(c.UnfilledOrderTimeout, out var t) && t > TimeSpan.Zero) so
invalid config entries cause the Ensure predicate to fail instead of throwing;
update the lambda in YoloConfigExtensions (the Ensure call referencing
UnfilledOrderTimeout) accordingly.
Comment on lines
+122
to
+142
| if (!pending.TryGetValue(evt.ClientOrderId, out var tracker)) | ||
| { | ||
| _logger.LogWarning("Received order update for unknown ClientOrderId {clientOrderId}", evt.ClientOrderId); | ||
| return null; | ||
| } | ||
|
|
||
| var order = evt.Order; | ||
| var trade = tracker.Trade; | ||
|
|
||
| if (order == null) | ||
| return null; | ||
|
|
||
| tracker.AddOrder(order); | ||
|
|
||
| if (tracker.IsCompleted()) | ||
| { | ||
| pending.TryRemove(evt.ClientOrderId, out _); | ||
| return null; | ||
| } | ||
|
|
||
| return new OrderUpdate(trade.Symbol, GetOrderUpdateType(order), order); |
There was a problem hiding this comment.
Don't force the replacement to reuse the canceled order's ClientOrderId.
Because replacement is overwritten with the original ClientOrderId, late cancel/fill events for the timed-out order are indistinguishable from updates for its replacement. HandleBrokerUpdate will then push that old terminal order, IsCompleted() flips true, and the live replacement drops out of pending. Keep a fresh ClientOrderId per replacement or additionally correlate updates to the active Order.Id.
Also applies to: 183-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/YoloBroker/OrderManager.cs` around lines 122 - 142, The code currently
reuses a timed-out/canceled order's ClientOrderId for its replacement which
causes late cancel/fill events to be applied to the old tracker and removes the
live replacement from the pending map; when creating a replacement for a
timed-out order (the logic that currently overwrites evt.ClientOrderId),
generate/assign a new ClientOrderId for the replacement and register that new id
in the pending dictionary (or alternatively correlate incoming updates by the
immutable Order.Id rather than ClientOrderId). Update tracker.AddOrder and
IsCompleted to validate Order.Id (or ensure the tracker is keyed by the active
replacement's ClientOrderId) so that late events for the old terminal order do
not mark the replacement completed; ensure pending.TryRemove(evt.ClientOrderId,
out _) only runs for the exact active ClientOrderId associated with the tracker.
Comment on lines
+147
to
+150
| var weightConstraint = unconstrainedTargetLeverage < yoloConfig.MaxLeverage | ||
| ? 1 | ||
| : yoloConfig.MaxLeverage / unconstrainedTargetLeverage; | ||
|
|
There was a problem hiding this comment.
Harden weight-constraint math for zero/invalid leverage inputs.
If unconstrained leverage is 0 and MaxLeverage <= 0, this can divide by zero. Negative MaxLeverage also yields negative constraints, which can invert target signs.
Proposed fix
- var weightConstraint = unconstrainedTargetLeverage < yoloConfig.MaxLeverage
- ? 1
- : yoloConfig.MaxLeverage / unconstrainedTargetLeverage;
+ var maxLeverage = Math.Max(0m, yoloConfig.MaxLeverage);
+ var weightConstraint = unconstrainedTargetLeverage == 0m
+ ? 1m
+ : Math.Min(1m, maxLeverage / unconstrainedTargetLeverage);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var weightConstraint = unconstrainedTargetLeverage < yoloConfig.MaxLeverage | |
| ? 1 | |
| : yoloConfig.MaxLeverage / unconstrainedTargetLeverage; | |
| var maxLeverage = Math.Max(0m, yoloConfig.MaxLeverage); | |
| var weightConstraint = unconstrainedTargetLeverage == 0m | |
| ? 1m | |
| : Math.Min(1m, maxLeverage / unconstrainedTargetLeverage); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/YoloFunk/Functions/EffectiveWeightsFunctionBase.cs` around lines 147 -
150, Protect the weightConstraint calculation from divide-by-zero and negative
MaxLeverage by validating and clamping inputs: compute a non-negative safeMax =
Math.Max(0, yoloConfig.MaxLeverage) and if safeMax == 0 or
unconstrainedTargetLeverage <= 0 set weightConstraint = 1 (no constraint);
otherwise compute weightConstraint = Math.Min(1, safeMax /
unconstrainedTargetLeverage) and then clamp to [0,1] to avoid negative or >1
results. Update the code around the weightConstraint assignment that references
unconstrainedTargetLeverage and yoloConfig.MaxLeverage accordingly.
Comment on lines
+151
to
+155
| var droppedTokens = positions.Keys.Except(factors.Keys.Union([yoloConfig.BaseAsset])); | ||
| foreach (var token in droppedTokens) | ||
| { | ||
| factors[token] = (0m, false); | ||
| } |
There was a problem hiding this comment.
Use case-insensitive set operations for dropped-token detection.
Except/Union here are case-sensitive. If positions contain sol while factors contain SOL, this path marks it as dropped, and Line 154 overwrites the existing factor entry to (0m, false) because factors is case-insensitive.
Proposed fix
- var droppedTokens = positions.Keys.Except(factors.Keys.Union([yoloConfig.BaseAsset]));
+ var knownTokens = factors.Keys.Union([yoloConfig.BaseAsset], StringComparer.OrdinalIgnoreCase);
+ var droppedTokens = positions.Keys.Except(knownTokens, StringComparer.OrdinalIgnoreCase);
foreach (var token in droppedTokens)
{
factors[token] = (0m, false);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/YoloFunk/Functions/EffectiveWeightsFunctionBase.cs` around lines 151 -
155, The Except/Union operations on positions.Keys and factors.Keys are
currently case-sensitive, causing tokens like "sol" vs "SOL" to be treated as
different; fix by using a case-insensitive comparison for these set operations
(e.g., pass StringComparer.OrdinalIgnoreCase as the IEqualityComparer to Union
and Except or normalize keys before comparing) so droppedTokens correctly
detects tokens regardless of case—apply this change around the droppedTokens
computation that references positions, factors and yoloConfig.BaseAsset in
EffectiveWeightsFunctionBase (the droppedTokens variable and related set ops).
🧹 Environment Cleaned UpThe ephemeral Azure Functions environment All associated resources (Function App, App Service Plan, Storage Account) have been removed. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Configuration Changes