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.

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

feat(consensus): add sync_topic to test config #412

Merged

Conversation

matan-starkware
Copy link
Contributor

@matan-starkware matan-starkware commented Aug 12, 2024

This change is Reviewable

Copy link

codecov bot commented Aug 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.63%. Comparing base (7aaa7cc) to head (e6893b3).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #412      +/-   ##
==========================================
- Coverage   76.63%   76.63%   -0.01%     
==========================================
  Files         348      348              
  Lines       36298    36310      +12     
  Branches    36298    36310      +12     
==========================================
+ Hits        27816    27825       +9     
- Misses       6173     6176       +3     
  Partials     2309     2309              

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

@matan-starkware matan-starkware force-pushed the matan/consensus/m3/node_main_network_refactor branch from 778ca40 to 0039511 Compare August 12, 2024 13:20
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/add_sync_topic_to_test_config branch from 37840e3 to c240f16 Compare August 12, 2024 13:20
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/node_main_network_refactor branch from 0039511 to 4557ca6 Compare August 12, 2024 13:22
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/add_sync_topic_to_test_config branch from c240f16 to 63f95c1 Compare August 12, 2024 13:22
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/node_main_network_refactor branch from 4557ca6 to aa41dfc Compare August 12, 2024 13:26
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/add_sync_topic_to_test_config branch from 63f95c1 to 3b21276 Compare August 12, 2024 13:27
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/node_main_network_refactor branch from aa41dfc to 87b40a7 Compare August 12, 2024 13:39
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/add_sync_topic_to_test_config branch from 3b21276 to f86cf8e Compare August 12, 2024 13:39
@matan-starkware matan-starkware requested review from asmaastarkware and removed request for DvirYo-starkware August 12, 2024 13:46
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/node_main_network_refactor branch from 87b40a7 to 999504e Compare August 13, 2024 10:29
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/add_sync_topic_to_test_config branch from f86cf8e to 4923e37 Compare August 13, 2024 10:29
Copy link
Contributor

@DvirYo-starkware DvirYo-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @asmaastarkware and @matan-starkware)

a discussion (no related file):
Consider using &str as the time instead of String


Copy link
Contributor Author

@matan-starkware matan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @asmaastarkware and @DvirYo-starkware)

a discussion (no related file):

Previously, DvirYo-starkware wrote…

Consider using &str as the time instead of String

I assume time should be type? Meaning sync_topic: &str? I'm not sure how this would work, since I think the config needs to own the data. This would be ok in Default since we have a static &str, but if we actually need to use a command line flag then we would need a real lifetime for the &str.

I checked all of the papyrus configs and they always use String not &str.


@matan-starkware matan-starkware force-pushed the matan/consensus/m3/node_main_network_refactor branch from 999504e to 2bf24d1 Compare August 13, 2024 13:50
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/add_sync_topic_to_test_config branch from 4923e37 to 8410812 Compare August 13, 2024 13:50
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/node_main_network_refactor branch from 2bf24d1 to 507ff6e Compare August 13, 2024 13:53
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/add_sync_topic_to_test_config branch from 8410812 to d80da87 Compare August 13, 2024 13:53
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/node_main_network_refactor branch from 507ff6e to 4347983 Compare August 14, 2024 08:13
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/add_sync_topic_to_test_config branch from d80da87 to 79c631e Compare August 14, 2024 08:14
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/node_main_network_refactor branch from 4347983 to 9a08733 Compare August 14, 2024 08:35
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/node_main_network_refactor branch from cd909f6 to 2cec2b7 Compare August 14, 2024 12:19
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/add_sync_topic_to_test_config branch from 5baf732 to 144119e Compare August 14, 2024 12:19
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/node_main_network_refactor branch from 2cec2b7 to 89307aa Compare August 15, 2024 05:57
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/add_sync_topic_to_test_config branch from 144119e to e8633e4 Compare August 15, 2024 05:57
Copy link
Contributor

@asmaastarkware asmaastarkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DvirYo-starkware)

@matan-starkware matan-starkware force-pushed the matan/consensus/m3/node_main_network_refactor branch from 89307aa to fcf74df Compare August 15, 2024 06:44
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/add_sync_topic_to_test_config branch from e8633e4 to 1d0ada2 Compare August 15, 2024 06:44
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/node_main_network_refactor branch from fcf74df to bffa577 Compare August 15, 2024 07:01
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/add_sync_topic_to_test_config branch from 1d0ada2 to 8a2db1c Compare August 15, 2024 07:01
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/node_main_network_refactor branch from bffa577 to 62cf873 Compare August 15, 2024 07:01
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/add_sync_topic_to_test_config branch from 8a2db1c to 0cc24c8 Compare August 15, 2024 07:02
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/node_main_network_refactor branch 2 times, most recently from c7bd97d to f8f2723 Compare August 15, 2024 07:24
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/add_sync_topic_to_test_config branch 2 times, most recently from d22bed1 to fdfaaa4 Compare August 15, 2024 07:24
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/node_main_network_refactor branch from f8f2723 to 0fc07e5 Compare August 15, 2024 07:26
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/add_sync_topic_to_test_config branch from fdfaaa4 to fb09214 Compare August 15, 2024 07:26
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/node_main_network_refactor branch from 0fc07e5 to 3ffee06 Compare August 15, 2024 11:58
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/add_sync_topic_to_test_config branch from fb09214 to f0a4be3 Compare August 15, 2024 11:58
@matan-starkware matan-starkware changed the base branch from matan/consensus/m3/node_main_network_refactor to graphite-base/412 August 15, 2024 12:26
@matan-starkware matan-starkware force-pushed the matan/consensus/m3/add_sync_topic_to_test_config branch from f0a4be3 to 1595302 Compare August 15, 2024 12:26
@matan-starkware matan-starkware changed the base branch from graphite-base/412 to main August 15, 2024 12:26
Copy link
Contributor

@asmaastarkware asmaastarkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @matan-starkware)

@matan-starkware matan-starkware force-pushed the matan/consensus/m3/add_sync_topic_to_test_config branch from 1595302 to e6893b3 Compare August 15, 2024 12:47
@matan-starkware
Copy link
Contributor Author

matan-starkware commented Aug 15, 2024

Merge activity

@matan-starkware matan-starkware merged commit 010f5d4 into main Aug 15, 2024
22 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants