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

Remove unused Argon2dHash and Argon2dHasher #2444

Merged
merged 3 commits into from
May 22, 2024
Merged

Conversation

hrxi
Copy link
Contributor

@hrxi hrxi commented May 8, 2024

They were only exposed to the web-client, but I couldn't find any reference to this function.

The Argon2d/Argon2id implementation in nimiq_hash::argon2kdf remain.

Fixes #2455 by removing the unused code.

@hrxi hrxi requested a review from sisou May 8, 2024 13:16
@hrxi
Copy link
Contributor Author

hrxi commented May 8, 2024

@sisou Am I correct that this function is also not used in the web client? I looked at https://github.com/nimiq/wallet, in the albatross-testnet branch.

@sisou
Copy link
Member

sisou commented May 8, 2024

I exposed it for backward-compatibility with the PoW client, which also had that method. It's not used in our apps, but doesn't mean someone else doesn't use it. But it's probably a low-usage API.
I think it's fine to remove as a breaking change with the Albatross client.

@sisou
Copy link
Member

sisou commented May 8, 2024

Can you maybe expose an Argon2id method on the web-client API as a replacement?

@hrxi
Copy link
Contributor Author

hrxi commented May 14, 2024

Can you maybe expose an Argon2id method on the web-client API as a replacement?

Done.

Copy link
Member

@sisou sisou left a comment

Choose a reason for hiding this comment

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

Since these are KDF functions, I'd name them as such.

Also, are you able to add rustdocs to the method, explaining their arguments? I wouldn't know what a derived_key_length is for example.

web-client/src/primitives/hash.rs Show resolved Hide resolved
web-client/src/primitives/hash.rs Show resolved Hide resolved
@hrxi
Copy link
Contributor Author

hrxi commented May 15, 2024

Since these are KDF functions, I'd name them as such.

Argon2 is always a key derivation function, a special kind of hash function. I'm not explicitly mentioning that sha256 is a cryptographic hash function, so intuitively I wouldn't do it for Argon2 either.

Also, are you able to add rustdocs to the method, explaining their arguments? I wouldn't know what a derived_key_length is for example.

I don't know it yet either, but I'll try to find out and document it.

@hrxi hrxi force-pushed the hrxi/rm_argon2d branch 3 times, most recently from ac5e018 to 634d07a Compare May 15, 2024 13:41
@styppo styppo added this to the Nimiq PoS Mainnet milestone May 20, 2024
hrxi added 3 commits May 22, 2024 13:52
They were only exposed to the web-client, but I couldn't find any
reference to this function.

The Argon2d/Argon2id implementation in `nimiq_hash::argon2kdf` remain.

Fixes #2455 by removing the unused code.
Copy link
Contributor

@jsdanielh jsdanielh left a comment

Choose a reason for hiding this comment

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

LGTM

@jsdanielh jsdanielh merged commit beb510a into albatross May 22, 2024
6 checks passed
@jsdanielh jsdanielh deleted the hrxi/rm_argon2d branch May 22, 2024 13:05
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.

Argon2dHasher uses a predictable salt
4 participants