Skip to content

Refactored MLDSAParams to remove the hideous turbofish. - #117

Closed
ounsworth wants to merge 15 commits into
bcgit:release/0.1.3alphafrom
ounsworth:refactor/algparams_traits
Closed

ounsworth wants to merge 15 commits into
bcgit:release/0.1.3alphafrom
ounsworth:refactor/algparams_traits

Conversation

@ounsworth

@ounsworth ounsworth commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

Solution: #105 introduces a really nice pattern for encapsulating AES params, and I want to also apply this to the ML-DSA and ML-KEM crates.
The pattern is to hide all the params in a pub trait, with a private trait that seals it.

The entrypoint to start the review is: the new trait MLDSAParams in params.rs.

This allows for greatly simplifying the previous turbofish construction, which was getting quite out of hand, brittle, and easy to make a mistake.

It used to be:

pub type MLDSA44 = MLDSA<
    MLDSA44_PK_LEN,
    MLDSA44_SK_LEN,
    MLDSA44_SIG_LEN,
    MLDSA44PublicKey,
    MLDSA44PrivateKey,
    MLDSA44_TAU,
    MLDSA44_LAMBDA,
    MLDSA44_GAMMA1,
    MLDSA44_GAMMA2,
    MLDSA44_k,
    MLDSA44_l,
    MLDSA44_ETA,
    MLDSA44_BETA,
    MLDSA44_OMEGA,
    MLDSA44_C_TILDE,
    MLDSA44_POLY_Z_PACKED_LEN,
    MLDSA44_POLY_W1_PACKED_LEN,
    MLDSA44_LAMBDA_over_4,
    MLDSA44_GAMMA1_MINUS_BETA,
    MLDSA44_GAMMA2_MINUS_BETA,
    MLDSA44_GAMMA1_MASK_LEN,
>;

And it's now:

pub type MLDSA44 = MLDSA<
    MLDSA44Params,
    MLDSA44PublicKey,
    MLDSA44PrivateKey,
    MLDSA44_PK_LEN,
    MLDSA44_SK_LEN,
    MLDSA44_SIG_LEN,
>;

Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com

@ounsworth
ounsworth force-pushed the refactor/algparams_traits branch 3 times, most recently from 93b16cd to 3ab3bb0 Compare September 7, 2026 01:16
@ounsworth
ounsworth force-pushed the refactor/algparams_traits branch from 3ab3bb0 to 79cc258 Compare September 7, 2026 01:56

@dghgit dghgit left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code passes sanity check, nice reduction, some reduction in public API as well.

Fable review was okay, I've sent you the report, there was one possible regression and a couple minor things picked up. I think they all predate this PR though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good to see. I wonder if there's some one we can tell our electronic friends to keep a watch out for patterns like this as well, it's the kind of thing that's very easy to do, even in a post-LLM world.

dghgit pushed a commit that referenced this pull request Sep 7, 2026
…rams/HashMLDSAParams/MLKEMParams traits, one impl per parameter set (#117)
@dghgit

dghgit commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Merged on release/0.1.3alpha

@dghgit dghgit closed this Sep 7, 2026
hubot pushed a commit that referenced this pull request Sep 9, 2026
…rams/HashMLDSAParams/MLKEMParams traits, one impl per parameter set (#117)
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