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

Move is_post_xxx functions to new module #3072

Merged
merged 4 commits into from
Nov 11, 2022

Conversation

etan-status
Copy link
Contributor

Moving the is_post_xxx functions to a separate module allows genesis to also use them (cyclic import from context prevented this before). This allows removing FORKS_BEFORE_ALTAIR and FORKS_BEFORE_BELLATRIX constants and adding a more general is_post_fork function that needs less maintenance. This then allows definition of with_all_phases_from to streamline the implementation of the with_xxx_and_later decorators.

Moving the `is_post_xxx` functions to a separate module allows `genesis`
to also use them (cyclic import from `context` prevented this before).
This allows removing `FORKS_BEFORE_ALTAIR` and `FORKS_BEFORE_BELLATRIX`
constants and adding a more general `is_post_fork` function that needs
less maintenance. This then allows definition of `with_all_phases_from`
to streamline the implementation of the `with_xxx_and_later` decorators.
Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

Nice refactoring!

Raised some naming nitpicking around phases v.s. forks. protocols here.

@@ -0,0 +1,33 @@
from .constants import (
Copy link
Contributor

Choose a reason for hiding this comment

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

Although we had Phase0, now I wish we could retire "phases" someday since developments are in-parallel.

What do you think about protocols.py or forks.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will go with forks.py then. It is consistent with the fork=... naming when running make citest.
I will not touch the decorators such as @with_all_phases for now, as it is out of scope of this PR.

return b in [PHASE0, ALTAIR]
if a == PHASE0:
return b in [PHASE0]
assert False # Fork is missing
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert False # Fork is missing
raise ValueError("Wrong fork name %s" % a)

@hwwhww hwwhww merged commit a6c4b9a into ethereum:dev Nov 11, 2022
@etan-status etan-status deleted the sf-ispostfork branch November 12, 2022 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants