Skip to content

op-node: implement RPC poll interval and rate-limit options and provide L1 RPC CLI options#5230

Merged
mergify[bot] merged 3 commits intodevelopfrom
fix-l1-head-polling-time
Mar 24, 2023
Merged

op-node: implement RPC poll interval and rate-limit options and provide L1 RPC CLI options#5230
mergify[bot] merged 3 commits intodevelopfrom
fix-l1-head-polling-time

Conversation

@protolambda
Copy link
Contributor

Description

  • Refactor RPC endpoint config setup functions to return the sources client config. This way we don't have to pass around an increasing number of options, and we can make the existing options configurable via flags/env, like the max-batch-size for L1 now.
  • Implement basic rate-limiter RPC wrapper, so users can optionally configure a sanity-limit for RPC usage.
  • Fix the default HTTP-subscription-fallback poll interval: it defaults to 12 seconds now, and is configurable to be set lower.
  • Refactor client.NewRPC to extend with options more easily.

Tests

Updated op-e2e to use the new RPC option pattern and specify a poll-time based on L1 block time.

Not sure how best to test the rate-limit functionality, but it's off by default and just a single call to x-std-lib rate limiter.

TODOs

Fix CLI-3694

@protolambda protolambda requested a review from a team as a code owner March 22, 2023 18:36
@protolambda protolambda requested a review from tynes March 22, 2023 18:36
@changeset-bot
Copy link

changeset-bot bot commented Mar 22, 2023

⚠️ No Changeset found

Latest commit: ce7165a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@netlify
Copy link

netlify bot commented Mar 22, 2023

Deploy Preview for opstack-docs ready!

Name Link
🔨 Latest commit ce7165a
🔍 Latest deploy log https://app.netlify.com/sites/opstack-docs/deploys/641d9e1268efd40008bf5848
😎 Deploy Preview https://deploy-preview-5230--opstack-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@mergify
Copy link
Contributor

mergify bot commented Mar 22, 2023

Hey @protolambda! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Mar 22, 2023
@protolambda protolambda force-pushed the fix-l1-head-polling-time branch from 2765d7b to d8764ed Compare March 22, 2023 20:59
@mergify mergify bot removed the conflict label Mar 22, 2023
@protolambda
Copy link
Contributor Author

Rebased to resolve merge conflicts; the sync endpoint config extended in another PR, and this touched the rpc config endpoint things too.

@trianglesphere trianglesphere self-requested a review March 22, 2023 21:56
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.

Really nice changes. I'm not sure why the test is flaky.

@codecov
Copy link

codecov bot commented Mar 24, 2023

Codecov Report

Merging #5230 (ce7165a) into develop (e59446e) will decrease coverage by 3.83%.
The diff coverage is 12.71%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5230      +/-   ##
===========================================
- Coverage    40.36%   36.54%   -3.83%     
===========================================
  Files          372      219     -153     
  Lines        23680    19269    -4411     
  Branches       832        0     -832     
===========================================
- Hits          9558     7041    -2517     
+ Misses       13394    11529    -1865     
+ Partials       728      699      -29     
Flag Coverage Δ
bedrock-go-tests 36.54% <12.71%> (-0.08%) ⬇️
common-ts-tests ?
contracts-bedrock-tests ?
contracts-tests ?
core-utils-tests ?
dtl-tests ?
fault-detector-tests ?
sdk-tests ?

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

Impacted Files Coverage Δ
op-node/client/rate_limited.go 0.00% <0.00%> (ø)
op-node/flags/flags.go 28.57% <ø> (ø)
op-node/node/client.go 0.00% <0.00%> (ø)
op-node/node/node.go 0.00% <0.00%> (ø)
op-node/service.go 0.00% <0.00%> (ø)
op-service/backoff/operation.go 78.78% <0.00%> (-7.88%) ⬇️
op-node/client/rpc.go 32.71% <36.84%> (+6.39%) ⬆️
op-node/client/polling.go 83.03% <100.00%> (ø)

... and 154 files with indirect coverage changes

@mergify
Copy link
Contributor

mergify bot commented Mar 24, 2023

This PR has been added to the merge queue, and will be merged soon.

@mergify
Copy link
Contributor

mergify bot commented Mar 24, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

1 similar comment
@mergify
Copy link
Contributor

mergify bot commented Mar 24, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

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