-
Notifications
You must be signed in to change notification settings - Fork 180
feat(fill): pass blobSchedule into t8n binary #2264
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
base: main
Are you sure you want to change the base?
Conversation
cbc03ea
to
220b7cf
Compare
220b7cf
to
030733b
Compare
coverted to draft b/c maybe we should send more in that JSON (to include |
Ready to review. The version pushed now will process (in line with ipsilon/evmone#1330) an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing this, some comments.
class TransitionToolConfig(CamelModel): | ||
"""Transition tool config.""" | ||
|
||
fork: Fork = Field(..., alias="network") | ||
chain_id: int = Field(..., alias="chainid") | ||
blob_schedule: BlobSchedule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is very similar to the already-existing TransitionToolContext
, so we can just repurpose that one instead.
|
||
fork: Fork = Field(..., alias="network") | ||
chain_id: int = Field(..., alias="chainid") | ||
blob_schedule: BlobSchedule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in chat group, let's pass instead:
blob_schedule: BlobSchedule | |
blob_params: BlobParams |
Also, we already have a definition that fits our needs in here:
execution-spec-tests/src/ethereum_test_base_types/composite_types.py
Lines 487 to 492 in 56ea9f2
class ForkBlobSchedule(CamelModel): | |
"""Representation of the blob schedule of a given fork.""" | |
target_blobs_per_block: HexNumber = Field(..., alias="target") | |
max_blobs_per_block: HexNumber = Field(..., alias="max") | |
base_fee_update_fraction: HexNumber = Field(...) |
We could even rename that type to BlobParams
because the current name ForkBlobSchedule
doesn't make much sense.
"--input.config", | ||
input_paths["config"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break client tools that do not support this parameter yet.
The way we handle this is by implementing a ClassVar
in TransitionTool
that could default to False
(example here:
supports_xdist: ClassVar[bool] = True |
True
only for evmone.
🗒️ Description
The blobSchedule.json is required by evmone-t8n tool now, in order to fill tests correctly. We add a new structure intended to be used as a JSON generator for the filesystem evaluation inputs - I guess trying to mimic what
TransitionToolRequest
does, lmk if I misread the idea here.Appropriate change on the evmone end: ipsilon/evmone#1330
🔗 Related Issues or PRs
N/A.
✅ Checklist
tox
checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint
type(scope):
.