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

feat: add initial EIP-7547 engine types #287

Merged
merged 7 commits into from
Mar 29, 2024

Conversation

Rjected
Copy link
Contributor

@Rjected Rjected commented Mar 12, 2024

Motivation

Inclusion lists are being implemented by clients for a poc:
ethereum/go-ethereum@3fd8199

To do this for reth, we'll need to get the proper rpc types, so we can implement the calls in reth.

Solution

Implemented the types defined in michaelneuder/execution-apis#1

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@prestwich
Copy link
Member

procedural question, should in-progress EIPs be homed in alloy-eips to minimize intrusion in other parts of the codebase?

@prestwich prestwich added the enhancement New feature or request label Mar 12, 2024
@Rjected
Copy link
Contributor Author

Rjected commented Mar 12, 2024

procedural question, should in-progress EIPs be homed in alloy-eips to minimize intrusion in other parts of the codebase?

For this one it could be, since it would not cause any circular deps. But if an in progress eip needed a transaction type for example, it would cause problems. IMO despite this one not causing circular issues, it seems best to just have it as a draft PR to the place where it will eventually live (rpc-engine-types).

@prestwich
Copy link
Member

another option would be to have a specific crate alloy-eipXXXX for these while they're in progress and not accepted

@mattsse
Copy link
Member

mattsse commented Mar 12, 2024

another option would be to have a specific crate alloy-eipXXXX for these while they're in progress and not accepted

that makes sense!

@Rjected
Copy link
Contributor Author

Rjected commented Mar 21, 2024

another option would be to have a specific crate alloy-eipXXXX for these while they're in progress and not accepted

that makes sense!

This is now in the eip7547 crate

@Rjected Rjected marked this pull request as ready for review March 21, 2024 22:56
@@ -20,6 +20,7 @@ rustdoc-args = ["--cfg", "docsrs"]
alloy-consensus = { version = "0.1.0", default-features = false, path = "crates/consensus" }
alloy-contract = { version = "0.1.0", default-features = false, path = "crates/contract" }
alloy-eips = { version = "0.1.0", default-features = false, path = "crates/eips" }
alloy-eip7547 = { version = "0.1.0", default-features = false, path = "crates/eip7547" }
Copy link
Member

Choose a reason for hiding this comment

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

key here doesn't match package name. different hyphenation. standardize on eip7547

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops, just fixed

Copy link
Member

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

UTACK. I have not reviewed these types for conformance, but given this is a pre-finalization EIP i don't think that matters much

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.

Comment on lines 84 to 85
/// The nonce of the inclusion list entry.
pub nonce: u64,
Copy link
Member

Choose a reason for hiding this comment

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

this should be hex serde?

Comment on lines 108 to 111
/// The slot of the inclusion list summary.
pub slot: u64,
/// The proposer index of the inclusion list summary.
pub proposer_index: u64,
Copy link
Member

Choose a reason for hiding this comment

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

should be hex?

@@ -82,7 +82,7 @@ pub struct InclusionListSummaryEntryV1 {
/// The address of the inclusion list entry.
pub address: Address,
/// The nonce of the inclusion list entry.
pub nonce: u64,
pub nonce: U64,
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 use u64 and serde_with alloy_serde::u64_hex for all U64?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops, yeah

@Rjected Rjected requested a review from mattsse March 25, 2024 13:51
@prestwich prestwich merged commit bc8eace into alloy-rs:main Mar 29, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants