-
Notifications
You must be signed in to change notification settings - Fork 162
Add support for gzipping messages in ws subscriptions #2412
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: dev
Are you sure you want to change the base?
Conversation
…eston/ws-submit-improvments
…eston/gzip-support
Greptile OverviewGreptile SummaryAdded gzip compression support for WebSocket subscriptions via Key Changes:
Observations:
Confidence Score: 4/5
Important Files Changed
|
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.
3 files reviewed, 1 comment
Additional Comments (1)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/full-node/sov-sequencer/src/rest_api.rs
Line: 425:425
Comment:
Consider adding compression support to `axum_get_tx_ws` for consistency with other subscription endpoints
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
Codecov Report❌ Patch coverage is
Additional details and impacted files
🚀 New features to boost your workflow:
|
| break 'outer; | ||
| } | ||
|
|
||
| if !err.is_recoverable() { |
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.
send_compressed_bytes calls socket.feed(..), so this error might not be sent in some cases (in the uncompressed branch, we call socket.send) is that ok? Let's rename send_compressed_bytes to feed_compressed_bytes?
| Some(Ok(_)) => { | ||
| // Client sent an unexpected message - notify them it was ignored | ||
| trace!("Incoming WebSocket message but none was expected; notifying client"); | ||
| if let Err(err) = send_json(&mut socket, &ErrorObject { |
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.
Here we send an uncompressed error even if config.compress == true
bkolad
left a comment
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.
Approving, provided my comments are addressed.
Description
This PR adds support for gzipping messages on the events and transactions websockets using the
?compression=gzipquery param. When gzip is enabled...