-
Notifications
You must be signed in to change notification settings - Fork 27
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
chore(batcher): add next_block_info method to block builder factory #2231
Conversation
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## arni/batcher/block_builder_factor/refactor/remove_block_metadata #2231 +/- ##
===================================================================================================
- Coverage 5.18% 5.18% -0.01%
===================================================================================================
Files 146 146
Lines 16981 16986 +5
Branches 16981 16986 +5
===================================================================================================
Hits 880 880
- Misses 16027 16032 +5
Partials 74 74 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @alonh5, @ArniStarkware, and @dafnamatsry)
crates/starknet_batcher/src/block_builder.rs
line 319 at r1 (raw file):
) -> BlockBuilderResult<TransactionExecutor<PapyrusReader>> { let block_builder_config = self.block_builder_config.clone(); // TODO(Arni): Get as a parameter for this function.
why no take the block number as you did here?
Code quote:
// TODO(Arni): Get as a parameter for this function.
e221112
to
54dff5c
Compare
78e3a01
to
58aa988
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @alonh5, @dafnamatsry, and @Yael-Starkware)
crates/starknet_batcher/src/block_builder.rs
line 319 at r1 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
why no take the block number as you did here?
This TODO is no longer relevant. I should removed it.
I don't want to pass the height
separately, as it is a part of the block_info
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @alonh5, @ArniStarkware, and @dafnamatsry)
crates/starknet_batcher/src/block_builder.rs
line 302 at r2 (raw file):
BlockInfo { block_number: height, block_timestamp: BlockTimestamp(chrono::Utc::now().timestamp().try_into().unwrap()),
if you remove this error, you also need to remove it from the error types.
Code quote:
.unwrap())
58aa988
to
ea98c81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @alonh5, @dafnamatsry, and @Yael-Starkware)
crates/starknet_batcher/src/block_builder.rs
line 302 at r2 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
if you remove this error, you also need to remove it from the error types.
Now not using unwrap()
in production code.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alonh5 and @dafnamatsry)
54dff5c
to
a33b3cc
Compare
ea98c81
to
3b0a743
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware, @dafnamatsry, and @Yael-Starkware)
a discussion (no related file):
Also no need for this intermediate PR. I'm having trouble keeping track. Above PR #2246 there could be one PR passing the block info from the consensus in the input (which is PR #2237 with some additions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @alonh5, @dafnamatsry, and @Yael-Starkware)
a discussion (no related file):
Previously, alonh5 (Alon Haramati) wrote…
Also no need for this intermediate PR. I'm having trouble keeping track. Above PR #2246 there could be one PR passing the block info from the consensus in the input (which is PR #2237 with some additions).
This PR is indeed no longer relevant.
This PR is no longer relevant. |
Refactor towards getting block info from consensus.