Add action contracts for migrating live orbit chains to espresso #1

zacshowa · 2024-08-14T21:15:34Z

This PR:

This PR adds action upgrade contracts to the orbit-action repo to upgrade orbit chains to be compatible with the espresso sequencing marketplace.

This PR does not:

Include scripts to deploy and perform these action upgrades. Set the chain config in the arbOS upgrade.

Key places to review:

EspressoOspMigrationAction.sol and EspressoMigrationAction.t.sol.

forge test in the repo's top level

EspressoOspMigrationAction.sol

Sneh1999 · 2024-08-15T15:36:13Z

contracts/parent-chain/contract-upgrades/EspressoArbOSUpgrade.sol

+    function perform() external {
+        ArbOwner arbOwner = ArbOwner(0x0000000000000000000000000000000000000070);
+        arbOwner.scheduleArbOSUpgrade({
+            newVersion: newArbOSVersion,


Shouldnt the new version and timestamp be function arguments instead of constructor arguments? Then we can use the same script for multiple upgrade IMO

This is what I originally thought. However, per the governance action contract guidelines this seems to be the proper format to write these contracts for an eventual proposal to the Arbitrum DAO.
In theory this also allows us to create more specific proposals by writing more wrapper contracts like the EspressoArbOSUpgrade that utilizes the general

Sneh1999 · 2024-08-15T15:36:44Z

contracts/parent-chain/contract-upgrades/EspressoOspMigrationAction.sol

+import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
+import "@openzeppelin/contracts/utils/Address.sol";
+
+error IncorrectWasmModuleRoot(bytes32 incorrectAddr);


The errors should be a part of the contract, so I think you should add them inside the contract object

Sneh1999 · 2024-08-15T15:37:19Z

contracts/parent-chain/contract-upgrades/EspressoOspMigrationAction.sol

+            revert IncorrectWasmModuleRoot(currentWasmModuleRoot);
+        }
+
+        if(Address.isContract(newOspEntry) == false){


nit: can we do if(!Address.isContract(newOspEntry))

Sneh1999 · 2024-08-15T15:37:26Z

contracts/parent-chain/contract-upgrades/EspressoOspMigrationAction.sol

+            revert AddressIsNotContract(newOspEntry);
+        }
+
+        if(Address.isContract(currentOspEntry) == false){


Sneh1999 · 2024-08-15T15:39:10Z

contracts/parent-chain/contract-upgrades/example-env/.env-espresso-osp-migration.example

@@ -0,0 +1,6 @@
+export NEW_OSP_ENTRY="0x7B5F0B437EE68A22992DdD629AA22525Fc5dfa9A"


Nit: I dont think you need export in front of the environment variables ( example here)

This might just be from my lack of familiarity with forge and foundry. I couldn't get the env vars to be visible to the tests unless I exported them. Sourcing the file without exporting them doesn't propagate the vars to any child processes, e.g. forge test.
If there is a different way I'm supposed to supply forge with the env vars I would love to know about it!

If the file is called .env exactly it is special and automatically picked up by foundry and many other tools. Unfortunately it has to be exactly .env in the case of foundry.

you can do source .env-espresso-osp-migration && forge test <> to be able to have a foundry script pick up env variables that's not in the expected .env file.

That's true but then breaks the ability of being able to override with env vars. So this can get messy when used in scripts if one expects environment variable that are already set to take precedence.

that's true. we aren't using the .env file for secrets anymore but only for config (since that's how it's been set up historically). So should this go into the .env file?

It seems to be how existing orbit upgrades are configured in the orbit-actions repo. I think it's a good idea to stick to how things are done in that repo unless it really gets in our way.

Sneh1999 · 2024-08-15T15:39:16Z

contracts/parent-chain/contract-upgrades/example-env/.env-schedule-arbos-upgrade.example

@@ -0,0 +1 @@
+export UPGRADE_TIMESTAMP = "1723664126"


Sneh1999 · 2024-08-15T15:40:01Z

contracts/tests/EspressoMigrationAction.t.sol

+
+
+    /* solhint-disable func-name-mixedcase */
+    //create items needed for a rollup and deploy it. This code is lovingly borrowed from the rollupcreator.t.sol foundry test.


alysiahuggins · 2024-08-16T14:06:25Z

Hey @zacshowa, can you also add a readme that explains these code additions and how to run the scripts

alysiahuggins · 2024-08-16T14:07:06Z

contracts/parent-chain/contract-upgrades/EspressoArbOSUpgrade.sol

+contract EspressoArbOSUpgrade is UpgradeArbOSVersionAtTimestampAction, Script   {
+    constructor()
+        UpgradeArbOSVersionAtTimestampAction(
+            35,


when is this called? curious about this hardcoded arbOsVersion

This will be called a decent amount of time before the network upgrade is supposed to take place. This will schedule a particular function inside the nodes of an orbit network to run at the given Unix timestamp.

The hardcoded 35 comes from a discussion I had with Mathis when we were first pursuing designating this as an ArbOS upgrade.

Basically, the way ArbOS upgrades are performed is via a switch statement that has some pre-defined gaps in version numbers left open for orbit chains. We chose 35 as it leaves some room for previous upgrades, as well as room for future upgrades in the most recent gap.

ahh interesting - let's add that in the comments then, and maybe leave instructions in the readme if it ever has to change

alysiahuggins · 2024-08-16T14:07:37Z

contracts/parent-chain/contract-upgrades/EspressoArbOSUpgrade.sol

+    }
+
+    function perform() external {
+        ArbOwner arbOwner = ArbOwner(0x0000000000000000000000000000000000000070);


why is this address used?

This magic value is just the location of the ArbOwner precompile that lives on all arbitrum orbit chains at a specific address.

I'll leave a comment explaining it in the code.

alysiahuggins · 2024-08-16T14:07:55Z

contracts/parent-chain/contract-upgrades/EspressoArbOSUpgrade.sol

+    uint64 public immutable newArbOSVersion;
+    uint64 public immutable upgradeTimestamp;
+
+    constructor(uint64 _newArbOSVersion, uint64 _upgradeTimestamp) {


do we need uint64 for the arbOSVersion?

I would argue that most of the way this is done doesn't "need" to happen this way, but I wrote these contracts to broadly conform with the guidance in the arbitrum governance repo here

For the sake of time we aren't using address registries and are instead using env variables for the generic and proposal specific version of the contracts. I'm just doing it this way to minimize the amount of future work to convert this to a reasonable proposal for the Arbitrum DAO

alysiahuggins · 2024-08-16T14:13:35Z

contracts/parent-chain/contract-upgrades/EspressoArbOSUpgrade.sol

+    uint64 public immutable upgradeTimestamp;
+
+    constructor(uint64 _newArbOSVersion, uint64 _upgradeTimestamp) {
+        newArbOSVersion = _newArbOSVersion;


consider adding error checking to ensure that the values passed here are valid

alysiahuggins · 2024-08-16T14:14:55Z

contracts/parent-chain/contract-upgrades/EspressoArbOSUpgrade.sol

@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: Apache-2.0


Not sure which license we'd want to use but we normally default to UNLICENSED

Ill go ahead and add UNLICENSED to all of the files that require it.

alysiahuggins · 2024-08-16T14:15:10Z

contracts/parent-chain/contract-upgrades/EspressoOspMigrationAction.sol

@@ -0,0 +1,107 @@
+pragma solidity ^0.8.9;


add the SPDX license identifier

alysiahuggins · 2024-08-16T14:17:46Z

contracts/parent-chain/contract-upgrades/EspressoOspMigrationAction.sol

+        );
+        address postUpgradeChalManAddr = ProxyAdmin(proxyAdmin).getProxyImplementation(challengeManager);
+
+        if(postUpgradeChalManAddr != chalManImpl){


i like this check!

alysiahuggins · 2024-08-16T14:18:13Z

contracts/parent-chain/contract-upgrades/EspressoOspMigrationAction.sol

+        address _rollup,
+        address _proxyAdmin
+    ){
+        newOspEntry = _newOspEntry;


perhaps add some error checking for these variables in the constructor

If we add the checks in the perform function to the constructor we can, in theory, remove them from the perform function to avoid using extra gas correct?

alysiahuggins · 2024-08-16T14:21:21Z

contracts/tests/EspressoMigrationAction.t.sol

+
+        address migration = address(new EspressoOspMigrationAction());
+
+        vm.prank(rollupOwner);


consider adding more tests that check for negative scenarios e.g. expecting a revert of a non-rollup owner tries to execute the upgrade

…OSUpgradeAtTimestampAction contract instead of duplicating it.

migration-test

contracts/parent-chain/contract-upgrades/DeployEspressoOsp.s.sol

This allows us to set all the config from the test script

… contract.

migration/README.md

sveitser · 2024-09-05T09:30:54Z

migration/README.md

+
+#### A note on Upgrade Executors
+
+This migration presumes there to be upgrade executor contracts that are chain/rollup owners on both the parent chain, as well as the child chain. If you do not have an upgrade executor on the child chain, the contract execution commands at the end of this guide will not work.


sveitser · 2024-09-05T09:32:20Z

migration/README.md

+The parent chain is where most of the contract deployments will happen for the migration to espresso. There are two forge scripts that you need to deploy on the parent chain. These scripts are located in the following directories:
+
+```
+orbit-actions/contracts/parent-chain/espresso-migration/DeployEspressoOsp.s.sol


You can use markdown links relative to files in this repo.

https://github.com/adam-p/markdown-here/wiki/Markdown-Cheatsheet#links

migration/README.md

sveitser · 2024-09-05T09:34:56Z

migration/README.md

+1. DeployEspressoOsp.s.sol
+2. DeployEspressoOspMigrationAction.s.sol
+
+In between these deployments you need to record the address of the newly deployed OneStepProverEntry contract and export it in the env var `NEW_OSP_ENTRY` 


Is this obvious from the output? Or should we add a jq command here?

I think the output of that command would be obvious if you're running the same verbosity level on the forge script. But I think it also makes sense to add the command anyways. as we know exactly how forge will output the broadcast file.

Co-authored-by: Mathis <[email protected]>

Sneh1999

nit: @zacshowa can you run forge fmt to format the contracts?

contracts/parent-chain/espresso-migration/EspressoOspMigrationAction.sol

…-actions into espresso-migration

zacshowa added 4 commits August 14, 2024 15:48

Add migration contract and test.

4ebee9a

add espresso ArbOS upgrade contract

7b31c80

commit example .env files for upgrades.

38ec00d

commit yarn.lock and package.json for new dependencies.

944af15

zacshowa requested review from sveitser, alysiahuggins, ImJeremyHe and Sneh1999 August 15, 2024 00:55

zacshowa changed the title ~~Add action contrats for migrating live orbit chains to espresso~~ Add action contracts for migrating live orbit chains to espresso Aug 15, 2024

Sneh1999 reviewed Aug 15, 2024

View reviewed changes

alysiahuggins reviewed Aug 16, 2024

View reviewed changes

zacshowa added 10 commits August 19, 2024 09:50

add nitro-contracts submodule.

5f1a716

Add license identifier to contracts

a79bb05

Update the EspressoArbOSUpgrade.sol contract to import the UpgradeArb…

7098927

…OSUpgradeAtTimestampAction contract instead of duplicating it.

address PR feedback

3c35709

add nitro-contracts submodule to .gitmodules

3440e75

remove unused nitro-contracts submodule

9004894

forge install: nitro-contracts.git

637f8ef

migration-test

forge install: nitro-contracts

0ef9057

migration-test

Deployment scripts and foundry remappings

8e72997

fix broken submodule from forge install

261b147

sveitser reviewed Aug 23, 2024

View reviewed changes

contracts/parent-chain/contract-upgrades/DeployEspressoOsp.s.sol Outdated Show resolved Hide resolved

sveitser reviewed Aug 23, 2024

View reviewed changes

contracts/parent-chain/contract-upgrades/DeployEspressoOsp.s.sol Outdated Show resolved Hide resolved

zacshowa and others added 6 commits August 23, 2024 11:01

Updates to action upgrade contracts.

15b00f0

Commit with current progress: updates to contracts

3a798bd

Update the rollup address

853faad

Set adresses and module roots via env vars

c9a1eeb

This allows us to set all the config from the test script

move migration scripts to their own folder

1123bb5

Add WIP README.md for the migration

0677dca

zacshowa added 4 commits September 3, 2024 16:38

Update ArbOS action upgrade to take a timestamp parameter.

313d834

update .example-env for reference in the README.md

fb31130

Update README.md to explain the full migration process.

1fe92a4

update Migration solidity test to point to relocated migration action…

5df09a9

… contract.

sveitser reviewed Sep 5, 2024

View reviewed changes

migration/README.md Outdated Show resolved Hide resolved

sveitser reviewed Sep 5, 2024

View reviewed changes

migration/README.md Show resolved Hide resolved

sveitser reviewed Sep 5, 2024

View reviewed changes

Update migration/README.md

fcb0ff9

Co-authored-by: Mathis <[email protected]>

Sneh1999 reviewed Sep 5, 2024

View reviewed changes

contracts/parent-chain/espresso-migration/EspressoOspMigrationAction.sol Outdated Show resolved Hide resolved

zacshowa added 3 commits September 5, 2024 14:17

run forge fmt

dc57c2c

Update README.md to reflect PR feedback

c993b69

Merge branch 'espresso-migration' of github.com:EspressoSystems/orbit…

b914e97

…-actions into espresso-migration

Sneh1999 approved these changes Sep 11, 2024

View reviewed changes

sveitser changed the base branch from main to integration September 12, 2024 06:37

zacshowa merged commit 751be3f into integration Sep 12, 2024
1 check passed

zacshowa mentioned this pull request Sep 12, 2024

update orbit-actions submodule EspressoSystems/nitro-testnode#52

Merged

		@@ -0,0 +1,6 @@
		export NEW_OSP_ENTRY="0x7B5F0B437EE68A22992DdD629AA22525Fc5dfa9A"



		/* solhint-disable func-name-mixedcase */
		//create items needed for a rollup and deploy it. This code is lovingly borrowed from the rollupcreator.t.sol foundry test.


		address migration = address(new EspressoOspMigrationAction());

		vm.prank(rollupOwner);


		#### A note on Upgrade Executors

		This migration presumes there to be upgrade executor contracts that are chain/rollup owners on both the parent chain, as well as the child chain. If you do not have an upgrade executor on the child chain, the contract execution commands at the end of this guide will not work.

Add action contracts for migrating live orbit chains to espresso #1

Add action contracts for migrating live orbit chains to espresso #1

Conversation

zacshowa commented Aug 14, 2024

This PR:

This PR does not:

Key places to review:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alysiahuggins commented Aug 16, 2024

Choose a reason for hiding this comment

zacshowa Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zacshowa Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sneh1999 left a comment

Choose a reason for hiding this comment

zacshowa Aug 19, 2024 •

edited

Loading

zacshowa Aug 19, 2024 •

edited

Loading