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

op-supervisor: access-list support, checksum handling #14784

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

op-supervisor: access-list support, checksum handling #14784

wants to merge 5 commits into from

Conversation

protolambda
Copy link
Contributor

@protolambda protolambda commented Mar 11, 2025 •
edited
Loading

Description

Changes for check-access-list RPC support in op-supervisor.
Implements spec of ethereum-optimism/specs#612

Tests

  • Existing e2e test updated to run access-list check for logs
  • Unit-tested all the new types / checksum utils / timestamp checking

Additional context

Metadata

Fix #14696

@protolambda protolambda added the H-interop Hardfork: change planned for interop upgrade label Mar 11, 2025
@codecov Codecov
Copy link

codecov bot commented Mar 11, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 68.38235% with 86 lines in your changes missing coverage. Please review.

Project coverage is 42.36%. Comparing base (7887930) to head (104a5a1).
Report is 15 commits behind head on develop.

Files with missing lines Patch % Lines
op-supervisor/supervisor/backend/backend.go 0.00% 50 Missing ⚠️
op-supervisor/supervisor/types/types.go 90.18% 12 Missing and 4 partials ⚠️
op-supervisor/supervisor/backend/db/query.go 0.00% 13 Missing ⚠️
op-supervisor/supervisor/backend/db/logs/db.go 86.36% 2 Missing and 1 partial ⚠️
op-service/sources/supervisor_client.go 0.00% 2 Missing ⚠️
op-supervisor/supervisor/backend/db/open.go 0.00% 1 Missing ⚠️
op-supervisor/supervisor/backend/mock.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #14784      +/-   ##
===========================================
- Coverage    46.67%   42.36%   -4.31%     
===========================================
  Files         1049      875     -174     
  Lines        90839    80773   -10066     
===========================================
- Hits         42399    34222    -8177     
+ Misses       45306    43590    -1716     
+ Partials      3134     2961     -173
Flag Coverage Δ
cannon-go-tests-32 ?
cannon-go-tests-64 ?
contracts-bedrock-tests ?

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

Files with missing lines Coverage Δ
op-program/client/interop/consolidate.go 72.28% <100.00%> (+0.63%) ⬆️
...-supervisor/supervisor/backend/cross/hazard_set.go 97.45% <100.00%> (+0.02%) ⬆️
op-supervisor/supervisor/frontend/frontend.go 76.92% <100.00%> (+3.58%) ⬆️
op-supervisor/supervisor/backend/db/open.go 0.00% <0.00%> (ø)
op-supervisor/supervisor/backend/mock.go 0.00% <0.00%> (ø)
op-service/sources/supervisor_client.go 19.73% <0.00%> (+3.43%) ⬆️
op-supervisor/supervisor/backend/db/logs/db.go 69.68% <86.36%> (+1.41%) ⬆️
op-supervisor/supervisor/backend/db/query.go 0.00% <0.00%> (ø)
op-supervisor/supervisor/types/types.go 69.28% <90.18%> (+69.28%) ⬆️
op-supervisor/supervisor/backend/backend.go 31.99% <0.00%> (+1.17%) ⬆️

... and 181 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

axelKingsley
axelKingsley reviewed Mar 11, 2025
View reviewed changes
Copy link
Contributor

@axelKingsley axelKingsley left a comment

Choose a reason for hiding this comment

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

Annotation of the draft as of March 11

  • The client drops Check Message APIs for CheckAccessList APIs.
  • Access is a fundamental type which is structurally equal to an Identifier, but comes from a different place and can be parsed out of a common.Hash
  • Messages can create Checksums now, likely to support Accesses
  • Safest is gone. I'm glad for that, the function was always a little weak.
  • CheckAccessList and checkAccess are basically the same as CheckMessage(s), except they use parse Access from checksums.

I left some comments, but things make sense generally.

@protolambda protolambda marked this pull request as ready for review March 11, 2025 21:20
@protolambda protolambda requested review from a team as code owners March 11, 2025 21:20
@protolambda protolambda requested review from mbaxter, axelKingsley and Inphi and removed request for mbaxter March 11, 2025 21:20
@protolambda
Copy link
Contributor Author

Ready for review. We might still want to wait with this PR until the spec PR lands though.

axelKingsley
axelKingsley approved these changes Mar 11, 2025
View reviewed changes
Copy link
Contributor

@axelKingsley axelKingsley left a comment

Choose a reason for hiding this comment

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

Approving but not auto-merging, based on the comment above, seems fine to wait for the spec to land first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@axelKingsley axelKingsley axelKingsley approved these changes

@Inphi Inphi Awaiting requested review from Inphi

Assignees
No one assigned
Labels
H-interop Hardfork: change planned for interop upgrade
Projects
Interoperability
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

op-supervisor: checkAccessList RPC
2 participants
@protolambda @axelKingsley