feat: add Docker support for running upgrade scripts#65
feat: add Docker support for running upgrade scripts#65
Conversation
Add Dockerfile and CI workflow to enable running orbit-actions commands in a containerized environment without requiring local installation of Foundry and Node.js. - Add Dockerfile with Node 18, Foundry, and pre-installed dependencies - Add smoke tests to verify tools and scripts are accessible - Add GitHub Actions workflow to build and test Docker image on PRs - Update README with Docker usage instructions
There was a problem hiding this comment.
Pull request overview
This PR adds Docker support for running orbit-actions commands in a containerized environment, eliminating the need for local installation of Foundry and Node.js.
Changes:
- Added Dockerfile with Node 18, Foundry installation, and dependency setup
- Created comprehensive smoke test suite to verify Docker image functionality
- Added GitHub Actions workflow for automated Docker image testing on PRs
- Updated documentation with Docker usage examples and configuration instructions
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Dockerfile | Defines containerized environment with Node 18, Foundry, and pre-built dependencies |
| .dockerignore | Excludes unnecessary files from Docker build context for optimization |
| test/docker/test-docker.bash | Implements smoke tests verifying tools, dependencies, and scripts in Docker image |
| .github/workflows/test-docker.yml | Automates Docker image building and testing on pull requests |
| package.json | Adds test:docker script for running Docker smoke tests |
| README.md | Documents Docker usage with examples for commands, environment variables, and volume mounting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The Docker build requires lib/ (forge-std, arbitrum-sdk) which comes from git submodules. Update CI to checkout with submodules and document the prerequisite for local builds.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add a path-based CLI that allows users to browse and execute scripts from the foundry directory: - Browse directories: `docker run orbit-actions contract-upgrades/1.2.1` - View files: `docker run orbit-actions contract-upgrades/1.2.1/README.md` - Run upgrades: `docker run orbit-actions contract-upgrades/1.2.1/deploy-execute-verify` Structure: - entrypoint.sh: thin shim that sources .env and delegates to router - bin/router: path parsing, directory listing, command dispatch - bin/contract-upgrade: deploy, execute, deploy-execute-verify commands - bin/arbos-upgrade: deploy, execute, verify, deploy-execute-verify commands - lib/common.sh: shared utilities for auth parsing and forge helpers Upgrade commands read configuration from mounted .env file and accept auth flags (--deploy-key, --execute-key, --ledger, etc.) for signing.
Add Verify*.s.sol Forge scripts to each contract-upgrade version folder, replacing hardcoded verification logic with discoverable scripts. - Add verify command to bin/contract-upgrade and bin/router - Create Verify scripts for 1.2.1, 2.1.0, 2.1.2, 2.1.3 - Update READMEs to reference forge script verification
Publish offchainlabs/chain-actions image to Docker Hub: - On push to main: tag as latest - On release tags (v*): tag as version (e.g., 1.2.3, 1.2) - Manual trigger: tag with branch name (for testing) Requires DOCKERHUB_USERNAME and DOCKERHUB_TOKEN secrets.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 21 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace bash CLI (bin/router, bin/contract-upgrade, bin/arbos-upgrade, lib/common.sh) with TypeScript implementation in src/cli/. - Add commander for argument parsing - Add execa for subprocess execution - Update Dockerfile to use node entrypoint directly - Remove entrypoint.sh (no longer needed)
- Remove redundant comments that state the obvious - Add named constants for ArbOS precompiles (ARB_OWNER_PUBLIC, ARB_SYS) - Add ARBOS_VERSION_OFFSET constant with explanation - Remove unused AuthArgs interface - Remove unused getRepoRoot import - Fix double findRepoRoot() call in loadEnv()
- Remove deprecated @typescript-eslint/tslint plugin from eslint config - Update docker tests to use --entrypoint for tool access - Apply prettier formatting to src/cli files - Fix no-implicit-coercion lint errors (!! -> Boolean())
Non-Docker tests for the bin/router and related scripts.
Replace verbose Docker documentation with concise CLI section and streamlined Docker examples. Move tooling documentation to end of README to prioritize upgrade guides.
Remove fallback locations for .env files. Now only loads from process.cwd(), matching Forge's behavior for transparency.
Extract executeUpgrade() and verifyUpgrade() helpers to eliminate code duplication between standalone commands and deploy-execute-verify.
Remove redundant subcommand interface - all functionality is accessed via the router's path-based syntax. Also extract deployAction helper to reduce duplication in arbos-upgrade.
Pin foundryup to nightly-2026-02-09 for reproducible Docker builds.
- Pin to actual nightly release (2026-02-10) using commit hash - Add dist/ to eslintignore - Fix prettier formatting in arbos-upgrade.ts
The foundryup --version flag doesn't work correctly in Docker when bootstrapping from the install script. Revert to unpinned foundryup which installs the latest stable release.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 29 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/cli/utils/forge.ts
Outdated
| const result = await execa('forge', args, { | ||
| stdio: 'inherit', | ||
| env: process.env, | ||
| }) | ||
|
|
||
| if (result.exitCode !== 0) { | ||
| die(`Forge script failed with exit code ${result.exitCode}`) | ||
| } |
There was a problem hiding this comment.
execa rejects/throws by default on non-zero exit codes, so this code path likely never reaches the exitCode check (it will throw before returning result). Handle failures via try/catch + die(...), or set reject: false in the execa options and keep the explicit exitCode check.
src/cli/utils/forge.ts
Outdated
| const result = await execa('cast', args, { | ||
| stdio: 'inherit', | ||
| env: process.env, | ||
| }) | ||
|
|
||
| if (result.exitCode !== 0) { | ||
| die(`Cast send failed with exit code ${result.exitCode}`) | ||
| } |
There was a problem hiding this comment.
Same issue as runForgeScript: execa throws on non-zero by default, so the exitCode branch likely won't run. Use try/catch to call die(...) with a clear message (and optionally surface stderr), or set reject: false and keep the exitCode handling.
src/cli/utils/forge.ts
Outdated
| args.push('--skip-simulation') | ||
| } | ||
|
|
||
| const verbosity = options.verbosity ?? 3 |
There was a problem hiding this comment.
If options.verbosity is set to 0 (or a negative number), this will generate an invalid flag like -. Clamp verbosity to a sane minimum (e.g., Math.max(1, verbosity)) before building the -v... argument.
| const verbosity = options.verbosity ?? 3 | |
| const verbosity = Math.max(1, options.verbosity ?? 3) |
| .argument('[args...]', 'Additional arguments') | ||
| .allowUnknownOption(true) | ||
| .action(async (pathArg?: string, args?: string[]) => { | ||
| await router(pathArg, args) |
There was a problem hiding this comment.
This passes string[] | undefined into router's second parameter typed as string[] (with a default). TypeScript will typically reject this (even though runtime defaulting works). Fix by calling await router(pathArg, args ?? []), or by making args non-optional in the action signature.
| await router(pathArg, args) | |
| await router(pathArg, args ?? []) |
src/cli/router.ts
Outdated
| console.error(`Error: ArbOS version required`) | ||
| console.error( | ||
| `Usage: arbos-upgrades/at-timestamp/${basename} <version> [options]` | ||
| ) | ||
| process.exit(1) |
There was a problem hiding this comment.
This bypasses the shared error-handling (die) used elsewhere in the CLI, which can lead to inconsistent formatting/output and duplicated behavior. Consider replacing these console.error + process.exit(1) calls with die(...) (including the usage string) for consistency.
| console.error(`Error: ArbOS version required`) | |
| console.error( | |
| `Usage: arbos-upgrades/at-timestamp/${basename} <version> [options]` | |
| ) | |
| process.exit(1) | |
| die( | |
| `Error: ArbOS version required | |
| Usage: arbos-upgrades/at-timestamp/${basename} <version> [options]` | |
| ) |
| # --ignore-scripts: forge install runs separately after full copy | ||
| RUN yarn install --frozen-lockfile --ignore-scripts | ||
|
|
||
| COPY . . |
There was a problem hiding this comment.
The Docker build relies on Forge dependencies being present, but the Dockerfile never runs forge install (and yarn install --ignore-scripts skips the repo's prepare hook that would normally do it). This makes docker build fail unless the build context already contains lib/ from a prior host-side forge install (as your CI currently does). Add an explicit RUN forge install inside the Dockerfile (after COPY . . and before forge build) so the image builds reliably from a clean checkout.
| COPY . . | |
| COPY . . | |
| RUN forge install |
| with: | ||
| images: offchainlabs/chain-actions | ||
| tags: | | ||
| type=raw,value=latest,enable={{is_default_branch}} | ||
| type=semver,pattern={{version}} | ||
| type=semver,pattern={{major}}.{{minor}} | ||
| type=ref,event=branch |
There was a problem hiding this comment.
The publish workflow targets offchainlabs/chain-actions, but the PR description/README/Docker usage references offchainlabs/orbit-actions. If orbit-actions is the intended image name, update images: accordingly to avoid publishing to the wrong repository.
godzillaba
left a comment
There was a problem hiding this comment.
still doing review but wanted to leave a couple comments before logging off
Co-authored-by: Henry <11198460+godzillaba@users.noreply.github.com>
src/cli/utils/auth.ts
Outdated
| if (arg === '--private-key' || arg === '--account') { | ||
| const value = args[i + 1] | ||
| if (value) { | ||
| return `${arg} ${value}` | ||
| } | ||
| } | ||
| if (arg === '--ledger' || arg === '--interactive') { | ||
| return arg | ||
| } |
There was a problem hiding this comment.
do we need to support more than private key? i doubt people will use the others
There was a problem hiding this comment.
we already had these args documented in the readme:
so i assumed they were useful for something
There was a problem hiding this comment.
ok gotcha, i guess i don't really have a preference either way
There was a problem hiding this comment.
remove command line args now, use env vars only for foundry with the cli
| env?: Record<string, string> | ||
| } | ||
|
|
||
| export async function runForgeScript( |
There was a problem hiding this comment.
we should expose -g, --gas-limit-multiplier since forge scripts can be finicky with L2 gas accounting
There was a problem hiding this comment.
only use env vars now
README.md
Outdated
|
|
||
| ```bash | ||
| # Browse available scripts | ||
| yarn orbit-actions # List top-level directories |
There was a problem hiding this comment.
this command doesn't work
yarn orbit-actions
yarn run v1.22.22
warning package.json: License should be a valid SPDX license expression
error Command "orbit-actions" not found.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
There was a problem hiding this comment.
yeh, needed npm link. I've got rid of this now - can use yarn cli locally now
README.md
Outdated
|
|
||
| ```bash | ||
| # Browse available scripts | ||
| yarn orbit-actions # List top-level directories |
There was a problem hiding this comment.
also why is the default just ls? personally doesn't feel intuitive
There was a problem hiding this comment.
is it because using docker you need to explore to find paths to versions and envs and readme's etc?
if so i think some kind of tree command would be a little better
There was a problem hiding this comment.
added some more help text. we have this so that a user can run any of the scripts in docker via the cli - not hardcoded commands.
I agree that it's not very intuitive - but i dont think tree is great either since you'll still need to pass a path to run the script with that
src/cli/utils/forge.ts
Outdated
| return result.stdout.trim() | ||
| } | ||
|
|
||
| export function parseActionAddress( |
There was a problem hiding this comment.
this function implies that any action deployment script MUST deploy the action last. which is probably true for most reasonable scripts but still seems like a subtle footgun that should at least be documented
Add comments to Dockerfile and workflow, format PERFORM_SELECTOR expression, use die() instead of manual console.error/process.exit in router.
…v vars
Remove custom auth flag parsing (--private-key, --account, --ledger, etc.)
and the deploy-execute-verify combined command. Forge/cast auth and behavior
is now configured entirely via FOUNDRY_*/ETH_* env vars passed through
process.env.
- Delete src/cli/utils/auth.ts
- Simplify ForgeScriptOptions to {script, rpcUrl, env?}
- Remove authArgs from CastSendOptions
- Gate arbos cast send on FOUNDRY_BROADCAST env var
- Remove parseOptions and combined command routing
- Update env templates with FOUNDRY_* vars
- Update README with env var docs and separate-step workflow
- Log full forge args instead of truncating for easier debugging - runCastCall uses die() on failure instead of returning 'N/A' - Format scheduled upgrade output as (version, timestamp) tuple - Add contextual help text at every CLI browsing level
…text - Trim env templates to FOUNDRY_BROADCAST and ETH_PRIVATE_KEY, link to Foundry config docs for the full list of supported env vars - Remove orbit-actions prefix from CLI help text so it reads correctly regardless of invocation method (yarn cli, Docker, linked binary) - Update README env var table to match
The orbit-actions binary only worked via yarn link and isn't used in Docker (entrypoint is node directly) or local dev (yarn cli).
The [orbit-actions] prefix added no value -- the user already knows what tool they're running. Drop the wrapper and use console.log throughout for consistent, unprefixed output.
Co-authored-by: Henry <11198460+godzillaba@users.noreply.github.com>
…xecute chaining Execute commands now read the deployed action address from Forge's broadcast output when UPGRADE_ACTION_ADDRESS is not set, enabling deploy && execute chaining without manual .env edits. The env var still takes precedence as an explicit override for multisig flows.
Extract duplicate resolveActionAddress from arbos-upgrade and contract-upgrade into a shared function in forge.ts. Make parseActionAddress private and document its last-CREATE assumption.
Test was checking for deploy-execute-verify which no longer exists. Updated to check for deploy/execute/verify as separate commands. Also fixes prettier formatting.
| async function cmdDeploy(version: string): Promise<void> { | ||
| const rpcUrl = requireEnv('CHILD_CHAIN_RPC') | ||
| console.log(`Running: ${path.basename(DEPLOY_SCRIPT)} for ArbOS ${version}`) | ||
| await deployAction(version, rpcUrl) | ||
|
|
||
| const address = await resolveActionAddress(DEPLOY_SCRIPT, rpcUrl) | ||
| console.log(`Deployed action address: ${address}`) | ||
| console.log( | ||
| 'Run execute next, or set UPGRADE_ACTION_ADDRESS in .env to override' | ||
| ) | ||
| } |
There was a problem hiding this comment.
imo this function should just short circuit if env.UPGRADE_ACTION_ADDRESS is defined. currently if it's set then it will run the deploy script, then instead of printing out the new contract, it'll say:
console.log(`Deployed action address: ${process.env.UPGRADE_ACTION_ADDRESS}`)
console.log(
'Run execute next, or set UPGRADE_ACTION_ADDRESS in .env to override'
)There was a problem hiding this comment.
if broadcast isn't turned on, then i think this will also say "deployed to XXX ... run execute next ..." even though XXX has not been deployed
| rpcUrl, | ||
| }) | ||
|
|
||
| const address = await resolveActionAddress(deployScript, rpcUrl) |
There was a problem hiding this comment.
same short circuit concern here
src/cli/commands/arbos-upgrade.ts
Outdated
| console.log(`To: ${upgradeExecutor}`) | ||
| console.log(`Calldata: ${executeCalldata}`) | ||
| console.log('') | ||
| console.log('Submit this to your multisig/Safe to execute the upgrade') |
There was a problem hiding this comment.
also seems weird to say "submit to your mutlisig" right before going ahead and executing from an EOA
There was a problem hiding this comment.
just deleting that comment. useless ayway
|
|
||
| // Assumes the action contract is the last CREATE in the broadcast file. | ||
| // This holds for all current deploy scripts, which deploy dependencies first | ||
| // and the action contract last. | ||
| function parseActionAddress( |
There was a problem hiding this comment.
this assumption should also be documented in all the foundry action deployment scripts, since those will surely be used as a reference for future versions. couldn't hurt to include it in the top level readme as well
src/cli/utils/forge.ts
Outdated
|
|
||
| export async function runCastCall(options: CastCallOptions): Promise<string> { | ||
| try { | ||
| const result = await execa('cast', [ |
There was a problem hiding this comment.
is stderr visible to the user if this fails?
There was a problem hiding this comment.
should we pass {stderr: 'inherit'}?
src/cli/commands/arbos-upgrade.ts
Outdated
| await runCastSend({ | ||
| to: upgradeExecutor, | ||
| sig: 'execute(address,bytes)', | ||
| args: [actionAddress, PERFORM_SELECTOR], | ||
| rpcUrl, | ||
| }) |
There was a problem hiding this comment.
we've already generated the actual calldata so imo we should just pass it along directly to cast send.
this is the only place runCastSend is called so i think it's okay to change the function sig.
| await runCastSend({ | |
| to: upgradeExecutor, | |
| sig: 'execute(address,bytes)', | |
| args: [actionAddress, PERFORM_SELECTOR], | |
| rpcUrl, | |
| }) | |
| await runCastSend({ | |
| to: upgradeExecutor, | |
| data: executeCalldata, | |
| rpcUrl, | |
| }) |
There was a problem hiding this comment.
use data in send and call
Skip action address resolution when not broadcasting - forge runs in simulation mode and doesn't produce a broadcast file at the normal path. Also early-return from deploy if UPGRADE_ACTION_ADDRESS is already set. Remove unused getEnv helper from CLI env utils.
Replace cast calldata/sig-based interfaces with pre-encoded --data param. Encode calldata and decode results with ethers Interface, removing the async castCalldata shell-out entirely.
The CLI identifies the action contract by taking the last CREATE from the broadcast file. Add comments to all deploy scripts noting this constraint, and document it in the README for future script authors.
- Block path traversal in router (reject ".." in path args) - Anchor version regex to prevent misrouting on nested paths - Fix .dockerignore excluding .env.sample and env template files - Remove incorrect 2.1.3 verify script (checked nativeTokenDecimals, a 2.1.2 concern; reverts on ETH-native chains) - Add ROLLUP env var to 1.2.1 and 2.1.0 env templates for verify scripts - Use node:22 and foundryup --version stable in Dockerfile - Use submodules: recursive in CI instead of forge install - Add broadcast guidance on arbos execute without FOUNDRY_BROADCAST - Add missing deploy script null check in contract-upgrade cmdExecute - Wrap getChainId with try/catch for consistent error handling - Load .env from repo root instead of cwd for path consistency - Warn on fallback to /app when repo root not found - Drop isCategoryDir messaging in directory listing
foundryup was rewritten and --version now prints the tool version instead of installing a specific Foundry version. The new flag is --install.
Add Dockerfile and CI workflow to enable running orbit-actions commands in a containerized environment without requiring local installation of Foundry and Node.js.
fixes: