Espresso migration #46
Conversation
…e during the migration test
…ume for espresso integrated node
…espresso-integrated sequencer
…idental removal of the container by docker compose donw
| @@ -165,6 +165,23 @@ services: | |||
| depends_on: | |||
| - geth | |||
|
|
|||
| espresso-sequencer: | |||
| pid: host # allow debugging | |||
| image: espresso-integration-testnode | |||
There was a problem hiding this comment.
what's the difference between this and nitro-node-dev-testnode
There was a problem hiding this comment.
I think this should be ghcr.io/espressosystems/nitro-espresso-integration/nitro-node-dev:integration eventually but it's currently necessary to use a local docker image for testing because we haven't merged @zacshowa's E2E changes in nitro and they will be needed for this upgrade test. So I guess it's a locally tagged image.
… as it is not present otherwise
…-testnode into espresso-migration
- Add `--tokenbridge` to get an UpgradeExecutor on child chain. This allows us to execute the Actions on the child chain. - Add pending check to verify staker is making progress. - Decode return values from cast by specifying return type. - Remove `--private-key` argument for cast calls. - Simplify the check for updated balances.
espresso-tests/.env
Outdated
| @@ -0,0 +1,21 @@ | |||
| # env vars for chain name and rpc_url | |||
| CHAIN_NAME="1337" | |||
There was a problem hiding this comment.
CHAIND_ID, also we should prefix everything from L1 with L1, or PARENT_CHAIN.
There was a problem hiding this comment.
I think renaming everything to parent and child chain makes sense as L1 and L2 don't necessarily make sense in the context of orbit chains.
| # Environment variables for chain name and rpc_url | ||
| # These are essential for the upgrade | ||
| PARENT_CHAIN_CHAIN_ID="1337" | ||
| CHILD_CHAIN_CHAIN_NAME="412346" |
There was a problem hiding this comment.
I there a reason to call this "name"? It's also the chain ID.
There was a problem hiding this comment.
No, we can rename this. In my head chain name and chain id are interchangeable which is not the case for Arbitrum or Ethereum.
|
|
||
| ## Steps related to upgrade in production | ||
|
|
||
| Many of the steps in the test are nearly identical to the steps needed to upgrade an orbit network to be compatible with the espresso network. |
There was a problem hiding this comment.
I think the instructions on how to carry out the upgrade should go into a new README in orbit-actions, not this repo. They should also contain instructions to deploy and execute all the actions required for the Espresso upgrade.
Let's link to this test from the new README in orbit-actions for the curious.
There was a problem hiding this comment.
I agree and I'm actively working on a README in that repo.
…-testnode into espresso-migration
test-node.bash
Outdated
| docker compose run scripts write-config --simple | ||
| echo == Writing configs for simple | ||
| docker compose run scripts write-config --simple --simpleWithValidator $simple_with_validator --espresso $l2_espresso --lightClientAddress $lightClientAddr | ||
| if $l2_espresso; then |
There was a problem hiding this comment.
Can you check if this is necessary? The only difference is running a different docker container but scripts and scripts-espresso actually are the same? And I think we need to use scripts-espresso only in the migration test, not the test-node.bash?
There was a problem hiding this comment.
I do think that this should be moved to the migration test bash script. I just remember this being a cleaner way to write a config to a different config volume than to write to two different locations in the javascript code. I believe it's probably better to write the configs to two different volumes as well. IMO overwriting the original config volume would be less clear to any future readers of the code.
scripts/index.ts
Outdated
| @@ -2,7 +2,7 @@ import { hideBin } from "yargs/helpers"; | |||
| import Yargs from "yargs/yargs"; | |||
| import { stressOptions } from "./stress"; | |||
| import { redisReadCommand, redisInitCommand } from "./redis"; | |||
| import { writeConfigCommand, writeGethGenesisCommand, writePrysmCommand, writeL2ChainConfigCommand, writeL3ChainConfigCommand } from "./config"; | |||
| import { writeConfigCommand, writeGethGenesisCommand, writePrysmCommand, writeL2ChainConfigCommand, writeL3ChainConfigCommand} from "./config"; | |||
| @@ -489,4 +531,4 @@ export const writeL3ChainConfigCommand = { | |||
| handler: (argv: any) => { | |||
| writeL3ChainConfig(argv) | |||
| } | |||
| } | |||
| } | |||
There was a problem hiding this comment.
I agree that this is unnecessary, but I don't know what I would do to fix this part of the commit log. I had added and removed a function after this brace. However, I didn't actually modify it at all, so I'm not exactly sure why git is registering it as a changed line.
There was a problem hiding this comment.
Are you using some formatter extension to remove the trailing new line?
scripts/Dockerfile
Outdated
| @@ -3,5 +3,8 @@ WORKDIR /workspace | |||
| COPY ./package.json ./yarn.lock ./ | |||
| RUN yarn | |||
| COPY ./*.ts ./tsconfig.json ./ | |||
| RUN mkdir /config | |||
| # populate this file with valid json that will later be overwritten. If we don't do this the docker image won't build because of typescript looking for this file before the volume that contains it is mounted (I assume) | |||
| RUN cp ./tsconfig.json /config/l2_chain_info.json | |||
There was a problem hiding this comment.
Is this still an issue? A bit strange I think
ImJeremyHe
requested review from
nomaxg,
alysiahuggins and
Sneh1999
as code owners
Closes # 162
TODO
integrationbranch on our nitro fork)This PR:
Is a draft for a PR that will include a full E2E migration test using the nitro test node.
Currently this PR:
This PR does not:
Currently install the new contracts and attempt to perform the upgrade.
Key places to review:
the espresso integrated sequencer is currently not starting due to a lack of some required items in its config. I still need to track down and copy these over to the correct volume.
Run the script migration-test.bash