Espresso 1: Contracts#443
Conversation
4bc5939 to
cbd97f9
Compare
Adds the Espresso-introduced contracts and the minimum supporting changes
required for them to compile, test, and pass the contract checks.
New contracts and scripts:
- src/L1/BatchAuthenticator.sol and interfaces/L1/IBatchAuthenticator.sol
(upgradeable contract that authenticates batch transactions, with switching
between Espresso and fallback batchers)
- scripts/deploy/DeployBatchAuthenticator.s.sol and
scripts/deploy/DeployEspresso.s.sol
- test/L1/BatchAuthenticator.t.sol and test/mocks/MockEspressoTEEVerifiers.sol
- snapshots/{abi,storageLayout}/BatchAuthenticator.json
- snapshots/semver-lock.json entry for BatchAuthenticator
New submodules:
- lib/espresso-tee-contracts (interfaces required by BatchAuthenticator)
- lib/openzeppelin-contracts-upgradeable-v5 (OZ v5 used by BatchAuthenticator
via OwnableUpgradeable)
Supporting changes (Espresso-driven):
- foundry.toml: remappings for OZ v5 and espresso-tee-contracts; ignored
warning codes for vendored libs; OOM-safe jobs settings; via-ir profile.
- justfile: fix-proxy-artifact recipe to handle OZ v5 shadowing Proxy/ProxyAdmin
artifacts; build/coverage hooks.
- src/universal/Proxy.sol, src/universal/ProxyAdmin.sol: pin pragma to exact
0.8.15 so they stay in their own compilation group and never emit PUSH0.
- src/universal/ReinitializableBase.sol: loosen pragma to ^0.8.15 so
BatchAuthenticator (compiled with OZ v5) can import it.
- scripts/* and test/*: disambiguate Proxy artifact lookups to
src/universal/Proxy.sol:Proxy (avoids OZ v5 proxy/Proxy.sol shadow).
- scripts/checks: bypass interface checks for artifacts originating from lib/;
add Espresso-related contract names to exclude lists; pragma exclusions for
Proxy/ProxyAdmin/BatchAuthenticator.
- test/vendor/Initializable.t.sol: exclude BatchAuthenticator (deployed by a
separate Espresso script).
Co-authored-by: OpenCode <noreply@opencode.ai>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee2d984108
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-authored-by: piersy <pierspowlesland@gmail.com>
- strict-pragma: remove unneeded exclusions for src/universal/Proxy.sol and src/universal/ProxyAdmin.sol — both already use strict 'pragma solidity 0.8.15;', so the entries (and their misleading comment claiming '^') were dead. - interfaces: move the Espresso excludeContracts block out of the upstream-shared area and down next to the Celo block, with one entry per line to match the surrounding style. Localizes future rebase deltas. Co-authored-by: OpenCode <noreply@opencode.ai>
Inline the EspressoTEEVerifier deployment in DeployEspresso.s.sol so it
no longer imports lib/espresso-tee-contracts/scripts/DeployTEEVerifier.s.sol
or DeployNitroTEEVerifier.s.sol. The upstream scripts pulled OZ v5's
TransparentUpgradeableProxy (and its auto-deployed ProxyAdmin) into the
OP artifact tree, shadowing src/universal/ProxyAdmin.sol and forcing a
~90-line fix-proxy-artifact justfile recipe.
The TEEVerifier is now deployed behind src/universal/Proxy.sol +
src/universal/ProxyAdmin.sol, matching how BatchAuthenticator is
deployed in the same script. ERC-1967 slots are unchanged, so external
callers see no difference.
The raw vm.getCode("ProxyAdmin") lookups in the deploy scripts and
BatchAuthenticator tests are switched to the explicit artifact path
vm.getCode("forge-artifacts/ProxyAdmin.sol/ProxyAdmin.json") to
deterministically resolve the default compilation profile's bytecode
(the dispute profile transitively compiles ProxyAdmin at optimizer_runs=5000,
creating a second artifact that broke unqualified lookups).
The fix-proxy-artifact recipe and its 5 callsites are removed.
Cherry-picked from piersy's commit 5d0a803 on PR ethereum-optimism#443. Walks the dual-batcher state machine: Espresso path → switchBatcher → fallback path → switchBatcher → Espresso path. Asserts every transition emits the expected event, that signer registration survives the round-trip, and that re-issuing the same call after a mode flip changes the outcome (the previously-valid Espresso signature is no longer consulted on the fallback path). Co-authored-by: Piers Powlesland <pierspowlesland@gmail.com> Co-authored-by: OpenCode <noreply@opencode.ai>
|
Hey @QuentinI! I have authorized to run CircleCI tests from your fork. May you commit something to trigger execution? Also I run manually the contract tests and found two are failing: |
| @@ -84,6 +108,13 @@ ffi = true | |||
| # you increase the gas limit above this value it must be a string. | |||
| gas_limit = 9223372036854775807 | |||
|
|
|||
| # Disable forge lint during build so 287+ linter warnings (e.g. unsafe-typecast) don't fail the build. | |||
| # Run `forge lint` separately when fixing style. | |||
| # Note: [lint] is a Foundry 1.5+ top-level section. Forge 1.2.x treats it as a deprecated profile | |||
| # notation and emits a harmless warning; lint_on_build has no effect there (feature didn't exist yet). | |||
| [lint] | |||
| lint_on_build = false | |||
|
|
|||
There was a problem hiding this comment.
Hey @QuentinI I'm wondering if we could resolve these workarounds in the way outlined below?
A few of the foundry.toml workarounds in this PR look like symptoms of the same root cause — the deploy scripts are pulling the entire espresso-tee-contracts impl chain into OP's compile groups. Specifically:
ignored_error_codesadds6321,5667,1878— these are coming from compiling submodule mocks (5667unused param), submodule scripts (1878missing SPDX), and other submodule code (6321unnamed return). The suppression is global, so any matching warning in OP's ownsrc/would now compile silently.[lint] lint_on_build = falsesilences 287+ warnings (unsafe-typecastetc.) — again mostly from the submodule, and again globally, so new lint regressions in OP's own code slip through.jobs = 1in four profiles (ci,cicoverage,ciheavy,lite) plusthreads = 4— band-aid for OOM caused by the 211-file espresso compile group running alongside the 530-file OP group on 16 GB runners. Real CI wall-time cost on every PR.
All three could be mitigated by one structural change: switch DeployEspresso.s.sol to use vm.getCode against the submodule's own out/ for the implementation contracts, mirroring the pattern this PR already uses for ProxyAdmin at DeployBatchAuthenticator.s.sol:50-58. Concretely:
- Pre-build the submodule (it has its own
foundry.tomlpinning solc 0.8.25):forge build --root lib/espresso-tee-contractsas a CI/Makefile step before OP's main build. - Drop the impl imports at
DeployEspresso.s.sol:12-13(EspressoTEEVerifier,EspressoNitroTEEVerifier). Keep the interface imports — they're tiny. - Replace the two
new EspressoTEEVerifier(...)/new EspressoNitroTEEVerifier(...)calls with the samevm.getCode+assembly { create(...) }pattern asProxyAdmin, pointing atlib/espresso-tee-contracts/out/...json. - Add
{ access = 'read', path = './lib/espresso-tee-contracts/out/' }tofs_permissions.
That eliminates the impl closure (TEEHelper, JournalValidation, and the chunk of aws-nitro-enclave-attestation reachable from it) from OP's Solc invocations — probably ~150 of the 211 files in the espresso group. What remains is just the inheritance closure from BatchAuthenticator → OwnableWithGuardiansUpgradeable → OZ, which can't be split this way but is ~30 files, not 200+.
Once the group is that small, jobs = 1 and threads = 4 should come off, and the three new ignored_error_codes + lint_on_build = false go with them since OP's build no longer compiles the submodule's mocks/scripts that emit those warnings.
There was a problem hiding this comment.
This sounds very reasonable, I will try it out!
There was a problem hiding this comment.
This does seems to allow us to drop the new error codes and lint_on_build, but the compilation groups don't shrink perceptibly. I'll see if I can do anything to fix that part. Implemented everything else in in 348fed7
There was a problem hiding this comment.
Actually, in my testing this PR seems to pull in only 28 new files in 0.8.28 group and doesn't increase other groups' sizes at all.
There was a problem hiding this comment.
@QuentinI cool, can we remove the ci jobs and threads limiting in the foundry.toml file?
There was a problem hiding this comment.
I think so, yes. Ideally we'd do a run with and without, but CircleCI doesn't want to execute contract tests on this PR for some reason
We cannot run this job using the selected resource class. Please check your configuration and try again.
cc @jcortejoso is this expected behavior? The PR doesn't touch any CI configuration
Replace the hand-rolled `EspressoBatcherEntry[]` history + binary search with OpenZeppelin's `Checkpoints.Trace160` (`(uint96 key, uint160 value)`). `uint160` is exactly an address with no waste, and `uint96` easily covers L1 block numbers. `upperLookupRecent` replaces the custom binary search and the same-block-overwrite branch is now handled inside `_insert`. Co-authored-by: OpenCode <noreply@opencode.ai>
|
@jcortejoso I have fixed the two tests; thank you for approving the CI - it CI seems to pass except for the workflows that refuse to run altogether |
…trictions entries src/L1/StandardValidator.sol was renamed to OPContractsManagerStandardValidator.sol upstream in ethereum-optimism#16237; the old path was a rebase artifact with no corresponding file. Co-authored-by: OpenCode <noreply@opencode.ai>
…sed warning codes Drop the two impl imports (EspressoTEEVerifier, EspressoNitroTEEVerifier) from DeployEspresso.s.sol and replace direct instantiation with vm.getCode + assembly create, reading bytecode from the submodule's own out/ directory. This removes the impl closure (TEEHelper, JournalValidation, and the aws-nitro-enclave-attestation chain) from OP's solc invocations. The impls are still parsed/ABI-checked by forge via libs=['lib'], but they no longer require bytecode emission or the optimizer backend. Since OP's build no longer compiles the submodule's impl files, the three error codes those files triggered (6321 unnamed return, 5667 unused param, 1878 missing SPDX) can be removed from ignored_error_codes. OP's own code does not trigger any of them. The lint_on_build=false workaround is also removed for the same reason — with the impl closure gone, forge lint reports 283 warnings (all from OP's own code), none of which cause a build failure. Adds fs_permissions read access for lib/espresso-tee-contracts/out/ so vm.getCode can locate the pre-built artifacts. The submodule must be built (forge build --root lib/espresso-tee-contracts) before OP's main build. Co-authored-by: OpenCode <noreply@opencode.ai>
Deploy the BatchAuthenticator and TEEVerifier proxies behind the existing OP Stack ProxyAdmin instead of dedicated ones (celo-org#443). Both proxies use the deployer as a transient admin to initialize directly, then changeAdmin to the shared ProxyAdmin (DeployAltDA/DeployFeesDepositor pattern). Reorder TEE deploy so the Nitro verifier is wired via initialize, removing the post-init onlyOwner call and ownership-transfer dance. Rename inputs to espressoOwner/sharedProxyAdmin and drop the teeVerifierProxyAdmin output. Co-authored-by: OpenCode <noreply@opencode.ai>
Co-authored-by: OpenCode <noreply@opencode.ai>
|
Hi @QuentinI I think we are getting close to completing this one, just wating for some feedback from @jcortejoso & @Mc01 on the last couple of comments. |
This PR pulls in the
BatchAuthenticatorpart of Espresso integration atpackages/contracts-bedrock/src/L1/BatchAuthenticator.sol. All other changes are deployment scripts, tests, and other wiring.BatchAuthenticatoris the contract that the batchers will have to authenticate their batches with; corresponding changes for the derivation pipeline and batchers forthcoming as subsequent PRs.