Skip to content
This repository has been archived by the owner on Oct 7, 2024. It is now read-only.

patch serialize and _initFromMnemonic to account for instances where mnemonic could be buffer array or string #59

Merged
merged 4 commits into from
Apr 14, 2022

Conversation

adonesky1
Copy link
Contributor

Adds patch of this package currently implemented extension side to this repo.

@adonesky1 adonesky1 requested a review from a team as a code owner April 4, 2022 18:47
@adonesky1 adonesky1 requested a review from Gudahtt April 4, 2022 18:47
Copy link

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Thanks for bringing this over. I think we'd want to update the unit tests before merging this.

index.js Show resolved Hide resolved
mcmire
mcmire previously approved these changes Apr 13, 2022
Copy link

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Looks good. The tests could be improved to be more precise, but that's not your fault — I think we can fix that later when we convert them to Jest.

index.js Outdated
* BIP39-compliant mnemonic.
*
* @param {string|Array<number>|Buffer} mnemonic - A seed phrase represented
* as a string, an array of UTF-8 bytes, or a Buffer.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe worth emphasizing here that the array and Buffer types are assumed to be NFKD-normalized: https://github.com/MetaMask/bip39/blob/master/ts_src/index.ts#L72-L73

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comment added here: 446ba4b

@Gudahtt
Copy link
Member

Gudahtt commented Apr 13, 2022

The .DS_Store file should be removed

@adonesky1
Copy link
Contributor Author

The .DS_Store file should be removed

done here

@adonesky1 adonesky1 requested review from Gudahtt and mcmire April 13, 2022 18:52
index.js Outdated Show resolved Hide resolved
@adonesky1 adonesky1 requested a review from Gudahtt April 13, 2022 21:24
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 e7e5ff8 into main Apr 14, 2022
@adonesky1 adonesky1 deleted the add-old-patch branch April 14, 2022 14:33
@mcmire mcmire mentioned this pull request Apr 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants