BREAKING: Transaction Insight API

[FrederikBolding]

Adds support for multiple handlers/exports on the BaseSnapExecutor and adds a new export to be used for transaction insight. Also adds an endowment that will work as a flag to detect whether the snap wants to receive transaction insight requests.

JS Implementation:

module.exports.onTransaction = async ({ origin, transaction, chainId }) => {
// do something
return { insights }
}

TS Implementation:

import { OnTransactionHandler } from '@metamask/snap-types';

export const onTransaction: OnTransactionHandler = ({ origin, transaction, chainId }) => {
// do something
return { insights }
}

This PR is breaking because some exports are renamed and moved around. For example, `@metamask/execution-environments' no longer exports any enums.

[codecov-commenter]

Codecov Report

Merging #642 (c524b38) into main (d81a713) will increase coverage by 0.04%.
The diff coverage is 86.58%.

@@            Coverage Diff             @@
##             main     #642      +/-   ##
==========================================
+ Coverage   85.75%   85.79%   +0.04%     
==========================================
  Files         108      107       -1     
  Lines        3165     3197      +32     
  Branches      612      621       +9     
==========================================
+ Hits         2714     2743      +29     
- Misses        421      424       +3     
  Partials       30       30              

Impacted Files	Coverage Δ
...kages/controllers/src/services/ExecutionService.ts	100.00% <ø> (ø)
packages/controllers/src/utils.ts	100.00% <ø> (ø)
...ution-environments/src/common/endowments/crypto.ts	100.00% <ø> (ø)
packages/rpc-methods/src/restricted/invokeSnap.ts	0.00% <0.00%> (ø)
packages/rpc-methods/src/restricted/manageState.ts	0.00% <0.00%> (ø)
packages/utils/src/manifest.ts	100.00% <ø> (ø)
...ages/execution-environments/src/common/commands.ts	93.02% <83.33%> (-6.98%)	⬇️
...ntrollers/src/services/AbstractExecutionService.ts	90.41% <100.00%> (-0.07%)	⬇️
...ges/controllers/src/services/iframe/test/server.ts	85.71% <100.00%> (-0.50%)	⬇️
packages/controllers/src/snaps/SnapController.ts	94.68% <100.00%> (-0.18%)	⬇️
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[rekmarks]

I think this approach makes sense. In addition to inline comments, a couple of things:

1. We should consider how we're going to route messages when snaps are able to export objects with methods, like the keyring. We don't need to support that here, but we shouldn't make it more difficult to implement in the future.
2. Only tangentially related to this PR, but do we throw an error if a snap exports something we don't recognize? We should create an issue for that if we don't.

[FrederikBolding]

1. We should consider how we're going to route messages when snaps are able to export objects with methods, like the keyring. We don't need to support that here, but we shouldn't make it more difficult to implement in the future.

Hmm, yeah I'm not entirely sure how we would do that 🤔 - Will think more about it.

2. Only tangentially related to this PR, but do we throw an error if a snap exports something we don't recognize? We should create an issue for that if we don't.

We don't, but only the exports with the proper names are registered as available exports in the BaseSnapExecutor (as of this PR). Is there a reason to throw?

addressed changes.

[rekmarks]

LGTM!

[rekmarks]

For future reference: Is there any reason not to have validation in these cases?

[hmalik88]

What do you mean by validation? At the snapRpc level we do a isHandler check before passing on the handler to getHandlerArguments.

[rekmarks]

@hmalik88 note that I removed the ChainId type because it was ultimately just a string and it was unclear if it referred to an Ethereum chain id or a CAIP chain id. The SnapId should also be removed under the same reasoning, but that's too big of a change for right now.

We should really just use string when something can be any string, and then use template string types (e.g. 0x${string}) if we want something more specific.

[hmalik88]

@hmalik88 note that I removed the ChainId type because it was ultimately just a string and it was unclear if it referred to an Ethereum chain id or a CAIP chain id. The SnapId should also be removed under the same reasoning, but that's too big of a change for right now.

We should really just use string when something can be any string, and then use template string types (e.g. 0x${string}) if we want something more specific.

Okay, I'm cool with the change. Can't use template string types for certain things though like the CAIP id.