Skip to content

Commit

Permalink
Stop including snap state in controller state (#876)
Browse files Browse the repository at this point in the history 
* Stop including snap state in controller state

* Improve typing slightly

* Simplify tests

* Fix PR comments

* coverage
  • Loading branch information
@FrederikBolding
FrederikBolding committed Oct 31, 2022
1 parent bf0807d commit 15212ab
Show file tree
Hide file tree
Showing 3 changed files with 78 additions and 73 deletions.
6 changes: 3 additions & 3 deletions packages/controllers/jest.config.js
View file
Expand Up @@ -8,10 +8,10 @@ module.exports = {
coveragePathIgnorePatterns: ['/node_modules/', '/mocks/', '/test/'],
coverageThreshold: {
global: {
branches: 84.63,
branches: 84.8,
functions: 95.8,
lines: 94.93,
statements: 95.03,
lines: 94.94,
statements: 95.04,
},
},
projects: [
Expand Down
97 changes: 41 additions & 56 deletions packages/controllers/src/snaps/SnapController.test.ts
View file
Expand Up @@ -93,12 +93,12 @@ describe('SnapController', () => {
await snapController.updateSnapState(snap.id, state);
const snapState = await snapController.getSnapState(snap.id);
expect(snapState).toStrictEqual(state);
expect(snapController.state.snapStates).toStrictEqual({
[MOCK_SNAP_ID]: await passworder.encrypt(
`stateEncryption:${MOCK_SNAP_ID}`,
state,
),
});
expect(
// @ts-expect-error Accessing private property
snapController._snapsRuntimeData.get(MOCK_SNAP_ID).state,
).toStrictEqual(
await passworder.encrypt(`stateEncryption:${MOCK_SNAP_ID}`, state),
);
snapController.destroy();
await service.terminateAllSnaps();
});
Expand Down Expand Up @@ -3569,15 +3569,18 @@ describe('SnapController', () => {
fizz: 'buzz',
};
const encrypted = await passworder.encrypt(
'stateEncryption:npm:fooSnap',
`stateEncryption:${MOCK_SNAP_ID}`,
state,
);
const snapController = getSnapController(
getSnapControllerOptions({
messenger,
state: {
snaps: {
[MOCK_SNAP_ID]: getPersistedSnapObject(),
},
snapStates: {
'npm:fooSnap': encrypted,
[MOCK_SNAP_ID]: encrypted,
},
},
}),
Expand All @@ -3586,7 +3589,7 @@ describe('SnapController', () => {
const getSnapStateSpy = jest.spyOn(snapController, 'getSnapState');
const result = await messenger.call(
'SnapController:getSnapState',
'npm:fooSnap',
MOCK_SNAP_ID,
);

expect(getSnapStateSpy).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -3657,17 +3660,7 @@ describe('SnapController', () => {
getSnapControllerOptions({
messenger,
state: {
snaps: getPersistedSnapsState(
getPersistedSnapObject({
permissionName: 'fooperm',
version: '0.0.1',
sourceCode: DEFAULT_SNAP_BUNDLE,
id: 'npm:fooSnap',
manifest: getSnapManifest(),
enabled: true,
status: SnapStatus.Installing,
}),
),
snaps: getPersistedSnapsState(),
},
}),
);
Expand All @@ -3678,17 +3671,17 @@ describe('SnapController', () => {
};
await messenger.call(
'SnapController:updateSnapState',
'npm:fooSnap',
MOCK_SNAP_ID,
state,
);

expect(updateSnapStateSpy).toHaveBeenCalledTimes(1);
expect(snapController.state.snapStates).toStrictEqual({
'npm:fooSnap': await passworder.encrypt(
'stateEncryption:npm:fooSnap',
state,
),
});
expect(
// @ts-expect-error Accessing private property
snapController._snapsRuntimeData.get(MOCK_SNAP_ID).state,
).toStrictEqual(
await passworder.encrypt(`stateEncryption:${MOCK_SNAP_ID}`, state),
);
});

it('has different encryption for the same data stored by two different snaps', async () => {
Expand All @@ -3699,23 +3692,9 @@ describe('SnapController', () => {
messenger,
state: {
snaps: getPersistedSnapsState(
getPersistedSnapObject(),
getPersistedSnapObject({
permissionName: 'fooperm',
version: '0.0.1',
sourceCode: DEFAULT_SNAP_BUNDLE,
id: 'npm:fooSnap',
manifest: getSnapManifest(),
enabled: true,
status: SnapStatus.Installing,
}),
getPersistedSnapObject({
permissionName: 'fooperm2',
version: '0.0.1',
sourceCode: DEFAULT_SNAP_BUNDLE,
id: 'npm:fooSnap2',
manifest: getSnapManifest(),
enabled: true,
status: SnapStatus.Installing,
id: MOCK_LOCAL_SNAP_ID,
}),
),
},
Expand All @@ -3728,31 +3707,37 @@ describe('SnapController', () => {
};
await messenger.call(
'SnapController:updateSnapState',
'npm:fooSnap',
MOCK_SNAP_ID,
state,
);

await messenger.call(
'SnapController:updateSnapState',
'npm:fooSnap2',
MOCK_LOCAL_SNAP_ID,
state,
);

expect(updateSnapStateSpy).toHaveBeenCalledTimes(2);
expect(snapController.state.snapStates).toStrictEqual({
'npm:fooSnap': await passworder.encrypt(
'stateEncryption:npm:fooSnap',
state,
),
'npm:fooSnap2': await passworder.encrypt(
'stateEncryption:npm:fooSnap2',
const snapState1 =
// @ts-expect-error Accessing private property
snapController._snapsRuntimeData.get(MOCK_SNAP_ID).state;

const snapState2 =
// @ts-expect-error Accessing private property
snapController._snapsRuntimeData.get(MOCK_LOCAL_SNAP_ID).state;

expect(snapState1).toStrictEqual(
await passworder.encrypt(`stateEncryption:${MOCK_SNAP_ID}`, state),
);

expect(snapState2).toStrictEqual(
await passworder.encrypt(
`stateEncryption:${MOCK_LOCAL_SNAP_ID}`,
state,
),
});

expect(snapController.state.snapStates['npm:fooSnap']).not.toStrictEqual(
snapController.state.snapStates['npm:fooSnap2'],
);

expect(snapState1).not.toStrictEqual(snapState2);
});
});

Expand Down
48 changes: 34 additions & 14 deletions packages/controllers/src/snaps/SnapController.ts
View file
Expand Up @@ -154,6 +154,11 @@ export interface SnapRuntimeData {
* The snap source code
*/
sourceCode: null | string;

/**
* The snap state (encrypted)
*/
state: null | string;
}

export type SnapError = {
Expand Down Expand Up @@ -188,14 +193,17 @@ type StoredSnaps = Record<SnapId, Snap>;

export type SnapControllerState = {
snaps: StoredSnaps;
snapStates: Record<SnapId, string>;
// This type needs to be defined but is always empty in practice.
// eslint-disable-next-line @typescript-eslint/ban-types
snapStates: {};
snapErrors: {
[internalID: string]: SnapError & { internalID: string };
};
};

export type PersistedSnapControllerState = SnapControllerState & {
snaps: Record<SnapId, PersistedSnap>;
snapStates: Record<SnapId, string>;
};

// Controller Messenger Actions
Expand Down Expand Up @@ -642,6 +650,7 @@ export class SnapController extends BaseController<
},
{},
),
snapStates: {},
};
super({
messenger,
Expand All @@ -651,7 +660,14 @@ export class SnapController extends BaseController<
anonymous: false,
},
snapStates: {
persist: true,
persist: () => {
return Object.keys(this.state.snaps).reduce<
Record<string, string | null>
>((acc, cur) => {
acc[cur] = this.getRuntimeExpect(cur).state;
return acc;
}, {});
},
anonymous: false,
},
snaps: {
Expand Down Expand Up @@ -713,8 +729,11 @@ export class SnapController extends BaseController<
this.initializeStateMachine();
this.registerMessageHandlers();

Object.entries(loadedSourceCode).forEach(([id, sourceCode]) =>
this.setupRuntime(id, sourceCode),
Object.keys(filteredState.snaps).forEach((id) =>
this.setupRuntime(id, {
sourceCode: loadedSourceCode[id],
state: state?.snapStates?.[id] ?? null,
}),
);
}

Expand Down Expand Up @@ -1257,9 +1276,8 @@ export class SnapController extends BaseController<
*/
async updateSnapState(snapId: SnapId, newSnapState: Json): Promise<void> {
const encrypted = await this.encryptSnapState(snapId, newSnapState);
this.update((state: any) => {
state.snapStates[snapId] = encrypted;
});
const runtime = this.getRuntimeExpect(snapId);
runtime.state = encrypted;
}

/**
Expand All @@ -1269,9 +1287,8 @@ export class SnapController extends BaseController<
* @param snapId - The id of the Snap whose state should be cleared.
*/
async clearSnapState(snapId: SnapId): Promise<void> {
this.update((state: any) => {
delete state.snapStates[snapId];
});
const runtime = this.getRuntimeExpect(snapId);
runtime.state = null;
}

/**
Expand Down Expand Up @@ -1319,7 +1336,7 @@ export class SnapController extends BaseController<
* @throws If the snap state decryption fails.
*/
async getSnapState(snapId: SnapId): Promise<Json> {
const state = this.state.snapStates[snapId];
const { state } = this.getRuntimeExpect(snapId);
return state ? this.decryptSnapState(snapId, state) : null;
}

Expand Down Expand Up @@ -1779,7 +1796,7 @@ export class SnapController extends BaseController<
) {
throw new Error(`Invalid add snap args for snap "${snapId}".`);
}
this.setupRuntime(snapId, null);
this.setupRuntime(snapId, { sourceCode: null, state: null });
const runtime = this.getRuntimeExpect(snapId);
if (!runtime.installPromise) {
console.info(`Adding snap: ${snapId}`);
Expand Down Expand Up @@ -2387,7 +2404,10 @@ export class SnapController extends BaseController<
return runtime;
}

private setupRuntime(snapId: SnapId, sourceCode: string | null) {
private setupRuntime(
snapId: SnapId,
data: { sourceCode: string | null; state: string | null },
) {
if (this._snapsRuntimeData.has(snapId)) {
return;
}
Expand All @@ -2411,7 +2431,7 @@ export class SnapController extends BaseController<
pendingInboundRequests: [],
pendingOutboundRequests: 0,
interpreter,
sourceCode,
...data,
});
}

Expand Down

0 comments on commit 15212ab

Please sign in to comment.