Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions .github/workflows/ext_py.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
name: Test ext_py

on: [push, pull_request]

jobs:
test:
if: ${{ github.event_name == 'push' || github.event.pull_request.head.repo.full_name != github.repository }}
runs-on: ubuntu-latest
env:
RUST_BACKTRACE: 1
RUSTFLAGS: "-D warnings"
Comment on lines +5 to +11

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Add a least-privilege permissions: block.

The workflow inherits the repository's default GITHUB_TOKEN permissions. Scope it down explicitly, since this job only checks out and builds/tests.

🔒 Suggested change
 jobs:
   test:
     if: ${{ github.event_name == 'push' || github.event.pull_request.head.repo.full_name != github.repository }}
     runs-on: ubuntu-latest
+    permissions:
+      contents: read
     env:
       RUST_BACKTRACE: 1
       RUSTFLAGS: "-D warnings"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
jobs:
test:
if: ${{ github.event_name == 'push' || github.event.pull_request.head.repo.full_name != github.repository }}
runs-on: ubuntu-latest
env:
RUST_BACKTRACE: 1
RUSTFLAGS: "-D warnings"
jobs:
test:
if: ${{ github.event_name == 'push' || github.event.pull_request.head.repo.full_name != github.repository }}
runs-on: ubuntu-latest
permissions:
contents: read
env:
RUST_BACKTRACE: 1
RUSTFLAGS: "-D warnings"
🧰 Tools
🪛 zizmor (1.26.1)

[info] 6-6: workflow or action definition without a name (anonymous-definition): this job

(anonymous-definition)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/ext_py.yaml around lines 5 - 11, Add a top-level
permissions block for the workflow with least-privilege access, granting only
contents: read for checkout and build/test operations. Place it alongside the
workflow’s jobs definition so the test job no longer inherits broader
GITHUB_TOKEN permissions.

Source: Linters/SAST tools


steps:
- uses: actions/checkout@v5

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Disable credential persistence on checkout.

actions/checkout persists the GITHUB_TOKEN in .git/config by default. Subsequent steps (pip install, maturin build) execute third-party build scripts that could exfiltrate the persisted token. Add persist-credentials: false to eliminate this attack vector.

🔒 Proposed fix
       - uses: actions/checkout@v5
+        with:
+          persist-credentials: false
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- uses: actions/checkout@v5
- uses: actions/checkout@v5
with:
persist-credentials: false
🧰 Tools
🪛 zizmor (1.26.1)

[warning] 14-14: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false

(artipacked)


[error] 14-14: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/ext_py.yaml at line 14, Add persist-credentials: false to
the actions/checkout@v5 step in the workflow, ensuring the checkout action does
not store the GITHUB_TOKEN in Git configuration before subsequent build and
installation steps run.


- name: Set up Python 3.14
uses: actions/setup-python@v6
with:
python-version: "3.14"

- name: Install rustup
id: rustup
uses: dtolnay/rust-toolchain@v1
with:
toolchain: stable

- name: Cache files
uses: actions/cache@v5
with:
path: |
~/.cargo
/usr/share/rust/.cargo
**/target
key: ext-py-${{ steps.rustup.outputs.rustc_hash }}-${{ hashFiles('**/Cargo.*') }}

- name: Build wheel
run: cd ext_py && pip install --group dev && maturin build --out dist

