-
Notifications
You must be signed in to change notification settings - Fork 173
lukas/websocket improvements #1807
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.
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
base: master
Are you sure you want to change the base?
Conversation
|
||
**How it works:** | ||
1. Initially subscribes to WebSocket updates | ||
2. If no WebSocket data is received for `resubTimeoutMs` (30s), switches to polling mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this only true, if usePollingInsteadOfResub
is true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
your right
// Start notification loop without awaiting | ||
this.handleNotificationLoop(subscription); | ||
// Start notification loop with the subscription promise | ||
this.handleNotificationLoop(subscriptionPromise); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels weird to be accepting a promise for a function
maybe could use .then
instead and pass the resolved Promise to this.handleNotificationLoop
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok it is totally weird but I did this for performance haha. I didn't want to wait for the WS subscription to succeed in the overall .subscribe() path because it slows down the app from rendering, while we can put this off to a separate thread because we do an initial fetch of the data anyway.
this.subscriptionPromiseResolver(true); | ||
|
||
// delete initial data | ||
this.removeInitialData(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be here? this.initialPerpMarketAccountData and this.initialSpotMarketAccountData are set just above, but we are deleting it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk tbh 😄 I was just following the original pattern from v1 which does this as well. Afraid to remove it and cause a regression. WDYT should we try removing it?
/** | ||
* This is a no-op method that always returns true. | ||
* Unlike the previous implementation, we don't need to manually subscribe to individual perp markets | ||
* because we automatically receive updates for all program account changes via a single websocket subscription. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something that im unsure - when you mention single websocket subscription for all program account changes, this means a single WS subscription for all perp market account changes right? both spot markets and perp markets use a different websocket?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could technically use a single websocket for both perp and spot markets. But in this case basically yeah we use one WS connection for all perp market account changes. I think it might be fine to keep them separate. There are benefits to keep them separate like isolation if one of the connections has a problem, it doesn't affect state for the other states.
|
||
await this.unsubscribeFromMarketAccounts(); | ||
await this.unsubscribeFromSpotMarketAccounts(); | ||
await this.unsubscribeFromOracles(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can these use Promise.all()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call
@@ -51,6 +71,16 @@ export class WebSocketProgramAccountSubscriberV2<T> | |||
private accountsCurrentlyPolling: Set<string> = new Set(); // Track which accounts are being polled | |||
private batchPollingTimeout?: ReturnType<typeof setTimeout>; // Single timeout for batch polling | |||
|
|||
// For batching initial account fetches | |||
private accountsPendingInitialMonitorFetch: Set<string> = new Set(); // Track accounts waiting for initial monitor fetch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without much docs on this, it really confused me a lot, thinking why we are doing the fallback polling at the beginning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for sure let me add docs for this functionality
i think the idea is there, but its been pretty hard to review ngl... I don't think its a you problem btw, this whole SDK has been coded this way where each class contains too much logic. One suggestion is to ask Cursor to help abstract this class' logic into utility managers e.g. WebsocketSubscriberManager, FallbackPollingManager, InitialFallbackPollingManager etc. |
* websockets gill temp * feat: feature parity between gill version ws acct sub and reg one + optional passing into driftClient * fix: post rebase bugs and cleanup * chore: websocket account subscriber export * feat: logging string update on ws acct v2 * rm: useless logging * chore: cleanup ws subscriber v2 docs * chore: specific name on custom ws acct sub param * fix: post rebase again cleanup * fix: prettier fixed
0388104
to
7195770
Compare
I appreciate that and I think you are right. I've added some deeper docs on the Regarding adding utility manager classes, I definitely think some smaller classes/better code isolation and single responsibility could be very helpful. I am wondering if maybe we could start with adding real good docs on these classes, and then I could do a follow up to break down logic into shareable utility classes/functions. Since that would be a huge refactor and then potentially implicates adjusting other classes so I'd wanna make sure I did it real well. Although I could just give it a short in this PR. Up to you. Thanks so much for the review!!! |
Overview:
WebSocketDriftClientAccountSubscriberV2
)WebSocketProgramAccountSubscriberV2
under the hood which opens a single programAccountSubscribe WS connection for an account type.kit
orgill
depending on which npm you install. We are usinggill
here. There are some inherit websocket optimizations for memory management and stability.Notes:
WebSocketProgramUserAccountSubscriber
but that is not going to be utilized immediately on the UI. But it is added and ready to be used when we want.Commits: