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.

Sign up for GitHub

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

Jump to bottom

feat: network-parameterized block responses #1106

Merged
merged 5 commits into from
Aug 22, 2024
Merged

feat: network-parameterized block responses #1106

merged 5 commits into from
Aug 22, 2024

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Jul 26, 2024 •
edited
Loading

Motivation

Closes #267

Similarly to #1101 adds 2 new traits to network-primitives: BlockResponse and HeaderResponse. BlockResponse is generic over Header and Transaction and is expected to hold both a header and BlockTransactions<Self::Transaction>

Provider methods are updated to return N::BlockResponse instead of concrete Ethereum block.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@mattsse mattsse mentioned this pull request Aug 5, 2024
Closed
@klkvr klkvr mentioned this pull request Aug 19, 2024
Merged
3 tasks
mattsse
mattsse approved these changes Aug 21, 2024
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

okay, let's do this because this is the logical next step and we def want this.

only have two suggestions

crates/network-primitives/src/traits.rs
Comment on lines +53 to +57
/// Base fee per unit of gas (If EIP-1559 is supported)
fn base_fee_per_gas(&self) -> Option<u128>;

/// Blob fee for the next block (if EIP-4844 is supported)
fn next_block_blob_fee(&self) -> Option<u128>;
Copy link
Member

Choose a reason for hiding this comment

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

this is a good start, we can add more functions based on the spec:

https://ethereum.github.io/execution-apis/api-documentation/

crates/network-primitives/src/traits.rs
@@ -89,3 +117,20 @@ impl<T: ReceiptResponse> ReceiptResponse for WithOtherFields<T> {
self.inner.block_number()
}
}

impl<T: BlockResponse> BlockResponse for WithOtherFields<T> {
Copy link
Member

Choose a reason for hiding this comment

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

can we do the same for HeaderResponse, in case the header is WithOtherFields

crates/network/src/any/mod.rs
@@ -73,5 +73,7 @@ impl Network for AnyNetwork {

type ReceiptResponse = AnyTransactionReceipt;

type HeaderResponse = WithOtherFields<Header>;
type HeaderResponse = Header;
Copy link
Member

Choose a reason for hiding this comment

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

should this remain WithOtherFields<Header> ?

Copy link
Member Author

@klkvr klkvr Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Header is getting flattened into block in RPC, so if block has additional fields those will get consumed by header's others. There's no concept of body/header separation in RPC, so we can't really tell which field belongs to a header or to a body of the block

Copy link
Member

Choose a reason for hiding this comment

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

right, that makes sense

@klkvr klkvr requested a review from prestwich as a code owner August 21, 2024 09:45
mattsse
mattsse approved these changes Aug 21, 2024
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm, suggesting to remove the Option for number,hash,timestamp because I think these should exist?

crates/network-primitives/src/traits.rs Outdated
Comment on lines 53 to 60
/// Hash of the block
fn hash(&self) -> Option<BlockHash>;

/// Block number
fn number(&self) -> Option<u64>;

/// Block timestamp
fn timestamp(&self) -> Option<u64>;
Copy link
Member

Choose a reason for hiding this comment

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

I believe it would be reasonable to expect these to exist?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we have them as options due to pending blocks missing some of the fields? though looks like we have issues with deserializing them anyway:

$ cast block pending --rpc-url mainnet                             
Error: 
deserialization error: invalid type: null, expected 20 bytes, represented as a hex string of length 40, an array of u8, or raw bytes at line 1 column 3009

Copy link
Member

@mattsse mattsse Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

{
  "baseFeePerGas": "0x2afc0987",
  "blobGasUsed": "0x40000",
  "difficulty": "0x0",
  "excessBlobGas": "0x120000",
  "extraData": "0xd883010e05846765746888676f312e32322e34856c696e7578",
  "gasLimit": "0x1c9c380",
  "gasUsed": "0x1c9a5ba",
  "hash": null,
  "logsBloom": "0x8660992a000618a00a004414e3d8d3b4340d45000628214c02085c82cc2067b3c209081200821a40a200a219fca201b04e40a8dc1a1040d95083e16d202030606791431110e02501594c0029d01700a8885090002046421c209e02806f2401a700d9680083c522b21a4f455300814d181d4850010120cc616004f9140f8cb22246a080000029402010092308222810dd85e0704021951c08940c3072c772b06e920080a2654432141a3625e11b81c174025134ca002044200c6128a800852480b8c2ca4200060111481316404101d816c0441882d04008344214900209142044009b2042114812069068119000098502c3049412a0206c9c00e0b402c4985c46",
  "miner": null,
  "mixHash": "0x0000000000000000000000000000000000000000000000000000000000000000",
  "nonce": null,
  "number": "0x139faac",
  "parentHash": "0x765d646630f533da7c231dd1bc53c4ab616df25641b1c4631377dceb7dda04ff",
  "receiptsRoot": "0x1bdeed67943ae3aa143a69b0a9c0420f94ddfea35dc5066c0c132eb8a16ac444",
  "sha3Uncles": "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347",
  "size": "0x9d37",
  "stateRoot": "0x4f112179f19f5c3a46039a2bc0443ce695857b0263de48810df6e99849013061",
  "timestamp": "0x66c5da95",
  "totalDifficulty": null,
  "transactions": [
    {

Copy link
Member

@mattsse mattsse Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

meh, this complicates things a lot because for pending, there's a ton missing

this was for alchemy

so looks like we can't easily embed the header here anyway...

@klkvr klkvr mentioned this pull request Aug 21, 2024
Open
@mattsse mattsse merged commit b91ba0e into main Aug 22, 2024
26 checks passed
@mattsse mattsse deleted the klkvr/generic-block branch August 22, 2024 03:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattsse mattsse mattsse approved these changes

@DaniPopes DaniPopes Awaiting requested review from DaniPopes DaniPopes is a code owner

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

@onbjerg onbjerg Awaiting requested review from onbjerg onbjerg is a code owner

@prestwich prestwich Awaiting requested review from prestwich

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Add network-parameterized block responses
2 participants
@klkvr @mattsse