RPC Error Revert Reason Cleanup

[reedsa]

What was wrong?

Closes #3053

How was it fixed?

Todo:

• Add entry to the release notes

Cute Animal Picture

[fselmo]

@reedsa I had mostly small nits (sorry, I can't help it... feel free to ignore them 😆). But I did have a couple of comments for us to think about. Let me know your thoughts. I can do another pass at this after we talk these through.

[fselmo]

This raises a warning for me coming from the type for id in RPCResponse. We define it as an int only. I think the id can be a string so maybe we need to change the type there to Union[int, str] to match.

I also don't think we should be validating a value of None here so loosely. Per the JSON-RPC 2.0 spec:

id
- This member is REQUIRED.
- It MUST be the same as the value of the id member in the Request Object.
- If there was an error in detecting the id in the Request object (e.g. Parse error/Invalid Request), it MUST be Null.

I think we grab the id and if it's None we make sure that there was an error as well.

        if "id" in response:
            response_id = response.get("id")
            if response_id is None and "error" not in response:
                _raise_bad_response_format(
                    response,
                    '"id" was null without error in response.',
                )
            elif not isinstance(response_id, (str, int)):
                _raise_bad_response_format(
                    response,
                    '"id" must be a string or integer.',
                )

This won't actually fail any of the current tests, so we should add a test that includes an "id" key in the response but does not include an "error" and validate the new exception is raised.

Unfortunately subscriptions in ethereum don't include an id in the response so this actually deviates from JSON-RPC 2.0. Which is why we keep the if "id" in response rather than making sure that it is always there. And if it's in the response it can only be null when there is an error.

Thoughts?

[reedsa]

Responses with an id may contain an error or a result so no need to enforce the check for an error within the "id" block.

I also saw a note in the "id" section of the response object of the spec that the value must be null when the request throws a parse error or invalid request. Unfortunately a bunch of the response types appear to lack an "id" entirely and seems like we are stuck with it deviating from the spec in this case.

The specific case you called out is caught in the final else of format_response, which is covered by a few tests like https://github.com/ethereum/web3.py/blob/main/tests/core/manager/test_response_formatters.py#L31

[fselmo]

I think I'm missing some context. Why can't we test that if an id key is present and the value is in null (EMPTY_VALUES = ("", "null", None)?) that there should be an error key present? That doesn't seem to be the case you linked to in your second link.

If we decide to keep this, the isinstance() check needs to check for (str, int, type(None)). But I believe the id type should be Optional[Union[int, str]] if we sometimes don't have an id. I'll make a comment there as well.

[reedsa]

My brain wasnt groking the error rule for some reason. It sounds good to me to enforce that case.

I read it as "if there is an error, the value of id must be NULL", therefore if id is anything but "null" it should raise an exception? Curious if we want to keep empty string and None as empty values here, especially since empty string could mean something different.

The spec for the requests also indicates if id is missing, the request is considered to be a message. I'm guessing the same is for responses and the way the logic is implemented it will be parsed the same as other errors, so no change needed for that case.

[fselmo]

I read it as "if there is an error, the value of id must be NULL", therefore if id is anything but "null" it should raise an exception?

Yeah I think we're on the same page. If id key is present and error is present, id value must be null. Put in another way, if id is null and there is no error, we raise since it can't be null any other time. My only question is what do they mean by null. They use NULL and Null in the text and null (not the string "null") in the examples below. My understanding is that json parsing into a Python object would yield None in those cases. So that's probably the only reasonable null value? Thoughts there?

Since the id can indeed be None when there's an error also present, that makes the typing an Optional[...].

If we're validating that, it could just look like the example block I posted above where we check if it is indeed None and there is no error, we throw... and then we only check for str or int cases below that since it can't be None beyond that first bit of logic. Does that make sense?

[fselmo]

Some new test cases arise from this.

1. There is an "id" key in the response with a value of None and no "error" key -> raises exception
2. There is an "id" key in the response with a value other than None and there is an error key with properly formatted error object -> raises exception
3. There is an "id" key in the response witha value of None and there is an error key with properly formatted error object -> does NOT raise exception

[fselmo]

Just a few remaining thoughts.

[fselmo]

I think I'm missing some context. Why can't we test that if an id key is present and the value is in null (EMPTY_VALUES = ("", "null", None)?) that there should be an error key present? That doesn't seem to be the case you linked to in your second link.

If we decide to keep this, the isinstance() check needs to check for (str, int, type(None)). But I believe the id type should be Optional[Union[int, str]] if we sometimes don't have an id. I'll make a comment there as well.

[reedsa]

I've wrapped up the last round of feedback, thank you! I think it is ready for 🚀 but happy to fix any remaining issues you might find.

[fselmo]

lgtm! Thanks for scrutinizing the specs with me 😅 ... this is a great addition