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: cleanup usage of share.Root(DAH) #2481

Merged
merged 3 commits into from
Sep 7, 2023
Merged

Conversation

Wondertan
Copy link
Member

@Wondertan Wondertan commented Jul 15, 2023

Somewhere we were using Root and somewhere DAH. Also, we mostly use DAH generator from the app.
These PRs clean up all the usages and localize the Root/DAH type in share pkg as was originally intended.

Up to discussion is rename of share.Root to share.DAH to minimize discrepancies between app and node. Alternatively, if we wanna keep Root naming as it is, we should also rename DAH name on the ExtendedHeader(and son tags) and in other places, which is breaking.

Now, I think that calling it Root was not right and we should be calling the DataHash type Root, which is in fact the Merkle root of all the shares in the EDS.

@Wondertan Wondertan self-assigned this Jul 15, 2023
@Wondertan Wondertan added the area:shares Shares and samples label Jul 15, 2023
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

LGTM and thank you for this - are we completely sure it's not breaking? I think it still is bc of header field change despite being same underlying type.

@renaynay
Copy link
Member

Units are also broken @Wondertan

@Wondertan Wondertan force-pushed the hlib/share/cleanup-root branch from 0ae2ad2 to 6b51fe9 Compare September 6, 2023 12:37
@codecov-commenter
Copy link

Codecov Report

Merging #2481 (4423d2f) into main (7601c08) will decrease coverage by 0.10%.
The diff coverage is 52.17%.

@@            Coverage Diff             @@
##             main    #2481      +/-   ##
==========================================
- Coverage   51.35%   51.26%   -0.10%     
==========================================
  Files         158      159       +1     
  Lines       10666    10669       +3     
==========================================
- Hits         5478     5469       -9     
- Misses       4710     4721      +11     
- Partials      478      479       +1     
Files Changed Coverage Δ
share/availability.go 0.00% <0.00%> (ø)
share/empty.go 0.00% <0.00%> (ø)
header/headertest/testing.go 81.30% <75.00%> (ø)
share/availability/cache/availability.go 82.50% <100.00%> (ø)
share/eds/eds.go 63.73% <100.00%> (ø)
share/eds/store.go 69.05% <100.00%> (ø)
share/getters/testing.go 83.33% <100.00%> (ø)

... and 3 files with indirect coverage changes

Copy link
Member

@walldiss walldiss left a comment

Choose a reason for hiding this comment

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

Great cleanup PR!

@Wondertan Wondertan enabled auto-merge (squash) September 7, 2023 09:45
@Wondertan Wondertan merged commit cb46dc1 into main Sep 7, 2023
@Wondertan Wondertan deleted the hlib/share/cleanup-root branch September 7, 2023 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:shares Shares and samples kind:chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants