Add hosted MT5 API broker (MetaTrader 5 integration) - #1132
Add hosted MT5 API broker (MetaTrader 5 integration)#1132miguelangelo78 wants to merge 4 commits into
Conversation
Adds an optional broker and data source for the hosted TickerAll MetaTrader 5 API (https://tickerall.com), so a Lumibot strategy can trade any MT5 account (Forex, metals, indices, CFDs, crypto) on any operating system with no local MetaTrader 5 terminal installed. Addresses the MT5 integration request in Lumiwealth#977. The integration is fully additive: two new modules plus one line each in the broker and data-source registries and the docs toctree. No existing code paths are changed. - lumibot/brokers/tickerall.py: TickerAll(Broker) with account balances, open positions, market/limit/stop orders (with optional stop-loss/take-profit), cancel and modify, and a PollingStream fill loop (modeled on the ccxt and bitunix brokers). Unsupported order types (stop_limit, trailing_stop) are rejected with a clear message rather than being silently mishandled. - lumibot/data_sources/tickerall_data.py: TickerAllData(DataSource) with historical bars, last price and quotes. Missing bars return None rather than fabricated data. - docsrc/brokers.tickerall.rst: usage documentation, wired into the brokers toctree. - tests/test_broker_tickerall.py: unit tests with the hosted client mocked (skipped when the optional tickerall package is not installed). The tickerall package is an optional dependency, imported lazily, so Lumibot stays importable without it.
Found and fixed by running a complete Strategy-through-Trader flow live on both a netting and a hedging demo account: - cancel_order: do not skip when the local order status is "cancelling". The strategy sets that status right before calling the broker, and is_canceled() treats "cancelling" as canceled, so the broker cancel was being silently skipped and the order stayed open at the broker. - close_position / sell_all: use the hosted API's native position-close (by ticket) instead of the base broker's offsetting sell order. On a netting account the offsetting fill could leave a transient phantom position in the tracker; closing by ticket flattens cleanly. Adds cancel_open_orders. - positions: aggregate the broker positions for a symbol into one net Lumibot position (BUY +, SELL -), so hedging accounts (which can hold several positions per symbol) map onto Lumibot's one-position-per-asset model, and remember every underlying ticket for closing. - reconcile the position tracker from the broker snapshot right after a fill. Tests: adds coverage for the cancelling-status guard, the terminal-status skip, the polling fill/cancel reconciliation, and hedging aggregation (24 total, all passing).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a hosted TickerAll MetaTrader 5 broker and data source with account access, market data, order handling, position reconciliation, polling, optional installation metadata, documentation, lazy exports, and mocked tests. ChangesTickerAll integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Strategy
participant TickerAll
participant TickerAllAPI
participant PollingStream
Strategy->>TickerAll: submit market or pending order
TickerAll->>TickerAllAPI: place hosted MT5 order
TickerAllAPI-->>TickerAll: return order result
TickerAll->>PollingStream: dispatch order event
PollingStream->>TickerAll: poll broker state
TickerAll->>TickerAllAPI: fetch positions and pending orders
TickerAllAPI-->>TickerAll: return current state
TickerAll-->>Strategy: dispatch reconciled order status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (4.0.6)lumibot/brokers/tickerall.py************* Module pylintrc ... [truncated 20875 characters] ... module": "lumibot.brokers.tickerall", tests/test_broker_tickerall.py************* Module pylintrc ... [truncated 31665 characters] ... .test_symbol_fetch_failure_not_cached", lumibot/data_sources/tickerall_data.py************* Module pylintrc ... [truncated 7642 characters] ... ule": "lumibot.data_sources.tickerall_data", 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 |
miguelangelo78
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docsrc/brokers.tickerall.rst`:
- Around line 28-36: Update the Tickerall data-source configuration to resolve
TICKERALL_API_KEY and TICKERALL_ACCOUNT_ID alongside config values, preserving
optional account selection behavior. Keep the environment-variable documentation
in docsrc/brokers.tickerall.rst lines 28-36 synchronized with that behavior, and
add regression coverage in tests/test_broker_tickerall.py lines 42-50 that
instantiates the integration using the documented variables.
In `@lumibot/brokers/tickerall.py`:
- Around line 530-549: The reconciliation logic around get_tracked_orders and
_process_trade_event must recognize exact close fills even when the asset is
absent from _filled_positions. Track or otherwise preserve broker-confirmed fill
information for net-zero positions, and use it to dispatch FILLED_ORDER before
falling back to CANCELED_ORDER for orders that disappeared from broker_ids.
In `@lumibot/data_sources/tickerall_data.py`:
- Around line 124-133: Update _ensure_symbols so a failed accounts.symbols fetch
does not assign [] to self._symbols; leave it as None after logging the
exception, allowing later calls to retry. Preserve the successful symbol-list
caching and return behavior.
- Around line 63-68: Populate the TickerAll-related environment variables into
the configuration before TickerAllData’s credential reads, including
TICKERALL_API_KEY, TICKERALL_ACCOUNT_ID, and the expected BASE_URL variable.
Update the initialization/configuration flow used by TickerAllData so _cfg can
resolve these values, while preserving explicitly supplied config values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ba4ecbb8-4226-49fb-9ccd-973663116477
📒 Files selected for processing (8)
docsrc/brokers.rstdocsrc/brokers.tickerall.rstlumibot/brokers/__init__.pylumibot/brokers/tickerall.pylumibot/data_sources/__init__.pylumibot/data_sources/tickerall_data.pysetup.pytests/test_broker_tickerall.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (13)
**/*.{py,md,rst,txt,yml,yaml,json,ini,env}
📄 CodeRabbit inference engine (CLAUDE.md)
Never fabricate, synthesize, forward-fill, interpolate, or default-fill missing market data in backtests; return empty / explicit absence instead, and remove any code that returns fake bars as real data.
Files:
docsrc/brokers.rstsetup.pydocsrc/brokers.tickerall.rstlumibot/brokers/__init__.pylumibot/data_sources/__init__.pytests/test_broker_tickerall.pylumibot/data_sources/tickerall_data.pylumibot/brokers/tickerall.py
**/*.{png,jpg,jpeg,webp,gif,svg,md,rst}
📄 CodeRabbit inference engine (CLAUDE.md)
For any generated or AI-edited image or documentation visual, use Nano Banana MCP only; do not use fallback image generators, Mermaid screenshots, or manual diagram pipelines, and visually inspect every output before committing.
Files:
docsrc/brokers.rstdocsrc/brokers.tickerall.rst
docsrc/**/*.rst
📄 CodeRabbit inference engine (CLAUDE.md)
docsrc/**/*.rst: When making user-facing changes, update the relevant Sphinx docs underdocsrc/(for example brokers, strategy methods, lifecycle methods, entities, backtesting, FAQ, common mistakes, getting started, or deployment pages).
For user-facing behavior changes, keep public documentation examples, parameters, return values, and edge cases current, and build the docs locally to verify rendering when applicable.
docsrc/**/*.rst: Update the appropriate public Sphinx documentation for user-facing changes, including brokers, strategy APIs, data sources, environment variables, deployment, FAQs, and common mistakes.
When adding or changing an environment variable, update docsrc/environment_variables.rst and, when useful for contributors, docs/ENV_VARS.md.
Files:
docsrc/brokers.rstdocsrc/brokers.tickerall.rst
**/*.{py,md,rst}
📄 CodeRabbit inference engine (CLAUDE.md)
If an environment variable is introduced or changed, make sure both engineering and public documentation stay synchronized with the new behavior.
Files:
docsrc/brokers.rstsetup.pydocsrc/brokers.tickerall.rstlumibot/brokers/__init__.pylumibot/data_sources/__init__.pytests/test_broker_tickerall.pylumibot/data_sources/tickerall_data.pylumibot/brokers/tickerall.py
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Never commit private, account-specific, machine-specific, or credential information to tracked repository files. Use placeholders and public environment-variable names instead; rotate exposed credentials immediately.
Backtesting accuracy should be evaluated against live broker behavior when possible; vendor-parity artifacts are regression signals, not absolute truth.
Normal work must remain in the canonical checkout on the active version branch. Do not create sibling worktrees, switch branches, create additional branches, or manage pull requests unless explicitly authorized.
Do not publish, deploy, tag, bump release versions, create GitHub Releases, trigger workflows, update downstream pins, or deploy to dev/production without explicit authorization.
Never run git checkout or destructive operations such as git reset --hard, git clean -f, or git stash; understand dirty files and preserve other agents' changes.
Commit coherent changes in small logical chunks, inspect the diff before committing, and avoid overwriting work from other agents.
Never launch ThetaTerminal locally with production credentials, because it can terminate the licensed production session and halt customers.
Never hardcode or share private downloader URLs; use placeholders or DATADOWNLOADER_BASE_URL.
Load local ThetaTerminal credentials only from an untracked secret manager or environment, use a disposable $TMPDIR file when necessary, delete it on exit, and never use those credentials for backtests.
Do not delete shared caches; use versioned S3 namespaces for cold-cache simulations and delete cache objects only when explicitly requested and tightly scoped.
Do not add environment variables merely to skip or disable tests. Prefer existing pytest markers and document genuinely required user-facing environment variables.
Every project must define and track North Star metrics and OKRs, review leading indicators weekly, and consider metric impact when prioritizing work.
Files:
docsrc/brokers.rstsetup.pydocsrc/brokers.tickerall.rstlumibot/brokers/__init__.pylumibot/data_sources/__init__.pytests/test_broker_tickerall.pylumibot/data_sources/tickerall_data.pylumibot/brokers/tickerall.py
**/*.{py,rst,md}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{py,rst,md}: Provider credentials required by selected components must be explicit in documentation, examples, errors, and metadata; do not rely on hidden SDK aliases or environment-variable mutation.
Behavioral changes require relevant documentation and regression tests in the same change, with comments explaining non-obvious invariants.
Files:
docsrc/brokers.rstsetup.pydocsrc/brokers.tickerall.rstlumibot/brokers/__init__.pylumibot/data_sources/__init__.pytests/test_broker_tickerall.pylumibot/data_sources/tickerall_data.pylumibot/brokers/tickerall.py
**
⚙️ CodeRabbit configuration file
**: Review every pull request as if LumiBot is a public open-source trading framework.
Prioritize real bugs, security/privacy issues, user-facing regressions, and release risk.Always check changed lines for:
- hardcoded credentials, API keys, tokens, account emails, private URLs, private hostnames, local credential paths, or personal filesystem paths;
- accidental leakage of BotSpot, Lumiwealth, customer, broker, paid-vendor, CI, or maintainer-only operational details into public code/docs/tests;
- code that logs, prints, persists, screenshots, or commits secrets or customer/broker data;
- test fixtures or docs that look fake but could be copied into real usage as credentials or private endpoints;
- changes that weaken authentication, authorization, data-source safety, broker/order safety, or CI/release gates.
If a finding depends on repository context, explain the specific source file and invariant instead of giving generic advice.
Files:
docsrc/brokers.rstsetup.pydocsrc/brokers.tickerall.rstlumibot/brokers/__init__.pylumibot/data_sources/__init__.pytests/test_broker_tickerall.pylumibot/data_sources/tickerall_data.pylumibot/brokers/tickerall.py
docsrc/**
⚙️ CodeRabbit configuration file
docsrc/**: Sphinx documentation is public. Flag private paths, credential examples, internal
account emails, private endpoints, stale security claims, or operational details that
should live only in private runbooks.
Files:
docsrc/brokers.rstdocsrc/brokers.tickerall.rst
setup.py
📄 CodeRabbit inference engine (CLAUDE.md)
Treat the
version=value insetup.pyas the authoritative project version source of truth.
Files:
setup.py
lumibot/brokers/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
lumibot/brokers/**/*.py: Reproduce customer broker issues through the exact saved strategy, broker, asset class, order lifecycle, local state transitions, account-selection behavior, and complete logs before claiming resolution.
Do not treat local CANCELLING as terminal before sending the broker cancel request, and do not suppress explicit broker calls such as cancel_order() or _modify_order() based only on local status helpers.
Files:
lumibot/brokers/__init__.pylumibot/brokers/tickerall.py
lumibot/**
⚙️ CodeRabbit configuration file
lumibot/**: Review runtime/library changes for trading safety, broker/account isolation, data
correctness, secret handling, and public/private boundary violations. Flag hardcoded
BotSpot-specific behavior unless it is clearly provider-generic and appropriate for
open-source LumiBot.
Files:
lumibot/brokers/__init__.pylumibot/data_sources/__init__.pylumibot/data_sources/tickerall_data.pylumibot/brokers/tickerall.py
tests/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
tests/**/*.py: Add unit tests for any new functionality
Ensure high level of test coverage using pytest with coverage reporting
Ensure all tests are well-documented and follow best practices
tests/**/*.py: Treat any test whose earliest commit date is before 2025-06-01 as LEGACY. For LEGACY tests, fix the code, not the test. Only change a LEGACY test when you can clearly justify that the old expectation was incorrect or behavior was intentionally changed for correctness, and document it in the test file.
Treat any test whose earliest commit date is before 2025-01-01 as FROZEN LEGACY (effectively 'do not change'). Only change a FROZEN LEGACY test in exceptional cases and include a clear write-up in the test and PR.
If you change any expected values or assertions in a test, add a short note near the change explaining why (what changed, and why the new expectation is correct). Prefer making the test more robust (less brittle) over updating magic numbers.
tests/**/*.py: Backtests and tests must use DATADOWNLOADER_BASE_URL and DATADOWNLOADER_API_KEY at runtime and must not shortcut through Theta directly.
Wrap long pytest, backtest, and stress commands with bin/safe-timeout 1200s when available, or use another safe timeout and split long jobs.
Consider test age before changing failures: tests older than one year generally require code fixes, while newer tests should be investigated and adjusted only when justified; document conflicts.
Files:
tests/test_broker_tickerall.py
tests/**
⚙️ CodeRabbit configuration file
tests/**: Tests are public. Flag hardcoded real-looking credentials, account emails, personal
paths, private endpoints, and fixtures that could accidentally exercise production or
paid-vendor resources without explicit opt-in.
Files:
tests/test_broker_tickerall.py
🔇 Additional comments (12)
docsrc/brokers.rst (1)
36-36: LGTM!lumibot/data_sources/tickerall_data.py (4)
189-199: 🎯 Functional Correctness | 💤 Low valueMinor:
length=0returns the full remaining candle list, not an empty one.
candles[-int(length):]withlength=0evaluates tocandles[0:](Python's-0 == 0), returning everything instead of nothing. Low real-world impact sincelengthis normally a positive count, but worth guarding explicitly.
1-56: LGTM!Also applies to: 169-256, 276-288
152-166: 🎯 Functional CorrectnessNo change needed.
_parse_source_timestepis inherited fromDataSource; it uses class-specificTIMESTEP_MAPPINGand raisesUnavailabeTimestepwhen a reverse mapping is missing.
257-274: 🚀 Performance & ScalabilityCheck TickerAll integration compatibility and live-stream timing assumptions.
The available documentation and repository context do not establish a
stream.wait_for_tick(..., timeout=6.0)API that returns ticks within these methods. If this method is not supported by the installed TickerAll SDK,get_last_price/get_quotewill reliably hit the candle fallback; if it is supported, document/verify that the call is guaranteed non-blocking or replace it with a cached-tick callback pattern.Also,
_latest_tick()/_ensure_stream()mutateself._streamandself._subscribedwithoutself._lock, so concurrentget_last_price/get_quotecalls that reconnect can leave subscriptions inconsistent with the active stream.lumibot/data_sources/__init__.py (1)
26-26: LGTM!setup.py (1)
123-128: LGTM!lumibot/brokers/tickerall.py (4)
226-249: 🎯 Functional Correctness | ⚡ Quick winSTOP orders get
limit_priceincorrectly populated fromprice.Line 234's fallback
getattr(response, "limit_price", None) or getattr(response, "price", None)runs unconditionally, regardless oforder_type. For a parsed STOP order (nolimit_priceattribute on the response), this falls through to the samepricevalue already used forstop_priceat line 235 — the resultingOrderends up with bothlimit_priceandstop_priceset to the stop target, whenlimit_priceshould beNone.🐛 Proposed fix
- limit_price = getattr(response, "limit_price", None) or getattr(response, "price", None) - stop_price = getattr(response, "price", None) if order_type == Order.OrderType.STOP else None + if order_type == Order.OrderType.STOP: + limit_price = None + stop_price = getattr(response, "price", None) + else: + limit_price = getattr(response, "limit_price", None) or getattr(response, "price", None) + stop_price = None
294-312: 🎯 Functional Correctness | ⚡ Quick winNo validation that
limit_price/stop_pricewas actually provided before submission.For
order_type in ("limit", "stop"),pricecan remainNoneif the caller built the order without a limit/stop price, and it's passed straight through toself.api.orders.place(..., price=None, ...). A clear pre-flight check would surface this as an actionableorder.set_error(...)instead of relying on whatever the remote API returns for a null price.🔧 Proposed fix
price = None if order_type == "limit": price = float(order.limit_price) if order.limit_price is not None else None elif order_type == "stop": price = float(order.stop_price) if order.stop_price is not None else None + if order_type in ("limit", "stop") and price is None: + msg = f"Order type '{order_type}' requires a price but none was provided." + logger.error(_colored(msg, "red")) + order.set_error(msg) + return order
1-65: LGTM!Also applies to: 107-225, 255-269, 334-529
301-317: 🩺 Stability & AvailabilityNo change needed.
TickerAllclients are created without a custom timeout, and the wrapped Python client useshttpx, which has a default request timeout for network operations.> Likely an incorrect or invalid review comment.lumibot/brokers/__init__.py (1)
17-17: LGTM!
- Read the documented TICKERALL_API_KEY / TICKERALL_ACCOUNT_ID / TICKERALL_BASE_URL environment variables: credentials now fall back from the config dict to os.environ, so the documented env-var path actually authenticates and selects an account. - Do not permanently cache a transient symbol-list fetch failure. On error, leave the cache unresolved so the next call retries, instead of disabling symbol resolution for the whole process lifetime. - Resolve a departed pending order as filled vs canceled from its terminal state in order history (deal_count > 0 means it produced a fill), not from whether a position exists. A fill that nets a netting position to exactly zero removes the position from the snapshot, which the position-existence check would have mis-reported as a cancel. Falls back to the position check only when history is unavailable. Adds regression tests for env-var credentials, the symbol-fetch retry, and the net-zero pending fill/cancel resolution (28 tests, all passing).
|
Thanks for the review - all four addressed in the latest commit:
Full suite is 28 tests, all passing, and the change is live-verified end-to-end against a demo account. |
1 participant
What this adds
An additive broker + data source for the hosted TickerAll MetaTrader 5 API, so a Lumibot strategy can trade any MetaTrader 5 account (Forex, metals, indices, CFDs, crypto) on any operating system with no local MetaTrader 5 terminal installed.
This addresses the MT5 integration request in #977, and goes a step beyond it: the issue suggested "the official MetaTrader5 Python library" (which is Windows-only and needs a running terminal), whereas this runs anywhere through a hosted API.
Fully additive (no existing code changed)
lumibot/brokers/tickerall.py-TickerAll(Broker)lumibot/data_sources/tickerall_data.py-TickerAllData(DataSource)docsrc/brokers.tickerall.rst- usage documentationtests/test_broker_tickerall.py- unit teststickeralldeclared as an optional extra:pip install lumibot[tickerall]The broker and data source are lazy-loaded through the existing registry maps, so nothing is imported unless the broker is actually used.
Design
ccxtfor the hosted-API shape,bitunixfor thePollingStreamfill loop).stop_limitandtrailing_stopare rejected with a clear message rather than being silently mishandled.None, never fabricated data (per the repo's RULE fixed alpaca timestamps (not tested) #1).Verification
pytest.importorskip, so they skip cleanly when the optionaltickerallpackage is not installed (CI stays green either way).Strategythrough a realTraderagainst demo accounts, exercising every user-facing surface (historical bars across timeframes, last price, quotes, cash / portfolio value / positions, market + limit + stop orders,modify_order,cancel_order,close_position,sell_all, and theon_filled_order/on_canceled_ordercallbacks) on both a netting and a hedging account.Known limitations / future work
get_historical_account_valuereturnsNone(as the CCXT broker does) - no equity-curve history yet.Closes #977
Summary by CodeRabbit