fix: fr9 recommendations #1743
fix: fr9 recommendations #1743
Conversation
WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
Test CoverageSummary
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/tests/secp256k1/Secp256k1.unit.test.ts (1)
88-100: Mocking hardware support error.
Consider restoring the original method post-test to avoid test leakages:afterAll(() => { + jest.restoreAllMocks(); });packages/core/src/secp256k1/Secp256k1.ts (1)
135-159: Avoid Promise constructor in async method.
A directtry/catchblock is simpler and avoids the promise anti-pattern:public static async generatePrivateKey(): Promise<Uint8Array> { - return await new Promise<Uint8Array>((resolve) => { - try { - resolve(nc_secp256k1.utils.randomPrivateKey()); - } catch (e) { - throw new InvalidSecp256k1PrivateKey(...); - } - }); + try { + return nc_secp256k1.utils.randomPrivateKey(); + } catch (e) { + throw new InvalidSecp256k1PrivateKey(...); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/secp256k1/Secp256k1.ts(3 hunks)packages/core/tests/secp256k1/Secp256k1.unit.test.ts(1 hunks)packages/core/tests/secp256k1/fixture.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/core/tests/secp256k1/fixture.ts
🔇 Additional comments (21)
packages/core/tests/secp256k1/Secp256k1.unit.test.ts (19)
7-9: Imports look well-organized.
The new references toKeccak256,Secp256k1,Txt, andZERO_BYTESappear straightforward for cryptographic operations.
10-13: Clear test data for message hashes.
Defining both valid and invalid hashes here is concise and logical.
15-32: Appropriate key fixtures.
Storing valid and invalid private/public keys in one place simplifies test maintenance.
34-38: Single valid signature fixture.
Straightforward signature usage for testing.
45-49: Compressing key test coverage.
Verifies that a compressed key remains unchanged.
52-55: Uncompressed-to-compressed check.
Ensures compression logic is correctly applied.
59-62: Derivation of compressed key from a valid private key.
Confirms correctness usingderivePublicKey.
66-69: Derivation of uncompressed key.
Tests the same logic in uncompressed mode.
72-75: Negative test for invalid private key.
Checks that the derived public key operation rejects an improper key.
79-86: Private key generation success path.
Confirms 32-byte length and validity.
104-108: Inflating compressed key test.
Ensures correct transformation to uncompressed format.
111-114: Inflating uncompressed key pass-through.
Validates no change for already uncompressed keys.
118-124: Message hash validation.
Properly tests both valid and invalid hash lengths.
128-136: Private key validation.
Covers both valid and invalid private keys.
140-144: Signing valid hash.
Checks expected signature output with a valid private key.
147-150: Signing invalid hash.
Properly throws for an invalid message hash.
153-155: Signing with invalid key.
Ensures an error is raised for bad private keys.
160-166: Random bytes generation.
Tests both default and user-defined lengths.
172-191: Recovery tests.
Validates public key recovery plus negative cases for hashes and signatures.packages/core/src/secp256k1/Secp256k1.ts (2)
Line range hint
110-133: AES-GCM fallback for private key generation.
While this fallback is helpful, confirm it meets cryptographic randomness requirements.Would you like to verify this approach with security experts or proceed as-is?
221-232: Fallback inrandomBytes.
Ifglobal.cryptois not available, consider throwing an error or using a polyfill:Loading a fallback library or explicitly warning users might prevent runtime failures.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/tests/keystore/keystore.unit.test.ts (1)
96-96: Maintain clarity in test logs.
Although it's straightforward, consider logging the generated private key in test output only if you need to debug. In production, handle such data with care to avoid leaking secrets.packages/core/tests/secp256k1/Secp256k1.unit.test.ts (1)
160-166: Test random byte generation
Verifies default 32-byte length and a custom 16-byte length. Consider verifying edge cases (e.g., zero length).Below is an example test addition:
+ test('error <- zero length', () => { + expect(() => Secp256k1.randomBytes(0)).toThrowError(); + });packages/core/src/secp256k1/Secp256k1.ts (1)
180-191: Consider adding input validation for bytesLength.While setting a default value for
bytesLengthis good, consider adding validation to ensure it's a positive integer.public static randomBytes(bytesLength: number = 32): Uint8Array { + if (!Number.isInteger(bytesLength) || bytesLength <= 0) { + throw new Error('bytesLength must be a positive integer'); + } try { return nh_randomBytes(bytesLength); } catch (e) { return global.crypto.getRandomValues(new Uint8Array(bytesLength)); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
docs/accounts.md(1 hunks)docs/certificates.md(1 hunks)docs/cryptography.md(1 hunks)docs/examples/accounts/keystore.ts(1 hunks)docs/examples/certificates/sign_verify.ts(1 hunks)docs/examples/cryptography/secp256k1.ts(1 hunks)docs/examples/transactions/blockref-expiration.ts(1 hunks)docs/examples/transactions/multiple-clauses.ts(1 hunks)docs/examples/transactions/sign-decode.ts(1 hunks)docs/examples/transactions/tx-dependency.ts(1 hunks)docs/transactions.md(4 hunks)packages/core/src/secp256k1/Secp256k1.ts(2 hunks)packages/core/tests/keystore/keystore.unit.test.ts(5 hunks)packages/core/tests/secp256k1/Secp256k1.unit.test.ts(1 hunks)packages/network/tests/provider/helpers/provider-internal-wallets/base-wallet/provider-internal-base-wallet.unit.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: rpc-proxy / test / test
- GitHub Check: unit-integration-test-browser / Build & Lint (latest)
- GitHub Check: unit-integration-test-browser / Build & Lint (lts/*)
- GitHub Check: unit-integration-test-browser / Build & Lint (18)
- GitHub Check: unit-integration-test / Build & Lint (latest)
- GitHub Check: unit-integration-test / Build & Lint (lts/*)
- GitHub Check: unit-integration-test / Build & Lint (18)
- GitHub Check: Execute doc examples
🔇 Additional comments (26)
docs/examples/certificates/sign_verify.ts (1)
7-7: Confirm synchronous error handling.
The call toSecp256k1.generatePrivateKey()no longer returns a promise. Verify that any potential exceptions (e.g., if a secure RNG is unavailable) are correctly caught or documented.Do you want me to propose a code snippet that gracefully handles or logs possible key generation failures?
docs/examples/accounts/keystore.ts (1)
8-8: Validate synchronous call usage.
Switching to a synchronous call simplifies the flow, but ensure your code handles or communicates potential RNG unavailability or failures.docs/examples/transactions/sign-decode.ts (1)
43-43: Aligned with new synchronous generation.
The removal ofawaitreflects the revised synchronous API. Confirm that any exceptions are handled and tested.docs/examples/transactions/blockref-expiration.ts (1)
40-40: Maintain defensive error handling.
Ensure that random private key generation failures produce a clear error path, especially in production environments.docs/examples/transactions/multiple-clauses.ts (1)
49-49: Ensure proper secure random generation environment.
Switching to the new synchronous private key generation is consistent with theSecp256k1refactor. However, ensure that the runtime environment truly provides secure randomness, since there's no await mechanism to handle fallback or error scenarios.If you need to confirm that a cryptographically secure random generator is available, consider adding environment checks or catch blocks for potential exceptions in a higher-level function.
docs/examples/cryptography/secp256k1.ts (1)
8-8: Looks good!
Synchronous private key generation streamlines usage and simplifies code. This is an appropriate example for newcomers to see that the process is no longer asynchronous.docs/examples/transactions/tx-dependency.ts (1)
60-60: Confirm synchronous key generation viability.
Removing theawaitis aligned with the new synchronousSecp256k1.generatePrivateKey()method. Just be sure the environment offers cryptographically secure randomness to avoid producing weak keys.If needed, you might add a catch or environment detection to handle environments lacking a reliable secure random generator.
packages/core/tests/keystore/keystore.unit.test.ts (4)
34-34: No async overhead.
Replacingawaitwith a direct call in the test makes sense now that the method is synchronous.
71-71: Test consistency confirmed.
This test remains accurate, as the synchronous generation approach ensures faster test feedback.
119-119: Exception handling remains valid.
Since the method now throws if random generation fails, the existing test logic for invalid keys remains robust.
152-152: Keep tests straightforward.
All references to asynchronous generation have been removed, aligning with the updated implementation. Good job keeping it consistent.packages/network/tests/provider/helpers/provider-internal-wallets/base-wallet/provider-internal-base-wallet.unit.test.ts (1)
172-173: Ensure synchronous key generation remains consistent
Now thatSecp256k1.generatePrivateKey()is synchronous, please verify that no upstream code still expects a Promise. Also confirm.digitsyields the intended format for this test.docs/certificates.md (1)
50-50: Documentation alignment looks good
Switching from async to synchronous private key generation here is correctly reflected. Consider adding a note about potential error handling if key generation fails.docs/cryptography.md (1)
54-54: Synchronized docs with Synchronous API
The updated snippet aligns with the synchronousSecp256k1.generatePrivateKey()method. No further issues detected.packages/core/tests/secp256k1/Secp256k1.unit.test.ts (9)
7-38: Proper setup of constants and imports
Defining new constants for valid/invalid keys and signatures improves organization. The overall naming is clear, and usingn_utils.hexToBytesis appropriate for test data.
45-55: Comprehensive compression tests
These tests confirm that both compressed and uncompressed public keys are handled. No issues detected in coverage.
59-72: Verify derived keys in all formats
Good job testing compressed and uncompressed public keys, plus an invalid key scenario. This covers typical edge cases thoroughly.
79-100: Robust private key generation tests
Excellent handling of success and error scenarios, including mocking hardware support issues. This ensures coverage for both normal and exceptional paths.
104-114: Inflate public key coverage
Testing inflating both compressed and uncompressed keys is consistent with the compression tests. No concerns here.
118-124: Valid vs. invalid hash checks
Straightforward checks confirming that message hashes of correct length pass validation. Implementation looks fine.
128-136: Private key validation
Tests for valid/invalid private keys are comprehensive. Good coverage.
140-155: Signature tests cover valid/invalid cases
Solid coverage for valid hash, invalid hash, and invalid private key inputs. Nicely done.
172-191: Recovery tests for valid and invalid input
Covers correct recovery with valid inputs and error handling for invalid hash/signature. No issues noted.docs/accounts.md (1)
133-133: LGTM! Documentation updated to reflect synchronous private key generation.The change correctly removes the
awaitkeyword asgeneratePrivateKeyis now a synchronous method.packages/core/src/secp256k1/Secp256k1.ts (1)
97-116: LGTM! Improved error handling for private key generation.The changes effectively:
- Make the method synchronous, removing unnecessary Promise wrapping
- Add detailed error handling to ensure secure RNG availability
- Improve error messages for better debugging
docs/transactions.md (1)
50-50: LGTM! Documentation examples updated for synchronous private key generation.All examples have been correctly updated to remove
awaitkeywords when callinggeneratePrivateKey, maintaining consistency with the updated API.Also applies to: 102-102, 217-217, 280-280
Description
The SDK generates a SECP256K1 private key only if the runtime provides a suitable cryptographic secure random generator device.
Fixes # FR9 recommendation.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
yarn testyarn test:browseryarn test:soloTest Configuration:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests