Commit 21618167 authored by Adrian Sutton's avatar Adrian Sutton

Merge branch 'develop' into aj/cache-provider

parents 1e6a656f ec11b3c5
---
'@eth-optimism/sdk': patch
---
Simplifies getMessageStatus to use an O(1) lookup instead of an event query
......@@ -131,7 +131,7 @@ jobs:
name: git submodules
command: make submodules
- check-changed:
patterns: op-chain-ops,packages/
patterns: op-chain-ops,packages/,op-node
- restore_cache:
name: Restore PNPM Package Cache
keys:
......@@ -1542,11 +1542,6 @@ workflows:
- check-generated-mocks-op-service
- cannon-go-lint-and-test
- cannon-build-test-vectors
- check-values-match:
pattern_file1: "uint8 internal constant INITIALIZER ="
pattern_file2: "const InitializedValue ="
file1_path: "packages/contracts-bedrock/src/libraries/Constants.sol"
file2_path: "op-chain-ops/genesis/config.go"
release:
when:
not:
......
......@@ -30,6 +30,8 @@ We recommend using the [Conventional Commits](https://www.conventionalcommits.or
Unless your PR is ready for immediate review and merging, please mark it as 'draft' (or simply do not open a PR yet).
Once ready for review, make sure to include a thorough PR description to help reviewers. You can read more about the guidelines for opening PRs in the [PR Guidelines](./handbook/pr-guidelines.md) file.
**Bonus:** Add comments to the diff under the "Files Changed" tab on the PR page to clarify any sections where you think we might have questions about the approach taken.
### Response time:
......
# Pull Request Guidelines and Best Practices
<!-- START doctoc generated TOC please keep comment here to allow auto update -->
<!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->
- [Overview](#overview)
- [PR Lifecycle Best Practices](#pr-lifecycle-best-practices)
- [Before Starting PRs](#before-starting-prs)
- [Opening PRs](#opening-prs)
- [Reviewing PRs](#reviewing-prs)
- [Merging PRs](#merging-prs)
<!-- END doctoc generated TOC please keep comment here to allow auto update -->
## Overview
This document contains guidelines best practices for PRs that should be enforced as much as possible. The motivations and goals behind these best practices are:
- **Ensure thorough reviews**: By the time the PR is merged, at least one other person—because there is always at least one reviewer—should understand the PR’s changes just as well as the PR author. This helps improve security by reducing bugs and single points of failure (i.e. there should never be only one person who understands certain code).
- **Reduce PR churn**: PRs should be quickly reviewable and mergeable without much churn (both in terms of code rewrites and comment cycles). This saves time by reducing the number of rebases due to conflicts. Similarly, too many review cycles is a burden for both PR authors and reviewers, and results in “review fatigue” where reviews become less careful and thorough, increasing the likelihood of bugs.
- **Traceability**: We should be able to look back at issues and PRs to understand why a certain decision was made or why a given approach was taken.
## PR Lifecycle Best Practices
This is organized by current state of PR, so it can be easily be referenced frequently to help internalize the guidelines.
### Before Starting PRs
- **Keep PRs Focused**: Each PR should be a single, narrow, well-defined scope.
### Opening PRs
- **Review Your Own Code**: Reviewing the diff yourself *in a different context*, can be very useful for discovering issues, typos, and bugs before opening the PR. For example, write code in your IDE, then review it in the GitHub diff view. The perspective change forces you to slow down and helps reveal issues you may have missed.
- **Explain Decisions/Tradeoffs**: Explain rationale for any design/architecture decisions and implementation details in the PR description. If it closes an issue, remember to mention the issue it closes, e.g. `Closes <issueUrl>`. Otherwise, just link to the issue. If there is no issue, whatever details would have been in the issue should be in the PR description.
- **Guide PR reviewers:** Let them know about areas of concern, under-tested areas, or vague requirements that should be ironed out.
### Reviewing PRs
- **Verify Requirements are Met**: If the PR claims to fix or close an issue, check that all the requirements in the issue are actually met. Otherwise the issue may be in a good place to merge, but just shouldn’t close the issue.
- **Focus on Tests**: The tests are the spec and therefore should be the focus of reviews. If tests are thorough and passing, the rest is an implementation detail (to an extent—don’t skip source code reviews) that can be fixed in a future optimization/cleanup PR. Make sure edge case behaviors are defined and handled.
- **Think like an Auditor:** What edge cases were ignored? How can the code break? When might it behave incorrectly and unexpectedly? What code should have been changed that isn’t in the diff? What implicit assumptions are made that might be invalid?
- **Ensure Comment Significance is Clear**: Indicate which comments are nits/optionals that the PR author can resolve, compared to which you want to follow up on.
- Prefix non-blocking comments with `[nit]` or `[non-blocking]`.
- **Consider Reviewing in Your IDE**: For example, GitHub has [this VSCode extension](https://marketplace.visualstudio.com/items?itemName=GitHub.vscode-pull-request-github) to review PRs. This provides more code context and enables review to benefit from your standard lints and IDE features, whereas GitHub’s diff shows none of that.
### Merging PRs
- **Resolve all Comments**: Comments can be resolved by (1) the PR author for nits/optionals, (2) the author or reviewer after discussions, or (3) extracting the comment into an issue to address in a future PR. For (3), ensure the new issue links to the specific comment thread. This is currently enforced by GitHub's merge requirements.
- **Other Standard Merge Requirements**: The PR must be approved by the appropriate reviewers, CI must passing, and other standard merge requirements apply.
......@@ -312,6 +312,10 @@ func (bs *BatcherService) Stop(ctx context.Context) error {
result = errors.Join(result, fmt.Errorf("failed to close balance metricer: %w", err))
}
}
if bs.TxManager != nil {
bs.TxManager.Close()
}
if bs.metricsSrv != nil {
if err := bs.metricsSrv.Stop(ctx); err != nil {
result = errors.Join(result, fmt.Errorf("failed to stop metrics server: %w", err))
......
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
......@@ -131,6 +131,7 @@ func BuildOptimism(immutable ImmutableConfig) (DeploymentResults, error) {
{
Name: "L2ERC721Bridge",
Args: []interface{}{
immutable["L2ERC721Bridge"]["messenger"],
immutable["L2ERC721Bridge"]["otherBridge"],
},
},
......@@ -216,7 +217,7 @@ func l2Deployer(backend *backends.SimulatedBackend, opts *bind.TransactOpts, dep
}
_, tx, _, err = bindings.DeployL1FeeVault(opts, backend, recipient, minimumWithdrawalAmount, withdrawalNetwork)
case "OptimismMintableERC20Factory":
_, tx, _, err = bindings.DeployOptimismMintableERC20Factory(opts, backend)
_, tx, _, err = bindings.DeployOptimismMintableERC20Factory(opts, backend, predeploys.L2StandardBridgeAddr)
case "DeployerWhitelist":
_, tx, _, err = bindings.DeployDeployerWhitelist(opts, backend)
case "LegacyMessagePasser":
......@@ -224,11 +225,15 @@ func l2Deployer(backend *backends.SimulatedBackend, opts *bind.TransactOpts, dep
case "L1BlockNumber":
_, tx, _, err = bindings.DeployL1BlockNumber(opts, backend)
case "L2ERC721Bridge":
otherBridge, ok := deployment.Args[0].(common.Address)
messenger, ok := deployment.Args[0].(common.Address)
if !ok {
return nil, fmt.Errorf("invalid type for messenger")
}
otherBridge, ok := deployment.Args[1].(common.Address)
if !ok {
return nil, fmt.Errorf("invalid type for otherBridge")
}
_, tx, _, err = bindings.DeployL2ERC721Bridge(opts, backend, otherBridge)
_, tx, _, err = bindings.DeployL2ERC721Bridge(opts, backend, messenger, otherBridge)
case "OptimismMintableERC721Factory":
bridge, ok := deployment.Args[0].(common.Address)
if !ok {
......
This diff is collapsed.
......@@ -2,22 +2,19 @@ package op_challenger
import (
"context"
"fmt"
"github.com/ethereum/go-ethereum/log"
"github.com/ethereum-optimism/optimism/op-challenger/config"
"github.com/ethereum-optimism/optimism/op-challenger/game"
"github.com/ethereum/go-ethereum/log"
"github.com/ethereum-optimism/optimism/op-service/cliapp"
)
// Main is the programmatic entry-point for running op-challenger
func Main(ctx context.Context, logger log.Logger, cfg *config.Config) error {
// Main is the programmatic entry-point for running op-challenger with a given configuration.
func Main(ctx context.Context, logger log.Logger, cfg *config.Config) (cliapp.Lifecycle, error) {
if err := cfg.Check(); err != nil {
return err
return nil, err
}
service, err := game.NewService(ctx, logger, cfg)
if err != nil {
return fmt.Errorf("failed to create the fault service: %w", err)
}
return service.MonitorGame(ctx)
srv, err := game.NewService(ctx, logger, cfg)
return srv, err
}
......@@ -12,6 +12,7 @@ import (
func TestMainShouldReturnErrorWhenConfigInvalid(t *testing.T) {
cfg := &config.Config{}
err := Main(context.Background(), testlog.Logger(t, log.LvlInfo), cfg)
app, err := Main(context.Background(), testlog.Logger(t, log.LvlInfo), cfg)
require.ErrorIs(t, err, cfg.Check())
require.Nil(t, app)
}
This diff is collapsed.
This diff is collapsed.
......@@ -232,6 +232,9 @@ func (m *mockTxManager) From() common.Address {
return m.from
}
func (m *mockTxManager) Close() {
}
type mockContract struct {
calls int
callFails bool
......
This diff is collapsed.
......@@ -84,8 +84,9 @@ func TestMonitorGames(t *testing.T) {
cancel()
}()
err := monitor.MonitorGames(ctx)
require.NoError(t, err)
monitor.StartMonitoring()
<-ctx.Done()
monitor.StopMonitoring()
require.Len(t, sched.scheduled, 1)
require.Equal(t, []common.Address{addr1, addr2}, sched.scheduled[0])
})
......@@ -129,8 +130,9 @@ func TestMonitorGames(t *testing.T) {
cancel()
}()
err := monitor.MonitorGames(ctx)
require.NoError(t, err)
monitor.StartMonitoring()
<-ctx.Done()
monitor.StopMonitoring()
require.NotEmpty(t, sched.scheduled) // We might get more than one update scheduled.
require.Equal(t, []common.Address{addr1, addr2}, sched.scheduled[0])
})
......
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
......@@ -54,6 +54,8 @@ func (f fakeTxMgr) BlockNumber(_ context.Context) (uint64, error) {
func (f fakeTxMgr) Send(_ context.Context, _ txmgr.TxCandidate) (*types.Receipt, error) {
panic("unimplemented")
}
func (f fakeTxMgr) Close() {
}
func NewL2Proposer(t Testing, log log.Logger, cfg *ProposerCfg, l1 *ethclient.Client, rollupCl *sources.RollupClient) *L2Proposer {
proposerConfig := proposer.ProposerConfig{
......
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
......@@ -94,7 +94,7 @@ func (d *recordsBook[K, V]) dsKey(key K) ds.Key {
func (d *recordsBook[K, V]) deleteRecord(key K) error {
d.cache.Remove(key)
err := d.store.Delete(d.ctx, d.dsKey(key))
if errors.Is(err, ds.ErrNotFound) {
if err == nil || errors.Is(err, ds.ErrNotFound) {
return nil
}
return fmt.Errorf("failed to delete entry with key %v: %w", key, err)
......
......@@ -33,8 +33,11 @@ func setupL2OutputOracle() (common.Address, *bind.TransactOpts, *backends.Simula
backend,
big.NewInt(10),
big.NewInt(2),
big.NewInt(100),
)
big.NewInt(0),
big.NewInt(0),
from,
common.Address{0xdd},
big.NewInt(100))
if err != nil {
return common.Address{}, nil, nil, nil, err
}
......
......@@ -279,6 +279,11 @@ func (ps *ProposerService) Stop(ctx context.Context) error {
result = errors.Join(result, fmt.Errorf("failed to close balance metricer: %w", err))
}
}
if ps.TxManager != nil {
ps.TxManager.Close()
}
if ps.metricsSrv != nil {
if err := ps.metricsSrv.Stop(ctx); err != nil {
result = errors.Join(result, fmt.Errorf("failed to stop metrics server: %w", err))
......
This diff is collapsed.
......@@ -43,6 +43,11 @@ func (_m *TxManager) BlockNumber(ctx context.Context) (uint64, error) {
return r0, r1
}
// Close provides a mock function with given fields:
func (_m *TxManager) Close() {
_m.Called()
}
// From provides a mock function with given fields:
func (_m *TxManager) From() common.Address {
ret := _m.Called()
......
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
......@@ -63,6 +63,6 @@
"@nomiclabs/hardhat-waffle": "^2.0.6",
"hardhat": "^2.19.0",
"ts-node": "^10.9.1",
"tsx": "^4.1.1"
"tsx": "^4.2.0"
}
}
This diff is collapsed.
This diff is collapsed.
......@@ -86,26 +86,11 @@ To deploy the smart contracts on a local devnet, run `make devnet-up` in the mon
### Tools
#### Layout Locking
We use a system called "layout locking" as a safety mechanism to prevent certain contract variables from being moved to different storage slots accidentally.
To lock a contract variable, add it to the `layout-lock.json` file which has the following format:
```json
{
"MyContractName": {
"myVariableName": {
"slot": 1,
"offset": 0,
"length": 32
}
}
}
```
With the above config, the `validate-spacers` script will check that we have a contract called `MyContractName`, that the contract has a variable named `myVariableName`, and that the variable is in the correct position as defined in the lock file.
You should add things to the `layout-lock.json` file when you want those variables to **never** change.
Layout locking should be used in combination with diffing the `.storage-layout` file in CI.
#### Validate Spacing
In order to make sure that we don't accidentally overwrite storage slots, contract storage layouts are checked to make sure spacing is correct.
This uses the `.storage-layout` file to check contract spacing. Run `pnpm validate-spacers` to check the spacing of all contracts.
#### Gas Snapshots
......
This diff is collapsed.
This diff is collapsed.
......@@ -10,4 +10,5 @@ library Executables {
string internal constant forge = "forge";
string internal constant echo = "echo";
string internal constant sed = "sed";
string internal constant find = "find";
}
This diff is collapsed.
......@@ -13,25 +13,24 @@ import { Constants } from "src/libraries/Constants.sol";
/// for sending and receiving data on the L1 side. Users are encouraged to use this
/// interface instead of interacting with lower-level contracts directly.
contract L1CrossDomainMessenger is CrossDomainMessenger, ISemver {
/// @notice Address of the OptimismPortal. The public getter for this
/// is legacy and will be removed in the future. Use `portal()` instead.
/// @custom:network-specific
/// @notice Address of the OptimismPortal. This will be removed in the
/// future, use `portal` instead.
/// @custom:legacy
OptimismPortal public PORTAL;
OptimismPortal public immutable PORTAL;
/// @notice Semantic version.
/// @custom:semver 1.7.1
string public constant version = "1.7.1";
/// @custom:semver 1.8.0
string public constant version = "1.8.0";
/// @notice Constructs the L1CrossDomainMessenger contract.
constructor() CrossDomainMessenger(Predeploys.L2_CROSS_DOMAIN_MESSENGER) {
initialize({ _portal: OptimismPortal(payable(0)) });
/// @param _portal Address of the OptimismPortal contract on this network.
constructor(OptimismPortal _portal) CrossDomainMessenger(Predeploys.L2_CROSS_DOMAIN_MESSENGER) {
PORTAL = _portal;
initialize();
}
/// @notice Initializes the contract.
/// @param _portal Address of the OptimismPortal contract on this network.
function initialize(OptimismPortal _portal) public reinitializer(Constants.INITIALIZER) {
PORTAL = _portal;
function initialize() public initializer {
__CrossDomainMessenger_init();
}
......
......@@ -55,14 +55,7 @@ contract ProtocolVersions is OwnableUpgradeable, ISemver {
/// @param _owner Initial owner of the contract.
/// @param _required Required protocol version to operate on this chain.
/// @param _recommended Recommended protocol version to operate on thi chain.
function initialize(
address _owner,
ProtocolVersion _required,
ProtocolVersion _recommended
)
public
reinitializer(Constants.INITIALIZER)
{
function initialize(address _owner, ProtocolVersion _required, ProtocolVersion _recommended) public initializer {
__Ownable_init();
transferOwnership(_owner);
_setRequired(_required);
......
......@@ -25,7 +25,7 @@ contract L2CrossDomainMessenger is CrossDomainMessenger, ISemver {
}
/// @notice Initializer.
function initialize() public reinitializer(Constants.INITIALIZER) {
function initialize() public initializer {
__CrossDomainMessenger_init();
}
......
......@@ -20,19 +20,13 @@ import { Predeploys } from "src/libraries/Predeploys.sol";
/// wait for the one-week challenge period to elapse before their Optimism-native NFT
/// can be refunded on L2.
contract L2ERC721Bridge is ERC721Bridge, ISemver {
/// @custom:semver 1.4.0
string public constant version = "1.4.0";
/// @custom:semver 1.5.0
string public constant version = "1.5.0";
/// @notice Constructs the L2ERC721Bridge contract.
/// @param _messenger Address of the CrossDomainMessenger on this network.
/// @param _otherBridge Address of the ERC721 bridge on the other network.
constructor(address _otherBridge) ERC721Bridge(_otherBridge) {
initialize();
}
/// @notice Initializes the contract.
function initialize() public reinitializer(Constants.INITIALIZER) {
__ERC721Bridge_init({ _messenger: CrossDomainMessenger(Predeploys.L2_CROSS_DOMAIN_MESSENGER) });
}
constructor(address _messenger, address _otherBridge) ERC721Bridge(_messenger, _otherBridge) { }
/// @notice Completes an ERC721 bridge from the other domain and sends the ERC721 token to the
/// recipient on this domain.
......@@ -114,7 +108,7 @@ contract L2ERC721Bridge is ERC721Bridge, ISemver {
// Send message to L1 bridge
// slither-disable-next-line reentrancy-events
messenger.sendMessage(OTHER_BRIDGE, message, _minGasLimit);
MESSENGER.sendMessage(OTHER_BRIDGE, message, _minGasLimit);
// slither-disable-next-line reentrancy-events
emit ERC721BridgeInitiated(_localToken, remoteToken, _from, _to, _tokenId, _extraData);
......
......@@ -139,7 +139,7 @@ contract FaultDisputeGame_Test is FaultDisputeGame_Init {
/// For now, it is critical that the first proposed output root of an OP stack
/// chain is done so by an honest party.
function test_initialize_firstOutput_reverts() public {
uint256 submissionInterval = l2OutputOracle.submissionInterval();
uint256 submissionInterval = l2OutputOracle.SUBMISSION_INTERVAL();
vm.expectRevert(abi.encodeWithSignature("Panic(uint256)", 0x11));
factory.create(GAME_TYPE, ROOT_CLAIM, abi.encode(submissionInterval, block.number - 1));
}
......
......@@ -35,8 +35,6 @@ contract L1StandardBridge_Initialize_Test is Bridge_Initializer {
assertEq(address(l1StandardBridge.messenger()), address(l1CrossDomainMessenger));
assertEq(address(l1StandardBridge.OTHER_BRIDGE()), Predeploys.L2_STANDARD_BRIDGE);
assertEq(address(l2StandardBridge), Predeploys.L2_STANDARD_BRIDGE);
bytes32 slot0 = vm.load(address(l1StandardBridge), bytes32(uint256(0)));
assertEq(slot0, bytes32(uint256(Constants.INITIALIZER)));
}
}
......
......@@ -12,7 +12,7 @@ contract LibPosition_Test is Test {
/// will likely be much lower.
uint8 internal constant MAX_DEPTH = 63;
function boundIndexAtDepth(uint8 _depth, uint64 _indexAtDepth) internal view returns (uint64) {
function boundIndexAtDepth(uint8 _depth, uint64 _indexAtDepth) internal pure returns (uint64) {
// Index at depth bound: [0, 2 ** _depth-1]
if (_depth > 0) {
return uint64(bound(_indexAtDepth, 0, 2 ** (_depth - 1)));
......
......@@ -11,7 +11,6 @@ import { EIP1967Helper } from "test/mocks/EIP1967Helper.sol";
// Libraries
import { Types } from "src/libraries/Types.sol";
import { Hashing } from "src/libraries/Hashing.sol";
import { Constants } from "src/libraries/Constants.sol";
// Target contract dependencies
import { Proxy } from "src/universal/Proxy.sol";
......@@ -900,24 +899,14 @@ contract OptimismPortalUpgradeable_Test is CommonTest {
/// @dev Tests that the proxy cannot be initialized twice.
function test_initialize_cannotInitProxy_reverts() external {
vm.expectRevert("Initializable: contract is already initialized");
optimismPortal.initialize({
_l2Oracle: L2OutputOracle(address(0)),
_systemConfig: SystemConfig(address(0)),
_guardian: address(0),
_paused: false
});
optimismPortal.initialize({ _paused: false });
}
/// @dev Tests that the implementation cannot be initialized twice.
function test_initialize_cannotInitImpl_reverts() external {
address opImpl = mustGetAddress("OptimismPortal");
vm.expectRevert("Initializable: contract is already initialized");
OptimismPortal(payable(opImpl)).initialize({
_l2Oracle: L2OutputOracle(address(0)),
_systemConfig: SystemConfig(address(0)),
_guardian: address(0),
_paused: false
});
OptimismPortal(payable(opImpl)).initialize({ _paused: false });
}
/// @dev Tests that the proxy can be upgraded.
......@@ -932,7 +921,7 @@ contract OptimismPortalUpgradeable_Test is CommonTest {
// The value passed to the initialize must be larger than the last value
// that initialize was called with.
Proxy(payable(address(optimismPortal))).upgradeToAndCall(
address(nextImpl), abi.encodeWithSelector(NextImpl.initialize.selector, Constants.INITIALIZER + 1)
address(nextImpl), abi.encodeWithSelector(NextImpl.initialize.selector, 2)
);
assertEq(Proxy(payable(address(optimismPortal))).implementation(), address(nextImpl));
......
......@@ -10,12 +10,7 @@ import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
/// @notice Simple wrapper around the StandardBridge contract that exposes
/// internal functions so they can be more easily tested directly.
contract StandardBridgeTester is StandardBridge {
constructor(
address payable _messenger,
address payable _otherBridge
)
StandardBridge(StandardBridge(_otherBridge))
{ }
constructor(address payable _messenger, address payable _otherBridge) StandardBridge(_messenger, _otherBridge) { }
function isOptimismMintableERC20(address _token) external view returns (bool) {
return _isOptimismMintableERC20(_token);
......
......@@ -49,7 +49,7 @@
"node-fetch": "^2.6.7"
},
"devDependencies": {
"@types/node": "^20.9.0",
"@types/node": "^20.9.3",
"mocha": "^10.2.0"
}
}
......@@ -43,8 +43,8 @@
"@nomiclabs/hardhat-waffle": "^2.0.1",
"@types/chai": "^4.3.10",
"@types/chai-as-promised": "^7.1.8",
"@types/mocha": "^10.0.4",
"@types/node": "^20.9.0",
"@types/mocha": "^10.0.6",
"@types/node": "^20.9.3",
"chai-as-promised": "^7.1.1",
"ethereum-waffle": "^4.0.10",
"ethers": "^5.7.2",
......
This diff is collapsed.
This diff is collapsed.
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment