-
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
feat!: pack squares densely by implementing ADR013 #1604
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.
note that there's only one real logic change required here, the rest is just fixing tests
Codecov Report
@@ Coverage Diff @@
## main #1604 +/- ##
==========================================
+ Coverage 49.05% 49.13% +0.08%
==========================================
Files 85 85
Lines 4964 4976 +12
==========================================
+ Hits 2435 2445 +10
- Misses 2290 2292 +2
Partials 239 239
... and 1 file 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. |
I'm still reviewing but @evan-forbes what are you thoughts on updating the specs (i.e. here) at the same time as merging the implementation? |
that's a good point! I don't want to block the merging of this by codeowners of ADRs and docs, so will do in separate PRs, I added #1606 and #1607 as requirements to the EPIC though |
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 mostly LGTM. One blocking comment. Defer to you if we want to update specs in this PR or after this PR.
{instructions: []WalkInstruction{WalkRight, WalkRight, WalkRight, WalkRight, WalkRight, WalkLeft}, row: 63}, | ||
}}, | ||
{ | ||
"all paths for a basic 2x2", 2, 2, 2, |
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.
[for reviewers] the first few test cases in this file have square layouts like:
// | | |
// |B|B|
// | | |B|B|
// | | | | |
// | | | | |
// | | | | |
// | | | |B|
// |B| | | |
// | | | | |
// | | | | |
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>
…-app into evan/dense-square
Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>
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.
This LGTM. Where do we update the ShareCommitment
generation rules (do they need to be updated). Do we also need to modify the logic for proving blobs?
// SubTreeWidth determines the maximum number of leaves per subtree in the share | ||
// commitment over a given blob. The input should be the total number of shares |
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.
👍 Like this description
Co-authored-by: Callum Waters <cmwaters19@gmail.com>
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.
Thanks for being so responsive and attentive to the comments 👍 LGTM!
// share commitment. If a blob contains more shares than this number, than the height | ||
// of the subtree roots will gradually increases to so that the amount remains within that limit. |
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.
// share commitment. If a blob contains more shares than this number, than the height | |
// of the subtree roots will gradually increases to so that the amount remains within that limit. | |
// share commitment. If a blob contains more shares than this number, then the height | |
// of the subtree roots will increase so that the number of subtree roots remains below the threshold. |
Overview
implements ADR013
closes #1572
part of #1538
Checklist