L1 Contracts Developer Vision and UX #177
Conversation
mds1
requested review from
ajsutton,
karlfloersch,
geoknee,
sebastianst,
tynes,
clabby,
smartcontracts,
blmalone,
maurelian and
skeletor-spaceman
mds1
changed the title
|
|
||
| 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)`. |
There was a problem hiding this comment.
nit, would it make it better if we used a struct(s) rather than hardcoded strings?
There was a problem hiding this comment.
I believe this is using the already implemented Addresses.sol from Forge Proposal Simulator. I'll tag the team here to add color.
There was a problem hiding this comment.
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
First the string that identifies the contract is passed, then the L2 ChainId is passed.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| ## Execution of Onchain Transactions | ||
|
|
||
| With new implementation contracts deployed, we can prepare playbooks, or tasks, in superchain-ops. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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.
| 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). |
There was a problem hiding this comment.
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
| 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 |
There was a problem hiding this comment.
| 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 |
| 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. |
There was a problem hiding this comment.
| The proposed UX upon for L1 smart contract development is described below. | |
| The proposed UX for L1 smart contract development is described below. |
| 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 |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
| 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 |
|
|
||
| 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. |
There was a problem hiding this comment.
Checking that I broadly understand the order of operations here for Isthmus Cycle 34:
- Identify the release candidate of
op-contracts/v2.0.0that we're happy with e.g.op-contracts/v2.0.0-rc.1 - Deploy implementation contracts via op-deployer
bootstrapfor release candidateop-contracts/v2.0.0-rc.1(this gives us an OPCM deployment address) - Get governance approval and then call the OPCM
upgradefunction. Upon invokingupgrade, the release candidate suffix is programmatically stripped and the deployments version now becomesop-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.
| 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, | ||
| 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. |
There was a problem hiding this comment.
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.
| 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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This matches my understanding of the discussions we've been having well. The golden rule for me is "keep develop releasable" - that's the basis of continuous integration and it's really really powerful. We often think we can take a shortcut because we are "100% sure" this change will be in the next release and then we discover something or there's a change of plans and it costs us more to get back to a releasable state than what we saved. And the more you do it the right way, the more tooling/patterns/knowledge we have and the easier it gets.
| 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). |
There was a problem hiding this comment.
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).
| 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. |
There was a problem hiding this comment.
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".
| 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 |
There was a problem hiding this comment.
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.
This design doc describes the vision we are working towards with the OPCM Upgrades and superchain-ops projects that the EVM Safety team is working on. We have spoken about these flows and ideas in various conversations and docs, but want to get the overall end goal we are working towards in a centralized place to ensure we are all aligned
Some details still need to be figured out, and suggestions on those areas is welcome, but the high level flow and UX is the key aspect here.