feat(share): add get proof by namespace #1339

walldiss · 2022-11-10T10:17:39Z

Resolves #1117

codecov-commenter · 2022-11-10T14:10:39Z

Codecov Report

Merging #1339 (24f2e83) into main (bd13b76) will increase coverage by 0.93%.
The diff coverage is 96.52%.

@@            Coverage Diff             @@
##             main    #1339      +/-   ##
==========================================
+ Coverage   55.06%   56.00%   +0.93%     
==========================================
  Files         178      186       +8     
  Lines       10689    11416     +727     
==========================================
+ Hits         5886     6393     +507     
- Misses       4227     4396     +169     
- Partials      576      627      +51

Impacted Files	Coverage Δ
share/eds/retriever.go	`93.71% <ø> (ø)`
share/ipld/nmt.go	`48.43% <ø> (ø)`
share/ipld/get.go	`92.18% <95.40%> (-0.45%)`	⬇️
share/eds/byzantine/share_proof.go	`81.03% <100.00%> (-8.63%)`	⬇️
share/get.go	`90.62% <100.00%> (ø)`
share/ipld/proof.go	`100.00% <100.00%> (ø)`
share/service/service.go	`75.34% <100.00%> (ø)`
nodebuilder/core/opts.go	`0.00% <0.00%> (-50.00%)`	⬇️
nodebuilder/core/config.go	`57.14% <0.00%> (-5.36%)`	⬇️
... and 45 more

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

vgonkivs · 2022-11-10T15:51:37Z

It seems that GetSharesWithProofByNameSpace and GetSharesByNamespace are pretty similar. I'm wondering, can we leave only one implementation but pass additional param(e.g. withProof flag)

wdyt @walldiss , @Wondertan ?

Wondertan · 2022-11-10T16:00:47Z

Yeah, we should do this. Leaving this to @walldiss to decide whether we should do in this PR or make a tech debt/refactoring issues

walldiss · 2022-11-10T16:02:30Z

I think it is a very good suggestion since functions are almost similar. I'll reduce both function to single one

distractedm1nd

Looks great!

share/get_proof_test.go

share/ipld/get.go

renaynay

intermediate comments, haven't reviewed fully.

share/get_proof.go

share/ipld/get.go

share/get_proof.go

share/get_proof_test.go

share/ipld/get.go

share/get_proof.go

share/ipld/get.go

share/get_test.go

share/ipld/get.go

distractedm1nd

I like this a lot, great work

share/ipld/get.go

share/get_test.go

share/ipld/get.go

distractedm1nd

great stuff!

share/ipld/proof.go

…ingle func

Co-authored-by: Ryan <ryanford@poluglottos.com>

Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>

- rescale Intn range - small renames & refactoring

…amespace

Co-authored-by: Ryan <ryan@celestia.org>

walldiss added the kind:feat Attached to feature PRs label Nov 10, 2022

walldiss self-assigned this Nov 10, 2022

walldiss force-pushed the get_shares_with_proofs_by_namespace branch 2 times, most recently from 68ea1cb to 8c9ec98 Compare November 10, 2022 13:48

walldiss changed the title ~~add get proof by namespace~~ share: add get proof by namespace Nov 10, 2022

walldiss changed the title ~~share: add get proof by namespace~~ feat(share): add get proof by namespace Nov 10, 2022

walldiss added the area:shares Shares and samples label Nov 10, 2022

walldiss marked this pull request as ready for review November 10, 2022 14:06

walldiss requested review from renaynay, Wondertan, vgonkivs and distractedm1nd as code owners November 10, 2022 14:06

walldiss force-pushed the get_shares_with_proofs_by_namespace branch 2 times, most recently from 90aacb0 to 2286beb Compare November 10, 2022 15:43

walldiss force-pushed the get_shares_with_proofs_by_namespace branch 2 times, most recently from 1e672fc to 03b441c Compare November 10, 2022 16:31

distractedm1nd reviewed Nov 14, 2022

View reviewed changes

walldiss force-pushed the get_shares_with_proofs_by_namespace branch from 15d61a6 to 9891efa Compare November 14, 2022 16:40

renaynay reviewed Nov 14, 2022

View reviewed changes

Wondertan reviewed Nov 16, 2022

View reviewed changes

share/ipld/get.go Outdated Show resolved Hide resolved

