-
Notifications
You must be signed in to change notification settings - Fork 25
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
update to op stack v1.9.3 #100
base: master
Are you sure you want to change the base?
Conversation
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.
Sorry it took me so long — looking good, have you tried running this on Sepolia or another non-devnet testnet?
Few things to fix and a couple of questions, but we can merge this soon 🚀
res = lib.send_json_rpc_request(host, 3, "debug_dumpBlock", ["latest"]) | ||
response = json.loads(res) | ||
allocs = response['result'] | ||
lib.write_json_file(config.l1_allocs_path, allocs) |
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.
What happened to this? Oh I see it's handled by the contracts now?
deps.py
Outdated
@@ -394,7 +394,7 @@ def check_or_install_pnpm(): | |||
pass | |||
|
|||
if lib.ask_yes_no("PNPM is required. Install?"): | |||
lib.run("install PNPM", cmd_with_node("npm install -g pnpm")) | |||
lib.run("install PNPM", cmd_with_node("npm update -g pnpm")) |
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.
Will this install pnpm if it's not already installed?
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.
From my tests it looks like not.
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.
it seems you need a fairly recent version of pnpm that's why I thought to change it to update
but let's keep it as install
for now and address it if we see issues in future
try: | ||
_deploy_contracts_on_l1(config, tmp_l1) | ||
finally: | ||
os.remove(config.op_deploy_config_path) |
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.
Is there a reason we shouldn't delete this and pass the config.deploy_config_path
(without the op
) as DEPLOY_CONFIG_PATH
below?
'--l1-deployments', config.addresses_path, | ||
'--outfile.l2', config.l2_genesis_path, | ||
'--outfile.rollup', config.rollup_config_path | ||
], cwd=config.op_node_dir, log_file=log_file) |
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.
Just checking if the last two invocations require rolling (i.e. if they emit a lot of output, or something we might want to save)
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.
looking at the outputs, I think there is some potentially useful info here so doesn't hurt to save it imo
setup.py
Outdated
cwd="optimism") | ||
except CalledProcessError as e: | ||
if "already exists" not in str(e): | ||
raise e |
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 might have been merged? We could probably remove the whole fix.
setup.py
Outdated
@@ -98,7 +95,7 @@ def setup_op_geth_repo(config: Config): | |||
Clone the op-geth repository and build it. | |||
""" | |||
github_url = "https://github.com/ethereum-optimism/op-geth.git" | |||
git_tag = "v1.101304.1" | |||
git_tag = "v1.101315.2" |
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.
Can you update the README accordingly (same for the monorepo version)?
deps.py
Outdated
""" | ||
Required version of forge. We're locking down foundry to a specific version, as new versions can | ||
introduce serious regressions, and have done so in the past. | ||
""" | ||
|
||
FOUNDRY_INSTALL_TAG = "nightly-09fe3e041369a816365a020f715ad6f94dbce9f2" | ||
FOUNDRY_INSTALL_TAG = "nightly-d7eac74cfd786447cec9650048e2d2fac63fba0c" |
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 broken — looks like after a certan time they prune most of the intermediate nightlies. For some reason, it looks like they often keep the 2nd of the month nightlies so I suggest we update to nightly-5ac78a9cd4b94dc53d1fe5e0f42372b28b5a7559 from 2024-06-02
remove custom tagging in the op monorepo
I have not tried this on non-devnet yet, I'd like to work on getting devnet working with ansible first. Do you think we need to test non-devnet deployments before we can merge this? |
Works for devnet. Still need to update the production config template.