-
Notifications
You must be signed in to change notification settings - Fork 9
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
Reuse block preparation logic in streaming API #617
Conversation
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- api/stream.go (4 hunks)
- bootstrap/bootstrap.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
bootstrap/bootstrap.go (1)
295-295
: LGTM: Approve the addition ofblockchainAPI
toNewStreamAPI
.The addition of
blockchainAPI
as a parameter toNewStreamAPI
is consistent with the PR objectives. This change allows the streaming API to reuse block preparation logic from the polling endpoint, which should address the issue with empty Bloom filters in block responses.To ensure consistency, let's verify the
NewStreamAPI
function signature:api/stream.go (2)
20-20
: Integration ofBlockChainAPI
intoStreamAPI
is appropriateThe addition of the
api
field toStreamAPI
, along with its initialization inNewStreamAPI
, allowsStreamAPI
to utilizeBlockChainAPI
for processing block responses. This enhances modularity and reusability by leveraging existing logic fromBlockChainAPI
.Also applies to: 33-33, 44-44
62-67
: Usage ofprepareBlockResponse
inNewHeads
improves block handlingThe
NewHeads
method now utilizess.api.prepareBlockResponse
to generate block responses, ensuring consistency between the streaming and polling endpoints. Error handling is appropriately implemented by returning a formatted error if the response preparation fails.
response, err := s.api.prepareBlockResponse(block, false) | ||
if err != nil { | ||
return err | ||
return fmt.Errorf("failed to get block response: %w", err) | ||
} | ||
|
||
return notifier.Notify(sub.ID, &Block{ | ||
Hash: h, | ||
Number: hexutil.Uint64(block.Height), | ||
ParentHash: block.ParentBlockHash, | ||
ReceiptsRoot: block.ReceiptRoot, | ||
Transactions: block.TransactionHashes, | ||
Uncles: []common.Hash{}, | ||
GasLimit: hexutil.Uint64(blockGasLimit), | ||
Nonce: gethTypes.BlockNonce{0x1}, | ||
Timestamp: hexutil.Uint64(block.Timestamp), | ||
BaseFeePerGas: hexutil.Big(*big.NewInt(0)), | ||
}) | ||
return notifier.Notify(sub.ID, response) |
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.
💡 Codebase verification
Bloom filter is not included in prepareBlockResponse
The prepareBlockResponse
method does not include the Bloom
field in the returned Block
struct, which is contrary to the PR objective.
- File:
api/stream.go
- Lines: 62-67
🔗 Analysis chain
Verify that the Bloom filter is included in block responses as intended
To fulfill the PR objective of including the Bloom filter in the streaming API's block responses, ensure that the prepareBlockResponse
method correctly incorporates the Bloom filter.
Run the following script to confirm that the Bloom
field is included in the block responses:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the Bloom filter is included in the block responses from the streaming API.
# Test: Search for the inclusion of the Bloom field in the block response preparation. Expect: The Bloom field should be part of the Block struct in the prepareBlockResponse method.
ast-grep --lang go --pattern $'func ($_ *BlockChainAPI) prepareBlockResponse($_, $_) {
$$$
return Block{
$$$
Bloom: $_,
$$$
}
}' --json
Length of output: 175
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.
its called LogsBloom
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.
@peterargue, thank you for the correction. I see that the Bloom filter is included as LogsBloom
in the prepareBlockResponse
method.
✏️ Learnings added
Learnt from: peterargue
PR: onflow/flow-evm-gateway#617
File: api/stream.go:62-67
Timestamp: 2024-10-18T19:26:37.250Z
Learning: In the `flow-evm-gateway` project, within the Go file `api/stream.go`, the `prepareBlockResponse` method includes the Bloom filter as the field `LogsBloom` in the returned `Block` struct.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
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 👏
Closes: #???
Related to: #524
Description
When using the go-ethereum client to stream block headers, I go this error:
error: hex string has length 0, want 512 for Bloom
This was because the code that produces the blocks that are returned does not include the bloom filter, and the stock go-ethereum client requires that the server returns a non-empy value.
The polling endpoint does include it as well as a few other fields. This PR updates the streaming endpoint to reuse the logic from the polling endpoint to generate the block responses.
For contributor use:
master
branchFiles changed
in the Github PR explorerSummary by CodeRabbit
Summary by CodeRabbit
New Features
StreamAPI
functionality to better handle block notifications and responses through integration with theBlockChainAPI
.Bug Fixes