Skip to content

refactor: extract DomainConstants struct#573

Merged
MauroToscano merged 3 commits into
mainfrom
refactor/domain-constants
Apr 30, 2026
Merged

refactor: extract DomainConstants struct#573
MauroToscano merged 3 commits into
mainfrom
refactor/domain-constants

Conversation

@MauroToscano
Copy link
Copy Markdown
Contributor

Summary

  • Introduces DomainConstants<F> in domain.rs — groups the four precomputed barycentric interpolation values (points, offset_pow_n, size_inv, offset_pow_n_inv) with a from_domain constructor
  • Reduces get_trace_evaluations_from_lde from 10 parameters to 6, removing the #[allow(clippy::too_many_arguments)] suppression
  • Replaces the 6-line inline computation block in round_3 (prover.rs) with DomainConstants::from_domain(domain)
  • Simplifies the test from 8 lines of manual precomputation to one constructor call

Test plan

  • All 124 stark lib tests pass (cargo test -p stark --lib)
  • barycentric_trace_eval_matches_horner_trace_eval — verifies the refactored path matches Horner evaluation
  • test_deep_poly_direct_2n_matches_interpolate_fft_extend — verifies DEEP poly direct 2N path

Pure refactor, no behavioral changes.

Group the four precomputed barycentric interpolation values (coset
points, offset^N, 1/N, 1/offset^N) into a DomainConstants struct with
a from_domain constructor. This reduces get_trace_evaluations_from_lde
from 10 parameters to 6 and removes the clippy::too_many_arguments
suppression.
Comment thread crypto/stark/src/trace.rs
Comment thread crypto/stark/src/domain.rs
@claude
Copy link
Copy Markdown

claude Bot commented Apr 30, 2026

Clean refactor. DomainConstants is a natural home for these four values, the removal of #[allow(clippy::too_many_arguments)] is a good signal it was the right call, and the test simplification validates the design.

Two minor issues flagged inline:

  1. trace.rs:397 — local variable n_inv_g_n_inv is a stale name leftover from before the rename; the surrounding comment already uses the new size_inv/offset_pow_n_inv names.
  2. domain.rs:34 — the expect message says "hence non-zero in the field" which is only true when char(F) does not divide n; the message should reflect the actual assumption rather than assert a consequence.

No security or correctness issues. The debug_assert_eq! on dc.points.len() is still present and sufficient for catching domain/constants mismatches in debug builds.

@github-actions
Copy link
Copy Markdown

Codex Code Review

No issues found in the PR diff.

I also attempted a targeted cargo test -p stark ..., but the environment blocked rustup from creating a temp file under /home/runner/.rustup due a read-only filesystem, so I could not verify tests locally. git diff --check passed.

The previous message ("hence non-zero in the field") overstated the
guarantee — it only holds when char(F) does not divide n. Reword to
state the actual precondition.
@MauroToscano MauroToscano enabled auto-merge April 30, 2026 19:27
@MauroToscano MauroToscano disabled auto-merge April 30, 2026 19:27
@MauroToscano MauroToscano added this pull request to the merge queue Apr 30, 2026
Merged via the queue into main with commit e2c4107 Apr 30, 2026
11 checks passed
@MauroToscano MauroToscano deleted the refactor/domain-constants branch April 30, 2026 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants