Some optimizations for JSON #3498
Conversation
|
fwiw, i think there are some things in this PR that might want to be picked up here, like making the web3.providers.auto not circular importing on web3.providers, even if some of the other bits are not wanted. |
There was a problem hiding this comment.
Thanks for the PR @toppk. There's definitely some good changes in here! I don't think we should break these existing methods and I don't think strictly passing a string to the socket is really the way to go. I'm not against opening the type up there though and encoding when a string is passed.
Thoughts on the comments here? We can take these changes on in a week or two, otherwise feel free to update here and I'll give this another pass.
| def _json_list_errors(self, iterable: Iterable[Any]) -> Iterable[str]: | ||
| @classmethod | ||
| def _json_list_errors( | ||
| self, |
There was a problem hiding this comment.
| self, | |
| cls, |
| def _json_mapping_errors(self, mapping: Dict[Any, Any]) -> Iterable[str]: | ||
| @classmethod | ||
| def _json_mapping_errors( | ||
| self, |
There was a problem hiding this comment.
| self, | |
| cls, |
| @@ -164,7 +164,7 @@ async def make_batch_request( | |||
| # -- abstract methods -- # | |||
|
|
|||
| @abstractmethod | |||
| async def socket_send(self, request_data: bytes) -> None: | |||
| async def socket_send(self, request_data: str) -> None: | |||
There was a problem hiding this comment.
I think I'm good with opening up this type but this is a breaking change otherwise and I'm not keen on keeping it as string only. If you want to handle the string case, encoding the data if it's a string, I think that would be the best approach here.
We should make sure we update the docstring to reflect the change as well.
| @@ -158,7 +158,7 @@ async def make_request(self, method: RPCEndpoint, params: Any) -> RPCResponse: | |||
| f"Making request HTTP. URI: {self.endpoint_uri}, Method: {method}" | |||
| ) | |||
| request_data = self.encode_rpc_request(method, params) | |||
There was a problem hiding this comment.
This flow ends up in a strange "encoding" limbo where we have an encode_rpc_request that generates the request_data and yet we call request_data.encode() below. We should properly handle encoding within the encode_rpc_request method.
What was wrong?
Closes #3496
How was it fixed?
This makes some modest changes to JSON, to squeeze out some spaces, get rid of some unnecessary object instantiation. added some test cases because JSONBaseProvider was tested, not AsyncJSONBaseProvider. The big issue I wanted to solve was to send text to websockets package, so the logging there would be in text, and not binary (see issue). But i didn't change Legacy for that, which still is bytes.
Todo:
Cute Animal Picture