Conversation
|
Warning MetaMask internal reviewing guidelines:
Ignoring alerts on:
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…-keys, share-transfer, seed-phrase from BN/elliptic to bigint/noble
|
@cursor can you do a review? (don't commit anything) |
|
Here is my code review of the Code Review:
|
ieow
left a comment
There was a problem hiding this comment.
lgtm
But it would be great to add backward compability tests
matthiasgeihs
left a comment
There was a problem hiding this comment.
I've skimmed over it and looks good at first glance.
For a thorough review I'd probably need half a day to a day which is hard for me to allocate at the moment, and not sure if it's needed. I let the Cursor bot double check the PR. It found some potential issues, which I think are worth looking into. Nothing major though, I believe. (1. should be easy to fix, 2. is a false positive, I believe.)
=> fix in commit 411e983 |
=> invalid, v2 support these options |
fix in 44252f7 |
|
|
||
| const msgHash = hexToBytes(stripHexPrefix(keccak256(utf8ToBytes(stringify(data, { replacer: bigIntReplacer }))))); | ||
| if (!secp256k1.verify(hexToBytes(sig), msgHash, m.pubKey.toSEC1(true), { prehash: false, format: "der" })) { | ||
| if (!secp256k1.verify(hexToBytes(sig), msgHash, m.pubKey.toSEC1(true), { prehash: false, format: "der", lowS: false })) { |
There was a problem hiding this comment.
will this cause issue for the new AuthMetdata that genereate signature with lowS:true?
Maybe we can add as the fallback for signature verification
using lowS: true and low:S false
There was a problem hiding this comment.
seems like verify lowS:false can verify signature with lowS:true
we could add comment on the verification mentioning lowS:true is to support backward compability
|
packages/default/test/run.test.js still imports and uses BN from bn.js (lines 1, 6, 7) |
|
I think only the first point need to confirmed ( check ) |
|
@SocketSecurity ignore npm/workerpool@9.3.4 |


Jira Link
https://consensyssoftware.atlassian.net/browse/W3APD-5328
Description
Migrate the entire tkey SDK from legacy cryptography libraries (
bn.js,elliptic,Buffer,@toruslabs/tweetnacl-js) to modern alternatives (bigint,@noble/curves,Uint8Array).Key changes:
bn.js(BN) with nativebigintacross all packages (common-types, core, service-providers, modules)ellipticsecp256k1 operations with@noble/curves/secp256k1@toruslabs/tweetnacl-jsED25519 operations with@noble/curves/ed25519Bufferusage withUint8ArraythroughoutbigIntReplacerutility for JSON serialization ofbigintvalues (sinceJSON.stringifycannot serializebigintnatively)ThresholdKey.fromJSON()deserialization to properly reconstructShareStore,AuthMetadata, andPublicShareinstances from JSON@tkey/tsspackage (to be migrated separately)Packages affected: common-types, core, default, private-keys, security-questions, seed-phrase, service-provider-base, service-provider-torus, service-provider-sfa, share-serialization, share-transfer, storage-layer-torus, chrome-storage, web-storage
How has this been tested?
bigint/@noble/curvesAPIsnpm run buildpasses for all 14 packagesnpm run test -w packages/common-types— all passingnpm run test -w packages/core— all passingnpm run test -w packages/share-serialization— all passingnpm run test -w packages/default— 264 passing, 0 failingScreenshots (if appropriate)
N/A — no UI changes
Types of changes
Checklist