Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Standard L2 Genesis #12057

Closed
wants to merge 93 commits into from
Closed

Standard L2 Genesis #12057

wants to merge 93 commits into from

Conversation

tynes
Copy link
Contributor

@tynes tynes commented Sep 23, 2024 •
edited by maurelian
Loading

Description

Moves network specific values into L1Block.sol.

Fixes: #12297, #12349, #12298, #12299, #12300, #12301, #12303, #12304, #12305, #12655

Open items:

  • Debug the failing IL1BlockInterop interface check. It does not behave the same on my machine (I have failures in both IL1Block and IL1BlockInterop). It's related to the constructor afaict, but I can't make it work.
  • Fix the one failing forge test, test_upgrade_correctEvent_succeeds
  • Fix the unused imports check
  • Add feeAdmin to op-deployer
  • Consider if the new SystemConfig.Roles struct should also contain the unsafeBlockSigner
    • There is also a Roles struct in OPCM. This is a bit confusing, can we disambiguate it?
  • Fix the semgrep findings

Cut from scope:

semgrep-app[bot]
semgrep-app bot reviewed Sep 23, 2024
View reviewed changes
packages/contracts-bedrock/src/libraries/StaticConfig.sol
}

/// @notice
function encodeSetRemoteChainId(uint256 _chainId) internal pure returns (bytes memory) {
Copy link
Contributor

@semgrep-app semgrep-app bot Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Named return arguments to functions must be appended with an underscore (_)

Ignore this finding from sol-style-return-arg-fmt.

semgrep-app[bot]
semgrep-app bot reviewed Sep 23, 2024
View reviewed changes
packages/contracts-bedrock/src/libraries/StaticConfig.sol
}

/// @notice
function encodeSetAddress(address _address) internal pure returns (bytes memory) {
Copy link
Contributor

@semgrep-app semgrep-app bot Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Named return arguments to functions must be appended with an underscore (_)

Ignore this finding from sol-style-return-arg-fmt.

semgrep-app[bot]
semgrep-app bot reviewed Sep 23, 2024
View reviewed changes
packages/contracts-bedrock/src/libraries/StaticConfig.sol Outdated
@@ -57,4 +78,34 @@ library StaticConfig {
function decodeRemoveDependency(bytes memory _data) internal pure returns (uint256) {
return abi.decode(_data, (uint256));
}

/// @notice
function encodeSetFeeVaultConfig(address _recipient, uint256 _min, FeeVault.WithdrawalNetwork _network) internal pure returns (bytes memory) {
Copy link
Contributor

@semgrep-app semgrep-app bot Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Named return arguments to functions must be appended with an underscore (_)

Ignore this finding from sol-style-return-arg-fmt.

@semgrep-app semgrep-app
Copy link
Contributor

semgrep-app bot commented Sep 23, 2024

Semgrep found 1 sol-style-return-arg-fmt finding:

  • packages/contracts-bedrock/src/L1/OPStackManager.sol

Named return arguments to functions must be appended with an underscore (_)

Ignore this finding from sol-style-return-arg-fmt.

@semgrep-app semgrep-app
Copy link
Contributor

semgrep-app bot commented Sep 28, 2024

Semgrep found 2 golang_fmt_errorf_no_params findings:

No fmt.Errorf invocations without fmt arguments allowed

Ignore this finding from golang_fmt_errorf_no_params.

Semgrep found 6 sol-style-return-arg-fmt findings:

Named return arguments to functions must be appended with an underscore (_)

Ignore this finding from sol-style-return-arg-fmt.

Semgrep found 2 sol-style-require-reason findings:

require() must include a reason string

Ignore this finding from sol-style-require-reason.

Semgrep found 12 sol-safety-deployutils-args findings:

_args parameter should be wrapped with DeployUtils.encodeConstructor

Ignore this finding from sol-safety-deployutils-args.

tynes
tynes commented Sep 29, 2024
View reviewed changes
packages/contracts-bedrock/src/L2/L2ProxyAdmin.sol
@@ -0,0 +1,9 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.15;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider refactoring the ProxyAdmin to remove the L1 specific features and make a L1ProxyAdmin

tynes
tynes commented Sep 29, 2024
View reviewed changes
packages/contracts-bedrock/scripts/L2Genesis.s.sol
// update the proxy to not be uninitialized (although not standard initialize pattern)
vm.store(impl, _ownerSlot, bytes32(uint256(uint160(cfg.proxyAdminOwner()))));
vm.store(impl, bytes32(0), bytes32(uint256(0xdead)));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

todo: double check if this is necessary, this adds complexity to the genesis definition

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of why it would be necessary... is there something in storage at that slot on OP Mainnet?

@semgrep-app semgrep-app
Copy link
Contributor

semgrep-app bot commented Sep 30, 2024

Semgrep found 6 sol-style-require-reason findings:

require() must include a reason string

Ignore this finding from sol-style-require-reason.

@tynes tynes mentioned this pull request Oct 3, 2024
Closed
@tynes tynes changed the title wip: holocene contracts wip: isthmus contracts Oct 6, 2024
@tynes tynes changed the title wip: isthmus contracts wip: Standard L2 Genesis Oct 6, 2024
@tynes tynes mentioned this pull request Oct 7, 2024
Open
16 tasks
maurelian
maurelian reviewed Oct 30, 2024
View reviewed changes
op-deployer/pkg/deployer/integration_test/apply_test.go
var defaultGovOwner common.Hash
defaultGovOwner.SetBytes(common.HexToAddress("0xDeaDDEaDDeAdDeAdDEAdDEaddeAddEAdDEAdDEad").Bytes())
checkStorageSlot(t, alloc, predeploys.GovernanceTokenAddr, common.Hash{31: 0x0a}, defaultGovOwner)
Copy link
Contributor

Choose a reason for hiding this comment

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

Restore this check.

This was referenced Oct 30, 2024
Closed
Merged
@ethereum-optimism ethereum-optimism deleted a comment from semgrep-app bot Oct 31, 2024
@tynes
Copy link
Contributor Author

tynes commented Nov 18, 2024

Going to close this PR due to it being replaced by another

@tynes tynes closed this Nov 18, 2024
@tynes tynes deleted the feat/holocene-contracts branch November 18, 2024 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@semgrep-app semgrep-app[bot] semgrep-app[bot] left review comments

@maurelian maurelian maurelian left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Standard L2 Genesis: Implement FeeVault Modifications
2 participants
@tynes @maurelian