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

crypto: add fnv1a hash #2122

Closed
littledivy opened this issue Apr 19, 2022 · 11 comments
Closed

crypto: add fnv1a hash #2122

littledivy opened this issue Apr 19, 2022 · 11 comments
Labels
enhancement New feature or request

Comments

@littledivy
Copy link
Member

Like the one in std/hash https://github.com/denoland/deno_std/blob/main/hash/_fnv

@littledivy littledivy added the enhancement New feature or request label Apr 19, 2022
@luk3skyw4lker
Copy link
Contributor

I can take this one. I'll take a look at the algorithm today and create an implementation.

@luk3skyw4lker
Copy link
Contributor

Why the need for them to be at the crypto module? Is it because it's internal? I didn't fully understand the need for it.

@KyleJune
Copy link
Contributor

KyleJune commented Apr 29, 2022

It's because hash module is deprecated (see top of README.md). They are going to delete the whole hash folder. Instead of deleting fnv that can't be replaced with WebCrypto and std/crypto, I think they are just going to just move it to std/crypto folder instead of keeping this folder with just fnv algorithms in it.

@luk3skyw4lker
Copy link
Contributor

Dang! That totally slipped through me, I'm sorry!

I'll do the work of implementing the algorithms then, thanks for the heads up!

@KyleJune
Copy link
Contributor

I think all the other ones in that directory can be done by WebCrypto or std/crypto. fnv is a non cryptographic hash, which is why it currently doesn't have a replacement. I'm not sure about internal discussions at Deno on this task, but I think the fnv implementation in hash could just be moved to std/crypto instead of re-implementing it.

@luk3skyw4lker
Copy link
Contributor

I'm checking with the maintainers about the internals, thanks for your tips!

@littledivy
Copy link
Member Author

Yup std/hash is deprecated so we need this in std/crypto. You can simply copy over the algorithm from std/hash/_fnv to expose this API:

import { crypto } from "https://deno.land/std/crypto/mod.ts";

// 32bit
await crypto.subtle.digest("fnv32", new TextEncoder().encode("deno")); // 0x6e, 0xd5, 0xa7, 0xa9
await crypto.subtle.digest("fnv32a", new TextEncoder().encode("deno")); // 0x8e, 0xf6, 0x47, 0x11

// 64bit
await crypto.subtle.digest("fnv64", new TextEncoder().encode("deno")); // 0x14, 0xed, 0xb2, 0x7e, 0xec, 0xda, 0xad, 0xc9
await crypto.subtle.digest("fnv64a", new TextEncoder().encode("deno")); // 0xa5, 0xd9, 0xfb, 0x67, 0x42, 0x6e, 0x48, 0xb1

The streaming implementation can also be simplified to a simple function, like the one used in std/http/file_server.ts https://github.com/denoland/deno_std/blob/main/http/file_server.ts#L183

@luk3skyw4lker
Copy link
Contributor

Yeah, I was thinking about placing a function just like the one from std/http/file_server.ts at the mod.ts file of the crypto module. But then I took a deeper look at the module implementation and saw that most of the algorithms are placed in the _wasm_crypto module which is in Rust.

In that case, should I do the Typescript implementation or follow the pattern and implement them in Rust? (I'm almost sure that the Rust implementation should be the option)

@littledivy
Copy link
Member Author

In that case, should I do the Typescript implementation or follow the pattern and implement them in Rust?

Typescript. The algorithm is fairly trivial and possibly more performant than WASM (calling into WASM each iteration is not cheap)

@luk3skyw4lker
Copy link
Contributor

Right! Thanks for the answers, I'll try to make a PR today.

@kt3k
Copy link
Member

kt3k commented Jun 9, 2022

done in #2200

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants