Skip to content

L1 Contracts Developer Vision and UX #177

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 159 additions & 0 deletions security/l1-contracts-dev-flow-vision.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
# Purpose

The goal of this design doc is to outline the vision we are working towards for the L1 contracts
development flow, so we can collect feedback from stakeholders on what we are working towards to
ensure we are all aligned on the vision.

# Summary

<!-- Most (if not all) documents should have a summary.
While the length will likely be proportional to the length of the full document,
the summary should be as succinct as possible. -->

The EVM Safety team is working on two projects—OPCM (OP Contracts Managr) Upgrades and superchain-ops
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The EVM Safety team is working on two projects—OPCM (OP Contracts Managr) Upgrades and superchain-ops
The EVM Safety team is working on two projects—OPCM (OP Contracts Manager) Upgrades and superchain-ops

improvements—that aim to improve the tools and standardize the processes around the full L1 smart
contract development cycle. This includes development, testing, releasing, deployment, and upgrading.

# Problem Statement + Context

<!-- Describe the specific problem that the document is seeking to address as well
as information needed to understand the problem and design space.
If more information is needed on the costs of the problem,
this is a good place to that information. -->

As the number of chains we need to manage increases, our existing L1 contract development processes
have proven to be insufficient, and will not scale well. We need to tighten up this end to end flow
with robust, easy to use tools and well-defined processes. Some existing issues we've had are:

- Deployment and upgrade processes are not standardized.
- Confusion around smart contract releases.
- Calldata and state changes are not part of governance proposals and not verifiable by governance.
- Task development and validation is a manual process that is time-consuming and error-prone.
- We do not have a good way to track the full changelog for a given contracts release.

# Proposed Solution

<!-- A high level overview of the proposed solution.
When there are multiple alternatives there should be an explanation
of why one solution was picked over other solutions.
As a rule of thumb, including code snippets (except for defining an external API)
is likely too low level. -->

The proposed UX upon for L1 smart contract development is described below.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The proposed UX upon for L1 smart contract development is described below.
The proposed UX for L1 smart contract development is described below.

This UX will be achieved once the ongoing OPCM Upgrades and superchain-ops projects are completed.
We start from assuming that `op-contracts/v2.0.0` with OPCM Upgrades is shipped as part of Isthmus
in Cycle 34. We intentionally exclude the upgrade to `op-contracts/v2.0.0` in this section, as it
may have some unique aspects because it puts this system in place, although we expect it to be
largely similar to the flow described here.

## Development

A team starts working on a feature. If, and only if, they are 100% sure this feature is going
into the next release, feature work can happen on contracts directly. Otherwise the feature must be
developed using the inheritance pattern described in [`smart-contract-feature-development.md`](../smart-contract-feature-development.md).
Comment on lines +51 to +53
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would express this as requiring the tip of develop to always be releasable. You can never be 100% sure that a change will go into the next release unless it is merged as a single PR because you never know when an emergency release may be required or something else happens that changes the plan. So basically stuff that's small enough to ship to production with a single PR can just be done, everything else needs some form of feature toggle (the inheritance pattern).


For example, if you are adding something to the SystemConfig that will ship in the next release, make
those changes directly to `SystemConfig.sol`. Tests would be added directly to `SystemConfig.t.sol` like normal.

If your feature may not go into the next release, create a child contract such as `contract SystemConfigInterop is SystemConfig`.
In this contract you can add your feature, and once we know it's ready for release it will be merged
into the parent contract it inherits from. Tests for `SystemConfigInterop` would live in `SystemConfigInterop.t.sol`
(TODO flesh out more details on how this should work with OPCM, e.g. do we need an `OPCM*` for each feature, like
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to decided on a path forward for interop asap. With the upcoming onsite, we will want to use OPCM to deploy the system. I lean towards an OPCMInterop sort of design. Designing a diff on top of base OPCM that executes after base OPCM seems more complex to think about.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, op-deployer currently allows users to specify a useInterop flag when deploying.

If a user chooses to deploy a chain with interop enabled, op-deployer deploys our OPContractsManagerInterop contract.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does seem like a really key part of the design. A key part of it will be making it easy to toggle specific parts of upgrades on and off. We will often work on multiple things at once (or change the order things ship) and don't want that to always involve a whole bunch of rebasing/rewriting OPCM child contracts. I'd suggest looking for a way to have upgrade "tasks" that are the smallest unit we might reasonably ship. We make sensible trade offs between flexibility in what we ship and complexity of reasoning about interactions between tasks etc, but importantly we wouldn't just have a Isthmus upgrade task, we'd have separate tasks for each feature in Isthmus.

Then we'd need a way to bundle different tasks together in supported configurations. The OPCM* pattern with child contracts could work well for that. I'm hesitant to just have booleans for it as that tends to lead to a combinatorial explosion of possible configurations. We want the flexibility to change the combination we ship, but to reduce testing cost we want a small set of configurations that are actually "in the development pipeline" so that we can dedicate testing to those.

`OPCMInterop`, etc? A simpler approach may be to always use OPCM as the base standard deploy, and write a `interopUpgrade()`
method that contains the code that will eventually go in `OPCM.upgrade`).

