-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Made Validation Middleware Pluggable And Added A Check For POA extraData #756
Conversation
This is what I've managed to do till now. However, the exception message is getting lost in the middle for reasons explained below
Since I raised web3.py/web3/utils/formatters.py Lines 72 to 73 in bf4ef09
@carver Should I add a new Exception class so that the code mentioned above doesn't hide the original exception? web3.py/tests/core/eth-module/test_poa.py Lines 10 to 16 in ced9cbd
|
tests are failing because of #753 |
I changed
I have updated the corresponding test. While I was looking at |
@voith I do think this belongs in the validation middleware. If you don't mind making that change.. |
No problem. I'll see how I can add it there! |
and maybe move the extraData printout to the end of the message:
|
update: will work on this today. did not get time over the weekend to work on this |
I finally had some time to work on this. I tried to make the validation middleware configurable like the pythonic middleware so that in future if more parameters need to validated it can be done easily. This is just a |
test failure is unrelated to this change. |
web3/middleware/validation.py
Outdated
def apply_validator_at_index(validator, at_index, web3, value): | ||
if at_index + 1 > len(value): | ||
raise IndexError( | ||
"Not enough values in iterable to apply formatter. Got: {0}. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blindly copy pasted: formatter -> validator
web3/middleware/validation.py
Outdated
if key in validators: | ||
try: | ||
yield key, validators[key](web3, item) | ||
except (TypeError, ValueError) as exc: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to remove this check as vaildators will raise their own exceptions on failure.
web3/middleware/validation.py
Outdated
|
||
|
||
transaction_params_validator = apply_validator_at_index( | ||
compose(transaction_normalizer, TRANSACTION_PARAMS_VALIDATORS), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is wrong. transaction_normalizer
here is specific to chainId
. This shouldn't be coupled with the logic for pluggable TRANSACTION_PARAMS_VALIDATORS
left some notes for myself to address later. |
web3/middleware/validation.py
Outdated
@curry | ||
@return_arg_type(3) | ||
def apply_validator_at_index(validator, at_index, web3, value): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these copy/pastes from the formatter methods can be removed, and use the formatter version directly (like apply_formatter_at_index
instead of defining this method). Bonus points for using the ones from eth-utils. Validators can just be a very specific type of formatter: it either returns the original, or raises an exception.
The whole construct_validator_middleware
looks like it can be removed and use construct_formatter_middleware
directly.
@carver web3.py/web3/middleware/validation.py Line 17 in 4ada39f
This doesn't conform with the existing formatter utilities and construct_formatting_middleware . So I had to make the following changes:
I have introduced a This addresses your previous comment for trying to reuse I had to introduce two new utilities The tests for this middleware are already are covered by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work on this!
I'm worried that this direction will lead to potentially a lot of new methods to maintain.
I added an idea for another direction, and am happy to hear more suggestions.
web3/middleware/formatting.py
Outdated
@@ -5,20 +5,27 @@ | |||
|
|||
def construct_formatting_middleware(request_formatters=None, | |||
result_formatters=None, | |||
error_formatters=None): | |||
error_formatters=None, | |||
request_formatters_with_web3=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking this to its natural conclusion means doubling the number of formatters arguments here, after adding result_formatters_with_web3
, and error_formatters_with_web3
. Let's look for a way that doesn't pack more complexity into this method.
One approach: I think we could curry the web3-aware formatters with the web3 argument only, and then pass them in as standard formatters. That way we can replace/reusing all that standard formatter middleware. Something like:
middleware/validation.py
# validator-specific implementation:
@curry
def validate_chain_id(w3, chainId):
if chainId == w3.net.chainId:
return chainId
else:
raise ...
def build_validators_with_web3(w3):
return dict(
request_formatters={
'eth_sendTransaction': validate_chain_id(w3),
}
}
validator_middleware = construct_web3_formatting_middleware(build_validators_with_web3)
middleware/formatting.py
from cytoolz import merge
def construct_formatting_middleware(
request_formatters=None,
result_formatters=None,
error_formatters=None):
def ignore_web3_in_standard_formatters(w3):
# This could use **kwargs from construct_formatting_middleware, but then
# you lose keyword name validation, so this seemed better.
return dict(
request_formatters=request_formatters,
response_formatters=response_formatters,
error_formatters=error_formatters,
)
return construct_web3_formatting_middleware(ignore_web3_in_standard_formatters)
def construct_web3_formatting_middleware(web3_formatters_builder)
def formatter_middleware(make_request, w3):
formatters = merge(
{
'request_formatters': {},
'result_formatters': {},
'error_formatters': {},
},
web3_formatters_builder(w3),
)
return apply_formatters(**formatters)
return formatter_middleware
@curry
def apply_formatters(method, params, request_formatters, result_formatters, error_formatters):
if method in request_formatters:
formatter = request_formatters[method]
formatted_params = formatter(params)
response = make_request(method, formatted_params)
else:
response = make_request(method, params)
if 'result' in response and method in result_formatters:
formatter = result_formatters[method]
formatted_response = assoc(
response,
'result',
formatter(response['result']),
)
return formatted_response
elif 'error' in response and method in error_formatters:
formatter = error_formatters[method]
formatted_response = assoc(
response,
'error',
formatter(response['error']),
)
return formatted_response
else:
return response
That also means that we can drop the web3
version of all the formatter applicators. One small downside is that most of the formatters would have to change from values to methods, for example:
def transaction_param_formatter(web3):
return apply_formatters_to_dict({'chainId': validate_chain_id(web3)})
def transaction_args_validator(web3):
return apply_formatter_at_index(transaction_param_formatter(web3), 0)
etc.
Anyway, I'm not convinced this is the right approach either. Happy to hear other ideas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very smart way of reusing the existing code.
web3/middleware/validation.py
Outdated
@@ -27,21 +38,62 @@ def validate_chain_id(web3, chain_id): | |||
) | |||
|
|||
|
|||
@curry | |||
def check_extradata_length(val, length): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is kind of straddling generic and specific. Since we have such a specific message we want to give, and don't have other generic uses for it yet, it's fine to just embrace the specificity here, like:
MAX_EXTRADATA_LENGTH = 32
def check_extradata_length(val):
...
if len(result) > MAX_EXTRADATA_LENGTH:
...
web3/utils/formatters.py
Outdated
@curry | ||
def apply_formatter_with_web3_at_index(validator, at_index, web3, value): | ||
_validator = curry(validator)(web3) | ||
return apply_formatter_at_index(_validator, at_index, value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of different formatter methods. Playing this through to the "end game" implies defining a duplicate of each of them for a web3-enabled version. That seems like it could cause a lot of code bloat in the future.
You did a great job of reusing most of the logic of the original method, though!
Thanks for the detailed review @carver. I'll get back to this in a day or two. |
eeb973e
to
02a82f2
Compare
I don't have a better idea, so I just followed yours. And sorry for the delay(TBH, limited time). It would have been great if this had made it to |
def build_validators_with_web3(w3): | ||
return dict( | ||
request_formatters={ | ||
'eth_sendTransaction': chain_id_validator(w3), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After having a look at https://github.com/ethereum/web3.py/pull/777/files#diff-1e8b0fe1b6f6e15899cb2f41637945bbL36, I'm not sure if I should remove these 3 lines below or If I should comment out line 67.
If I remove the lines below, it won't run the entire transaction_param_validator
chain, However It won't leave an example behind on how to use the validation middleware with w3 instance. Perhaps, I should just comment them out with a TODO
note
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One approach would be to leave the validator in, but short-circuit it if w3.net.chainId is None
. In that case, we assume chainId
is unknowable, so we just ignore its value in the transaction, and strip it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -18,7 +18,6 @@ | |||
pytest.param( | |||
lambda web3: 999999999999, | |||
False, | |||
marks=pytest.mark.xfail, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the approach on short-circuiting if chainId
is None
, this line should be left in.
fixed linting errors
I've split the PR into two commits: One for making the middleware configurable and the other for adding the POA check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, just a couple tweak suggestions. Let me know which of any of these you want to take on, and we'll get this merged!
def construct_formatting_middleware( | ||
request_formatters=None, | ||
result_formatters=None, | ||
error_formatters=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the style-fix!
web3/middleware/formatting.py
Outdated
}, | ||
web3_formatters_builder(w3), | ||
) | ||
return apply_formatters(**formatters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small readability suggestion: consider moving make_request
out of the formatters
keyword args, like:
return apply_formatters(make_request=make_request, **formatters)
web3/middleware/validation.py
Outdated
"The field extraData is %d bytes, but should be %d. " | ||
"It is quite likely that you are connected to a POA chain. " | ||
"Refer " | ||
"http://web3py.readthedocs.io/en/latest/middleware.html#geth-style-proof-of-authority " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably use stable
here instead of latest
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch!!
'chainId': apply_formatter_if( | ||
# Bypass `validate_chain_id` if chainId can't be determined | ||
lambda _: is_not_null(web3.net.chainId), | ||
validate_chain_id(web3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, this looks like a good solution that can stay in place for the long term 👍
web3/middleware/validation.py
Outdated
) | ||
|
||
|
||
extra_data_validator = apply_formatter_if(is_not_null, block_validator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is intended to be a full block validator, the name extra_data_validator
feels a bit off. Rather than come up with yet another name, it could be reasonable to combine this with the earlier block_validator
definition:
block_validator = apply_formatter_if(
is_not_null,
apply_formatters_to_dict(BLOCK_VALIDATORS),
)
Thanks! |
What was wrong?
see #712
How was it fixed?
A check for POA extra has been added with an error message directing to the docs.
Cute Animal Picture