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

Refactor start_rollup #1210

Merged
merged 14 commits into from
Sep 26, 2024
Merged

Refactor start_rollup #1210

merged 14 commits into from
Sep 26, 2024

Conversation

exeokan
Copy link
Contributor

@exeokan exeokan commented Sep 20, 2024 •
edited
Loading

Description

start_rollup function has 12 parameters where 7 of them can be fitted inside the rollup_config, and sequencer_config parameters. With these changes, to use the start_rollup, FullNodeConfig and SequencerConfig objects must be created beforehand and be passed to the start_rollup. This makes the parameters categorized and easy to understand, also removes the need to pass dummy values to them when they are not needed.

Linked Issues

@exeokan exeokan marked this pull request as ready for review September 23, 2024 14:17
exeokan
exeokan commented Sep 23, 2024
View reviewed changes
bin/citrea/tests/e2e/mod.rs Outdated Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented Sep 23, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.6%. Comparing base (af71dd3) to head (5ccca71).
Report is 28 commits behind head on nightly.

Additional details and impacted files
Files with missing lines Coverage Δ
crates/sequencer/src/config.rs 100.0% <100.0%> (ø)

... and 5 files with indirect coverage changes

rakanalh
rakanalh reviewed Sep 23, 2024
View reviewed changes
Copy link
Contributor

@rakanalh rakanalh left a comment

Choose a reason for hiding this comment

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

Can you please describe how this refactoring improved things?

  1. What problems did we originally have that this fixes?
  2. How was the UX of writing tests improved?

bin/citrea/tests/test_helpers/mod.rs Show resolved Hide resolved
bin/citrea/tests/test_helpers/mod.rs Outdated Show resolved Hide resolved
jfldde
jfldde reviewed Sep 24, 2024
View reviewed changes
bin/citrea/tests/e2e/mod.rs Outdated Show resolved Hide resolved
@eyusufatik eyusufatik added the HOLD-MERGE PR is not draft but should not be merged yet label Sep 24, 2024
@eyusufatik eyusufatik removed the HOLD-MERGE PR is not draft but should not be merged yet label Sep 25, 2024
eyusufatik
eyusufatik requested changes Sep 25, 2024
View reviewed changes
bin/citrea/tests/soft_confirmation_rule_enforcer/mod.rs Outdated Show resolved Hide resolved
bin/citrea/tests/mempool/mod.rs Outdated Show resolved Hide resolved
bin/citrea/tests/evm/tracing.rs Outdated Show resolved Hide resolved
bin/citrea/tests/evm/subscription.rs Outdated Show resolved Hide resolved
bin/citrea/tests/evm/mod.rs Outdated Show resolved Hide resolved
@eyusufatik eyusufatik merged commit 7c7941c into nightly Sep 26, 2024
14 checks passed
@eyusufatik eyusufatik deleted the ege/refactor-start-rollup branch September 26, 2024 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rakanalh rakanalh rakanalh left review comments

@jfldde jfldde jfldde left review comments

@eyusufatik eyusufatik eyusufatik approved these changes

@ercecan ercecan Awaiting requested review from ercecan

@kpp kpp Awaiting requested review from kpp

@yaziciahmet yaziciahmet Awaiting requested review from yaziciahmet

Assignees

@exeokan exeokan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor start_rollup test helper
4 participants
@exeokan @rakanalh @eyusufatik @jfldde