Skip to content

Conversation

@jsvisa
Copy link
Contributor

@jsvisa jsvisa commented Oct 9, 2024

Description

closes #11630

Tests

Additional context

Metadata

@jsvisa jsvisa requested review from a team as code owners October 9, 2024 04:59
@jsvisa jsvisa requested a review from bitwiseguy October 9, 2024 04:59
@codecov
Copy link

codecov bot commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.14%. Comparing base (fb62380) to head (d8dcfc5).
Report is 57 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #12383      +/-   ##
===========================================
- Coverage    64.32%   64.14%   -0.19%     
===========================================
  Files           52       52              
  Lines         4348     4348              
===========================================
- Hits          2797     2789       -8     
- Misses        1376     1385       +9     
+ Partials       175      174       -1     
Flag Coverage Δ
cannon-go-tests 64.14% <ø> (-0.19%) ⬇️

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

see 1 file with indirect coverage changes

@jsvisa
Copy link
Contributor Author

jsvisa commented Oct 9, 2024

@protolambda PTAL

@tynes
Copy link
Contributor

tynes commented Oct 11, 2024

Thank you for the PR @jsvisa !

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.

Looks good! Thank you for implementing this change. Two small nits, then happy to merge. In terms of testing I think it's covered well enough in op-e2e, almost every test already starts the batcher and awaits genesis (depending on genesis timing).

Signed-off-by: jsvisa <[email protected]>
@protolambda protolambda added this pull request to the merge queue Oct 14, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 14, 2024
@protolambda protolambda added this pull request to the merge queue Oct 14, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 14, 2024
@tynes
Copy link
Contributor

tynes commented Oct 14, 2024

small nit would be ok without it

@ajsutton
Copy link
Contributor

I think semgrep is failing when this gets updated with the latest changes from develop: https://app.circleci.com/pipelines/github/ethereum-optimism/optimism/68416/workflows/7f921a39-67d0-4347-a696-ed7d359aa0f4/jobs/2831634

@tynes
Copy link
Contributor

tynes commented Oct 15, 2024

    op-batcher/batcher/driver.go
   ❯❯❱ dgryski.semgrep-go.timeafter.leaky-time-after
          Leaky use of time.After in for-select, see: https://groups.google.com/g/golang-
          nuts/c/cCdm0Ixwi9A/m/jMiJJScAEAAJ                                              
          Details: https://sg.run/dgrQ                                                   
                                                                                         
          310┆ for {
          311┆   cCtx, cancel := context.WithTimeout(ctx, l.Config.NetworkTimeout)
          312┆   syncStatus, err = rollupClient.SyncStatus(cCtx)
          313┆   cancel()
          314┆ 
          315┆   // Ensure that we have the sync status
          316┆   if err != nil {
          317┆           return eth.BlockID{}, eth.BlockID{}, fmt.Errorf("failed to get sync status: %w",
               err)                                                                                      
          318┆   }
          319┆ 
             [hid 17 additional lines, adjust with --max-lines-per-finding] 
                            
  BLOCKING CODE RULES FIRED:
    dgryski.semgrep-go.timeafter.leaky-time-after

@jsvisa
Copy link
Contributor Author

jsvisa commented Oct 17, 2024

I think semgrep is failing when this gets updated with the latest changes from develop: https://app.circleci.com/pipelines/github/ethereum-optimism/optimism/68416/workflows/7f921a39-67d0-4347-a696-ed7d359aa0f4/jobs/2831634

thanks for the info, we did leaked the timer, now use timer.Reset to reuse the timer

@protolambda protolambda added this pull request to the merge queue Oct 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 17, 2024
@protolambda protolambda enabled auto-merge October 17, 2024 14:37
@protolambda protolambda disabled auto-merge October 17, 2024 14:52
@protolambda protolambda enabled auto-merge October 17, 2024 14:53
@protolambda protolambda added this pull request to the merge queue Oct 17, 2024
Merged via the queue into ethereum-optimism:develop with commit 7ce9165 Oct 17, 2024
@jsvisa jsvisa deleted the op-batcher-pre-genesis branch October 22, 2024 12:40
samlaf pushed a commit to samlaf/optimism that referenced this pull request Nov 10, 2024
…imism#12383)

* feat(op-batcher): loop fetch sync status

Signed-off-by: jsvisa <[email protected]>

* feat(op-batcher): wait for l2 genesis time

Signed-off-by: jsvisa <[email protected]>

* feat(batcher): add remaining time for tick printing

Signed-off-by: jsvisa <[email protected]>

* apply code reviews

Signed-off-by: jsvisa <[email protected]>

* use min instead of if test

Signed-off-by: jsvisa <[email protected]>

* fix(batcher): reset timer

Signed-off-by: jsvisa <[email protected]>

---------

Signed-off-by: jsvisa <[email protected]>
Co-authored-by: protolambda <[email protected]>
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.

op-batcher: fix pre-genesis RPC request loop

4 participants