Skip to content

Schwab token tweaks #886

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Schwab token tweaks #886

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
afcdbea
Schwab token tweaks
tony-gutierrez Nov 2, 2025
155565b
Add conditional to refresh functionality
tony-gutierrez Nov 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 25 additions & 11 deletions lumibot/brokers/schwab.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import base64
import json
import os
import re
Expand Down Expand Up @@ -254,19 +255,31 @@ def _update_token(updated_token):
# Build kwargs for token refresh – only include client_secret if it actually exists
refresh_kwargs = {
"client_id": api_key,
"grant_type": "refresh_token",
# "grant_type": "refresh_token", #breaks refresh in oauth2session as it creates a dupe param type error
}
client_secret_env = config.get("SCHWAB_APP_SECRET") or os.environ.get("SCHWAB_APP_SECRET")
if client_secret_env:
refresh_kwargs["client_secret"] = client_secret_env

#add expires_at to token_dict_for_session. This is needed for the auto_refresh to work. Otherwise oauth2session always thinks it expires 30min from startup
token_dict_for_session['expires_at'] = int(token_dict_for_session['issued_at']/1000 + (token_dict_for_session['expires_in']) - 30) #30 second buffer
Copy link
Contributor

@korbit-ai korbit-ai bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repeated dictionary key lookups category Performance

Tell me more
What is the issue?

Inefficient repeated dictionary key lookups when calculating expires_at timestamp.

Why this matters

The code performs multiple dictionary lookups for the same keys, which is slower than storing the values in local variables first.

Suggested change ∙ Feature Preview

Store dictionary values in local variables before calculation:

issued_at = token_dict_for_session['issued_at']
expires_in = token_dict_for_session['expires_in']
token_dict_for_session['expires_at'] = int(issued_at/1000 + expires_in - 30)
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Copy link
Contributor

@korbit-ai korbit-ai bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Complex Token Expiration Calculation category Readability

Tell me more
What is the issue?

Complex one-liner with magic numbers and unclear calculation logic for token expiration.

Why this matters

The calculation's purpose and the significance of dividing by 1000 and subtracting 30 is not immediately clear, making future maintenance risky.

Suggested change ∙ Feature Preview
# Convert issued_at from milliseconds to seconds
issued_at_seconds = token_dict_for_session['issued_at'] / 1000
# Add expiration time (in seconds)
expiration_time = issued_at_seconds + token_dict_for_session['expires_in']
# Subtract buffer time (30 seconds) for safety
EXPIRATION_BUFFER_SECONDS = 30
token_dict_for_session['expires_at'] = int(expiration_time - EXPIRATION_BUFFER_SECONDS)
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Copy link
Contributor

@korbit-ai korbit-ai bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect token expiration calculation category Functionality

Tell me more
What is the issue?

The expires_at calculation assumes issued_at is in milliseconds but divides by 1000, which may be incorrect if issued_at is already in seconds, leading to premature token expiration.

Why this matters

If issued_at is already in seconds (Unix timestamp), dividing by 1000 will result in a timestamp from 1970, causing the token to appear expired immediately and breaking authentication.

Suggested change ∙ Feature Preview

Check the format of issued_at before calculation:

# Check if issued_at is in milliseconds or seconds
if token_dict_for_session['issued_at'] > 1e10:  # Likely milliseconds
    issued_at_seconds = token_dict_for_session['issued_at'] / 1000
else:  # Likely seconds
    issued_at_seconds = token_dict_for_session['issued_at']

token_dict_for_session['expires_at'] = int(issued_at_seconds + token_dict_for_session['expires_in'] - 30)
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.


oauth_session = _OAS(
client_id=api_key,
token=token_dict_for_session,
auto_refresh_url="https://api.schwabapi.com/v1/oauth/token",
auto_refresh_kwargs=refresh_kwargs,
token_updater=_update_token,
)

