Introduce dedicated BlockHeader type for usage in newHeads subscription endpoint
#620
Conversation
m-Peter
requested review from
janezpodhostnik,
peterargue,
zhangchiqing and
ramtinms
as code owners
WalkthroughThe pull request introduces a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
tests/web3js/eth_streaming_test.js (2)
35-37: LGTM: Enhanced data capture and consistent variable usage.The changes improve the test by capturing the entire block object instead of just the transaction hash. This provides more comprehensive data for validation, aligning with the PR's objective.
Consider adding a comment explaining the significance of
testValues.lengthin the condition, e.g.:// Unsubscribe after receiving the expected number of blocks if (blocksHeaders.length === testValues.length) {
94-113: LGTM: Comprehensive block header validation added.The new assertions significantly enhance the test's robustness by validating all relevant properties of the block headers against the actual blockchain data. This thorough validation ensures that the
newHeadssubscription endpoint returns accurate and complete data, aligning perfectly with the PR's objectives.Consider adding error messages to the assertions to provide more context in case of failures. For example:
assert.equal(blockHeader.number, block.number, `Block number mismatch for block ${blockHeader.number}`);This will make debugging easier if an assertion fails in the future.
api/stream.go (1)
161-161: Evaluate the performance impact of computingLogsBloomThe comment suggests considering moving the computation of
LogsBloominto storage for performance reasons. If computingLogsBloombecomes a bottleneck:
- Analyze the performance impact of the current implementation.
- Consider caching or precomputing
LogsBloomduring block processing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- api/models.go (1 hunks)
- api/stream.go (3 hunks)
- bootstrap/bootstrap.go (0 hunks)
- tests/web3js/eth_streaming_test.js (4 hunks)
💤 Files with no reviewable changes (1)
- bootstrap/bootstrap.go
🧰 Additional context used
🔇 Additional comments (5)
tests/web3js/eth_streaming_test.js (4)
28-29: LGTM: Improved variable naming and comment clarity.The changes in these lines enhance the readability of the code by accurately reflecting the focus on block headers rather than transaction hashes. This aligns well with the PR's objective of implementing a dedicated
BlockHeadertype.
42-42: Minor grammatical improvement noted.The change from "submission" to "submissions" in the comment is a small but welcome improvement in accuracy.
90-93: LGTM: Consistent updates reflecting focus on block headers.The changes in these lines maintain consistency with the earlier modifications, removing the assertion for
blockTxHashesand updating the comment to focus on transaction hashes. This aligns well with the PR's objective of streamlining the response to include only necessary fields.
Line range hint
1-113: Overall assessment: Excellent enhancements to the test suite.The changes in this file significantly improve the test coverage for the new
BlockHeadertype and thenewHeadssubscription endpoint. The modifications align perfectly with the PR objectives, ensuring that the returned data structure matches the expected format and contains accurate information.Key improvements:
- Focus shifted from transaction hashes to complete block headers.
- Comprehensive validation of all relevant block header properties.
- Consistent updates throughout the test to reflect the new focus on block headers.
These changes will help ensure the reliability and correctness of the
eth_subscribe("newHeads")endpoint implementation.api/models.go (1)
294-310: LGTM: NewBlockHeaderstruct aligns with PR objectivesThe newly added
BlockHeaderstruct is well-structured and includes all the essential fields for an Ethereum block header. It aligns perfectly with the PR objective of introducing a dedicated type for thenewHeadssubscription endpoint. The use of appropriate types and consistent JSON tags ensures compatibility with Ethereum RPC responses.
m-Peter
force-pushed
the
mpeter/fix-streaming-new-block-headers-response
branch
from
cf91dc8 to
5648ef4
Compare
m-Peter
force-pushed
the
mpeter/fix-streaming-new-block-headers-response
branch
from
5648ef4 to
5e57a61
Compare
Closes: #524
Description
This will allow us to return only the necessary fields for
eth_subscribe("newHeads")subscription endpoint.For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
Release Notes
New Features
BlockHeadertype for structured representation of Ethereum block header data.StreamAPIto prepare block headers more effectively and independently of the blockchain API.StreamAPI.Bug Fixes
StreamAPIfor clearer messaging related to block header responses.Tests