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

feat: add fnv alg to crypto #2200

Merged
merged 10 commits into from
Jun 1, 2022

Conversation

luk3skyw4lker
Copy link
Contributor

Add implementation of fnv32, fnv32a, fnv64, fnv64a to crypto module. Addressing #2122

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

@luk3skyw4lker Thank you for looking into this! Left a few comments.

crypto/test.ts Outdated Show resolved Hide resolved
crypto/mod.ts Outdated Show resolved Hide resolved
@luk3skyw4lker
Copy link
Contributor Author

@kt3k thanks for the review!

@luk3skyw4lker
Copy link
Contributor Author

@kt3k I think I have solved the issues. I wasn't able to keep the return of the fnv functions as ArrayBuffers because something was happening when I tried to convert the hashes into Uint32Arrays and the result was reversed (I think it may be because of MSB and LSB problems, I didn't manage to find information or fix for that) so I just convert the string to hex inside the function itself and the result comes as a hex string (and with the right ordenation).

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

Thanks for updating! The resulted values look correct. Nice progress, but left a few comments on some points.

crypto/mod.ts Outdated Show resolved Hide resolved
crypto/mod.ts Outdated Show resolved Hide resolved
crypto/mod.ts Outdated Show resolved Hide resolved
@luk3skyw4lker
Copy link
Contributor Author

@kt3k I think that everything is right now! I had a lot of trouble to make the returns come as ArrayBuffers but I did it

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

@luk3skyw4lker LGTM. Thanks!

@kt3k kt3k merged commit c5589ef into denoland:main Jun 1, 2022
@luk3skyw4lker luk3skyw4lker deleted the feat/add_fnv_alg_to_crypto branch June 2, 2022 00:17
@kt3k kt3k mentioned this pull request Jun 9, 2022
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.

None yet

2 participants