feat(crypto): implement validateMnemonic#159
feat(crypto): implement validateMnemonic#159ShantelPeters wants to merge 2 commits intoancore-org:mainfrom
Conversation
|
@ShantelPeters Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR implements Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/crypto/src/encryption.ts (2)
1-1: Consider removing or narrowing the ESLint disable directive.The file-level
/* eslint-disable no-undef */is overly broad and may mask legitimate issues. If this was added to suppress errors forglobalThis,BufferSource, or other Web API globals, these should now be properly typed with"DOM"added to thetsconfig.jsonlib array.If specific lines still trigger false positives, prefer targeted inline disables with explanatory comments.
♻️ Proposed fix
-/* eslint-disable no-undef */ import { Buffer } from 'node:buffer';If specific globals still need suppression, use inline comments:
// eslint-disable-next-line no-undef -- BufferSource is a DOM global salt: salt as BufferSource,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/crypto/src/encryption.ts` at line 1, Remove the file-level "/* eslint-disable no-undef */" in packages/crypto/src/encryption.ts and instead either add "DOM" to the tsconfig.json "lib" array so globals like BufferSource and globalThis are properly typed, or replace the broad disable with targeted inline disables only where needed (for example next to the usage of salt: salt as BufferSource or any specific global references) including a brief comment explaining why the inline disable is required.
54-54: RedundantBufferSourcecasts.
Uint8Arrayalready implements theBufferSourceinterface, so these explicit casts are unnecessary now that"DOM"is included in the TypeScript lib configuration. They don't cause runtime issues but add visual noise.♻️ Proposed removal of redundant casts
- salt: salt as BufferSource, + salt,- iv: iv as BufferSource, + iv,- ciphertext as BufferSource + ciphertextAlso applies to: 134-134, 171-171, 174-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/crypto/src/encryption.ts` at line 54, Remove the redundant "as BufferSource" casts in packages/crypto/src/encryption.ts: replace occurrences like "salt: salt as BufferSource" with just "salt" (and similarly drop "as BufferSource" on the other BufferSource-typed variables in this file). Locate the objects/payloads that pass the variable named salt (and the two other spots flagged) and remove the explicit casts so the underlying Uint8Array is used directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/crypto/package.json`:
- Line 27: The package.json currently declares both "bip39": "^3.1.0" and a
redundant devDependency "@types/bip39"; since bip39 v3.1.0 bundles its own
TypeScript definitions (types/index.d.ts), remove the "@types/bip39" entry from
devDependencies to avoid duplicate type declarations and keep only the existing
"bip39" dependency referenced in package.json.
In `@packages/crypto/src/__tests__/mnemonic-validate.test.ts`:
- Around line 40-45: The test name and behavior disagree: update the test in
mnemonic-validate.test.ts so it asserts the intended behavior (either change the
description to "returns true for strings with extra spaces but correct word
count" or flip the expectation to toBe(false)); also make the whitespace
injection robust by replacing all spaces (e.g., use validMnemonic.replace(/ /g,
' ') or validMnemonic.replaceAll(' ', ' ')) and keep the assertion to match
the new description; key symbols: the test case itself and the
validateMnemonic(...) call.
---
Nitpick comments:
In `@packages/crypto/src/encryption.ts`:
- Line 1: Remove the file-level "/* eslint-disable no-undef */" in
packages/crypto/src/encryption.ts and instead either add "DOM" to the
tsconfig.json "lib" array so globals like BufferSource and globalThis are
properly typed, or replace the broad disable with targeted inline disables only
where needed (for example next to the usage of salt: salt as BufferSource or any
specific global references) including a brief comment explaining why the inline
disable is required.
- Line 54: Remove the redundant "as BufferSource" casts in
packages/crypto/src/encryption.ts: replace occurrences like "salt: salt as
BufferSource" with just "salt" (and similarly drop "as BufferSource" on the
other BufferSource-typed variables in this file). Locate the objects/payloads
that pass the variable named salt (and the two other spots flagged) and remove
the explicit casts so the underlying Uint8Array is used directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b18a07d5-81d1-4211-be65-0d83fea63d00
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
packages/crypto/package.jsonpackages/crypto/src/__tests__/mnemonic-validate.test.tspackages/crypto/src/encryption.tspackages/crypto/src/index.tspackages/crypto/src/mnemonic.tspackages/crypto/tsconfig.json
| "@noble/hashes": "^1.4.0", | ||
| "@stellar/stellar-sdk": "^12.0.0" | ||
| "@stellar/stellar-sdk": "^12.0.0", | ||
| "bip39": "^3.1.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if bip39 3.1.0 exists and inspect its package info
curl -s https://registry.npmjs.org/bip39/3.1.0 | jq '{name, version, types, typings}'
# Check latest version and if it has bundled types
curl -s https://registry.npmjs.org/bip39/latest | jq '{name, version, types, typings}'
# Check `@types/bip39` info
curl -s https://registry.npmjs.org/@types/bip39/3.0.4 | jq '{name, version}'Repository: ancore-org/ancore
Length of output: 303
Remove redundant @types/bip39 dependency.
The bip39 package at version 3.1.0 includes bundled TypeScript definitions (./types/index.d.ts), making the @types/bip39 devDependency at line 31 unnecessary. Remove @types/bip39 to eliminate the redundant type definition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/crypto/package.json` at line 27, The package.json currently declares
both "bip39": "^3.1.0" and a redundant devDependency "@types/bip39"; since bip39
v3.1.0 bundles its own TypeScript definitions (types/index.d.ts), remove the
"@types/bip39" entry from devDependencies to avoid duplicate type declarations
and keep only the existing "bip39" dependency referenced in package.json.
| it('returns false for strings with extra spaces but correct word count', () => { | ||
| const validMnemonic = bip39.generateMnemonic(128); | ||
| // Extra spaces should be handled correctly by our split logic | ||
| const spacedMnemonic = validMnemonic.replace(' ', ' '); | ||
| expect(validateMnemonic(spacedMnemonic)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Test description contradicts the expected result.
The test name says "returns false for strings with extra spaces..." but the assertion expects true. The actual behavior being tested is that extra whitespace is correctly normalized (which is good), but the description is misleading.
Additionally, String.replace(' ', ' ') only replaces the first occurrence. Consider using a regex for more thorough testing.
🐛 Proposed fix
- it('returns false for strings with extra spaces but correct word count', () => {
+ it('handles strings with extra spaces correctly', () => {
const validMnemonic = bip39.generateMnemonic(128);
- // Extra spaces should be handled correctly by our split logic
- const spacedMnemonic = validMnemonic.replace(' ', ' ');
+ // Extra spaces should be normalized by our split logic
+ const spacedMnemonic = validMnemonic.replace(/ /g, ' ');
expect(validateMnemonic(spacedMnemonic)).toBe(true);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('returns false for strings with extra spaces but correct word count', () => { | |
| const validMnemonic = bip39.generateMnemonic(128); | |
| // Extra spaces should be handled correctly by our split logic | |
| const spacedMnemonic = validMnemonic.replace(' ', ' '); | |
| expect(validateMnemonic(spacedMnemonic)).toBe(true); | |
| }); | |
| it('handles strings with extra spaces correctly', () => { | |
| const validMnemonic = bip39.generateMnemonic(128); | |
| // Extra spaces should be normalized by our split logic | |
| const spacedMnemonic = validMnemonic.replace(/ /g, ' '); | |
| expect(validateMnemonic(spacedMnemonic)).toBe(true); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/crypto/src/__tests__/mnemonic-validate.test.ts` around lines 40 -
45, The test name and behavior disagree: update the test in
mnemonic-validate.test.ts so it asserts the intended behavior (either change the
description to "returns true for strings with extra spaces but correct word
count" or flip the expectation to toBe(false)); also make the whitespace
injection robust by replacing all spaces (e.g., use validMnemonic.replace(/ /g,
' ') or validMnemonic.replaceAll(' ', ' ')) and keep the assertion to match
the new description; key symbols: the test case itself and the
validateMnemonic(...) call.
|
@ShantelPeters resolve conflicts |
Hi @wheval conflicts has been resolved |
|
Hello @wheval please review my PR |
2 participants
Description
Type of Change
Security Impact
Testing
Test Coverage
Manual Testing Steps
Breaking Changes
Checklist
For High-Security Changes
Related Issues
Closes #
Related to #
Additional Context
Reviewer Notes
CLOSES #66
Summary by CodeRabbit
New Features
Tests
Chores