-
Notifications
You must be signed in to change notification settings - Fork 278
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
chore: share encoding refactor #1462
Conversation
Codecov Report
@@ 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
... 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Co-authored-by: Rootul P <rootulp@gmail.com>
Yeah, totally agreed. |
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
…tia-app into mojtaba/share-refactor
will review this by eod!! apologies for the delay |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
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. |
Appears residual after celestiaorg#1462
Thanks @cmwaters for discovering. Appears residual after celestiaorg/celestia-app#1462. Co-authored-by: Callum Waters <cmwaters19@gmail.com>
Thanks @cmwaters for discovering. Appears residual after celestiaorg/celestia-app#1462. Co-authored-by: Callum Waters <cmwaters19@gmail.com>
context in the issue #1443
Overview
Checklist