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

chore: De-duplicate code to create BlockHeader for genesis and view #12109

Merged
merged 12 commits into from
Oct 11, 2024

Conversation

tayfunelmas
Copy link
Contributor

@tayfunelmas tayfunelmas commented Sep 18, 2024

We address a TODO item to use BlockHeader::new when creating a BlockHeader in two other places (today these places use similar logic to choose the right BlockHeader version and contruct the inner structs based on the protocol version).

  1. When generating BlockHeader from BlockHeaderView.
  2. When generating the genesis BlockHeader for the genesis block.

For this, we add BlockHeader::new_impl to contain the common code and have two other wrapper functions to call new_impl for the use cases above: BlockHeader::from_view and BlockHeader::genesis.

@tayfunelmas tayfunelmas marked this pull request as ready for review September 19, 2024 09:34
@tayfunelmas tayfunelmas requested a review from a team as a code owner September 19, 2024 09:34
@tayfunelmas tayfunelmas removed the request for review from akhi3030 September 19, 2024 10:25
@tayfunelmas tayfunelmas marked this pull request as draft September 19, 2024 10:26
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 99.19028% with 2 lines in your changes missing coverage. Please review.

Project coverage is 71.74%. Comparing base (99ecfa4) to head (f789307).

Files with missing lines Patch % Lines
core/primitives/src/block_header.rs 99.53% 0 Missing and 1 partial ⚠️
core/primitives/src/views.rs 96.87% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12109      +/-   ##
==========================================
+ Coverage   71.69%   71.74%   +0.04%     
==========================================
  Files         825      825              
  Lines      165834   165697     -137     
  Branches   165834   165697     -137     
==========================================
- Hits       118902   118884      -18     
+ Misses      41751    41635     -116     
+ Partials     5181     5178       -3     
Flag Coverage Δ
backward-compatibility 0.17% <0.00%> (+<0.01%) ⬆️
db-migration 0.17% <0.00%> (+<0.01%) ⬆️
genesis-check 1.26% <0.00%> (+<0.01%) ⬆️
integration-tests 38.91% <99.19%> (+0.06%) ⬆️
linux 71.43% <99.19%> (+0.05%) ⬆️
linux-nightly 71.34% <99.19%> (+0.07%) ⬆️
macos 53.73% <95.54%> (-0.52%) ⬇️
pytests 1.57% <0.00%> (+<0.01%) ⬆️
sanity-checks 1.38% <0.00%> (+<0.01%) ⬆️
unittests 65.53% <95.54%> (+0.15%) ⬆️
upgradability 0.21% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@tayfunelmas tayfunelmas marked this pull request as ready for review September 19, 2024 14:20
@tayfunelmas tayfunelmas assigned akhi3030 and unassigned akhi3030 Sep 19, 2024
Copy link
Member

@Longarithm Longarithm left a comment

Choose a reason for hiding this comment

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

This is great, thank you!

core/primitives/src/block_header.rs Outdated Show resolved Hide resolved
}

/// Common logic for generating BlockHeader for different purposes, including new blocks, from views, and for genesis block
fn new_impl(
Copy link
Member

Choose a reason for hiding this comment

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

We could probably unite arguments in BlockHeaderInnerLite, BlockHeaderInnerRestV1 (?) and the rest earlier to reduce number of arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be great to reduce the number of arguments (it is way too many now), but not sure how to do that at this point, let me submit this and I will reconsider that in a new PR.

@tayfunelmas tayfunelmas added this pull request to the merge queue Oct 11, 2024
Merged via the queue into near:master with commit 30e4ace Oct 11, 2024
29 of 30 checks passed
@tayfunelmas tayfunelmas deleted the header-new branch October 11, 2024 13:24
@tayfunelmas tayfunelmas restored the header-new branch October 11, 2024 13:27
@tayfunelmas tayfunelmas deleted the header-new branch October 11, 2024 13:28
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.

3 participants