-
Notifications
You must be signed in to change notification settings - Fork 2
Use fromDerSignature function from @turnkey/crypto #102
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
Use fromDerSignature function from @turnkey/crypto #102
Conversation
…not checked to be 0x30, No checks for canonical (minimal) representation of DER-encoded numbers, Bounds for array slicings are not checked
…tion between repos
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
export-and-sign/package.json
Outdated
| "@noble/ed25519": "2.0.0", | ||
| "@noble/hashes": "1.3.2", | ||
| "@solana/web3.js": "1.98.4", | ||
| "@turnkey/crypto": "^2.8.6", |
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.
tiny nit: let's hard-pin this dependency just to keep things consistent/locked down
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.
Done. Is this a normal approach here at Turnkey?
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.
It sorta depends. In security-critical packages or here in frames, we would prefer to, just to be really on top of the dependencies we're using. Otherwise, in less-security-critical places like example apps, it's not as critical
Using export function
fromDerSignaturefrom@turnkey/cryptoto standardize this function between repos. They are both supposed to have the same functionality anyways. Since this function specifically looks for short form, we don't need to support long form length:https://github.com/tkhq/sdk/blob/3406928772254aeb4f47259e43d8618a677dd0a2/packages/crypto/src/crypto.ts#L671-L672