-
Notifications
You must be signed in to change notification settings - Fork 252
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!: fixed sequence len #1137
feat!: fixed sequence len #1137
Conversation
Markdownlint fails here b/c our blog appears offline |
@@ -54,12 +54,12 @@ func Test_finalizeLayout(t *testing.T) { | |||
}, | |||
{ | |||
squareSize: 4, | |||
nonreserveStart: 6, | |||
nonreserveStart: 7, | |||
ptxs: generateParsedTxsWithNIDs( |
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] this test now generates 7 compact shares so the nonreserveStart
field had to be updated and one blob had to be removed to fit in a square size of 4
@@ -77,14 +77,14 @@ func TestCreateCommitment(t *testing.T) { | |||
name: "blob of 11 shares succeeds", | |||
namespace: bytes.Repeat([]byte{0xFF}, 8), | |||
blob: bytes.Repeat([]byte{0xFF}, 11*ShareSize), | |||
expected: []byte{0x1e, 0xdc, 0xc4, 0x69, 0x8f, 0x47, 0xf6, 0x8d, 0xfc, 0x11, 0xec, 0xac, 0xaa, 0x37, 0x4a, 0x3d, 0xbd, 0xfc, 0x1a, 0x9b, 0x6e, 0x87, 0x6f, 0xba, 0xd3, 0x6c, 0x6, 0x6c, 0x9f, 0x5b, 0x65, 0x38}, | |||
expected: []byte{0x9f, 0x44, 0xd5, 0x12, 0xe9, 0x6b, 0xea, 0xb3, 0xf2, 0xfe, 0x7b, 0x46, 0xc6, 0x4c, 0xee, 0x70, 0xb0, 0x86, 0xca, 0x94, 0x7e, 0x1b, 0x95, 0xd2, 0x0, 0x78, 0x32, 0xb5, 0x94, 0x68, 0x67, 0xf0}, |
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] until this unit test verifies the commitment bytes, these bytes are expected to change whenever we modify how the commitment is generated or the schema of the blob 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.
yaaaasssssssss!! very nice! I still need to look this over again to make sure I didn't miss anything, but first glance is really good.
Codecov Report
@@ Coverage Diff @@
## main #1137 +/- ##
==========================================
+ Coverage 47.98% 48.23% +0.25%
==========================================
Files 71 71
Lines 3995 3997 +2
==========================================
+ Hits 1917 1928 +11
+ Misses 1909 1899 -10
- Partials 169 170 +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.
couldn't find anything else to change! all changes make sense to me. great work!
can't wait for the website to go back up so we can get ✔️ again... |
Follow up to #1137 that fixes Godocs and removes an inaccurate helper function. While investigating informalsystems/audit-celestia#14 I noticed that the `BlobSharesUsed` helper still exists. It should be removed because it assumes blobs are prefixed with varints (no longer the case) and doesn't differentiate between the first and continuation shares in a sparse share sequence. The helper `SparseSharesNeeded` should be used instead.
Follow up to celestiaorg/celestia-app#1137 that fixes Godocs and removes an inaccurate helper function. While investigating informalsystems/audit-celestia#14 I noticed that the `BlobSharesUsed` helper still exists. It should be removed because it assumes blobs are prefixed with varints (no longer the case) and doesn't differentiate between the first and continuation shares in a sparse share sequence. The helper `SparseSharesNeeded` should be used instead.
Closes #1121
Closes #1053
Closes #1118