Skip to content

Commit

Permalink
fix: Fix Engine context types (#9566)
Browse files Browse the repository at this point in the history
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

The `context` property of `Engine` was typed as `any`, effectively
disabling type checking for most controller interactions. The `any` has
been removed, restoring controller type checking.

## **Related issues**

This problem was introduced in #6749

## **Manual testing steps**

N/A

## **Screenshots/Recordings**

N/A

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
  • Loading branch information
Gudahtt authored May 8, 2024
1 parent 71b4226 commit c0c3b8e
Show file tree
Hide file tree
Showing 14 changed files with 294 additions and 195 deletions.
2 changes: 1 addition & 1 deletion app/core/BackgroundBridge/BackgroundBridge.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ export class BackgroundBridge extends EventEmitter {

try {
const pc = Engine.context.PermissionController;
const controllerMessenger = Engine.context.controllerMessenger;
const controllerMessenger = Engine.controllerMessenger;
controllerMessenger.subscribe(
`${pc.name}:stateChange`,
(subjectWithPermission) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,17 @@ import { addTransaction } from '../../../util/transaction-controller';
jest.mock('../../../util/networks');
jest.mock('../../../util/transactions');
jest.mock('../../../util/address');
jest.mock('../../Engine');
jest.mock('../../Engine', () => ({
context: {
NetworkController: { setProviderType: jest.fn() },
PreferencesController: {
state: { selectedAddress: '0xMockSenderAddress' },
},
TransactionController: {
addTransaction: jest.fn(),
},
},
}));
jest.mock('../../NotificationManager');
jest.mock('../../../../locales/i18n', () => ({
strings: jest.fn().mockImplementation((key) => key),
Expand All @@ -33,12 +43,6 @@ const mockDeeplinkManager = {

const mockOrigin = 'testOrigin';

Engine.context.TransactionController = { addTransaction: jest.fn() };
Engine.context.PreferencesController = {
state: { selectedAddress: '0xMockSenderAddress' },
};
Engine.context.NetworkController = { setProviderType: jest.fn() };

describe('approveTransaction', () => {
const spyGetNetworkTypeById = jest.spyOn(
NetworksUtilsModule,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ async function approveTransaction({

if (chain_id) {
const newNetworkType = getNetworkTypeById(chain_id);
// @ts-expect-error TODO: Consolidate the network types used here with the controller-utils types
NetworkController.setProviderType(newNetworkType);
}

Expand Down
159 changes: 86 additions & 73 deletions app/core/Engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,51 @@ export interface EngineState {
AccountsController: AccountsControllerState;
}

/**
* All mobile controllers, keyed by name
*/
interface Controllers {
AccountsController: AccountsController;
AccountTrackerController: AccountTrackerController;
AddressBookController: AddressBookController;
ApprovalController: ApprovalController;
AssetsContractController: AssetsContractController;
CurrencyRateController: CurrencyRateController;
GasFeeController: GasFeeController;
KeyringController: KeyringController;
LoggingController: LoggingController;
NetworkController: NetworkController;
NftController: NftController;
NftDetectionController: NftDetectionController;
// TODO: Fix permission types
PermissionController: PermissionController<any, any>;
PhishingController: PhishingController;
PreferencesController: PreferencesController;
PPOMController: PPOMController;
TokenBalancesController: TokenBalancesController;
TokenListController: TokenListController;
TokenDetectionController: TokenDetectionController;
TokenRatesController: TokenRatesController;
TokensController: TokensController;
TransactionController: TransactionController;
SignatureController: SignatureController;
///: BEGIN:ONLY_INCLUDE_IF(snaps)
SnapController: SnapController;
SubjectMetadataController: SubjectMetadataController;
///: END:ONLY_INCLUDE_IF
SwapsController: SwapsController;
}

/**
* Controllers that area always instantiated
*/
type RequiredControllers = Omit<Controllers, 'PPOMController'>;

/**
* Controllers that are sometimes not instantiated
*/
type OptionalControllers = Pick<Controllers, 'PPOMController'>;

/**
* Core controller responsible for composing other metamask controllers together
* and exposing convenience methods for common wallet operations.
Expand All @@ -300,35 +345,7 @@ class Engine {
/**
* A collection of all controller instances
*/
context:
| {
AccountTrackerController: AccountTrackerController;
AddressBookController: AddressBookController;
ApprovalController: ApprovalController;
AssetsContractController: AssetsContractController;
CurrencyRateController: CurrencyRateController;
GasFeeController: GasFeeController;
KeyringController: KeyringController;
LoggingController: LoggingController;
NetworkController: NetworkController;
NftController: NftController;
NftDetectionController: NftDetectionController;
// TODO: Fix permission types
PermissionController: PermissionController<any, any>;
PhishingController: PhishingController;
PreferencesController: PreferencesController;
PPOMController?: PPOMController;
TokenBalancesController: TokenBalancesController;
TokenListController: TokenListController;
TokenDetectionController: TokenDetectionController;
TokenRatesController: TokenRatesController;
TokensController: TokensController;
TransactionController: TransactionController;
SignatureController: SignatureController;
SwapsController: SwapsController;
AccountsController: AccountsController;
}
| any;
context: RequiredControllers & Partial<OptionalControllers>;
/**
* The global controller messenger.
*/
Expand Down Expand Up @@ -1008,7 +1025,7 @@ class Engine {

const codefiTokenApiV2 = new CodefiTokenPricesServiceV2();

const controllers = [
const controllers: Controllers[keyof Controllers][] = [
keyringController,
accountTrackerController,
new AddressBookController(),
Expand Down Expand Up @@ -1212,42 +1229,37 @@ class Engine {
];

if (isBlockaidFeatureEnabled()) {
try {
const ppomController = new PPOMController({
chainId: networkController.state.providerConfig.chainId,
blockaidPublicKey: process.env.BLOCKAID_PUBLIC_KEY as string,
cdnBaseUrl: process.env.BLOCKAID_FILE_CDN as string,
// @ts-expect-error TODO: Resolve/patch mismatch between base-controller versions. Before: never, never. Now: string, string, which expects 3rd and 4th args to be informed for restrictedControllerMessengers
messenger: this.controllerMessenger.getRestricted<
'PPOMController',
never,
'NetworkController:stateChange'
>({
name: 'PPOMController',
allowedEvents: [`${networkController.name}:stateChange`],
}),
onPreferencesChange: (listener) =>
this.controllerMessenger.subscribe(
`${preferencesController.name}:stateChange`,
listener,
),
provider: networkController.getProviderAndBlockTracker()
.provider as any,
ppomProvider: {
PPOM: PPOM as any,
ppomInit,
},
storageBackend: new RNFSStorageBackend('PPOMDB'),
securityAlertsEnabled:
initialState.PreferencesController?.securityAlertsEnabled ?? false,
state: initialState.PPOMController,
nativeCrypto: Crypto as any,
});
controllers.push(ppomController as any);
} catch (e) {
Logger.log(`Error initializing PPOMController: ${e}`);
return;
}
const ppomController = new PPOMController({
chainId: networkController.state.providerConfig.chainId,
blockaidPublicKey: process.env.BLOCKAID_PUBLIC_KEY as string,
cdnBaseUrl: process.env.BLOCKAID_FILE_CDN as string,
// @ts-expect-error TODO: Resolve/patch mismatch between base-controller versions. Before: never, never. Now: string, string, which expects 3rd and 4th args to be informed for restrictedControllerMessengers
messenger: this.controllerMessenger.getRestricted<
'PPOMController',
never,
'NetworkController:stateChange'
>({
name: 'PPOMController',
allowedEvents: [`${networkController.name}:stateChange`],
}),
onPreferencesChange: (listener) =>
this.controllerMessenger.subscribe(
`${preferencesController.name}:stateChange`,
listener,
),
provider: networkController.getProviderAndBlockTracker()
.provider as any,
ppomProvider: {
PPOM: PPOM as any,
ppomInit,
},
storageBackend: new RNFSStorageBackend('PPOMDB'),
securityAlertsEnabled:
initialState.PreferencesController?.securityAlertsEnabled ?? false,
state: initialState.PPOMController,
nativeCrypto: Crypto as any,
});
controllers.push(ppomController);
}

// set initial state
Expand Down Expand Up @@ -1278,9 +1290,7 @@ class Engine {
...context,
[controller.name]: controller,
}),
{
controllerMessenger: this.controllerMessenger,
},
{},
) as typeof this.context;

const {
Expand All @@ -1293,6 +1303,7 @@ class Engine {
nfts.setApiKey(process.env.MM_OPENSEA_KEY);
}

// @ts-expect-error TODO: Align transaction types between keyring and TransactionController
transaction.configure({ sign: keyring.signTransaction.bind(keyring) });

transaction.hub.on('incomingTransactionBlock', (blockNumber: number) => {
Expand Down Expand Up @@ -1392,7 +1403,12 @@ class Engine {
} = this.context;
const { provider } = NetworkController.getProviderAndBlockTracker();

// Skip configuration if this is called before the provider is initialized
if (!provider) {
return;
}
provider.sendAsync = provider.sendAsync.bind(provider);
// @ts-expect-error TODO: Align provider types
AccountTrackerController.configure({ provider });
AssetsContractController.configure({ provider });

Expand Down Expand Up @@ -1425,10 +1441,7 @@ class Engine {
const networkProvider = NetworkController.state.providerConfig;
const conversionRate =
CurrencyRateController.state?.currencyRates?.[networkProvider?.ticker]
?.conversionRate === null
? 0
: CurrencyRateController.state?.currencyRates?.[networkProvider?.ticker]
?.conversionRate;
?.conversionRate ?? 0;
const { accountsByChainId } = AccountTrackerController.state;

const { tokens } = TokensController.state;
Expand Down
Loading

0 comments on commit c0c3b8e

Please sign in to comment.