refactor: replace asmcrypto.js with native crypto module #6550
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request upgrades various cryptographic modules by converting key derivation, hashing, encryption, and signature functions to asynchronous patterns. It adds new crypto function exports and updates type declarations. Test cases are modified to support async/await. Several package configuration files receive dependency and script updates, while webpack and Jest configurations are adjusted for module mapping. Database migration and wallet creation methods are also updated with asynchronous logic, and global references are modernized. Overall, the changes ensure that cryptographic operations and key management handle promises appropriately across multiple packages. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CryptoModule
participant RN_AES
participant SubtleCrypto
Client->>CryptoModule: Call aesCbcEncrypt({iv, key, data})
CryptoModule->>CryptoModule: Check platformEnv.isNative
alt Native
CryptoModule->>RN_AES: Call async encrypt()
RN_AES-->>CryptoModule: Return encrypted Buffer
else Non-native
CryptoModule->>SubtleCrypto: Call async crypto.subtle.encrypt()
SubtleCrypto-->>CryptoModule: Return encrypted Buffer
end
CryptoModule-->>Client: Return Promise<Buffer>
sequenceDiagram
participant Caller
participant BatchGetKeys
participant Bip32Deriver
Caller->>BatchGetKeys: Request key derivation
BatchGetKeys->>Bip32Deriver: await generateMasterKeyFromSeed()
Bip32Deriver-->>BatchGetKeys: Return master key
BatchGetKeys->>Bip32Deriver: await CKDPub for child keys
Bip32Deriver-->>BatchGetKeys: Return derived keys
BatchGetKeys-->>Caller: Return keys array
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
sidmorizon
requested review from
ezailWang,
loatheb,
originalix and
huhuanming
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (3)
packages/core/src/secret/__tests__/crypto-functions.test.ts (1)
45-90: Add error handling test cases.The encryption/decryption tests cover happy paths well. Add tests for:
- Invalid key lengths
- Corrupted ciphertext
- Mismatched IV
packages/core/src/chains/nostr/sdkNostr/index.ts (2)
67-85: Add input validation for encrypt function.Validate inputs before encryption:
- Check privateKey and pubkey lengths
- Ensure plaintext is not null/undefined
- Validate hex format of keys
87-101: Add error handling for decrypt function.Add try-catch blocks to handle:
- Invalid base64 encoding
- Missing IV parameter
- Decryption failures
export async function decrypt( privateKey: string, pubkey: string, ciphertext: string, ): Promise<string> { + try { const key = secp256k1.getSharedSecret(privateKey, `02${pubkey}`); const [cip, iv] = ciphertext.split('?iv='); + if (!iv) throw new Error('Missing IV parameter'); const normalizedKey = key.slice(1, 33); const decrypted = await aesCbcDecrypt({ data: Buffer.from(cip, 'base64'), key: Buffer.from(normalizedKey), iv: Buffer.from(iv, 'base64'), }); return Buffer.from(decrypted).toString('utf-8'); + } catch (error) { + throw new Error(`Decryption failed: ${error.message}`); + } }
♻️ Duplicate comments (1)
packages/core/src/secret/__tests__/secret.test.ts (1)
858-858: 🧹 Nitpick (assertive)Consider optimizing the async operations.
The test timeouts have been increased to 30 seconds, which suggests the async crypto operations might be taking longer than ideal. Consider using
Promise.allfor parallel execution where possible to improve performance.Also applies to: 992-992
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/mobile/ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (38)
@types/react-native-aes-crypto.d.ts(1 hunks)__mocks__/emptyMock.js(1 hunks)apps/mobile/package.json(3 hunks)development/spellCheckerSkipWords.js(2 hunks)development/webpack/webpack.base.config.js(1 hunks)jest.config.js(1 hunks)package.json(2 hunks)packages/core/package.json(0 hunks)packages/core/src/chains/btc/CoreChainSoftware.ts(1 hunks)packages/core/src/chains/btc/sdkBtc/index.ts(1 hunks)packages/core/src/chains/nexa/CoreChainSoftware.ts(1 hunks)packages/core/src/chains/nexa/sdkNexa/sdk/sign.ts(6 hunks)packages/core/src/chains/nexa/sdkNexa/utils.test.ts(1 hunks)packages/core/src/chains/nexa/sdkNexa/utils.ts(10 hunks)packages/core/src/chains/nostr/sdkNostr/index.ts(2 hunks)packages/core/src/chains/ton/sdkTon/tx.ts(1 hunks)packages/core/src/secret/__tests__/__snapshots__/secret-hash.test.ts.snap.web(0 hunks)packages/core/src/secret/__tests__/crypto-functions.test.ts(2 hunks)packages/core/src/secret/__tests__/secret-hash.test.ts(1 hunks)packages/core/src/secret/__tests__/secret.test.ts(5 hunks)packages/core/src/secret/bip32.ts(7 hunks)packages/core/src/secret/crypto-functions.ts(2 hunks)packages/core/src/secret/encryptors/aes256.ts(3 hunks)packages/core/src/secret/index.test.ts(3 hunks)packages/core/src/secret/index.ts(5 hunks)packages/kit-bg/src/dbs/local/LocalDbBase.ts(5 hunks)packages/kit-bg/src/dbs/local/consts.ts(1 hunks)packages/kit-bg/src/migrations/v4ToV5Migration/V4MigrationForAccount.ts(1 hunks)packages/kit-bg/src/services/ServiceAccount/ServiceAccount.ts(2 hunks)packages/kit-bg/src/services/ServiceAddressBook.ts(1 hunks)packages/kit-bg/src/vaults/impls/nexa/KeyringHardware.ts(2 hunks)packages/kit-bg/src/vaults/impls/nexa/Vault.ts(1 hunks)packages/kit/src/views/Setting/pages/DevUnitTests/PageDevUnitTests.tsx(7 hunks)packages/shared/src/errors/errors.test.ts(1 hunks)packages/shared/src/modules3rdParty/cross-crypto/index.js(1 hunks)packages/shared/src/modules3rdParty/cross-crypto/index.native.js(2 hunks)packages/shared/src/modules3rdParty/react-native-aes-crypto/index.native.ts(1 hunks)packages/shared/src/modules3rdParty/react-native-aes-crypto/index.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/core/package.json
- packages/core/src/secret/tests/snapshots/secret-hash.test.ts.snap.web
🧰 Additional context used
🪛 Biome (1.9.4)
packages/shared/src/modules3rdParty/cross-crypto/index.native.js
[error] 8-8: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 9-9: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/core/src/secret/index.ts
[error] 301-301: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 301-301: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 337-337: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 338-338: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
packages/core/src/secret/crypto-functions.ts
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: unittest (20.x)
- GitHub Check: lint (20.x)
🔇 Additional comments (69)
packages/kit-bg/src/services/ServiceAddressBook.ts (1)
44-44: LGTM! Good async handling.The await keyword correctly handles the promise from hash160. This matches the native crypto module's async pattern.
packages/core/src/secret/bip32.ts (9)
67-70: Good move to async.
Switching the interface methods to async is a solid upgrade. Double-check that all callers await these methods.
101-104: New async CKDPriv looks fine.
No immediate concerns. Nice work on consistency with the interface.
115-115: Verify error handling.
Check if you need error handling aroundawait hmacSHA512in case something fails or returns undefined.
131-134: Async CKDPub is consistent.
Everything aligns with the rest of the interface. Keep it up.
144-144: Repeated HMAC usage.
This line is good. Just ensure your HMAC usage remains uniform across files.
168-169: Async master key generation.
Nice shift to async. Be sure tests cover any possible failures during HMAC.
177-180: Hardened CKDPriv for ed25519.
Fully correct. Good job restricting to hardened indices.
188-188: Proper use of async HMAC.
Looks consistent. Testing with abnormal chain codes would be prudent.
192-195: Not supported, as expected.
Throwing an error here is correct for ed25519'sCKDPub.packages/core/src/chains/nexa/sdkNexa/sdk/sign.ts (8)
29-32: nonceFunctionRFC6979 goes async.
ReturningPromise<BN>is fine. Ensure that BN usage doesn't cause slow performance.
111-111: findSignature becomes async.
Great. Double-check for concurrency issues if multiple signatures are generated simultaneously.
116-116: Awaiting nonce.
Callingawait nonceFunctionRFC6979ensures correct sequence. No issues spotted.
129-129: Async sha256 call.
Neat approach. Keep an eye on performance in tight loops.
145-148: sign is now async.
This is consistent with the rest of the code. Good job.
151-151: Awaiting findSignature.
All good. Properly returns { r, s }.
158-158: verify goes async.
Proper if downstream code awaits it. Good consistency.Also applies to: 162-162
199-199: Async sha256 again.
Implementation looks consistent. Keep verifying each step carefully.packages/core/src/secret/crypto-functions.ts (7)
3-4: Platform environment imports.
These look fine. No further changes needed.
15-38: Async hmacSHA256.
Well done. However, watch out for.toString('utf8')if you handle binary data.
40-63: Good to see hmacSHA512.
Matches your pattern. Keep it consistent.
65-77: sha256 now returns a Promise.
Looks valid. Just confirm yourutf8usage for binary data.
79-82: hash160 update.
Asynchronous sha256 followed by synchronous ripemd160 is okay.
168-191: aesCbcEncrypt in async.
It’s consistent. Check if the console output in line 189 is intentional.
208-244: aesCbcDecrypt in async.
Same comment on console output. This code looks stable.packages/core/src/chains/nexa/sdkNexa/utils.ts (7)
99-124: Added async publickeyToAddress function.
This is a good switch to asynchronous logic. The use ofawait hash160is appropriate.
130-141: Async getDisplayAddress enhances flexibility.
Returning a promise is helpful if future logic needs additional async steps.
143-145: sha256sha256 chaining is neat.
Chaining two sha256 calls with awaits is clean and straightforward.
253-288: buildTxid is now async and clear.
Usingsha256sha256multiple times is correct. The code flow is easier to read with awaits.
350-422: Async buildSignatureBuffer is consistent.
It computes multiple hashes. The approach is sound and readable.
438-449: Async signEncodedTx flow is well-structured.
Signature hashing and final transaction building integrate well.
459-493: decodeScriptBufferToNexaAddress uses async hashing.
Everything looks fine. Handling isPublicKeyTemplate checks carefully.packages/core/src/secret/index.ts (3)
293-308: Async master key derivation looks good.
Great to see the seed decrypted before generating the master key. Code is more future-proof now.🧰 Tools
🪛 Biome (1.9.4)
[error] 301-301: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
[error] 301-301: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
309-323: Caching prefix keys is efficient.
Local caching avoids unnecessary repeated derivations. Implementation is clear.
326-377: Parallel path processing is good.
UsingPromise.allis an effective way to handle multiple path derivations concurrently.🧰 Tools
🪛 Biome (1.9.4)
[error] 337-337: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
[error] 338-338: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
@types/react-native-aes-crypto.d.ts (1)
15-30: New cryptographic functions make the module more complete.
The typed declarations for PBKDF2, HMAC, hashing, and randomness are clearly defined.packages/shared/src/modules3rdParty/cross-crypto/index.js (2)
5-6: Good move usingglobalThis!The switch from
globaltoglobalThisimproves compatibility across different JavaScript environments.Also applies to: 11-11
17-21: Verify crypto polyfill compatibilityThe crypto polyfill setup looks correct, but let's verify it works across all target environments.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check for crypto polyfill usage across the codebase rg -l "crypto.getRandomValues|crypto.randomBytes" --type js --type tsLength of output: 621
Crypto polyfill usage is consistent across target environments.
The polyfill setup in
packages/shared/src/modules3rdParty/cross-crypto/index.jscorrectly assignsrandomBytesandgetRandomValuesbased on global availability. The grep output shows that these methods are referenced in various modules (web, core, native, and polyfills), indicating that the intended fallback strategy is in place and used throughout.
- Polyfill setup in
index.jsproperly covers the required cases.- Usage spans web (e.g., in
polyfillsPlatform.jsandwebAuth/index.ts), core, and native contexts.- A dedicated
verify.tsfile in the same folder adds confidence to the approach.packages/shared/src/modules3rdParty/cross-crypto/index.native.js (1)
32-34: Consistent crypto method assignmentThe crypto method assignments look good. They provide proper fallbacks while maintaining compatibility.
packages/core/src/chains/ton/sdkTon/tx.ts (1)
59-60: Good async implementationThe SHA-256 operations are correctly converted to async/await. This change improves performance by not blocking the main thread.
packages/core/src/chains/nexa/CoreChainSoftware.ts (1)
121-124: Good type safety addition!The explicit type annotation and await keyword improve code safety and handle the asynchronous operation correctly.
packages/kit-bg/src/vaults/impls/nexa/KeyringHardware.ts (1)
179-180: Fixed async operation handling.Added missing await keywords to properly handle asynchronous operations. This prevents potential race conditions and ensures operations complete in the correct order.
Also applies to: 206-206
packages/core/src/secret/encryptors/aes256.ts (3)
151-151: Added async support for key derivation.Properly awaits the asynchronous key derivation operation.
156-162: Added async support for encryption.Properly awaits the asynchronous encryption operation.
244-248: Added async support for decryption.Properly awaits the asynchronous decryption operation.
development/webpack/webpack.base.config.js (1)
47-47: Good webpack configuration for native module.Correctly prevents bundling of react-native-aes-crypto in web builds, avoiding potential runtime errors.
development/spellCheckerSkipWords.js (1)
20-20: LGTM!The new entries are appropriate technical terms that should be skipped by the spell checker.
Also applies to: 25-25, 34-34
packages/kit-bg/src/vaults/impls/nexa/Vault.ts (1)
80-83: LGTM!The method now correctly awaits the asynchronous
getDisplayAddressfunction.packages/kit/src/views/Setting/pages/DevUnitTests/PageDevUnitTests.tsx (1)
110-130: LGTM!All test handlers now correctly handle asynchronous cryptographic operations while maintaining proper error handling and validation.
Also applies to: 132-152, 154-168, 170-184, 187-200, 202-217, 219-237
packages/core/src/chains/btc/sdkBtc/index.ts (1)
694-694: LGTM!The method now correctly awaits the asynchronous
CKDPubfunction.packages/core/src/secret/index.test.ts (3)
619-621: LGTM! Test cases properly handle async behavior.The test cases correctly use
await expect(...).rejects.toThrow()for testing asynchronous error conditions.Also applies to: 624-629
639-643: LGTM! Test case properly handles async rejection.The test case correctly uses
await expect(...).rejects.toThrow()for testing ed25519's lack of CKDPub support.
683-685: LGTM! Test case properly handles async sha256 operations.The test case correctly awaits the results of the asynchronous
sha256function calls.Also applies to: 690-691
packages/core/src/chains/btc/CoreChainSoftware.ts (1)
479-479: LGTM! Method properly handles async key derivation.The
CKDPrivcall is correctly awaited, maintaining proper async/await chain.packages/core/src/chains/nexa/sdkNexa/utils.test.ts (2)
261-273: LGTM! Test case properly handles async signing.The test case correctly awaits the result of the asynchronous
signfunction.
275-289: LGTM! Test case properly handles multiple async operations.The test case correctly awaits both the
sha256sha256andsignfunction calls.packages/kit-bg/src/services/ServiceAccount/ServiceAccount.ts (1)
2103-2104: LGTM! Good async/await handling.The changes properly convert the
walletHashBuildercallback to an async function and correctly await its result.Also applies to: 2121-2122
__mocks__/emptyMock.js (1)
1-2: LGTM: Mock implementation looks good.Simple and clean mock implementation.
packages/shared/src/modules3rdParty/react-native-aes-crypto/index.ts (1)
1-2: LGTM: Type assertion looks good.Clean implementation of module type export.
packages/shared/src/modules3rdParty/react-native-aes-crypto/index.native.ts (1)
1-4: LGTM: Native module export looks good.Clean implementation of native module export.
packages/shared/src/errors/errors.test.ts (1)
6-8: Test Instruction Comment Added.
The comment providing the Jest command is clear and helpful for running tests. Keep it updated if any command changes occur.apps/mobile/package.json (3)
33-33: Review Dependency @metamask/react-native-aes-crypto.
This new dependency is added. Ensure it truly aligns with replacing asmcrypto.js with native crypto functions. Prior comments suggested its removal—please reconcile if it’s still needed.
58-58: New Dependency: expo-crypto Added.
This dependency supports native crypto operations in Expo. Confirm that its implementation is consistent throughout the affected crypto code.
83-83: Added Dependency: react-native-aes-crypto.
Verify that this package is the preferred choice for the refactor and completely replaces asmcrypto.js as intended.package.json (2)
35-35: New iOS Pod Install Script Added.
This script simplifies CocoaPods setup for iOS. Ensure it integrates well with your CI/CD pipeline and developer workflow.
239-239: Updated Resolution for protobufjs.
Pinning protobufjs to version 6.11.2 can help avoid version conflicts. Please double-check that this version remains compatible with all dependent packages.packages/kit-bg/src/migrations/v4ToV5Migration/V4MigrationForAccount.ts (1)
1629-1632: LGTM! Proper async handling of wallet hash generation.The change correctly makes the hash generation asynchronous by awaiting the sha256 function call.
Run this script to verify wallet hash generation during migration:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify wallet hash generation during migration # Test: Search for any instances where sha256 is used without await rg -A 2 'sha256\(' | grep -v 'await'Length of output: 8070
The asynchronous hash generation in the migration is correctly implemented.
The code in
packages/kit-bg/src/migrations/v4ToV5Migration/V4MigrationForAccount.tsproperly awaits thesha256call, ensuring the operation completes before converting the result to a hex string. Although some other parts of the codebase usesha256withoutawait, these instances are unrelated and do not impact the migration functionality.packages/kit-bg/src/dbs/local/LocalDbBase.ts (2)
985-1009: LGTM! Proper async handling of account ID hash generation.The method correctly handles asynchronous hash generation while maintaining proper error checking.
1051-1076: LGTM! Proper handling of async record generation.The changes correctly handle asynchronous operations when generating multiple indexed account records.
Pull request was converted to draft
sidmorizon
force-pushed
the
fix/replace-to-native-crypto
branch
from
720d141 to
5ac95ad
Compare
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
sidmorizon
force-pushed
the
fix/replace-to-native-crypto
branch
from
6a585c7 to
a75d824
Compare
| @@ -437,10 +437,15 @@ export abstract class LocalDbBase extends LocalDbBaseContainer { | |||
| } | |||
|
|
|||
| // update context verifyString | |||
| const verifyString = await encryptVerifyString({ password: newPassword }); | |||
| console.log('update context verifyString 1'); | |||
|
|
|||
| await this.txUpdateContextVerifyString({ | |||
There was a problem hiding this comment.
web 下设置密码失败，需排查原因
Summary by CodeRabbit
New Features
Enhancements
Tests