Use @metamask/keyring-controller
#19659
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
|
No top level dependency changes detected. Learn more about Socket for GitHub ↗︎ |
Codecov Report
@@ Coverage Diff @@
## develop #19659 +/- ##
===========================================
- Coverage 69.36% 69.36% -0.00%
===========================================
Files 987 987
Lines 37289 37292 +3
Branches 10011 10011
===========================================
+ Hits 25865 25867 +2
- Misses 11424 11425 +1
|
Builds ready [ed4efdb]
Page Load Metrics (1663 ± 65 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
mikesposito
force-pushed
the
refactor/use-core-keyring-controller
branch
from
ed4efdb to
8d67d4e
Compare
|
Uhm, it looks like we are adding ~150kb to the background bundle, which isn't great. From the bundle visualizer, looks like
|
Builds ready [8d67d4e]
Page Load Metrics (1950 ± 37 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
mikesposito
force-pushed
the
refactor/use-core-keyring-controller
branch
3 times, most recently
from
40b1e8b to
a73071d
Compare
|
@metamaskbot update-policies |
|
Policies updated |
Builds ready [6d13c99]
Page Load Metrics (1605 ± 75 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
mikesposito
force-pushed
the
refactor/use-core-keyring-controller
branch
2 times, most recently
from
c65c6b9 to
2d584d7
Compare
|
In the extension we still use |
Builds ready [2d584d7]
Page Load Metrics (1561 ± 33 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
|
@metamaskbot update-policies |
|
No policy changes |
Builds ready [a052202]
Page Load Metrics (1705 ± 83 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
mikesposito
force-pushed
the
refactor/use-core-keyring-controller
branch
from
a052202 to
95313e3
Compare
Builds ready [95313e3]
Page Load Metrics (1588 ± 45 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
mikesposito
force-pushed
the
refactor/use-core-keyring-controller
branch
from
95313e3 to
1567ae0
Compare
|
@metamaskbot update-policies |
|
Policies updated |
Builds ready [608b911]
Page Load Metrics (1503 ± 42 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
| @@ -1,10 +1,11 @@ | ||
| import type { TxData, TypedTransaction } from '@ethereumjs/tx'; | ||
| -import { type MetaMaskKeyring as QRKeyring, type IKeyringState as IQRKeyringState } from '@keystonehq/metamask-airgapped-keyring'; | ||
| +import type { MetaMaskKeyring as QRKeyring, IKeyringState as IQRKeyringState } from '@keystonehq/metamask-airgapped-keyring'; |
There was a problem hiding this comment.
Curious why these type imports were changed. Was this unintended? Or did the import { type ___ } syntax cause problems with the build system?
There was a problem hiding this comment.
It is intended, but it looks like it breaks only the lint step: https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/51596/workflows/800ab01e-5b6f-44c1-9312-934fdd12c838/jobs/1501250
There was a problem hiding this comment.
Ah I see. The TypeScript version used here is fairly old, that's probably why.
There was a problem hiding this comment.
Hmm. Should we add TypeScript ~4.6.3 as an optional peer dependency to all packages in the core repo?
There was a problem hiding this comment.
Maybe >=4.6.3 instead? But yes that might be a good idea
| syncIdentities: this.preferencesController.syncAddresses.bind( | ||
| this.preferencesController, | ||
| ), | ||
| updateIdentities: this.preferencesController.setAddresses.bind( |
There was a problem hiding this comment.
I noticed that setAddresses isn't quite the equivalent of updateIdentities. updateIdentities will update selectedAddress if the referenced account is removed, but setAddresses will not do that.
There was a problem hiding this comment.
We can handle this in a later PR, but it will come up once we start using the core keyring controller for creating accounts and keyrings
There was a problem hiding this comment.
we can do this as part of #18772
Builds ready [16e373d]
Page Load Metrics (1596 ± 48 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
| @@ -1,10 +1,11 @@ | ||
| import type { TxData, TypedTransaction } from '@ethereumjs/tx'; | ||
| -import { type MetaMaskKeyring as QRKeyring, type IKeyringState as IQRKeyringState } from '@keystonehq/metamask-airgapped-keyring'; | ||
| +import type { MetaMaskKeyring as QRKeyring, IKeyringState as IQRKeyringState } from '@keystonehq/metamask-airgapped-keyring'; |
There was a problem hiding this comment.
Hmm. Should we add TypeScript ~4.6.3 as an optional peer dependency to all packages in the core repo?
metamaskbot
added
the
release-10.36.0
Builds ready [9530eed]
Page Load Metrics (1601 ± 41 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Gudahtt
added
release-11.1.0
Explanation
This PR enables a smooth transition from
@metamask/eth-keyring-controllerto@metamask/keyring-controller.In order to replace all interactions between the extension client and the
EthKeyringControllerwithKeyringControllerin separate PRs we need to be able to start usingKeyringController, while being able to keep direct interactions withEthKeyringControllerpossible.As the instance of
EthKeyringControllermanaged byKeyringControlleris inaccessible, we need a temporary patch to add agetEthKeyringControllermethod onKeyringControllerso that we can keep usingEthKeyringControllerinMetamaskController.Manual Testing Steps
Nothing should be changed in terms of functionality.
Pre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Boardlabel.In this case, a QA Engineer approval will be be required.