Skip to content

perf: Reduce JSON validation during state updates #3660

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

Merged
FrederikBolding merged 5 commits into from
Oct 2, 2025
Merged

perf: Reduce JSON validation during state updates #3660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3b060ec
perf: Reduce JSON validation during state updates
FrederikBolding Oct 1, 2025
bb64a7a
Add tests
FrederikBolding Oct 1, 2025
65f5538
Tweak comment
FrederikBolding Oct 1, 2025
f7f6ae4
Update LavaMoat policies
metamaskbot Oct 1, 2025
168610a
Fix simulation
FrederikBolding Oct 1, 2025
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
1 change: 1 addition & 0 deletions packages/snaps-execution-environments/lavamoat/webpack/iframe/policy.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
},
"@metamask/snaps-utils": {
"globals": {
"TextEncoder": true,
"URL": true,
"console.error": true,
"console.log": true,
Expand Down
1 change: 1 addition & 0 deletions packages/snaps-execution-environments/lavamoat/webpack/node-process/policy.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
},
"@metamask/snaps-utils": {
"globals": {
"TextEncoder": true,
"URL": true,
"console.error": true,
"console.log": true,
Expand Down
1 change: 1 addition & 0 deletions packages/snaps-execution-environments/lavamoat/webpack/node-thread/policy.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
},
"@metamask/snaps-utils": {
"globals": {
"TextEncoder": true,
"URL": true,
"console.error": true,
"console.log": true,
Expand Down
1 change: 1 addition & 0 deletions packages/snaps-execution-environments/lavamoat/webpack/webview/policy.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
},
"@metamask/snaps-utils": {
"globals": {
"TextEncoder": true,
"URL": true,
"console.error": true,
"console.log": true,
Expand Down
6 changes: 3 additions & 3 deletions packages/snaps-rpc-methods/jest.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ module.exports = deepmerge(baseConfig, {
],
coverageThreshold: {
global: {
branches: 95.37,
branches: 95.7,
functions: 98.75,
lines: 98.92,
statements: 98.62,
lines: 98.99,
statements: 98.69,
},
},
});
18 changes: 18 additions & 0 deletions packages/snaps-rpc-methods/src/permitted/setState.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,14 @@ describe('snap_setState', () => {
const updateSnapState = jest.fn().mockReturnValue(null);
const getUnlockPromise = jest.fn().mockResolvedValue(undefined);
const hasPermission = jest.fn().mockReturnValue(true);
const getSnap = jest.fn().mockReturnValue({ preinstalled: false });

const hooks = {
getSnapState,
updateSnapState,
getUnlockPromise,
hasPermission,
getSnap,
};

const engine = new JsonRpcEngine();
Expand Down Expand Up @@ -87,12 +89,14 @@ describe('snap_setState', () => {
const updateSnapState = jest.fn().mockReturnValue(null);
const getUnlockPromise = jest.fn().mockResolvedValue(undefined);
const hasPermission = jest.fn().mockReturnValue(true);
const getSnap = jest.fn().mockReturnValue({ preinstalled: false });

const hooks = {
getSnapState,
updateSnapState,
getUnlockPromise,
hasPermission,
getSnap,
};

const engine = new JsonRpcEngine();
Expand Down Expand Up @@ -141,12 +145,14 @@ describe('snap_setState', () => {
const updateSnapState = jest.fn().mockReturnValue(null);
const getUnlockPromise = jest.fn().mockResolvedValue(undefined);
const hasPermission = jest.fn().mockReturnValue(true);
const getSnap = jest.fn().mockReturnValue({ preinstalled: false });

const hooks = {
getSnapState,
updateSnapState,
getUnlockPromise,
hasPermission,
getSnap,
};

const engine = new JsonRpcEngine();
Expand Down Expand Up @@ -200,12 +206,14 @@ describe('snap_setState', () => {
const updateSnapState = jest.fn().mockReturnValue(null);
const getUnlockPromise = jest.fn().mockResolvedValue(undefined);
const hasPermission = jest.fn().mockReturnValue(false);
const getSnap = jest.fn().mockReturnValue({ preinstalled: false });

const hooks = {
getSnapState,
updateSnapState,
getUnlockPromise,
hasPermission,
getSnap,
};

const engine = new JsonRpcEngine();
Expand Down Expand Up @@ -252,12 +260,14 @@ describe('snap_setState', () => {
const updateSnapState = jest.fn().mockReturnValue(null);
const getUnlockPromise = jest.fn().mockResolvedValue(undefined);
const hasPermission = jest.fn().mockReturnValue(true);
const getSnap = jest.fn().mockReturnValue({ preinstalled: false });

const hooks = {
getSnapState,
updateSnapState,
getUnlockPromise,
hasPermission,
getSnap,
};

const engine = new JsonRpcEngine();
Expand Down Expand Up @@ -303,12 +313,14 @@ describe('snap_setState', () => {
const updateSnapState = jest.fn().mockReturnValue(null);
const getUnlockPromise = jest.fn().mockResolvedValue(undefined);
const hasPermission = jest.fn().mockReturnValue(true);
const getSnap = jest.fn().mockReturnValue({ preinstalled: false });

const hooks = {
getSnapState,
updateSnapState,
getUnlockPromise,
hasPermission,
getSnap,
};

const engine = new JsonRpcEngine();
Expand Down Expand Up @@ -358,12 +370,14 @@ describe('snap_setState', () => {
const updateSnapState = jest.fn().mockReturnValue(null);
const getUnlockPromise = jest.fn().mockResolvedValue(undefined);
const hasPermission = jest.fn().mockReturnValue(true);
const getSnap = jest.fn().mockReturnValue({ preinstalled: false });

const hooks = {
getSnapState,
updateSnapState,
getUnlockPromise,
hasPermission,
getSnap,
};

const engine = new JsonRpcEngine();
Expand Down Expand Up @@ -408,12 +422,14 @@ describe('snap_setState', () => {
const updateSnapState = jest.fn().mockReturnValue(null);
const getUnlockPromise = jest.fn().mockResolvedValue(undefined);
const hasPermission = jest.fn().mockReturnValue(true);
const getSnap = jest.fn().mockReturnValue({ preinstalled: false });

const hooks = {
getSnapState,
updateSnapState,
getUnlockPromise,
hasPermission,
getSnap,
};

const engine = new JsonRpcEngine();
Expand Down Expand Up @@ -461,12 +477,14 @@ describe('snap_setState', () => {
const updateSnapState = jest.fn().mockReturnValue(null);
const getUnlockPromise = jest.fn().mockResolvedValue(undefined);
const hasPermission = jest.fn().mockReturnValue(true);
const getSnap = jest.fn().mockReturnValue({ preinstalled: false });

const hooks = {
getSnapState,
updateSnapState,
getUnlockPromise,
hasPermission,
getSnap,
};

const engine = new JsonRpcEngine();
Expand Down
45 changes: 30 additions & 15 deletions packages/snaps-rpc-methods/src/permitted/setState.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@ import type { PermittedHandlerExport } from '@metamask/permission-controller';
import { providerErrors, rpcErrors } from '@metamask/rpc-errors';
import type { SetStateParams, SetStateResult } from '@metamask/snaps-sdk';
import type { JsonObject } from '@metamask/snaps-sdk/jsx';
import { type InferMatching } from '@metamask/snaps-utils';
import {
getJsonSizeUnsafe,
type InferMatching,
type Snap,
} from '@metamask/snaps-utils';
import {
boolean,
create,
Expand All @@ -16,13 +20,7 @@ import type {
Json,
JsonRpcRequest,
} from '@metamask/utils';
import {
getJsonSize,
hasProperty,
isObject,
assert,
JsonStruct,
} from '@metamask/utils';
import { hasProperty, isObject, assert, JsonStruct } from '@metamask/utils';

import {
manageStateBuilder,
Expand All @@ -36,6 +34,7 @@ const hookNames: MethodHooksObject<SetStateHooks> = {
getSnapState: true,
getUnlockPromise: true,
updateSnapState: true,
getSnap: true,
};

/**
Expand Down Expand Up @@ -85,6 +84,13 @@ export type SetStateHooks = {
newState: Record<string, Json>,
encrypted: boolean,
) => Promise<void>;

/**
* Get Snap metadata.
*
* @param snapId - The ID of a Snap.
*/
getSnap: (snapId: string) => Snap | undefined;
};

const SetStateParametersStruct = objectStruct({
Expand Down Expand Up @@ -112,6 +118,7 @@ export type SetStateParameters = InferMatching<
* @param hooks.getSnapState - Get the state of the requesting Snap.
* @param hooks.getUnlockPromise - Wait for the extension to be unlocked.
* @param hooks.updateSnapState - Update the state of the requesting Snap.
* @param hooks.getSnap - The hook function to get Snap metadata.
* @returns Nothing.
*/
async function setStateImplementation(
Expand All @@ -124,6 +131,7 @@ async function setStateImplementation(
getSnapState,
getUnlockPromise,
updateSnapState,
getSnap,
}: SetStateHooks,
): Promise<void> {
const { params } = request;
Expand All @@ -150,13 +158,20 @@ async function setStateImplementation(

const newState = await getNewState(key, value, encrypted, getSnapState);

const size = getJsonSize(newState);
if (size > STORAGE_SIZE_LIMIT) {
throw rpcErrors.invalidParams({
message: `Invalid params: The new state must not exceed ${
STORAGE_SIZE_LIMIT / 1_000_000
} MB in size.`,
});
const snap = getSnap(
(request as JsonRpcRequest<SetStateParams> & { origin: string }).origin,
);

Copy link

@cursor cursor bot Oct 1, 2025

Choose a reason for hiding this comment

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

Bug: Snap Origin Handling Inconsistency

The setState handler uses an unsafe type assertion to extract origin from the request object, which JsonRpcRequest doesn't inherently include. This differs from manageState.ts, which gets origin from context. As a result, getSnap may receive undefined, causing the preinstalled snap check to fail and the state size limit to always apply, or leading to runtime errors.

Fix in Cursor Fix in Web

Copy link
Member Author

@FrederikBolding FrederikBolding Oct 1, 2025

Choose a reason for hiding this comment

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

setState is a permitted RPC method, which means we have to access the origin like this.

if (!snap?.preinstalled) {
// We know that the state is valid JSON as per previous validation.
const size = getJsonSizeUnsafe(newState, true);
if (size > STORAGE_SIZE_LIMIT) {
throw rpcErrors.invalidParams({
message: `Invalid params: The new state must not exceed ${
STORAGE_SIZE_LIMIT / 1_000_000
} MB in size.`,
});
}
}

await updateSnapState(newState, encrypted);
Expand Down
Loading
Loading