walldiss force-pushed the get_shares_with_proofs_by_namespace branch from 568b7f1 to 939717e Compare November 16, 2022 15:49

Wondertan reviewed Nov 16, 2022

View reviewed changes

share/get_proof.go Outdated Show resolved Hide resolved

renaynay reviewed Nov 17, 2022

View reviewed changes

distractedm1nd previously approved these changes Nov 17, 2022

View reviewed changes

share/ipld/get.go Outdated Show resolved Hide resolved

share/ipld/get.go Outdated Show resolved Hide resolved

MSevey reviewed Nov 18, 2022

View reviewed changes

share/get_test.go Outdated Show resolved Hide resolved

MSevey reviewed Nov 18, 2022

View reviewed changes

share/ipld/get.go Outdated Show resolved Hide resolved

walldiss dismissed Wondertan’s stale review via 674ed6b November 28, 2022 18:19

walldiss force-pushed the get_shares_with_proofs_by_namespace branch 3 times, most recently from 53f3594 to 15df954 Compare November 28, 2022 18:21

renaynay previously approved these changes Nov 28, 2022

View reviewed changes

share/ipld/get.go Show resolved Hide resolved

distractedm1nd previously approved these changes Nov 29, 2022

View reviewed changes

share/ipld/proof.go Outdated Show resolved Hide resolved

walldiss dismissed stale reviews from distractedm1nd and renaynay via 1cfe6e7 November 29, 2022 11:07

walldiss and others added 16 commits November 29, 2022 14:15

add get proof by namespace

435be57

simplify reversal a bit

da07a46

simplify GetLeavesByNamespace and GetLeavesWithProofsByNamespace to s…

ba925ae

…ingle func

Apply suggestions from code review

0539a4d

Co-authored-by: Ryan <ryanford@poluglottos.com>

apply review comments

fcd3de2

Apply suggestions from code review

5f7a5f0

Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>

Update share/ipld/get.go

031afa4

Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>

create ipld.NMTIgnoreMaxNamespace const

af26a52

improve GetLeavesWithProof API semantics

6077684

apply review suggestions

08f280e

Apply review suggestions:

ff260ac

- rescale Intn range - small renames & refactoring

- reduce size of allocated array used for proof collection

443a917

remove locks

3ed5060

use cid instead of node hashes.

afe8f4b

fix returned shares order in case error is returned from GetLeavesByN…

d7bfd30

…amespace

Update share/ipld/proof.go

cecc5bf

Co-authored-by: Ryan <ryan@celestia.org>

walldiss force-pushed the get_shares_with_proofs_by_namespace branch from 1cfe6e7 to cecc5bf Compare November 29, 2022 11:15

Wondertan approved these changes Nov 29, 2022

View reviewed changes

renaynay approved these changes Nov 29, 2022

View reviewed changes

vgonkivs approved these changes Nov 29, 2022

View reviewed changes

distractedm1nd approved these changes Nov 29, 2022

View reviewed changes

walldiss merged commit c05065e into celestiaorg:main Nov 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(share): add get proof by namespace #1339

feat(share): add get proof by namespace #1339

walldiss commented Nov 10, 2022

codecov-commenter commented Nov 10, 2022 •

edited

Loading

vgonkivs commented Nov 10, 2022 •

edited

Loading

Wondertan commented Nov 10, 2022

walldiss commented Nov 10, 2022 •

edited

Loading

distractedm1nd left a comment

renaynay left a comment

distractedm1nd left a comment

distractedm1nd left a comment

feat(share): add get proof by namespace #1339

feat(share): add get proof by namespace #1339

Conversation

walldiss commented Nov 10, 2022

codecov-commenter commented Nov 10, 2022 • edited Loading

Codecov Report

vgonkivs commented Nov 10, 2022 • edited Loading

Wondertan commented Nov 10, 2022

walldiss commented Nov 10, 2022 • edited Loading

distractedm1nd left a comment

Choose a reason for hiding this comment

renaynay left a comment

Choose a reason for hiding this comment

distractedm1nd left a comment

Choose a reason for hiding this comment

distractedm1nd left a comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 10, 2022 •

edited

Loading

vgonkivs commented Nov 10, 2022 •

edited

Loading

walldiss commented Nov 10, 2022 •

edited

Loading