Conversation
This commit fixes the style issues introduced in 652d8b0 according to the output from Black and isort. Details: None
style: format code with Black and isort
This commit fixes the style issues introduced in 206c787 according to the output from Black and isort. Details: None
style: format code with Black and isort
This commit fixes the style issues introduced in e5920fc according to the output from Black and isort. Details: None
style: format code with Black and isort
This commit fixes the style issues introduced in d5b9f53 according to the output from Black and isort. Details: None
style: format code with Black and isort
Assume only one account exists
Change account number for fetching positions
Handle missing MFA method in login process
Add HTTP logging with the debug attribute passed to FTSession.
Fix load cookies
- explanation to using mfa_secret
- detail of what is printed with some indentation
- history transactions limited to last december
- fix missing price for the dry run order: Error placing order: Bad Request : Wrong order price Reference code: 1062
- fix cancellation of dry_run order leading to an error:
Traceback (most recent call last):
File "/home/da/./test.py", line 104, in <module>
cancel = ft_accounts.cancel_order(order_conf["result"]["order_id"])
~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^
KeyError: 'order_id'
- dry_run/preview option was not setting the price despite the preceding text, it was using a MARKET type of price, leading to an error if not executed in market hours: Option Market orders are not accepted outside of regular market hour. Please enter a limit order.
Several improvements to test.py
…selected duration is no longer available. Please update your app to the latest version to access the new extended hours features. Reference code: 1567`
PRE_MARKET&AFTER_MARKET orders have been replaced by OVERNIGHT
Add OHLC data retrieval to fix #64
There was a problem hiding this comment.
Pull request overview
This PR updates the Firstrade client to use Firstrade’s newer api3x.firstrade.com JSON endpoints (replacing prior HTML/XML scraping), adds MFA/TOTP support and options/OHLC functionality, and introduces Ruff configuration.
Changes:
- Reworked session/login flow with MFA support, cookie persistence, and a request wrapper (
FTSession). - Replaced quote/order/positions/history logic with new JSON API endpoints; added options and OHLC support.
- Added Ruff configuration and updated packaging metadata/version.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
firstrade/account.py |
New session/login implementation, MFA handling, account data retrieval, and helper utilities. |
firstrade/order.py |
Replaced legacy HTML parsing with JSON order endpoints; added option-order support. |
firstrade/symbols.py |
Replaced BeautifulSoup quote parsing with JSON quote + added OHLC/options helpers. |
firstrade/urls.py |
Switched to api3x endpoint URLs and new headers/token helpers. |
firstrade/exceptions.py |
Added dedicated exception types for login/account/quote failures. |
test.py |
Updated example script to exercise new login/MFA, quotes, orders, OHLC, and options flows. |
setup.py |
Bumped version and adjusted dependencies/classifiers. |
pyproject.toml |
Added Ruff configuration and style extension. |
styles/ruff.toml |
Introduced Ruff formatting/lint baseline. |
.gitignore |
Replaced ignore rules with a catch-all ignore. |
README.md |
Updated quickstart/feature list and added disclaimer. |
.deepsource.toml |
Removed DeepSource configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| os.remove(path) | ||
| local, domain = email.split(sep="@") | ||
| masked_local: str = local[0] + "*" * 4 | ||
| domain_name, tld = domain.split(".") |
There was a problem hiding this comment.
_mask_email() assumes the domain contains exactly one dot (domain.split(".")), which will throw ValueError for valid addresses like user@mail.co.uk or domains with multiple subdomains. Use a safer split (e.g., split once from the right) and preserve the remainder to avoid crashing when masking emails.
| domain_name, tld = domain.split(".") | |
| domain_name, tld = domain.rsplit(".", 1) |
| if price_type in {PriceType.LIMIT, PriceType.STOP_LIMIT}: | ||
| data["limit_price"] = price | ||
| if price_type in {PriceType.STOP, PriceType.STOP_LIMIT}: | ||
| data["stop_price"] = stop_price |
There was a problem hiding this comment.
For PriceType.STOP / STOP_LIMIT, stop_price can currently be left as None, and the code will still include stop_price: None in the request payload. This is very likely an invalid API request. Validate that stop_price is provided when required and raise a clear ValueError (or omit the field when not applicable).
| include = ["src/*"] | ||
| exclude = ["src/vendors/*", "guides/*"] |
There was a problem hiding this comment.
Ruff is configured with include = ["src/*"], but this repository’s Python packages live under firstrade/ (and test.py is at the repo root). As written, Ruff will lint/format nothing. Update the include/exclude globs to match the actual project layout (or move code under src/).
| include = ["src/*"] | |
| exclude = ["src/vendors/*", "guides/*"] | |
| include = ["firstrade/**/*.py", "test.py"] | |
| exclude = ["guides/*"] |
| def delete_cookies(self) -> None: | ||
| """Delete the session cookies.""" | ||
| path: Path = Path(self.profile_path) / f"ft_cookies{self.username}.json" if self.profile_path is not None else Path(f"ft_cookies{self.username}.json") | ||
| path.unlink() |
There was a problem hiding this comment.
delete_cookies() unconditionally calls path.unlink(), which will raise FileNotFoundError if the cookie file doesn't exist (e.g., first run, or already deleted). Consider using unlink(missing_ok=True) (Py3.8+) or checking path.exists() before unlinking to make this method safe to call.
| path.unlink() | |
| path.unlink(missing_ok=True) |
| sec_type (str): The security type of the symbol. | ||
| tick (str): The tick size of the symbol. | ||
| bid (int): The bid price for the symbol. | ||
| bid_size (int): The size of the bid. | ||
| ask (int): The ask price for the symbol. | ||
| ask_size (int): The size of the ask. |
There was a problem hiding this comment.
The SymbolQuote docstring lists several fields as int/bool (e.g., bid, ask, bid_size, has_option), but the implementation stores them as str (and has_option is annotated as str while the doc says bool). This mismatch makes the public API confusing; either coerce to the documented types or update the docstring/type hints to match the actual values returned by the API.
| def __init__(self, session: requests.Session) -> None: | ||
| """Initialize a new instance of the FTAccountData class. | ||
|
|
||
| Args: | ||
| session (requests.Session): The session object used for making HTTP requests. | ||
|
|
||
| """ | ||
| Initializes a new instance of the FTAccountData class. | ||
| self.session: requests.Session = session | ||
| self.all_accounts: list[dict[str, object]] = [] | ||
| self.account_numbers: list[str] = [] | ||
| self.account_balances: dict[str, object] = {} | ||
| response: requests.Response = self.session._request("get", url=urls.user_info()) | ||
| self.user_info: dict[str, object] = response.json() | ||
| response: requests.Response = self.session._request("get", urls.account_list()) |
There was a problem hiding this comment.
FTAccountData is annotated as taking a requests.Session, but it immediately calls session._request(...) (a method that requests.Session does not have). This is misleading for users/type-checkers and will fail at runtime if a real requests.Session is passed. Update the type hints (and docstring) to accept FTSession (or a Protocol defining _request).
| def get_orders(self, account: str) -> list[dict[str, object]]: | ||
| """Retrieve existing order data for a given account. | ||
|
|
||
| Args: | ||
| account (str): Account number of the account to retrieve orders for. | ||
|
|
||
| Returns: | ||
| list: A list of dictionaries, each containing details about an order. | ||
|
|
||
| """ | ||
| response = self.session._request("get", url=urls.order_list(account)) | ||
| return response.json() |
There was a problem hiding this comment.
get_orders() is annotated to return list[dict[str, object]], but it returns response.json() directly (which is typically a dict for these APIs, and is treated as such in test.py). Align the return type with the actual response shape (or parse out the list) to avoid confusing callers and type checkers.
| for k, v in node.items(): | ||
| _walk(node=v, path=[*path, str(object=k)]) | ||
| elif isinstance(node, list): | ||
| for i, v in enumerate(iterable=node): |
There was a problem hiding this comment.
In get_balance_overview(), enumerate(iterable=node) is not valid (the first parameter of enumerate is positional-only). This will raise TypeError at runtime when walking lists. Call enumerate(node) instead.
| for i, v in enumerate(iterable=node): | |
| for i, v in enumerate(node): |
| def access_token() -> str: | ||
| """Access token for FirstTrade API.""" | ||
| return "833w3XuIFycv18ybi" |
There was a problem hiding this comment.
urls.access_token() returns a hard-coded access token string. Committing tokens/keys in source is a security risk and makes rotation difficult. Load this value from an environment variable or configuration, and fail with a clear error if it’s not provided.
| self.start_of_day: Optional[int] = result.get("startOfDay") | ||
| self.ohlc_raw: list = result["ohlc"] | ||
| self.vol_raw: list = result.get("vol", []) | ||
|
|
||
| self.candles: List[Tuple[int, float, float, float, float, int]] = [] | ||
|
|
||
| self._parse_ohlc_and_volume() |
There was a problem hiding this comment.
SymbolOHLC uses Optional, List, Tuple, and Dict type annotations but this module only imports Any. Without from __future__ import annotations, these names are evaluated at import time and will raise NameError. Import the missing types from typing (or enable postponed evaluation) so the module can be imported.
No description provided.