Skip to content
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

Rename column_index to cell_index in KZG spec #3841

Merged
merged 7 commits into from
Jul 15, 2024

Conversation

jtraglia
Copy link
Member

Primarily, this PR renames some of the index params in the polynomial commitment spec. We do this because KZG libraries should have no concept of "row" or "column". The core DAS spec will still use RowIndex and ColumnIndex types.

Notably:

  • column_indices -> cell_indices
  • row_indices -> commitment_indices
  • row_commitments -> commitments

It also does some other small things like:

  • Add a new CommitmentIndex type.
  • Improve compute_verify_cell_kzg_proof_batch_challenge function.
  • Remove punctuation from MAX_CELLS_IN_EXTENDED_MATRIX description.
  • Rename constants section which had no constants.

@jtraglia jtraglia added the EIP-7594 PeerDAS label Jul 10, 2024
@jtraglia jtraglia self-assigned this Jul 10, 2024
@jtraglia jtraglia requested a review from asn-d6 July 10, 2024 19:59
Copy link
Contributor

@asn-d6 asn-d6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments, and then LGTM.

Copy link
Contributor

@b-wagn b-wagn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, looks good.
I left some minor comments.

@asn-d6 asn-d6 merged commit 252b852 into ethereum:dev Jul 15, 2024
26 checks passed
@jtraglia jtraglia deleted the update-index-names branch July 15, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EIP-7594 PeerDAS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants