-
Notifications
You must be signed in to change notification settings - Fork 925
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
fix(blob/service): fix getByCommitment #2828
Conversation
d34143e
to
dbe35db
Compare
dbe35db
to
068e114
Compare
Codecov Report
@@ Coverage Diff @@
## main #2828 +/- ##
==========================================
- Coverage 51.50% 51.32% -0.19%
==========================================
Files 168 168
Lines 10764 10750 -14
==========================================
- Hits 5544 5517 -27
- Misses 4743 4752 +9
- Partials 477 481 +4
|
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.
I'm a bit confused around some naming in this 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.
The logic looked correct to me and I don't have any simplification in mind.
We should add a regression test for the fixed case.
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.
Much much simpler and more readable then the initial solution. Well done
0a5747a
to
2325579
Compare
Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>
2325579
to
209e1cb
Compare
Overview
Fixes #2808
Root cause: I did not check the entire row once the first blob was identified(and moved to the next row). So, in the end, we had a lot of unchecked blobs. Reworked logic now goes through the whole rawShares, until it is possible, constructing blobs and verifying them.
Checklist