An alternate approach to inheritance is to put code directly into `SystemConfig`, but behind a
feature flag. This is the approach client code uses so they can ship releases without also shipping
features that are not production ready. However, in safety-critical systems dead code is typically
not allowed, so we likely will not allowed this approach for contracts. However, we mention it here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
not allowed, so we likely will not allowed this approach for contracts. However, we mention it here
not allowed, so we likely will not allow this approach for contracts. However, we mention it here

anyway for completeness.

## Testing

In the above section we described how standard unit tests would be written during development. Another
aspect is how deploy and upgrade scripts are written and tested during development, as this is a key
change.

Originally, `Deploy.s.sol` was the script that run as part of test `setUp`, so we can run tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just linking out to this github issue as I don't see it mentioned here: ethereum-optimism/optimism#13331

The same solidity code that is called by op-deployer should be called by a test setup script to populate the unit tests. There should be no ffi out to an external process to get JSON to populate the state for solidity unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah +1 on this. The tests should compose the Solidity scripts that OP Deployer uses under the hood. If we want to make defining test chains in Solidity more declarative, we can build that into the Solidity itself.

IMO it's a smell every time we FFI out to something from Solidity.

against the state resulting from the deploy script. In practice, that `Deploy.s.sol` script was
never used for real deploys. So `Deploy.s.sol` has been refactored to instead use OPCM to deploy
a chain's L1 contracts and use that fresh deploy as the base for tests. This means that all tests
will be known to pass against the state resulting after a new chain deploy. (In the future, we may
delete the full `Deploy.s.sol` file and replace it with OPCM code directly, as embedding OPCM into
this legacy deploy script was only done as it's a simpler transition).

OPCM contains a `deploy()` method that takes some arguments to deploy a chain. Some changes to
source code will require no changes to this method—for example, if you only change things like
code comments or function internals. Other changes will require changes to this method, like
adding new contracts or modifying the `initialize` signature. For the latter case, because OPCM
is what's used to scaffold chains for testing, you will be forced to modify the `deploy` method to
account for your changes in order to test them. This is great, as it's now trivial to keep deploy
scripts up to date, and we essentially get this for free.

OPCM will also contain an `upgrade` method as described in the [OPCM Upgrades design doc](l1-upgrades.md).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be super clear, the upgrade method is used to upgrade from the previous version to the same version that is deployed via the deploy method

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

Some basic unit tests will test this method. Additionally, a flag can be set to have tests
run against the state resulting from running `OPCM.upgrade` against an existing chain (instead of the
default where tests are run against the state resulting from a fresh deploy via `OPCM.deploy()`). CI will be
responsible for running the tests against the `upgrade` method for all mainnet and sepolia chains
that the Security Council + Optimism Foundation (the 2/2) hold keys for. This enforces that
developers are also writing the upgrade logic at the same time, to provide guarantees that the full
test suite passes for new deploys and upgrades for all existing chains.
Comment on lines +97 to +100
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly are "the tests"? I'd expect most of our e2e tests and possibly even contract tests to fail when used with existing chain data like this.

It would make a lot of sense though to have dedicated "compatibility tests" which can assert a bunch of different invariants. We can then upgrade all chains and check those invariants again. Invariants might be "can't withdraw using an in-progress dispute game" like what the superchain-registry checks (possibly exactly those) but more powerful would be if the invariants can capture data before the upgrade and assert after. Then we can assert invariants like "balance of OptimismPortal is unchanged by the upgrade".


## Releases and Contract Deployments

After development, we are now ready to cut a release candidate and deploy the new implementation
contracts to a chain, such as devnet, sepolia, or mainnet. All contract deployments must be done
against a tag of the form `op-contracts/vX.Y.Z[-rc.n]`. To deploy the contract, create a `bootstrap`
command in op-deployer, and run that new command to deploy the new contracts.

An alternate approach that we might support that avoids the need for bootstrap commands is using
CREATE2 in the `DeploySuperchain.s.sol` and `DeployImplementations.s.sol` scripts. This way,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea - there'd be just one bootstrap command to deploy the implementations in this case. We should take care to minimize the amount of configuration required to deploy the implementations so that deploying individual contracts is easy.

deploying contracts is as easy as re-running those scripts—if bytecode for a contract has not changed,
the resulting create2 address is the same, so we can skip deploying that unchanged contract.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By doing this, are we loosing the ability to map all implementation contracts to a single commit? I like the side affect of having all implementations on the same commit. But I see positives in the create2 approach too.


The current [release and versioning process](https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/meta/VERSIONING.md)
is unintuitive and requires a lot of manual work. It is not possible to have the source code for the
entirety of a release at a single commit, and therefore very difficult to test contract releases.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking that I broadly understand the order of operations here for Isthmus Cycle 34:

  1. Identify the release candidate of op-contracts/v2.0.0 that we're happy with e.g. op-contracts/v2.0.0-rc.1
  2. Deploy implementation contracts via op-deployer bootstrap for release candidate op-contracts/v2.0.0-rc.1 (this gives us an OPCM deployment address)
  3. Get governance approval and then call the OPCM upgrade function. Upon invoking upgrade, the release candidate suffix is programmatically stripped and the deployments version now becomes op-contracts/v2.0.0. Ref: Contract Versioning - L1 Upgrades

The DeployImplementations script will write the version into the OPCM, along with an rc tag. The rc tag will be programmatically removed when upgrade() is called by the Upgrade Controller multisig, which signifies governance approval.

Is this accurate? If so, I think it's worth adding something similar into this doc.

Additionally, deploy and upgrade scripts are not part of a release.

Now, OPCM serves as the source of truth for what a release is. A release is defined as the OPCM contract
itself, along with all contracts that are used in the `deploy()` or `upgrade()` methods of OPCM.

## Execution of Onchain Transactions

With new implementation contracts deployed, we can prepare playbooks, or tasks, in superchain-ops.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move away from gnosis safe JSON being the input to the superchain-ops tasks? We don't use the UI anyways, its not safe enough to use because it doesn't allow for the various security checks that we do (local simulation + assertions, observe the various hashes and compare to ledger).

Looking at the work that the protocol team has to do to generate the JSON for the superchain-ops task (ethereum-optimism/optimism#13412) it is WAY to complex. We need something that is dead simple. It sounds like we will just write a forge script now? And have access to a "superchain-ops standard library" of sorts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, writing forge scripts is the plan. From disussions with @mds1, we want to get as close as possible to having everything just be a template.
Even upgrades should mostly reuse code from an upgrade library, although it will vary somewhat depending on what new config values are introduced.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is for everything to be a template, so no new solidity code needs to be written for any proposals. You just create a new toml config file for the proposal you want to run, and then run a forge script passing the toml config file path.

If a new type of proposal is created that cannot be templatized, then it can be written in plain Solidity like so:

Screenshot 2024-12-20 at 2 10 47 PM

This new way of doing things will not have any more JSON files with calldata in them. Instead, everything will be as human readable as possible.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This toml config file https://github.com/solidity-labs-io/superchain-ops/pull/10/files#diff-83b78f6d8fd1a6be20fd6fcebebf9b523e0beb9d40f539a0b330eff579d945b4 shows some of how we may templatize changing the gas limit in future proposals. This PR is very much still a WIP.

Once we have finished setting up the scaffolding, we will fully flush out templates and docs for a smooth task devex.

There will be two ways to create tasks: bespoke tasks, and template tasks.

Bespoke tasks are for one-off tasks that are not expected to be repeated. These will be created
by running a command such as `just new-task {l1ChainId} {taskName}`. This will scaffold a new
directory. In this directory would be a solidity file that inherits from some base contract. In here
you write two things: the calls to execute, and validations. The validations are similar to what we
currently do, and the calls to execute are different—instead of specifying the calldata in an `input.json`
file, calls are specified in solidity as if you are writing a regular forge script. The tooling
will extract the sequence of calls from the script and generate the calldata bundle.

Template tasks are for tasks that are expected to be repeated. These will be created by creating
a solidity template of the same format as a bespoke task, but with some parts replaced with variables.
These variables are read from a TOML file, and the path to that TOML file is the only input to the
template script. To create a new task from a template you would run a command such as
`just new-task-from-template {l1ChainId} {taskName} {templateName}`. This will scaffold a new directory,
but there will be no solidity file. Instead, a TOML template will be copied into the directory and
populated as required.

There will also be some other UX and safety improvements for writing these solidity files.

- Addresses for a chain can be fetched in Solidity via `addresses.getAddress("ContractName", l2ChainId)`.
Copy link
Contributor

@skeletor-spaceman skeletor-spaceman Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, would it make it better if we used a struct(s) rather than hardcoded strings?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is using the already implemented Addresses.sol from Forge Proposal Simulator. I'll tag the team here to add color.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests give some color to how Addresses will be fetched in the superchain-ops repo: https://github.com/ethereum-optimism/superchain-ops/blob/main/test/AddressRegistry.t.sol#L28-L42

Screenshot 2024-12-20 at 2 13 51 PM

First the string that identifies the contract is passed, then the L2 ChainId is passed.

- CI will automatically run for tasks that have not yet been executed.
- Commands to help autogenerate the `VALIDATIONS.md` file as much as possible.
- Additional safety checks, such as storage layout compatibility and data hash verification.

The data hash verification in particular is an important one. Currently, upgrade scripts are not part
of governance proposals—only contract bytecode is. The OPCM changes described above make it trivial to
add deploy and upgrade scripts to the governance proposals. As a result, because the OPCM address and
the inputs to the `upgrade()` call will be known at the time of the governance post, we can precompute
the data hash and include it in the governance proposal.

This means Security Council and Optimism Foundation signers only need to verify that the hash shown on
their ledger matches the hash in the governance proposal. If it does, we know all state changes are
what's expected and what governance has approved. This is much simpler and less error prone than the
current approach, which requires signers to validate every individual state change.