-
Notifications
You must be signed in to change notification settings - Fork 251
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
More spec-compliant handling of unknown fields in REST json #3647
Conversation
@@ -694,7 +722,7 @@ proc readValue*[BlockType: Web3SignerForkedBeaconBlock|ForkedBeaconBlock]( | |||
"ForkedBeaconBlock") | |||
data = some(reader.readValue(JsonString)) | |||
else: | |||
reader.raiseUnexpectedField(fieldName, "ForkedBeaconBlock") | |||
unrecognizedFieldWarning() |
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 will get noisy with lists - ie a list with 100k blocks during sync will result in 100k warning logs - the fact that an unrecognised field was hit should instead be passed up to the caller that can take appropriate action
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.
lgtm, except that serializers must never log (it's not the right layer for it)
I can imagine an API for notifying the caller, but implementing something like this would be a larger development effort. For this hotfix, we can either disable this warning (silently ignoring the fields) or we can assume that these warnings won't be encountered in practice (they can arise only when using the REST API, so the potential excessive output shouldn't be a security concern). |
I've changed the warning to |
I would tend towards complete disabling (or |
No description provided.