-
Notifications
You must be signed in to change notification settings - Fork 105
fix(l2): add script to read address from deployer in docker compose #4624
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
base: main
Are you sure you want to change the base?
Conversation
Lines of code reportTotal lines added: Detailed view
|
f242924
to
cb60fa6
Compare
Benchmark for f1d1a5dClick to view benchmark
|
Benchmark Results ComparisonNo significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
FROM debian:12-slim | ||
WORKDIR /usr/local/bin | ||
|
||
RUN apt-get update && apt-get install -y --no-install-recommends libssl3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is because we need ssl that it is not included on the base image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just some small comments
crates/l2/docker-compose.yaml
Outdated
entrypoint: | ||
- /bin/bash | ||
- -c | ||
- touch /env/.env; exec ./ethrex "$0" "$@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd put "l2 deploy" as part of the entrypoint, as the contract deployer is always suposed to run it. This way, you can run:
docker compose run contract_deployer <custom_deployer_options>
instead of
docker compose run contract_deployer l2 deploy <custom_deployer_options>
Also, the exec
is not needed.
entrypoint:
- /bin/bash
- -c
- touch /env/.env; exec ./ethrex "$0" "$@"
command: >
--randomize-contract-deployment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done e9ec17b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the "$@" is still needed. By default the command is just one argument, but the user may want to add more
crates/l2/docker-compose.yaml
Outdated
entrypoint: | ||
- /bin/bash | ||
- -c | ||
- export $$(xargs < /env/.env); exec ./ethrex "$$0" "$$@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here about l2
subcommand and exec
. Also, it's weird the double $ here but not in the previous command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the extra $ here 6921165
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done the remaining here e9ec17b
60bfc00
to
6921165
Compare
Motivation
Running
docker compose up
was broken running oncrates/l2
. The problem is that the ethrex l2 container reads the.env
at the moment of running thedocker compose up
and miss the updates on the file that writes the deployer.Description
.env
in the shared volume that theethrex_l2
container will readethrex_l2
container as a previous step to running the sequencer.Dockerfile
to use a Debian as we need to use common unix binariesHow to Test