Skip to content

feat: network-parameterized block responses#1106

Merged
mattsse merged 5 commits into
mainfrom
klkvr/generic-block
Aug 22, 2024
Merged

feat: network-parameterized block responses#1106
mattsse merged 5 commits into
mainfrom
klkvr/generic-block

Conversation

@klkvr

@klkvr klkvr commented Jul 26, 2024

Copy link
Copy Markdown
Member

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 left a comment

Copy link
Copy Markdown
Member

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

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
Copy Markdown
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/

}
}

impl<T: BlockResponse> BlockResponse for WithOtherFields<T> {

Copy link
Copy Markdown
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

type ReceiptResponse = AnyTransactionReceipt;

type HeaderResponse = WithOtherFields<Header>;
type HeaderResponse = Header;

Copy link
Copy Markdown
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> ?

@klkvr klkvr Aug 21, 2024

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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 left a comment

Copy link
Copy Markdown
Member

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?

Comment thread 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
Copy Markdown
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
Copy Markdown
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

@mattsse mattsse Aug 21, 2024

Copy link
Copy Markdown
Member

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": [
    {

@mattsse mattsse Aug 21, 2024

Copy link
Copy Markdown
Member

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...

@mattsse mattsse merged commit b91ba0e into main Aug 22, 2024
@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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Add network-parameterized block responses

2 participants