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.

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

Update op-geth dependency to upstream geth v1.13.8 and migrate to slog #8917

Merged
merged 34 commits into from
Feb 6, 2024

Conversation

sebastianst
Copy link
Member

@sebastianst sebastianst commented Jan 9, 2024

Description

Updated op-geth dependency to ethereum-optimism/op-geth#215 based on upstream geth v1.13.8

Also migrates logging infra to slog, after upstream migrated to slog in ethereum/go-ethereum#28187.

I opened an upstream PR to add the Handler getter to the Logger interface (slog's Logger interface itself has this getter). It's already in the op-geth version that this PR is based on.

Metadata

@sebastianst sebastianst requested a review from ajsutton January 9, 2024 21:39
Copy link

codecov bot commented Jan 9, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (fdcd8ff) 16.57% compared to head (6c61cc8) 24.62%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #8917      +/-   ##
===========================================
+ Coverage    16.57%   24.62%   +8.04%     
===========================================
  Files          119      104      -15     
  Lines         5098     3732    -1366     
  Branches      1130      536     -594     
===========================================
+ Hits           845      919      +74     
+ Misses        4178     2767    -1411     
+ Partials        75       46      -29     
Flag Coverage Δ
cannon-go-tests 62.77% <0.00%> (?)
chain-mon-tests ?
contracts-bedrock-tests 0.65% <ø> (ø)
contracts-ts-tests ?
sdk-next-tests ?
sdk-tests ?

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

Files Coverage Δ
cannon/cmd/run.go 0.00% <0.00%> (ø)
cannon/cmd/log.go 0.00% <0.00%> (ø)

... and 45 files with indirect coverage changes

@ajsutton
Copy link
Contributor

Looks like the logging changes bit us.

Copy link
Contributor

semgrep-app bot commented Jan 11, 2024

Semgrep found 10 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 39 sol-style-notice-over-dev-natspec findings:

Prefer @notice over @dev in natspec comments

Ignore this finding from sol-style-notice-over-dev-natspec.

@sebastianst sebastianst changed the title go: Update op-geth dependency to upstream geth v1.13.8 go: Update op-geth dependency to upstream geth v1.13.8 and migrate to slog Jan 15, 2024
@sebastianst sebastianst changed the title go: Update op-geth dependency to upstream geth v1.13.8 and migrate to slog Update op-geth dependency to upstream geth v1.13.8 and migrate to slog Jan 15, 2024
@sebastianst sebastianst force-pushed the seb/op-geth-1.101308.0 branch 2 times, most recently from 6c1072c to 83caf39 Compare January 15, 2024 21:51
Copy link
Member Author

@sebastianst sebastianst left a comment

Choose a reason for hiding this comment

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

Left a couple of pointers for reviewers.

op-service/log/cli.go Outdated Show resolved Hide resolved
op-wheel/cheat/cheat.go Outdated Show resolved Hide resolved
op-program/client/l2/engine.go Outdated Show resolved Hide resolved
@sebastianst sebastianst force-pushed the seb/op-geth-1.101308.0 branch from e865bd9 to 23453cb Compare January 16, 2024 15:54
@sebastianst sebastianst force-pushed the seb/op-geth-1.101308.0 branch from 520cc52 to e7fd916 Compare February 1, 2024 21:54
Copy link
Contributor

semgrep-app bot commented Feb 1, 2024

Semgrep found 7 ban-common-hex2bytes findings:

  • op-service/testutils/random.go: L109
  • op-challenger/game/fault/trace/alphabet/prestate.go: L13
  • op-chain-ops/deployer/testdata.go: L20, L18, L16, L14, L12

Found banned use of common.Hex2Bytes. Use common.FromHex instead.

Ignore this finding from ban-common-hex2bytes.

@sebastianst sebastianst requested a review from ajsutton February 4, 2024 19:30
Copy link
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

Some small comments, but nothing big.

op-chain-ops/cmd/check-canyon/main.go Show resolved Hide resolved
op-chain-ops/cmd/check-deploy-config/main.go Show resolved Hide resolved
op-program/host/cmd/main_test.go Show resolved Hide resolved
op-service/log/cli.go Show resolved Hide resolved
op-e2e/config/init.go Outdated Show resolved Hide resolved
Copy link
Contributor

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

Changes LGTM. A few imports seem to need lint fixes, specifically for "golang.org/x/exp/slog" in a few places

@sebastianst sebastianst enabled auto-merge February 6, 2024 19:16
@sebastianst sebastianst added this pull request to the merge queue Feb 6, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 6, 2024
@sebastianst sebastianst added this pull request to the merge queue Feb 6, 2024
Merged via the queue into develop with commit 15e868a Feb 6, 2024
66 checks passed
@sebastianst sebastianst deleted the seb/op-geth-1.101308.0 branch February 6, 2024 21:17
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.

6 participants