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.

Sign up for GitHub

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

Jump to bottom

chore!: add error return params to tree interface #157

Merged
merged 5 commits into from
Apr 14, 2023
Merged

chore!: add error return params to tree interface #157

merged 5 commits into from
Apr 14, 2023

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Mar 31, 2023

Closes #156

@rootulp rootulp self-assigned this Mar 31, 2023
@codecov
Copy link

codecov bot commented Mar 31, 2023 •
edited
Loading

Codecov Report

Merging #157 (1956b16) into master (1e85aab) will decrease coverage by 0.85%.
The diff coverage is 81.63%.

@@            Coverage Diff             @@
##           master     #157      +/-   ##
==========================================
- Coverage   82.28%   81.43%   -0.85%     
==========================================
  Files           7        7              
  Lines         508      528      +20     
==========================================
+ Hits          418      430      +12     
- Misses         54       58       +4     
- Partials       36       40       +4
Impacted Files Coverage Δ
extendeddatacrossword.go 74.03% <59.09%> (-2.50%) ⬇️
datasquare.go 93.71% <100.00%> (+0.28%) ⬆️
tree.go 100.00% <100.00%> (ø)

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

evan-forbes
evan-forbes reviewed Apr 1, 2023
View reviewed changes
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

nice! thanks for doing this, I think its a really good change!

only had a single comment, but otherwise LGTM 👍

datasquare.go Outdated Show resolved Hide resolved
@rootulp
Copy link
Collaborator Author

rootulp commented Apr 4, 2023

Before

goos: darwin
goarch: arm64
pkg: github.com/celestiaorg/rsmt2d
BenchmarkEDSRoots/32x32x256_ODS-10         	     880	   1277862 ns/op	 1791181 B/op	   34052 allocs/op
BenchmarkEDSRoots/64x64x256_ODS-10         	     393	   2902884 ns/op	 6203529 B/op	  133636 allocs/op
BenchmarkEDSRoots/128x128x256_ODS-10       	     100	  10280551 ns/op	26235483 B/op	  530439 allocs/op
BenchmarkEDSRoots/256x256x256_ODS-10       	      31	  37474563 ns/op	106997125 B/op	 2110473 allocs/op
BenchmarkEDSRoots/512x512x256_ODS-10       	       8	 142572162 ns/op	490815672 B/op	 8419332 allocs/op
PASS

After

goos: darwin
goarch: arm64
pkg: github.com/celestiaorg/rsmt2d
BenchmarkEDSRoots/32x32x256_ODS-10         	     939	   1268924 ns/op	 1791219 B/op	   34052 allocs/op
BenchmarkEDSRoots/64x64x256_ODS-10         	     400	   3206118 ns/op	 6203693 B/op	  133637 allocs/op
BenchmarkEDSRoots/128x128x256_ODS-10       	     121	   9694509 ns/op	26235166 B/op	  530437 allocs/op
BenchmarkEDSRoots/256x256x256_ODS-10       	      32	  35897461 ns/op	106996929 B/op	 2110477 allocs/op
BenchmarkEDSRoots/512x512x256_ODS-10       	       8	 137695750 ns/op	490829368 B/op	 8419440 allocs/op
PASS

rootulp
rootulp commented Apr 4, 2023
View reviewed changes
datasquare_test.go
@@ -224,18 +234,6 @@ func computeRowProof(ds *dataSquare, x uint, y uint) ([]byte, [][]byte, uint, ui
return merkleRoot, proof, uint(proofIndex), uint(numLeaves), nil
}

func computeColProof(ds *dataSquare, x uint, y uint) ([]byte, [][]byte, uint, uint, error) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed an unused function

@evan-forbes
Copy link
Member

cc @Wondertan as this api change would have an effect on node

evan-forbes
evan-forbes previously approved these changes Apr 6, 2023
View reviewed changes
Wondertan
Wondertan reviewed Apr 10, 2023
View reviewed changes
datasquare.go Outdated Show resolved Hide resolved
@rootulp rootulp requested a review from Wondertan April 10, 2023 20:54
@rootulp rootulp requested review from evan-forbes and removed request for distractedm1nd April 10, 2023 20:59
@rootulp rootulp removed the request for review from MSevey April 11, 2023 15:15
@rootulp
Copy link
Collaborator Author

rootulp commented Apr 11, 2023

It looks like @Wondertan doesn't have write permissions on this repo so his approval doesn't satisfy the 2 approvals requirement. I removed @MSevey 's review request b/c he's on parental leave so this needs @liamsi 's review.

@Wondertan
Copy link
Member

And @liamsi is ooo as well, maybe @musalbas ?

@rootulp
Copy link
Collaborator Author

rootulp commented Apr 13, 2023

Maybe @cmwaters has write permission to this repo b/c he's a member of core/app

@rootulp rootulp merged commit 6515446 into celestiaorg:master Apr 14, 2023
@rootulp rootulp deleted the rp/errors branch April 14, 2023 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evan-forbes evan-forbes evan-forbes approved these changes

@rach-id rach-id rach-id approved these changes

@Wondertan Wondertan Wondertan approved these changes

@liamsi liamsi Awaiting requested review from liamsi

@musalbas musalbas Awaiting requested review from musalbas

@cmwaters cmwaters Awaiting requested review from cmwaters

Assignees

@rootulp rootulp

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider adding error return parameters for Tree interface methods
4 participants
@rootulp @evan-forbes @Wondertan @rach-id