-
Notifications
You must be signed in to change notification settings - Fork 94
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
fix: increase websocket payload default max size #795
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe changes introduce a new constant Changes
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
xrpl/asyncio/clients/websocket_base.py (1)
19-19
: Clarify usage context for thePAYLOAD_MAX_SIZE
.Providing a short docstring or explanatory comment for the
PAYLOAD_MAX_SIZE
constant would help maintainers and extend the clarity of its intended usage. Consider adding context on why 16 MB was chosen and how it addresses the larger payload sizes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
xrpl/asyncio/clients/websocket_base.py
(2 hunks)
🔇 Additional comments (1)
xrpl/asyncio/clients/websocket_base.py (1)
94-96
: Ensure that the environment can handle large payloads.While raising the
max_size
parameter to 16 MB fixes the immediate issue, consider verifying that this change does not lead to unintended memory overhead or performance bottlenecks. Ensure there are appropriate tests or monitoring in place for large payload handling.
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.
Please update the changelog
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
10-11
: Enhance the changelog entry with more details.The changelog entry could be more informative by including the specific size increase and the motivation. Consider expanding it to:
- - Increase default maximum payload size for websocket client + - Increase default maximum payload size for websocket client from 1MB to 16MB to handle larger LedgerData responses
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md
(1 hunks)xrpl/asyncio/clients/websocket_base.py
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- xrpl/asyncio/clients/websocket_base.py
Added the requested changes. |
High Level Overview of Change
Certain requests (e.g.
LedgerData
) may return payloads that exceed the current maximum (2**20
).Type of Change
Did you update CHANGELOG.md?