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

contracts: Make L2 genesis script configurable #10764

Merged
merged 4 commits into from
Jun 11, 2024

Conversation

sebastianst
Copy link
Member

Description

Makes the L2 genesis script configurable via two new env vars

  • OUTPUT_MODE
    • none - no state dump file written, used in tests
    • latest - only dump state of latest fork
    • all - dump states of all forks (default)
  • FORK - for fork selection (delta, ecotone,fjord or latest (default, always selects latest fork))

All generated devnet allocs now explicitly have their fork as suffix, also the latest (no suffix previously).

Still WIP:

  • solidity documentation needs update

Additional context

The L2 genesis script before had the latest fork hardcoded.

Metadata

Copy link
Contributor

semgrep-app bot commented Jun 6, 2024

Semgrep found 5 sol-style-return-arg-fmt findings:

Named return arguments to functions must be appended with an underscore (_)

Ignore this finding from sol-style-return-arg-fmt.

Semgrep found 1 golang_fmt_errorf_no_params finding:

  • op-node/rollup/derive/engine_queue_test.go

No fmt.Errorf invocations without fmt arguments allowed

Ignore this finding from golang_fmt_errorf_no_params.

Semgrep found 1 writable-filesystem-service finding:

  • ufm-test-services/docker-compose.yml

Service 'grafana' is running with a writable root filesystem. This may allow malicious applications to download and run additional payloads, or modify container files. If an application inside a container has to save something temporarily consider using a tmpfs. Add 'read_only: true' to this service to prevent this.

Ignore this finding from writable-filesystem-service.

Semgrep found 3 port-all-interfaces findings:

Service port is exposed on all interfaces

Ignore this finding from port-all-interfaces.

Semgrep found 1 math-random-used finding:

  • op-node/rollup/derive/engine_queue_test.go

Do not use math/rand. Use crypto/rand instead.

Ignore this finding from math-random-used.

Semgrep found 1 todos_require_linear finding:

  • op-node/rollup/derive/engine_queue.go

Please create a GitHub ticket for this TODO.

Ignore this finding from todos_require_linear.

Semgrep found 1 missing-user finding:

  • ufm-test-services/metamask/Dockerfile

By not specifying a USER, a program in the container may run as 'root'. This is a security hazard. If an attacker can control a process running as root, they may have control over the container. Ensure that the last USER in a Dockerfile is a USER other than 'root'.

Ignore this finding from missing-user.

Semgrep found 1 err-todo finding:

  • op-node/rollup/derive/engine_queue.go

TODO in error handling code

Ignore this finding from err-todo.

@tynes
Copy link
Contributor

tynes commented Jun 7, 2024

These abstractions definitely look clean to me! I am wondering since because the deploy config has access to the fork offsets, we could simplify things and remove the need to configure the fork via env var and instead parse the forks that have a genesis offset of 0

I imagine this looking like in the deploy config:

fjordOffsetTimestamp = _envOr(json, "fjordOffsetTimestamp", type(uint256).max);

then you could have something like DeployConfig.latestFork() or something like that or isXYZ

Just suggesting this to reduce possible footguns. Besides that, this code is clean and well structured

@sebastianst
Copy link
Member Author

@tynes thanks for the suggestion, implemented (hopefully something like) this in the 2nd commit. If the FORK env var is set, it takes precedent, otherwise the fork is read from the deploy config. If the deploy config isn't set either, it fails to run, which is probably good because if forces the user to actively think about which fork to set.

If the 2nd commit isn't going in the right direction or needs much more work, we can also split it out into a 2nd PR and get the 1st commit in so that the functionality for fork selection is already there. So chain providers asking for this already have something that works.

@sebastianst sebastianst marked this pull request as ready for review June 10, 2024 22:03
@sebastianst sebastianst requested review from a team as code owners June 10, 2024 22:03
@sebastianst
Copy link
Member Author

I should probably add some documentation about the new env vars.

@tynes
Copy link
Contributor

tynes commented Jun 11, 2024

I think there are some ways to simplify this, left comments, curious what you think @sebastianst

@tynes
Copy link
Contributor

tynes commented Jun 11, 2024

Looks like linting has failed

@tynes
Copy link
Contributor

tynes commented Jun 11, 2024

If you rebase on develop, then it will fix ci/circleci: sdk-tests

@sebastianst sebastianst added this pull request to the merge queue Jun 11, 2024
Merged via the queue into develop with commit 4826337 Jun 11, 2024
58 of 61 checks passed
@sebastianst sebastianst deleted the seb/l2genesis-params branch June 11, 2024 21:39
rdovgan pushed a commit to rdovgan/optimism that referenced this pull request Jun 24, 2024
* contracts: Make L2 genesis script configurable

* contracts: Read latest fork from deploy config in L2 genesis script

* contracts: add README entry for new L2 genesis script env vars

* address review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants