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.

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

Add last interaction tracking for Snap requests #3147

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add last interaction tracking for Snap requests #3147

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
53 changes: 53 additions & 0 deletions packages/snaps-controllers/src/snaps/SnapController.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10700,4 +10700,57 @@ describe('SnapController', () => {
snapController.destroy();
});
});

describe('SnapController:updateLastInteraction', () => {
it('should update last snap interaction time', () => {
const messenger = getSnapControllerMessenger();

const snapController = getSnapController(
getSnapControllerOptions({
messenger,
state: {
snaps: getPersistedSnapsState(),
},
}),
);

const snap = snapController.getExpect(MOCK_SNAP_ID);
const handlerType = HandlerType.OnRpcRequest;
const spyOnUpdate = jest.spyOn(snapController as any, 'update');

snapController.updateLastInteraction(snap.id, handlerType);

expect(spyOnUpdate).toHaveBeenCalledTimes(1);
expect(spyOnUpdate).toHaveBeenCalledWith(expect.any(Function));

const stateUpdater = spyOnUpdate.mock.calls[0][0] as (state: any) => void;
const mockState: { snaps: Record<string, any> } = {
snaps: { [snap.id]: {} },
};
stateUpdater(mockState);
expect(mockState.snaps[snap.id]?.lastInteraction).toBeDefined();
});

it('should not update last interaction if handler is not on allowlist', () => {
const messenger = getSnapControllerMessenger();

const snapController = getSnapController(
getSnapControllerOptions({
messenger,
state: {
snaps: getPersistedSnapsState(),
},
}),
);

const snap = snapController.getExpect(MOCK_SNAP_ID);
const handlerType = HandlerType.OnCronjob;
// @ts-expect-error Spying on private method
const spyOnUpdate = jest.spyOn(snapController, 'update');

snapController.updateLastInteraction(snap.id, handlerType);

expect(spyOnUpdate).toHaveBeenCalledTimes(0);
});
});
});
18 changes: 18 additions & 0 deletions packages/snaps-controllers/src/snaps/SnapController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ import {
getLocalizedSnapManifest,
MAX_FILE_SIZE,
OnSettingsPageResponseStruct,
TRACKABLE_HANDLER_TYPES,
} from '@metamask/snaps-utils';
import type {
Json,
Expand Down Expand Up @@ -3449,6 +3450,8 @@ export class SnapController extends BaseController<

const timeout = this.#getExecutionTimeout(handlerPermissions);

this.updateLastInteraction(snapId, handlerType);

return handler({ origin, handler: handlerType, request, timeout });
}

Expand Down Expand Up @@ -4185,4 +4188,19 @@ export class SnapController extends BaseController<
runtime.state = undefined;
}
}

/**
* Track usage of a Snap by updating last interaction with it.
* Note: Interaction tracking is done only for specific handler types.
*
* @param snapId - ID of a Snap.
* @param handlerType - Type of Snap handler.
*/
updateLastInteraction(snapId: SnapId, handlerType: HandlerType): void {
Copy link
Member

Choose a reason for hiding this comment

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

We already have some code that tracks this, we just don't persist it in state, it is only stored in memory:

snaps/packages/snaps-controllers/src/snaps/SnapController.ts

Lines 3807 to 3822 in 4b6dadb

#recordSnapRpcRequestStart(snapId: SnapId, requestId: unknown, timer: Timer) {
const runtime = this.#getRuntimeExpect(snapId);
runtime.pendingInboundRequests.push({ requestId, timer });
runtime.lastRequest = null;
}
#recordSnapRpcRequestFinish(snapId: SnapId, requestId: unknown) {
const runtime = this.#getRuntimeExpect(snapId);
runtime.pendingInboundRequests = runtime.pendingInboundRequests.filter(
(request) => request.requestId !== requestId,
);
if (runtime.pendingInboundRequests.length === 0) {
runtime.lastRequest = Date.now();
}
}

Side-note: I wonder how many extra state updates this will cause? May be somewhat excessive and cause load on the clients 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if it would be any better if we use SnapRuntimeData and that process around RPC. Since, we need to get this straight into the UI, easily available through the Snap object, and we need to filter which requests we want to track before recording it to state. Last request seems to be updated in different context under different use cases.

On the note: My idea was to build something that would somehow debounce the state update. If state is already available at the moment of potential timestamp update, it should be cheaper operation to first check if the update has been done in the last minute or so, then update the state only if it's not updated for more than a minute or similar timespan. Feedback and ideas on this are also welcome.

if (TRACKABLE_HANDLER_TYPES.has(handlerType)) {
this.update((state) => {
state.snaps[snapId].lastInteraction = new Date().toISOString();
});
}
}
}
2 changes: 1 addition & 1 deletion packages/snaps-utils/coverage.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@
"branches": 99.74,
"functions": 98.93,
"lines": 99.61,
"statements": 96.94
"statements": 96.95
}
18 changes: 18 additions & 0 deletions packages/snaps-utils/src/handler-types.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { HandlerType, TRACKABLE_HANDLER_TYPES } from './handler-types';

describe('TRACKABLE_HANDLER_TYPES', () => {
const expectedHandlerTypes = [
HandlerType.OnRpcRequest,
HandlerType.OnInstall,
HandlerType.OnUpdate,
HandlerType.OnHomePage,
HandlerType.OnSettingsPage,
HandlerType.OnUserInput,
];

it('should match the exact set of trackable handler types', () => {
expect(TRACKABLE_HANDLER_TYPES).toStrictEqual(
new Set(expectedHandlerTypes),
);
});
});
12 changes: 12 additions & 0 deletions packages/snaps-utils/src/handler-types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,15 @@ export type SnapHandler = {
};

export const SNAP_EXPORT_NAMES = Object.values(HandlerType);

/**
* A subset of handler types that are allowed for Snap interaction tracking.
*/
export const TRACKABLE_HANDLER_TYPES = new Set<HandlerType>([
HandlerType.OnRpcRequest,
HandlerType.OnInstall,
HandlerType.OnUpdate,
HandlerType.OnHomePage,
HandlerType.OnSettingsPage,
HandlerType.OnUserInput,
]);
5 changes: 5 additions & 0 deletions packages/snaps-utils/src/snaps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,11 @@ export type Snap = TruncatedSnap & {
* Flag to signal whether this snap should hide the Snap branding like header or avatar in the UI or not.
*/
hideSnapBranding?: boolean;

/**
* Date in ISO String format, representing time of the last interaction with snap.
*/
lastInteraction?: string;
};

export type TruncatedSnapFields =
Expand Down
Loading