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(blocksync): share.ReadEDS #1158

Merged
merged 4 commits into from
Oct 20, 2022
Merged

Conversation

distractedm1nd
Copy link
Member

Closes #1104
Includes implementation and tests

Based on #1139

The base of this PR will change along with 1139 when the feature branch is set up and ready

@distractedm1nd distractedm1nd added area:shares Shares and samples kind:feat Attached to feature PRs labels Sep 23, 2022
@distractedm1nd distractedm1nd self-assigned this Sep 23, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #1158 (cdeeeef) into main (8bce8d0) will decrease coverage by 0.12%.
The diff coverage is 61.72%.

@@            Coverage Diff             @@
##             main    #1158      +/-   ##
==========================================
- Coverage   57.10%   56.98%   -0.13%     
==========================================
  Files         139      140       +1     
  Lines        9357     9519     +162     
==========================================
+ Hits         5343     5424      +81     
- Misses       3467     3528      +61     
- Partials      547      567      +20     
Impacted Files Coverage Δ
ipld/nmt_adder.go 64.86% <58.33%> (-3.14%) ⬇️
service/share/eds.go 62.00% <62.00%> (ø)
fraud/bad_encoding.go 63.00% <0.00%> (-3.00%) ⬇️
fraud/pb/proof.pb.go 34.71% <0.00%> (-1.90%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

service/share/eds.go Outdated Show resolved Hide resolved
service/share/eds.go Outdated Show resolved Hide resolved
service/share/eds.go Outdated Show resolved Hide resolved
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.

tiny nits.

share/eds/eds.go Show resolved Hide resolved
share/eds/eds.go Outdated Show resolved Hide resolved
share/eds/eds.go Outdated Show resolved Hide resolved
share/eds/eds_test.go Outdated Show resolved Hide resolved
Wondertan
Wondertan previously approved these changes Oct 19, 2022
share/eds/eds.go Outdated Show resolved Hide resolved
@distractedm1nd
Copy link
Member Author

distractedm1nd commented Oct 19, 2022

TODO:
From: @walldiss

  • Nit: here and in other places there is no need to start error message with "failure..." since it is already an error type and will be handled accordingly. Could be just "get root cids: %w"

From: @renaynay

  • Make sure all t.Cleanup(cancel)s are called under the definition of the ctx

@distractedm1nd distractedm1nd merged commit d08a614 into celestiaorg:main Oct 20, 2022
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:feat Attached to feature PRs
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

share: ReadEDS
5 participants