solana-labs / solana-web3.js

Solana JavaScript SDK
https://solana-labs.github.io/solana-web3.js
MIT License
2.13k stars 853 forks source link

[Experimental]: Webcrypto Ed25519 Polyfill: should private key be able to be extractable? #1723

Closed mcintyre94 closed 11 months ago

mcintyre94 commented 12 months ago

Overview

There's a mismatch between the test description and the code behaviour and I'm not sure which is correct. We need to either correct the test description, or change the code to match the test description behaviour. https://github.com/solana-labs/solana-web3.js/blob/5e1b2e5d451f4016b1d846d84b3fa3309e43e494/packages/webcrypto-ed25519-polyfill/src/__tests__/secrets-test.ts#L70-L76

The code itself also uses the extractable value: https://github.com/solana-labs/solana-web3.js/blob/5e1b2e5d451f4016b1d846d84b3fa3309e43e494/packages/webcrypto-ed25519-polyfill/src/secrets.ts#L58-L63

While it explicitly overrides extractable for the public key: https://github.com/solana-labs/solana-web3.js/blob/5e1b2e5d451f4016b1d846d84b3fa3309e43e494/packages/webcrypto-ed25519-polyfill/src/secrets.ts#L64-L69

My guess is that we just need to fix the test description, but I figure it's worth making sure there's not a code bug here.

Also see https://github.com/solana-labs/solana-web3.js/pull/1681#discussion_r1355296887

buffalojoec commented 12 months ago

Seems like the behavior of the Subtle Crypto API is to allow either value for extractable for both, since you don't actually tell the API which is public or private when you import. I think we're probably nudging people toward marking public keys extractable since they need to be used to convert to a Base58EncodedAddress likely using this:

https://github.com/solana-labs/solana-web3.js/blob/5e1b2e5d451f4016b1d846d84b3fa3309e43e494/packages/addresses/src/public-key.ts#L5-L14

This function should fail if the key isn't marked as extractable.

mikemaccana commented 11 months ago

Also related - we may not actually need the polyfill as it's only for EoL node: https://github.com/solana-labs/solana-web3.js/issues/1680

steveluscher commented 11 months ago

…we may not actually need the polyfill as it's only for EoL node…

It's for browsers, mainly. And probably Hermes (React Native) although I haven't checked yet.

steveluscher commented 11 months ago

Honestly, I probably just got the test description wrong because I couldn't use the %s printf placeholder twice in the description string. https://jestjs.io/docs/api#1-testeachtablename-fn-timeout

github-actions[bot] commented 11 months ago

:tada: This issue has been resolved in version 1.87.2 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] commented 11 months ago

Because there has been no activity on this issue for 7 days since it was closed, it has been automatically locked. Please open a new issue if it requires a follow up.