Upgrade TransactionController to BaseControllerV2 #3758
Assignees
Labels
Comments
This was referenced Jan 9, 2024
3 tasks
|
Moving into blocked until we decide how to proceed with this given that the DevEx team is also making changes to this controller in order to make it multichain-compatible. |
13 tasks
mcmire
added a commit
that referenced
this issue
Mar 7, 2024
## Explanation Fundamentally, this commit changes TransactionController so that it inherits from BaseController v2 instead of BaseController v1. This affects the signature of the constructor and the way that state is updated inside of the controller, but it also prompts other changes: - Previously, the controller used a `hub` property, which was an instance of EventEmitter, to emit events at key points in the transaction processing flow. Now, a controller messenger is used instead, and all of the events now use a standard name. - Due to the superclass change, the controller state type is now `Record<string, Json>`. As a result, all state property types needed to be `Json`-compatible. Some types were defined as interfaces and could merely be changed to type aliases, but concessions needed to be made in other cases to achieve compatibility. - Due to the superclass change, once `update` is called, state is recursively frozen at runtime, which means that transactions cannot be modified directly. Hence, numerous places within TransactionController needed to be updated so that transactions were copied before being modified. Some methods were even changed so that instead of directly modifying a transaction, they now return a new version. - Tests did not reset Jest mocks correctly, and modules were mocked in a way where changes in one test bled over into others. This has been fixed. - Some tests made a new instance of TransactionController and then directly updated the TransactionController state. Since it is possible to instantiate TransactionController by passing initial state, these tests have been updated to do exactly this. ## References Fixes #3758. ## Changelog ### `@metamask/transaction-controller` ~~~ ### Added - Add new types for TransactionController messenger actions ([#3827](#3827)) - `TransactionControllerActions` - `TransactionControllerGetStateAction` - Add new types for TransactionController messenger events ([#3827](#3827)) - `TransactionControllerEvents` - `TransactionControllerIncomingTransactionBlockReceivedEvent` - `TransactionControllerPostTransactionBalanceUpdatedEvent` - `TransactionControllerSpeedupTransactionAddedEvent` - `TransactionControllerStateChangeEvent` - `TransactionControllerTransactionApprovedEvent` - `TransactionControllerTransactionConfirmedEvent` - `TransactionControllerTransactionDroppedEvent` - `TransactionControllerTransactionFailedEvent` - `TransactionControllerTransactionFinishedEvent` - `TransactionControllerTransactionNewSwapApprovalEvent` - `TransactionControllerTransactionNewSwapEvent` - `TransactionControllerTransactionPublishingSkipped` - `TransactionControllerTransactionRejectedEvent` - `TransactionControllerTransactionStatusUpdatedEvent` - `TransactionControllerTransactionSubmittedEvent` - `TransactionControllerUnapprovedTransactionAddedEvent` ### Changed - **BREAKING:** Change superclass of TransactionController from BaseController v1 to BaseController v2 ([#3827](#3827)) - Instead of accepting three arguments, the constructor now takes a single options argument. All of the existing options that were supported in the second argument are now a part of this options object, including `messenger`; `state` (the previous third argument) is also an option. - **BREAKING:** Rename `txHistoryLimit` option to `transactionHistoryLimit` ([#3827](#3827)) - **BREAKING:** Switch some type definitions from `interface` to `type` ([#3827](#3827)) - These types are affected: - `DappSuggestedGasFees` - `Log` - `MethodData` - `TransactionControllerState` (formerly `TransactionState`) - `TransactionParams` - `TransactionReceipt` - This is a breaking change because type aliases have different behavior from interfaces. Specifically, the `Json` type in `@metamask/utils`, which BaseController v2 controller state must conform to, is not compatible with interfaces. - **BREAKING:** Align `parsedRegistryMethod` in `MethodData` type with usage ([#3827](#3827)) - The type of this is now `{ name: string; args: { type: string }[]; } | { name?: any; args?: any; }`, which is a `Json`-compatible version of a type found in `eth-method-registry`. - **BREAKING:** Rename `TransactionState` to `TransactionControllerState` ([#3827](#3827)) - This change aligns this controller with other MetaMask controllers. - **BREAKING:** Update allowed events in `TransactionControllerMessenger` type ([#3827](#3827)) - The type of the restricted messenger that TransactionController takes must allow the following events: - `TransactionController:incomingTransactionBlockReceived` - `TransactionController:postTransactionBalanceUpdated` - `TransactionController:speedUpTransactionAdded` - `TransactionController:transactionApproved` - `TransactionController:transactionConfirmed` - `TransactionController:transactionDropped` - `TransactionController:transactionFinished` - `TransactionController:transactionFinished` - `TransactionController:transactionPublishingSkipped` - `TransactionController:transactionRejected` - `TransactionController:transactionStatusUpdated` - `TransactionController:transactionSubmitted` - `TransactionController:unapprovedTransactionAdded` - **BREAKING:** Update `TransactionMeta` type to be compatible with `Json` ([#3827](#3827)) - As dictated by BaseController v2, any types that are part of state need to be compatible with the `Json` type from `@metamask/utils`. - **BREAKING:** Transform `rpc` property on transaction errors so they're JSON-encodable ([#3827](#3827)) - This change also results in typing this property as `Json` instead of `unknown`, avoiding a "Type instantiation is excessively deep and possibly infinite" error when resolving the `TransactionControllerState` type. ### Removed - **BREAKING:** Remove `TransactionConfig` type ([#3827](#3827)) - The properties in this type have been absorbed into `TransactionControllerOptions`. - **BREAKING:** Remove `hub` property from TransactionController ([#3827](#3827)) - TransactionController now fully makes use of its messenger object to announce various kinds of activities. Instead of subscribing to an event like this: ``` transactionController.hub.on(eventName, ...) ``` use this: ``` messenger.subscribe('TransactionController:${eventName}', ...) ``` - The complete list of renamed events are: - `incomingTransactionBlock` -> `TransactionController:incomingTransactionBlockReceived` - `post-transaction-balance-updated` -> `TransactionController:postTransactionBalanceUpdated` - `transaction-approved` -> `TransactionController:transactionApproved` - `transaction-confirmed` -> `TransactionController:transactionConfirmed` - `transaction-dropped` -> `TransactionController:transactionDropped` - `transaction-finished` -> `TransactionController:transactionFinished` - `transaction-rejected` -> `TransactionController:transactionRejected` - `transaction-status-update` -> `TransactionController:transactionStatusUpdated` - `transaction-submitted` -> `TransactionController:transactionSubmitted` - `unapprovedTransaction` -> `TransactionController:unapprovedTransactionAdded` - Some events announced the state of specific transactions. These have been removed. Instead, subscribe to the appropriate generic event (which yields the transaction) and check for a specific transaction ID in your event handler: - `${transactionId}:finished` -> `TransactionController:transactionFinished` - `${transactionId}:speedup` -> `TransactionController:speedUpTransactionAdded` - `${transactionId}:publish-skip` -> `TransactionController:transactionPublishingSkipped` ### Fixed - Fix various methods so that they no longer update transactions in state directly but only via `update` ([#3827](#3827)) - `addTransaction` - `confirmExternalTransaction` - `speedUpTransaction` - `updateCustodialTransaction` - `updateSecurityAlertResponse` - `updateTransaction` - Fix `handleMethodData` method to update state with an empty registry object instead of blowing up if registry could be found ([#3827](#3827)) ~~~ --- Co-authored-by: Jongsun Suh <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
To match other controllers, have TransactionController inherit from BaseController v1 instead of v2. The exact changes are detailed here: #3707
The text was updated successfully, but these errors were encountered: