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

op-service: create a common admin API RPC and a common RPC request metrics #7482

Conversation

seungjulee
Copy link
Contributor

@seungjulee seungjulee commented Sep 29, 2023

Description

  • 1. Separate out RPC server/client response/request metrics into op-service/metrics from op-node/metrics (perhaps this needs to be a separate PR. Please let me know if I need to do so) 56a6979
  • 2. Extract the AdminAPI logic from op-node to op-service. d3c3a23 Use the consolidated AdminAPI for op-node and op-batcher 28aa528
  • 3. Remove custom AdminAPI config from op-batcher, and move it to CLIFlags on op-service/server. 07647f5

This PR separates out AdminAPI functionality from op-node into op-service so that other packages like op-batcher that runs its own RPC server out of op-service/rpc can adopt the newly created AdminAPI without copying the logic.

This work is a part of op-service refactor effort. Specifically, this work was spun out from the below comment.

          Probably out of scope for this PR, but this would be nice to also have in op-service so the batcher/proposer/challenger would automatically inherit this capability.

Originally posted by @trianglesphere in #6895 (comment)

Tests
This is a simple refactor that does not change the existing logic. The change does not fail the existing tests.

I would love to get your opinion on where and how I should test the changes.

Additional context

cc @sebastianst @tynes @ajsutton perhaps this PR can be used as a supplementary way to evaluate my skills for the interview process.

Metadata

@seungjulee seungjulee requested a review from a team as a code owner September 29, 2023 14:28
@seungjulee seungjulee requested a review from refcell September 29, 2023 14:28
@mergify mergify bot added A-op-batcher Area: op-batcher A-op-node Area: op-node A-op-service Area: op-service labels Sep 29, 2023
@trianglesphere trianglesphere self-requested a review September 29, 2023 15:51
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.

If you could split it out into a PR for just the admin RPC & one just for the RPC metrics, that would make it easier toreview.

op-batcher/batcher/config.go Show resolved Hide resolved
op-batcher/batcher/batch_submitter.go Outdated Show resolved Hide resolved
@seungjulee
Copy link
Contributor Author

seungjulee commented Sep 30, 2023

If you could split it out into a PR for just the admin RPC & one just for the RPC metrics, that would make it easier toreview.

I looked more on this, but this will create a two PRs that depends on another. Perhaps I'm misunderstanding, but the AdminAPI PR will inevitably touch on RPCMetrics PR, and will need to coordinate merging the two. If it's not too bothersome, I prefer to handle the two changes together in this PR.

But I can still split it out into two PRs. Let me know what I should do.

@seungjulee seungjulee changed the title op-service: create a common admin API RPC and create a common RPC request metrics op-service: create a common admin API RPC and a common RPC request metrics Sep 30, 2023
@trianglesphere
Copy link
Contributor

If you could split it out into a PR for just the admin RPC & one just for the RPC metrics, that would make it easier toreview.

I looked more on this, but this will create a two PRs that depends on another. Perhaps I'm misunderstanding, but the AdminAPI PR will inevitably touch on RPCMetrics PR, and will need to coordinate merging the two. If it's not too bothersome, I prefer to handle the two changes together in this PR.

But I can still split it out into two PRs. Let me know what I should do.

This is fine to keep at one PR. Yes, there is a dependency with changes like this, but usually they can be stacked.

…imental flag description for enabling Admin API
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.

TY for this fix

@trianglesphere
Copy link
Contributor

@seungjulee could you merge in the latest from develop for CI?

@trianglesphere trianglesphere added this pull request to the merge queue Oct 4, 2023
Merged via the queue into ethereum-optimism:develop with commit a762fa7 Oct 4, 2023
@seungjulee seungjulee deleted the issue-7417-standard-admin-rpc branch October 6, 2023 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-op-batcher Area: op-batcher A-op-node Area: op-node A-op-service Area: op-service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

op-service: standard admin RPC building block
2 participants