-
Notifications
You must be signed in to change notification settings - Fork 81
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: NewExtendedDataSquare
with chunkSize param
#236
Conversation
@@ -96,6 +96,49 @@ func TestMarshalJSON(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestNewExtendedDataSquare(t *testing.T) { |
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 for reviewers] I tend to prefer table driven tests but I found it difficult to write a singular table driven test for all the test cases I wanted to cover because the scenarios that cause errors are different.
If reviewers don't like this style of tests, I can try to refactor to something else.
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 makes sense to me, but will defer to celestia-node
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!
Optionally there could be a test that replicates scenario that is currently broken:
- Create test chunks
- Create eds with empty data
- Populate eds by 1/4 of chunks using SetCell
- Repair eds
- Check that GetCell returns valid data (were 0 size chunks before)
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, added some clarifying questions.
Closes #231
This PR adds a new constructor called
NewExtendedDataSquare
that allows @celestia-node to explicitly set thechunkSize
on an empty EDS. I choose to not drop the existingImportExtendedDataSquare
as proposed in #85 because that would be public API breaking.cc: @Wondertan @walldiss