Rename eip7732 specs to gloas#4506
Conversation
jtraglia
force-pushed
the
rename-eip7732-to-gloas
branch
from
f1dae60 to
5e6f575
Compare
jihoonsong
left a comment
jihoonsong
left a comment
There was a problem hiding this comment.
I've skimmed changes and haven't read the ePBS tests. So there might be something I missed.
One thing I'd like to highlight is with_all_phases_from_except vs with_all_phases_from_to. There is one test that uses the latter but I think we should use the latter in other tests as well, if this exclusion comes from the separation of beacon block and payload.
| | `CAPELLA_FORK_VERSION` | `capella.SignedBeaconBlock` | | ||
| | `DENEB_FORK_VERSION` | `deneb.SignedBeaconBlock` | | ||
| | `EIP7732_FORK_VERSION` | `eip7732.SignedBeaconBlock` | | ||
| | `GLOAS_FORK_VERSION` | `gloas.SignedBeaconBlock` | |
There was a problem hiding this comment.
Do we need Electra fork version? Also just curious, why Fulu doesn't have this section?
There was a problem hiding this comment.
Good catch. Fulu should probably have this section. Will add in a separate PR.
There was a problem hiding this comment.
there is no block changes in Fulu
| # after EIP-7732 the execution payload is no longer in the body | ||
| if is_post_eip7732(spec): | ||
| # After Gloas the execution payload is no longer in the body | ||
| if is_post_gloas(spec): |
There was a problem hiding this comment.
I haven't read this entire file but only some of tests in this file call this helper function. It feels like there should be a cleaner way to modify these Bellatrix tests without introducing these invasive is_post_gloas checks.
There was a problem hiding this comment.
Yes, but this is a bit out-of-scope of this PR.
|
@jihoonsong thanks for the review 🙂 I think I fixed everything (and a little more). |
jihoonsong
left a comment
jihoonsong
left a comment
There was a problem hiding this comment.
This is great. I gave a second skim, please have a look if it makes sense :)
| | `DENEB_FORK_VERSION` | `deneb.BlobSidecar` | | ||
| | `EIP7732_FORK_VERSION` | `eip7732.BlobSidecar` | | ||
| | `ELECTRA_FORK_VERSION` | `electra.SignedBeaconBlock` | | ||
| | `FULU_FORK_VERSION` | `fulu.SignedBeaconBlock` | |
There was a problem hiding this comment.
Do we really need this? Fulu does not have block changes isn't it?
There was a problem hiding this comment.
We don't really need it. And yes, no block changes in Fulu. We chatted more about it here:
There was a problem hiding this comment.
I'm going to keep this how it is. We can remove it later if we want. Not a blocker.
3 participants
This PR creates the initial Gloas specs by moving/renaming the eip7732 specs.
One notable change is in this commit (41fbea2) which disables some new fork transition tests.
Another notable change,
GLOAS_FORK_VERSIONis different thanEIP7732_FORK_VERSION.Everything else is boiler-plate.