feat: make SafeEventEmitterProvider compatible with eth req libraries

[cryptodev-2s]

Explanation

• Aligning SafeEventEmitterProvider to EIP-1193 by adding a request method and deprecating sendAsync (which aren't part of the spec)
• Ensuring that request does not require id and jsonrpc to be set, filling them in if they are missing

References

Fixes #4095

Changelog

@metamask/eth-json-rpc-provider

• ADDED: request method type compatible with ethereum request libraries.
• CHANGED: markes sendAsync as deprecated for usage in favor of request.

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
• I've highlighted breaking changes using the "BREAKING" category above as appropriate

[socket-security]

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert	Package	Note	Source	CI
Deprecated	npm/[email protected]	Reason: core-js@<3.23.3 is no longer maintained and not recommended for usage due to the number of issues. Because of the V8 engine whims, feature detection in old core-js versions could cause a slowdown up to 100x even if nothing is polyfilled. Some versions have web compatibility issues. Please, upgrade your dependencies to the actual version of core-js.	packages/eth-json-rpc-provider/package.json	🚫
Install scripts	npm/[email protected]	Install script: postinstall Source: node -e "try{require('./postinstall')}catch(e){}"	packages/eth-json-rpc-provider/package.json	🚫

View full report↗︎

Next steps

What is a deprecated package?

The maintainer of the package marked it as deprecated. This could indicate that a single version should not be used, or that the package is no longer maintained and any new vulnerabilities will not be fixed.

Research the state of the package and determine if there are non-deprecated versions that can be used, or if it should be replaced with a new, supported solution.

What is an install script?

Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts.

Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

• @SocketSecurity ignore npm/[email protected]

[mcmire]

Hey! After reviewing this PR, I realized that we may want to go a little further than using tsd. Let me know what you think.

[mcmire]

@cryptodev-2s Hey! I still think we ought to add smoke tests so that we can be confident that these changes fix the compatibility issues we have with the various Ethereum libraries. I outlined ideas for this in the comment above. What are your thoughts?

[cryptodev-2s]

@cryptodev-2s Hey! I still think we ought to add smoke tests so that we can be confident that these changes fix the compatibility issues we have with the various Ethereum libraries. I outlined ideas for this in the comment above. What are your thoughts?
@mcmire yes make sense I will add smoke tests as outlined in your comment

[mcmire]

Thanks for adding those smoke tests! This looks good to me.

[legobeat]

It looks like this is only used for dependencies?

Could the types be re-exported in this package, thus delegating @metamask/utils to a devDependency?

[legobeat]

To complete it, the types used in exports also have to be exported from packages/eth-json-rpc-provider/src/index.ts

[MajorLift]

@legobeat I'm not sure I understand the reasoning for this change, based on the criteria for categorizing dep vs. devDep discussed here: #4449 (comment).

The types imported from @metamask/utils always need to be present for the user, since they are dependencies for the exported types and functions Eip1193Request, convertEip1193RequestToJsonRpcRequest, SafeEventEmitterProvider['request'], SafeEventEmitterProvider['sendAsync'], SafeEventEmitterProvider['send']. These exports will always directly reference the imported types, and "any project using this package would need to have the package the types come from as well, or it would be an invalid reference."

The type imports from @metamask/utils are also present in the built type declaration files for this package, which wouldn't happen if they were only being used internally at build time.

Re-exporting the imports would give the user access to them, but wouldn't it also require the user to manually import the re-exported types in order to use the exports from this package -- even if the re-exported types are not used anywhere else? It seems like that would effectively make @metamask/utils a package dependency, just with extra steps.

[mcmire]

The type imports from @metamask/utils are also present in the built type declaration files for this package, which wouldn't happen if they were only being used internally at build time.

I see this as the key criterion for deciding that @metamask/utils should be in dependencies and not devDependencies, so that TypeScript users are able to use the type declarations that ship with this package.

I see that we are exporting Eip1193Request in this PR. If we were not doing this, then it would be less obvious that the type declaration files referenced types imported from @metamask/utils, but I believe that even without this, providerFromMiddleware takes types that reference Json and JsonRpcParams. So I believe that @metamask/utils still needs to be included in dependencies.

Thoughts?

[legobeat]

My suggestion here is that the used types (Json, JsonRpcParams) would get re-export-ed from this package.

This will be equivalent from the TypeScript user perspective (the exact same types are available for them) but without requiring the downstream user to pull in the not-actually used js files. So effectively the built .d.ts files in @metamask/eth-json-rpc-provider would include the necessary definitions which would otherwise get pulled in from @metamask/utils.

TypeScript will transparently see that the types are identical and not distinguish between them when imported from different sources downstream.

This also has a side-benefit that any future otherwise non-breaking module refactors (renaming or moving initial type definitions between different packages, for example) becomes transparent for users of this package.

[mcmire]

This will be equivalent from the TypeScript user perspective (the exact same types are available for them) but without requiring the downstream user to pull in the not-actually used js files. So effectively the built .d.ts files in @metamask/eth-json-rpc-provider would include the necessary definitions which would otherwise get pulled in from @metamask/utils.

Hmm. Is the thought here that if we re-export types from @metamask/utils, the TypeScript compiler will recognize this and embed those types into the type declarations for @metamask/eth-json-rpc-provider? If so, I'm not sure it works that way. I believe the type declaration files would still include import lines from @metamask/utils, even if those types are being re-exported. Therefore, the consumer would still need @metamask/utils somewhere in the dependency tree for TypeScript to "see" those types.

[mcmire]

One more comment from me above and then I think we may be good to go on this.

[mcmire]

I want to make sure we handle errors correctly here. I had some more thoughts on how we can do that. Additionally, I noticed some lack of test coverage.

[socket-security]

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@metamask/[email protected]	None	+6	1.46 MB	lgbot
npm/@metamask/[email protected]	Transitive: environment, eval, filesystem	+6	2.94 MB	lgbot
npm/@metamask/[email protected]	None	+3	92.5 kB	metamaskbot
npm/[email protected]	network	+5	19 MB	ricmoo

🚮 Removed packages: npm/[email protected]

View full report↗︎

[mcmire]

Two more comments but I think this looks good otherwise!

[mcmire]

LGTM!