ts-sdk: add secp256k1 keypair #4410
Conversation
joyqvq
force-pushed
the
wallet-secp
branch
7 times, most recently
from
514f882
to
d769ff9
Compare
| @@ -0,0 +1,101 @@ | |||
| // Copyright (c) 2022, Mysten Labs, Inc. | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
There was a problem hiding this comment.
this is a rename from PublicKey.ts
| @@ -1,136 +1,82 @@ | |||
| // Copyright (c) 2022, Mysten Labs, Inc. | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
There was a problem hiding this comment.
this file is repurposed to an interface PublicKey implemented by Ed25519PublicKey and Secp256k1PublicKey
| import { sha3_256 } from 'js-sha3'; | ||
| import { checkPublicKeyData, PublicKey, PublicKeyInitData, SIGNATURE_SCHEME_TO_FLAG, toHexString } from './publickey'; | ||
|
|
||
| const SECP256K1_PUBLIC_KEY_SIZE = 33; |
There was a problem hiding this comment.
note that the pubkey size is different for secp256k1
| ED25519: 0x00, | ||
| Secp256k1: 0x01, | ||
| }; | ||
|
|
||
| function isPublicKeyData(value: PublicKeyInitData): value is PublicKeyData { | ||
| export function checkPublicKeyData(value: PublicKeyInitData): value is PublicKeyData { |
There was a problem hiding this comment.
this is renamed due to name collision with index.guard.ts for another isPublicKeyData
joyqvq
force-pushed
the
wallet-secp
branch
4 times, most recently
from
537065b
to
c51c3e7
Compare
sdk/typescript/README.md
Outdated
| @@ -174,3 +182,17 @@ const publishTxn = await signer.publish( | |||
| ); | |||
| console.log('publishTxn', publishTxn); | |||
| ``` | |||
|
|
|||
|
|
|||
| Alternatively, a Secp256k1 can be ininiated: | |||
| const PUBLIC_KEY_SIZE = 32; | ||
|
|
||
| /** | ||
| * A Ed25519 public key |
| // Copyright (c) 2022, Mysten Labs, Inc. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| import * as secp from "@noble/secp256k1"; |
There was a problem hiding this comment.
How did we choose that particular dep? compared to other available libs? (maturity? speed? maintenance, popularity?)
There was a problem hiding this comment.
https://www.npmjs.com/package/@noble/secp256k1
- this is a top 10 library for secp256k1
- the benchmark marks itself to be the fastest, esp signing
- I liked this one the most bc 0 dep
- the latest version is less than 6 months old
here are some other ones, https://www.npmjs.com/search?ranking=optimal&q=secp256k1
another option is https://www.npmjs.com/package/secp256k1 a native C binding to the underlying library (this may be nicer since our rust library also wraps to the same C library)
| * Return the signature for the provided data. | ||
| */ | ||
| signData(data: Base64DataBuffer): Base64DataBuffer { | ||
| const msgHash = sha256(data.getData()); |
There was a problem hiding this comment.
do we use sha256 or sha3?
There was a problem hiding this comment.
in rust, we use sha256 (adding some more test cases against wycheproof)
let message = Message::from_hashed_data::<rust_secp256k1::hashes::sha256::Hash>(msg);
| signData(data: Base64DataBuffer): Base64DataBuffer { | ||
| const msgHash = sha256(data.getData()); | ||
| return new Base64DataBuffer( | ||
| secp.signSync(msgHash, this.keypair.secretKey) |
There was a problem hiding this comment.
Do we know if this produces deterministic signatures? If not we should ensure it is aligned with our Rust impl (+ test against test vectors)
There was a problem hiding this comment.
yup! the underlying library is tested against wycheproof, would be porting them over in a follow up PR, I would like to do that for ed25519 as well.
There was a problem hiding this comment.
is this deterministic ECDSA implementation per rfc6979?
There was a problem hiding this comment.
joyqvq
requested review from
Jordan-Mysten and
kchalkias
and removed request for
Clay-Mysten
There was a problem hiding this comment.
Approving, but please get the green light from @666lcz or @pchrysochoidis as well to ensure no breaking change elsewhere (ie wallet)
to follow up: