Check string Id in response #644
Conversation
|
Hey @whalelephant Thanks for your PR, the reason why we assert that the response ID must be a Can you elaborate why you need |
|
Hi @niklasad1
makes sense!
Sure! I think the main reason is that I cannot guarentee the server parses / deserializes to the same Id type in the response since Id supports both str and num. This all comes from the investigation related to deserializing the Thanks! |
|
https://github.com/AleoHQ/snarkOS/issues/1369 is not related this library but I see that you need to workaround this because it suffers from serde bug that it can't deduce the number type properly probably because of the FWIW, we are not supporting From the "clients" point-of-view the server must be reply with the same With that said if you could implement a configuration on the clients in the builders to support |
| if let Some(response_id) = response.id.as_number().copied() { | ||
| if response_id == *id.inner() { | ||
| Ok(response.result) | ||
| } else { | ||
| Err(Error::InvalidRequestId) | ||
| } | ||
| } else if let Some(response_id) = response.id.as_str() { | ||
| if response_id == id.inner().to_string() { | ||
| Ok(response.result) | ||
| } else { | ||
| Err(Error::InvalidRequestId) | ||
| } | ||
| } else { | ||
| Err(Error::InvalidRequestId) | ||
| } |
There was a problem hiding this comment.
This would have been much cleaner if it was a match on the enum variants.
| match rp.id.as_number().copied() { | ||
| Some(id) => { | ||
| let pos = match request_set.get(&id) { | ||
| Some(pos) => *pos, | ||
| None => return Err(Error::InvalidRequestId), | ||
| }; | ||
| responses[pos] = rp.result | ||
| } | ||
| None => match rp.id.as_str() { | ||
| Some(s) => { | ||
| let id: u64 = s.trim().parse().map_err(|_| Error::InvalidRequestId)?; | ||
| let pos = match request_set.get(&id) { | ||
| Some(pos) => *pos, | ||
| None => return Err(Error::InvalidRequestId), | ||
| }; | ||
| responses[pos] = rp.result | ||
| } | ||
| None => return Err(Error::InvalidRequestId), | ||
| }, |
|
Closing in favor of #659 |
Hi,
I would like to add support checking
Repsonse.idasId::Str(x)when client is parsing responses.Not sure if this is something already the works so please let me know. thanks!