Typing fixes for `provider`, `ppom` and other `any` usage #89

MajorLift · 2023-11-16T00:03:04Z

Explanation

Reduces repo-wide any count to 1 (excluding test files).

Types provider variables using the Provider type from @metamask/network-controller.
- The Provider type is now safe to use, with its recent alignment issues resolved as of Align types with @metamask/eth-json-rpc-provider eth-query#21.
- See also Align NetworkController provider type w/ eth-query core#2028).
Fixes JSON-RPC typing for provider requests.
- Installs @metamask/utils as a dependency.
Types PPOM variables with provisional skeleton type.
Fixes for controller-messenger typing.
Fixes for misc. any usage

References

Closes [ppom-validator] Replace use of any with proper types (non-test files only) #144

Changelog

Changed

Add @metamask/utils ^8.3.0 as a dependency. (#89)

Removed

BREAKING: NetworkControllerStateChangeEvent is removed from the PPOMControllerEvents union type. (#89)

Fixed

BREAKING: PPOMController class constructor option types are narrowed from any. (#89)
- The constructor expects provider to be the Provider type from @metamask/network-controller (equivalent to SafeEventEmitterProvider from @metamask/eth-json-rpc-provider).
- The constructor expects onPreferencesChange to be (callback: (preferencesState: { securityAlertsEnabled: boolean } & Record<string, Json>) => void) => void.
BREAKING: The UsePPOM type replaces any with the PPOM type in its definition. (#89)
BREAKING: When a PPOM class instance makes JSON-RPC requests to the provider, the params type is now widened from Record<string, unknown> to JsonRpcParams, and the response type is narrowed from any to JsonRpcSuccess<Json>. (#89)

src/ppom-controller.ts

socket-security · 2024-01-29T19:37:04Z

🚨 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
Network access	npm/agent-base@7.1.0	Module: http Location: Package overview
Network access	npm/agent-base@7.1.0	Module: https Location: Package overview
Shell access	npm/foreground-child@3.1.1	Module: child_process Location: Package overview
Shell access	npm/foreground-child@3.1.1	Module: child_process Location: Package overview
Network access	npm/@npmcli/agent@2.2.0	Module: tls Location: Package overview
Network access	npm/@npmcli/agent@2.2.0	Module: https-proxy-agent Location: Package overview
Network access	npm/@npmcli/agent@2.2.0	Module: http-proxy-agent Location: Package overview
Network access	npm/@npmcli/agent@2.2.0	Module: dns Location: Package overview
Network access	npm/@npmcli/agent@2.2.0	Module: net Location: Package overview
Network access	npm/socks-proxy-agent@8.0.2	Module: net Location: Package overview

View full report↗︎

Next steps

What is network access?

This module accesses the network.

Packages should remove all network access that is functionally unnecessary. Consumers should audit network access to ensure legitimate use.

What is shell access?

This module accesses the system shell. Accessing the system shell increases the risk of executing arbitrary code.

Packages should avoid accessing the shell which can reduce portability, and make it easier for malicious shell access to be introduced.

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/foo@1.0.0 or ignore all packages with @SocketSecurity ignore-all

@SocketSecurity ignore npm/agent-base@7.1.0
@SocketSecurity ignore npm/foreground-child@3.1.1
@SocketSecurity ignore npm/@npmcli/agent@2.2.0
@SocketSecurity ignore npm/socks-proxy-agent@8.0.2

src/ppom-controller.ts

MajorLift · 2024-01-30T05:16:26Z

src/index.ts

+export type {
+  NativeCrypto,
+  PPOMState,
+  UsePPOM,
+  UpdatePPOM,
+  PPOMInitialisationStatusType,
+  PPOMControllerActions,
+  PPOMControllerInitialisationStateChangeEvent,
+  PPOMControllerEvents,
+  PPOMControllerMessenger,
+} from './ppom-controller';
+export {
+  REFRESH_TIME_INTERVAL,
+  NETWORK_CACHE_DURATION,
+  PPOMInitialisationStatus,
+  PPOMController,
+} from './ppom-controller';


There should be no breaking changes here. This is the list of existing package-level exports from the ppom-controller file.

Explicit enumeration of exports is safer, and also allows sharing exports internally, which comes in handy with e.g. AllowedEvents, controllerName for test files.

src/ppom-controller.ts

jiexi

LGTM, but again deferring to @jpuri as this is her domain

MajorLift · 2024-02-02T21:43:20Z

src/ppom-controller.ts

+    | JsonRpcSuccess<Json>
+    | (Omit<JsonRpcFailure, 'error'> & { error: unknown })


I used the JsonRpcResponse<Json> type here instead of unknown, even though this technically narrows the response type in the sendAsync callback compared to any (see line 1059).

This is because JsonRpcEngine is where the callback passed into sendAsync actually gets invoked, and that class consistently uses the JsonRpcResponse type for responses.

The error type in JsonRpcFailure is widened to unknown since that's the only type accepted by sendAsync. See https://github.com/MetaMask/ppom-validator/pull/89/files#r1470601394 for a discussion on this point.

Note: sendAsync is set to be deprecated in favor of request in a new EIP1193-compliant provider class. request will use JsonRpcResponse<Json>.

mcmire

Changes here make sense to me, although I will also defer to @jpuri for final approval.

mcmire · 2024-02-06T18:13:17Z

jest.config.js

@@ -39,10 +39,10 @@ module.exports = {
  // An object that configures minimum threshold enforcement for coverage results
  coverageThreshold: {
    global: {
-      branches: 100,
+      branches: 98.18,


Would be nice to not reduce the test coverage here 😭 What caused this?

Brought it back up to 100%! 3ff8ebe 572cab2

mcmire · 2024-02-06T18:18:00Z

src/ppom-controller.ts

-export type PPOMControllerEvents =
-  | PPOMControllerInitialisationStateChangeEvent
-  | NetworkControllerStateChangeEvent;
+export type PPOMControllerEvents = PPOMControllerInitialisationStateChangeEvent;


For context, these changes align with changes we've been making to controllers in the core repo. For instance, see SelectedNetworkController: https://github.com/MetaMask/core/blob/d3861d03df0ff3f4cf2b479f746b46e602065d76/packages/selected-network-controller/src/SelectedNetworkController.ts#L71-L89

Thanks for adding context here. To be clear, I didn't use the ControllerGetStateAction and ControllerStateChangeEvent types since those were introduced in base-controller v4, and this package is using v3.

Thanks a lot @MajorLift for these improvements 🙏

MajorLift · 2024-02-09T06:12:27Z

src/ppom-controller.ts

+// Provisional skeleton type for PPOM class
+// TODO: Replace with actual PPOM class
+type PPOM = {
+  new: (...args: unknown[]) => PPOM;
+  validateJsonRpc: () => Promise<unknown>;
+  free: () => void;
+} & Record<string, unknown>;


Wrote temporary PPOM type that expresses a minimal interface. Added TODO so this would be eventually replaced with a full type.

MajorLift · 2024-02-09T06:13:41Z

src/ppom-controller.ts

+        // TOOD: Replace with `PreferencesState` from `@metamask/preferences-controller`
+        preferencesState: { securityAlertsEnabled: boolean } & Record<
+          string,
+          Json
+        >,


Wrote temporary PreferencesState surrogate to avoid introducing @metamask/preferences-controller as a dependency. Added TODO so this would be eventually replaced with the full type.

MajorLift · 2024-02-09T06:13:53Z

src/ppom-controller.ts

+    // TOOD: Replace with `PreferencesState` from `@metamask/preferences-controller`
+    preferenceControllerState: { securityAlertsEnabled: boolean } & Record<
+      string,
+      Json
+    >,


Same as above.

MajorLift · 2024-02-09T06:39:02Z

Added Changelog with breaking changes highlighted.

seaona · 2024-02-09T09:19:19Z

CHANGELOG.md

@@ -5,6 +5,20 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

 ## [Unreleased]


I'm curious about the Changelog changes. Typically the changelog only includes user-facing changes and it is generated with the update-changelog script during the RC phase 🤔 for this reason I think we should not include these changes here?

I removed some entries that weren't user-facing. Thanks for pointing that out!

All of the remaining entries are major changes for package-level exports. See our documentation on reviewing release PRs for how these qualify as breaking changes.

As for why these are in the changelog file, the shared libraries team has been recording changelog entries for new PRs under the "Unreleased" heading, so that they're immediately available for reference when the next release is being created.

update-changelog auto-populates a list of unreleased commits to changelog, but these are only intended as starting points. Recording under "Unreleased" streamlines the process of retrieving the full changelog entries by making it unnecessary to visit each linked PR and check the changelog section of the PR description.

That said, I've realized our team won't be creating releases for this repo, and introducing a new practice like this will be confusing.

I've removed the additions to the changelog file: 3d957cd. The changelog entries can be found in the PR description!

MajorLift requested a review from a team as a code owner November 16, 2023 00:03

MajorLift self-assigned this Nov 16, 2023

MajorLift added the team-wallet-framework label Nov 16, 2023

MajorLift requested a review from jiexi November 16, 2023 00:07

MajorLift mentioned this pull request Nov 16, 2023

Support optional networkClientId in usePPOM() #85

Closed

jiexi reviewed Nov 16, 2023

View reviewed changes

src/ppom-controller.ts Outdated Show resolved Hide resolved

jiexi reviewed Nov 16, 2023

View reviewed changes

src/ppom-controller.ts Outdated Show resolved Hide resolved

jiexi reviewed Nov 16, 2023

View reviewed changes

src/ppom-controller.ts Outdated Show resolved Hide resolved

jiexi reviewed Nov 16, 2023

View reviewed changes

src/ppom-controller.ts Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

MajorLift requested a review from jpuri November 16, 2023 17:28

This comment was marked as outdated.

Sign in to view

MajorLift changed the base branch from jl/use-networkClientId-3 to main January 29, 2024 19:26

MajorLift force-pushed the 231115-fix-any-provider-typing branch from 6a5a04a to 39e9f8c Compare January 29, 2024 19:30

This comment was marked as outdated.

Sign in to view

MajorLift force-pushed the 231115-fix-any-provider-typing branch from 39e9f8c to 934940f Compare January 29, 2024 19:35

MajorLift force-pushed the 231115-fix-any-provider-typing branch 2 times, most recently from 8812de5 to 4d84e41 Compare January 29, 2024 20:01

Linter fixes

8bcb05b

MajorLift force-pushed the 231115-fix-any-provider-typing branch 2 times, most recently from d03e901 to f413c9d Compare January 29, 2024 20:11

MajorLift added 2 commits January 29, 2024 23:09

Install @metamask/utils as dependency

88f8633

Add entries to depcheck ignore to fix yarn depcheck error

9f10d02

MajorLift force-pushed the 231115-fix-any-provider-typing branch from f413c9d to 46c8936 Compare January 30, 2024 04:51

MajorLift changed the title ~~Typing fixes for #85~~ Typing fixes for provider, ppom and other any usage Jan 30, 2024

MajorLift commented Jan 30, 2024

View reviewed changes

src/ppom-controller.ts Show resolved Hide resolved

MajorLift commented Jan 30, 2024

View reviewed changes

src/ppom-controller.ts Outdated Show resolved Hide resolved

jiexi reviewed Feb 1, 2024

View reviewed changes

src/ppom-controller.ts Outdated Show resolved Hide resolved

Replace null check for oldestChainId with non-null assertion

da5a54f

MajorLift force-pushed the 231115-fix-any-provider-typing branch from 9f5c709 to da5a54f Compare February 2, 2024 15:49

jiexi previously approved these changes Feb 2, 2024

View reviewed changes

MajorLift commented Feb 2, 2024

View reviewed changes

mcmire previously approved these changes Feb 6, 2024

View reviewed changes

MajorLift added 2 commits February 6, 2024 18:15

Replace null check for this.#ppom with non-null assertion

3ff8ebe

Replace unnecessary null fallback with non-null assertion

572cab2

MajorLift dismissed stale reviews from mcmire and jiexi via 572cab2 February 6, 2024 23:16

MajorLift added 3 commits February 7, 2024 10:10

Merge branch 'main' into 231115-fix-any-provider-typing

736419e

Merge branch 'main' into 231115-fix-any-provider-typing

a2dd86f

Linter fix

9d0baf2

MajorLift commented Feb 9, 2024

View reviewed changes

MajorLift force-pushed the 231115-fix-any-provider-typing branch from e358aa5 to 2d2a2de Compare February 9, 2024 06:36

seaona reviewed Feb 9, 2024

View reviewed changes

MajorLift force-pushed the 231115-fix-any-provider-typing branch 5 times, most recently from 7cdd21f to 5fc353e Compare February 9, 2024 16:38

Record CHANGELOG

9af9e41

MajorLift force-pushed the 231115-fix-any-provider-typing branch from 5fc353e to 9af9e41 Compare February 9, 2024 16:39

Remove changelog entries

3d957cd

jpuri approved these changes Feb 15, 2024

View reviewed changes

MajorLift merged commit 2d00a58 into main Feb 15, 2024
13 of 14 checks passed

MajorLift deleted the 231115-fix-any-provider-typing branch February 15, 2024 05:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typing fixes for `provider`, `ppom` and other `any` usage #89

Typing fixes for `provider`, `ppom` and other `any` usage #89

MajorLift commented Nov 16, 2023 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

socket-security bot commented Jan 29, 2024 •

edited

Loading

MajorLift Jan 30, 2024 •

edited

Loading

jiexi left a comment

MajorLift Feb 2, 2024 •

edited

Loading

mcmire left a comment

mcmire Feb 6, 2024

MajorLift Feb 6, 2024

mcmire Feb 6, 2024

MajorLift Feb 6, 2024

jpuri Feb 15, 2024

MajorLift Feb 9, 2024

MajorLift Feb 9, 2024

MajorLift Feb 9, 2024

MajorLift commented Feb 9, 2024

seaona Feb 9, 2024

MajorLift Feb 9, 2024

MajorLift Feb 9, 2024 •

edited

Loading

		\| JsonRpcSuccess<Json>
		\| (Omit<JsonRpcFailure, 'error'> & { error: unknown })

		@@ -5,6 +5,20 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
		and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

		## [Unreleased]

Typing fixes for provider, ppom and other any usage #89

Typing fixes for provider, ppom and other any usage #89

Conversation

MajorLift commented Nov 16, 2023 • edited Loading

Explanation

References

Changelog

Changed

Removed

Fixed

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

socket-security bot commented Jan 29, 2024 • edited Loading

Next steps

MajorLift Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

jiexi left a comment

Choose a reason for hiding this comment

MajorLift Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorLift commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorLift Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Typing fixes for `provider`, `ppom` and other `any` usage #89

Typing fixes for `provider`, `ppom` and other `any` usage #89

MajorLift commented Nov 16, 2023 •

edited

Loading

socket-security bot commented Jan 29, 2024 •

edited

Loading

MajorLift Jan 30, 2024 •

edited

Loading

MajorLift Feb 2, 2024 •

edited

Loading

MajorLift Feb 9, 2024 •

edited

Loading