def _refresh_token_hook(token_url, headers, body):
headers['Authorization'] = f"Basic {base64.b64encode(f'{api_key}:{client_secret_env}'.encode()).decode()}"
logger.info(f"[Schwab] Refreshing token with auth headers")
return token_url, headers, body
Copy link
Contributor

@korbit-ai korbit-ai bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant base64 encoding on token refresh category Performance

Tell me more
What is the issue?

Base64 encoding is performed on every token refresh instead of being cached.

Why this matters

The authorization header value is recalculated each time the hook is called, wasting CPU cycles on redundant encoding operations.

Suggested change ∙ Feature Preview

Pre-calculate and cache the authorization header:

auth_header = f"Basic {base64.b64encode(f'{api_key}:{client_secret_env}'.encode()).decode()}"

def _refresh_token_hook(token_url, headers, body):
    headers['Authorization'] = auth_header
    logger.info(f"[Schwab] Refreshing token with auth headers")
    return token_url, headers, body
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Copy link
Contributor

@korbit-ai korbit-ai bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclear Authorization Header Construction category Readability

Tell me more
What is the issue?

Inline function definition with complex base64 encoding operation makes the authorization logic hard to follow.

Why this matters

The base64 encoding of credentials mixed with function definition reduces readability and obscures the security-critical authorization logic.

Suggested change ∙ Feature Preview
def _create_basic_auth_header(api_key: str, client_secret: str) -> str:
    """Create a Basic Auth header value from credentials."""
    credentials = f'{api_key}:{client_secret}'
    encoded_credentials = base64.b64encode(credentials.encode()).decode()
    return f'Basic {encoded_credentials}'

def _refresh_token_hook(token_url, headers, body):
    headers['Authorization'] = _create_basic_auth_header(api_key, client_secret_env)
    logger.info("[Schwab] Refreshing token with auth headers")
    return token_url, headers, body
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Copy link
Contributor

@korbit-ai korbit-ai bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing null check for client_secret_env category Functionality

Tell me more
What is the issue?

The refresh token hook references client_secret_env which may be None if SCHWAB_APP_SECRET is not configured, causing a TypeError when trying to encode None.

Why this matters

When client_secret_env is None, the f-string will attempt to encode 'api_key:None' which will fail, breaking the token refresh mechanism and causing authentication failures.

Suggested change ∙ Feature Preview

Add a null check before using client_secret_env:

def _refresh_token_hook(token_url, headers, body):
    if client_secret_env:
        headers['Authorization'] = f"Basic {base64.b64encode(f'{api_key}:{client_secret_env}'.encode()).decode()}"
        logger.info(f"[Schwab] Refreshing token with auth headers")
    else:
        logger.warning(f"[Schwab] Cannot refresh token - SCHWAB_APP_SECRET not configured")
    return token_url, headers, body
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.


#create refresh hook. This is beacuse oa2session does not perform refreshes with auth headers, only with json bodies.
oauth_session.register_compliance_hook("refresh_token_request", _refresh_token_hook)

# NOTE: schwab-py >=1.6 removed the app_secret parameter from the Client constructor.
# Passing it raises: TypeError: BaseClient.__init__() got an unexpected keyword argument 'app_secret'.
# The secret is only needed when REFRESHING a token via the auth helpers, not when we already
Expand Down Expand Up @@ -1119,10 +1132,10 @@ def on_trade_event_error(order, error_msg):

