feat: implement non-interactive defaults logic #680
Conversation
Codecov Report
@@ Coverage Diff @@
## main #680 +/- ##
==========================================
+ Coverage 39.28% 40.41% +1.12%
==========================================
Files 27 28 +1
Lines 2795 2853 +58
==========================================
+ Hits 1098 1153 +55
- Misses 1609 1611 +2
- Partials 88 89 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
evan-forbes
requested review from
adlerjohn,
liamsi,
mojtaba-esk,
rahulghangas,
rootulp and
rach-id
|
|
||
| // MsgSharesUsedNIDefaults calculates the number of shares used by a given set | ||
| // of messages share lengths. It follows the non-interactive default rules and | ||
| // assumes that each msg length in msgShareLens |
There was a problem hiding this comment.
I think the rest of this comment got lost. Perhaps it meant to say
| // assumes that each msg length in msgShareLens | |
| // assumes that each msg length in msgShareLens is <= origSquareSize |
If so, should this function panic if this assumption is not met?
There was a problem hiding this comment.
This function does not rely on that assumption, otherwise it would not be able to handles messges that are larger than a single row. It definitely looks like we have to complete those docs, not exactly sure what I was going for originally 91c0d72
| fits: false, | ||
| }, | ||
| { | ||
| name: "8 msgs of various sizes, 7 starting shares (63 msg shares, 1 contigous, size 8)", |
There was a problem hiding this comment.
[no change needed] I had a tough time visualizing so sharing if helpful for other reviewers
| |3|3|3| | | | |
|9|9|9|9|9|9|9|9|
|9| | | |3|3|3| |
|7|7|7|7|7|7|7| |
|8|8|8|8|8|8|8|8|
|3|3|3| | | | | |
|7|7|7|7|7|7|7| |
|8|8|8|8|8|8|8|8|
| fits: true, | ||
| }, | ||
| { | ||
| name: "8 msgs of various sizes, 7 starting shares (63 msg shares, 6 contigous, size 8)", |
There was a problem hiding this comment.
[question] I don't understand what contiguous means in this context. Given the cursor starts at 6 in this test, is it possible contiguous should instead be start or cursor?
There was a problem hiding this comment.
Sorry, I still don't understand what contiguous means in this context or why 6 is incorrect but 7 is correct.
There was a problem hiding this comment.
starting was just a confusing way to say contiguous shares. switched all to compact here 2bc32cf
There was a problem hiding this comment.
Okay thanks for explaining. I think the test names in 2bc32cf are inaccurate so commented #681 (review)
There was a problem hiding this comment.
yeah, sorry, accidentally pushed to the wrong branch. will fix there.
| tests := []test{ | ||
| {2, 4, 1, []int{1}, []uint32{2}}, | ||
| {2, 2, 1, []int{1}, []uint32{2}}, | ||
| {3, 4, 8, []int{3, 3}, []uint32{4, 8}}, |
There was a problem hiding this comment.
[no change needed] visual
| | | | |
|3|3|3| |
|3|3|3| |
| | | | |
Co-authored-by: Rootul Patel <[email protected]>
Co-authored-by: Rootul Patel <[email protected]>
Co-authored-by: Rootul Patel <[email protected]>
Co-authored-by: Rootul Patel <[email protected]>
adds the non-interactive default logic to the share's package as described in #673
part of #382