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

refactor!(share): integrate new Namespace type #2388

Merged
merged 23 commits into from
Jun 27, 2023

Conversation

Wondertan
Copy link
Member

@Wondertan Wondertan commented Jun 22, 2023

Notable Changes

  • namespace.ID to share.Namespace
    • Changes every comment namespace ID mentioning to just namespace. I renamed every such mention besides in ADRs. I don't want to touch ADRs here, as they need a more holistic re-review and up-to-date catchup.
    • Uses all the utility methods on the type, where suitable
  • Namespace constructor now only creates Blob Namespaces. For other reserved namespaces, the predefined globals should be used.
  • Uses namespace.ValidateDataNamespace everywhere data is requested. This is guarantees we verify the namespace are 100% valid and forbids requesting parity and padding namespaces.
  • Restricts PFBs for reserved namespaces
  • Reversed the dependency from share -> share/ipld to share/ipld -> share
  • NewBlobV0 constructor. Similar to NewNamespaceV0
  • sharetest pkg for share related testing utilities
  • edstest pkg for eds related testing utilities

Follow-ups

  • blobtest pkg to generate node's blob type

Refs

@Wondertan Wondertan added area:shares Shares and samples kind:break! Attached to breaking PRs kind:refactor Attached to refactoring PRs labels Jun 22, 2023
@Wondertan Wondertan self-assigned this Jun 22, 2023
@Wondertan Wondertan changed the title Hlib/share/namespace integration new refactor!(share): integrate new Namespace type Jun 22, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jun 22, 2023

Codecov Report

Merging #2388 (07c3ec8) into main (0267e21) will increase coverage by 2.38%.
The diff coverage is 41.76%.

@@            Coverage Diff             @@
##             main    #2388      +/-   ##
==========================================
+ Coverage   50.68%   53.07%   +2.38%     
==========================================
  Files         157      156       -1     
  Lines        9935     9915      -20     
==========================================
+ Hits         5036     5262     +226     
+ Misses       4456     4199     -257     
- Partials      443      454      +11     
Impacted Files Coverage Δ
api/gateway/endpoints.go 0.00% <0.00%> (ø)
api/gateway/share.go 7.69% <0.00%> (ø)
header/headertest/testing.go 72.48% <0.00%> (+0.55%) ⬆️
nodebuilder/share/constructors.go 12.76% <0.00%> (-0.88%) ⬇️
nodebuilder/share/module.go 0.00% <ø> (ø)
nodebuilder/share/share.go 0.00% <ø> (ø)
share/empty.go 0.00% <0.00%> (-55.56%) ⬇️
share/getter.go 0.00% <0.00%> (ø)
share/getters/cascade.go 78.08% <0.00%> (ø)
share/getters/tee.go 37.50% <0.00%> (ø)
... and 25 more

... and 4 files with indirect coverage changes

@Wondertan
Copy link
Member Author

Now the PR is properly rebased

nodebuilder/share/constructors.go Show resolved Hide resolved
share/eds/byzantine/bad_encoding_test.go Show resolved Hide resolved
share/namespace.go Outdated Show resolved Hide resolved
share/p2p/shrexnd/pb/share.proto Show resolved Hide resolved
renaynay
renaynay previously approved these changes Jun 23, 2023
share/availability/test/testing.go Show resolved Hide resolved
share/p2p/shrexnd/client.go Outdated Show resolved Hide resolved
blob/blob.go Show resolved Hide resolved
vgonkivs
vgonkivs previously approved these changes Jun 26, 2023
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.

Gigantic work, thank you for this!

  1. Not sure how this test is passing, when it is clearly broken. Pls take a look:
    share/availability/light/availability_test.go:162
randNID := root.RowRoots[(len(root.RowRoots)-1)/2][:8]
  1. There are few places that depend on appconsts.NamespaceSize, that could use on share.NamespaceSize instead.

share/eds/eds.go Outdated Show resolved Hide resolved
share/empty.go Outdated Show resolved Hide resolved
share/getters/shrex_test.go Show resolved Hide resolved
share/getters/shrex_test.go Show resolved Hide resolved
share/namespace.go Outdated Show resolved Hide resolved
share/namespace_test.go Outdated Show resolved Hide resolved
share/namespace.go Outdated Show resolved Hide resolved
@Wondertan
Copy link
Member Author

Thank you for the great review @walldiss!

@Wondertan Wondertan dismissed stale reviews from vgonkivs and renaynay via cedb940 June 26, 2023 11:54
@Wondertan Wondertan force-pushed the hlib/share/namespace-integration-new branch from 47e95dc to bce5a03 Compare June 26, 2023 12:46
@Wondertan
Copy link
Member Author

Rebased on main to resolve a conflict

@Wondertan Wondertan force-pushed the hlib/share/namespace-integration-new branch from 8f379d2 to 4d87ba1 Compare June 26, 2023 14:20
@Wondertan
Copy link
Member Author

Rebased on main

@Wondertan Wondertan enabled auto-merge June 27, 2023 13:26
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.

finally

@Wondertan Wondertan added this pull request to the merge queue Jun 27, 2023
Merged via the queue into main with commit ccae150 Jun 27, 2023
15 of 17 checks passed
@Wondertan Wondertan deleted the hlib/share/namespace-integration-new branch June 27, 2023 13:40
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:break! Attached to breaking PRs kind:refactor Attached to refactoring PRs
Projects
None yet
6 participants