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

Consider storing all hardfork times explicitly in each chain's config file #751

Open
geoknee opened this issue Dec 6, 2024 · 3 comments · May be fixed by #810
Open

Consider storing all hardfork times explicitly in each chain's config file #751

geoknee opened this issue Dec 6, 2024 · 3 comments · May be fixed by #810

Comments

@geoknee
Copy link
Collaborator

geoknee commented Dec 6, 2024

I've been wondering if we can leverage/extend the existing codegen tooling to help keep this stuff tidy. It could:

  1. throw an error if folks have set superchain_time in a way that doesn't make sense.
  2. actually write all active hardforks into each chain's TOML file using the existing algorithm for resolving them from superchain-wide values + superchain_time + any overrides. We can then do the usual git diff --exit-code to ensure the modified TOMLs are checked in and reproducible.

Doing 2 has the advantage that hardfork times are no longer implicit / the result of an opaque resolution process, but there for all to see in the raw TOML files. We could also kill the diffbots (which are there to compute essentially the same thing) which are proving difficult to maintain in a secure way.
When we set e.g. the Isthmus time, we run codegen and it changes all existing chains' TOML files if appropriate. We see the diff in the actual Github diff, and don't need a bot to compute it and comment on the PR with the result.

@sebastianst
Copy link
Member

If this helps overall, I'm supportive. My main opposition is that we duplicate information from the superchain.toml into individual chains' toml files. We'd only do this for fork activation times, but not for other fields in the superchain.toml file, which seems inconsistent.

What I also like about 1. is that this would mean we only see explicit diffs to superchain activation times in individual chains' configs, so only overrides that don't equal the superchain time. When we do 2. it's not obvious from looking at an individual chain's config which is an override changing the superchain time, and which is just a copy of a superchain activation.

@sebastianst sebastianst mentioned this issue Dec 9, 2024
3 tasks
@bitwiseguy
Copy link
Collaborator

bitwiseguy commented Dec 9, 2024

I'm in favor of [2] because I think it would be a huge devX improvement by removing confusion around hardforks for internal+external teams. Additionally, if the toml files contain the entire set of config data, it would allow us to support the work here: #757

I agree with @sebastianst that this would make it more difficult to tell how a chain diverges from the default values. But perhaps we could alleviate this concern by adding a simple command to the superchain-registry/ops module which could print out the diff?

./ops default-diffs <chain_id>

@geoknee
Copy link
Collaborator Author

geoknee commented Dec 9, 2024

Maintaining our CI config "diff bots" has also proven to be more work than anticipated, and this change would likely remove the need for those diff bots.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants