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

internal/sequencerapi,miner: move conditional rate limiter to the rpc layer instead of miner #377

Merged
merged 1 commit into from
Sep 25, 2024
Merged

internal/sequencerapi,miner: move conditional rate limiter to the rpc layer instead of miner #377

merged 1 commit into from
Sep 25, 2024
+38 −49

Conversation

hamdiallam
Copy link
Contributor

@hamdiallam hamdiallam commented Sep 12, 2024 •
edited
Loading

Description
Incorporating a suggestion from @tynes

Since rpc requests are multi-routine, we can use the WaitN functionality with the parent context, allowing the request timeout to simply cancel the reservation.

Renamed the flag names such that they have the same prefix for consistency

Tests

Additional context
A followup up #330

Metadata

@hamdiallam hamdiallam requested a review from a team as a code owner September 12, 2024 18:05
@anacrolix anacrolix self-assigned this Sep 12, 2024
anacrolix
anacrolix approved these changes Sep 13, 2024
View reviewed changes
Copy link
Contributor

@anacrolix anacrolix left a comment

Choose a reason for hiding this comment

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

Looks good. Few questions but nothing blocking.

eth/ethconfig/config.go Outdated Show resolved Hide resolved
eth/backend.go Outdated Show resolved Hide resolved
internal/sequencerapi/api.go Outdated Show resolved Hide resolved
internal/sequencerapi/api.go Outdated Show resolved Hide resolved
internal/sequencerapi/api.go Show resolved Hide resolved
@tynes
Copy link
Contributor

tynes commented Sep 16, 2024

Please squash the commits into a single commit before merge, with a commit message in the style that is used by geth

@protolambda
Copy link
Collaborator

Can you please rebase this PR @hamdiallam ? We had to squash some sloppy commits on the base branch. Also please squash your net-new commits on this PR, so this PR is merge-ready.

@hamdiallam
Copy link
Contributor Author

Can you please rebase this PR @hamdiallam ? We had to squash some sloppy commits on the base branch. Also please squash your net-new commits on this PR, so this PR is merge-ready.

yes will do! @protolambda @tynes

out of curiosity should squash commit by default be enabled for PRs? If It was my op-geth PR that wasn't squashed, I apologize. Used to that being the norm and not thinking about clicking merge

@tynes
Copy link
Contributor

tynes commented Sep 17, 2024

Can you please rebase this PR @hamdiallam ? We had to squash some sloppy commits on the base branch. Also please squash your net-new commits on this PR, so this PR is merge-ready.

yes will do! @protolambda @tynes

out of curiosity should squash commit by default be enabled for PRs? If It was my op-geth PR that wasn't squashed, I apologize. Used to that being the norm and not thinking about clicking merge

We don't have squash merge on bc of how it interacts with upstream, so the norm is to manually squash commits before merge

@hamdiallam
Copy link
Contributor Author

hamdiallam commented Sep 17, 2024 •
edited
Loading

out of curiosity should squash commit by default be enabled for PRs? If It was my op-geth PR that wasn't squashed, I

ahh okay, gotcha.

Edit: Just seeing the convo in discord

@hamdiallam
Copy link
Contributor Author

rebase'd and squashed @protolambda @tynes

@tynes
Copy link
Contributor

tynes commented Sep 18, 2024

@hamdiallam Can you update the commit message to match the style that geth uses? package name: feat

@hamdiallam hamdiallam changed the title change: conditional rate limiter in the rpc layer instead of miner internal/sequencerapi: conditional rate limiter in the rpc layer instead of miner Sep 18, 2024
@hamdiallam hamdiallam changed the title internal/sequencerapi: conditional rate limiter in the rpc layer instead of miner internal/sequencerapi: move conditional rate limiter to the rpc layer instead of miner Sep 18, 2024
@hamdiallam hamdiallam changed the title internal/sequencerapi: move conditional rate limiter to the rpc layer instead of miner internal/sequencerapi, miner: move conditional rate limiter to the rpc layer instead of miner Sep 18, 2024
@hamdiallam hamdiallam changed the title internal/sequencerapi, miner: move conditional rate limiter to the rpc layer instead of miner internal/sequencerapi,miner: move conditional rate limiter to the rpc layer instead of miner Sep 20, 2024
@hamdiallam hamdiallam enabled auto-merge (squash) September 20, 2024 10:01
@hamdiallam
Copy link
Contributor Author

cc @tynes soft bump just so that this commit ends up in the same release tag as the main PR since this contains a flag name change

@hamdiallam
Copy link
Contributor Author

bump @tynes

@hamdiallam hamdiallam merged commit 6da7cc2 into optimism Sep 25, 2024
10 checks passed
@hamdiallam hamdiallam deleted the sendtxcond.limiter branch September 25, 2024 04:47
@hamdiallam hamdiallam mentioned this pull request Dec 20, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anacrolix anacrolix anacrolix approved these changes

@tynes tynes tynes approved these changes

@axelKingsley axelKingsley Awaiting requested review from axelKingsley axelKingsley is a code owner automatically assigned from ethereum-optimism/op-geth-maintainers

Assignees

@anacrolix anacrolix

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hamdiallam @tynes @protolambda @anacrolix