MetaMask / MetaMask/metamask-extension
Multichain: Audit MetaMaskController's `getApi` usages for chain ID
- Dominant language
- TypeScript
- Stars
- 13.2k
- Forks
- 5.6k
- Avg merge
- 2d 5h
- Merged PRs (30d)
- 451
Description
### What is this about?
Work was done months ago to bring Controllers to a multichain architecture. That said, there could be regression. All of the methods that `app/scripts/metamask-controller.js` 's `getApi` method should be audited to ensure they are passed a `chainId` and not assume the global chain Id.
Per Mark:
> Back to your question, the general area of concern I had was "every background operation that assumes it's for the globally selected chain ID". I had mentioned in a previous meeting something about auditing controller method calls or background API calls. It should be the case that all controller operations pertaining to a given chain accept a chainId parameter (apart from TokenBalancesController maybe, which someone is handling). But in many cases it's optional, and the globally selected chain is used as a default. Once we enable multichain, that would be a bug. The chain ID needs to be passed through everywhere.
> Some subset of them accept an optional chainId or networkClientId. We could read each one to come up with that smaller list, then look at the callsites to ensure that it's always passed in.
## WIP Audit
```
Legend:
* "-" have been studied and either have no reliance on chainId or properly accept a chainId argument
* "!" likely need controller work to not assume current globally selected chain ID
* "?" require more digging
CurrencyRateController
=================================
- setCurrentCurrency
- startPolling
- stopPollingByPollingToken
PreferencesController
=================================
- setUseBlockie
- setUseNonceField
- setUsePhishDetect
- setUseMultiAccountBalanceChecker
- setUseSafeChainsListValidation
- setUseTokenDetection
- setUseNftDetection
- setUse4ByteResolution
- setUseCurrencyRateCheck
- setOpenSeaEnabled
- getUseRequestQueue
- setSecurityAlertsEnabled
- setAddSnapAccountEnabled
- setWatchEthereumAccountEnabled
- setSolanaSupportEnabled
- setBitcoinSupportEnabled
- setBitcoinTestnetSupportEnabled
- setUseExternalNameSources
- setUseTransactionSimulations
- setIpfsGateway
- setIsIpfsGatewayEnabled
- setUseAddressBarEnsResolution
- setCurrentLocale
- setIncomingTransactionsPreferences
- setServiceWorkerKeepAlivePreference
- setFeatureFlag
- setPreference
- addKnownMethodData
- setDismissSeedBackUpReminder
- setAdvancedGasFee
- setTheme
- setSnapsAddSnapAccountModalDismissed
- setSelectedAddress
- setUseRequestQueue
- setPasswordForgotten
- toggleExternalServices
MetaMetricsController
=================================
`app/scripts/controllers/metametrics.ts` is affected heavily by https://github.com/MetaMask/metamask-extension/issues/28304
- setParticipateInMetaMetrics
- setDataCollectionForMarketing
- setMarketingCampaignCookieId
- metaMetricsController
- trackEvent
- trackPage
- createEventFragment
- updateEventFragment
- finalizeEventFragment
KeyringController
=================================
- submitQRCryptoHDKey
- submitQRCryptoAccount
- cancelQRSynchronization
- submitQRSignature
- cancelQRSignRequest
- setLocked
- createNewVaultAndKeychain
- createNewVaultAndRestore
- exportAccount
AccountsController
=================================
- getNextAvailableAccountName
- setSelectedInternalAccount
- setAccountName
- setAccountLabel
MetaMetricsDataDeletionController
=================================
- updateDataDeletionTaskStatus
- createMetaMetricsDataDeletionTask
MultichainBalancesController
=================================
- updateBalances
- updateBalance
NameController
=================================
- setName
- updateProposedNames
NotificationServicesController
=================================
? checkAccountsPresence
? createOnChainTriggers
? deleteOnChainTriggersByAccount
? updateOnChainTriggersByAccount
? fetchAndUpdateMetamaskNotifications
? markMetamaskNotificationsAsRead
? setFeatureAnnouncementsEnabled
? enablePushNotifications
? disablePushNotifications
? updateTriggerPushNotifications
? enableMetamaskNotifications
? disableMetamaskNotifications
UserStorageController
=================================
- enableProfileSyncing
- disableProfileSyncing
- setIsProfileSyncingEnabled
- syncInternalAccountsWithUserStorage
- performDeleteStorageAllFeatureEntries
AuthenticationController
=================================
- performSignIn
- performSignOut
AssetsContractController
=================================
! getBalancesInSingleCall - Calls `getCorrectChainId` which has a `NetworkController:getState` usage
NftDetectionController
=================================
! detectNfts - Uses `NetworkController:getNetworkClientById` to get the `chainId`
TransactionController
=================================
- updateTransaction
- approveTransactionsWithSameNonce
- retryTransaction
- stopTransaction
- createSpeedUpTransaction
- estimateGasFee
? getNonceLock - `networkClientId` is optional
! getTransactions - uses `getChainId`, `this.#getGlobalChainId()`, `this.#getGlobalNetworkClientId()`
- updateEditableParams
- updateTransactionGasFees
- updateTransactionSendFlowHistory
- updatePreviousGasParams
- abortTransactionSigning
? getLayer1GasFee - `chainId` is optional
DecryptMessageController
=================================
- decryptMessage
- decryptMessageInline
- cancelDecryptMessage
EncryptionPublicKeyController
=================================
- encryptionPublicKey
- cancelEncryptionPublicKey
OnboardingController
=================================
- setSeedPhraseBackedUp
- completeOnboarding
- setFirstTimeFlowType
AlertController
=================================
- setAlertEnabledness
- setUnconnectedAccountAlertShown
- setWeb3ShimUsageAlertDismissed
SmartTransactionsController
=================================
- setStatusRefreshInterval
! updateSmartTransaction - falls back to #this.chainId and uses `NetworkController:getNetworkClientById`
! fetchLiveness - uses `this.#getChainId({ networkClientId })`
! cancelSmartTransaction - uses `this.#getChainId({ networkClientId })`
! submitSignedTransactions - uses `this.#getChainId({ networkClientId })`
- clearFees
! getFees - uses `this.#getChainId({ networkClientId })`
TokensController
=================================
- ignoreTokens
! addImportedTokens - Calls 'NetworkController:getNetworkClientById'
! addDetectedTokens - Uses an initial chainId as fallback. Additionally has an `onNetworkDidChange` to store the fallback chainId
- addToken
? updateTokenType - `networkClientId` is an optional param
TokenRatesController
=================================
- startPolling
- stopPollingByPollingToken
ApprovalController
=================================
- accept
- reject
- addAndShowApprovalRequest
TokenDetectionController
=================================
? detectTokens - Accepts `networkClientId`
Backup
=================================
- backupUserData
- restoreUserData
AppStateController
=================================
- removePollingToken
- addPollingToken
- setLastActiveTime
- setCurrentExtensionPopupId
- setDefaultHomeActiveTabName
- setConnectedStatusPopoverHasBeenShown
- setRecoveryPhraseReminderHasBeenShown
- setRecoveryPhraseReminderLastShown
- setTermsOfUseLastAgreed
- setSurveyLinkLastClickedOrClosed
- setOnboardingDate
- setLastViewedUserSurvey
- setNewPrivacyPolicyToastClickedOrClosed
- setNewPrivacyPolicyToastShownDate
- setSnapsInstallPrivacyWarningShownStatus
- setOutdatedBrowserWarningLastShown
- setShowTestnetMessageInDropdown
- setShowBetaHeader
- setShowPermissionsTour
- setShowAccountBanner
- setShowNetworkBanner
- updateNftDropDownState
- setFirstTimeUsedNetwork
- setSwitchedNetworkDetails
- clearSwitchedNetworkDetails
- setSwitchedNetworkNeverShowMessage
- getLastInteractedConfirmationInfo
- setLastInteractedConfirmationInfo
GasFeeController
=================================
- getTimeEstimate
- stopPollingByPollingToken
- startPollingByNetworkClientId
AnnouncementController
=================================
- resetViewed
- updateViewed
PermissionController
=================================
- grantPermissionsIncremental
- grantPermissions
- revokePermissions
- acceptPermissionsRequest
- rejectPermissionsRequest
-
NotificationManager
=================================
- markAsAutomaticallyClosed
NetworkController
=================================
? setActiveNetwork - Will this go away?
? setActiveNetworkConfigurationId - Will this go away?
? rollbackToPreviousProvider
- addNetwork
- updateNetwork
- removeNetwork
- getCurrentNetworkEIP1559Compatibility
- getNetworkConfigurationByNetworkClientId
SelectedNetworkController
=================================
? setNetworkClientIdForDomain - Will this go away?
NftController
=================================
? addNft - `networkClientId` is not required
? addNftVerifyOwnership - `networkClientId` is not required
? removeAndIgnoreNft - `networkClientId` is not required
? removeNft - `networkClientId` is not required
? checkAndUpdateAllNftsOwnershipStatus - `networkClientId` is not required
? checkAndUpdateSingleNftOwnershipStatus - `networkClientId` is not required
- getNFTContractInfo
? isNftOwner - `networkClientId` is not required
AddressBookController
=================================
- set
- delete
ENSController
=================================
- reverseResolveAddress
MMIController
=================================
- connectCustodyAddresses
- getCustodianAccounts
- getCustodianTransactionDeepLink
- getCustodianConfirmDeepLink
- getCustodianSignMessageDeepLink
- getCustodianToken
- getCustodianJWTList
- getAllCustodianAccountsWithToken
- setCustodianNewRefreshToken
CustodyController
=================================
- setWaitForConfirmDeepLinkDialog
- getConfiguration
- removeAddTokenConnectRequest
- setConnectionRequest
- showInteractiveReplacementTokenBanner
- setCustodianDeepLink
- setNoteToTraderMessage
- logAndStoreApiRequest
SnapController
=================================
- disableSnap
- enableSnap
- install
- removeSnap
- handleSnapRequest
- revokeDynamicSnapPermissions
- dismissNotifications
- markNotificationsAsRead
- SnapController:disconnectOrigin
AccountOrderController
=================================
- updateNetworksList
- updateAccountsList
- updateHiddenAccountsList
PhishingController
=================================
- maybeUpdateState
-
Misc
=================================
- getRequestAccountTabIds
- openMetamaskTabsIDs
- addNewAccount
- getSeedPhrase
- submitPassword
- verifyPassword
- decodeTransactionData
- throwTestError
- trackInsightSnapView
- resetAccount
- removeAccount
- importAccountWithStrategy
- getAccountsBySnapId
- connectHardware
- forgetDevice
- checkHardwareStatus
- getDeviceNameForMetric
- unlockHardwareWalletAccount
- attemptLedgerTransportCreation
- getTokenStandardAndDetails
- getTokenSymbol
- estimateGas
? addTransaction - does `dappRequest?.networkClientId ?? this.networkController.state.selectedNetworkClientId,`
? getAddTransactionRequest - does `dappRequest?.networkClientId ?? this.networkController.state.selectedNetworkClientId,`
- createTransactionEventFragment
- getPermissionBackgroundApiMethods
// Left to audit:
getApi() {
return {
getProviderConfig: () =>
getProviderConfig({
metamask: this.networkController.state,
})
deleteInterface: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SnapInterfaceController:deleteInterface',
),
updateInterfaceState: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SnapInterfaceController:updateInterfaceState',
),
// swaps
fetchAndSetQuotes: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:fetchAndSetQuotes',
),
setSelectedQuoteAggId: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSelectedQuoteAggId',
),
resetSwapsState: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:resetSwapsState',
),
setSwapsTokens: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsTokens',
),
clearSwapsQuotes: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:clearSwapsQuotes',
),
setApproveTxId: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setApproveTxId',
),
setTradeTxId: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setTradeTxId',
),
setSwapsTxGasPrice: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsTxGasPrice',
),
setSwapsTxGasLimit: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsTxGasLimit',
),
setSwapsTxMaxFeePerGas: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsTxMaxFeePerGas',
),
setSwapsTxMaxFeePriorityPerGas: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsTxMaxFeePriorityPerGas',
),
safeRefetchQuotes: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:safeRefetchQuotes',
),
stopPollingForQuotes: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:stopPollingForQuotes',
),
setBackgroundSwapRouteState: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setBackgroundSwapRouteState',
),
resetPostFetchState: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:resetPostFetchState',
),
setSwapsErrorKey: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsErrorKey',
),
setInitialGasEstimate: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setInitialGasEstimate',
),
setCustomApproveTxData: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setCustomApproveTxData',
),
setSwapsLiveness: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsLiveness',
),
setSwapsFeatureFlags: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsFeatureFlags',
),
setSwapsUserFeeLevel: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsUserFeeLevel',
),
setSwapsQuotesPollingLimitEnabled: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsQuotesPollingLimitEnabled',
),
// Bridge
[BridgeBackgroundAction.SET_FEATURE_FLAGS]:
this.controllerMessenger.call.bind(
this.controllerMessenger,
`${BRIDGE_CONTROLLER_NAME}:${BridgeBackgroundAction.SET_FEATURE_FLAGS}`,
),
[BridgeUserAction.SELECT_SRC_NETWORK]: this.controllerMessenger.call.bind(
this.controllerMessenger,
`${BRIDGE_CONTROLLER_NAME}:${BridgeUserAction.SELECT_SRC_NETWORK}`,
),
[BridgeUserAction.SELECT_DEST_NETWORK]:
this.controllerMessenger.call.bind(
this.controllerMessenger,
`${BRIDGE_CONTROLLER_NAME}:${BridgeUserAction.SELECT_DEST_NETWORK}`,
),
[BridgeUserAction.UPDATE_QUOTE_PARAMS]:
this.controllerMessenger.call.bind(
this.controllerMessenger,
`${BRIDGE_CONTROLLER_NAME}:${BridgeUserAction.UPDATE_QUOTE_PARAMS}`,
),
}
```
Contributor guide
Research direction
Start in app/scripts/metamask-controller.js and review the listed getApi usages across the controllers, beginning with entries marked ! or ?. Compare each call with its chainId or networkClientId handling and trace the relevant callsites. Done means the audit is complete and operations for a given chain no longer rely on the globally selected chain ID.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- api, backend
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100