feat(flashblock): commit shared flashblock data structures#606
feat(flashblock): commit shared flashblock data structures#606mattsse merged 10 commits intoalloy-rs:mainfrom
Conversation
0x00101010
force-pushed
the
flashblock
branch
from
4112f3d to
d935c96
Compare
0x00101010
force-pushed
the
flashblock
branch
from
d935c96 to
11edb9b
Compare
0x00101010
force-pushed
the
flashblock
branch
from
1927fbd to
1a69f7a
Compare
0x00101010
force-pushed
the
flashblock
branch
from
ac3ab39 to
3fd0815
Compare
klkvr
left a comment
klkvr
left a comment
There was a problem hiding this comment.
I would prefer this to simply copy-paste the flashblock types from reth, right now this also introduces new types and renames existing ones making it tricky to review
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] | ||
| #[cfg_attr(feature = "serde", serde(untagged))] | ||
| pub enum OpFlashblockExecutionPayloadBase { |
There was a problem hiding this comment.
hmm I think we can introduce versioned types separately?
There was a problem hiding this comment.
Understood, my thinking is that this renaming should be relatively straight-forward. What I did is that:
-
Renaming:
FlashblocksPayloadV1->OpFlashblockPayload
ExecutionPayloadBaseV1->OpFlashblockExecutionPayloadBase
ExecutionPayloadFlashblockDeltaV1->OpFlashlockExecutionPayloadDelta
Metadata->OpFlashlockMeatdata -
Added versioning and helper functions
-
Added Ref types for efficient access
Since this is purely data accessors and the core data structures remains identical (aside from added blob_gas_used field for Jovian support, my thought is to do them together so that we avoid another op-alloy release / versioning.
Let me know what you think, I can do it in phases if you still think this is tricky to review
There was a problem hiding this comment.
the non optional v1 functions would become breaking when there's a new V2
then we don't really gain anything from introducing the enum I think
There was a problem hiding this comment.
I don't think they'll break per se, this is the exact same pattern as pub enum ExecutionPayload in alloy-rpc-types-engine. V1's always going to be there, all future versions are going to be optional, and we can use as_v1().v1_field, and as_v2().v2_field when exposing accesor methods.
But as discussed in below thread, personally prefer no versioning as well so no problem here changing it.
| // Note: this uses mixed camel, snake case: <https://github.com/flashbots/rollup-boost/blob/dd12e8e8366004b4758bfa0cfa98efa6929b7e9f/crates/flashblocks-rpc/src/cache.rs#L31> | ||
| #[derive(Clone, Debug, Default, PartialEq, Eq)] | ||
| #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] | ||
| pub struct OpFlashblockMetadataV1 { |
There was a problem hiding this comment.
There was a problem hiding this comment.
After the versioning and rework, the helper is not needed. We have the Ref types that can be efficiently used to retrieve receipts
There was a problem hiding this comment.
hmm I don't think versioning and ref types make sense to me yet
what do we gain via an OpFlashblockMetadataRef vs &OpFlashblockMetadata?
There was a problem hiding this comment.
I debated this myself as well, for pure json rpc requests, I personally also like no versioning.
However the reasons that moves me towards versioning are:
- Base for example is considering FAL (flashblock access lists) which requires storing data for performance hints, versioning here helps with hardfork boundry, validation and ambiguity, migration safety, etc
- Since this is ethereum land, tends to match the existing versioning pattern for consistency
Re Ref types, you're right, with as_v1, as_v2, etc, we don't need the Ref types, I'll change that and remove relevant Deref, etc
There was a problem hiding this comment.
yeah, though I think we might be able to get far enough with just optional fields for extra stuff
versioning via untagged enums might end up pretty wasteful during deser
There was a problem hiding this comment.
all right, ok with me to proceed with no versions but name changes? I think that'll be easier enough for reviews and we can make the names more consistent in one go
0x00101010
requested review from
clabby,
emhane,
mattsse and
refcell
as code owners
0x00101010
force-pushed
the
flashblock
branch
from
5e07d75 to
a15b7ff
Compare
…op-alloy#606) <!-- Thank you for your Pull Request. Please provide a description above and review the requirements below. Bug fixes and new features should include tests. Contributors guide: https://github.com/alloy-rs/core/blob/main/CONTRIBUTING.md The contributors guide includes instructions for running rustfmt and building the documentation. --> <!-- ** Please select "Allow edits from maintainers" in the PR Options ** --> ## Motivation commit shared flashblock data structures <!-- Explain the context and why you're making that change. What is the problem you're trying to solve? In some cases there is not a problem and this can be thought of as being the motivation for your change. --> ## Solution <!-- Summarize the solution and provide any necessary context needed to understand the code change. --> ## PR Checklist - [x] Added Tests - [x] Added Documentation - [ ] Breaking changes --------- Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
3 participants
Motivation
commit shared flashblock data structures
Solution
PR Checklist