Skip to content
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.

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

feat: Implement interface persistence #2856

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
43ce993
implement interface persistence
hmalik88 Oct 23, 2024
d598379
Merge branch 'main' into hm/implement-interface-persistence
hmalik88 Oct 23, 2024
7335c30
update coverage
hmalik88 Oct 23, 2024
0a19daf
test fixes
hmalik88 Oct 23, 2024
5c4f5b8
update coverage
hmalik88 Oct 23, 2024
bf86a94
update params
hmalik88 Oct 23, 2024
5fd08ae
update more types
hmalik88 Oct 23, 2024
4c84900
fix tests
hmalik88 Oct 23, 2024
6f40593
fix tests
hmalik88 Oct 23, 2024
e5b4551
address PR comments
hmalik88 Oct 24, 2024
1afed94
Merge branch 'main' into hm/implement-interface-persistence
hmalik88 Oct 24, 2024
7a516fb
Merge branch 'main' into hm/implement-interface-persistence
hmalik88 Oct 25, 2024
2ab88f0
update coverage
hmalik88 Oct 25, 2024
e005581
add logic to cleanup notification interfaces
hmalik88 Oct 25, 2024
5cbc1a7
update tests
hmalik88 Oct 28, 2024
686b1fa
update yarn.lock
hmalik88 Oct 28, 2024
3d89f80
add ts expect error
hmalik88 Oct 28, 2024
305980c
fix coverage and add missing event
hmalik88 Oct 28, 2024
58fce06
add missing event
hmalik88 Oct 28, 2024
4dc542d
update coverage
hmalik88 Oct 28, 2024
c988b19
fix more tests
hmalik88 Oct 28, 2024
89d529c
fix coverage again
hmalik88 Oct 28, 2024
47545fc
Merge branch 'main' into hm/implement-interface-persistence
hmalik88 Oct 31, 2024
a90208a
use weakmap to maintain function reference
hmalik88 Nov 1, 2024
291af42
Merge branch 'main' into hm/implement-interface-persistence
hmalik88 Nov 1, 2024
d361570
update coverage
hmalik88 Nov 1, 2024
e05f6f0
rebuild
hmalik88 Nov 1, 2024
0c5bd6d
address PR comments
hmalik88 Nov 6, 2024
c599507
update coverage
hmalik88 Nov 6, 2024
c6bfb69
update coverage again
hmalik88 Nov 6, 2024
c130124
update types
hmalik88 Nov 7, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"url": "https://github.com/MetaMask/snaps.git"
},
"source": {
"shasum": "zA6fni0b6B+ELhkMRiw6vcAgKj1/9A/Rm+dxkPY/oxM=",
"shasum": "27qYDxM1vuhrXntxwo8v6GE52cJ7OsaIYo51Q4p3gq0=",
"location": {
"npm": {
"filePath": "dist/bundle.js",
Expand Down
2 changes: 1 addition & 1 deletion packages/examples/packages/browserify/snap.manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"url": "https://github.com/MetaMask/snaps.git"
},
"source": {
"shasum": "diJJzn5l3lSAKQvHldlYmQXjTcO/IDnLOnxH7kGmkW0=",
"shasum": "oT2RHVQmSMHq87FYqFRPXqjERd+gOrI0RKjwNWDbSWY=",
"location": {
"npm": {
"filePath": "dist/bundle.js",
Expand Down
8 changes: 4 additions & 4 deletions packages/snaps-controllers/coverage.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"branches": 92.73,
"functions": 96.65,
"lines": 97.99,
"statements": 97.69
"branches": 92.65,
"functions": 96.7,
"lines": 97.95,
"statements": 97.65
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
import { getPersistentState } from '@metamask/base-controller';
import type { SnapId } from '@metamask/snaps-sdk';
import { form, image, input, panel, text } from '@metamask/snaps-sdk';
import {
form,
image,
input,
panel,
text,
ContentType,
} from '@metamask/snaps-sdk';
import {
Box,
Field,
Expand Down Expand Up @@ -29,6 +37,40 @@ jest.mock('@metamask/snaps-utils', () => ({
}));

describe('SnapInterfaceController', () => {
describe('constructor', () => {
it('persists notification interfaces', () => {
const rootMessenger = getRootSnapInterfaceControllerMessenger();
const controllerMessenger =
getRestrictedSnapInterfaceControllerMessenger(rootMessenger);

const controller = new SnapInterfaceController({
messenger: controllerMessenger,
state: {
interfaces: {
// @ts-expect-error missing properties
'1': {
contentType: ContentType.Notification,
},
// @ts-expect-error missing properties
'2': {
contentType: ContentType.Dialog,
},
},
},
});

expect(
getPersistentState(controller.state, controller.metadata),
).toStrictEqual({
interfaces: {
'1': {
contentType: ContentType.Notification,
},
},
});
});
});

describe('createInterface', () => {
it('can create a new interface', async () => {
const rootMessenger = getRootSnapInterfaceControllerMessenger();
Expand Down Expand Up @@ -160,6 +202,41 @@ describe('SnapInterfaceController', () => {
expect(context).toStrictEqual({ foo: 'bar' });
});

it('supports providing an interface content type', async () => {
const rootMessenger = getRootSnapInterfaceControllerMessenger();
const controllerMessenger =
getRestrictedSnapInterfaceControllerMessenger(rootMessenger);

/* eslint-disable-next-line no-new */
new SnapInterfaceController({
messenger: controllerMessenger,
});

const element = (
<Box>
<Text>
<Link href="https://foo.bar">foo</Link>
</Text>
</Box>
);

const id = await rootMessenger.call(
'SnapInterfaceController:createInterface',
MOCK_SNAP_ID,
element,
{ foo: 'bar' },
ContentType.Notification,
);

const { contentType } = rootMessenger.call(
'SnapInterfaceController:getInterface',
MOCK_SNAP_ID,
id,
);

expect(contentType).toStrictEqual(ContentType.Notification);
});

it('throws if interface context is too large', async () => {
const rootMessenger = getRootSnapInterfaceControllerMessenger();
const controllerMessenger =
Expand Down Expand Up @@ -1068,6 +1145,51 @@ describe('SnapInterfaceController', () => {
});
});

describe('updateInterfaceContentType', () => {
it("can update an interface's content type", async () => {
const rootMessenger = getRootSnapInterfaceControllerMessenger();
const controllerMessenger =
getRestrictedSnapInterfaceControllerMessenger(rootMessenger);

/* eslint-disable-next-line no-new */
new SnapInterfaceController({
messenger: controllerMessenger,
});

const content = form({ name: 'foo', children: [input({ name: 'bar' })] });

let contentType;

const id = await rootMessenger.call(
'SnapInterfaceController:createInterface',
MOCK_SNAP_ID,
content,
);

contentType = rootMessenger.call(
'SnapInterfaceController:getInterface',
MOCK_SNAP_ID,
id,
).contentType;

expect(contentType).toBeNull();

rootMessenger.call(
'SnapInterfaceController:updateInterfaceContentType',
id,
ContentType.Dialog,
);

contentType = rootMessenger.call(
'SnapInterfaceController:getInterface',
MOCK_SNAP_ID,
id,
).contentType;

expect(contentType).toStrictEqual(ContentType.Dialog);
});
});

describe('resolveInterface', () => {
it('resolves the interface with the given value', async () => {
const rootMessenger = getRootSnapInterfaceControllerMessenger();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
ComponentOrElement,
InterfaceContext,
} from '@metamask/snaps-sdk';
import { ContentType } from '@metamask/snaps-sdk';
import type { JSXElement } from '@metamask/snaps-sdk/jsx';
import { getJsonSizeUnsafe, validateJsxLinks } from '@metamask/snaps-utils';
import type { Json } from '@metamask/utils';
Expand Down Expand Up @@ -61,6 +62,11 @@
handler: SnapInterfaceController['updateInterfaceState'];
};

export type UpdateInterfaceContentType = {
type: `${typeof controllerName}:updateInterfaceContentType`;
handler: SnapInterfaceController['updateInterfaceContentType'];
};

export type ResolveInterface = {
type: `${typeof controllerName}:resolveInterface`;
handler: SnapInterfaceController['resolveInterface'];
Expand All @@ -84,6 +90,7 @@
| UpdateInterface
| DeleteInterface
| UpdateInterfaceState
| UpdateInterfaceContentType
| ResolveInterface
| SnapInterfaceControllerGetStateAction;

Expand All @@ -109,6 +116,7 @@
content: JSXElement;
state: InterfaceState;
context: InterfaceContext | null;
contentType: ContentType | null;
};

export type SnapInterfaceControllerState = {
Expand All @@ -132,7 +140,22 @@
super({
messenger,
metadata: {
interfaces: { persist: false, anonymous: false },
interfaces: {
persist: (interfaces: Record<string, StoredInterface>) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we should have any limits on this? How long do we persist Snap notifications for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is mainly to cover the edge case where the interface is lost for the detailed view notification across sessions. I realize it would persist an unread notification's interface indefinitely if the user never reads it, but that's an extreme edge case. I've started conversation with the notification team about exposing some settings to the user to control the lifecycle of notifications.

return Object.entries(interfaces).reduce<
Record<string, StoredInterface>
>((persistedInterfaces, [id, snapInterface]) => {
switch (snapInterface.contentType) {
case ContentType.Notification:
hmalik88 marked this conversation as resolved.
Show resolved Hide resolved
persistedInterfaces[id] = snapInterface;
return persistedInterfaces;
default:
return persistedInterfaces;
}
}, {});
},
anonymous: false,
},
},
name: controllerName,
state: { interfaces: {}, ...state },
Expand Down Expand Up @@ -161,6 +184,11 @@
this.updateInterface.bind(this),
);

this.messagingSystem.registerActionHandler(
`${controllerName}:updateInterfaceContentType`,
this.updateInterfaceContentType.bind(this),
);

this.messagingSystem.registerActionHandler(
`${controllerName}:deleteInterface`,
this.deleteInterface.bind(this),
Expand All @@ -183,12 +211,14 @@
* @param snapId - The snap id that created the interface.
* @param content - The interface content.
* @param context - An optional interface context object.
* @param contentType - The type of content.
* @returns The newly interface id.
*/
async createInterface(
snapId: SnapId,
content: ComponentOrElement,
context?: InterfaceContext,
contentType?: ContentType,
) {
const element = getJsxInterface(content);
await this.#validateContent(element);
Expand All @@ -205,6 +235,7 @@
content: castDraft(element),
state: componentState,
context: context ?? null,
contentType: contentType ?? null,
};
});

Expand Down Expand Up @@ -255,6 +286,20 @@
});
}

/**
* Update the type of content in an interface.
*
* @param id - The interface id.
* @param contentType - The type of content.
*/
updateInterfaceContentType(id: string, contentType: ContentType) {
hmalik88 marked this conversation as resolved.
Show resolved Hide resolved
this.#validateContentType(contentType);
assert(this.state.interfaces[id], 'Interface does not exist.');
this.update((draftState) => {
draftState.interfaces[id].contentType = contentType;
});
}

/**
* Delete an interface from state.
*
Expand Down Expand Up @@ -393,4 +438,17 @@
(id: string) => this.messagingSystem.call('SnapController:get', id),
);
}

/**
* Utility function to validate the type of interface content.
* Must be a value of the enum ContentType.
* Throws if the passed string is invalid.
*
* @param contentType - The content type.
*/
#validateContentType(contentType: string) {
if (!(contentType in ContentType)) {
throw new Error('Invalid content type.');

Check warning on line 451 in packages/snaps-controllers/src/interface/SnapInterfaceController.ts

View check run for this annotation

Codecov / codecov/patch

packages/snaps-controllers/src/interface/SnapInterfaceController.ts#L451

Added line #L451 was not covered by tests
hmalik88 marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
22 changes: 20 additions & 2 deletions packages/snaps-controllers/src/snaps/SnapController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,11 @@ import type {
SnapId,
ComponentOrElement,
} from '@metamask/snaps-sdk';
import { AuxiliaryFileEncoding, getErrorMessage } from '@metamask/snaps-sdk';
import {
AuxiliaryFileEncoding,
getErrorMessage,
ContentType,
} from '@metamask/snaps-sdk';
import type {
FetchedSnapFiles,
InitialConnections,
Expand Down Expand Up @@ -111,7 +115,7 @@ import { nanoid } from 'nanoid';
import semver from 'semver';

import { forceStrict, validateMachine } from '../fsm';
import type { CreateInterface, GetInterface } from '../interface';
import { type CreateInterface, type GetInterface } from '../interface';
import { log } from '../logging';
import type {
ExecuteSnapAction,
Expand Down Expand Up @@ -3369,16 +3373,20 @@ export class SnapController extends BaseController<
*
* @param snapId - The snap ID.
* @param content - The initial interface content.
* @param contentType - The type of content.
* @returns An identifier that can be used to identify the interface.
*/
async #createInterface(
snapId: SnapId,
content: ComponentOrElement,
contentType: ContentType,
): Promise<string> {
return this.messagingSystem.call(
'SnapInterfaceController:createInterface',
snapId,
content,
undefined,
contentType,
);
}

Expand Down Expand Up @@ -3416,10 +3424,20 @@ export class SnapController extends BaseController<
// If a handler returns static content, we turn it into a dynamic UI
if (castResult && hasProperty(castResult, 'content')) {
const { content, ...rest } = castResult;
const getContentType = (handler: HandlerType) => {
if (
handler === HandlerType.OnSignature ||
handler === HandlerType.OnTransaction
) {
return ContentType.Insight;
}
return ContentType.HomePage;
};

const id = await this.#createInterface(
snapId,
content as ComponentOrElement,
getContentType(handlerType),
);

return { ...rest, id };
Expand Down
1 change: 1 addition & 0 deletions packages/snaps-controllers/src/test-utils/controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -764,6 +764,7 @@ export const getRestrictedSnapInterfaceControllerMessenger = (
'PhishingController:maybeUpdateState',
'ApprovalController:hasRequest',
'ApprovalController:acceptRequest',
'SnapController:get',
],
allowedEvents: [],
});
Expand Down
Loading
Loading