Add n_columns() to PreProcessedTraceVariant#1709
Add n_columns() to PreProcessedTraceVariant#1709ilyalesokhin-starkware wants to merge 1 commit intoilya/pptfrom
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| PreProcessedTraceVariant::CanonicalWithoutPedersen => 105, | ||
| PreProcessedTraceVariant::CanonicalSmall => 156, | ||
| } | ||
| } |
There was a problem hiding this comment.
Hardcoded magic numbers for column counts are fragile
Low Severity
n_columns() uses inline magic numbers (161, 105, 156) with no named constants or derivation from existing definitions, unlike n_trace_cells() which references named constants (CANONICAL_SIZE, etc.) defined in the common crate alongside the column definitions. If column definitions change (e.g., adding a new range check or changing PEDERSEN_TABLE_N_COLUMNS), these hardcoded values silently become wrong until the test happens to run. Extracting these into named constants co-located with the column definitions would make the relationship explicit and match the existing pattern.
There was a problem hiding this comment.
that would be the only places where those constants are used.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
83ca059 to
e8b6141
Compare
9d83ed5 to
945d07f
Compare
Gali-StarkWare
left a comment
There was a problem hiding this comment.
@Gali-StarkWare reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on ilyalesokhin-starkware).
stwo_cairo_prover/crates/common/src/preprocessed_columns/preprocessed_trace.rs line 42 at r2 (raw file):
} pub fn n_columns(&self) -> usize {
What will you do with the ids? You can just move the list here..
Gali-StarkWare
left a comment
There was a problem hiding this comment.
@Gali-StarkWare reviewed 1 file.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ilyalesokhin-starkware).
|
Previously, Gali-StarkWare wrote…
I don't need the IDs when I'm asking about the number of columns. |
| PreProcessedTraceVariant::CanonicalWithoutPedersen => 105, | ||
| PreProcessedTraceVariant::CanonicalSmall => 156, | ||
| } | ||
| } |
There was a problem hiding this comment.
that would be the only places where those constants are used.
ilyalesokhin-starkware
left a comment
There was a problem hiding this comment.
@ilyalesokhin-starkware resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Gali-StarkWare).
|
Previously, ilyalesokhin-starkware wrote…
What list needs to move here? |
Gali-StarkWare
left a comment
There was a problem hiding this comment.
@Gali-StarkWare made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ilyalesokhin-starkware).
stwo_cairo_prover/crates/common/src/preprocessed_columns/preprocessed_trace.rs line 42 at r2 (raw file):
Previously, ilyalesokhin-starkware wrote…
What list needs to move here?
The one you have in stwo-circuits. You don't need it for the len but you need it for CairoStatement::get_preprocessed_column_ids()
|
Previously, Gali-StarkWare wrote…
I thought I'd get the IDs by calling to_preprocessed_trace(). |



Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Note
Low Risk
Low risk: adds a small helper method and a unit test; no proving/verification logic or data handling is changed.
Overview
Adds
PreProcessedTraceVariant::n_columns()to expose the expected number of preprocessed columns for each trace variant.Adds a unit test (
test_n_columns) that assertsvariant.to_preprocessed_trace().columns.len()matchesvariant.n_columns()for all variants, keeping the hardcoded metadata in sync with trace construction.Written by Cursor Bugbot for commit e8b6141. This will update automatically on new commits. Configure here.
This change is