Add missing blockHeaderSchema properties
#6243
Conversation
Bundle StatsHey there, this message comes from a github action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
Deploying with
|
| Latest commit: |
b50a093
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3884ef45.web3-js-docs.pages.dev |
| Branch Preview URL: | https://wyatt-4-x-6198-missing-info.web3-js-docs.pages.dev |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## 4.x #6243 +/- ##
=======================================
Coverage 88.72% 88.72%
=======================================
Files 198 198
Lines 7608 7609 +1
Branches 2094 2094
=======================================
+ Hits 6750 6751 +1
Misses 858 858
Flags with carried forward coverage won't be shown. Click here to find out more.
|
spacesailor24
requested review from
avkos,
jdevcs,
luu-alex and
Muhammad-Altabba
as code owners
| export const blockHeaderSchema = { | ||
| type: 'object', | ||
| properties: { | ||
| author: { |
There was a problem hiding this comment.
some of these fields are not aligned with EL specs, there can be possibilities:
- EL specs not updated
- geth specific fields
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.
@jdevcs What EL spec are you looking at? I've been using this one and don't even see eth_subscribe listed
Regardless, 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:
- author
- totalDifficulty
- size
- excessDataGas
- mixHash
- transactions
- uncles
- withdrawals
There was a problem hiding this comment.
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 no eth_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.
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?
I'll suggest lets have additional types as optional and add doc comments that these are specific for NM.
* Add missing blockHeaderSchema properties * Update CHANGELOGs * Update BlockHeaderOutput * Refactor subscript new heads integration test * Add try/catch to new heads sub test for faster failing * Init e2e new heads subscription test * Debug failing tests * Debugging failing tests for cypress
* Add missing blockHeaderSchema properties * Update CHANGELOGs * Update BlockHeaderOutput * Refactor subscript new heads integration test * Add try/catch to new heads sub test for faster failing * Init e2e new heads subscription test * Debug failing tests * Debugging failing tests for cypress
closes #6198