-
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
refactor: cliarify a few names and add some docs to the wrapper #1418
Conversation
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.
LGTM w/ some optional feedback
pkg/wrapper/nmt_wrapper.go
Outdated
// isQuadrant0 returns true if the current share index and axis index are both | ||
// in the original data square. | ||
func (w *ErasuredNamespacedMerkleTree) isQuadrant0() bool { | ||
return !(uint64(w.shareIndex)+1 > w.squareSize || uint64(w.axisIndex)+1 > w.squareSize) | ||
} |
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.
[very optional] removed a negation because this may be clearer for readers
// isQuadrant0 returns true if the current share index and axis index are both | |
// in the original data square. | |
func (w *ErasuredNamespacedMerkleTree) isQuadrant0() bool { | |
return !(uint64(w.shareIndex)+1 > w.squareSize || uint64(w.axisIndex)+1 > w.squareSize) | |
} | |
// isQuadrantZero returns true if the current share index and axis index are both | |
// in the original data square. | |
func (w *ErasuredNamespacedMerkleTree) isQuadrantZero() bool { | |
return w.shareIndex < w.squareSize && w.axisIndex < w.squareSize | |
} |
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.
yes good idea, I just copy pasta'd
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.
couldn't accept here as we had to update the usage to the new name
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
Codecov Report
@@ Coverage Diff @@
## main #1418 +/- ##
==========================================
+ Coverage 48.39% 49.25% +0.85%
==========================================
Files 79 79
Lines 4465 4467 +2
==========================================
+ Hits 2161 2200 +39
+ Misses 2121 2085 -36
+ Partials 183 182 -1
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.
LGTM, would defer to others for final approval
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.
Nice! Thanks a lot for this clarifying PR! (added some suggestions)
// pushed to the tree. It is expected to be in the range: 0 <= shareIndex < | ||
// 2*squareSize. shareIndex is used to help determine which quadrant each | ||
// leaf belongs to. | ||
shareIndex uint64 |
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.
It is worth mentioning that the shareIndex
also keeps track of the number of shares (i.e., the total number of leaves) that have been added to the tree so far.
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.
sure thing! afd0849
Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>
Overview
some non-breaking refactors to hopefully make the wrapper slightly clearer
Checklist