-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add missing blockHeaderSchema
properties
#6243
Merged
Merged
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
0b35b29
Add missing blockHeaderSchema properties
spacesailor24 c7a3bd2
Update CHANGELOGs
spacesailor24 2525816
Update BlockHeaderOutput
spacesailor24 4878762
Refactor subscript new heads integration test
spacesailor24 13db64d
Add try/catch to new heads sub test for faster failing
spacesailor24 6992ccd
Init e2e new heads subscription test
spacesailor24 111116b
Debug failing tests
spacesailor24 c00b16a
Merge branch '4.x' into wyatt/4.x/6198-missing-info-block-headers
spacesailor24 b50a093
Debugging failing tests for cypress
spacesailor24 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
some of these fields are not aligned with EL specs, there can be possibilities:
so could you add an integration test , so when we add more EL clients we can catch issue earlier.
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.
@jdevcs What EL spec are you looking at? I've been using this one and don't even see
eth_subscribe
listedRegardless, these added fields don't exist on Geth's response, but are instead from Nethermind's. It's okay and actually necessary to have these fields in the
blockHeaderSchema
, because it's used to format responses. Meaning as is, any fields not mentioned in this schema will be dropped from the response before it's returned to the user. No error is caused if the fields are not present. This does, however, beg the question of how do we declare the BlockHeaderOutput type - do we include these properties as optional since they could be there if the connected RPC client uses them? Or do we only include the common ones as the type is declared now?The extra fields included by Nethermind that don't seem to be available using Geth:
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 wasnt able to find
author
field in EL Specs https://github.com/ethereum/execution-apis/blob/4da6a02f094d9d8c826272f6ceac2d751866f132/src/schemas/block.yaml#L22 in block schema at first place, and later there is noeth_subscribe
in EL specs ( as you found as well in specs doc).If we include integration tests ( we will need to have a check for Nethermind in tests and expect that these additional fields are present), if its breaking in future for Nethermind we will be able to detect. Currently we dnt have NM, so it can be skipped.
I'll suggest lets have additional types as optional and add doc comments that these are specific for NM.