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

Update bip39 implementation to @metamask/scure-bip39 #101

Merged
merged 4 commits into from
Jan 6, 2023

Conversation

adonesky1
Copy link
Contributor

@adonesky1 adonesky1 commented Jan 4, 2023

We are updating all instances of bip39 across our packages to use the @metamask/scure-bip39 (our fork of @scure/bip39) - which allows us to pass around SRP's as Uint8Arrays rather than plain strings for security purposes.

Currently this change may also be blocking: MetaMask/metamask-extension#17056

@adonesky1 adonesky1 requested a review from a team as a code owner January 4, 2023 22:47
@adonesky1 adonesky1 changed the title Update bip-39 implementation to @metamask/scure-bip39 Update bip39 implementation to @metamask/scure-bip39 Jan 4, 2023
Copy link
Member

@FrederikBolding FrederikBolding left a comment

Choose a reason for hiding this comment

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

The internals of this library will still expect the SRP to be passed as a string. We will need to make further changes to eliminate that assumption.

cc @Mrtenz

package.json Outdated Show resolved Hide resolved
src/derivers/bip39.ts Outdated Show resolved Hide resolved
Gudahtt
Gudahtt previously approved these changes Jan 5, 2023
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!

@Gudahtt
Copy link
Member

Gudahtt commented Jan 5, 2023

Could you elaborate on why this might be blocking that extension PR? I'm having trouble finding a connection.

@adonesky1 adonesky1 marked this pull request as draft January 5, 2023 22:08
@adonesky1
Copy link
Contributor Author

Not going to merge this yet @Gudahtt, @FrederikBolding has indicated that there may be some other adaptations required to make sure this is not breaking. I'm meeting with @Mrtenz tomorrow to discuss.

@adonesky1
Copy link
Contributor Author

Could you elaborate on why this might be blocking that extension PR? I'm having trouble finding a connection.

I'm not sure if this is going to unblock that but it may be related. The error thrown here in the bip32 test-snap e2e test, is throw here in key-tree. This is happening because the changed bip39 implementation being pulled in in the latest eth-keyring-controller changes are causing the seedphrase to be passed as a Uint8Array rather than a plain text string as is expected in validatePathSegment.

@adonesky1 adonesky1 marked this pull request as ready for review January 6, 2023 17:55
@adonesky1 adonesky1 force-pushed the update-bip39-implementation branch from b4538a5 to 118d884 Compare January 6, 2023 17:55
@adonesky1 adonesky1 merged commit 3a35a8d into main Jan 6, 2023
@adonesky1 adonesky1 deleted the update-bip39-implementation branch January 6, 2023 20:54
@adonesky1 adonesky1 mentioned this pull request Jan 9, 2023
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.

3 participants