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

Add Fulu boilerplate #6695

Open
wants to merge 14 commits into
base: unstable
Choose a base branch
from
Open

Conversation

macladson
Copy link
Member

@macladson macladson commented Dec 14, 2024

Proposed Changes

Adds Fulu fork boilerplate.

Additional Info

This makes no attempt to unify Fulu with PeerDAS. This will need to be a separate PR.

@macladson macladson added the work-in-progress PR is a work-in-progress label Dec 14, 2024
@dapplion
Copy link
Collaborator

dapplion commented Jan 7, 2025

@macladson I've applied the fork_enabled pattern on the remaining places where it is possible in this commit a543765 please review and consider cherry-picking into this PR

To motivate my case against the match fork_name:

  • Relying on the type system to direct our dev attention to certain places would make sense ONLY if we add a new variant to ForkName once the fork is fully spec'ed. Otherwise (like here, now) we just fill all those match cases assuming the fork is empty, losing the all benefits of this approach. Let's accept that we are unlikely to wait for a full spec before adding the boilerplate.

@macladson
Copy link
Member Author

Thanks for the commit! I agree with your reasoning and I've cherry-picked it into the main branch.

I ended up reverting the web3signer.rs changes as it actually uses a local variant of ForkName not types::ForkName so to remove that boilerplate, we'd either need to write a mapping from types::ForkName => web3signer::ForkName or rewrite the web3signer to use types::ForkName. I can't comment on why types::ForkName was chosen in the first place, but either way, it feels out of scope for this PR.

@macladson macladson added ready-for-review The code is ready for review fulu Required for the upcoming Fulu hard fork and removed work-in-progress PR is a work-in-progress labels Jan 7, 2025
Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Looks great! Send it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fulu Required for the upcoming Fulu hard fork ready-for-review The code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants