optimism
optimism copied to clipboard
contracts-bedrock: Sys config contract and L2 contract updates
Description
This introduces the SystemConfig
contract and L2 contract changes written by @tynes, with some minor modifications to use initialize
for integration with proxy etc. things in the chain-ops PR.
This is part 2/5 to introduce batcher-key and GPO param configuration:
- Derivation aware of system config (with placeholders for later actual contract/event changes) - #3787
- Contract changes: system config on L1, and updated L2 contracts - #3788
- Update bindings, update chainops to use updated bindings and dev-deploy system config, and misc integration like log parsing - #3789
- Action-test the batcher key change and GPO params change. - #3790
- Spec updates
Do not merge: this is a breaking change, and the other parts of the PR stack should be reviewed first.
Note: CI will fail on op-bindings, these bindings get their own PR.
Changes:
- L1 Block info contract now holds on to GPO params and batcher. We should maybe just rename it to L1 info contract, the scope is a bit expanded.
- GPO contract just reads the block info contract - this is for backwards compatibility. The gas cost is lower to keep storage in one contract
- SystemConfig on L1, owner by some account/contract, can change the batcher key and GPO params. On a change an event is emitted that can then be picked up by L2 derivation.
⚠️ No Changeset found
Latest commit: f36e7f8fa96e58f4af4d53e29819a2a830acd859
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
This PR includes no changesets
When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
This PR changes implementation code, but doesn't include a changeset. Did you forget to add one?
Do we want to emit the same events that are on the GPO to make it easy for data analytics? It will now require some abi decoding to get that data
- in the system config contract?
- in the L1Block contract but only if there is a diff?
/**
* @notice Emitted when the overhead value is updated.
*/
event OverheadUpdated(uint256 overhead);
/**
* @notice Emitted when the scalar value is updated.
*/
event ScalarUpdated(uint256 scalar);
@tynes
in the system config contract?
There's no current dependency on such L1 events afaik, so I wouldn't add it there. And we do have the generic events, which analytics would want to support anyway, since it captures all kinds of updates.
in the L1Block contract but only if there is a diff?
Since the tx receipts now get the exact fee data already I don't think analytics solutions actually have to rely on the L2 events? Do we ever need to log-filter for L1 fee changes on L2, when the data is in transaction receipts already?
Hey @protolambda! This PR has merge conflicts. Please fix them before continuing review.
Hey @protolambda! This PR has merge conflicts. Please fix them before continuing review.
@tynes can you help update the gas-snapshot? (Locally it fails when running diff fuzz tests with some typescript module errors, not sure what's wrong). PR is ready for review after that. And we have ENG-2949 to track solidity testing. The sysconfig functionality is covered in Go action testing with the other PRs, so I think we can split out the solidity testing to unblock the other PRs.
I'm updating the gas snapshot, I'm also not opposed to changing the base that these commits are being opened as a PR against, I feel like the contracts changes are self contained to the point where they can be opened against develop
Unclear why the hive p2p tests are failing here, must be a flake
Semgrep found 1 unchecked-type-assertion
finding:
- op-bindings/bindings/l1block.go: L308
Unchecked type assertion.
Created by unchecked-type-assertion.
Fighting the tests here a bit, we need a plan of action with merging this stuff because I think the tests here are failing due to needing changes in a different PR that is in this set of PRs, cc @protolambda
Hey @protolambda! This PR has merge conflicts. Please fix them before continuing review.
Rebased + updated gas snapshot and bindings during merge conflicts where necessary
Hmm, the fuzz-op-node one fails because the fuzzing test updates are in the next PR of the stack. Might pull those updates into this PR, will see how isolated those change are first, it may just have to stay in that next PR.
@protolambda it's OK to leave in the next PR if it's fixed in that part of the stack. We'll merge these in quick succession.
Semgrep found 6 unchecked-type-assertion
findings:
Unchecked type assertion.
Created by unchecked-type-assertion.