Skip to content

Conversation

@prajwolrg
Copy link
Contributor

Resolves #90

@prajwolrg prajwolrg requested a review from delbonis July 25, 2024 01:12
delbonis
delbonis previously approved these changes Jul 25, 2024
Copy link
Contributor

@delbonis delbonis left a comment

Choose a reason for hiding this comment

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

Looks pretty good, just the weird thing that would be nice to clean up. If it's too much trouble to change I think it can be merged.

@prajwolrg prajwolrg marked this pull request as ready for review July 28, 2024 07:41
@prajwolrg prajwolrg force-pushed the 90-finalized-blocks branch 2 times, most recently from d8bea9d to 2502589 Compare July 28, 2024 11:15
@codecov
Copy link

codecov bot commented Jul 28, 2024

Codecov Report

Attention: Patch coverage is 80.98160% with 31 lines in your changes missing coverage. Please review.

Project coverage is 53.85%. Comparing base (350f4df) to head (c76e73d).

Files Patch % Lines
crates/consensus-logic/src/duty_executor.rs 81.81% 16 Missing ⚠️
crates/consensus-logic/src/duties.rs 0.00% 15 Missing ⚠️
@@            Coverage Diff             @@
##           master     #164      +/-   ##
==========================================
+ Coverage   53.62%   53.85%   +0.23%     
==========================================
  Files          86       86              
  Lines        7596     7687      +91     
==========================================
+ Hits         4073     4140      +67     
- Misses       3523     3547      +24     
Files Coverage Δ
crates/consensus-logic/src/unfinalized_tracker.rs 94.90% <100.00%> (-0.62%) ⬇️
crates/test-utils/src/l2.rs 98.66% <100.00%> (+1.69%) ⬆️
crates/consensus-logic/src/duties.rs 0.00% <0.00%> (ø)
crates/consensus-logic/src/duty_executor.rs 22.29% <81.81%> (+22.29%) ⬆️

... and 4 files with indirect coverage changes

@prajwolrg prajwolrg force-pushed the 90-finalized-blocks branch 2 times, most recently from d89f3e8 to 6af2468 Compare July 29, 2024 03:05
Copy link
Contributor

@delbonis delbonis left a comment

Choose a reason for hiding this comment

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

Some minor cleanup but otherwise looks good I think. I'll DM about the bookkeeping I mentioned in the previous review.

delbonis
delbonis previously approved these changes Jul 31, 2024
Copy link
Contributor

@delbonis delbonis 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, I didn't think that would be this clean.

@prajwolrg prajwolrg force-pushed the 90-finalized-blocks branch 3 times, most recently from d59f55f to 71e93cf Compare August 1, 2024 15:11
@prajwolrg prajwolrg requested a review from delbonis August 2, 2024 01:09
@prajwolrg prajwolrg force-pushed the 90-finalized-blocks branch from 71e93cf to c76e73d Compare August 3, 2024 03:09
@delbonis delbonis merged commit f333d45 into master Aug 5, 2024
@storopoli storopoli deleted the 90-finalized-blocks branch November 28, 2024 10:38
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.

Determine which blocks are finalized in sequencer duty executor

3 participants