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.

Sign up for GitHub

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

Jump to bottom

fix: throw explicit error when KeyringController is locked #5172

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix: throw explicit error when KeyringController is locked #5172

wants to merge 8 commits into from

Conversation

mikesposito
Copy link
Member

@mikesposito mikesposito commented Jan 17, 2025 •
edited
Loading

Blocked by:

Explanation

No specific error is thrown when an operation that requires an unlocked vault is attempted while KeyringController.state.isUnlocked is false. This doesn't make the operations possible, but it doesn't give a clear error message either.

This PR adds an assertion on almost all KeyringController methods to check if the controller is unlocked and to throw an error when it isn't.

References

Changelog

@metamask/keyring-controller

  • CHANGED: A specific error message is thrown when any operation is attempted while the controller is locked

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

@mikesposito mikesposito requested a review from a team as a code owner January 17, 2025 12:12
@mikesposito mikesposito marked this pull request as draft January 17, 2025 12:29
@mikesposito mikesposito force-pushed the fix/assert-controller-unlocked branch from 5834c49 to 5a86062 Compare January 17, 2025 13:40
Base automatically changed from mikesposito/lint-fix to main February 3, 2025 10:23
@mikesposito mikesposito force-pushed the fix/assert-controller-unlocked branch from b8a66bc to c60cc5a Compare February 3, 2025 11:01
@mikesposito mikesposito requested a review from a team February 3, 2025 11:02
@mikesposito mikesposito marked this pull request as ready for review February 3, 2025 11:08
mcmire
mcmire reviewed Feb 6, 2025
View reviewed changes
Copy link
Contributor

@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.

@mikesposito What are your thoughts on adding tests for all the cases which we need to assert that the controller is locked? I know there are a lot of methods you're changing, and also there are more changes planned, so not sure if this is unblocking some more work later or if much of this code will disappear later anyway or what.

@mcmire
Copy link
Contributor

mcmire commented Feb 6, 2025

Regardless of the above comment, I reviewed this and it looks like the controller does not need to be in unlocked state to call these methods:

  • createNewVaultAndRestore
  • createNewVaultAndKeychain
  • verifyPassword
  • isUnlocked
  • exportSeedPhrase
  • exportAccount
  • submitEncryptionKey
  • submitPassword

Is that right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcmire mcmire mcmire left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
team-wallet-framework
Projects
PR review queue
Status: Needs dev review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[KeyringController] No specific error is thrown when the controller is locked
2 participants
@mikesposito @mcmire