Schwab token tweaks #886
Schwab token tweaks #886
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Complex Token Expiration Calculation ▹ view | |
 |
Inconsistent spacing in environment variable lookup ▹ view | |
|
Inconsistent Function Call Spacing ▹ view | |
 |
Repeated environment variable lookup ▹ view | |
|
Repeated dictionary key lookups ▹ view | |
|
Redundant base64 encoding on token refresh ▹ view | |
|
Unclear Authorization Header Construction ▹ view | |
|
Incorrect token expiration calculation ▹ view | |
|
Missing null check for client_secret_env ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| lumibot/data_sources/schwab_data.py | ✅ |
| lumibot/brokers/schwab.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
|
|
||
| # 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) |
There was a problem hiding this comment.
Inconsistent spacing in environment variable lookup 
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
💬 Looking for more details? Reply to this comment to chat with Korbit.
|
|
||
| # 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) |
There was a problem hiding this comment.
Inconsistent Function Call Spacing 
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
💬 Looking for more details? Reply to this comment to chat with Korbit.
|
|
||
| # 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) |
There was a problem hiding this comment.
Repeated environment variable lookup 
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
💬 Looking for more details? Reply to this comment to chat with Korbit.
| 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 |
There was a problem hiding this comment.
Repeated dictionary key lookups
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
💬 Looking for more details? Reply to this comment to chat with Korbit.
lumibot/brokers/schwab.py
Outdated
| 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 |
There was a problem hiding this comment.
Redundant base64 encoding on token refresh
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, bodyProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| 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 |
There was a problem hiding this comment.
Complex Token Expiration Calculation
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
💬 Looking for more details? Reply to this comment to chat with Korbit.
lumibot/brokers/schwab.py
Outdated
| 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 |
There was a problem hiding this comment.
Unclear Authorization Header Construction
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, bodyProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| 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 |
There was a problem hiding this comment.
Incorrect token expiration calculation
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
💬 Looking for more details? Reply to this comment to chat with Korbit.
lumibot/brokers/schwab.py
Outdated
| 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 |
There was a problem hiding this comment.
Missing null check for client_secret_env
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, bodyProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
|
I am not to concerned with the remaining Korbis recs. Schwab needs a rewrite to use the built in Client and eliminate a lot of cruff, but for now this gets you running and keeping a fresh token. |
2 participants
Fixes for schwab refresh tokens, schwab env var usage, etc