fixes for rabbit #201
fixes for rabbit #201
Conversation
📝 WalkthroughWalkthroughThis PR removes the TradeMonitor GenServer-based real-time trade monitoring system, including its configuration, CLI interface, and supervision tree setup. Monitoring functionality is transitioned to Oban-based workers. Minor refactoring enhances component flexibility and defensive type handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
holden
force-pushed
the
01-29-fixes_for_rabbit
branch
from
ed922e5 to
3b1058f
Compare
holden
force-pushed
the
01-29-fixes_for_rabbit
branch
from
3b1058f to
856c43d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/polymarket/OPERATOR_RUNBOOK.md (1)
40-70: Remove obsoletemix polymarket.monitorcommands/flags.The TradeMonitor task was removed, but the runbook still lists
mix polymarket.monitorand its flags (enable/disable/poll and threshold tuning). Please replace these with the Oban-based flow and the currentmix polymarket.ingest/ config guidance to avoid operator confusion.Also applies to: 294-300, 404-407
🤖 Fix all issues with AI agents
In `@lib/volfefe_machine/polymarket.ex`:
- Around line 4664-4665: The monitor_status map in polymarket.ex is hard-coded
to enabled: true; change it to reflect the real runtime setting by querying the
app config or existing helper that indicates whether alerting/AlertingWorker is
enabled (e.g., Application.get_env/3 key or a function like alerting_enabled?/0)
and use that boolean for monitor_status[:enabled]; update any related via value
if you rely on Oban workers being present. Ensure you reference the
monitor_status map in polymarket.ex and the AlertingWorker/alerting config when
implementing the replacement.
In `@lib/volfefe_machine/workers/polymarket/alerting_worker.ex`:
- Around line 110-118: In find_alert_candidates, make min_score normalization
robust: if min_score is nil return a safe default Decimal (e.g.,
Decimal.new(0)); if min_score is already a %Decimal{} return it unchanged; for
floats and integers keep Decimal.from_float/Decimal.new; for binaries use
Decimal.parse/1 and handle {:ok, dec} | :error (fall back to default Decimal on
:error); remove the catch-all Decimal.new(min_score) that can crash on
unsupported types and ensure any other unexpected type falls back to the safe
default Decimal.
| # Monitoring is handled by Oban workers (TradeIngestionWorker, TradeScoringWorker, AlertingWorker) | ||
| monitor_status: %{enabled: true, via: "oban_workers"} |
There was a problem hiding this comment.
Don’t hard-code monitoring as enabled in the dashboard.
monitor_status now always shows enabled: true, which can mislead ops if AlertingWorker is disabled in config. Consider reflecting the actual config value so the UI matches runtime state.
🛠️ Suggested adjustment
- monitor_status: %{enabled: true, via: "oban_workers"}
+ monitor_status: %{
+ enabled:
+ Application.get_env(:volfefe_machine, VolfefeMachine.Workers.Polymarket.AlertingWorker, [])
+ |> Keyword.get(:enabled, true),
+ via: "oban_workers"
+ }📝 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.
| # Monitoring is handled by Oban workers (TradeIngestionWorker, TradeScoringWorker, AlertingWorker) | |
| monitor_status: %{enabled: true, via: "oban_workers"} | |
| # Monitoring is handled by Oban workers (TradeIngestionWorker, TradeScoringWorker, AlertingWorker) | |
| monitor_status: %{ | |
| enabled: | |
| Application.get_env(:volfefe_machine, VolfefeMachine.Workers.Polymarket.AlertingWorker, []) | |
| |> Keyword.get(:enabled, true), | |
| via: "oban_workers" | |
| } |
🤖 Prompt for AI Agents
In `@lib/volfefe_machine/polymarket.ex` around lines 4664 - 4665, The
monitor_status map in polymarket.ex is hard-coded to enabled: true; change it to
reflect the real runtime setting by querying the app config or existing helper
that indicates whether alerting/AlertingWorker is enabled (e.g.,
Application.get_env/3 key or a function like alerting_enabled?/0) and use that
boolean for monitor_status[:enabled]; update any related via value if you rely
on Oban workers being present. Ensure you reference the monitor_status map in
polymarket.ex and the AlertingWorker/alerting config when implementing the
replacement.
| def find_alert_candidates(min_score, limit \\ 100) do | ||
| # Convert to decimal for comparison | ||
| min_score_decimal = Decimal.from_float(min_score) | ||
| # Defensively normalize min_score to Decimal (handles floats, integers, strings) | ||
| min_score_decimal = | ||
| cond do | ||
| is_float(min_score) -> Decimal.from_float(min_score) | ||
| is_integer(min_score) -> Decimal.new(min_score) | ||
| is_binary(min_score) -> Decimal.new(min_score) | ||
| true -> Decimal.new(min_score) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n lib/volfefe_machine/workers/polymarket/alerting_worker.ex | head -150 | tail -100Repository: razrfly/volfefe
Length of output: 4250
🏁 Script executed:
# Search for how find_alert_candidates is called and where min_score originates
rg "find_alert_candidates" -A 3 -B 3Repository: razrfly/volfefe
Length of output: 1428
🏁 Script executed:
# Look for config_min_score or similar configuration in the file
rg "config_min_score|config|min_score" lib/volfefe_machine/workers/polymarket/alerting_worker.ex -nRepository: razrfly/volfefe
Length of output: 1375
🏁 Script executed:
# Check Decimal library behavior - does Decimal.new/1 raise on nil?
python3 << 'EOF'
# Look for Decimal.new behavior in Elixir documentation
# Decimal.new/1 behavior:
# - Decimal.new(nil) -> raises ArgumentError
# - Decimal.new(%Decimal{}) -> raises ArgumentError
# - Decimal.new("invalid") -> raises ArgumentError
# This is documented behavior in the Decimal library
print("Decimal.new/1 raises on:")
print("- nil")
print("- %Decimal{} struct")
print("- invalid string inputs")
print("\nDecimal.from_float/1 accepts floats and converts them")
print("Decimal.parse/1 returns {:ok, decimal} or :error tuple")
EOFRepository: razrfly/volfefe
Length of output: 243
🏁 Script executed:
# Search for where this function is called from and what arguments it receives
rg "find_alert_candidates" -B 5 -A 5Repository: razrfly/volfefe
Length of output: 2129
🏁 Script executed:
# Look for job/task args processing to understand min_score origin
rg "perform|min_score" lib/volfefe_machine/workers/polymarket/alerting_worker.ex -n -A 2 -B 2Repository: razrfly/volfefe
Length of output: 1636
Harden min_score normalization to prevent Decimal conversion crashes.
Decimal.new/1 raises on nil, invalid strings, or %Decimal{} inputs. Although min_score receives a fallback at line 54, the catch-all on line 117 still accepts any type and will crash the worker on unexpected inputs (e.g., if config returns an invalid value or a %Decimal{} is passed). Handle %Decimal{} pass-through, use Decimal.parse/1 for safe binary conversion, and add an explicit nil check.
🔧 Proposed fix
- min_score_decimal =
- cond do
- is_float(min_score) -> Decimal.from_float(min_score)
- is_integer(min_score) -> Decimal.new(min_score)
- is_binary(min_score) -> Decimal.new(min_score)
- true -> Decimal.new(min_score)
- end
+ min_score_decimal =
+ cond do
+ is_nil(min_score) -> Decimal.from_float(config_min_score())
+ match?(%Decimal{}, min_score) -> min_score
+ is_float(min_score) -> Decimal.from_float(min_score)
+ is_integer(min_score) -> Decimal.new(min_score)
+ is_binary(min_score) ->
+ case Decimal.parse(min_score) do
+ {d, ""} -> d
+ _ -> Decimal.from_float(config_min_score())
+ end
+ true ->
+ Decimal.new(to_string(min_score))
+ end🤖 Prompt for AI Agents
In `@lib/volfefe_machine/workers/polymarket/alerting_worker.ex` around lines 110 -
118, In find_alert_candidates, make min_score normalization robust: if min_score
is nil return a safe default Decimal (e.g., Decimal.new(0)); if min_score is
already a %Decimal{} return it unchanged; for floats and integers keep
Decimal.from_float/Decimal.new; for binaries use Decimal.parse/1 and handle
{:ok, dec} | :error (fall back to default Decimal on :error); remove the
catch-all Decimal.new(min_score) that can crash on unsupported types and ensure
any other unexpected type falls back to the safe default Decimal.
TL;DR
Replaced the GenServer-based
TradeMonitorwith Oban workers for more reliable real-time monitoring of suspicious trades.What changed?
TradeMonitorGenServer and its associated Mix task (mix polymarket.monitor)TradeIngestionWorker(runs every 2 minutes)TradeScoringWorker(runs every 5 minutes)AlertingWorker(runs every 10 minutes)AlertingWorkerto better handle different types of score inputsPredictionWorkerto properly handle string lengthHow to test?
TradeMonitormix polymarket.ingestto manually trigger trade ingestionWhy make this change?
The GenServer-based approach had several limitations:
Oban workers provide several advantages:
This change completes the migration to a more robust and maintainable architecture for real-time trade monitoring.
Summary by CodeRabbit
Bug Fixes
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.