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

Implement sha512-256 and sha512-224 #67

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ChALkeR
Copy link

@ChALkeR ChALkeR commented May 31, 2020

Basically, the same relationship as between sha256 and sha224 -- I copied that approach.

Simple tests included that it matches Node.js impl, but there are no other test vectors due to test organization (vectors are in a separate package).

Older don't have sha512-256 and sha512-224, but they should get tested.
@ChALkeR
Copy link
Author

ChALkeR commented Sep 14, 2020

cc @dcousens

@andrewtoth
Copy link

ping @dcousens

@dcousens
Copy link
Member

LGTM, maybe @ForbesLindesay can merge it for you

@PatNeedham
Copy link

@ForbesLindesay does this PR also look good to you?

@PatNeedham
Copy link

@jprichardson @calvinmetcalf @fanatid any chance this PR has a viable path towards being merged? Or is it as dead as a doornail.

@PatNeedham
Copy link

.

Copy link
Contributor

@fanatid fanatid left a comment

Choose a reason for hiding this comment

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

LGTM except for 1 comment about the test. I can merge it but only @calvinmetcalf can make npm release.

tape("hash is the same as node's crypto for all algos provided by node", function (t) {
var hashes = crypto.getHashes()
Object.keys(sha).forEach(function (alg) {
if (hashes.indexOf(alg) === -1) return // skip unsupported by current Node.js version
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really test newly added sha512-224 / sha512-256?

Copy link
Author

@ChALkeR ChALkeR Feb 1, 2022

Choose a reason for hiding this comment

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

@fanatid #73 should fail (introduces 6be33ae)

But it looks like CI is disabled for some reason?

taimanhui added a commit to taimanhui/blockchain-libs that referenced this pull request Mar 2, 2022
As crypto-browserify doesn't support sha512/256 yet.
See browserify/sha.js#67
taimanhui added a commit to taimanhui/blockchain-libs that referenced this pull request Mar 2, 2022
As crypto-browserify doesn't support sha512/256 yet.
See browserify/sha.js#67
taimanhui added a commit to taimanhui/blockchain-libs that referenced this pull request Mar 2, 2022
As crypto-browserify doesn't support sha512/256 yet.
See browserify/sha.js#67
taimanhui added a commit to taimanhui/blockchain-libs that referenced this pull request Mar 2, 2022
As crypto-browserify doesn't support sha512/256 yet.
See browserify/sha.js#67
taimanhui added a commit to taimanhui/blockchain-libs that referenced this pull request Mar 2, 2022
As crypto-browserify doesn't support sha512/256 yet.
See browserify/sha.js#67
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.

5 participants