Skip to content
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

Bump eth-keyring-controller version to @metamask/eth-keyring-controller v10 #1072

Merged
merged 5 commits into from
Jan 24, 2023

Conversation

adonesky1
Copy link
Contributor

@adonesky1 adonesky1 commented Jan 20, 2023

Fixes: #774 (not an initial target of this work)

Pulls in latest version - v10.0.0 - of eth-keyring-controller (now @metamask/eth-keyring-controller) and adapts according.

BREAKING: exportSeedPhrase now returns a Uint8Array typed SRP (can be converted to a string using this approach). It was previously a Buffer.

@adonesky1 adonesky1 force-pushed the bump-eth-keyring-controller-version branch from 069d401 to 67b8f76 Compare January 23, 2023 19:52
@adonesky1 adonesky1 changed the title bump eth-keyring-controller version to @metamask/eth-keyring-controll… Bump eth-keyring-controller version to @metamask/eth-keyring-controller v10 Jan 23, 2023
@adonesky1 adonesky1 marked this pull request as ready for review January 23, 2023 19:58
@adonesky1 adonesky1 requested a review from a team as a code owner January 23, 2023 19:58
const currentState = await keyringController.createNewVaultAndRestore(
password,
currentSeedWord.toString(),
currentSeedWord,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currentSeedWord is now exported as a Uint8Array and createNewVaultAndRestore can receive it in this form (or still as a string as well).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so this would be a breaking change then. Pretty sneaky, since there is no change on that line because the return type is implicit.

Do you know of any other examples of breaking changes? It would be great to have those listed in the PR description.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know of any other examples of breaking changes?

🤔 I'll poke around a bit more and see if I can find others!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading through this I'm not detecting any other breaking changes. But good catch for sure. We will definitely want very thorough QA on both products before these KeyringController updates are pushed to prod.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this would also include any breaking changes to eth-hd-keyring introduced in v5 and v6. Prior to this PR, v4 was used.

The keyring itself is part of the public API, since it's directly in the controller state.

Comment on lines +55 to +60
const additionalKeyringBuilders = additionalKeyrings.map((keyringType) =>
keyringBuilderFactory(keyringType),
);
const baseConfig: Partial<KeyringConfig> = {
encryptor: new MockEncryptor(),
keyringTypes: additionalKeyrings,
keyringBuilders: additionalKeyringBuilders,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relevant changelog entry in @metamask/eth-keyring-controller: https://github.com/MetaMask/KeyringController/blame/main/CHANGELOG.md#L22

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@adonesky1 adonesky1 merged commit 604a90a into main Jan 24, 2023
@adonesky1 adonesky1 deleted the bump-eth-keyring-controller-version branch January 24, 2023 21:02
gantunesr pushed a commit that referenced this pull request Jan 25, 2023
…er v10 (#1072)

Fixes: #774  (not an initial target of this work)

Pulls in latest version - v10.0.0 - of `eth-keyring-controller` (now
`@metamask/eth-keyring-controller`) and adapts according.

**BREAKING:** `exportSeedPhrase` now returns a `Uint8Array` typed SRP
(can be converted to a string using [this
approach](https://github.com/MetaMask/eth-hd-keyring/blob/main/index.js#L40)).
It was previously a Buffer.
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
…er v10 (#1072)

Fixes: #774  (not an initial target of this work)

Pulls in latest version - v10.0.0 - of `eth-keyring-controller` (now
`@metamask/eth-keyring-controller`) and adapts according.

**BREAKING:** `exportSeedPhrase` now returns a `Uint8Array` typed SRP
(can be converted to a string using [this
approach](https://github.com/MetaMask/eth-hd-keyring/blob/main/index.js#L40)).
It was previously a Buffer.
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
…er v10 (#1072)

Fixes: #774  (not an initial target of this work)

Pulls in latest version - v10.0.0 - of `eth-keyring-controller` (now
`@metamask/eth-keyring-controller`) and adapts according.

**BREAKING:** `exportSeedPhrase` now returns a `Uint8Array` typed SRP
(can be converted to a string using [this
approach](https://github.com/MetaMask/eth-hd-keyring/blob/main/index.js#L40)).
It was previously a Buffer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please chage import name 'Keyring' to another.
2 participants