fix(manager): move update height #1782

tuxcanfly · 2024-07-27T04:07:21Z

Overview

Update height only after block responses are saved.

Summary by CodeRabbit

New Features
- Enhanced testing capabilities for the block publishing process, including validation for successful executions and error handling.
Bug Fixes
- Improved state management logic during block publication to enhance consistency and error handling.
Tests
- Added new test cases to ensure the correct behavior of the block publishing method under various scenarios.

coderabbitai · 2024-07-27T04:07:26Z

Walkthrough

The recent changes enhance the publishBlock method in the Manager class within block/manager.go, refining the execution order to bolster state consistency. Additionally, the testing capabilities in block/manager_test.go have been significantly improved with new functions and comprehensive tests, ensuring thorough validation of both successful and erroneous block publishing scenarios. These modifications align with the objective of achieving atomic storage operations.

Changes

Files	Change Summary
block/manager.go	Repositioned `m.store.SetHeight(ctx, blockHeight)` for improved execution order during block processing.
block/manager_test.go	Added `TestManager_publishBlock` function to enhance validation of the `publishBlock` method across various scenarios with mocked components.

Assessment against linked issues

Objective	Addressed	Explanation
Block processing should use storage atomically (#477)	❓	It's unclear if the changes fully ensure atomicity across all operations as stated.

🐇 In the meadow, blocks do hop,
With heights that climb and never stop.
Through tests so bright, our code we cheer,
Atomic dreams now drawing near!
Let's celebrate this change today,
For a happy path, hip-hip-hooray! 🎉

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 92a7df1 and 0341c26.

Files selected for processing (2)

block/manager.go (2 hunks)
block/manager_test.go (3 hunks)

Additional comments not posted (3)

block/manager_test.go (2)

44-48: LGTM!

The prepareProposalResponse function correctly constructs a response with the transactions from the request.

568-719: Comprehensive test coverage for publishBlock.

The TestManager_publishBlock function effectively tests the publishBlock method under various scenarios, including successful execution and error handling. The use of mocks for dependencies and assertions for expected outcomes is well-implemented.

block/manager.go (1)

944-946: Improved atomicity in block processing.

The repositioning of m.store.SetHeight(ctx, blockHeight) ensures that the store height is updated only after block responses are saved, enhancing atomicity and reducing potential inconsistencies in the database.

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 0341c26 and 82446d4.

Files selected for processing (1)

block/manager_test.go (3 hunks)

Files skipped from review as they are similar to previous changes (1)

block/manager_test.go

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 82446d4 and b7bce1b.

Files selected for processing (1)

block/manager_test.go (3 hunks)

Additional comments not posted (3)

block/manager_test.go (3)

6-11: Imports look good.

The added imports are necessary for the new functionalities and tests.

Also applies to: 15-27, 36-37

44-48: Helper function prepareProposalResponse is well-implemented.

This function correctly returns a mock response for proposal preparation.

568-709: Comprehensive test for publishBlock method.

The test function effectively verifies the behavior of publishBlock in both successful and error scenarios. It ensures the block height is updated correctly and handles errors as expected.

block/manager_test.go

tzdybal

I left some nit-picking comments about error strings - IMHO they can as well be handled as followup good first issue.

block/manager_test.go

tuxcanfly · 2024-08-09T15:19:26Z

There are quite a few err strings around the same place e.g.: failed to save block, failed to Commit. So refactoring might be better suited for a follow up PR.

codecov · 2024-08-09T15:30:00Z

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 43.84%. Comparing base (5f1c0ad) to head (b60bb62).
Report is 1 commits behind head on main.

Files	Patch %	Lines
block/manager.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    rollkit/rollkit#1782      +/-   ##
==========================================
+ Coverage   43.27%   43.84%   +0.56%     
==========================================
  Files          76       76              
  Lines       10033    10033              
==========================================
+ Hits         4342     4399      +57     
+ Misses       4975     4900      -75     
- Partials      716      734      +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between b7bce1b and 6c40f73.

Files selected for processing (1)

block/manager_test.go (3 hunks)

Files skipped from review as they are similar to previous changes (1)

block/manager_test.go

block/manager.go

MSevey

Comment should be updated to prevent confusion.

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 6c40f73 and b60bb62.

Files selected for processing (2)

block/manager.go (2 hunks)
block/manager_test.go (3 hunks)

Files skipped from review as they are similar to previous changes (2)

block/manager.go
block/manager_test.go

github-actions bot assigned tuxcanfly Jul 27, 2024

tuxcanfly force-pushed the tux/fix-block-height branch 2 times, most recently from 0a8ef11 to 58dc593 Compare August 8, 2024 13:54

tuxcanfly force-pushed the tux/fix-block-height branch from 58dc593 to 0341c26 Compare August 9, 2024 02:32

tuxcanfly marked this pull request as ready for review August 9, 2024 02:33

tuxcanfly requested review from Manav-Aggarwal, gupadhyaya and tzdybal August 9, 2024 02:33

RollkitBot requested a review from a team August 9, 2024 02:33

coderabbitai bot reviewed Aug 9, 2024

View reviewed changes

gupadhyaya reviewed Aug 9, 2024

View reviewed changes

block/manager_test.go Outdated Show resolved Hide resolved

gupadhyaya previously approved these changes Aug 9, 2024

View reviewed changes

tzdybal previously approved these changes Aug 9, 2024

View reviewed changes

block/manager_test.go Show resolved Hide resolved

block/manager_test.go Outdated Show resolved Hide resolved

Manav-Aggarwal mentioned this pull request Aug 9, 2024

Extract error as const #1792

Closed

tuxcanfly dismissed stale reviews from tzdybal and gupadhyaya via 3cc3769 August 9, 2024 15:12

Manav-Aggarwal requested review from gupadhyaya and tzdybal August 9, 2024 15:23

coderabbitai bot reviewed Aug 9, 2024

View reviewed changes

gupadhyaya previously approved these changes Aug 13, 2024

View reviewed changes

MSevey reviewed Aug 13, 2024

View reviewed changes

block/manager.go Outdated Show resolved Hide resolved

MSevey suggested changes Aug 13, 2024

View reviewed changes

tuxcanfly added 3 commits August 14, 2024 13:27

fix(manager): move update height

2230ccf

fix(manager): test update height

34e35f8

fix(manager): fix test update height

3682810

tuxcanfly added 5 commits August 14, 2024 13:27

fix(manager): update context timeout

ca3cef4

fix(manager): fix test update height

957aa9d

fix(manager): inline prepareProposalResponse

5daf42c

fix(manager): fix lint

3c3f42b

fix(manager): doc nit

b60bb62

tuxcanfly dismissed gupadhyaya’s stale review via b60bb62 August 14, 2024 20:29

tuxcanfly force-pushed the tux/fix-block-height branch from 6c40f73 to b60bb62 Compare August 14, 2024 20:29

tuxcanfly requested review from MSevey and gupadhyaya August 14, 2024 20:30

coderabbitai bot reviewed Aug 14, 2024

View reviewed changes

gupadhyaya approved these changes Aug 15, 2024

View reviewed changes

MSevey approved these changes Aug 15, 2024

View reviewed changes

MSevey added this pull request to the merge queue Aug 15, 2024

Merged via the queue into main with commit 8deede4 Aug 15, 2024

MSevey deleted the tux/fix-block-height branch August 15, 2024 14:41

Manav-Aggarwal mentioned this pull request Aug 15, 2024

chore(manager): extract some errors as variables #1793

Merged

This was referenced Sep 13, 2024

feat: header/data and sequencer/block-builder separation #1789

Merged

perf: refactor block store #1791

Closed

coderabbitai bot mentioned this pull request Oct 4, 2024

fix: use batchtime from sequencer for header time, upgrade to latest go-da and go-sequencing #1871

Merged

tzdybal mentioned this pull request Oct 8, 2024

Block processing should use storage atomically evstack/ev-abci#21

Closed

tac0turtle removed this from Evolve Apr 24, 2025

fix(manager): move update height #1782

fix(manager): move update height #1782

Uh oh!

Conversation

tuxcanfly commented Jul 27, 2024 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Jul 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Assessment against linked issues

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tzdybal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tuxcanfly commented Aug 9, 2024

Uh oh!

codecov bot commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MSevey left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tuxcanfly commented Jul 27, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jul 27, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

codecov bot commented Aug 9, 2024 •

edited

Loading