Skip to content

Comments

contracts-bedrock: delete dead code#10359

Merged
tynes merged 15 commits intodevelopfrom
cleanup/deploy
May 2, 2024
Merged

contracts-bedrock: delete dead code#10359
tynes merged 15 commits intodevelopfrom
cleanup/deploy

Conversation

@tynes
Copy link
Contributor

@tynes tynes commented May 1, 2024

Description

Deletes dead code and simplifies the deploy script
infrastructure to make it less error prone and more
explicit.

  • contracts-bedrock: simplify L1 state dump
  • contracts-bedrock: explicit deploy config
  • contracts-bedrock: fix possible error
  • config: cleanup
  • deploy-config: fixup
  • contracts-bedrock: delete name function
  • contracts: delete dead code

tynes added 7 commits April 30, 2024 20:07
Removes the need to specify the L1 chainid via CLI.
Follows similar patterns to L2 genesis generation that
were done recently.

Follow up PRs can potentially use the same sort of deployer pattern
as in #10343
for when doing the L1 genesis dump to remove the need to specify the
private key.
Remove the concept of implicit deploy config for explicit deploy config.
This removes confusing implicit behavior as well as makes it much
more straight forward for deploying multiple superchain targets
to the same L1. This is a breaking change but the error message
makes it very obvious and the docs are being updated in a way
that should include this information.
The `name()` function existed due to legacy
purposes when dealing with hardhat artifacts
and no longer is necessary. This commit removes
it in favor of a simpler approach of just using
the chainid instead of the name. The files that
are written are not committed into the repo.
@tynes tynes requested review from a team as code owners May 1, 2024 02:32
@tynes tynes requested review from geoknee and refcell May 1, 2024 02:32
@coderabbitai
Copy link
Contributor

coderabbitai bot commented May 1, 2024

Walkthrough

Walkthrough

The recent updates focus on enhancing script execution and configuration management system-wide. Changes include adding new environment variables for deployment, updating logging and file handling strategies, and simplifying deployment interfaces by removing specific artifacts in scripts.

Changes

File Path Change Summary
bedrock-devnet/devnet/__init__.py Modified devnet_l1_allocs and devnet_l2_allocs functions with new environment variables and calls.
packages/contracts-bedrock/.gitignore Added deployments/31337-deploy.json to the ignore list.
packages/contracts-bedrock/lib/forge-std Updated subproject commit hash.
packages/contracts-bedrock/scripts/Config.sol Imported VmSafe, updated deployConfigPath, and removed old deployment artifacts functions.
.../scripts/Deploy.s.sol
.../scripts/DeployPeriphery.s.sol
.../scripts/Deployer.sol
.../scripts/L2Genesis.s.sol
Removed name() function, updated deployment scripts, logging, and context handling.

Recent Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between 9cd4581 and 3d54d90.
Files selected for processing (1)
  • packages/contracts-bedrock/scripts/Config.sol (5 hunks)
Files skipped from review as they are similar to previous changes (1)
  • packages/contracts-bedrock/scripts/Config.sol

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@tynes tynes marked this pull request as draft May 1, 2024 02:33
@tynes
Copy link
Contributor Author

tynes commented May 1, 2024

This PR exists to improve the devex of using the deploy script so that its always expected to provide the deploy config path as an env var when running the deploy script

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Out of diff range and nitpick comments (4)
bedrock-devnet/devnet/__init__.py (4)

Line range hint 7-7: Consider removing the unused import calendar.

- import calendar

Line range hint 12-12: The gzip module is imported but not used in this file. It should be removed unless planned for future use.

- import gzip

Line range hint 18-18: The import devnet.log_setup is unused. It should be removed to clean up the code.

- import devnet.log_setup

Line range hint 223-223: Replace direct boolean comparisons with more idiomatic Python expressions.

- if os.path.exists(l2_allocs_path) == False or DEVNET_FPAC == True:
- if not os.path.exists(l2_allocs_path) or DEVNET_FPAC:

@codecov
Copy link

codecov bot commented May 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 29.22%. Comparing base (794a333) to head (3d54d90).
Report is 7 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff              @@
##           develop   #10359       +/-   ##
============================================
- Coverage    42.33%   29.22%   -13.11%     
============================================
  Files           73       31       -42     
  Lines         4845     2898     -1947     
  Branches       766      614      -152     
============================================
- Hits          2051      847     -1204     
+ Misses        2684     1976      -708     
+ Partials       110       75       -35     
Flag Coverage Δ
cannon-go-tests ?
chain-mon-tests 27.14% <ø> (ø)
common-ts-tests ?
contracts-ts-tests 12.25% <ø> (ø)
core-utils-tests ?
sdk-tests 40.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 42 files with indirect coverage changes

@tynes tynes marked this pull request as ready for review May 1, 2024 23:40
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Out of diff range and nitpick comments (5)
bedrock-devnet/devnet/__init__.py (5)

Line range hint 7-7: Remove unused import calendar.

- import calendar

Line range hint 12-12: Remove unused import gzip.

- import gzip

Line range hint 18-18: Remove unused import devnet.log_setup.

- import devnet.log_setup

Line range hint 222-222: Refactor the condition checks to use more Pythonic expressions.

- if os.path.exists(l2_allocs_path) == False or DEVNET_FPAC == True:
+ if not os.path.exists(l2_allocs_path) or DEVNET_FPAC:

Line range hint 311-311: Remove the unused local variable e.

- except Exception as e:
+ except Exception:

Co-authored-by: Matt Solomon <matt@mattsolomon.dev>
@tynes tynes enabled auto-merge May 2, 2024 02:10
@tynes tynes added this pull request to the merge queue May 2, 2024
Merged via the queue into develop with commit 40421e8 May 2, 2024
@tynes tynes deleted the cleanup/deploy branch May 2, 2024 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants