Skip to content
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(server): improve too big batch response msg #1107

Merged
merged 3 commits into from
May 2, 2023

Conversation

niklasad1
Copy link
Member

No description provided.

@niklasad1 niklasad1 requested a review from a team as a code owner May 2, 2023 13:02
}

/// Helper to get a `JSON-RPC` error object when the maximum batch response size have been exceeded.
pub fn reject_too_big_batch_response(limit: usize) -> ErrorObjectOwned {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We could also provide extra insight into how many bytes the message contained, also we could state for clarity the units (ie bytes)

Some(format!("Request length {len} exceeded max limit of {limit} bytes"))

This info will also be useful in debugging some tests that are still exceeding the 100 char limit.

Although, I leave this up to you if it makes sense! 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a little weird as we terminate when the limit is exceeded as we are processing/building responses

so the entire response could be much bigger than len but for "batch request (input)" that makes sense

Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I expect a few tests are exceeding the limits of batches, once those are fixed we are clear to merge :D

@niklasad1 niklasad1 merged commit bef63ae into master May 2, 2023
@niklasad1 niklasad1 deleted the na-fix-bad-response-on-too-big-response branch May 2, 2023 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants