Skip to content

perf: Change request notification order #3381

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 2 commits into from
May 6, 2025
Merged

perf: Change request notification order #3381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
15e4d42
perf: Change request notification order
FrederikBolding May 6, 2025
518bc17
Remove unnecesary terminate call
FrederikBolding May 6, 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
3 changes: 1 addition & 2 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 @@ -1802,7 +1802,7 @@ describe('SnapController', () => {
});

const { rootMessenger } = options;
const [snapController, service] = getSnapControllerWithEES(options);
const [snapController] = getSnapControllerWithEES(options);
const snap = snapController.getExpect(MOCK_SNAP_ID);

rootMessenger.registerActionHandler(
Expand Down Expand Up @@ -1845,7 +1845,6 @@ describe('SnapController', () => {
);

snapController.destroy();
await service.terminateAllSnaps();
Copy link
Member

@Mrtenz Mrtenz May 6, 2025

Choose a reason for hiding this comment

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

How come this is unnecessary?

Copy link
Member Author

@FrederikBolding FrederikBolding May 6, 2025

Choose a reason for hiding this comment

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

Some of the timing changed and so the Snap is already torn down when we get to the end of the test

Copy link
Member Author

@FrederikBolding FrederikBolding May 6, 2025

Choose a reason for hiding this comment

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

It was causing the test to fail due to some timing issue where we were trying to terminate a Snap that had already been terminated. Simplest solution was to remove it for now and look into it later

});

// This isn't stable in CI unfortunately
Expand Down
4 changes: 2 additions & 2 deletions packages/snaps-execution-environments/coverage.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"branches": 90,
"functions": 94.57,
"lines": 90.13,
"statements": 89.5
"lines": 90.15,
"statements": 89.52
}
26 changes: 16 additions & 10 deletions packages/snaps-execution-environments/src/common/BaseSnapExecutor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,12 +531,15 @@ export class BaseSnapExecutor {
assertSnapOutboundRequest(sanitizedArgs);
return await withTeardown(
(async () => {
await this.#notify({
method: 'OutboundRequest',
params: { source: 'snap.request' },
});
try {
return await originalRequest(sanitizedArgs);
const promise = originalRequest(sanitizedArgs);

await this.#notify({
method: 'OutboundRequest',
params: { source: 'snap.request' },
});

return await promise;
} finally {
await this.#notify({
method: 'OutboundResponse',
Expand Down Expand Up @@ -573,12 +576,15 @@ export class BaseSnapExecutor {
assertEthereumOutboundRequest(sanitizedArgs);
return await withTeardown(
(async () => {
await this.#notify({
method: 'OutboundRequest',
params: { source: 'ethereum.request' },
});
try {
return await originalRequest(sanitizedArgs);
const promise = originalRequest(sanitizedArgs);

await this.#notify({
method: 'OutboundRequest',
params: { source: 'ethereum.request' },
});

return await promise;
} finally {
await this.#notify({
method: 'OutboundResponse',
Expand Down
9 changes: 5 additions & 4 deletions packages/snaps-execution-environments/src/common/endowments/network.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,15 +226,16 @@ const createNetwork = ({ notify }: EndowmentFactoryOptions = {}) => {
return await withTeardown(
(async () => {
try {
await notify({
method: 'OutboundRequest',
params: { source: 'fetch' },
});
const fetchPromise = fetch(input, {
...init,
signal: abortController.signal,
});

await notify({
method: 'OutboundRequest',
params: { source: 'fetch' },
});

openFetchConnection = {
cancel: async () => {
abortController.abort();
Expand Down
Loading