You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Automatically subscribing a subaccount subscription tier (>=200) client to all subaccounts upon connection has multiple issues:
It violates standard WebSocket streaming expectations that subscriptions must be requested before the server begins publishing them.
If new subaccounts are added after the client connects, the client will not be subscribed to the new subaccounts.
It subjects all clients to receiving all subaccounts, even if they only desire specific subaccounts. This can cause performance issues on the client.
It forces the server to send all subaccounts to every client, which will cause performance issues on the server.
This PR adds a "subscribe_subaccounts" which must be sent by the client before all subaccounts are subscribed, and which includes new subaccounts when they are added.
Related Issues (JIRA)
[Reference any related issues or tasks that this pull request addresses or closes.]
Checklist
I have tested my changes on testnet.
I have updated any necessary documentation.
I have added unit tests for my changes (if applicable).
If there are breaking changes for validators, I have (or will) notify the community in Discord of the release.
Reviewer Instructions
[Provide any specific instructions or areas you would like the reviewer to focus on.]
Definition of Done
Code has been reviewed.
All checks and tests pass.
Documentation is up to date.
Approved by at least one reviewer.
Checklist (for the reviewer)
Code follows project conventions.
Code is well-documented.
Changes are necessary and align with the project's goals.
No breaking changes introduced.
Optional: Deploy Notes
[Any instructions or notes related to deployment, if applicable.]
This PR refactors the WebSocket subscription model from automatic subscription to an opt-in approach for subaccount dashboard updates. It removes the auto-subscription logic that ran during authentication and introduces new message types (subscribe_subaccounts, unsubscribe_subaccounts) to allow clients to explicitly request subscriptions. The changes also add authorization checks to ensure clients can only subscribe to subaccounts matching their entity hotkey.
✅ Strengths
Better architectural design: Explicit subscription model follows WebSocket best practices and gives clients more control
Authorization improvement: The hotkey_matches() method and validation logic prevent unauthorized access to other entities' subaccounts
Dynamic subscription handling: The update queue processing now handles lazy subscription creation (lines 443-448), enabling auto-subscription to newly created subaccounts
Cleaner authentication flow: Removing auto-subscription logic simplifies the connection handshake
Consistent error handling: Error responses now consistently use message_type variable for the action field
⚠️ Concerns
CRITICAL - Race Condition Risk (lines 443-448)
if ((subscriptionisNone) andclient.subscribe_all_dashboard_updatesandclient.hotkey_matches(synthetic_hotkey)):
subscription=DashboardSubscription()
client.dashboard_subscriptions[synthetic_hotkey] =subscription
Multiple threads could simultaneously access/modify client.dashboard_subscriptions from the thread pool
The check-then-set pattern is not atomic
Impact: Potential data corruption or race conditions under high load
Recommendation: Add thread-safe locking (e.g., threading.Lock) around dashboard_subscriptions modifications
Using startswith() is vulnerable to prefix collisions
Example: entity_hotkey="abc" would match "abc123" AND "abc_xyz"
Impact: Potential unauthorized access to subaccounts
Recommendation: Use exact matching or verify the delimiter pattern (e.g., hotkey == entity_hotkey or hotkey.startswith(entity_hotkey + "_"))
MODERATE - Missing Null Check (line 106)
If hotkey parameter is None, this will raise an AttributeError
Recommendation: Add null check: return self.entity_hotkey and hotkey and hotkey.startswith(self.entity_hotkey)
MODERATE - Memory Leak Potential (line 758)
client.dashboard_subscriptions.clear()
When unsubscribing from all subaccounts, subscriptions are cleared but subscribe_all_dashboard_updates flag might cause them to be recreated
No cleanup mechanism for orphaned subscriptions if clients disconnect without unsubscribing
Recommendation: Ensure cleanup happens in the disconnect/finally block
MODERATE - Inconsistent State Management (lines 757-759)
unsubscribe_subaccounts clears all subscriptions including those manually added via subscribe_subaccount
This might be unexpected behavior for clients who mixed subscription methods
Recommendation: Document this behavior or track subscriptions separately
💡 Suggestions
Add Request Validation (lines 738+)
elifmessage_type=="subscribe_subaccounts":
# Add validation for duplicate subscription requestsifclient.subscribe_all_dashboard_updates:
awaitwebsocket.send(json.dumps({
"type": "subscription_status",
"status": "info",
"action": message_type,
"message": "Already subscribed to all subaccounts"
}))
continue
Add Metrics/Monitoring
Track the number of active subscriptions per client
Monitor subscription churn rate
Log when clients hit the dynamic subscription path (line 443)
Improve Error Messages
Line 700: Add the client's actual tier to the error message for debugging
Line 722: Include the client's entity_hotkey in the unauthorized error for troubleshooting
Documentation Gaps
The subscribe_all_dashboard_updates field (line 103) lacks a docstring
The new message types should be documented in API documentation
The hotkey_matches() method needs a docstring explaining the matching logic
Consider Rate Limiting
Clients could spam subscribe/unsubscribe messages
Add per-client rate limiting for subscription operations
Breaking Change Communication
The PR checklist mentions breaking changes notification, but this IS a breaking change
Clients expecting auto-subscription will break
Action Required: Update Discord notification checkbox and provide migration guide
🔒 Security Notes
Authorization Bypass Risk (HIGH)
The hotkey_matches() implementation using startswith() is a security vulnerability
Must be fixed before deployment
Entity Hotkey Trust (MEDIUM)
Line 594: entity_hotkey = self.api_key_to_alias.get(api_key)
Verify that api_key_to_alias is securely populated and cannot be manipulated
Consider adding validation that entity_hotkey format is expected
Information Disclosure (LOW)
Error messages reveal tier requirements and synthetic hotkey existence
This is likely acceptable but consider if information leakage is a concern
Thread Safety (HIGH)
As mentioned in concerns, concurrent dictionary access needs protection
Review all paths where dashboard_subscriptions is accessed/modified
📋 Additional Notes
Testing Recommendations:
Test concurrent subscribe/unsubscribe operations
Test the hotkey matching logic with edge cases (empty strings, special characters, similar prefixes)
Test behavior when new subaccounts are created while client is subscribed
Test cleanup on client disconnect
Test tier enforcement for all new message types
Load test the dynamic subscription creation path
Migration Path:
Provide clear documentation for clients on how to migrate from auto-subscription
Consider a deprecation period with warnings if feasible
Update client libraries/SDKs if any exist
Code Style:
Generally follows existing patterns well
Consider extracting subscription validation logic into helper methods to reduce duplication
Overall Assessment: The architectural direction is sound and addresses real performance/flexibility concerns. However, there are critical security and thread-safety issues that must be addressed before merging. The hotkey_matches() authorization logic needs immediate attention, and thread-safe access to shared state is essential for production stability.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Taoshi Pull Request
Description
Automatically subscribing a subaccount subscription tier (>=200) client to all subaccounts upon connection has multiple issues:
This PR adds a "subscribe_subaccounts" which must be sent by the client before all subaccounts are subscribed, and which includes new subaccounts when they are added.
Related Issues (JIRA)
[Reference any related issues or tasks that this pull request addresses or closes.]
Checklist
Reviewer Instructions
[Provide any specific instructions or areas you would like the reviewer to focus on.]
Definition of Done
Checklist (for the reviewer)
Optional: Deploy Notes
[Any instructions or notes related to deployment, if applicable.]
/cc @mention_reviewer