Conversation
Expose all remaining read-only pdmt5.Mt5DataClient and Mt5Client
functions, plus write operations (order_send, order_check,
symbol_select, market_book_add/release).
New read-only endpoints:
- GET /last-error: last MT5 error info
- GET /symbols/total: total symbol count
- GET /orders/total: active orders count
- GET /positions/total: open positions count
- GET /history/orders/total: historical orders count by date range
- GET /history/deals/total: historical deals count by date range
- GET /calc/margin: margin calculation for a trading operation
- GET /calc/profit: profit calculation for a trading operation
New write endpoints:
- POST /order/check: validate funds sufficiency for a trade
- POST /order/send: execute a trade request
- POST /symbols/{symbol}/select: show/hide symbol in MarketWatch
- POST /market-book/{symbol}/subscribe: subscribe to market depth
- POST /market-book/{symbol}/unsubscribe: unsubscribe from market depth
https://claude.ai/code/session_01SJQJzQNfkFizmofhvFEByB
Update README, docs/index.md, docs/api/index.md, and docs/api/rest-api.md to cover all new endpoints added in the previous commit: read-only endpoints (last-error, symbols/total, orders/total, positions/total, history totals, calc/margin, calc/profit) and write endpoints (order/check, order/send, symbol select, market-book subscribe/unsubscribe). Also revise the "read-only" framing to reflect that the API now includes trading write operations. https://claude.ai/code/session_01SJQJzQNfkFizmofhvFEByB
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c728814d71
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Code Review Summary
Well-structured PR that cleanly extends existing patterns (Mt5ConstantSpec, router conventions, test fixtures). The new endpoints follow established async/threadpool patterns and the ORDER_TYPE constant support mirrors the existing TIMEFRAME/COPY_TICKS implementation nicely.
However, there are several issues that should be addressed before merging, primarily around security and input validation for the new write endpoints.
Critical
-
OrderCheckRequest/OrderSendRequestaccept unvalidateddict[str, Any]— Every other request model in the codebase uses strongly-typed, validated fields. These two models pass an arbitrary dict directly to the MT5 client, bypassing all input validation and making the OpenAPI schema useless for consumers. At minimum, type the core fields (action,symbol,volume,type,price). ConsiderConfigDict(extra="allow")to preserve flexibility for less common fields. -
Write endpoints are unprotected when
MT5API_SECRET_KEYis unset —verify_api_keysilently passes through when no key is configured. This was acceptable for a read-only API but is dangerous for/order/send, which executes real trades. Write endpoints should require auth unconditionally, or the trading router should refuse to register when auth is disabled. -
CLAUDE.md contradicts this PR — The project's own architecture doc states "The service is intentionally read-only" and lists only the original five routers. This must be updated in the same PR.
Important
-
No
symbolpath parameter validation on market-book endpoints —post_market_book_subscribeandpost_market_book_unsubscribeaccept barestrwith no constraints, unlike every other symbol parameter in the codebase which usesmin_length=1, max_length=32. -
Market book subscription resource leak — No tracking of active subscriptions and no cleanup on shutdown. Leaked subscriptions accumulate MT5 server-side resources.
-
Missing test coverage — No Parquet tests for any of the 5 trading endpoints (pattern required by every other router). No failure-path tests for
symbol_select,market_book_add/releasereturningFalse. No validation error tests for calc endpoints (invalid action, zero/negative values). No auth-rejection tests for new routers.
Minor
API_DESCRIPTIONinconstants.pystill says "Provides read-only access" — needs updating.- Duplicated date-range validation in
HistoryTotalRequestvsHistoryRequestBase— consider extracting a shared helper.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
PR Review: Add calculation and MT5 utility endpoints
Overall this is a well-structured PR. The new routers, models, and tests integrate cleanly with established patterns. The Mt5ConstantSpec / Mt5OrderType reuse is particularly clean. A few noteworthy items below (inline comments on specific files, and general observations here).
Positive:
- Consistent use of run_in_threadpool, dependency injection, and format_response across all new endpoints
- Good test coverage of the subscription lifecycle including the full lifespan round-trip shutdown test
- Defensive shutdown handling with try/finally ensuring shutdown_mt5_client() runs even if subscription cleanup fails
- Clean separation of read-only vs operational endpoints in docs
Test gaps to close (project targets 100%):
- No Parquet output tests for orders/total, positions/total, history/orders/total, history/deals/total (other similar endpoints like symbols/total do have them)
- No test for _release_market_book_subscriptions when the subscriptions attribute is missing or is an empty set (early-return branch)
- No equal-dates boundary test for HistoryTotalRequest (validator uses >= but only reversed dates are tested)
- No validation edge-case tests for TradeRequest field constraints (empty symbol, zero volume, negative action, invalid ORDER_TYPE)
- Missing date-range validation test for GET /history/deals/total (exists for orders/total but not deals/total)
- No test for MT5 returning None from order_calc_margin / order_calc_profit
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
This PR adds non-executing MT5 utility endpoints while preserving the service read-only boundary. It introduces calculation endpoints, order validation via
/order/check, symbol selection and market-book subscription management, and several diagnostic/count endpoints.Key Changes
Non-executing operational endpoints
POST /order/check- Validate a trade request without executing itPOST /symbols/{symbol}/select- Show or hide symbols in MarketWatchPOST /market-book/{symbol}/subscribe- Subscribe to DOM eventsPOST /market-book/{symbol}/unsubscribe- Unsubscribe from DOM eventsCalculation endpoints
GET /calc/margin- Calculate required margin for a tradeGET /calc/profit- Calculate profit/loss for a tradeCount and diagnostic endpoints
GET /orders/total- Get active orders countGET /positions/total- Get open positions countGET /history/orders/total- Get historical orders count with date rangeGET /history/deals/total- Get historical deals count with date rangeGET /symbols/total- Get total symbol countGET /last-error- Get last MT5 error informationSafety and validation updates
POST /order/send) to keep the API read-onlyTradeRequestmodel for core field validationTests and docs
README.mdanddocs/api/rest-api.mdto remove trade execution and describe the remaining operational endpoints