chore: share encoding refactor

[mojtaba-esk]

context in the issue #1443

Overview

Checklist

• New and updated code has appropriate documentation
• New and updated code has new and/or updated testing
• Required CI checks are passing
• Visual proof for any user facing features like CLI or documentation updates
• Linked issues closed with keywords

[codecov-commenter]

Codecov Report

Merging #1462 (33da3e9) into main (651b1cf) will increase coverage by 1.43%.
The diff coverage is 79.22%.

@@            Coverage Diff             @@
##             main    #1462      +/-   ##
==========================================
+ Coverage   49.22%   50.66%   +1.43%     
==========================================
  Files          79       82       +3     
  Lines        4477     4719     +242     
==========================================
+ Hits         2204     2391     +187     
- Misses       2089     2134      +45     
- Partials      184      194      +10     

Impacted Files	Coverage Δ
pkg/shares/padding.go	84.00% <62.50%> (-9.11%)	⬇️
pkg/shares/split_sparse_shares.go	46.55% <72.72%> (-18.62%)	⬇️
pkg/shares/parse_compact_shares.go	70.00% <75.60%> (-1.67%)	⬇️
pkg/shares/merge.go	77.35% <76.92%> (-3.90%)	⬇️
pkg/shares/share_builder.go	79.31% <79.31%> (ø)
pkg/shares/shares.go	79.82% <82.60%> (-7.68%)	⬇️
pkg/shares/split_compact_shares.go	82.72% <83.33%> (-4.56%)	⬇️
pkg/proof/proof.go	67.46% <100.00%> (ø)
pkg/shares/parse.go	82.00% <100.00%> (-0.36%)	⬇️
pkg/shares/parse_sparse_shares.go	63.79% <100.00%> (-0.62%)	⬇️

... and 14 files with indirect coverage changes

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

[rootulp]

Overall I like this approach! I think the builder would benefit from unit tests.

In a separate PR we may consider enforcing invariants on the share type (i.e. is a byte slice of length 512) and on a related note enforce that the share type can only be constructed through this share builder. Thoughts?

[mojtaba-esk]

Overall I like this approach! I think the builder would benefit from unit tests.

In a separate PR we may consider enforcing invariants on the share type (i.e. is a byte slice of length 512) and on a related note enforce that the share type can only be constructed through this share builder. Thoughts?

Yeah, totally agreed.
And I let's make a separate PR for error propagation thing.

[evan-forbes]

will review this by eod!! apologies for the delay

[evan-forbes]

overall this is a really really good refactor!! well done @mojtaba-esk and and good idea @rootulp

I do think there are some instances where we are panicking, and if we want this to be a library that can be used by others outside of celestia-app, then I think those need to be removed. Otherwise its way too easy to accidently write a bug where someone can shutdown your light node/whatever. I also think most can be bubbled easily or with little consequence to other current apis, but even if that's not the case, we should avoid panics at all costs for all user facing things. We can definitely do this in a different PR though!

since this is a refactor and we're not in a particular rush, we could take advantage of the new design to add unit tests where previously we couldn't. Addional fuzzers would also be huge bonus, but very optional given our existing high level ones and certainly can be done in a different PR

[evan-forbes]

!

[gitpoap-bot]

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2023 Celestia Contributor:

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.