Fix batched reqs #315
Fix batched reqs #315
Conversation
| let mut batched_request = false; | ||
| let mut items = if let Value::Array(items) = json { | ||
| batched_request = true; | ||
| items | ||
| } else { | ||
| return Err(VersionMiddlewareError::InvalidRequestFormat); | ||
| vec![json] | ||
| }; |
There was a problem hiding this comment.
Could we do an helper function for batched_request like:
fn is_batched_request(json: &Value) -> Option<&Vec<Value>> {
match json {
Value::Array(items) => Some(items),
_ => None,
}
}There was a problem hiding this comment.
Not mandatory but it would be cleaner
There was a problem hiding this comment.
Hmm, although making is_batched_request return a optional vector looks non intuitive. I can maybe change it to get_items_from_request but then I would need to return a tuple - (items, is_batched_request)
|
Oh, thank you i've thought about this problem i believe this is also wrong in the case of batched requests |
You're right! Thanks for pointing it out. I have changed the code now so that requests without |
apoorvsadana
force-pushed
the
fix_batched_reqs
branch
from
277d6c5 to
77f7542
Compare
| @@ -248,7 +243,7 @@ where | |||
| let body = json!({ | |||
| "jsonrpc": "2.0", | |||
| "error": error, | |||
| "id": 0 | |||
| "id": null | |||
There was a problem hiding this comment.
ah so we can just null out that field, okay
i guess that makes sense
|
|
||
| if let Some(method) = json.get_mut("method").as_deref().and_then(Value::as_str) { | ||
| let new_method = format!("starknet_{}_{}", version.name(), method.strip_prefix("starknet_").unwrap_or(method)); | ||
| let json: Value = serde_json::from_slice(&whole_body)?; |
There was a problem hiding this comment.
i really don't like the fact that we're deserializing and reserializing the body like that 😔 this has a perf cost, especially with serde_json::Value
but that isnt the scope of this PR i guess..
There was a problem hiding this comment.
Ya we can do that in a new PR probably, I can create an issue if you like. Do you've any specific suggestions?
There was a problem hiding this comment.
i think it would require us to handle versions very differently
the problem i have with parsing requests to a serde_json::Value is that it looks like this https://docs.rs/serde_json/latest/serde_json/enum.Value.html
it does one heap allocation per object, string and array, recursively for the whole request and it's really unnecessary
also, their Object(Map<... variant is a BTreeMap, which is really really pushing it - it does a ton of allocs, as you would expect
parsing a concrete struct is much cheaper since nested structs and enums only require one allocation as a whole unless there's a Box or Vec or such in between, so it's not that we're deserializing it twice for nothing, we're actually deserializing it first in the most inefficient way first and then deserializing it again in an okay form
(it's not a problem of the serde_json library, their serde_json::Value type is very sensible, it's just that we should avoid it in such a critical path)
The version middleware doesn't handle batched requests (i.e. array of JSON RPC reuqests). This fixes that.
Resolves #313