-
Notifications
You must be signed in to change notification settings - Fork 2
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
Da specs #70
Da specs #70
Conversation
I finished the first review. looks nice 🚀 |
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.
Well structured, 🚀
I'm merging and open deeper PRs on top of it. |
assert len(encoded_data.row_commitments) == len(self.nodes) | ||
assert len(encoded_data.row_proofs) == len(self.nodes) | ||
columns = encoded_data.extended_matrix.columns() | ||
column_commitments = encoded_data.column_commitments | ||
row_commitments = encoded_data.row_commitments | ||
rows_proofs = encoded_data.row_proofs | ||
aggregated_column_commitment = encoded_data.aggregated_column_commitment | ||
aggregated_column_proof = encoded_data.aggregated_column_proof | ||
for index, (column, column_commitment, row_proofs) in enumerate(zip(columns, column_commitments, rows_proofs)): |
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.
nitpick: It seems that we also need to assert len(columns) == len(column_commitments) == len(self.nodes)
(or == len(row_proofs)
, so we can make sure that the zip
in the line 22 produces the expected number of elements. I came up with this point when reading the disperse
function below. But, this is not very important. Also, it's okay if the definition of columns
and column_commitments
already imply this property.
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.
Good catch. I'll add this in subsequent PRs. Thanks!!
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
This is the main branch for nomos da specs. Currently there is a broad skeleton of what we intend to do without any of the kzg+rs implementation. This may be a bit too big already. I intend to at least fill a few more tests here and then use other branches to merge on top of this.
Notice that this is incomplete and open as a draft, but reviews are more than welcome.