Skip to content

refactor(wallet-cli): Replace { wallet, store } with { wallet, dispose } #8847

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.

Sign up for GitHub

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

Draft
sirtimid wants to merge 1 commit into
base: rekm/wallet-cli
Choose a base branch
from
+272 −142
Draft

refactor(wallet-cli): Replace { wallet, store } with { wallet, dispose } #8847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 11 additions & 90 deletions packages/wallet-cli/src/daemon/daemon-entry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ function enoent(): NodeJS.ErrnoException {
}

/**
* Create a mock createWallet result with a mocked wallet and store.
* Create a mock createWallet result with a mocked wallet and dispose callback.
*
* @returns A mock createWallet result.
*/
Expand All @@ -66,9 +66,7 @@ function createMockWallet(): MockCreateWalletResult {
state: {},
destroy: jest.fn().mockResolvedValue(undefined),
},
store: {
close: jest.fn(),
},
dispose: jest.fn().mockResolvedValue(undefined),
} as unknown as MockCreateWalletResult;
}

Expand Down Expand Up @@ -306,7 +304,7 @@ describe('daemon-entry', () => {
);
});

it('cleans up wallet, store, and PID file when server fails to start', async () => {
it('disposes the wallet and removes the PID file when server fails to start', async () => {
const result = createMockWallet();
mockCreateWallet.mockResolvedValue(result);
mockStartRpcSocketServer.mockRejectedValue(new Error('server failed'));
Expand All @@ -321,8 +319,7 @@ describe('daemon-entry', () => {

await importDaemonEntry();

expect(result.wallet.destroy).toHaveBeenCalled();
expect(result.store.close).toHaveBeenCalled();
expect(result.dispose).toHaveBeenCalledTimes(1);
expect(mockRm).toHaveBeenCalledWith('/tmp/daemon.pid', { force: true });
expect(process.exitCode).toBe(1);
});
Expand Down Expand Up @@ -431,37 +428,6 @@ describe('daemon-entry', () => {
expect(process.exitCode).toBe(1);
});

it('still cleans up wallet/store when wallet.destroy fails during error cleanup', async () => {
const result = createMockWallet();
(result.wallet.destroy as jest.Mock).mockRejectedValue(
new Error('destroy failed'),
);
mockCreateWallet.mockResolvedValue(result);
mockStartRpcSocketServer.mockRejectedValue(new Error('server failed'));

await importDaemonEntry();

expect(result.store.close).toHaveBeenCalled();
expect(process.exitCode).toBe(1);
});

it('logs and continues when store.close throws during error cleanup', async () => {
const result = createMockWallet();
(result.store.close as jest.Mock).mockImplementation(() => {
throw new Error('close failed');
});
mockCreateWallet.mockResolvedValue(result);
mockStartRpcSocketServer.mockRejectedValue(new Error('server failed'));

await importDaemonEntry();

expect(mockAppendFile).toHaveBeenCalledWith(
'/tmp/daemon.log',
expect.stringContaining('store.close() failed during cleanup'),
);
expect(process.exitCode).toBe(1);
});

it('logs and continues when ownership-aware PID removal throws during error cleanup', async () => {
const result = createMockWallet();
mockCreateWallet.mockResolvedValue(result);
Expand Down Expand Up @@ -559,8 +525,7 @@ describe('daemon-entry', () => {
}

expect(handle.close).toHaveBeenCalled();
expect(result.wallet.destroy).toHaveBeenCalled();
expect(result.store.close).toHaveBeenCalled();
expect(result.dispose).toHaveBeenCalledTimes(1);
});

it('triggers shutdown when SIGINT handler is called', async () => {
Expand All @@ -582,11 +547,10 @@ describe('daemon-entry', () => {
}

expect(handle.close).toHaveBeenCalled();
expect(result.wallet.destroy).toHaveBeenCalled();
expect(result.store.close).toHaveBeenCalled();
expect(result.dispose).toHaveBeenCalledTimes(1);
});

it('shutdown still calls wallet.destroy when handle.close fails', async () => {
it('shutdown still disposes the wallet when handle.close fails', async () => {
const result = createMockWallet();
mockCreateWallet.mockResolvedValue(result);
const handle = createMockHandle();
Expand All @@ -599,55 +563,13 @@ describe('daemon-entry', () => {
const onShutdown = callArgs.onShutdown as () => Promise<void>;
await onShutdown();

expect(result.wallet.destroy).toHaveBeenCalled();
expect(result.dispose).toHaveBeenCalledTimes(1);
expect(mockAppendFile).toHaveBeenCalledWith(
'/tmp/daemon.log',
expect.stringContaining('handle.close() failed'),
);
});

it('shutdown logs wallet.destroy failure', async () => {
const result = createMockWallet();
(result.wallet.destroy as jest.Mock).mockRejectedValue(
new Error('destroy failed'),
);
mockCreateWallet.mockResolvedValue(result);
const handle = createMockHandle();
mockStartRpcSocketServer.mockResolvedValue(handle);

await importDaemonEntry();

const callArgs = mockStartRpcSocketServer.mock.calls[0][0];
const onShutdown = callArgs.onShutdown as () => Promise<void>;
await onShutdown();

expect(mockAppendFile).toHaveBeenCalledWith(
'/tmp/daemon.log',
expect.stringContaining('wallet.destroy() failed'),
);
});

it('shutdown logs store.close failure', async () => {
const result = createMockWallet();
(result.store.close as jest.Mock).mockImplementation(() => {
throw new Error('close failed');
});
mockCreateWallet.mockResolvedValue(result);
const handle = createMockHandle();
mockStartRpcSocketServer.mockResolvedValue(handle);

await importDaemonEntry();

const callArgs = mockStartRpcSocketServer.mock.calls[0][0];
const onShutdown = callArgs.onShutdown as () => Promise<void>;
await onShutdown();

expect(mockAppendFile).toHaveBeenCalledWith(
'/tmp/daemon.log',
expect.stringContaining('store.close() failed'),
);
});

it('handles rm rejection during shutdown cleanup gracefully', async () => {
const result = createMockWallet();
mockCreateWallet.mockResolvedValue(result);
Expand Down Expand Up @@ -675,7 +597,7 @@ describe('daemon-entry', () => {
await onShutdown();

expect(handle.close).toHaveBeenCalled();
expect(result.wallet.destroy).toHaveBeenCalled();
expect(result.dispose).toHaveBeenCalledTimes(1);
expect(mockAppendFile).toHaveBeenCalledWith(
'/tmp/daemon.log',
expect.stringContaining('Failed to remove socket file'),
Expand All @@ -692,7 +614,7 @@ describe('daemon-entry', () => {
expect(process.exitCode).toBe(1);
});

it('onShutdown closes server and destroys wallet', async () => {
it('onShutdown closes server and disposes the wallet', async () => {
const result = createMockWallet();
mockCreateWallet.mockResolvedValue(result);
const handle = createMockHandle();
Expand All @@ -713,8 +635,7 @@ describe('daemon-entry', () => {
await onShutdown();

expect(handle.close).toHaveBeenCalled();
expect(result.wallet.destroy).toHaveBeenCalled();
expect(result.store.close).toHaveBeenCalled();
expect(result.dispose).toHaveBeenCalledTimes(1);
expect(mockRm).toHaveBeenCalledWith('/tmp/daemon.pid', { force: true });
});

Expand Down
34 changes: 6 additions & 28 deletions packages/wallet-cli/src/daemon/daemon-entry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import type { Wallet } from '@metamask/wallet';
import { mkdirSync } from 'node:fs';
import { appendFile, chmod, readFile, rm, writeFile } from 'node:fs/promises';

import type { KeyValueStore } from '../persistence/KeyValueStore';
import { pingDaemon } from './daemon-client';
import { getDaemonPaths } from './paths';
import { startRpcSocketServer } from './rpc-socket-server';
Expand Down Expand Up @@ -88,11 +87,11 @@ async function main(): Promise<void> {
}

let wallet: Wallet | undefined;
let store: KeyValueStore | undefined;
let dispose: (() => Promise<void>) | undefined;
let handle: RpcSocketServerHandle | undefined;

try {
({ wallet, store } = await createWallet({
({ wallet, dispose } = await createWallet({
databasePath: dbPath,
infuraProjectId,
password,
Expand Down Expand Up @@ -135,19 +134,8 @@ async function main(): Promise<void> {
// synchronously, so this runs before any client can connect.
await chmod(socketPath, 0o600);
} catch (error) {
if (wallet) {
try {
await wallet.destroy();
} catch (destroyError) {
log(`wallet.destroy() failed during cleanup: ${String(destroyError)}`);
}
}
if (store) {
try {
store.close();
} catch (closeError) {
log(`store.close() failed during cleanup: ${String(closeError)}`);
}
if (dispose) {
await dispose();
}
// Only remove the PID file if it's still ours (we may have lost the race
// and the file now belongs to another daemon).
Expand All @@ -162,8 +150,7 @@ async function main(): Promise<void> {
// Capture the now-resolved bindings so the shutdown closures below have
// a stable, non-undefined reference (TS narrowing across closure escape).
const activeHandle = handle;
const activeWallet = wallet;
const activeStore = store;
const activeDispose = dispose;

log(`Daemon started. Socket: ${socketPath}`);

Expand All @@ -184,16 +171,7 @@ async function main(): Promise<void> {
} catch (closeError) {
log(`handle.close() failed: ${String(closeError)}`);
}
try {
await activeWallet.destroy();
} catch (destroyError) {
log(`wallet.destroy() failed: ${String(destroyError)}`);
}
try {
activeStore.close();
} catch (closeError) {
log(`store.close() failed: ${String(closeError)}`);
}
await activeDispose();
await Promise.all([
removeOwnedPidFile(pidPath, pidFileContents).catch(
(rmError: unknown) => {
Expand Down
Loading
Loading