- name: Install wheel and run tests
run: |
cd ext_py
pip install --force-reinstall dist/*.whl
pytest tests
139 changes: 139 additions & 0 deletions ext/crates/fp/src/field/dyn_field.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
use std::{
fmt,
ops::{Add, Mul, Neg, Sub},
};

use super::{Fp, SmallFq, element::FieldElement};
use crate::prime::ValidPrime;

/// A field element whose field type has been erased to one of the two concrete kinds the public
/// API exposes: a prime field [`Fp`] or a small extension field [`SmallFq`], each over a dynamic
/// [`ValidPrime`].
///
/// This lets callers that cannot be generic over the [`Field`](super::Field) trait — most notably
/// the Python bindings, where a `pyclass` cannot carry generic parameters — manipulate field
/// elements of either kind through a single type, with the arithmetic operators implemented once
/// here rather than re-matched at every call site.
///
/// Combining two elements that do not live in the same field (a different kind, or the same kind
/// over a different prime/degree) is a genuine failure, not a value: the binary operators report it
/// as [`None`]/[`Err`] rather than silently coercing one operand or returning a default element.
#[derive(Clone, Copy, PartialEq, Eq, Hash, Debug)]
pub enum DynFieldElement {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

So the only reason we can't just have DynField be the enum and then type DynFieldElement = FieldElement<DynField> is because we want fallible field operations when the fields are different.
That makes me think we should fix that API in the fp crate itself.

Fp(FieldElement<Fp<ValidPrime>>),
SmallFq(FieldElement<SmallFq<ValidPrime>>),
}

/// Why [`DynFieldElement::try_div`] could not divide.
///
/// Division has two distinct failure modes that callers may want to surface differently (the Python
/// bindings map them to `ValueError` and `ZeroDivisionError` respectively), so unlike the additive
/// operators this is reported with a dedicated enum rather than a bare [`Option`].
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum DivError {
/// The two operands do not live in the same field.
MismatchedField,
/// The divisor is the zero element.
DivisionByZero,
}

impl fmt::Display for DivError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Self::MismatchedField => f.write_str("cannot combine elements from different fields"),
Self::DivisionByZero => f.write_str("division by zero"),
}
}
}

impl std::error::Error for DivError {}

/// Implement an additive/multiplicative operator once, returning `None` when the operands are not
/// in the same field. The field-equality guard is necessary: [`FieldElement`]'s own operators use
/// the left operand's field unconditionally, so without it a mismatch would silently compute a
/// bogus result instead of signalling failure.
macro_rules! impl_binop {
($trait:ident, $method:ident) => {
impl $trait for DynFieldElement {
type Output = Option<Self>;

fn $method(self, rhs: Self) -> Self::Output {
match (self, rhs) {
(Self::Fp(a), Self::Fp(b)) if a.field() == b.field() => {
Some(Self::Fp(a.$method(b)))
}
(Self::SmallFq(a), Self::SmallFq(b)) if a.field() == b.field() => {
Some(Self::SmallFq(a.$method(b)))
}
_ => None,
}
}
}
};
}

impl_binop!(Add, add);
impl_binop!(Sub, sub);
impl_binop!(Mul, mul);

impl Neg for DynFieldElement {
type Output = Self;

fn neg(self) -> Self::Output {
match self {
Self::Fp(x) => Self::Fp(-x),
Self::SmallFq(x) => Self::SmallFq(-x),
}
}
}

impl DynFieldElement {
/// Divide, distinguishing a mismatched-field failure from division by zero. Returns
/// [`DivError::MismatchedField`] when the operands are not in the same field and
/// [`DivError::DivisionByZero`] when `rhs` is the zero element.
pub fn try_div(self, rhs: Self) -> Result<Self, DivError> {
match (self, rhs) {
(Self::Fp(a), Self::Fp(b)) if a.field() == b.field() => {
(a / b).map(Self::Fp).ok_or(DivError::DivisionByZero)
}
(Self::SmallFq(a), Self::SmallFq(b)) if a.field() == b.field() => {
(a / b).map(Self::SmallFq).ok_or(DivError::DivisionByZero)
}
_ => Err(DivError::MismatchedField),
}
}

/// The multiplicative inverse, or [`None`] for the zero element.
pub fn inv(self) -> Option<Self> {
match self {
Self::Fp(x) => x.inv().map(Self::Fp),
Self::SmallFq(x) => x.inv().map(Self::SmallFq),
}
}

/// The Frobenius endomorphism `x -> x^p`.
pub fn frobenius(self) -> Self {
match self {
Self::Fp(x) => Self::Fp(x.frobenius()),
Self::SmallFq(x) => Self::SmallFq(x.frobenius()),
}
}

/// The canonical `u32` value of a prime-field element, or [`None`] for a [`SmallFq`] element,
/// which has no canonical integer representative.
pub fn try_as_u32(self) -> Option<u32> {
match self {
Self::Fp(x) => Some(*x),
Self::SmallFq(_) => None,
}
}
}

impl fmt::Display for DynFieldElement {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Self::Fp(x) => write!(f, "{x}"),
Self::SmallFq(x) => write!(f, "{x}"),
}
}
}
2 changes: 2 additions & 0 deletions ext/crates/fp/src/field/mod.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
use self::field_internal::FieldInternal;
use crate::prime::Prime;

pub mod dyn_field;
pub mod element;
pub(crate) mod field_internal;

pub mod fp;
pub mod smallfq;

pub use dyn_field::{DivError, DynFieldElement};
use element::FieldElement;
pub use fp::Fp;
pub use smallfq::SmallFq;
Expand Down
4 changes: 4 additions & 0 deletions ext/crates/fp/src/matrix/matrix_inner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1558,6 +1558,10 @@ impl<'a> MatrixSliceMut<'a> {
l.add_masked(r, 1, mask);
}
}

pub fn to_vec(&self) -> Vec<Vec<u32>> {
self.iter().map(|row| row.iter().collect()).collect()
}
}

#[cfg(test)]
Expand Down
2 changes: 2 additions & 0 deletions ext_py/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
__pycache__
*.so
Comment on lines +1 to +2

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add target/ and dist/ to ignore build artifacts.

ext_py/Cargo.toml will cause cargo build/maturin build to create ext_py/target/, and maturin build --out dist creates ext_py/dist/. Neither is ignored, risking accidental commits of large, platform-specific artifacts.

♻️ Proposed addition
 __pycache__
 *.so
+target/
+dist/
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
__pycache__
*.so
__pycache__
*.so
target/
dist/
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ext_py/.gitignore` around lines 1 - 2, Update the ext_py ignore rules to
include target/ and dist/ alongside the existing Python cache and shared-library
patterns, preventing Cargo and maturin build artifacts from being committed.

19 changes: 19 additions & 0 deletions ext_py/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
[package]
name = "ext_py"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
[lib]
name = "ext_py"
crate-type = ["cdylib"]

[dependencies]
anyhow = "1.0"
fp = { path = "../ext/crates/fp" }

pyo3 = "0.29.0"
serde_json = "1.0.141"

[dev-dependencies]
tempfile = "3.20.0"
24 changes: 24 additions & 0 deletions ext_py/pyproject.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
[build-system]
requires = ["maturin>=1.8"]
build-backend = "maturin"

[project]
name = "sseq_ext"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is that the name we're settling on? I suppose it fits.

requires-python = ">=3.10"
classifiers = [
"Programming Language :: Rust",
"Programming Language :: Python :: Implementation :: CPython",
"Programming Language :: Python :: Implementation :: PyPy",
]
dynamic = ["version"]

[tool.maturin]
features = ["pyo3/extension-module"]
module-name = "ext"
python-source = "python"

[dependency-groups]
dev = [
"maturin>=1.8",
"pytest",
]
1 change: 1 addition & 0 deletions ext_py/python/ext/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
from .ext import *
Loading