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

feat!: create new vault with any keyring #329

Merged
merged 5 commits into from
Jan 15, 2024
Merged

Conversation

gantunesr
Copy link
Member

@gantunesr gantunesr commented Jan 4, 2024

Description

This change updates the logic inside the eth-keyring-controller to allow the creation of a vault with any keyring. This removes the hardcoded HD Keyring from the code and allow the client to pass the parameters to instantiate any keyring that is accepted by the controller.

Changes

  • BREAKING: Unify createNewVaultAndKeychain and createNewVaultAndRestore into new method createNewVaultWithKeyring. createNewVaultWithKeyring accepts a password and a keyring object provided by the client and returns the KeyringControllerState

References

No references

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation for new or updated code as appropriate (note: this will usually be JSDoc)
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@gantunesr gantunesr added the team-accounts This should be handled by the Accounts Team label Jan 4, 2024
@gantunesr gantunesr self-assigned this Jan 4, 2024
@gantunesr gantunesr marked this pull request as ready for review January 4, 2024 23:04
@gantunesr gantunesr requested a review from a team as a code owner January 4, 2024 23:04
password: string,
seedPhrase: Uint8Array | string | number[],
keyring: {
type: string;
Copy link

@owencraston owencraston Jan 5, 2024

Choose a reason for hiding this comment

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

is there now better way we can type the options here? I see in the tests we are using KeyringType?

Choose a reason for hiding this comment

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

should we default to HD if one is not passed in?

Copy link
Member Author

@gantunesr gantunesr Jan 5, 2024

Choose a reason for hiding this comment

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

is there now better way we can type the options here? I see in the tests we are using KeyringType?

No, some keyrings are passed from the outside (QRKeyring, LedgerLeyring, SnapKeyring, etc) and there isn't a way to know what they would be unless we enforce it and I'm not sure if that's the right decision since other projects can be using this package with keyrings that are unknown to us, for the moment having them as string and unknown and letting the keyring itself handle it is the best choice.

should we default to HD if one is not passed in?

I don't see the benefit of it at least inside this package, the clients should handle it

@legobeat
Copy link
Contributor

legobeat commented Jan 6, 2024

@gantunesr Do you think we can get in #314 and release #304 prior to merging this? Seems desirable to sync the branches and get the fixes out separately from this breaking feature?

Copy link
Member

@mikesposito mikesposito 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!

Question: will this be done in @metamask/keyring-controller too?

@gantunesr
Copy link
Member Author

@mikesposito Yes! I'll have this done in @metamask/keyring-controller too

@legobeat legobeat force-pushed the feat/new-vault-and-keyring branch from 2a19483 to 8db383b Compare January 15, 2024 11:09
@gantunesr gantunesr merged commit aa68888 into main Jan 15, 2024
17 checks passed
@gantunesr gantunesr deleted the feat/new-vault-and-keyring branch January 15, 2024 13:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
team-accounts This should be handled by the Accounts Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants