Make deployment scripts more flexible #54
Conversation
Move config files into a chain specific directory and adjust makefile
- docker-compose.yml - integration-tests - deposit script - comments in the code
|
@wawrzek LGTM, let me merge the other PR that fixed the docker compose up test, and then you can rebase on top and we'll merge your changes. EDIT: just merged. Can you rebase and fix the conflicts? Hopefully it won't be too painful. :D |
|
@samlaf thanks for the message. I noticed that you merged the other branch, so I merged it into my PR. AFAIK it's still fine. |
There was a problem hiding this comment.
LGTM. Not a big fan of having CHAINID defined in the Makefile b/c there's no way to overwrite it with CHAINID=1 make ... for eg. I'm guessing you're changing CHAINID manually in the makefile when you want to target another chain?
We could do something like set
ifndef CHAINID
$(error CHAINID is not set)
endif
but then that's globally enforced for all commands in the makefile, which can get annoying. I typically move all make commands to a bash script and use set -o nounset to crash if a variable is not defined. But that's a lot of work here, so let's just run with this for now.
| @@ -14,4 +14,4 @@ CHAINID=$(cast chain-id) | |||
| DEPLOYMENT_OUTPUT_FILE=./contracts/script/output/${CHAINID}/credible_squaring_avs_deployment_output.json | |||
| STRATEGY_ADDRESS=$(jq -r '.addresses.erc20MockStrategy' $DEPLOYMENT_OUTPUT_FILE) | |||
|
|
|||
| go run cli/main.go --config config-files/operator.anvil.yaml deposit-into-strategy --strategy-addr ${STRATEGY_ADDRESS} --amount 100 | |||
| go run cli/main.go --config config-files/${CHAINID}/operator.anvil.yaml deposit-into-strategy --strategy-addr ${STRATEGY_ADDRESS} --amount 100 | |||
There was a problem hiding this comment.
if you can change this to ${CHAINID:?} or use set -o nounset at the top of the file. This way the script will early exit if the variable is not defined.
There was a problem hiding this comment.
The following works
make CHAINID=32382 start-aggregator
so, there is an easy way to overwrite the CHAINID and other variables. However, I agree that maybe setting the CHAINID in the Makefile is not optimal. I just wanted to make an incremental change. Collect all variables in one place, so they can be handled better.
There was a problem hiding this comment.
@wawrzek could you update this to reflect 2 items:
- to use
${CHAINID:?}(in spite ofmake CHAIND=32382...working) - update the README to highlight that
start-aggregatorshould be run with a CHAINID env var
Otherwise lgtm
There was a problem hiding this comment.
@NimaVaziri , I've been looking at changing the behaviour of the make commands and I don't like it. Despite me wanting to use a different chain id I think the change should not make life harder for others, so keep the current behaviour when the CHAINID might be omitted.
I suggest an alternative. Keep current Makefile, but extend documentation by a paragraph describing how to deploy AVS into a network different from the anvil one. The change is in the latest commit.
- add information on how to use CHAINID
New version: - prometheus - grafana Remove trailing whitespaces
Bump version of monitoring images
Move config files into a chain specific subfolder of the config-files folder. This enables easy testing with non-chain specific deployments, but adjusting the CHAINID variable in the Makefile.
For the same reason, I also moved one of the key, the one used to top the operator up with Ether, into a variable.
Why? I deploy the AVS into non-anvil chain, quite a few values are hardcoded in config files. The change is probably a minimal change to enables such test, and it does not affect current method of deployment.