Skip to content
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

Fix subscribe blocks #330

Merged
merged 6 commits into from
Mar 18, 2024
Merged

Conversation

0xc0ffeebabeeth
Copy link
Contributor

Motivation

Some RPC Providers (Alchemy and Reth were tested) don't have the "uncles" field in their newHeads subscription, this breaks the subscribe_blocks() stream.

Solution

Providing the serde(default) flag on the uncles field allows graceful handling of both existing and missing uncle fields in the subscription response.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Some RPC Providers (Alchemy and Reth were tested) don't have the "uncles" field in their newHeads subscription, hence make it optional
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Ref https://t.me/ethers_rs/36155
Pending @mattsse or @prestwich, should this also be skip_serializing_if?

@prestwich
Copy link
Member

prestwich commented Mar 16, 2024

Thanks! Ref https://t.me/ethers_rs/36155 Pending @mattsse or @prestwich, should this also be skip_serializing_if?

Given that we are post-merge, does it now ALWAYS omit that field in the newHeads objects? The field is present in getBlockByNumber even for recent blocks, so the newHeads notification object is NOT a valid block object according to rpc spec? cc @mattsse what's reth do?

@prestwich
Copy link
Member

my initial take is that we should NOT do skip_serializing_if, and just leave it as default

@DaniPopes
Copy link
Member

@prestwich
Copy link
Member

Actually should this be Header instead of block?

this is quite likely

@0xc0ffeebabeeth
Copy link
Contributor Author

The newHeads object does seem to contain withdrawals though, which aren't included in the Header object

@prestwich
Copy link
Member

so its a separate object? 😮‍💨

@DaniPopes
Copy link
Member

most consistent eth rpc api

@DaniPopes DaniPopes merged commit 7054d5e into alloy-rs:main Mar 18, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants