feat: add expo-sqlite driver adapter, key service, and live-query hooks#7396
feat: add expo-sqlite driver adapter, key service, and live-query hooks#7396diegolmello wants to merge 4 commits into
Conversation
…ks (NATIVE-1274) Delivers the driver adapter layer sitting between expo-sqlite and the rest of the app, as the only permitted call site for expo-sqlite: - connection.ts: open/close lifecycle with App Group path resolution (iOS), post-open PRAGMA key → busy_timeout=500 → WAL invariant, per-DB handle registry, drizzle() wrapping, deleteDb support. - keyService.ts: getOrCreateDatabaseKey / deleteDatabaseKey backed by a keychainShim interface; shim defaults to an in-memory dev stand-in pending the native Keychain binding (NATIVE-1276). CSPRNG from @rocket.chat/mobile-crypto randomBytes (64 hex chars / 32 bytes). - observe.ts: useTableQuery (V2 structural-sharing list hook, ~16ms debounce, table-filtered addDatabaseChangeListener) and useRowObserve (V3 per-rowid hook), both ported from the on-device validated PoC. - ios/Podfile.properties.json: expo.sqlite.useSQLCipher = "true" - android/gradle.properties: expo.sqlite.useSQLCipher=true - expo-sqlite ~16.0.10 added via `expo install` (SDK-54 compatible) L1 Jest tests cover: key creation/idempotence, no key material in errors, shim replacement, DB name derivation, open-sequence ordering (PRAGMA key first), busy_timeout, WAL, registry dedup, debounce coalescing, table filtering, structural sharing (same ref/new ref), useRowObserve rowId matching. 33 tests added; full suite 1572 tests green.
WalkthroughEnables SQLCipher for expo-sqlite via build configuration, implements a key and salt service with keychain abstraction and persistence, establishes a database connection module enforcing strict PRAGMA ordering and registry caching, and provides reactive observation hooks (useTableQuery, useRowObserve) with comprehensive tests for behavior, filtering, structural sharing, and error safety. ChangesEncrypted SQLite Database Integration
Sequence Diagram(s)sequenceDiagram
participant App
participant KeyService
participant IKeychainShim
participant randomKey as CSPRNG (randomKey)
App->>KeyService: installKeychainShim(nativeShim)
Note over KeyService: Replace dev shim with native
App->>KeyService: getOrCreateDatabaseKey(dbName)
KeyService->>IKeychainShim: getItem(db_key_v1:dbName)
alt Key exists
IKeychainShim-->>KeyService: 64-hex key
else Key not found
KeyService->>randomKey: Generate 32 bytes
randomKey-->>KeyService: 32-byte buffer
KeyService->>KeyService: Encode as 64 hex chars, validate
KeyService->>IKeychainShim: setItem(db_key_v1:dbName, key)
IKeychainShim-->>KeyService: stored
end
KeyService-->>App: 64-hex key
sequenceDiagram
participant Caller
participant ConnectionModule
participant KeyService
participant openDatabaseAsync as expo-sqlite
participant Drizzle
Caller->>ConnectionModule: openServerDb(serverUrl)
ConnectionModule->>ConnectionModule: deriveServerDbName(serverUrl)
ConnectionModule->>KeyService: getOrCreateDatabaseKey(dbName)
KeyService-->>ConnectionModule: 64-hex key
ConnectionModule->>KeyService: getOrCreateDatabaseSalt(dbName)
KeyService-->>ConnectionModule: 32-hex salt
ConnectionModule->>ConnectionModule: resolveDbDirectory()
ConnectionModule->>openDatabaseAsync: openDatabaseAsync(dbName, options)
openDatabaseAsync-->>ConnectionModule: SQLiteDatabase handle
Note over ConnectionModule: Apply PRAGMAs in strict order
ConnectionModule->>ConnectionModule: PRAGMA key = x'...'
ConnectionModule->>ConnectionModule: PRAGMA cipher_plaintext_header_size = 32
ConnectionModule->>ConnectionModule: PRAGMA cipher_salt = x'...'
ConnectionModule->>ConnectionModule: PRAGMA busy_timeout = 500
ConnectionModule->>ConnectionModule: PRAGMA journal_mode = WAL
ConnectionModule->>ConnectionModule: SELECT COUNT(*) FROM sqlite_master
Note over ConnectionModule: Verify encryption successful
ConnectionModule->>Drizzle: Wrap with schema (app or servers)
Drizzle-->>ConnectionModule: ExpoSQLiteDatabase
ConnectionModule->>ConnectionModule: Register handle in cache
ConnectionModule-->>Caller: DbHandle { db, sqlite, dbName }
sequenceDiagram
participant Component
participant useTableQuery as useTableQuery Hook
participant ExpoSQLite as expo-sqlite Listener
participant queryFn as queryFn Callback
Component->>useTableQuery: useTableQuery(dbHandle, options)
useTableQuery->>queryFn: Call synchronously on mount
queryFn-->>useTableQuery: Initial rows []
useTableQuery->>ExpoSQLite: addDatabaseChangeListener(callback)
Note over useTableQuery: Subscribe to changes
Component->>Component: Rapid table changes fire events
Component->>ExpoSQLite: Emit event (table, rowId, databaseFilePath)
ExpoSQLite->>useTableQuery: Change event
useTableQuery->>useTableQuery: Filter by dbName & allowed tables
useTableQuery->>useTableQuery: Debounce 16ms timer
useTableQuery->>queryFn: Call after debounce window
queryFn-->>useTableQuery: Updated rows []
useTableQuery->>useTableQuery: Reconcile by rowKey, reuse via equalFn
useTableQuery-->>Component: State update with new/reused rows
Component->>Component: Unmount
useTableQuery->>ExpoSQLite: Remove listener
useTableQuery->>useTableQuery: Clear debounce timer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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. 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)
app/lib/database/driver/__tests__/keyService.test.ts (1)
53-62: ⚡ Quick winThe test title claims key difference, but the assertions don’t verify it.
With a constant
randomKeymock, this case only checks format. Please either rename the test to match current intent or mock distinct values and assert isolation behavior directly.Stronger deterministic test option
it('returns different keys for different db names', async () => { - // The mock always returns the same hex but each name gets its own entry; - // since we mock randomKey to the same value both will equal the mock value — - // the key isolation per name is structural (separate store entries), tested via delete below. + (randomKey as jest.Mock) + .mockResolvedValueOnce('1111111111111111111111111111111111111111111111111111111111111111') + .mockResolvedValueOnce('2222222222222222222222222222222222222222222222222222222222222222'); const k1 = await getOrCreateDatabaseKey('server-a.db'); const k2 = await getOrCreateDatabaseKey('server-b.db'); - // Both are 64-hex; they happen to be equal under the mock but the storage keys differ - expect(k1).toMatch(/^[0-9a-fA-F]{64}$/); - expect(k2).toMatch(/^[0-9a-fA-F]{64}$/); + expect(k1).not.toBe(k2); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/database/driver/__tests__/keyService.test.ts` around lines 53 - 62, The test title says "returns different keys for different db names" but it only asserts format because randomKey is mocked to a constant; either rename the test to reflect format-only verification or modify the mock for randomKey and assert isolation: change the mock of randomKey to return distinct deterministic values for successive calls and then assert k1 !== k2 (or verify store-level isolation by checking deletion of one DB key doesn't remove the other) when calling getOrCreateDatabaseKey; update the test title and expectations accordingly.app/lib/database/driver/__tests__/observe.test.ts (1)
19-60: 💤 Low valueUnused
mockSubscriptionvariable.
mockSubscriptionis defined but never used — eachaddDatabaseChangeListenercall returns a fresh object with its ownremovefunction (lines 31-36). Consider removing it to reduce confusion.🧹 Proposed cleanup
-const mockSubscription = { remove: jest.fn() }; - jest.mock('expo-sqlite', () => ({ addDatabaseChangeListener: jest.fn((fn: ChangeListener) => { _listeners.push(fn); return { remove: jest.fn(() => { const idx = _listeners.indexOf(fn); if (idx !== -1) _listeners.splice(idx, 1); }) }; }) }));Also remove the unused
mockSubscription.remove.mockClear()call inbeforeEach(line 55).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/database/driver/__tests__/observe.test.ts` around lines 19 - 60, The test file defines an unused mockSubscription constant and calls mockSubscription.remove.mockClear() in beforeEach, which is confusing since addDatabaseChangeListener returns its own remove object; remove the mockSubscription declaration and the mockSubscription.remove.mockClear() call, and keep the real per-subscription remove behavior from the jest.mock implementation (reference: mockSubscription, addDatabaseChangeListener and the beforeEach block).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/database/driver/connection.ts`:
- Around line 151-171: openDb currently calls openDatabaseAsync then
applyOpenPragmas; if applyOpenPragmas throws the opened sqlite handle is never
closed. Wrap the applyOpenPragmas call in a try/catch (inside openDb) and in the
catch await/ call sqlite.close() (or the appropriate close method on the
returned sqlite) to ensure the DB handle is released, then rethrow the original
error; keep registry logic unchanged so _registry.set(dbName, ...) only runs
after successful open and pragmas.
In `@app/lib/database/driver/keyService.ts`:
- Around line 81-99: Concurrent callers to getOrCreateDatabaseKey can race and
persist different keys; serialize first-write per dbName by introducing per-name
in-flight serialization: add a Map (e.g., pendingKeys) keyed by
storageKey(dbName) that stores the Promise for the ongoing creation, return that
promise if present, otherwise create and store the promise for the generator;
inside the creation promise perform the existing check via _shim.getItem(k),
generate hex via randomKey(32), validate it, then before setItem re-check
_shim.getItem(k) and if it now exists return that value instead of overwriting;
finally call _shim.setItem(k, hex) and cleanup the pendingKeys entry. Ensure all
references use getOrCreateDatabaseKey, storageKey, _shim.getItem, _shim.setItem,
randomKey and that the pending map is cleared on completion/error.
---
Nitpick comments:
In `@app/lib/database/driver/__tests__/keyService.test.ts`:
- Around line 53-62: The test title says "returns different keys for different
db names" but it only asserts format because randomKey is mocked to a constant;
either rename the test to reflect format-only verification or modify the mock
for randomKey and assert isolation: change the mock of randomKey to return
distinct deterministic values for successive calls and then assert k1 !== k2 (or
verify store-level isolation by checking deletion of one DB key doesn't remove
the other) when calling getOrCreateDatabaseKey; update the test title and
expectations accordingly.
In `@app/lib/database/driver/__tests__/observe.test.ts`:
- Around line 19-60: The test file defines an unused mockSubscription constant
and calls mockSubscription.remove.mockClear() in beforeEach, which is confusing
since addDatabaseChangeListener returns its own remove object; remove the
mockSubscription declaration and the mockSubscription.remove.mockClear() call,
and keep the real per-subscription remove behavior from the jest.mock
implementation (reference: mockSubscription, addDatabaseChangeListener and the
beforeEach block).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01046025-0401-439c-ac3a-e3f42052ee89
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
android/gradle.propertiesapp/lib/database/driver/__tests__/connection.test.tsapp/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/__tests__/observe.test.tsapp/lib/database/driver/connection.tsapp/lib/database/driver/keyService.tsapp/lib/database/driver/observe.tsios/Podfile.properties.jsonpackage.json
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
ios/Podfile.properties.jsonpackage.jsonapp/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/__tests__/connection.test.tsapp/lib/database/driver/keyService.tsapp/lib/database/driver/observe.tsapp/lib/database/driver/__tests__/observe.test.tsapp/lib/database/driver/connection.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/__tests__/connection.test.tsapp/lib/database/driver/keyService.tsapp/lib/database/driver/observe.tsapp/lib/database/driver/__tests__/observe.test.tsapp/lib/database/driver/connection.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/__tests__/connection.test.tsapp/lib/database/driver/keyService.tsapp/lib/database/driver/observe.tsapp/lib/database/driver/__tests__/observe.test.tsapp/lib/database/driver/connection.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/__tests__/connection.test.tsapp/lib/database/driver/keyService.tsapp/lib/database/driver/observe.tsapp/lib/database/driver/__tests__/observe.test.tsapp/lib/database/driver/connection.ts
🧠 Learnings (3)
📚 Learning: 2026-02-05T13:55:00.974Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6930
File: package.json:101-101
Timestamp: 2026-02-05T13:55:00.974Z
Learning: In this repository, the dependency on react-native-image-crop-picker should reference the RocketChat fork (RocketChat/react-native-image-crop-picker) with explicit commit pins, not the upstream ivpusic/react-native-image-crop-picker. Update package.json dependencies (and any lockfile) to point to the fork URL and a specific commit, ensuring edge-to-edge Android fixes are included. This pattern should apply to all package.json files in the repo that declare this dependency.
Applied to files:
package.json
📚 Learning: 2026-05-07T17:47:14.516Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7303
File: package.json:5-5
Timestamp: 2026-05-07T17:47:14.516Z
Learning: When reviewing pnpm `packageManager` version pins in any `package.json` (e.g., `"packageManager": "pnpm@<version>"`), don’t rely solely on web-search results to determine whether a version exists. For very recently published versions, cross-check the target version against the official pnpm release page (https://github.com/pnpm/pnpm/releases) and the npm registry page for pnpm (https://www.npmjs.com/package/pnpm) before flagging the pinned version as non-existent.
Applied to files:
package.json
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/__tests__/connection.test.tsapp/lib/database/driver/keyService.tsapp/lib/database/driver/observe.tsapp/lib/database/driver/__tests__/observe.test.tsapp/lib/database/driver/connection.ts
🔇 Additional comments (24)
android/gradle.properties (1)
51-54: LGTM!ios/Podfile.properties.json (1)
1-3: LGTM!package.json (1)
76-76: LGTM!app/lib/database/driver/connection.ts (6)
1-36: LGTM!
90-113: LGTM!
125-144: LGTM!
177-212: LGTM!
214-228: LGTM!
65-77: LGTM!app/lib/database/driver/__tests__/connection.test.ts (6)
18-66: LGTM!
72-91: LGTM!
97-116: LGTM!
122-166: LGTM!
172-193: LGTM!
199-215: LGTM!app/lib/database/driver/observe.ts (4)
32-40: LGTM!
46-67: LGTM!
79-149: LGTM!
164-210: LGTM!app/lib/database/driver/__tests__/observe.test.ts (5)
62-80: LGTM!
86-143: LGTM!
149-194: LGTM!
200-250: LGTM!
256-333: LGTM!
| async function openDb<K extends DbKind>(dbName: string, kind: K): Promise<DbHandle<K>> { | ||
| const cached = _registry.get(dbName); | ||
| if (cached) { | ||
| return cached as DbHandle<K>; | ||
| } | ||
|
|
||
| const keyHex = await getOrCreateDatabaseKey(dbName); | ||
|
|
||
| const sqlite = await openDatabaseAsync(dbName, { enableChangeListener: true }, DB_DIRECTORY); | ||
|
|
||
| await applyOpenPragmas(sqlite, keyHex); | ||
|
|
||
| const schema = kind === 'servers' ? serversSchema : appSchema; | ||
| // The conditional type SchemaForKind<K> cannot be narrowed by the JS runtime check above; | ||
| // casting through unknown is the standard TS pattern for this shape. | ||
| const db = drizzle(sqlite, { schema }) as unknown as ExpoSQLiteDatabase<SchemaForKind<K>>; | ||
|
|
||
| const handle: DbHandle<K> = { db, sqlite, dbName }; | ||
| _registry.set(dbName, handle as DbHandle); | ||
| return handle; | ||
| } |
There was a problem hiding this comment.
Resource leak when applyOpenPragmas fails.
If applyOpenPragmas throws (e.g., open-verify fails), the sqlite handle returned by openDatabaseAsync is never closed. This leaks an open file descriptor.
🛠️ Proposed fix to close handle on failure
async function openDb<K extends DbKind>(dbName: string, kind: K): Promise<DbHandle<K>> {
const cached = _registry.get(dbName);
if (cached) {
return cached as DbHandle<K>;
}
const keyHex = await getOrCreateDatabaseKey(dbName);
const sqlite = await openDatabaseAsync(dbName, { enableChangeListener: true }, DB_DIRECTORY);
- await applyOpenPragmas(sqlite, keyHex);
+ try {
+ await applyOpenPragmas(sqlite, keyHex);
+ } catch (e) {
+ await sqlite.closeAsync();
+ throw e;
+ }
const schema = kind === 'servers' ? serversSchema : appSchema;📝 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.
| async function openDb<K extends DbKind>(dbName: string, kind: K): Promise<DbHandle<K>> { | |
| const cached = _registry.get(dbName); | |
| if (cached) { | |
| return cached as DbHandle<K>; | |
| } | |
| const keyHex = await getOrCreateDatabaseKey(dbName); | |
| const sqlite = await openDatabaseAsync(dbName, { enableChangeListener: true }, DB_DIRECTORY); | |
| await applyOpenPragmas(sqlite, keyHex); | |
| const schema = kind === 'servers' ? serversSchema : appSchema; | |
| // The conditional type SchemaForKind<K> cannot be narrowed by the JS runtime check above; | |
| // casting through unknown is the standard TS pattern for this shape. | |
| const db = drizzle(sqlite, { schema }) as unknown as ExpoSQLiteDatabase<SchemaForKind<K>>; | |
| const handle: DbHandle<K> = { db, sqlite, dbName }; | |
| _registry.set(dbName, handle as DbHandle); | |
| return handle; | |
| } | |
| async function openDb<K extends DbKind>(dbName: string, kind: K): Promise<DbHandle<K>> { | |
| const cached = _registry.get(dbName); | |
| if (cached) { | |
| return cached as DbHandle<K>; | |
| } | |
| const keyHex = await getOrCreateDatabaseKey(dbName); | |
| const sqlite = await openDatabaseAsync(dbName, { enableChangeListener: true }, DB_DIRECTORY); | |
| try { | |
| await applyOpenPragmas(sqlite, keyHex); | |
| } catch (e) { | |
| await sqlite.closeAsync(); | |
| throw e; | |
| } | |
| const schema = kind === 'servers' ? serversSchema : appSchema; | |
| // The conditional type SchemaForKind<K> cannot be narrowed by the JS runtime check above; | |
| // casting through unknown is the standard TS pattern for this shape. | |
| const db = drizzle(sqlite, { schema }) as unknown as ExpoSQLiteDatabase<SchemaForKind<K>>; | |
| const handle: DbHandle<K> = { db, sqlite, dbName }; | |
| _registry.set(dbName, handle as DbHandle); | |
| return handle; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/lib/database/driver/connection.ts` around lines 151 - 171, openDb
currently calls openDatabaseAsync then applyOpenPragmas; if applyOpenPragmas
throws the opened sqlite handle is never closed. Wrap the applyOpenPragmas call
in a try/catch (inside openDb) and in the catch await/ call sqlite.close() (or
the appropriate close method on the returned sqlite) to ensure the DB handle is
released, then rethrow the original error; keep registry logic unchanged so
_registry.set(dbName, ...) only runs after successful open and pragmas.
| export async function getOrCreateDatabaseKey(dbName: string): Promise<string> { | ||
| const k = storageKey(dbName); | ||
| const existing = await _shim.getItem(k); | ||
| if (existing !== null) { | ||
| return existing; | ||
| } | ||
|
|
||
| // randomKey (not randomBytes): the mobile-crypto bridge encodes randomBytes as | ||
| // BASE64 on both platforms, while randomKey returns hex (SecureRandom/SecRandomCopyBytes | ||
| // + bytesToHex). The argument is the byte count: 32 bytes → 64 hex chars. | ||
| const hex = await randomKey(32); | ||
| // Guard the bridge contract: anything but 64 hex chars must not be used as a key | ||
| if (!/^[0-9a-fA-F]{64}$/.test(hex)) { | ||
| // Sanitize error — do not include the value in the message | ||
| throw new Error('key generation produced unexpected output; cannot open database safely'); | ||
| } | ||
|
|
||
| await _shim.setItem(k, hex); | ||
| return hex; |
There was a problem hiding this comment.
Serialize first-write key creation per database name.
Concurrent first calls can generate different keys for the same dbName; one caller may receive a key that is no longer persisted. That breaks idempotence and can cause sporadic encrypted-open failures.
Proposed fix
const KEY_PREFIX = 'db_key_v1:';
+const _inflight = new Map<string, Promise<string>>();
function storageKey(dbName: string): string {
return `${KEY_PREFIX}${dbName}`;
}
export async function getOrCreateDatabaseKey(dbName: string): Promise<string> {
const k = storageKey(dbName);
- const existing = await _shim.getItem(k);
- if (existing !== null) {
- return existing;
- }
-
- const hex = await randomKey(32);
- if (!/^[0-9a-fA-F]{64}$/.test(hex)) {
- throw new Error('key generation produced unexpected output; cannot open database safely');
- }
-
- await _shim.setItem(k, hex);
- return hex;
+ const pending = _inflight.get(k);
+ if (pending) {
+ return pending;
+ }
+
+ const task = (async (): Promise<string> => {
+ const existing = await _shim.getItem(k);
+ if (existing !== null) {
+ return existing;
+ }
+
+ const hex = await randomKey(32);
+ if (!/^[0-9a-fA-F]{64}$/.test(hex)) {
+ throw new Error('key generation produced unexpected output; cannot open database safely');
+ }
+
+ await _shim.setItem(k, hex);
+ return hex;
+ })();
+
+ _inflight.set(k, task);
+ try {
+ return await task;
+ } finally {
+ _inflight.delete(k);
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/lib/database/driver/keyService.ts` around lines 81 - 99, Concurrent
callers to getOrCreateDatabaseKey can race and persist different keys; serialize
first-write per dbName by introducing per-name in-flight serialization: add a
Map (e.g., pendingKeys) keyed by storageKey(dbName) that stores the Promise for
the ongoing creation, return that promise if present, otherwise create and store
the promise for the generator; inside the creation promise perform the existing
check via _shim.getItem(k), generate hex via randomKey(32), validate it, then
before setItem re-check _shim.getItem(k) and if it now exists return that value
instead of overwriting; finally call _shim.setItem(k, hex) and cleanup the
pendingKeys entry. Ensure all references use getOrCreateDatabaseKey, storageKey,
_shim.getItem, _shim.setItem, randomKey and that the pending map is cleared on
completion/error.
The SQLCipher-encrypted databases run in WAL mode inside the iOS App Group container. iOS kills a suspended app that holds a file lock on a file in a shared container unless it can recognise the WAL file as SQLite — but default SQLCipher encrypts the file header, so iOS denies the idle-WAL background exemption and the held shared lock trips RUNNINGBOARD 0xdead10cc on suspend. Open every database with PRAGMA cipher_plaintext_header_size = 32 so iOS reads the WAL magic and grants the exemption. With a plaintext header SQLCipher no longer stores the salt in the file, so generate and persist a per-database salt alongside the key and supply it via PRAGMA cipher_salt at open time. The 32 plaintext bytes are header metadata only (version/page size), so this is not a security regression. Losing the salt makes the DB unreadable, same as losing the key, so both are destroyed together.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/lib/database/driver/__tests__/keyService.test.ts (1)
115-127: ⚡ Quick winConsider adding a test for concurrent first-write race condition.
The tests verify idempotency under sequential calls, but don't exercise concurrent calls to
getOrCreateDatabaseKeyorgetOrCreateDatabaseSaltfor the samedbName. Adding a test that fires multiple parallel calls and asserts all receive the same value would both document the expected behavior and catch the race condition bug.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/database/driver/__tests__/keyService.test.ts` around lines 115 - 127, Add a new test case to verify concurrent first-write behavior for the getOrCreateDatabaseKey and getOrCreateDatabaseSalt functions. The test should fire multiple parallel calls using the same dbName parameter, and assert that all concurrent calls receive the same value back. This test will document the expected concurrent idempotency behavior and help catch potential race condition bugs when multiple async operations try to create the same database salt or key simultaneously.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/database/driver/keyService.ts`:
- Around line 122-137: The function getOrCreateDatabaseSalt has a race condition
where concurrent calls for the same dbName can each generate and store different
salts, causing data loss and permanent database corruption. Apply the same
serialization fix used for getOrCreateDatabaseKey to the getOrCreateDatabaseSalt
function by introducing a locking mechanism (such as a Map of pending promises)
that ensures only one salt generation and storage operation proceeds for each
unique dbName at a time. Alternatively, extract the
check-then-generate-then-store pattern into a shared helper function that both
getOrCreateDatabaseSalt and getOrCreateDatabaseKey can use to eliminate
duplication and ensure consistent race condition handling.
---
Nitpick comments:
In `@app/lib/database/driver/__tests__/keyService.test.ts`:
- Around line 115-127: Add a new test case to verify concurrent first-write
behavior for the getOrCreateDatabaseKey and getOrCreateDatabaseSalt functions.
The test should fire multiple parallel calls using the same dbName parameter,
and assert that all concurrent calls receive the same value back. This test will
document the expected concurrent idempotency behavior and help catch potential
race condition bugs when multiple async operations try to create the same
database salt or key simultaneously.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bae71627-1156-448a-94f9-c82d37ae9942
📒 Files selected for processing (4)
app/lib/database/driver/__tests__/connection.test.tsapp/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/connection.tsapp/lib/database/driver/keyService.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/lib/database/driver/tests/connection.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/database/driver/keyService.tsapp/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/connection.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/lib/database/driver/keyService.tsapp/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/connection.ts
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/lib/database/driver/keyService.tsapp/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/connection.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/lib/database/driver/keyService.tsapp/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/connection.ts
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/database/driver/keyService.tsapp/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/connection.ts
🔇 Additional comments (11)
app/lib/database/driver/keyService.ts (4)
94-112: Race condition in concurrent first-write remains unaddressed.The past review correctly identified that concurrent first calls to
getOrCreateDatabaseKeycan race: both callers pass theexisting !== nullcheck, both generate different keys viarandomKey, and one overwrites the other's persisted key. The caller that loses the race receives a key that is no longer stored, causing sporadic encrypted-open failures.
2-15: LGTM!
81-85: LGTM!
139-147: LGTM!app/lib/database/driver/connection.ts (4)
171-191: Resource leak whenapplyOpenPragmasfails remains unaddressed.The past review correctly identified that if
applyOpenPragmasthrows (e.g., open-verify fails due to wrong key/salt, or file corruption), thesqlitehandle fromopenDatabaseAsyncis never closed, leaking an open file descriptor.
12-25: LGTM!
43-43: LGTM!
137-164: LGTM!app/lib/database/driver/__tests__/keyService.test.ts (3)
15-27: LGTM!Also applies to: 46-47
91-113: LGTM!
146-157: LGTM!
| export async function getOrCreateDatabaseSalt(dbName: string): Promise<string> { | ||
| const k = storageKey(SALT_PREFIX, dbName); | ||
| const existing = await _shim.getItem(k); | ||
| if (existing !== null) { | ||
| return existing; | ||
| } | ||
|
|
||
| // 16 bytes → 32 hex chars; same hex-encoding contract as randomKey above. | ||
| const hex = await randomKey(16); | ||
| if (!/^[0-9a-fA-F]{32}$/.test(hex)) { | ||
| throw new Error('salt generation produced unexpected output; cannot open database safely'); | ||
| } | ||
|
|
||
| await _shim.setItem(k, hex); | ||
| return hex; | ||
| } |
There was a problem hiding this comment.
Same race condition applies to getOrCreateDatabaseSalt.
This function follows the identical check-then-generate-then-store pattern as getOrCreateDatabaseKey. Concurrent first calls for the same dbName can persist different salts, causing one caller to receive a salt that no longer matches the stored value. Since losing the salt makes the database permanently unreadable (as documented in lines 13-15), this race has severe consequences.
The serialization fix proposed for getOrCreateDatabaseKey should be applied to both functions (or extracted into a shared helper).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/lib/database/driver/keyService.ts` around lines 122 - 137, The function
getOrCreateDatabaseSalt has a race condition where concurrent calls for the same
dbName can each generate and store different salts, causing data loss and
permanent database corruption. Apply the same serialization fix used for
getOrCreateDatabaseKey to the getOrCreateDatabaseSalt function by introducing a
locking mechanism (such as a Map of pending promises) that ensures only one salt
generation and storage operation proceeds for each unique dbName at a time.
Alternatively, extract the check-then-generate-then-store pattern into a shared
helper function that both getOrCreateDatabaseSalt and getOrCreateDatabaseKey can
use to eliminate duplication and ensure consistent race condition handling.
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Review: driver adapter, key service, live-query hooks
Infrastructure-only PR (nothing imports it yet) and the design is sound — raw-key PRAGMA form is correct, PRAGMA ordering enforced, the plaintext-header + externalised-salt approach for the 0xdead10cc fix is well-reasoned, and the dev-shim production guard is a good rail. Findings below are all fixable before the facade cutover lands; the openDb race and the deriveServerDbName collision are the two worth prioritising.
Reviewed by an automated pass; treat inline comments as suggestions, not blockers.
|
|
||
| const [keyHex, saltHex] = await Promise.all([getOrCreateDatabaseKey(dbName), getOrCreateDatabaseSalt(dbName)]); | ||
|
|
||
| const sqlite = await openDatabaseAsync(dbName, { enableChangeListener: true }, DB_DIRECTORY); |
There was a problem hiding this comment.
🔴 [high] TOCTOU race in openDb
Two concurrent callers both miss the registry cache, both call openDatabaseAsync, and the second _registry.set silently overwrites the first. The first handle is orphaned while still holding a file lock, which can cause SQLITE_BUSY or database is locked on the next open.
Fix: use an in-flight promise registry so concurrent calls coalesce onto one open:
const _inflight = new Map<string, Promise<DbHandle>>();
async function openDb<K extends DbKind>(dbName: string, kind: K): Promise<DbHandle<K>> {
const cached = _registry.get(dbName);
if (cached) return cached as DbHandle<K>;
const existing = _inflight.get(dbName);
if (existing) return existing as Promise<DbHandle<K>>;
const promise = _openDbImpl(dbName, kind).finally(() => _inflight.delete(dbName));
_inflight.set(dbName, promise);
return promise;
}|
|
||
| const sqlite = await openDatabaseAsync(dbName, { enableChangeListener: true }, DB_DIRECTORY); | ||
|
|
||
| await applyOpenPragmas(sqlite, keyHex, saltHex); |
There was a problem hiding this comment.
🟡 [medium] Handle leak on PRAGMA failure
openDatabaseAsync succeeds (line 179), then applyOpenPragmas throws (line 181). The raw sqlite handle is never closed, so the file lock is held for the process lifetime. On repeated open attempts this accumulates leaked handles.
Fix: wrap the pragma sequence in a try/catch and close on failure before rethrowing:
const sqlite = await openDatabaseAsync(dbName, { enableChangeListener: true }, DB_DIRECTORY);
try {
await applyOpenPragmas(sqlite, keyHex, saltHex);
} catch (err) {
await sqlite.closeAsync().catch(() => {});
throw err;
}| const k = storageKey(KEY_PREFIX, dbName); | ||
| const existing = await _shim.getItem(k); | ||
| if (existing !== null) { | ||
| return existing; |
There was a problem hiding this comment.
🟡 [medium] No hex validation on keychain read-back
Fresh-generated keys are guarded by !/^[0-9a-fA-F]{64}$/ (line 107), but the loaded path returns the stored value immediately (line 98) without the same check. A tampered keychain entry on a jailbroken device would be interpolated verbatim into PRAGMA key = "x'<value>'", enabling PRAGMA string injection.
Fix: apply the same regex guard before returning the loaded value:
const existing = await _shim.getItem(k);
if (existing !== null) {
if (!/^[0-9a-fA-F]{64}$/.test(existing)) {
throw new Error('stored key failed format validation; keychain may be tampered');
}
return existing;
}Apply the same pattern in getOrCreateDatabaseSalt (32-hex guard).
| const sanitized = serverUrl | ||
| .replace(/\/+$/, '') | ||
| .replace(/(^\w+:|^)\/\//, '') | ||
| .replace(/\//g, '.'); |
There was a problem hiding this comment.
🟡 [medium] deriveServerDbName produces colliding filenames
.replace(/\//g, '.') maps both https://foo/bar and https://foo.bar to foo.bar.db. Two different servers share one encrypted SQLite file and one keychain key — a data isolation bug.
Fix: use a separator that can't appear in a hostname. Replace / with _ (hostnames cannot contain _), leaving . as the host-segment delimiter only:
export function deriveServerDbName(serverUrl: string): string {
const sanitized = serverUrl
.replace(/\/+$/, '')
.replace(/(^\w+:|^)\/\//, '')
.replace(/\//g, '_'); // _ cannot appear in a hostname
return `${sanitized}.db`;
}Ensure the keychain storage-key derivation in keyService.ts uses the same function so the key and the file always agree.
| if (!tableSet.has(event.tableName)) return; | ||
| // Debounce: coalesce all events from a single large transaction into one re-query | ||
| if (timerRef.current !== null) clearTimeout(timerRef.current); | ||
| timerRef.current = setTimeout(fetchAndReconcile, debounceMs); |
There was a problem hiding this comment.
🟢 [low] queryFn throw inside setTimeout is unhandled
fetchAndReconcile (which calls queryFnRef.current()) is scheduled via setTimeout. A throw inside it becomes an uncaught exception on the timer thread — React error boundaries cannot catch it, and it surfaces as an unhandled promise rejection or a native crash in some RN versions.
Fix: wrap the timer callback:
timerRef.current = setTimeout(() => {
try {
fetchAndReconcile();
} catch (err) {
// surface via error state so React boundaries can catch it
setRows(() => { throw err; });
}
}, debounceMs);
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Structural review (NATIVE-1274). The hand-rolled reactivity (table-filtered listener + ~16ms debounce + structural sharing) is sound, the stable-ref pattern correctly avoids the stale-closure bug, PRAGMA key ordering and the x'<hex>' form are correct, and key material never reaches logs or thrown errors. Findings inline.
Test gap (low): no test asserts sub.remove() runs on unmount and that a post-unmount change event does not re-query — the most common hook-leak failure, and pinning it would let the dead mounted ref (flagged inline) be removed with confidence.
| * Returns the same handle on repeated calls for the same name. | ||
| */ | ||
| async function openDb<K extends DbKind>(dbName: string, kind: K): Promise<DbHandle<K>> { | ||
| const cached = _registry.get(dbName); |
There was a problem hiding this comment.
bug: high — concurrent-open race leaks a handle. The _registry.get check (here) and _registry.set (~line 189) are separated by several awaits (keychain x2, openDatabaseAsync, the PRAGMA round-trips). Two concurrent callers (e.g. two sagas opening the same server DB on cold boot) both see no cached handle, both run the full open, and the second set overwrites the first — leaving an orphaned open SQLCipher connection that is never closed. Track in-flight opens in a Map<string, Promise<DbHandle>> and return the pending promise on a second concurrent call.
| export async function getOrCreateDatabaseKey(dbName: string): Promise<string> { | ||
| const k = storageKey(KEY_PREFIX, dbName); | ||
| const existing = await _shim.getItem(k); | ||
| if (existing !== null) { |
There was a problem hiding this comment.
bug: low — stored key returned without re-validation. The freshly-generated hex is regex-checked before storing, but on a cache hit existing is returned unconditionally and fed straight into PRAGMA key. A future shim bug (truncation/encoding) would surface as the misleading "key may be wrong or file corrupt" verify error. Re-run the same regex on existing; on failure throw a distinct "stored key corrupt" error (with no key material).
| rowId: number | null | undefined, | ||
| fetchRow: FetchRowFn<T> | ||
| ): T | null { | ||
| const [row, setRow] = useState<T | null>(() => { |
There was a problem hiding this comment.
bug: low — initial row fetched twice on mount. The useState lazy initializer runs fetchRow(rowId) synchronously, then the effect immediately calls setRow(fetchRowRef.current(rowId)) again before any change event. Drop the lazy initializer and default to null; the effect already handles the initial load.
| * The key is 32 bytes (256-bit) from the platform CSPRNG, hex-encoded to 64 chars. | ||
| * It is never logged, never included in thrown errors, never sent to telemetry. | ||
| */ | ||
| export async function getOrCreateDatabaseKey(dbName: string): Promise<string> { |
There was a problem hiding this comment.
slop: moderate — getOrCreateDatabaseKey / getOrCreateDatabaseSalt are near-duplicates. They differ only by prefix, byte count, expected hex length and one error string (~45 lines duplicated). Extract a private getOrCreate(prefix, dbName, bytes) helper; both public functions become one-liners.
| async function applyOpenPragmas(sqlite: SQLiteDatabase, keyHex: string, saltHex: string): Promise<void> { | ||
| // 1. Key MUST be first — before any schema read/write | ||
| // Raw-key form: the x'...' prefix tells SQLCipher to skip PBKDF2 | ||
| await sqlite.execAsync(`PRAGMA key = "x'${keyHex}'";`); |
There was a problem hiding this comment.
slop: moderate — five sequential execAsync PRAGMA calls. execAsync accepts a semicolon-separated multi-statement string; the four config pragmas (key through WAL) can be one call, with only the getFirstAsync verify separate. Fewer round-trips and it makes the "configure before any schema access" intent atomic and explicit.
| }; | ||
| // deps are caller-controlled; tables/debounceMs treated as stable across renders | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [dbHandle, fetchAndReconcile, ...deps]); |
There was a problem hiding this comment.
slop: moderate — ...deps spread blinds the hooks linter to tables. tables is closed over inside the effect but isn't in the literal deps array, so if a caller's table set changes without threading it through deps the subscription silently watches the stale set — and the eslint-disable hides it. Derive a stable tableKey from tables and put it in the deps array so re-subscription is automatic and lint-visible.
| return fetchRow(rowId); | ||
| }); | ||
|
|
||
| const mounted = useRef(true); |
There was a problem hiding this comment.
slop: nit — mounted ref is dead code. The listener is registered in an effect whose cleanup calls sub.remove(); in JS's single-threaded model the listener can't fire after removal, so the if (!mounted.current) return guard is unreachable. Remove the ref and its effect.
| * After calling this, the database file is permanently inaccessible. | ||
| */ | ||
| export async function deleteDatabaseKey(dbName: string): Promise<void> { | ||
| await _shim.removeItem(storageKey(KEY_PREFIX, dbName)); |
There was a problem hiding this comment.
slop: nit — key and salt removed sequentially. The two removeItem calls are independent; Promise.all halves the round-trips.
1 participant
Proposed changes
Second step of the WatermelonDB → expo-sqlite + Drizzle migration, stacked on the schema PR (#7395): adds the encrypted database driver layer. No runtime behavior changes — nothing in the app imports these modules yet; integration arrives with the data-access facade work.
app/lib/database/driver/connection.ts):PRAGMA keyfirst (raw-keyx'<64-hex>'form, skipping PBKDF2 since keys are already full-entropy CSPRNG output), thenPRAGMA busy_timeout = 500(required for multi-process WAL safety with the iOS notification service extension), thenPRAGMA journal_mode = WAL, then an open-verify read that throws a key-free error on failure.group.ios.chat.rocketApp Group container (shared with extensions), with a logged fallback to the default directory; Android uses the default location..dbfilenames from server URLs (drops the legacy.db.dbdouble suffix) and caches handles in a registry so each file opens once.app/lib/database/driver/keyService.ts):randomKeyfrom@rocket.chat/mobile-crypto— chosen overrandomBytes, whose bridge encoding is base64 on both platforms — with a format guard before any key is used.IKeychainShiminterface; the real Keychain/Keystore backend lands with the native-readers work. The built-in in-memory stand-in is dev-only and throws outside__DEV__, because silently using it in production would create databases whose keys vanish on restart (permanent data loss).app/lib/database/driver/observe.ts), patterns validated in the live-query proof of concept:useTableQuery— table-filtered change listener with a 16ms debounce (expo-sqlite fires one event per row, so large transactions must coalesce into one re-query) and structural sharing so unchanged rows keep their object identity andReact.memobails out.useRowObserve— per-rowid subscription for hot single-row sites.addDatabaseChangeListeneris global across all open databases and the default and per-server databases share table names (e.g.users).expo.sqlite.useSQLCipherflag inios/Podfile.properties.jsonandandroid/gradle.properties; addsexpo-sqlite ~16.0.10.Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1274
How to test or reproduce
TZ=UTC pnpm test app/lib/database/driver— runs the driver, key-service, and live-query hook tests.pnpm exec tsc --noEmit -p .— type-checks the new modules.TZ=UTC pnpm test(180 suites / 1576 tests passing locally).Screenshots
N/A — no UI changes.
Types of changes
Checklist
Further comments
Stacked on #7395 — merge that first; GitHub will retarget this PR to develop automatically. The native Keychain/Keystore key-storage backend and the native database readers (iOS notification service extension, Android notification path) come in a follow-up, which is why key persistence sits behind the
IKeychainShiminterface here.Summary by CodeRabbit