fix: Add IndexedDB support to persist the private key generated by Se… #68
Conversation
WalkthroughThe pull request introduces enhancements to the secret management system by adding an IndexedDB-based storage mechanism. The Changes
Sequence DiagramsequenceDiagram
participant SM as SecretManager
participant IDB as IndexedDBStore
participant WC as WalletClient
SM->>IDB: initialize()
IDB-->>SM: Database ready
SM->>IDB: getAsyncItem(privateKey)
alt Key exists in store
IDB-->>SM: Return stored key
else No stored key
SM->>WC: Derive private key
SM->>IDB: setItem(privateKey)
IDB-->>SM: Key stored
end
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
packages/core/src/lib/secretManager/secretManager.ts (1)
28-28: InitializesecretStoreafter checking for custom storeThe assignment of
this.secretStoreshould occur after verifying if a custom store is provided to ensure correct initialization.Apply this diff to improve initialization:
this.walletClient = walletClient; - this.secretStore = store ?? new IndexedDBStore(); + this.secretStore = store ? store : new IndexedDBStore();packages/utils/src/lib/store/store.interface.ts (3)
34-35: Add JSDoc documentation for the new methods.The new methods lack documentation. Please add JSDoc comments to maintain consistency with the existing methods and improve code maintainability.
Add documentation like this:
+ /** + * Retrieves an item asynchronously from the store using the provided key. + * @param {string} key - The key of the item to retrieve. + * @returns {AsyncResult<string, null>} A result containing the item if found, or null if not found. + */ getAsyncItem?(key: string): AsyncResult<string, null>; + /** + * Initializes the store asynchronously. This should be called before using the store. + * @returns {Promise<void>} A promise that resolves when initialization is complete. + */ initialize?(): Promise<void>;
Line range hint
38-40: Add private key related constants to StoreKeys enum.Since this PR adds IndexedDB support for persisting private keys, consider adding relevant constants to the StoreKeys enum.
Add a constant like this:
export enum StoreKeys { AUTH_TOKEN = 'auth_token', + PRIVATE_KEY = 'private_key', }
Line range hint
1-40: Consider architectural implications of mixing sync and async storage methods.The interface now supports both synchronous (
getItem) and asynchronous (getAsyncItem) storage operations. While this maintains backward compatibility, it could lead to confusion about which method to use. Consider:
- Adding documentation explaining when to use each method
- Potentially deprecating the sync methods in favor of async ones
- Or splitting into separate interfaces for sync and async storage
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
packages/core/src/lib/secretManager/secretManager.ts(3 hunks)packages/utils/src/lib/store/index.ts(1 hunks)packages/utils/src/lib/store/indexedDbStorage.ts(1 hunks)packages/utils/src/lib/store/store.interface.ts(2 hunks)
🔇 Additional comments (2)
packages/core/src/lib/secretManager/secretManager.ts (1)
65-70: Handle potential errors when setting the private key in the storeWhen storing the private key using
setItem, consider handling potential errors that might occur during the operation.Ensure that errors during
setItemare caught and handled appropriately.packages/utils/src/lib/store/index.ts (1)
3-3: LGTM!The new export statement correctly adds the
indexedDbStoragemodule to the public API.
| getItem(key: string): string | null { | ||
| if (!this.db) { | ||
| throw new Error('Database not initialized'); | ||
| } | ||
| const transaction = this.db.transaction([this.STORE_NAME], 'readonly'); | ||
| const store = transaction.objectStore(this.STORE_NAME); | ||
| const request = store.get(key.toLowerCase()); | ||
|
|
||
| let result = null; | ||
| request.onsuccess = () => { | ||
| result = request.result || null; | ||
| }; | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Synchronous getItem method will not work due to IndexedDB's asynchronous nature
The getItem method attempts to perform a synchronous operation using the asynchronous IndexedDB API. The result will always be null because the onsuccess callback executes after the return statement. IndexedDB does not support synchronous operations, so the method should be made asynchronous.
Apply this diff to correct the issue:
- getItem(key: string): string | null {
+ async getItem(key: string): Promise<string | null> {
if (!this.db) {
throw new Error('Database not initialized');
}
const transaction = this.db.transaction([this.STORE_NAME], 'readonly');
const store = transaction.objectStore(this.STORE_NAME);
const request = store.get(key.toLowerCase());
- let result = null;
- request.onsuccess = () => {
- result = request.result || null;
- };
- return result;
+ return new Promise<string | null>((resolve, reject) => {
+ request.onsuccess = () => {
+ resolve(request.result || null);
+ };
+ request.onerror = () => {
+ reject(null);
+ };
+ });
}📝 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.
| getItem(key: string): string | null { | |
| if (!this.db) { | |
| throw new Error('Database not initialized'); | |
| } | |
| const transaction = this.db.transaction([this.STORE_NAME], 'readonly'); | |
| const store = transaction.objectStore(this.STORE_NAME); | |
| const request = store.get(key.toLowerCase()); | |
| let result = null; | |
| request.onsuccess = () => { | |
| result = request.result || null; | |
| }; | |
| return result; | |
| } | |
| async getItem(key: string): Promise<string | null> { | |
| if (!this.db) { | |
| throw new Error('Database not initialized'); | |
| } | |
| const transaction = this.db.transaction([this.STORE_NAME], 'readonly'); | |
| const store = transaction.objectStore(this.STORE_NAME); | |
| const request = store.get(key.toLowerCase()); | |
| return new Promise<string | null>((resolve, reject) => { | |
| request.onsuccess = () => { | |
| resolve(request.result || null); | |
| }; | |
| request.onerror = () => { | |
| reject(null); | |
| }; | |
| }); | |
| } |
| console.log('Generated secret:', secret); | ||
| console.log('Generated secret hash:', secretHash); |
There was a problem hiding this comment.
Do not log generated secret and secret hash
Logging the generated secret and its hash can compromise security by exposing sensitive information.
Remove the log statements:
- console.log('Generated secret:', secret);
- console.log('Generated secret hash:', secretHash);| if (!this.privKey) { | ||
| return Err('No private key found'); | ||
| } | ||
| console.log('Master private key:', this.privKey); |
There was a problem hiding this comment.
Avoid logging the master private key
The master private key should not be logged as it poses a significant security risk.
Remove the log statement:
- console.log('Master private key:', this.privKey);| console.log('Signature derived:', signature); | ||
| this.privKey = trim0x(sha256(signature)); | ||
| this.emit('initialized', true); | ||
|
|
||
| console.log('Private key derived:', this.privKey); |
There was a problem hiding this comment.
Remove console logs that expose sensitive information
Logging sensitive data such as signatures and private keys can lead to security vulnerabilities. These values should not be logged.
Apply this diff to remove sensitive logs:
console.log('Signature derived:', signature);
this.privKey = trim0x(sha256(signature));
this.emit('initialized', true);
- console.log('Private key derived:', this.privKey);Committable suggestion skipped: line range outside the PR's diff.
No description provided.