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

argon2: const ParamsBuilder #436

Closed
C0D3-M4513R opened this issue Jul 3, 2023 · 5 comments · Fixed by #438
Closed

argon2: const ParamsBuilder #436

C0D3-M4513R opened this issue Jul 3, 2023 · 5 comments · Fixed by #438

Comments

@C0D3-M4513R
Copy link
Contributor

Can the ParamsBuilder methods be made const?
Const fn's would help out, because then I could make the Params object a Compile time const.
I took a little look at the code and saw no obvious reason why it should not be possible.

const PARAMS:argon2::Params = match argon2::ParamsBuilder::new().build() {
    Ok(v) => v,
    Err(_) => panic!("Failed to create argon2 params"),
};

https://github.com/RustCrypto/password-hashes/blob/master/argon2/src/params.rs#L367

@tarcieri
Copy link
Member

tarcieri commented Jul 3, 2023

It seems like new and build are the only methods of ParamsBuilder that could be made const fn as the setters require &mut self. That seems like a circuitous way of returning the default Params.

If that's all you want, we could hoist out the Default impl into a proper constant: https://github.com/RustCrypto/password-hashes/blob/c29e6d7/argon2/src/params.rs#L189-L196

@C0D3-M4513R
Copy link
Contributor Author

C0D3-M4513R commented Jul 3, 2023

I‘d also be interested in having the settees const. I‘ll try to make a pr with your message in mind.
Edit: okay, I didn't know that &mut T was not allowed in const fn's.

@tarcieri
Copy link
Member

tarcieri commented Jul 3, 2023

Here's a simpler approach to a Params::DEFAULT constant: #439

@C0D3-M4513R
Copy link
Contributor Author

"completed"...
I will see to redoing my pr at a later point in time.
I don't have the time rn.

@tarcieri
Copy link
Member

tarcieri commented Aug 7, 2023

The PR auto-closed it as completed when it was merged, then the tests failed on master

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 a pull request may close this issue.

2 participants