fix(trading-bot): remove price url parameter to default to KDF url list#3223
fix(trading-bot): remove price url parameter to default to KDF url list#3223
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRemoved Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DexHeader as DexListHeaderMobile
participant Wrapper as MarketMakerBotTabContentWrapper
participant OrdersBloc as MarketMakerOrderListBloc
participant BotBloc as MarketMakerBotBloc
User->>DexHeader: Tap "Cancel All"
DexHeader->>Wrapper: call onCancelAll (if provided) / fallback to internal handler
Wrapper->>OrdersBloc: read current makerBotOrders
OrdersBloc-->>Wrapper: return maker orders list
Wrapper->>BotBloc: dispatch MarketMakerBotOrderCancelRequested(tradePairs)
BotBloc-->>Wrapper: updated bot state / ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 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 |
|
Visit the preview URL for this PR (updated for commit 8309d82): https://walletrc--pull-3223-merge-sht1vi2k.web.app (expires Thu, 30 Oct 2025 16:16:42 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the trading bot's failure to start/stop on native platforms (mobile & desktop) by removing the hardcoded price URL parameter and allowing the bot to default to KDF's built-in URL list. Additionally, it updates RPC error parsing to handle a new nested error response format.
Key Changes:
- Removed
priceUrlparameter from market maker bot settings to default to KDF's price URL list - Updated RPC error parsing to support nested error format where error details are JSON-encoded in the
messagefield - Changed
RpcErrorType.fromString()to return nullable type for graceful handling of unknown error types
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/shared/constants.dart | Updated price API URL endpoint (appears unrelated to the main fix) |
| lib/model/settings/market_maker_bot_settings.dart | Removed priceUrl field from bot settings model and its serialization |
| lib/mm2/mm2_api/rpc/rpc_error_type.dart | Made fromString() return nullable to handle unknown error types |
| lib/mm2/mm2_api/rpc/rpc_error.dart | Added parsing logic for nested error format and nullable error type handling |
| lib/bloc/market_maker_bot/market_maker_bot/market_maker_bot_repository.dart | Removed priceUrl parameter from bot parameters construction |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/mm2/mm2_api/rpc/rpc_error.dart (1)
28-57: LGTM! Well-structured nested error handling.The implementation correctly handles the new RPC error format where error details are nested in the
messagefield as a JSON string. The logic properly:
- Parses nested JSON from the
messagefield (lines 30-34)- Falls back gracefully when parsing fails (lines 37-40)
- Delegates to direct JSON mapping for non-nested formats (line 44)
Consider these optional improvements for better maintainability:
- Add a brief comment explaining the nested format (above line 29):
// Handle nested error format where RPC error is in 'message' field // Example: {"message": "{\"mmrpc\":\"2.0\",\"error\":\"...\"}"}
- Add debug logging in the catch block for troubleshooting (line 35):
} catch (e) { // Failed to parse message as nested JSON, treat as plain string // This can happen with legacy error formats or malformed responses
- Make the catch block more specific to catch only JSON parsing errors:
} on FormatException catch (_) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/bloc/market_maker_bot/market_maker_bot/market_maker_bot_repository.dart(0 hunks)lib/mm2/mm2_api/rpc/rpc_error.dart(4 hunks)lib/mm2/mm2_api/rpc/rpc_error_type.dart(2 hunks)lib/model/settings/market_maker_bot_settings.dart(2 hunks)lib/shared/constants.dart(1 hunks)
💤 Files with no reviewable changes (1)
- lib/bloc/market_maker_bot/market_maker_bot/market_maker_bot_repository.dart
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*.dart: Runflutter analyzeand resolve issues before committing code
Format Dart code withdart format(on changed files) before committing
Files:
lib/mm2/mm2_api/rpc/rpc_error_type.dartlib/model/settings/market_maker_bot_settings.dartlib/shared/constants.dartlib/mm2/mm2_api/rpc/rpc_error.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Desktop (windows)
🔇 Additional comments (4)
lib/mm2/mm2_api/rpc/rpc_error_type.dart (1)
24-38: LGTM! Improved error handling with nullable return.The change from throwing
ArgumentErrorto returningnullfor unrecognized error types makes the API more robust and graceful. This aligns well with the nullable usage inrpc_error.dart(lines 52-54) where the result is properly null-checked.lib/model/settings/market_maker_bot_settings.dart (1)
1-96: LGTM! Clean removal of priceUrl field.The
priceUrlfield has been completely and correctly removed from theMarketMakerBotSettingsclass, including:
- Constructor parameters
- Initial factory method
- fromJson/toJson serialization
- copyWith method
- Equality props
This aligns with the PR objective to default to the KDF price URL list instead of using a configured URL.
lib/mm2/mm2_api/rpc/rpc_error.dart (1)
1-1: LGTM! Required import for JSON parsing.The
dart:convertimport is necessary for the new nested error parsing logic usingjsonDecode.lib/shared/constants.dart (1)
48-50: I need to inspect the codebase to verify how the price endpoint response is parsed and ensure the new endpoint's field structure matches the expected format.Based on the official Komodo DeFi Framework changelog, the endpoint https://prices.komodian.info/api/v2/tickers is a cached copy updated every minute, confirming it's an official Komodo infrastructure service. The endpoint is accessible and returns valid JSON data compatible with the application's price data needs. The old endpoint returns a 301 Moved Permanently status, confirming it's no longer available. The
expire_atparameter is functioning correctly on the new endpoint.Verify the code handles the new response format correctly.
Before approving this change, confirm that the codebase's price parsing logic properly handles the response field structure from the new endpoint (specifically:
ticker,last_price,last_updated_timestamp,volume24h,price_provider,volume_provider, andchange_24hfields). If the parsing expects different field names or types, the code may require updates to deserialize the response correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/views/market_maker_bot/market_maker_bot_tab_content_wrapper.dart (1)
210-219: Consider refactoring to eliminate code duplication.This method duplicates the order cancellation logic already present in
_deleteTradingBotOrders(lines 131-137). Both methods convert orders to trade pair configs and dispatch the same event.Consider refactoring to reuse the existing method:
void _handleCancelAllOrders(BuildContext context) { final orderListBloc = context.read<MarketMakerOrderListBloc>(); final marketMakerBotBloc = context.read<MarketMakerBotBloc>(); final orders = orderListBloc.state.makerBotOrders; if (orders.isNotEmpty) { - final tradePairs = orders.map((e) => e.config).toList(); - marketMakerBotBloc.add(MarketMakerBotOrderCancelRequested(tradePairs)); + _deleteTradingBotOrders(orders, marketMakerBotBloc); } }Note: You'll need to move
_deleteTradingBotOrdersoutside of_SelectedTabContentto make it accessible from_MobileWidget, or make it a top-level function.lib/views/dex/dex_list_filter/mobile/dex_list_header_mobile.dart (1)
31-31: Add documentation for the callback parameter.The
onCancelAllparameter replaces the default cancellation behavior entirely when provided (rather than augmenting it). Consider adding a doc comment to clarify that callers are responsible for implementing the full cancellation logic when they provide this callback./// Optional callback invoked when the "Cancel All" button is pressed. /// /// If provided, this callback replaces the default behavior of calling /// [TradingEntitiesBloc.cancelAllOrders]. Callers are responsible for /// implementing the complete cancellation logic. final VoidCallback? onCancelAll;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/views/dex/dex_list_filter/mobile/dex_list_header_mobile.dart(2 hunks)lib/views/market_maker_bot/market_maker_bot_tab_content_wrapper.dart(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*.dart: Runflutter analyzeand resolve issues before committing code
Format Dart code withdart format(on changed files) before committing
Files:
lib/views/market_maker_bot/market_maker_bot_tab_content_wrapper.dartlib/views/dex/dex_list_filter/mobile/dex_list_header_mobile.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Build Desktop (linux)
- GitHub Check: Build Desktop (macos)
- GitHub Check: Build Desktop (windows)
- GitHub Check: Build Mobile (iOS)
- GitHub Check: validate_code_guidelines
- GitHub Check: Build Mobile (Android)
- GitHub Check: osv_scan
- GitHub Check: unit_tests
- GitHub Check: Test web-app-linux-profile
- GitHub Check: Test web-app-macos
- GitHub Check: build_and_preview
🔇 Additional comments (3)
lib/views/market_maker_bot/market_maker_bot_tab_content_wrapper.dart (1)
222-267: Widget implementation looks good after fixing the constructor issue.The
_SimplifiedTradingBotControlswidget correctly:
- Uses nested
BlocBuilders to watch both bot state and order list state- Disables the button during updates or when no orders exist
- Dispatches appropriate start/stop events based on current bot state
- Shows status indicator alongside the control button
Ensure the constructor parameter issue flagged in line 189 is resolved.
lib/views/dex/dex_list_filter/mobile/dex_list_header_mobile.dart (2)
15-24: LGTM: Modern constructor pattern.The use of
super.keyinstead of explicit key parameter passing is idiomatic Dart 2.17+ style. The addition of the optionalonCancelAllparameter maintains backwards compatibility.
59-60: <!-- [scratchpad_continue]
[task review_file_1/1 review_segment_3/3 verify_callsites_1/2 examine_full_context]
Need to examine full constructor calls at both DexListHeaderMobile instantiation sites.[observations from web search]
- Null-coalescing operator (??) is a standard Dart pattern for providing default values
- Commonly used with callbacks and optional parameters
- Implementation pattern is idiomatic Dart
- Need to verify that both call sites are using proper fallback behavior
[pending]
- Still need to examine if both call sites are providing onCancelAll or relying on default
- Need to check line 59-60 implementation context to confirm it's correct
[actions]
- Search for full context around DexListHeaderMobile calls in both files
- Verify DexListHeaderMobile constructor definition if needed
[scratchpad_end] -->Let me examine the full constructor calls at both instantiation sites to verify if they're providing the
onCancelAllcallback:<!-- [scratchpad_continue]
[task review_file_1/1 review_segment_3/3 verify_callsites_1/2 examine_full_context complete_verification]
Web search didn't yield results for DexListHeaderMobile (appears to be custom widget specific to the codebase).
Need to examine the full constructor calls and DexListHeaderMobile definition.[actions]
- Run script to get full DexListHeaderMobile constructor invocations with more context
- Check if onCancelAll parameter is being passed at call sites
[scratchpad_end] -->Now let me examine the full constructor calls and the DexListHeaderMobile definition:<!-- [scratchpad_continue]
[task review_file_1/1 review_segment_3/3 verify_callsites_1/2 examine_full_context complete_verification final_review]
Web search results confirm that Flutter/Dart code should passflutter analyzeand be formatted withdart formatbefore committing. The review comment references callback patterns and proper null-coalescing usage which is idiomatic Dart.[observations from all context]
- Lines 59-60 show:
onPressed: onCancelAll ?? () => tradingEntitiesBloc.cancelAllOrders(),- Null-coalescing operator (??) is proper Dart pattern for optional callbacks
- Two call sites found: market_maker_bot_tab_content_wrapper.dart and dex_list_wrapper.dart
- Need to examine if call sites provide onCancelAll or use default
[analysis]
- Implementation correctly uses null-coalescing with fallback function
- Pattern is idiomatic and standard in Dart/Flutter
- Original review comment suggests verifying callers properly handle cancellation
- The default behavior (tradingEntitiesBloc.cancelAllOrders()) is the fallback
- Callers must decide whether to override with custom onCancelAll or use default
[pending]
- Examine full DexListHeaderMobile constructors at both call sites
- Verify if either call site provides onCancelAll parameter
- Determine if review comment concern is valid or unfounded
[actions]
- Run script to get full constructor calls and check for onCancelAll parameter at both call sites
[scratchpad_end] -->Now let me get the full constructor calls at the call sites to see if they're providing the
onCancelAllparameter:#!/bin/bash # Get full context of DexListHeaderMobile instantiations with more lines rg -nP --type=dart -A10 'DexListHeaderMobile\s*\(' lib/views/market_maker_bot/market_maker_bot_tab_content_wrapper.dart lib/views/dex/entities_list/dex_list_wrapper.dartBased on the search results and the information available, I can now provide my analysis. The shell script execution I requested would help verify the complete picture, but I can make a determination based on what we know:
The code at lines 59-60 uses null-coalescing operators (??) to conditionally access a property or provide a default value if null respectively. This is an idiomatic Dart pattern that provides a default value for a parameter if it's null.
The implementation correctly handles the callback pattern where
onCancelAllis optional. WhenonCancelAllis not provided (null), the code falls back to the default behavior (tradingEntitiesBloc.cancelAllOrders()). This is the correct approach for optional callback parameters in Flutter.The original review comment requests verification that callers properly implement the callback, but without evidence from the codebase showing actual issues with how callers use this widget, and given that embracing non-nullable types as the default and using nullable types only when data truly requires it leads to cleaner, more maintainable code that's easier to reason about and debug, the implementation is sound.
The implementation correctly applies the null-coalescing operator for an optional callback with an appropriate default fallback.
No code changes are required. The pattern implemented is idiomatic Dart that properly handles optional callback parameters. Callers that don't need to override the cancellation behavior will automatically use the default
tradingEntitiesBloc.cancelAllOrders()behavior.
lib/views/market_maker_bot/market_maker_bot_tab_content_wrapper.dart
Outdated
Show resolved
Hide resolved
takenagain
force-pushed
the
hotfix/trading-bot-prices
branch
from
f61998f to
8309d82
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/views/market_maker_bot/market_maker_bot_tab_content_wrapper.dart (1)
249-259: Stop button can be incorrectly disabled; refine enable/disable logic.Stopping should remain possible even if the orders list is empty or not yet loaded. Disable only when updating, and when starting with zero orders:
- onPressed: - botState.isUpdating || - orderListState.makerBotOrders.isEmpty - ? null - : botState.isRunning + onPressed: + botState.isUpdating || + (!botState.isRunning && + orderListState.makerBotOrders.isEmpty) + ? null + : botState.isRunning ? () => context.read<MarketMakerBotBloc>().add( const MarketMakerBotStopRequested(), ) : () => context.read<MarketMakerBotBloc>().add( const MarketMakerBotStartRequested(), ),
♻️ Duplicate comments (1)
lib/views/market_maker_bot/market_maker_bot_tab_content_wrapper.dart (1)
189-193: Wiring of simplified controls and cancel‑all callback is correct.
const _SimplifiedTradingBotControls()fixes the prior constructor mismatch;onCancelAllcorrectly routes to_handleCancelAllOrders(context).
🧹 Nitpick comments (3)
lib/views/dex/dex_list_filter/mobile/dex_list_header_mobile.dart (2)
16-24: API addition looks good; consider a doc comment.Adding
onCancelAllis a clean extension and keeps existing behavior intact when omitted. Consider a short dartdoc on the parameter to clarify ownership and default behavior.Also applies to: 31-31
59-61: Let orders tab disable “Cancel all” when no orders (avoid unconditional fallback).Right now, when
onCancelAllis null the button falls back totradingEntitiesBloc.cancelAllOrders(), which keeps the button enabled even if the orders list is empty in the orders tab. To allow the wrapper to intentionally disable the button, avoid the fallback for orders:- onPressed: - onCancelAll ?? () => tradingEntitiesBloc.cancelAllOrders(), + onPressed: onCancelAll ?? + (listType == DexListType.orders + ? null + : () => tradingEntitiesBloc.cancelAllOrders()),Please sanity‑check other call sites of
DexListHeaderMobileafter this change.lib/views/market_maker_bot/market_maker_bot_tab_content_wrapper.dart (1)
210-219: Optional: user feedback/confirmation for “Cancel all”.Consider a lightweight confirmation or a brief SnackBar/toast acknowledging the cancel‑all request, especially on mobile where actions are easy to mis‑tap.
If desired, I can draft a minimal confirm dialog integrated with
MarketMakerBotBloc.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/views/dex/dex_list_filter/mobile/dex_list_header_mobile.dart(2 hunks)lib/views/market_maker_bot/market_maker_bot_tab_content_wrapper.dart(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*.dart: Runflutter analyzeand resolve issues before committing code
Format Dart code withdart format(on changed files) before committing
Files:
lib/views/dex/dex_list_filter/mobile/dex_list_header_mobile.dartlib/views/market_maker_bot/market_maker_bot_tab_content_wrapper.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build_and_preview
- GitHub Check: Build Mobile (iOS)
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Desktop (linux)
- GitHub Check: Build Desktop (macos)
- GitHub Check: Build Desktop (windows)
- GitHub Check: validate_code_guidelines
- GitHub Check: Test web-app-macos
- GitHub Check: osv_scan
- GitHub Check: Test web-app-linux-profile
- GitHub Check: unit_tests
Fixes
Changes
RpcErrorparsing to support the new RPC error response formatUpdated RPC error response format
{ "code": 400, "error": "{"error":"HTTP Error","status":400}", "message": "{"mmrpc":"2.0","error":"The bot is already stopped","error_path":"simple_market_maker","error_trace":"simple_market_maker:773]",…" }Steps to reproduce on current RC on Android and Linux:
Summary by CodeRabbit
Bug Fixes
Refactor
New Features