-
-
Notifications
You must be signed in to change notification settings - Fork 185
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
fix: update transaction controllers to use selected account #4244
fix: update transaction controllers to use selected account #4244
Conversation
@metamaskbot publish-preview |
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions.
|
0e5df33
to
b874e4c
Compare
packages/transaction-controller/src/helpers/IncomingTransactionHelper.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Charly Chevalier <charly.chevalier@consensys.net>
@@ -1035,7 +1036,7 @@ export class TransactionController extends BaseController< | |||
if (origin) { | |||
await validateTransactionOrigin( | |||
await this.getPermittedAccounts(origin), | |||
this.getSelectedAddress(), | |||
this.getSelectedAccount().address, |
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.
It looks like this controller takes a function that allows for getting the currently selected account. However, this is an old pattern and we are slowly trying to move all controllers to use the messenger pattern for intercommunication.
Since you're making a breaking change anyway, what are your thoughts on extending the TransactionControllerMessenger to allow the AccountsController:getSelectedAccount
action? You can do this by importing AccountsControllerGetSelectedAccountAction
from @metamask/accounts-controller
and adding it to AllowedActions
. Then you can remove the getSelectedAccount
option from the constructor altogether and replace this line with:
this.getSelectedAccount().address, | |
this.messagingSystem.call('AccountsController:getSelectedAccount').address, |
@@ -52,6 +52,7 @@ | |||
"@metamask/controller-utils": "^11.0.0", | |||
"@metamask/eth-query": "^4.0.0", | |||
"@metamask/gas-fee-controller": "^17.0.0", | |||
"@metamask/keyring-api": "6.4.0", |
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.
Noting that this is a type-only import and not under peerDependencies
, this is a something that could be moved to devDependencies
?
(If strict compatibility with the imported class is required, that indicates to me that it could also be a candidate for peerDependencies
, if introducing that requirement here is indeed intended)
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.
Looking at the emitted type declaration file, it looks like the @metamask/keyring-api
shows up there, so I believe this needs to be in dependencies
, not devDependencies
or peerDependencies
so that it doesn't create a TypeScript error on build. Right?
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.
Unrelated, it looks like this is missing a ^
:
"@metamask/keyring-api": "6.4.0", | |
"@metamask/keyring-api": "^6.4.0", |
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.
@mcmire I may be missing something but it seems to me that if we just consider the peerDependencies
aspect in isolation, it should also be here.
And if the peerDependencies
is satisifed, then there should be no TypeScript error on build, and therefore no need for dependencies
since the types will be provided by the peerDependencies
entry?
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.
I'm thinking if it's not added in peerDependencies
, using code may run the risk of getting inconsistent type-checking results or plain incompatibilities, due to using the wrong version of @metamask/keyring-api
for its type-checking?
Specifically here, it would be wrt the getSelectedAccount
constructor option.
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.
I've removed the callback and used the messaging system instead to get the selected account. @metamask/keyring-api
can just be a dev dependency for the tests.
…use-selectedAccountId
packages/transaction-controller/src/TransactionControllerIntegration.test.ts
Outdated
Show resolved
Hide resolved
packages/transaction-controller/src/TransactionControllerIntegration.test.ts
Outdated
Show resolved
Hide resolved
…use-selectedAccountId
…use-selectedAccountId
@@ -99,6 +143,11 @@ const setupController = async ( | |||
givenOptions: Partial< | |||
ConstructorParameters<typeof TransactionController>[0] | |||
> = {}, | |||
mockData: { |
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.
Nit: We named it mocks
in our previous PRs! So we could rename it if you want
…use-selectedAccountId
This pr updates the transaction controller to use account id from the InternalAccount instead of an address Related to MetaMask/accounts-planning#381 - **BREAKING**: `getSelectedAddress` is replaced with `getSelectedAccount` in the `TransactionController` - **BREAKING**: `getCurrentAccount` returns an `InternalAccount` instead of a `string` in the `IncomingTransactionHelper` - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate Fixes MetaMask/accounts-planning#381 --------- Co-authored-by: Charly Chevalier <charly.chevalier@consensys.net>
This pr updates the transaction controller to use account id from the InternalAccount instead of an address Related to MetaMask/accounts-planning#381 - **BREAKING**: `getSelectedAddress` is replaced with `getSelectedAccount` in the `TransactionController` - **BREAKING**: `getCurrentAccount` returns an `InternalAccount` instead of a `string` in the `IncomingTransactionHelper` - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate Fixes MetaMask/accounts-planning#381 --------- Co-authored-by: Charly Chevalier <charly.chevalier@consensys.net>
This pr updates the transaction controller to use account id from the InternalAccount instead of an address Related to MetaMask/accounts-planning#381 - **BREAKING**: `getSelectedAddress` is replaced with `getSelectedAccount` in the `TransactionController` - **BREAKING**: `getCurrentAccount` returns an `InternalAccount` instead of a `string` in the `IncomingTransactionHelper` - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate Fixes MetaMask/accounts-planning#381 --------- Co-authored-by: Charly Chevalier <charly.chevalier@consensys.net>
The `@metamask/transaction-controller` package has been updated from v32 to v34. The `@metamask/user-operation-controller` package had to be bumped as well to avoid new peer dependency warnings. Primarily the only breaking changes in these ranges were peer dependency updates and an update to the minimum supported Node.js version. The new peer dependencies did not introduce any new warnings, they are all met. This update has resolved some pre-existing peer dependency warnings. The only functional change required was this update to the TransactionController in v33.0.0: ``` - **BREAKING:** The `TransactionController` messenger must now allow the `AccountsController:getSelectedAccount` action ([#4244](MetaMask/core#4244)) ... - **BREAKING:** Remove `getSelectedAddress` option from `TransactionController` ([#4244](MetaMask/core#4244)) - The AccountsController is used to get the currently selected address automatically. ``` That change has been applied. See the full changelogs here: * [`@metamask/transaction-controller`](https://github.com/MetaMask/core/blob/%40metamask/transaction-controller%4034.0.0/packages/transaction-controller/CHANGELOG.md) * [`@metamask/user-operation-controller`](https://github.com/MetaMask/core/blob/%40metamask/user-operation-controller%4013.0.0/packages/user-operation-controller/CHANGELOG.md) This helps unblock #9372
## **Description** The `@metamask/transaction-controller` package has been updated from v32 to v34. The `@metamask/user-operation-controller` package had to be bumped as well to avoid new peer dependency warnings. Primarily the only breaking changes in these ranges were peer dependency updates and an update to the minimum supported Node.js version. The new peer dependencies did not introduce any new warnings, they are all met. This update has resolved some pre-existing peer dependency warnings. The only functional change required was this update to the TransactionController in v33.0.0: ``` - **BREAKING:** The `TransactionController` messenger must now allow the `AccountsController:getSelectedAccount` action ([#4244](MetaMask/core#4244)) ... - **BREAKING:** Remove `getSelectedAddress` option from `TransactionController` ([#4244](MetaMask/core#4244)) - The AccountsController is used to get the currently selected address automatically. ``` That change has been applied. See the full changelogs here: * [`@metamask/transaction-controller`](https://github.com/MetaMask/core/blob/%40metamask/transaction-controller%4034.0.0/packages/transaction-controller/CHANGELOG.md) * [`@metamask/user-operation-controller`](https://github.com/MetaMask/core/blob/%40metamask/user-operation-controller%4013.0.0/packages/user-operation-controller/CHANGELOG.md) [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26124?quickstart=1) ## **Related issues** This helps unblock #9372 ## **Manual testing steps** N/A ## **Screenshots/Recordings** N/A ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.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-extension/blob/develop/.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. --------- Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com>
Explanation
This pr updates the transaction controller to use account id from the InternalAccount instead of an address
References
Related to https://github.com/MetaMask/accounts-planning/issues/381
Changelog
@metamask/transaction-controller
getSelectedAddress
is replaced withgetSelectedAccount
in theTransactionController
getCurrentAccount
returns anInternalAccount
instead of astring
in theIncomingTransactionHelper
Checklist
Fixes https://github.com/MetaMask/accounts-planning/issues/381