def _run_stream(self):
self._stream_established()
logger.info(colored("Starting Schwab stream...", "green"))
try:
# Add check to ensure self.stream is initialized
if self.stream:
logger.info(colored("Starting Schwab stream...", "green"))
self.stream._run()
else:
# Log that the stream object wasn't created, likely due to init failure
Expand Down Expand Up @@ -1299,15 +1312,16 @@ def _submit_order(self, order: Order) -> Order:
order_id = None
try:
# Use the Schwab utility function to extract order ID if available
try:
from schwab.utils import Utils
# Create a Utils instance with required client and account_hash parameters
utils_instance = Utils(self.client, self.hash_value)
order_id = utils_instance.extract_order_id(response)
if order_id:
logger.info(colored(f"Extracted order ID using Utils.extract_order_id: {order_id}", "green"))
except (ImportError, Exception) as e:
logger.warning(colored(f"Could not use Utils.extract_order_id: {e}", "yellow"))
#this fails because we are using Oauth2Session which uses Requests which uses "ok", whereas schwab-py uses HTTPX which has (undocumented) "is_error" to check the response.
# try:
# from schwab.utils import Utils
# # Create a Utils instance with required client and account_hash parameters
# utils_instance = Utils(self.client, self.hash_value)
# order_id = utils_instance.extract_order_id(response)
# if order_id:
# logger.info(colored(f"Extracted order ID using Utils.extract_order_id: {order_id}", "green"))
# except (ImportError, Exception) as e:
# logger.warning(colored(f"Could not use Utils.extract_order_id: {e}", "yellow"))

# Fallback methods if Utils.extract_order_id fails
if not order_id and hasattr(response, 'headers') and 'Location' in response.headers:
Expand Down
2 changes: 1 addition & 1 deletion lumibot/data_sources/schwab_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ def create_schwab_client(api_key=None, secret=None, account_number=None):
os.makedirs(os.path.dirname(token_path), exist_ok=True)

# Create Schwab API client
client = easy_client(api_key, secret, 'https://127.0.0.1:8182', token_path)
client = easy_client(api_key, secret, os.environ.get( 'SCHWAB_BACKEND_CALLBACK_URL', 'https://127.0.0.1:8182'), token_path)
Copy link
Contributor

@korbit-ai korbit-ai bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent spacing in environment variable lookup category Functionality

Tell me more
What is the issue?

The environment variable lookup has an extra space after the opening parenthesis in os.environ.get( 'SCHWAB_BACKEND_CALLBACK_URL', ...) which creates inconsistent code formatting but does not affect functionality.

Why this matters

While this doesn't break functionality, inconsistent spacing in function calls can make code harder to read and maintain, and may not pass linting checks in some development environments.

Suggested change ∙ Feature Preview

Remove the extra space after the opening parenthesis:

client = easy_client(api_key, secret, os.environ.get('SCHWAB_BACKEND_CALLBACK_URL', 'https://127.0.0.1:8182'), token_path)
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Copy link
Contributor

@korbit-ai korbit-ai bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent Function Call Spacing category Readability

Tell me more
What is the issue?

Inconsistent spacing in the os.environ.get() call makes the line harder to read.

Why this matters

Inconsistent spacing in function calls can make code harder to scan and understand at a glance, especially in configuration-related code where precision is important.

Suggested change ∙ Feature Preview
client = easy_client(api_key, secret, os.environ.get('SCHWAB_BACKEND_CALLBACK_URL', 'https://127.0.0.1:8182'), token_path)
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Copy link
Contributor

@korbit-ai korbit-ai bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repeated environment variable lookup category Performance

Tell me more
What is the issue?

The os.environ.get() call is executed on every invocation of create_schwab_client(), even when the environment variable is not set and the default value is used.

Why this matters

This creates unnecessary overhead by performing a dictionary lookup in os.environ on each function call, which could be avoided by caching the result or using a module-level constant for better performance in frequently called functions.

Suggested change ∙ Feature Preview

Cache the environment variable value at module level or use a constant:

# At module level
SCHWAB_CALLBACK_URL = os.environ.get('SCHWAB_BACKEND_CALLBACK_URL', 'https://127.0.0.1:8182')

# In the function
client = easy_client(api_key, secret, SCHWAB_CALLBACK_URL, token_path)
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.


logger.info(colored("Successfully created Schwab client", "green"))
return client
Expand Down
Loading