-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Use the SHA-256 implementation built into JavaScript engines #7361
Conversation
@@ -1,7 +1,7 @@ | |||
{ | |||
"name": "@solana/spl-type-length-value", | |||
"description": "SPL Type Length Value Library", | |||
"version": "0.1.0", | |||
"version": "0.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking change.
export async function splDiscriminate(discriminator: string, length = 8): Promise<Uint8Array> { | ||
assertDigestCapabilityIsAvailable(); | ||
const bytes = new TextEncoder().encode(discriminator); | ||
const digest = await crypto.subtle.digest('SHA-256', bytes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh. crypto
wasn't added as a global until Node 19. Node 20 is presently LTS.
Our choices are:
- Elevate the requirements to Node 19, or
- Introduce a dual-build Node/Browser system like we have in
@solana/web3.js
tosolana-program-library
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can keep it simple and elevate the requirement for this package to Node 19. Considering it'll become a dev dependency for the two packages that use it, I can't see any drawbacks. Are there any other potential negative consequences that I'm missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As part of that change, can you also update CI to only run the 'libraries' step for node v20?
solana-program-library/.github/workflows/pull-request-js.yml
Lines 43 to 61 in b1e526d
node-version: [16.x, 18.x, 20.x] | |
package: | |
[ | |
account-compression, | |
libraries, | |
memo, | |
name-service, | |
stake-pool, | |
token, | |
token-group, | |
token-metadata, | |
token-swap, | |
] | |
include: | |
# Restrict single-pool and token-lending to supported Node.js versions. | |
- package: single-pool | |
node-version: 20.5 | |
- package: token-lending | |
node-version: 18.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally.
d4f790a
to
fa04fe7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks!
This is a breaking change which removes the need for browsers to introduce a polyfill for
createHash
. Engines now have a built-inSubtleCrypto#digest
which serves this purpose, polyfill-free, with good support.The reason this is a breaking change is that
createHash
is synchronous andSubtleCrypto#digest
is async.Related to #7351.