-
Notifications
You must be signed in to change notification settings - Fork 179
Replace react-native-quick-crypto with @noble/hashes #841
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
Conversation
|
Warning Rate limit exceeded@transphorm has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (3)
WalkthroughA new test suite has been added to validate the correctness of ethers crypto polyfills. The tests use mocked crypto modules and cover functions such as random byte generation, HMAC computation, PBKDF2 key derivation, and SHA-2 hashing, asserting their outputs against known values. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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.
Actionable comments posted: 1
🧹 Nitpick comments (5)
app/tests/utils/ethers.test.ts (5)
21-30: Well-implemented HMAC test with known vector.Using known test vectors is the gold standard for cryptographic function testing. The implementation correctly validates HMAC-SHA256 functionality.
Consider adding tests for other HMAC algorithms and edge cases:
it('computeHmac supports different algorithms', () => { const result = ethers.computeHmac( 'sha512', ethers.toUtf8Bytes('key'), ethers.toUtf8Bytes('data'), ); // Add expected SHA512 HMAC result }); it('computeHmac handles edge cases', () => { // Empty key const emptyKey = ethers.computeHmac('sha256', new Uint8Array(0), ethers.toUtf8Bytes('data')); // Empty data const emptyData = ethers.computeHmac('sha256', ethers.toUtf8Bytes('key'), new Uint8Array(0)); // Both non-empty results should be produced expect(emptyKey.length).toBe(32); expect(emptyData.length).toBe(32); });
32-43: Solid PBKDF2 test implementation with room for enhancement.The test correctly validates PBKDF2 functionality with known parameters. Note that 1000 iterations is suitable for testing but low for production security standards (recommend 100,000+ for production).
Consider adding tests for different parameters and edge cases:
it('pbkdf2 works with different iteration counts and output lengths', () => { const result1 = ethers.pbkdf2( ethers.toUtf8Bytes('password'), ethers.toUtf8Bytes('salt'), 10000, // Higher iteration count 64, // Longer output 'sha256', ); expect(result1).toHaveLength(64); const result2 = ethers.pbkdf2( ethers.toUtf8Bytes('password'), ethers.toUtf8Bytes('salt'), 1000, 16, // Shorter output 'sha512', // Different hash ); expect(result2).toHaveLength(16); });
45-50: Correct SHA256 test with standard test vector.The implementation properly tests SHA256 hashing with a known input/output pair and correctly handles UTF-8 encoding.
Add tests for additional inputs and edge cases:
it('sha256 handles various inputs correctly', () => { // Empty string const empty = ethers.sha256(new Uint8Array(0)); expect(ethers.hexlify(empty)).toBe( '0xe3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855' ); // Binary data const binary = ethers.sha256(new Uint8Array([0x00, 0xFF, 0x42])); expect(binary).toHaveLength(32); });
52-57: Comprehensive SHA512 test with correct expected output.The test properly validates SHA512 functionality with the expected 64-byte output length and correct hash value for the input "hello".
Similar to SHA256, consider adding tests for edge cases:
it('sha512 produces consistent 64-byte outputs', () => { const empty = ethers.sha512(new Uint8Array(0)); expect(empty).toHaveLength(64); const large = ethers.sha512(new Uint8Array(1000).fill(0xAA)); expect(large).toHaveLength(64); });
11-58: Excellent foundation for crypto polyfill testing with opportunities for enhancement.This test suite provides solid coverage of the core cryptographic functions with proper use of known test vectors and appropriate mocking. The structure is clean and maintainable.
For a production-grade crypto test suite, consider these architectural improvements:
- Parameterized tests: Use
describe.each()orit.each()for testing multiple input/output pairs- Property-based testing: For randomness functions, consider using libraries like
fast-checkto test statistical properties- Performance benchmarks: Add basic performance tests to catch regressions in crypto operations
- Cross-platform validation: Consider running a subset of tests against actual react-native-quick-crypto in integration tests
- Security test cases: Add tests that verify crypto functions fail appropriately with invalid inputs
Example enhancement:
const hashTestVectors = [ ['hello', '0x2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824'], ['', '0xe3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'], // More vectors... ]; describe.each(hashTestVectors)('sha256 test vectors', (input, expected) => { it(`hashes "${input}" correctly`, () => { const result = ethers.sha256(ethers.toUtf8Bytes(input)); expect(ethers.hexlify(result)).toBe(expected); }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/tests/utils/ethers.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{test,spec}.{ts,js,tsx,jsx}
⚙️ CodeRabbit Configuration File
**/*.{test,spec}.{ts,js,tsx,jsx}: Review test files for:
- Test coverage completeness
- Test case quality and edge cases
- Mock usage appropriateness
- Test readability and maintainability
Files:
app/tests/utils/ethers.test.ts
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: analyze-ios
- GitHub Check: analyze-android
- GitHub Check: build
🔇 Additional comments (1)
app/tests/utils/ethers.test.ts (1)
5-9: Mocks and Polyfills Verified – Ready to MergeI’ve confirmed that
app/src/utils/ethers.tsregisters all necessary crypto polyfills—randomBytes,computeHmac,pbkdf2,sha256, andsha512—using the same method signatures as Node’s nativecryptoAPI. Mockingreact-native-quick-cryptoto delegate to Node’s crypto will faithfully exercise those hooks in tests.Everything looks solid; no further changes needed.
* Add tests for ethers polyfills * Add crypto utils * Inline crypto polyfills into ethers util * sort and update gemfile lock * update lock
* Replace react-native-quick-crypto with @noble/hashes (#841) * Add tests for ethers polyfills * Add crypto utils * Inline crypto polyfills into ethers util * sort and update gemfile lock * update lock * chore: incrementing ios build number for version 2.6.3 [github action] * android works. ios wip. update locks * Specify Maestro platform * Fix Android build step in e2e workflow * fix android * update ios * add concurrency * update Podfile.lock * fix android * prettier * fix * fix android pipeline * try job again * fix ios * fix android * fix ios * fix command * use android runner now that path is fixed * fix android e2e test * fix adb * add caching * fix build * speed up build * fix * test emulator options * updates * fix pipeline * fix * fix script and move on * add comment --------- Co-authored-by: Self GitHub Actions <[email protected]>
* Add Maestro e2e testing * Run Maestro flows in parallel * Fix mobile e2e workflow * Fix e2e script flow path * prettier * fix * prettier * standardize yml files and new formatting commands * fix ndk * fix exclusions * use double quotes for yml files * feedback * fixes * fixes * fix * fix ios job * unneeded * fix workflows * fix launch workflow * fix * fix pipeline * workflow fixes * install app to emulators * better logging * save current version of test script * android works. ios wip. update locks * fix pipelines * cr feedback * fix android e2e test * Split mobile e2e workflow by platform (#842) * Replace react-native-quick-crypto with @noble/hashes (#841) * Add tests for ethers polyfills * Add crypto utils * Inline crypto polyfills into ethers util * sort and update gemfile lock * update lock * chore: incrementing ios build number for version 2.6.3 [github action] * android works. ios wip. update locks * Specify Maestro platform * Fix Android build step in e2e workflow * fix android * update ios * add concurrency * update Podfile.lock * fix android * prettier * fix * fix android pipeline * try job again * fix ios * fix android * fix ios * fix command * use android runner now that path is fixed * fix android e2e test * fix adb * add caching * fix build * speed up build * fix * test emulator options * updates * fix pipeline * fix * fix script and move on * add comment --------- Co-authored-by: Self GitHub Actions <[email protected]> * feedback * fixes * ignore for now * ignore * fix tests * fix ios simulator booting * fix ios test * shutdown after run * fix ios test * better timing * increase ios timeout * fix both flows * fix pipeline * combine command * fix ios * break up build steps for better caching * remove cache * fix ios and android test pipelines * update logic --------- Co-authored-by: Self GitHub Actions <[email protected]>
Removing
react-native-quick-cryptobecause it seems to be at the center of a frequent build conflicts. Hopefully this improves the stability of our builds 🤞Summary
react-native-quick-cryptowith Node's crypto during testsTesting
yarn lintyarn buildyarn workspace @selfxyz/contracts build(fails: Invalid account for network)yarn typesyarn workspace @selfxyz/common testyarn workspace @selfxyz/circuits test(fails: Unsupported signature algorithm: undefined)yarn workspace @selfxyz/mobile-app testhttps://chatgpt.com/codex/tasks/task_b_689122b8b05c832d971bf0b1aba26277
Summary by CodeRabbit