-
-
Notifications
You must be signed in to change notification settings - Fork 6
Typing fixes for provider, ppom and other any usage
#89
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.
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
Changes from all commits
8bcb05b
88f8633
9f10d02
b07b624
e5af86e
5223546
7f89517
f8dcde3
04cc64b
19b74f4
ed6f2e1
da5a54f
3ff8ebe
572cab2
736419e
a2dd86f
9d0baf2
9af9e41
3d957cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,8 @@ | |
| "@types/*", | ||
| "prettier-plugin-packagejson", | ||
| "ts-node", | ||
| "typedoc" | ||
| "typedoc", | ||
| "@yarnpkg/*", | ||
| "clipanion" | ||
| ] | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,19 @@ | ||
| export * from './ppom-controller'; | ||
| 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'; | ||
|
Comment on lines
+1
to
+17
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There should be no breaking changes here. This is the list of existing package-level exports from the Explicit enumeration of exports is safer, and also allows sharing exports internally, which comes in handy with e.g. |
||
|
|
||
| export type { StorageBackend, StorageKey } from './ppom-storage'; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,17 @@ | ||
| import type { RestrictedControllerMessenger } from '@metamask/base-controller'; | ||
| import { BaseControllerV2 } from '@metamask/base-controller'; | ||
| import { safelyExecute, timeoutFetch } from '@metamask/controller-utils'; | ||
| import type { NetworkControllerStateChangeEvent } from '@metamask/network-controller'; | ||
| import type { | ||
| NetworkControllerStateChangeEvent, | ||
| NetworkState, | ||
| Provider, | ||
| } from '@metamask/network-controller'; | ||
| import type { | ||
| JsonRpcFailure, | ||
| Json, | ||
| JsonRpcParams, | ||
| JsonRpcSuccess, | ||
| } from '@metamask/utils'; | ||
| import { Mutex } from 'await-semaphore'; | ||
|
|
||
| import type { | ||
|
|
@@ -52,6 +62,14 @@ const ALLOWED_PROVIDER_CALLS = [ | |
| 'trace_filter', | ||
| ]; | ||
|
|
||
| // 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>; | ||
|
Comment on lines
+65
to
+71
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wrote temporary PPOM type that expresses a minimal interface. Added TODO so this would be eventually replaced with a full type. |
||
|
|
||
| /** | ||
| * @type PPOMFileVersion | ||
| * @augments FileMetadata | ||
|
|
@@ -121,7 +139,7 @@ const versionInfoFileHeaders = { | |
|
|
||
| export type UsePPOM = { | ||
| type: `${typeof controllerName}:usePPOM`; | ||
| handler: (callback: (ppom: any) => Promise<any>) => Promise<any>; | ||
| handler: (callback: (ppom: PPOM) => Promise<unknown>) => Promise<unknown>; | ||
| }; | ||
|
|
||
| export type UpdatePPOM = { | ||
|
|
@@ -147,24 +165,23 @@ export type PPOMControllerInitialisationStateChangeEvent = { | |
| payload: [PPOMInitialisationStatusType]; | ||
| }; | ||
|
|
||
| export type PPOMControllerEvents = | ||
| | PPOMControllerInitialisationStateChangeEvent | ||
| | NetworkControllerStateChangeEvent; | ||
| export type PPOMControllerEvents = PPOMControllerInitialisationStateChangeEvent; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For context, these changes align with changes we've been making to controllers in the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for adding context here. To be clear, I didn't use the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks a lot @MajorLift for these improvements 🙏 |
||
|
|
||
| export type AllowedEvents = NetworkControllerStateChangeEvent; | ||
|
|
||
| export type PPOMControllerMessenger = RestrictedControllerMessenger< | ||
| typeof controllerName, | ||
| PPOMControllerActions, | ||
| | PPOMControllerInitialisationStateChangeEvent | ||
| | NetworkControllerStateChangeEvent, | ||
| PPOMControllerEvents | AllowedEvents, | ||
| never, | ||
| NetworkControllerStateChangeEvent['type'] | ||
| AllowedEvents['type'] | ||
| >; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| type PPOMProvider = { | ||
| ppomInit: (wasmFilePath: string) => Promise<void>; | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| PPOM: any; | ||
| PPOM: PPOM; | ||
| }; | ||
|
|
||
| /** | ||
|
|
@@ -182,15 +199,15 @@ export class PPOMController extends BaseControllerV2< | |
| PPOMState, | ||
| PPOMControllerMessenger | ||
| > { | ||
| #ppom: any; | ||
| #ppom: PPOM | undefined; | ||
|
|
||
| #provider: any; | ||
| #provider: Provider; | ||
|
|
||
| #storage: PPOMStorage; | ||
|
|
||
| #refreshDataInterval: any; | ||
| #refreshDataInterval: ReturnType<typeof setInterval> | undefined; | ||
|
|
||
| #fileScheduleInterval: any; | ||
| #fileScheduleInterval: ReturnType<typeof setInterval> | undefined; | ||
|
|
||
| /* | ||
| * This mutex is used to prevent concurrent usage of the PPOM instance | ||
|
|
@@ -269,10 +286,18 @@ export class PPOMController extends BaseControllerV2< | |
| }: { | ||
| chainId: string; | ||
| messenger: PPOMControllerMessenger; | ||
| provider: any; | ||
| provider: Provider; | ||
| storageBackend: StorageBackend; | ||
| securityAlertsEnabled: boolean; | ||
| onPreferencesChange: (callback: (perferenceState: any) => void) => void; | ||
| onPreferencesChange: ( | ||
| callback: ( | ||
| // TOOD: Replace with `PreferencesState` from `@metamask/preferences-controller` | ||
| preferencesState: { securityAlertsEnabled: boolean } & Record< | ||
| string, | ||
| Json | ||
| >, | ||
|
Comment on lines
+294
to
+298
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wrote temporary |
||
| ) => void, | ||
| ) => void; | ||
| ppomProvider: PPOMProvider; | ||
| cdnBaseUrl: string; | ||
| providerRequestLimit?: number; | ||
|
|
@@ -361,7 +386,7 @@ export class PPOMController extends BaseControllerV2< | |
| * @param callback - Callback to be invoked with PPOM. | ||
| */ | ||
| async usePPOM<Type>( | ||
| callback: (ppom: any) => Promise<Type>, | ||
| callback: (ppom: PPOM) => Promise<Type>, | ||
| ): Promise<Type & { providerRequestsCount: Record<string, number> }> { | ||
| if (!this.#securityAlertsEnabled) { | ||
| throw Error('User has securityAlertsEnabled set to false'); | ||
|
|
@@ -378,7 +403,9 @@ export class PPOMController extends BaseControllerV2< | |
| this.#providerRequests = 0; | ||
| this.#providerRequestsCount = {}; | ||
| return await this.#ppomMutex.use(async () => { | ||
| const result = await callback(this.#ppom); | ||
| // `this.#ppom` is defined in `#initPPOMIfRequired` | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| const result = await callback(this.#ppom!); | ||
|
|
||
| return { | ||
| ...result, | ||
|
|
@@ -503,7 +530,7 @@ export class PPOMController extends BaseControllerV2< | |
| * 2. if network is supported by blockaid add / update network in state variable chainStatus | ||
| * 2. instantiate PPOM for new network if user has enabled security alerts | ||
| */ | ||
| #onNetworkChange(networkControllerState: any): void { | ||
| #onNetworkChange(networkControllerState: NetworkState): void { | ||
| const id = addHexPrefix(networkControllerState.providerConfig.chainId); | ||
| if (id === this.#chainId) { | ||
| return; | ||
|
|
@@ -537,7 +564,13 @@ export class PPOMController extends BaseControllerV2< | |
| /* | ||
| * enable / disable PPOM validations as user changes preferences | ||
| */ | ||
| #onPreferenceChange(preferenceControllerState: any): void { | ||
| #onPreferenceChange( | ||
| // TOOD: Replace with `PreferencesState` from `@metamask/preferences-controller` | ||
| preferenceControllerState: { securityAlertsEnabled: boolean } & Record< | ||
| string, | ||
| Json | ||
| >, | ||
|
Comment on lines
+568
to
+572
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. |
||
| ): void { | ||
| const blockaidEnabled = preferenceControllerState.securityAlertsEnabled; | ||
| if (blockaidEnabled === this.#securityAlertsEnabled) { | ||
| return; | ||
|
|
@@ -819,9 +852,11 @@ export class PPOMController extends BaseControllerV2< | |
| const currentTimestamp = new Date().getTime(); | ||
|
|
||
| const chainIds = Object.keys(this.state.chainStatus); | ||
| const oldChaninIds: any[] = chainIds.filter( | ||
| const oldChainIds = chainIds.filter( | ||
| (chainId) => | ||
| (this.state.chainStatus[chainId] as any).lastVisited < | ||
| // `chainId` is of type `keyof typeof this.state.chainStatus` | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| this.state.chainStatus[chainId]!.lastVisited < | ||
| currentTimestamp - NETWORK_CACHE_DURATION && | ||
| chainId !== this.#chainId, | ||
| ); | ||
|
|
@@ -832,11 +867,13 @@ export class PPOMController extends BaseControllerV2< | |
| Number(this.state.chainStatus[c2]?.lastVisited) - | ||
| Number(this.state.chainStatus[c1]?.lastVisited), | ||
| )[NETWORK_CACHE_LIMIT.MAX]; | ||
| oldChaninIds.push(oldestChainId); | ||
| // `oldestChainId` will always be defined, as `chainIds` is guaranteed to have at least `NETWORK_CACHE_LIMIT.MAX` elements | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| oldChainIds.push(oldestChainId!); | ||
| } | ||
|
|
||
| const chainStatus = { ...this.state.chainStatus }; | ||
| oldChaninIds.forEach((chainId) => { | ||
| oldChainIds.forEach((chainId) => { | ||
| delete chainStatus[chainId]; | ||
| }); | ||
|
|
||
|
|
@@ -918,6 +955,7 @@ export class PPOMController extends BaseControllerV2< | |
| url: string, | ||
| options: Record<string, unknown> = {}, | ||
| method = 'GET', | ||
| // TODO: Fix `any` usage - provide minimum expected type for `response`. | ||
| ): Promise<any> { | ||
| const response = await safelyExecute( | ||
| async () => | ||
|
|
@@ -996,8 +1034,12 @@ export class PPOMController extends BaseControllerV2< | |
| */ | ||
| async #jsonRpcRequest( | ||
| method: string, | ||
| params: Record<string, unknown>, | ||
| ): Promise<any> { | ||
| params: JsonRpcParams, | ||
| ): Promise< | ||
| | JsonRpcSuccess<Json> | ||
| | (Omit<JsonRpcFailure, 'error'> & { error: unknown }) | ||
|
Comment on lines
+1039
to
+1040
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I used the This is because The Note: |
||
| | ReturnType<(typeof PROVIDER_ERRORS)[keyof typeof PROVIDER_ERRORS]> | ||
| > { | ||
| return new Promise((resolve) => { | ||
| // Resolve with error if number of requests from PPOM to provider exceeds the limit for the current transaction | ||
| if (this.#providerRequests > this.#providerRequestLimit) { | ||
|
|
@@ -1018,7 +1060,7 @@ export class PPOMController extends BaseControllerV2< | |
| // Invoke provider and return result | ||
| this.#provider.sendAsync( | ||
| createPayload(method, params), | ||
| (error: Error, res: any) => { | ||
| (error, res: JsonRpcSuccess<Json>) => { | ||
mcmire marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (error) { | ||
| resolve({ | ||
| jsonrpc: '2.0', | ||
|
|
@@ -1039,7 +1081,7 @@ export class PPOMController extends BaseControllerV2< | |
| * | ||
| * It will load the data files from storage and pass data files and wasm file to ppom. | ||
| */ | ||
| async #getPPOM(chainId: string): Promise<any> { | ||
| async #getPPOM(chainId: string): Promise<PPOM> { | ||
| // PPOM initialisation in contructor fails for react native | ||
| // thus it is added here to prevent validation from failing. | ||
| await this.#initialisePPOM(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolves following error from running
yarn depcheck