Consider moving snap's initial eval to lazily execute during first RPC command

[ritave]

Currently we eval the snap script during execution-environment initialisation. This means we need to keep more state and have more complicated timeout logic. The Snap's global state is ephemeral anyways since it can be killed at any time due to a timeout. What we should do is run the eval during the first time a RPC comes. That way the we can simplify initialization, timeout and execution-environment logic.

[rekmarks]

@FrederikBolding @ritave where are we at with this issue following the onRpcRequest refactor?

[ritave]

@rekmarks orthogonal and untouched

[FrederikBolding]

Agreed, this has not been started yet.

I'm also not entirely sure this will improve anything, but happy to be proven wrong 🤔

[rekmarks]

Copy that, thanks. Before proceeding, let's establish the current behavior:

• In the execution service, "jobs" are only created when a snap is executed, via executeSnap: https://github.com/MetaMask/snaps-skunkworks/blob/f3f48312ea1f429ce273c42931f2aeea7fe0eeaa/packages/controllers/src/services/AbstractExecutionService.ts#L177-L205
• When an execution environment receives the executeSnap command, the method startSnap is called, which immediately evaluates the Snap source code, without calling its onRpcRequest function: https://github.com/MetaMask/snaps-skunkworks/blob/f3f48312ea1f429ce273c42931f2aeea7fe0eeaa/packages/execution-environments/src/common/rpcMethods.ts#L29-L46

When a snap is installed, we want to evaluate the snap without calling its onRpcRequest function to ensure that it doesn't throw during evaluation. This is just a basic fail-early validity measure, and any changes we make would need to maintain this on-install evaluation. (We also leave it running in this case in anticipation of receiving RPC requests, but that's fine and I believe out-of-scope for present purposes.)

The current status quo works, and gives us the above benefit. It's not immediately apparent to me what complexity we could get rid of by, in the RPC request case, wait to evaluate the snap until the first call to the onRpcRequest handler. @ritave could you go into more detail about the complexity we'd get rid of by doing this and any other benefits you have identified?

[ritave]

@rekmarks I don't see a reason for the additional on-install evaluation step. It doesn't catch any additional errors that wouldn't be caught during lazy initialisation. I don't see it as an added benefit at all. The idle logic will be the same and the snap would be left running for more incoming RPC requests after the initial one.

We already have lazy initialisation in place - if the Snap idles for too long it is stopped and then restarted on next request. And so the Snap starting logic is currently inconsistent, the Snap is started in multiple places and startup errors need to be handled in all of them. This issue removes the complexity that happens during Snap installation, leaving only the existing logic of starting the Snap during RPC requests.

[rekmarks]

Hey team! Please add your planning poker estimate with ZenHub @FrederikBolding @ritave

[rekmarks]

I don't see a reason for the additional on-install evaluation step. It doesn't catch any additional errors that wouldn't be caught during lazy initialisation.

This is true, but we need to consider when those errors are caught, not just that they are caught at all. In particular, evaluating during installation allows us to reject installation of Snaps that are fundamentally broken and will not execute under any circumstances. Such such snaps are effectively junk that should never be added to the user's MetaMask instance. Evaluating during installation ensures that this is the case. I don't see a good path to preserving this behavior if we only execute snaps when they receive external RPC requests.

[ritave]

@rekmarks If that's a requirement, then this issue can be closed with a wontfix. It wouldn't remove any complexity.

[FrederikBolding]

I think failing early on install is a nice property to have. It also gives us a place to show the feedback since the user is already interacting with the UI.